[boost] RE: ANN: review of FSM library starts

From: Augustus Saunders (infinite_8_monkey_at_[hidden])
Date: 2005-03-02 08:43:07

I hesitate to call this a review; it's more my reactions looking
through the fsm documentation and examples--I have not actually used
it yet.  I feel like these comments should have been given a long
time ago, but I guess I missed all the pre-review discussion.

I'm not an expert in this area, but I feel comfortable with the ideas
and have fooled around with making my own state machines in the past.
 My main interest in a library like fsm lies in correctness and
simplicity, not performance.

Before I delve into answering the "official review questionare," I
want to make some overall comments.  First, it is obvious to me that
this library has a lot of potential.  The authors have thought
through a lot of issues that I've never considered.  My problems
mainly lie with the syntax and documentation, and I think problems
with the latter stem from the former.  I'd like to see some
discussion on the matter.

Overall, I feel the syntax is too burdensome and the organization of
code that I have to write is a hopeless mess (at least, looking at
the examples).  For simple cases, I really want to see a way of
defining a fsm with less line noise and less indirection.  I haven't
thought through the ramifications, but it just seems to me that some
mpl, lambda, and BOOST_TYPEOF loving should make something roughly
akin to the following possible:

{
  using namespace boost::fsm;
  using namespace boost::lambda;

  // I guess there's no way to avoid declaring
  //  these ahead of time??
  class Machine, State1, State2, Event1, Event2;

  struct S1Data
  {
     int i;
     S1Data() : i(0) {}
  };

  typedef simple_state< Machine, S1Data,
      BOOST_TYPEOF( cout << constant("Entering State1.") ),
      BOOST_TYPEOF( cout << constant("Exiting State1.") ),
         on_event<Event1, BOOST_TYPEOF( _1->i++ ) >,
         transition<Event1, State2> >
    State1;
  // the "data" member of State1 has type S1Data, and is 
  //  passed into event handlers

  struct S2Data
  {
     int j;
     S2Data() : j(0) {}
  };

  typedef simple_state< Machine, S2Data,
      BOOST_TYPEOF( cout << constant("Entering State2.") ),
      BOOST_TYPEOF( cout << constant("Exiting State2.") ) >
         on_event<Event2, BOOST_TYPEOF( _1->j++ )>,
         transition<Event2, State1> >
    State2;
  // the "data" member of State2 has type S2Data, and is 
  //  passed into event handlers

  // It seems that in the propsed fsm library,
  //  state composition is specified bottom up,
  //  and I'm not sure what the motivation is.
  //  We're all used to writing classes top-down.
  typedef state_machine< State1, State2 > Machine;

  Machine m;
  m.initiate();

  // process events
  while (whatever)
    m.process_event(blah);

  // I don't quite understand why state_cast is necessary
  //  versus just overloading regular casting.
  cout << "State1 had Event1 called " << ((State1)m).data.i 
       << " times." << endl;
  cout << "State2 had Event2 called " << ((State2)m).data.j 
       << " times." << endl;
}

I'm sure I've messed up some details sine I don't really get why some
of the fsm mechanisms are there.  Also, I'm ignoring for the moment
that member dereferencing is a little uglier with lambda, so _1->i++
would have to be (_1->* &S1Data::i)++.  I assume there exists a way
to make state-specific data available to a lambda expression in a
clean way.  I've also ignored sticking in lots of mpl::vector or
mpl::lists, as it's my impression that with mpl one can iterate
through the template arguments and figure out if they are events or
transitions or whatever.  So I smushed together the different
elements, and presumed for < 10 arguments (or whatever, using
PREPROCESSOR lib to configure that) the mpl::vector could be omitted.
 Finally, even though I've not really tested BOOST_TYPEOF, they use
lambda expressions as an example, so I don't think it's entirely out
of line of think that it might work.

Now, I don't know if this example has really reduced the total number
of characters required to be typed, but it is more clear to me.  I
realize that I've seperated the state data out into a seperate
struct; personally, I like that.  More importantly to me, I can look
at those typedefs and reasonably guess what's being defined.  If you
made FSM_ENTER and FSM_EXIT macros to alias BOOST_TYPEOF you would
make it clear what those entries were for.  In general, you don't
have to keep refering back and forth to the reference manual to
figure out what each entry is.  Looking at the examples in the
tutorial just made my head spin, especially trying to figure out how
states and machines get tied together (this is still unclear to me). 
This kind of "inline" code would be great for lots of cases where you
just want to increment/decrement some counter or push/pop some
variable.  Maybe I'm hoping for too much magic, but I feel that at
least little magic or secret sauce is needed here.

    * What is your evaluation of the documentation?
      How easy (or hard) it is to understand library
      features? What can be improved?

All the other reviewers have said nice things about the
documentation, but I had a lot of trouble with the tutorial.  As
already mentioned by others, totally targetted at the wrong people. 
In general, I would drop all mention of UML except for some appendix
somewhere (and the reference manual too).  "Getting Started" and
"Target Audience" should be in a differenct document, and "How to
read this tutorial" should be removed.  Then, before you get into the
C++ code for "Hello, World!", you should work through "Hello, World!"
and maybe "StopWatch" using pseudo-code.  Even if you can work out
some magic like I hope for, I think you need to map out the basic
process with pseudo-code, like this:

// Hello World example

// First, we have to declare all of our states 
//  and events ahead of time.  This is because
//  all states can transition to each other,
//  so you can't necessarily define them in order.
class mystate1, mystate2, event1, event2, etc;

// Next, we're going to define some states
//  for our machine.  Each state definition
//  indicates what other states it transitions
//  to on each input, buried in "bunch_of_stuff"
typedef simple_state< bunch_of_stuff > mystate;
typedef simple_state< bunch_of_stuff > mystate2;
// etc

// Then, we're going define a machine with
//  states.  The first listed state is the default
typdef state_machine<mystate, mystate2> Machine;

// etc, in this style

In general, all of your "remarks" and "comments" need to be in the
code!  I think the example state machines are basically OK, but I
think that it is absolutely essential that you include a simple
example that is alphabet-based.  In fact, some helper functions for
alphabet-based languages are probably a good idea, just to make the
tutorial easier.  In particular, you should be able to create
letters-as-events, and dispatch them easily.  The switch statement
for dispatch in the Camera example is scandalous ;)

The reference manual seems ok, although I didn't really use it.  The
rationale tended to feel unconvincing, although I'm not well informed
on the issues.

    * What is your evaluation of the design?
      What features are supported better by
      alternative FSMs? What could be added
      (or removed) from the library?

I'm confused about why entering/exiting states causes construction /
destruction.  It's taken as a given, but I would tend to assume that
state-local-storage is maintained throughout unless I explicitly
reset it.  I'm also surprised by the lack of "accept" states.  Maybe
it's not universal, but my theory certainly had them, and it's just
disconcerting for them not to be there.

    * The library documentation contains
      few not yet solved issues (name,
      separating the library into two parts,
      exception handling). What is you opinion here?

boost::fsm is good, IMO.  Don't know about seperating libraries.

    * What is your evaluation of the implementation?
      Are there parts of code too obscure or
      duplicating exiting Boost functionality?
      Can something be factored out to standalone
      library or among general utilities?

Didn't look.

    * Are there performance bottlenecks?
      Does the library design fit requirements
      of real-time systems? How is it useable
      for embedded systems?
      Is the documentation clear on performance
      tradeoffs?

No comment.

    * What is your evaluation of the potential
      usefulness of the library? Can you compare
      this FSM to other implementations?

Tremendous potential.  I can't compare directly to other FSM
libraries/tools, but just like spirit, eliminating the need for extra
tools is a great benefit.  

    * Did you try to use the library?  With what
      compiler? Did you have any problems?
      Do you have tips for better support of older
      compilers? What are possible portability problems?

No.

    * How much effort did you put into your
      evaluation?
      A glance? A quick reading? In-depth study?

Looked through docs and examples, thought carefully about what I
wanted, and poked around to see what might be feasible. Many hours.

    * Are you knowledgeable about the problem domain?

Not an expert, not completely ignorant.

And finally, every reviewer should answer this question:

    * Do you think the library should be accepted as a
      Boost library?  Be sure to say this explicitly so that
      your other comments don't obscure your overall opinion.

I think that boost::fsm will benefit greatly from some further
consideration.  The Serialization library took two tries and was much
improved, to good effect.  I can be accused of hoping for too much
"magic" syntax, but it'll take a lot of convincing that the current
way is best.  I'm not unreasonable, but I haven't seen any discussion
on it, and Spirit, Lambda, Serialization, and others given me high
expectations for usability.

And lastly, thanks to all involved in bringing us boost::fsm. 
Despite my critique, this is first-rate work.

Thanks-
Augustus

        
                
__________________________________ 
Celebrate Yahoo!'s 10th Birthday! 
Yahoo! Netrospective: 100 Moments of the Web 
http://birthday.yahoo.com/netrospective/