Boost logo

Boost :

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. 

Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk