Trigger is not reentrant

redge76 commented 8 years ago

Hi,

I would like to be able to call trigger from within a "enter" or "transition" function. Do you see a easy modification of your lib to be able to write something like this:( modify your light_switch.ino and change just de bellow functions)

void on_light_on_enter()
{
  Serial.println("Entering LIGHT_ON");
  delay(2000);
  fsm.trigger(FLIP_LIGHT_SWITCH);
}

void on_light_off_enter()
{
  Serial.println("Entering LIGHT_OFF");
  delay(2000);
  fsm.trigger(FLIP_LIGHT_SWITCH);
}

void loop()
{
  fsm.trigger(FLIP_LIGHT_SWITCH);
}

My goal is to write some "routing state" that would automatically branch to another state depending on the value of some sensor. Something like:

void on_router_enter()
{
   int a = readSensor()
   if (a>5) {
       fsm.trigger(GO_STATE_HIGH);
   } else {
       fsm.trigger(GO_STATE_LOW);
   }
}

Regards, Redge

jonblack commented 8 years ago

Let's see if I understand. It sounds like you have more than two states A -> B -> C. When you transition from A -> B the handler for state B will trigger a transition from B -> C. Is that correct? If so, why don't you transition directly to C? Or even better, why does the transition callback need to trigger the next transition?

redge76 commented 8 years ago

I have a small LED matrix display that I use to show time and some messages contained in a circular buffer. The home button cycles through the different “applications” (CLEAR – Display TIME/DATE and display messages) The next button cycle between the “pages” in the selected application. This display is driven by a arduino-fsm state machine.

STATE_CLEAR_enter()  {
    clear display()
}

STATE_TIME_enter(){
    write(time) to LED
}

STATE_DATE_enter() {
    Write(date) to LED
}

STATE_MESSAGE_enter() {
    if (circ_buffer not empty){
        write (circ_buffer[start_pointer]) to LED
    } else   {
        fsm.trigger(EVENT_HOME)
    }
}

MESSAGE_MESSAGE_transition() {
   delete circ_buffer[start_pointer]
}

addtransition(STATE_CLEAR, STATE_TIME, EVENT_HOME)
addtransition(STATE_TIME, STATE_MESSAGE, EVENT_HOME)
addtransition(STATE_MESSAGE, STATE_CLEAR, EVENT_HOME)

addtransition(STATE_TIME, STATE_DATE, EVENT_NEXT)
addtransition(STATE_DATE, STATE_TIME, EVENT_NEXT)

addtransition(STATE_MESSAGE, STATE_MESSAGE, EVENT_NEXT, MESSAGE_MESSAGE_transition)

loop() {
   if (digitalread(button1) == low) fsm.trigger(EVENT_HOME)
   if (digitalread(button2) == low) fsm.trigger(EVENT_NEXT)
}

I know I could put the code contained in the MESSAGE_MESSAGE_transition() in the second “if” statement of the loop, but I have to make sure to trigger the HOME_EVENT only if I am in the state STATE_MESSAGE. (by the way that’s why it is good to have a state name or state ID). If I add more "applications", I will have to make even more tests.

Regards, Redge

jonblack commented 8 years ago

Thanks, now I understand your state machine, but sadly not your problem. What is working as you'd like it to?

redge76 commented 8 years ago

trigger is not reentrant. I mean that when I push the NEXT button and the buffer is empty, a second recursive call to trigger is made inside the STATE_MESSAGE_enter() function. So the second trigger update the m_current_state variable to the HOME state and then when the first trigger exits, it update back the m_current_state to MESSAGE state. There should be at least a test to prevent recursion.

redge76 commented 8 years ago

for example in this kind of state machine, the states are "chained". The machine moves from one state to another without external event. http://teachmetomake.com/wordpress/arduino-tutorial-state-machine

jonblack commented 8 years ago

Thanks, that's a better explanation.

This is (maybe) a bug. The problem is that the state in the machine is set after the callbacks are called.

If we want to allow state changes to be triggered in the callbacks, the state machine state must be set before they're triggered; however, I'm not sure what state the machine should be in when callbacks are called.

Should each callback have different assumptions about the machine state when called? e.g. in on_exit it's the from state and in on_enter it's the to state; what about the on_transition state?
If there are assumptions, how can they be enforced? Run-time is a bit late for that.
What kinds of things do people want to do in the callbacks that depend on the machine's state?
Should triggering a state change in a callback be allowed anyway?

So in answer to your question, this can be fixed in the library but the questions above need answering first. You can, however, workaround the issue by moving the logic in STATE_MESSAGE_enter to loop:

STATE_MESSAGE_enter() {
    write (circ_buffer[start_pointer]) to LED
}

...
...

loop() {
    if (digitalread(button1) == low) fsm.trigger(EVENT_HOME)
    if (digitalread(button2) == low) {
        if (circ_buffer not empty){
            fsm.trigger(EVENT_NEXT)
        } else   {
            fsm.trigger(EVENT_HOME)
        }
    }
}

What do you think?

redge76 commented 8 years ago

Should each callback have different assumptions about the machine state when called?

During a transition, the machine has no state...or a IN_FLIGHT/NULL state. The exit() state is in the state it is exiting and the enter() state, the one it is entering.... The thing is more if we have 4 states A->B and A->C->D and we call the trigger state during the transition between A->B, from where this trigger is applied ? A or B ? Maybe a trigger should be forbiden in a transition. In a exit or enter callback we are still in a state. So there is no question to answer.

What kinds of things do people want to do in the callbacks that depend on the machine's state?

? Anything....

workaround

Yes. That could be a workaround for very simple case. But it becomes really complex when the number of states increases. And in my application, if I'm in the TIME state, I need to trigger EVENT_NEXT even if the buffer is not empty. So That's why we need a getName() function in state.

loop() {
    if (digitalread(button1) == low) fsm.trigger(EVENT_HOME)
    if (digitalread(button2) == low) {
        if ((fsm.getName()==STATE_TIME) || (fsm.getName()==STATE_DATE) ) {
            fsm.trigger(EVENT_NEXT)
        } else if (circ_buffer not empty) {
            fsm.trigger(EVENT_NEXT)
        } else   {
            fsm.trigger(EVENT_HOME)
        }
    }

redge76 commented 8 years ago

we have:

STATE_A with STATE_A_onEnter() and STATE_A_onExit()
STATE_B with STATE_B_onEnter() and STATE_B_onExit()
STATE_C with STATE_C_onEnter() and STATE_C_onExit()
STATE_D with STATE_D_onEnter() and STATE_D_onExit()

STATE_A_STATE_B_transition()

addTransition(STATE_A,  STATE_B, EVENT_GOTOB)
addTransition(STATE_A,  STATE_C, EVENT_GOTOC)
addTransition(STATE_C,  STATE_D, EVENT_GOTOD)

A -> B
A -> C -> D

We are in STATE_A We do a trigger(EVENT_GOTOB) the STATE_A_onExit() contains trigger(EVENT_GOTOC) the callbacks will be STATE_A_onExit() -> STATE_C_onEnter() and we are now in STATE_C

We are in STATE_A We do a trigger(EVENT_GOTOC) the STATE_C_onEnter() contains trigger(EVENT_GOTOD) the callbacks will be STATE_A_onExit() -> STATE_C_onEnter() -> STATE_C_onExit() -> STATE_D_onEnter() and we are now in STATE_D

A trigger in STATE_A_STATE_B_transition() in error does nothing (returns error)

The other question is: What happen if we call 2 trigger() in a callback. I think this should be forbiden. Also, the next instruction after a callback should be "return ;"

jonblack commented 8 years ago

I've drawn out the state machine you've described so I can visualise it better:

fsm

jonblack commented 8 years ago

During a transition, the machine has no state...or a IN_FLIGHT/NULL state. The exit() state is in the state it is exiting and the enter() state, the one it is entering.... The thing is more if we have 4 states A->B and A->C->D and we call the trigger state during the transition between A->B, from where this trigger is applied ? A or B ? Maybe a trigger should be forbiden in a transition. In a exit or enter callback we are still in a state. So there is no question to answer.

I was thinking the same. That's a good point about triggering a state change during a transition, and I agree that should be ignored (and well documented).

Yes. That could be a workaround for very simple case. But it becomes really complex when the number of states increases. And in my application, if I'm in the TIME state, I need to trigger EVENT_NEXT even if the buffer is not empty. So That's why we need a getName() function in state.

I'm certain that getName isn't required and will only encourage people to misuse it instead of reasoning about their states and transitions. It's probably useful for debugging, but I'd rather keep it out: there are other ways to debug code.

STATE_A_onExit() -> STATE_C_onEnter() -> STATE_C_onExit() -> STATE_D_onEnter() and we are now in STATE_D

This is a nice example.

The other question is: What happen if we call 2 trigger() in a callback. I think this should be forbidden.

We can't stop people from doing this, and I don't think it's semantically a problem. It will, however, lead to code that's really hard to follow.

There is also the risk that people end up putting their code in an infinite loop, but this will eventually (quite quickly) crash the arduino when it runs out of memory.

redge76 commented 8 years ago

I've drawn out the state machine you've described

Yes That's a simplified version of what I'm currently developing. It's a 6$ clone of https://lametric.com/ with a wemos D1 mini and 2 or 3 max7219 LED Matrix. It work already really well with IFTTT. I'm using arduino-fsm to simplify interface.

I'm certain that getName isn't required

And a getId() ? I mean, how do I know in which state my machine is from outside ? Your workaround would not work without it. How do I remove the " if ((fsm.getName()==STATE_TIME) || (fsm.getName()==STATE_DATE) ) " I added ? The other solution I see, is to install the right button "callbacks" when I enter the TIME state and install others when I enter the MESSAGE state.

We can't stop people from doing this,

Just add a counter each time a trigger() is called. If counter > 2 then "return ERROR;" Decrease counter when exiting the trigger() function... Don't you think ?

There is also the risk that people end up putting their code in an infinite loop,

Like any other while() loop....

jonblack commented 8 years ago

It's a 6$ clone of https://lametric.com/ with a wemos D1 mini and 2 or 3 max7219 LED Matrix.

That's an interesting project. I'd love to read a blog post about it when you're finished. :smile:

Your workaround would not work without it.

True. The idea is we fix the problem so you don't need the workaround :smile:

Just add a counter each time a trigger() is called. If counter > 2 then "return ERROR;"

By stopping people, I mean a compile time error. At runtime it's a bit late. Since it's not semantically a problem, I don't see a reason to do this.

As far as I can tell we'd need to move make_transition from Transition to Fsm or make Fsm a friend of Transition (assuming Aduino supports that).

We'll also need a good bunch of test cases for it, including an insane one just to see what happens.

I can work on this as soon as my motherboard is returned from the repair shop, which should have been two weeks ago :sad:

redge76 commented 8 years ago

i'll try to make a post on https://hackaday.io

About the state name or ID, I really don't know how to do without it. I understand It encourages some bad programming behavior, but the work around it is to have a current_state global variable and update it in all the STATE_enter() callback. Not really cool too.

Since it's not semantically a problem

At compile time it's not an problem, but at runtime, it is an unrecoverable exception.

We'll also need a good bunch of test cases for it, including an insane one just to see what happens.

My next project for your lib is the "IoT-isation of a table soccer where all the actions are states of a FSM. That may include quite a lot of test cases as I will relie a lot on these auto triggers.

Redge

redge76 commented 8 years ago

no news ?

jonblack commented 8 years ago

Motherboard is back. This coming weekend is quite busy. If I don't manage it, I'll do it the weekend after. Sorry it can't be sooner.

Thelmos commented 8 years ago

In my opinion, Trigger should not be called in transition actions, what i miss in this library api is the posibilitiy of adding a function to a state so it would be called every time the engine checks the machine and while in this state, something like this:;

State::State(void (*on_enter)(), void (*on_estate)(), void (*on_exit)()) : on_enter(on_enter), on_state(on_state), on_exit(on_exit) { } We would need then a new function that would be called in main loop, something like .runMachine() that internally includes a call to all on_state() functions and a .check_timer(), so integrating all funcionality in one call.

A FSM could then be autosuficient, checking events into that state functions (and only the interesting ones for that state) and tiggering transitions acordingly.

@redge76 could then do all the automatic transition job in that on_state() function.

Hope my explanation was clear enough.

redge76 commented 8 years ago

I also miss the onState() function. But I think it's 2 different features. So far, for my current project, I don't need to call anything in the main loop and I would like to avoid it. It would be a workaround I would like to avoid. But a onState() with a possibility to call trigger inside is also needed.

Thelmos commented 8 years ago

@redge76 regarding your first sample:

STATE_MESSAGE_enter() {
    if (circ_buffer not empty){
        write (circ_buffer[start_pointer]) to LED
    } else   {
        fsm.trigger(EVENT_HOME)
    }
}

You could put your code in the new STATE_MESSAGE_onstate() function I propose, that would work nicely because you are out of the previous fsm.trigger call, no recursive calls then.

I'm going to fork proyect to test some of this improvements.

jonblack commented 8 years ago

@Thelmos Are you proposing that STATE_MESSAGE_onstate() be called for each state every time runMachine is called or just for the current state?

Thelmos commented 8 years ago

Yes, sorry, just for the current state.

This way the state can monitor events and trigger transitions by itself.

Thelmos commented 8 years ago

Explained in new issue #10

jonblack commented 8 years ago

@redge76 Why do you want to avoid calling things in loop()? I like the idea of an on_state callback. It avoids having to deal with recursion from calling trigger twice; it also consolidates trigger and check_timer.

sepastian commented 7 years ago

Hello @jonblack, thanks for a great library.

LIke @redge76, I would also like to avoid calling things in loop(). This way, everything FSM, including logic, could be encapsulated. I also agree that the logic can become quite complex, which is one reason for turning to FSMs in the first place.

In my application, I am looping through N LEDs, and triggering a camera once for each source of light. (I'm building this.)

I would prefer having some way of setting the state to transition to inside the FSM, i.e. writing a self-sufficient FSM containing states and transition logic.

With the on_state() callback suggested by @Thelmos, together with a call to runMachine() inside loop() I could achieve that.

What is the status on this and on #10? How can we help getting this implemented, I think this is a much needed improvement.

redge76 commented 7 years ago

The thing is.... it's implemented.... But you have to use the "thelmos-master" branch.

It works perfectly.... This new branch should become the master branch....

jonblack commented 7 years ago

Sorry for dropping the ball on this. It's been hectic here.

I don't have the overview I once had. If someone can make a pull-request out of that branch and make sure it works (e.g. all examples), I'll merge it and make a new release. (I wish I had more time to do this myself).

I just merged in @Thelmos 2.2.0 pull-request and made a release, too, so there's already one update out there.

sepastian commented 7 years ago

Thank you all, i will give it a try next week.

On Oct 25, 2017 3:09 PM, "Jon Black" notifications@github.com wrote:

Sorry for dropping the ball on this. It's been hectic here.

I don't have the overview I once had. If someone can make a pull-request out of that branch and make sure it works (e.g. all examples), I'll merge it and make a new release.

I just merged in @Thelmos https://github.com/thelmos 2.2.0 pull-request and made a release, too, so there's already one update out there.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jonblack/arduino-fsm/issues/3#issuecomment-339324093, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA6QzNF7K52gNss2LhsMjMrdqKgPOLmks5svzL8gaJpZM4HflT_ .

sle118 commented 6 years ago

I've been hoping for a light weight state machine like this, and there it is! I've used a stateless state machine in c# in some other project, and transitions/triggers are implemented a bit differently (it's stateless).

I'm stepping in this conversation just to suggest yet another possible approach to the reentry problem: it might be possible to add an asynchronous trigger that decouples setting the new state from the trigger event itself. State transition could then be executed outside these methods in a loop "process" method, etc.

I'm going to give the branch a try... State machines generally make the code much easier to implement and maintain and I really need something like that right now. So thanks a bunch to @jonblack !

Edit: looks like someone implemented something similar in a fork https://github.com/machadolab/arduino-fsm/commit/a4e02930cc278823dd122509ba42aa897e74a50c

jonblack / arduino-fsm

Trigger is not reentrant #3