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

From: Andreas Huber (ahd6974-spamgroupstrap_at_[hidden])
Date: 2005-03-02 11:40:21

Hi Augustus

[snip]
>  My main interest in a library like fsm lies in correctness and
> simplicity, not performance.

Which also happens to be the focus of this library.

> 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).

Have you ever had a look at other FSM libraries? I think boost::fsm is less 
messy than any other library I've seen.

>  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;

Actually there is, but I found out about this only recently (Peter Dimov 
mentioned this in an unrelated post) and haven't changed the docs because I 
feared that this does not work on all compilers:

Instead of

struct Greeting;
struct Machine : fsm::state_machine< Machine, Greeting > {};

you can write:

struct Machine : fsm::state_machine< Machine, struct Greeting > {};

>   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

You consider this more readable? Sorry, I have to disagree. For entry and exit 
actions and extended state variables I consider my approach superior, for 
reaction definition, your approach seems about equivalent. 

>   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.

I don't understand, you do define states top-down in boost::fsm.

>   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

One sure can but that would duplicate mpl::list<> functionality to a certain 
degree.

> 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.

Given that the approach you outlined above really works, I think you can use 
lambda for transitions without changing a single line of boost::fsm, you 
simply define your own reaction that accepts typeofed lambda expressions 
instead of function pointers for transition actions. I'm not so sure whether 
the same is possible with entry/exit actions but I'm inclined to think that it 
is (that would require deriving your own state class template from 
simple_state).

> 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

Well, it's the other way round for me ;-). Most people get used to the syntax 
of boost::fsm quite quickly. Also, I've received feedback that people really 
like the fact that they can easily put the bodies of complex actions in cpp 
files, I *guess* that would be a little more difficult with your approach.

[snip]
> 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

I don't think there is that much to learn:

state_machine class template:
1. Param: The most-derived subclass
2. Param: Initial state

simple_state, state class templates:
1. Param: The most-derived subclass
2. Param: Context
3. Param: Reactions
4. Param: Inner initial states
5. Param: History

Reactions:
1. Param: Event
2. - 4. Params: transition destination & transition action (only present in 
transitions)

event class template:
1. Param: The most-derived subclass

I'd say it takes one day tops of playing around with boost::fsm to learn this. 
I did consider using named template parameters, but rejected it because it 
would add to the amount of text you have to write and would offer a benefit 
only for beginners.

> tutorial just made my head spin, especially trying to figure out how
> states and machines get tied together (this is still unclear to me). 

I guess you mean how the implementation ties them toghether? As a user, why do 
you need to know that? This might be part of a future implementation 
documentation but in general I think users can live very well without that 
knowledge.

> 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.

As I've hinted throughout the comments above, I think your approach is worse 
than mine for regular users, who IMHO are the most important part in the 
picture.

>     * 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. 

As I already mentioned in another post: Targetting every C++ developer would 
require an *additional* document at least as thick as the current tutorial. I 
think it is reasonable to expect previous exposure to FSMs. I also do give 
links to FSM introductory material. That might not be enough but then again I 
don't think it is my duty as library writer to educate people about FSMs in 
general.

> In general, I would drop all mention of UML except for some appendix
> somewhere (and the reference manual too).

Why?

> "Getting Started" and
> "Target Audience" should be in a differenct document, and

Please make a suggestion.

> "How to
> read this tutorial" should be removed.

Why?

> 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!

You're the second person to request that. I personally think it is better to 
not clutter code with too many comments. If people think it is better to have 
all comments in the code, I'll sure change it.

> 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.

Why?

> 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 ;)

You mean the mapping between the pressed key and the generated event? Why?

> 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,

It is not: http://tinyurl.com/5mjra (State-local storage)

> but I would tend to assume that
> state-local-storage is maintained throughout unless I explicitly
> reset it.

Why?

> 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.

I don't know what accept states are. UML doesn't have them.

[snip]
> And lastly, thanks to all involved in bringing us boost::fsm. 

You're welcome.

> Despite my critique, this is first-rate work.

Thanks, and thanks for your review!

Regards,

-- 
Andreas Huber
When replying by private email, please remove the words spam and trap
from the address shown in the header.