[IGSTK-Developers] Re: State Machine for ObjectRepresentation class

[Luis Ibanez]

Hi Patrick,

Testing classes in an "application" is far from ideal.

The code coverage that we get in an application is rather
small, because a human user in a typical use of a demo
application will trigger a small set of transitions in
the state machine of the classes.

Applications are not run nightly nor continuously, and
therefore are not appropriate as components of a test-suite.

Our official testing base must consist of the files in
the testing directory, and must pursue 100% code coverage.

---

The need for AND states is usually not visible at first sight.

The fact that the TimeStamp is a member-variable property
of the SpatialObject, doesn't reduce its relevance for the
SpatialObjectRepresentation.



The fundamental point here is the following:


    The Representation class is responsible for conveying
    information to the surgeon. This is the class that
    shows where in space an object is placed in the scene
    of the surgery. The functionality of flagging when this
    location information is not reliable, is essential for
    providing safety to the patient.



The reason why the AND states are needed is that this class
has three orthogonal cycles:

   1) The cycle of updating positions and orientation
   2) The cycle of updating representation (e.g. geometry)
   3) The cycle of requesting/receiving timestamps

By "orthogonal" we mean that each one of the cycles happens
independently of the other, and that they can become intertwined.

The class needs the States for each one of the cycles, but
given that the cycles may becomes intertwined, the real state
of the class is given by the combination (*and*) of is state
on each of the cycles.  Therefore the need for "And" states.

The "ValidTimeStamp"/"InvalidTimeStamp" inputs and states,
are the ones managing cycle #3.  Probably the problem here
is rather a matter of the naming, instead of a problem with
the state machine itself.  These cycle is the one that manages
the following protocol:


   a) The View class schedules a screen refresh.

   b) It informs all its registered representation objects
      to prepare themselves to present their appropriate
      representation for that scheduled time.

   c) The representation classes check with their associated
      spatial objects on whether their Transform are still
      valid. This requires a Request message sent to the
      Spatial object, and a cycle of waiting for receiving
      an answer from the Spatial object. This is converted to
      input is one of ("ValidTimeStamp" or "InvalidTimeStamp").



A more fortunate choosing may have been an "itk-style" name
such as:


        m_MyAssociatedSpatialObjectHasAValidTimeStamp
        m_MyAssociatedSpatialObjectHasAnInValidTimeStamp


Where it is more explicit what we mean.



In general, we should be very careful on removing states
and inputs from State Machines. Such task should be done
only on the basis of having tests that cover not only the
100% code coverage of the class, but also 100% of arcs in
the graph of the state diagram.   Without such support,
it is very likely that we will be amputating functionality
that is needed, and that we will only notice it when it is
too late for the patient.




Please let me know if you still have concerns about the
design and implementation of this class.



    Thanks



      Luis




-------------------
Patrick Cheng wrote:
> Hi Luis,
> 
> This is being tested in the "application" for both tracked and 
> non-tracked object, and it meets the function requirements. We should 
> also modify the tests to reflect this change.
> 
> The reason I think those states are unnecessary is: we mixed the 
> 'inputs' and 'states' here.
> We receive a 'ValidTimeStampInput', we get into 'VisibleState'
> We receive a 'InvalidTimeStampInput', we get into 'InvisibleState'
> 
> I don't see the reason of using a 'AND-States' here.
> 
> Patrick
> 
> Luis Ibanez wrote:
> 
>>
>>
>> Hi Patrick,
>>
>> I understand your desire for simplifying this class,
>> and it seems to be a good idea to do so, *as long as*
>> the class is still working for *all* the use cases that
>> it is designed to satisfy.
>>
>>
>> When you say that:
>>
>>
>>   "after removing the states, the class still works"
>>
>>
>> Do you mean that you run exhaustive testing in this class ?
>>
>>
>> or simply that your application is still running after
>> you remove the states ?
>>
>>
>>
>> In particular:
>>
>>
>> 1) Did you test that the objects still become invisible
>>    when their timestamps expire ?
>>
>>
>> 2) Did you test that the objects still become visible again
>>    when they get updated (non-expired) timestamps ?
>>
>>
>> 3) Did you run both tests for "tracked" and "non-tracked"
>>    objects ?
>>
>>
>>
>> The State Machine of this class may look unfamiliar because it
>> is using the concept of AND-States, that we discussed during
>> the annual review meeting. The states that you propose to
>> remove are representing the cases when the position of a
>> Spatial Object is updated, and it is in an visible or invisible
>> states. You should test this cases with a tracked object (an
>> object being attached to an igstk tracker).
>>
>>
>> The functionality of Visibility/Invisibility is fundamental
>> for the toolkit, and altering it represents a serious risk
>> to the patient (since we may end up displayin objects in
>> positions whose timestamps have expired. In other words,
>> we could tell the surgeon that an object is in a particular
>> position, when the information about that position happens
>> to be outdated, and the object is no longer there.)
>>
>>
>>
>>
>> Please let me know what tests you ran after removing the
>> states from this class, that lead you to conclude that
>> the states are unnecessary.
>>
>>
>>
>>
>>    Thanks
>>
>>
>>
>>       Luis
>>
>>
>>
>>
>> --------------------
>> Patrick Cheng wrote:
>>
>>> Hi Luis,
>>>
>>> I think the state machine of this class it too complicated, and 
>>> causing the class to stuck in some stat won't be able to get out.
>>>
>>> This is the change I made for the demo.
>>>
>>> I took these states out:
>>>   //igstkAddStateMacro( ValidTimeStamp );
>>>   //igstkAddStateMacro( InvalidTimeStamp );
>>>   //igstkAddStateMacro( AttemptingUpdatePositionAndVisible );
>>>   //igstkAddStateMacro( AttemptingUpdatePositionAndInvisible );
>>>
>>> My reason is:
>>> 1. TimeStamp is a property of SpatialObejct, not the Representation 
>>> class.
>>> 2. Only the visibility makes sense to the representation class
>>>
>>> When we verify the time stamp in representation class, we translate 
>>> them into Valid/InvalidTimeStampInput, and let them act as a switch 
>>> for the visibility, I think that's enough.
>>>
>>> The state machine is simpler, and it works.
>>>
>>> I know you did some recent refactoring of the class, so I want your 
>>> opinion on this.
>>>
>>> PS: Both version of this class is attached
>>> Patrick
>>>
>>>
>>
>>
>>
>>
>