Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Can we simplify time-based transitions? #4

Closed
mrisher opened this issue Jul 21, 2022 · 9 comments
Closed

Can we simplify time-based transitions? #4

mrisher opened this issue Jul 21, 2022 · 9 comments

Comments

@mrisher
Copy link

mrisher commented Jul 21, 2022

Hi: I love your library and have a project that needs to instantiate multiple, independent state machines. For code cleanliness and to avoid global variables, I tried to subclass YA_FSM to encapsulate the shared settings, a la

class PulsarStateMachine : public YA_FSM {public: int pulsarTimeout = 100; ...}

and then I have a PulsarStateMachine::Setup() function that defines the states, actions, and transitions.

However, I'm getting stuck following your timed transition pattern from within that Setup() function, because the lambda doesn't know how to find the CurrentState()->timeout. Your recommended pattern is:

this->AddTransition(ON_STATE, OFF_STATE, [](){return stateMachine.CurrentState()->timeout;} );

...but I'm wondering whether we could simplify by adding something to YA_FSM::Update() that directly checks _currentState->timeout; Is there a reason you implemented with lambdas that I'm overlooking or could we add an AddTimedTransition(FROM_STATE, TO_STATE) function and skip the complexity of lambdas and function pointers?

Thank you for humoring me, and apologies if I'm missing something subtle for why you designed it this way. I'm not super familiar with lambda syntax and after a day of fighting with various captures and compiler warnings, wondered if this might be a simpler solution.

@mrisher
Copy link
Author

mrisher commented Jul 21, 2022

P.S. Here's the line where I get compiler errors (https://github.com/mrisher/alien_escape/blob/refactor-pulsarFSM/src/matrixPulsarFsm.cpp#L13)

/home/mrisher/src/alien_escape/src/matrixPulsarFsm.cpp: In lambda function:
matrixPulsarFsm.cpp:13:63: error: 'this' was not captured for this lambda function
                                 { return YA_FSM::CurrentState()->timeout; });
                                                               ^
matrixPulsarFsm.cpp:13:63: error: cannot call member function 'FSM_State* YA_FSM::CurrentState()' without object
/home/mrisher/src/alien_escape/src/matrixPulsarFsm.cpp: In member function 'void MatrixPulsarFSM::Setup()':
/home/mrisher/src/alien_escape/src/matrixPulsarFsm.cpp:13:76: warning: invalid user-defined conversion from 'MatrixPulsarFSM::Setup()::<lambda()>' to 'condition_cb {aka bool (*)()}' [-fpermissive]
                                 { return YA_FSM::CurrentState()->timeout; });
                                                                            ^
/home/mrisher/src/alien_escape/src/matrixPulsarFsm.cpp:12:43: note: candidate is: MatrixPulsarFSM::Setup()::<lambda()>::operator void (*)()() const <near match>
   AddTransition(PULSAR_ON, PULSAR_OFF, []()
                                           ^
/home/mrisher/src/alien_escape/src/matrixPulsarFsm.cpp:12:43: note:   no known conversion from 'void (*)()' to 'condition_cb {aka bool (*)()}'
In file included from /home/mrisher/src/alien_escape/src/matrixPulsarFsm.h:4:0,
                 from /home/mrisher/src/alien_escape/src/matrixPulsarFsm.cpp:1:
/home/mrisher/src/libraries/YA_FSM/src/YA_FSM.h:73:11: note:   initializing argument 3 of 'uint8_t YA_FSM::AddTransition(uint8_t, uint8_t, condition_cb)'
  uint8_t  AddTransition(uint8_t inputState, uint8_t outputState, condition_cb condition);
           ^~~~~~~~~~~~~

@mrisher
Copy link
Author

mrisher commented Jul 24, 2022

Created a pull request: #5 #5

@cotestatnt
Copy link
Owner

cotestatnt commented Jul 25, 2022

Hi @mrisher
I'm sorry for late reply, I was on vacation last week.

I am glad you are satisfied with this library, I have developed it mainly for my benefit and for this reason it is unfortunately not very well documented.

Regarding your request, it actually makes sense that if I define a maximum timeout time for the state, this then automatically go to the next one (according to the defined transitions) without the need to add a callback function or a dedicated variable.

However, I am not entirely convinced of the way in which you have implemented the functionality because I don't think there was a need to add dedicated property and method to the YA_FSM class.

Take a look at how I implemented the functionality. It's only in testing for now, but I think I'll officially add it to the repository soon

@mrisher
Copy link
Author

mrisher commented Jul 25, 2022 via email

@cotestatnt
Copy link
Owner

I could simply check whether the time has expired

That's more or less what I did in the wokwi example I linked you to.

If a trigger variable or callback function is not defined, the transition will be triggered on timeout, but this condition is the "weakest" because it is evaluated first.

By default, now both the callback pointer and the variable pointer are == nullptr so, when they are assigned, the local _trigger bool variable will be overwritten regardless of the timeout state.

     bool _trigger = false;

     // If a max time was defined check if current state is on timeout      
     if (_currentState->maxTime) {        
       _currentState->timeout = millis() - _currentState->enterTime > _currentState->maxTime;              
       // Trigger transition on timeout
       if (_currentState->timeout)    
         _trigger = true;        
     }  
   
     // Trigger transition on callback function result if defined
     if (actualtr->Condition != nullptr)
       _trigger = actualtr->Condition();    

     // Trigger transition on bool variable value == true
     if (actualtr->ConditionVar != nullptr) 
       _trigger =  *(actualtr->ConditionVar);   

@cotestatnt
Copy link
Owner

cotestatnt commented Jul 25, 2022

Thinking about it, maybe it would be the case that instead the timeout is a priority by putting the relative if statement as last position...
I'm not sure about it: after all, if I decide that a state must last for a maximum of XXX milliseconds, what is the point of binding the transition to an external condition if it arrives late? What's your opinion?

@mrisher
Copy link
Author

mrisher commented Jul 25, 2022 via email

@cotestatnt
Copy link
Owner

cotestatnt commented Jul 28, 2022

I've updated the library as you suggest (if, else if). I'm going to close this issue if you agree.
I've modified also the examples.

@mrisher
Copy link
Author

mrisher commented Aug 10, 2022 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants