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.