[IGSTK-Developers] Make SM even safer? Request Validation

[David Gobbi]

Hi Patrick,

I see what you are getting at: a single Request() method plus
an enum could be used instead of

  void RequestSetPatientName();
  void RequestLoadImage();
  void RequestSetImageLandmarks();
  etc.

A "case" statement isn't necessary, though, all that is
needed is a std::map to convert an enum value into state
machine input, and then you can push the input:

void Request( RequestType request )
{
  // m_RequestToInputMap is a std::map object
  const InputType & input = m_RequestToInputMap[request];
  
  m_StateMachine.PushInput(input);
  m_StateMachine.ProcessInputs();
}

Right now you are probably looking at the this Request
function and wondering... where is the validation?

The answer: the state machine already does the validation,
and adding a RequestValidation() method doesn't make it
any safer.

I can see an advantage to your RequestValidation(), though: it
can be used to set a default behaviour for if the request is
invalid.  You can either make it so that an invalid request will
just "return", or you can make it so that an invalid request will
always cause an error.

Here is what I think we should do, though: instead of having a
RequestValidation() method, we should do something like what you
suggested at the T-con, and add a method to igstk::StateMachine
to define a default action for an input:

  void StateMachine::SetDefaultAction(const InputType &, const ActionType &);

That way, you can either set "NoAction" as the default, or you
can provide a default error handler for states where the input is
not valid.

Does this make sense, or did I make a mistake when I was reading
your email?

 - David


--- Patrick Cheng <cheng at isis.georgetown.edu> wrote:

> Hi all,
> 
> As you all know, for SM warped class, we are using the request methods 
> in the API to talk or command the SM. The request method is basically 
> generating some inputs to the SM, and let the SM do the remaining job, 
> which is pretty safe.
> 
> But I think, every time before we generating all the inputs and push 
> them back to the SM, we should do a request validation check, to see if 
> current state is good for this kind of request or not.
> 
> My reason is: For some complex state machine, ( eg. SM for 
> LandmarkRegistration and SM for my demo applications), there are 
> multiple inputs and request, and there are certain call sequence for the 
> request methods to make things happen.  In the application, some of the 
> inputs are taken from the user inputs, like a confirm dialog or open a 
> file chooser. But I don’t want the application to take those actions if 
> this request is invalid for right now. Say, trying to load an image, but 
> the patient hasn’t been registered.
> 
> So I propose add a request validation to the state machine. I have 
> implemented in the application class. In the 
> Sandbox/Example/FourViewsTrackingWithCT
> 
> Basically, at the beginning of all the request method, I will do a 
> requestvalidation check, if it’s invalid, it will return. The data 
> structure is pretty simple. I add an enumeration of all the request 
> methods in head file. And create a vector with all valid states that can 
> perform this kind of request for each request method. Those are being 
> done at the contractor, below the state machine initialization. 
> Currently I didn’t implement any action for invalid request, but that 
> could be easily added.
> 
> .h--------------------------------
> typedef enum
>      {
>      SetPatientNameRequest,
>      LoadImageRequest			// There are some other requests
>      } RequestType;
> 
>    const static int NumberOfRequest = 9;
> 
>    typedef std::vector < StateMachineType::StateIdentifierType > 
> RequestToStateMapType;
> 
>    std::vector < RequestToStateMapType > m_RequestValidationMap;
> 
>    bool RequestValidation( RequestType request );
> -----------------------------------
> 
> .cxx------------------------------
> 
> In constructor
> RequestToStateMapType rtsm;
>    for( unsigned int i = 0; i < NumberOfRequest; i++)
>      m_RequestValidationMap.push_back( rtsm );
> 
>    rtsm.clear();
>    rtsm.push_back( m_InitialState.GetIdentifier() );
>    rtsm.push_back( m_PatientNameReadyState.GetIdentifier() );
>    m_RequestValidationMap[ SetPatientNameRequest ] = rtsm;
>    rtsm.clear();
>    rtsm.push_back( m_PatientNameReadyState.GetIdentifier() );
>    rtsm.push_back( m_ImageReadyState.GetIdentifier() );
>    rtsm.push_back( m_PatientNameVerifiedState.GetIdentifier() );
>    m_RequestValidationMap[ LoadImageRequest ] = rtsm;
> 
> bool FourViewsTrackingWithCT::RequestValidation( 
> FourViewsTrackingWithCT::RequestType request)
> {
>    RequestToStateMapType::iterator it;
>    for ( it = m_RequestValidationMap[request].begin(); it 
> !=m_RequestValidationMap[request].end(); it++ )
>    {
>      if ( *it == m_StateMachine.GetCurrentStateIdentifier() )
>        return true;
>    }
>    return false;
> }
> 
> In request method add:
> void FourViewsTrackingWithCT::RequestLoadImage()
> {
>    if( !RequestValidation( LoadImageRequest ) ) return;
> 










.
> }
> 
> 
> 
> So I think this should work, and pretty simple.
> 
> Further more, we can even expose the request enum in API, and only put 
> a generic Request( RequestType request) method in API for all the class.
> 
> If the implementation of that Request method. We can do sth like
> 
> Request( RequestType request)
> {
>    If( ! RequestValidation( request ) ) return;     // we could also add 
> some invalid request action before return.
> 
> Case request
> 


. Call the actual Request Method.
> }
> 
> Thanks for reading this long email.
> Please let me know your thoughts
> 
> Patrick
> 
> -- 
> Peng (Patrick) Cheng
> Research Engineer
> cheng at isis.georgetown.edu
> 
> Imaging Science and Information Systems (ISIS) Center
> Department of Radiology, Georgetown University Medical Center
> 2115 Wisconsin Avenue, Suite 603
> Washington, DC, 20007
> 
> Work phone: 202-687-2902
> Work fax:   202-784-3479
> Cell phone: 202-674-5626
> _______________________________________________
> IGSTK-Developers mailing list
> IGSTK-Developers at public.kitware.com
> http://public.kitware.com/cgi-bin/mailman/listinfo/igstk-developers
>