boost-ext / sml

C++14 State Machine library
https://boost-ext.github.io/sml
Boost Software License 1.0
1.16k stars 179 forks source link

How do I do this? #298

Open jonesmz opened 5 years ago

jonesmz commented 5 years ago
inline namespace statemachine
{
    using namespace boost::sml;

    struct StateMachineLogger
    {
        template <class SM, class TEvent>
        static void log_process_event(const TEvent&)
        {
            BOOST_LOG_TRIVIAL(info) << "[" << aux::get_type_name<SM>() << "][[process_event]"
                                    << " " << aux::get_type_name<TEvent>();
        }

        template <class SM, class TGuard, class TEvent>
        static void log_guard(const TGuard&, const TEvent&, bool result)
        {

            BOOST_LOG_TRIVIAL(info) << "[" << aux::get_type_name<SM>() << "][[guard]"
                                    << " " << aux::get_type_name<TGuard>()
                                    << " " << aux::get_type_name<TEvent>() << " Accepted: " << std::boolalpha << result;
        }

        template <class SM, class TAction, class TEvent>
        static void log_action(const TAction&, const TEvent&)
        {
            BOOST_LOG_TRIVIAL(info) << "[" << aux::get_type_name<SM>() << "][[action]"
                                    << " " << aux::get_type_name<TAction>()
                                    << " " << aux::get_type_name<TEvent>();
        }

        template <class SM, class TSrcState, class TDstState>
        static void log_state_change(const TSrcState& src, const TDstState& dst)
        {
            BOOST_LOG_TRIVIAL(info) << "[" << aux::get_type_name<SM>() << "][[transition]"
                                    << " " << src.c_str()
                                    << " -> " << dst.c_str();
        }
    };

    // There's no documentation in any of the examples of doing this
    // as far as I can find. But this is *much* more type safe than using 'user defined literals' in every place.
    static constexpr auto idle_state    = "idle"_s;
    static constexpr auto execute_state = "execute"_s;
} // inline namespace statemachine

class TransactionClass
{
    void execute(void)
    {
        m_machine.process_event(execute_start_ev{});
    }
    void execute_impl(void)
    {
        // The parameter to this function is called when the action is finished
        async_action([this](){ m_machine.process_event(execute_complete_ev{}); });
    }
    decltype(auto) configure()
    {
        using namespace boost::sml;
        return make_transition_table(
            *idle_state + event<execute> = execute_state,
             execute_state + sml::on_entry<_> / &TransactionClass::execute_impl
        );
    }
    // Stateless logger. How can I do this?
    // The this parameter here is applied to the pointer-to-member-functions used in the transition table above.
    // I apparently can't do TransactionClass in the template parameter list here.
    // Can I provide the transition table from a constexpr function? E.g.
    // boost::sml::sm<my_transition_table_function_here()> m_machine{this};
    boost::sml::sm<TransactionClass, sml::logger<StateMachineLogger>> m_machine{this};
};

int main()
{
    TransactionClass c;
    c.execute();
}

So I have multiple things I'm asking about here.

  1. Can I use a stateless logger? I don't want to have to provide an instance of the logger type i specify. There don't seem to be any examples showing this is possible. And when I modified the logger example to use static functions, I got a compile error.
  2. How can I provide the transition table to the state machine without needing to provide a class type? E.g. I'd like to define my transition table like
static constexpr auto transitionTable = make_transition_table( /* my transition table here */);
...
boost::sml::sm<decltype(transitionTable)> myMachine; // No constructor parameters needed
  1. Directly use pointer-to-member-functions as part of the transition table. E.g.
struct MyStruct
{
    decltype(auto) configure()
    {
        using namespace boost::sml;
        return make_transition_table(
            *"idle"_s + event<execute> / &TypeNameHere::FunctionNameHere = "execute"_s,
        );
    }
}
TypeNameHere myObj;
boost::sml::sm<MyStruct> machine{myObj}; // pointer-to-member-functions called on myObj as appropriate.

Bonus questions

  1. The documentation for how parameters to guards and actions work is... extremely confusing. Can you explain what my options are?
  2. Inheriting from boost::sml::sm ??
etam commented 5 years ago
  1. Can I use a stateless logger? I don't want to have to provide an instance of the logger type i specify. There don't seem to be any examples showing this is possible. And when I modified the logger example to use static functions, I got a compile error.

AFAIK: No. I also stumbled upon this and found out that you have to make a logger object, even if it's empty.

  1. How can I provide the transition table to the state machine without needing to provide a class type?

You can't. SML uses this: using transitions_t = decltype(aux::declval<sm_t>().operator()());, where sm_t is a class provided by you.

  1. Directly use pointer-to-member-functions as part of the transition table

I tried. It failed to compile.

  1. The documentation for how parameters to guards and actions work is... extremely confusing. Can you explain what my options are?

It works like this: whatever you passed to state machine constructor + processed event is passed further to your guards and actions. SML is quite flexible that you can specify any subset of those and in any order in guard or action arguments and it will do the right thing.

  1. Inheriting from boost::sml::sm ??

Don't.

My closing thoughts: you're trying to do too much in a single class. Your implementation details (like execute_impl) is mixed with public interface. Write a separate internal class, that just provides a transition table. If you need to access other fields of your class from state machine actions, pass them to state machine constructor. If you actually need to call your class public interface from actions, maybe your state machine shouldn't live inside it, but outside.

jonesmz commented 5 years ago

Thank you for sharing your thoughts, i really appreciate it.

AFAIK: No. I also stumbled upon this and found out that you have to make a logger object, even if it's empty.

Frustrating. Not a dealbreaker, but still frustrating.

You can't. SML uses this: using transitions_t = decltype(aux::declval().operator()());, where sm_t is a class provided by you.

This is a weird design decision. If all SML wants is just transitions_t, I'd really rather provide that directly.

Or perhaps something like

struct transition_table { type = /* my transition table type */; };

Needing to overload operator() is super weird in my opinion.

It works like this: whatever you passed to state machine constructor + processed event is passed further to your guards and actions. SML is quite flexible that you can specify any subset of those and in any order in guard or action arguments and it will do the right thing.

I have a strong aversion to "It will do the right thing", as in the past I've discovered that libraries that automatically map the order of parameters provided to the order of parameters accepted by the callback tend to get very broken as soon as I do something unexpected, like provide two of the same argument type.

I'd be OK with something that just provides the arguments, as provided, to every guard and action object, every time, in the order given to SML, but any form of re-arranging or subsetting makes me uneasy.

Am I understanding correctly how dynamic SML tries to be in this context?

My closing thoughts: you're trying to do too much in a single class. Your implementation details (like execute_impl) is mixed with public interface. Write a separate internal class, that just provides a transition table. If you need to access other fields of your class from state machine actions, pass them to state machine constructor. If you actually need to call your class public interface from actions, maybe your state machine shouldn't live inside it, but outside.

You're not wrong that my example has a lot of mixing of internal details vs interface. I was trying to be concise, not necessarily indicate that I want to do those things in exactly that way.

I'm trying to figure out how to adapt an existing set of code to use SML.

One codebase that I'm considering adapting is : https://github.com/redboltz/mqtt_cpp

That codebase involves a hand-written state machine for parsing incoming packets, and there's a lot of places where errors are handled in exactly the same way every time. I'm investigating whether using SML, and throwing exceptions for error handling which SML will catch and do a state transition action, is an appropriate change.

The trouble with putting the transition table into a different class is that I just want the state machine to call the existing code. Accepting the existing class as a parameter, and then having to write a lambda for each guard / action that accesses data out of the existing class sounds like a royal pain.

Among other things, the state machine would call internal functions which could theoretically (but with some difficulty) be split into some kind of separate object, but the state machine would also need to trigger calls to virtual functions to notify end-user code of various events (Packet processing completed, error encountered, and so on).

If I could provide SML with the transition table without the use of operator(), that wouldn't fix all of my complaints, but it would at least make it directly possible to do what I'm trying to do.

etam commented 5 years ago

Thank you for sharing your thoughts, i really appreciate it.

AFAIK: No. I also stumbled upon this and found out that you have to make a logger object, even if it's empty.

Frustrating. Not a dealbreaker, but still frustrating.

I agree.

You can't. SML uses this: using transitions_t = decltype(aux::declval().operator()());, where sm_t is a class provided by you.

This is a weird design decision. If all SML wants is just transitions_t, I'd really rather provide that directly.

Or perhaps something like

struct transition_table { type = /* my transition table type */; };

Needing to overload operator() is super weird in my opinion.

I also agree.

It works like this: whatever you passed to state machine constructor + processed event is passed further to your guards and actions. SML is quite flexible that you can specify any subset of those and in any order in guard or action arguments and it will do the right thing.

I have a strong aversion to "It will do the right thing", as in the past I've discovered that libraries that automatically map the order of parameters provided to the order of parameters accepted by the callback tend to get very broken as soon as I do something unexpected, like provide two of the same argument type.

I'd be OK with something that just provides the arguments, as provided, to every guard and action object, every time, in the order given to SML, but any form of re-arranging or subsetting makes me uneasy.

Am I understanding correctly how dynamic SML tries to be in this context?

Don't take my word for it, but I think yes.

My closing thoughts: you're trying to do too much in a single class. Your implementation details (like execute_impl) is mixed with public interface. Write a separate internal class, that just provides a transition table. If you need to access other fields of your class from state machine actions, pass them to state machine constructor. If you actually need to call your class public interface from actions, maybe your state machine shouldn't live inside it, but outside.

You're not wrong that my example has a lot of mixing of internal details vs interface. I was trying to be concise, not necessarily indicate that I want to do those things in exactly that way.

I'm trying to figure out how to adapt an existing set of code to use SML.

One codebase that I'm considering adapting is : https://github.com/redboltz/mqtt_cpp

That codebase involves a hand-written state machine for parsing incoming packets, and there's a lot of places where errors are handled in exactly the same way every time. I'm investigating whether using SML, and throwing exceptions for error handling which SML will catch and do a state transition action, is an appropriate change.

The trouble with putting the transition table into a different class is that I just want the state machine to call the existing code. Accepting the existing class as a parameter, and then having to write a lambda for each guard / action that accesses data out of the existing class sounds like a royal pain.

Among other things, the state machine would call internal functions which could theoretically (but with some difficulty) be split into some kind of separate object, but the state machine would also need to trigger calls to virtual functions to notify end-user code of various events (Packet processing completed, error encountered, and so on).

If I could provide SML with the transition table without the use of operator(), that wouldn't fix all of my complaints, but it would at least make it directly possible to do what I'm trying to do.

good luck

jonesmz commented 5 years ago

Thanks for the feedback :-)

cblauvelt commented 5 years ago

I was able to create actions that called member functions of a class.

I created actions like:

struct Connect {
    auto operator()(detail::TcpClientPrivate* client) {
        client->connect();
    }
};

struct ConnectingStateMachine {
    Resolve resolve;
    Connect connect;

    auto operator()() const {
        using namespace boost::sml;
        /**
         * Initial state: *initial_state
         * Transition DSL: src_state + event [ guard ] / action = dst_state
         */
        return make_transition_table(
            * state<Resolving>          + on_entry<_>                      / resolve,
                state<Resolving>          + event<Event::ResolveComplete>    / connect    = state<ConnectingSocket>
        );
    }
};

TcpClientPrivate was a class that contained the following as the last member variable:

struct StateMachine;
shared_ptr<StateMachine> mStateMachine;

StateMachine is then defined in TcpClientPrivate.cpp as:

using StateMachineType = sml::sm<TcpClientStateMachine::ClientMachine,sml::thread_safe<std::recursive_mutex>>;

struct TcpClientPrivate::StateMachine : public StateMachineType {
    explicit StateMachine(TcpClientPrivate* client)
        : StateMachineType(static_cast<TcpClientPrivate*>(client)) {}
};

The member function has to be public. I could not figure out a way around this.

jonesmz commented 5 years ago

Ah, so you worked around the problem that @redboltz was having here: https://github.com/boost-experimental/sml/issues/93

by using type erasure and allocating a new shared pointer.

Interesting take!

jonesmz commented 5 years ago

I might end up doing something like this:

https://github.com/ubeh/fsm_examples/tree/master/sml_refactored

Still a whole bunch of lambdas to just to call member functions, which i'm really unhappy with, but at least they're able to embed the state machine in the class that's being manipulated.

cblauvelt commented 5 years ago

That's what I was looking for. That's the example I used to make my code.

My issue was that I couldn't set a timer inside an action (try to connect again in 1 second) and I ended up needing to use the example you reference.

redboltz commented 5 years ago

I noticed that https://github.com/boost-experimental/sml/issues/93#issuecomment-283630876 is compiled on g++(9.1.0) but not compiled on clang++ (8.0.1).

https://github.com/boost-experimental/sml/issues/93#issuecomment-283629397 can be compiled both g++ and clang++.