LennartHennigs / SimpleFSM

Arduino/ESP library to simplify setting up and running a state machine.
MIT License
66 stars 15 forks source link

Behavior of TimedTransition undefined if "wrong" constructor is used #9

Closed bloodhoundmichael closed 11 months ago

bloodhoundmichael commented 1 year ago

The constructors look like this:

TimedTransition::TimedTransition() : start(0), interval(0) {}

/////////////////////////////////////////////////////////////////

TimedTransition::TimedTransition(State* from, State* to, int interval, CallbackFunction on_run /* = NULL */, String name /* = "" */, GuardCondition guard /* = NULL */) {
  setup(from, to, interval, on_run, name, guard);
}

So the first constructor is setting start to 0 whereas the second constructor doesn't.

start is being used by SimpleFSM in the run function like this:

  // go through the timed events
  for (int i = 0; i < num_timed; i++) {
    if (timed[i].from != current_state) continue;
    // start the transition timer
    if (timed[i].start == 0) {
      timed[i].start = now;
      continue;
    }
    // reached the interval?
    if (now - timed[i].start >= timed[i].interval) {
      if (_transitionTo(&timed[i])) {
        timed[i].start = 0;
        return;
      }
    }
  }

Because start is not set in constructor, the value of start is arbitrary if the second constructor is being used. In my case the value is being -1 so start is not set to now in the for loop so now - timed[i].start is a huge value, if now is huge. In summary this leads to undefined behavior.

Workaround: Use the default constructor and then setup. Long term solution: The second constructor should call the default constructor to set start to 0.