How to make this a proper state machine in c++

I have now this c++ code :

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   next_color: COLOR 
}

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   next_color: COLOR 
}

Is this a proper state machine and if not how can I improve it ? @Rägnar

45 Replies

Rägnar O'ock•3mo ago

all the remarques from chit chat still hold, I don't know if you want me to explain something specifically, if that's the case ask away I'll do my best

roelofOP•3mo ago

oke so next-color needs to be a function ??

Rägnar O'ock•3mo ago

if you want to use a finite state machine, yes

roelofOP•3mo ago

oke then it schould be

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color()  { 
      if (currentMillis - previousMillis >= interval) {
         previousMillis = currentMillis;
         switch color {
           case traffic_colors:green :  traffic_light:: yellow 
           ....
         } 
   }
}

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color()  { 
      if (currentMillis - previousMillis >= interval) {
         previousMillis = currentMillis;
         switch color {
           case traffic_colors:green :  traffic_light:: yellow 
           ....
         } 
   }
}

So something like this ?

Rägnar O'ock•3mo ago

not at all xD

roelofOP•3mo ago

🙁

Rägnar O'ock•3mo ago

let me explain your traffic_light is your state machine from the outside it's a black box all of it's logic is handled by the state it is currently in. that state should map to the color of the light green, orange, red, blinking orange (and broken if you want but... whatever)

roelofOP•3mo ago

yes, and that piece I do not see or I must use a list to hold all the different states

Rägnar O'ock•3mo ago

to know what color the light is at any moment in time you interogate the state machine, the traffic_light that will then pass on the call to the currently active state that active state will only deal with the logic of what state should the machine be set to after the call : - stay on the current state : the timer for it hasn't run out yet - got to another state : the timer ran out

roelofOP•3mo ago

oke, and the current active state is the color variable ? yep, that is what I tried to do This part:

hat active state will only deal with the logic of what state should the machine be set to after the call :

    stay on the current state : the timer for it hasn't run out yet

got to another state : the timer ran out

hat active state will only deal with the logic of what state should the machine be set to after the call :

    stay on the current state : the timer for it hasn't run out yet

got to another state : the timer ran out

Rägnar O'ock•3mo ago

no, the current state is an object (or enum in your case) that holds all the info required to model the state (as in the physical state not the machine state) of the machine so light color, timer duration, time since the last state change and a function that uses all this info to determinate if the machine should move to an other state or not

roelofOP•3mo ago

oke my understanding was that next-color is the function that changes or keep the state

Rägnar O'ock•3mo ago

it is the small part you missed in your code block above is that this function will be implemented differently for each state

roelofOP•3mo ago

yep, that is why I used a switch

Rägnar O'ock•3mo ago

the green can go to solid orange, blinking orange or stay gree the orange can go to solid red, blinking orange or stay orange the red can go to solid green, blinking orange or stay red and the blinking orange can... go to whatever initial state you want I guess or stay blinking orange so you will have 4 implementation of the next_light function, one for each of the 4 states (or 3 if you don't want to implement the blinking orange) and for each one you don't need to worry about checking the current state because it can only be the one the function is attached to so you don't need the switch

roelofOP•3mo ago

or schould I do this :

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color() 
} 

states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return ??
       }}
       ......
}

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color() 
} 

states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return ??
       }}
       ......
}

Rägnar O'ock•3mo ago

it would be more like this yes

roelofOP•3mo ago

I have then to think what schould be ???

Rägnar O'ock•3mo ago

it would be the state that comes after the current one

roelofOP•3mo ago

That will be the green state that will be decleared also in that list

Rägnar O'ock•3mo ago

so if you don't need to change the state (because the timer didn't run out yet) you return the current one, and if the timer ran out you return the appropriate new state yup

roelofOP•3mo ago

yep, but can I then return the whole state. That schould be then double code there and I think that is no right

Rägnar O'ock•3mo ago

either you keep a list of all the possible states in memory or you can construct a new one each time in your case it's easier to keep a list because your states are immutable (unless you add the last_changed_at in there) so you can re-use them over and over but if you have to store something (like the last_changed_at) in the states it would be better to construct a new one each time to avoid cross-contamination

roelofOP•3mo ago

Oke, I have to think how I declare the green state there Doing { traffic_light: Green ...;. } feels not good because in the same list I declare it already but right now I cannot think of another solution

Rägnar O'ock•3mo ago

what do you mean ?

roelofOP•3mo ago

what do I put instead of the ?? Making a totally new state feels like im doing things double because next I can declare the green state but right now I do not know a better solution

Rägnar O'ock•3mo ago

what do you mean by "Making a totally new state feels like im doing things double " ?

roelofOP•3mo ago

like this :

states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return {traffic_color:GREEN, 400, next_color() {..} 
       }}
       {traffic_color:Green; 400, next_color() {
         if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return {traffic_color:Yellow, 400, next_color() {..} 
       }} 

}

states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return {traffic_color:GREEN, 400, next_color() {..} 
       }}
       {traffic_color:Green; 400, next_color() {
         if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return {traffic_color:Yellow, 400, next_color() {..} 
       }} 

}

feels like I 2 times declare all the states

Rägnar O'ock•3mo ago

that's because you are building new states for each next_color call AND maintaining a list of all states

roelofOP•3mo ago

and if I want to add a walker traffic_right or more traffic_lights from another direction It will be a very big unmaintable code mess

Rägnar O'ock•3mo ago

you only need to do one of those if you have a list of state pick the next state in the list and return it if you want to build the next state as needed you don't need the list

roelofOP•3mo ago

oke like this :

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color() 
} 

all-lights = [traffic_color:RED, traffic_color:YELLOW, traffic_color:Green] 


states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return all-lights[2]
       }}
       ......
}

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color() 
} 

all-lights = [traffic_color:RED, traffic_color:YELLOW, traffic_color:Green] 


states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return all-lights[2]
       }}
       ......
}

Rägnar O'ock•3mo ago

and if you want to build the states as needed you should use builder functions that return a new instance of the state you want, because as is, you would need to recursively implement next_color to the end of times or your program will halt yup well... no

roelofOP•3mo ago

🙁

Rägnar O'ock•3mo ago

you don't need all-lights you already have states that's your list of states

roelofOP•3mo ago

oke but then I could do :

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color() 
} 

states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return states[2]
       }}

uint32_t previousMillis;
uint32_t currentMillis = millis();

enum traffic_colors { 
   RED; 
   GREEN; 
   YELLOW; 
} 

enum traffic_light { 
   color: COLOR; 
   time : int 
   traffic_light next_color() 
} 

states = {  
   { traffic_color:RED, 300, next_color() {  
      if (currentMillis - previousMillis >= interval) { 
           previousMillis = currentMillis; 
           return states[2]
       }}

Rägnar O'ock•3mo ago

yup

roelofOP•3mo ago

thanks, it is not compiling but I will ask the arduino people for help with it Had trouble to make it work but this compiles :

#include <Arduino.h>

unsigned long previousMillis = 0; // track last switch time

enum traffic_colors { RED, GREEN, YELLOW };

struct traffic_light {
  traffic_colors color;
  unsigned long interval;   // milliseconds
  traffic_light* next;      // pointer to next state

  traffic_light* next_color() {
    unsigned long now = millis();
    if (now - previousMillis >= interval) {
      previousMillis = now;
      return next;
    }
    return this;
  }
};

// Single array — elements can point to other elements in the same array
traffic_light states[3] = {
  { RED,    3000UL, &states[1] }, // states[0] -> states[1]
  { GREEN,  2000UL, &states[2] }, // states[1] -> states[2]
  { YELLOW, 1000UL, &states[0] }  // states[2] -> states[0]
};

traffic_light* currentState = &states[0];

void setup() {
  Serial.begin(9600);
}

void loop() {
  currentState = currentState->next_color();
  Serial.println(currentState->color); // prints 0/1/2
  delay(100);
}

#include <Arduino.h>

unsigned long previousMillis = 0; // track last switch time

enum traffic_colors { RED, GREEN, YELLOW };

struct traffic_light {
  traffic_colors color;
  unsigned long interval;   // milliseconds
  traffic_light* next;      // pointer to next state

  traffic_light* next_color() {
    unsigned long now = millis();
    if (now - previousMillis >= interval) {
      previousMillis = now;
      return next;
    }
    return this;
  }
};

// Single array — elements can point to other elements in the same array
traffic_light states[3] = {
  { RED,    3000UL, &states[1] }, // states[0] -> states[1]
  { GREEN,  2000UL, &states[2] }, // states[1] -> states[2]
  { YELLOW, 1000UL, &states[0] }  // states[2] -> states[0]
};

traffic_light* currentState = &states[0];

void setup() {
  Serial.begin(9600);
}

void loop() {
  currentState = currentState->next_color();
  Serial.println(currentState->color); // prints 0/1/2
  delay(100);
}

Rägnar O'ock•3mo ago

noice now try to add the blinking orange to see how a single state can go to more than one depending on the condition of the system (basically all states can go the the blinking orange and when it comes back on it goes to red by default I would say)

roelofOP•3mo ago

first I have to look if this is really works as expected for the blinking state I have to think how I know it must be in that state I think normally it is on a time schedule but I cannot do that I think

Rägnar O'ock•3mo ago

just looking at it I would say it looks ok, but yeah definitely test it out

roelofOP•3mo ago

Could it be a idea to make a button to set in on the blinking state or in the "normal" state

Kingpin•3mo ago

Shouldnt he add the blinking state seperatly from the color?

Rägnar O'ock•3mo ago

the color of the light is a state and there's only one that can blink it's the orange so it's a state in of its own so is "off" but... I'm not sure it's interesting to model if you just have to turn off the arduino to have it

roelofOP•3mo ago

chips, not compiling work to do 🙁

Gaming

Programming

How to make this a proper state machine in c++

Did you find this page helpful?