Small bug when I replaced delays with millis

I am working on a traffic light for project and I am using millis to control the timing of my lights. I have two sets of lights. At first I was using delays, but then I realized that was not going to work for the other features of my project, so I implemented the millis() function instead. Here is my original code with the delays:

const int greenPin1 = 5;
const int yellowPin1 = 6;
const int redPin1 = 7;

//second set of lights
const int greenPin2 = 8;
const int yellowPin2 = 9;
const int redPin2 = 10;
//-----------------------------------------
 //start at green for first set of lights 
  digitalWrite(redPin1, LOW);
  digitalWrite(greenPin1, HIGH);
  delay(2000);

  //change to yellow, then red for first set
  digitalWrite(greenPin1, LOW);
  digitalWrite(yellowPin1, HIGH);
  delay(2000);
  digitalWrite(yellowPin1, LOW);
  digitalWrite(redPin1, HIGH);
  delay(5000);

  //start at green for second set of lights
  digitalWrite(redPin2, LOW);
  digitalWrite(greenPin2, HIGH);
  delay(2000);

  //change to yellow, red
  digitalWrite(greenPin2, LOW);
  digitalWrite(yellowPin2, HIGH);
  delay(2000);
  digitalWrite(yellowPin2, LOW);
  digitalWrite(redPin2, HIGH);
  delay(2000); <--********* I think this got lost somewhere??? ***************

This is what I changed it to:

const byte led1[] = { greenPin1, yellowPin1, redPin1 };
const byte led2[] = {redPin2, greenPin2, yellowPin2};

//array of the lengths of time each light will flash
const unsigned int duration1[] = { 2000, 2000, 5000};
const unsigned int duration2[] = { 5000, 2000, 2000};

int state1; //state of LED - on or off?
int state2;
unsigned long lastTransition;
unsigned long lastTransition2;

void setup() {
 
  pinMode(greenPin1, OUTPUT);
  pinMode(yellowPin1, OUTPUT);
  pinMode(redPin1, OUTPUT);
  pinMode(greenPin2, OUTPUT);
  pinMode(yellowPin2, OUTPUT);
  pinMode(redPin2, OUTPUT);
  digitalWrite(led1[state1], HIGH);
  digitalWrite(led2[state2], HIGH);
}

void loop() {

   //cycle for first set of traffic lights
  unsigned long topLoop = millis();
  if (topLoop - lastTransition >= duration1[state1]) {
    digitalWrite(led1[state1], LOW);
    lastTransition = topLoop;
    if (++state1 >= sizeof(led1)) {
      state1 = 0;
    }
    digitalWrite(led1[state1], HIGH);
  }

  //cycle for second set of traffic lights
  unsigned long topLoop2 = millis();
  if (topLoop2 - lastTransition2 >= duration2[state2]) {
    digitalWrite(led2[state2], LOW);
    lastTransition2 = topLoop2;
    if (++state2 >= sizeof(led2)) {
      state2 = 0;
    }
    digitalWrite(led2[state2], HIGH);
  }

The timing isn’t the same as when I was using the delays, so now my green and red lights are switching at the EXACT same time, but what I actually want is for there to be a two second hold between my red to green transitions. Can anyone help me with fixing this problem?

autumnadagio:
I am working on a traffic light for project and I am using millis to control the timing of my lights. I have two sets of lights. At first I was using delays, but then I realized that was not going to work for the other features of my project, so I implemented the millis() function instead. Here is my original code with the delays:

const int greenPin1 = 5;

const int yellowPin1 = 6;
const int redPin1 = 7;

//second set of lights
const int greenPin2 = 8;
const int yellowPin2 = 9;
const int redPin2 = 10;
//-----------------------------------------
//start at green for first set of lights
 digitalWrite(redPin1, LOW);
 digitalWrite(greenPin1, HIGH);
 delay(2000);

//change to yellow, then red for first set
 digitalWrite(greenPin1, LOW);
 digitalWrite(yellowPin1, HIGH);
 delay(2000);
 digitalWrite(yellowPin1, LOW);
 digitalWrite(redPin1, HIGH);
 delay(5000);

//start at green for second set of lights
 digitalWrite(redPin2, LOW);
 digitalWrite(greenPin2, HIGH);
 delay(2000);

//change to yellow, red
 digitalWrite(greenPin2, LOW);
 digitalWrite(yellowPin2, HIGH);
 delay(2000);
 digitalWrite(yellowPin2, LOW);
 digitalWrite(redPin2, HIGH);
 delay(2000); <–********* I think this got lost somewhere??? ***************




This is what I changed it to:


const byte led1 = { greenPin1, yellowPin1, redPin1 };
const byte led2 = {redPin2, greenPin2, yellowPin2};

//array of the lengths of time each light will flash
const unsigned int duration1 = { 2000, 2000, 5000};
const unsigned int duration2 = { 5000, 2000, 2000};

int state1; //state of LED - on or off?
int state2;
unsigned long lastTransition;
unsigned long lastTransition2;

void setup() {

pinMode(greenPin1, OUTPUT);
 pinMode(yellowPin1, OUTPUT);
 pinMode(redPin1, OUTPUT);
 pinMode(greenPin2, OUTPUT);
 pinMode(yellowPin2, OUTPUT);
 pinMode(redPin2, OUTPUT);
 digitalWrite(led1[state1], HIGH);
 digitalWrite(led2[state2], HIGH);
}

void loop() {

//cycle for first set of traffic lights
 unsigned long topLoop = millis();
 if (topLoop - lastTransition >= duration1[state1]) {
   digitalWrite(led1[state1], LOW);
   lastTransition = topLoop;
   if (++state1 >= sizeof(led1)) {
     state1 = 0;
   }
   digitalWrite(led1[state1], HIGH);
 }

//cycle for second set of traffic lights
 unsigned long topLoop2 = millis();
 if (topLoop2 - lastTransition2 >= duration2[state2]) {
   digitalWrite(led2[state2], LOW);
   lastTransition2 = topLoop2;
   if (++state2 >= sizeof(led2)) {
     state2 = 0;
   }
   digitalWrite(led2[state2], HIGH);
 }




The timing isn't the same as when I was using the delays, so now my green and red lights are switching at the EXACT same time, but what I actually want is for there to be a two second hold between my red to green transitions. Can anyone help me with fixing this problem?

You are not setting the INITIAL values of state1 and state2.

Also realize that your array indices start with 0 (in case you didn’t know).

autumnadagio:
I am working on a traffic light for project and I am using millis to control the timing of my lights. I have two sets of lights. At first I was using delays, but then I realized that was not going to work for the other features of my project, so I implemented the millis() function instead. Here is my original code with the delays:

const int greenPin1 = 5;

const int yellowPin1 = 6;
const int redPin1 = 7;

//second set of lights
const int greenPin2 = 8;
const int yellowPin2 = 9;
const int redPin2 = 10;
//-----------------------------------------
//start at green for first set of lights
 digitalWrite(redPin1, LOW);
 digitalWrite(greenPin1, HIGH);
 delay(2000);

//change to yellow, then red for first set
 digitalWrite(greenPin1, LOW);
 digitalWrite(yellowPin1, HIGH);
 delay(2000);
 digitalWrite(yellowPin1, LOW);
 digitalWrite(redPin1, HIGH);
 delay(5000);

//start at green for second set of lights
 digitalWrite(redPin2, LOW);
 digitalWrite(greenPin2, HIGH);
 delay(2000);

//change to yellow, red
 digitalWrite(greenPin2, LOW);
 digitalWrite(yellowPin2, HIGH);
 delay(2000);
 digitalWrite(yellowPin2, LOW);
 digitalWrite(redPin2, HIGH);
 delay(2000); <–********* I think this got lost somewhere??? ***************




This is what I changed it to:


const byte led1 = { greenPin1, yellowPin1, redPin1 };
const byte led2 = {redPin2, greenPin2, yellowPin2};

//array of the lengths of time each light will flash
const unsigned int duration1 = { 2000, 2000, 5000};
const unsigned int duration2 = { 5000, 2000, 2000};

int state1; //state of LED - on or off?
int state2;
unsigned long lastTransition;
unsigned long lastTransition2;

void setup() {

pinMode(greenPin1, OUTPUT);
 pinMode(yellowPin1, OUTPUT);
 pinMode(redPin1, OUTPUT);
 pinMode(greenPin2, OUTPUT);
 pinMode(yellowPin2, OUTPUT);
 pinMode(redPin2, OUTPUT);
 digitalWrite(led1[state1], HIGH);
 digitalWrite(led2[state2], HIGH);
}

void loop() {

//cycle for first set of traffic lights
 unsigned long topLoop = millis();
 if (topLoop - lastTransition >= duration1[state1]) {
   digitalWrite(led1[state1], LOW);
   lastTransition = topLoop;
   if (++state1 >= sizeof(led1)) {
     state1 = 0;
   }
   digitalWrite(led1[state1], HIGH);
 }

//cycle for second set of traffic lights
 unsigned long topLoop2 = millis();
 if (topLoop2 - lastTransition2 >= duration2[state2]) {
   digitalWrite(led2[state2], LOW);
   lastTransition2 = topLoop2;
   if (++state2 >= sizeof(led2)) {
     state2 = 0;
   }
   digitalWrite(led2[state2], HIGH);
 }




The timing isn't the same as when I was using the delays, so now my green and red lights are switching at the EXACT same time, but what I actually want is for there to be a two second hold between my red to green transitions. Can anyone help me with fixing this problem?

Would something simpler be easier to debug? like:

void loop() {
  static unsigned long EverySecondTimer; // "static" keyword: Keep its value from last loop
  static int SecondsCounter; // "static" keyword: Keep its value from last loop
  // Other Code Here
  if ( millis() - EverySecondTimer >= (1000)) {// One Second Timer
    EverySecondTimer += (1000);
    if (SecondsCounter > 14)SecondsCounter = 0;
    digitalWrite(greenPin1, (SecondsCounter < 2) ? HIGH : LOW);
    digitalWrite(yellowPin1, ((SecondsCounter  >= 2) && (SecondsCounter  < 4)) ? HIGH : LOW);
    digitalWrite(redPin1, ((SecondsCounter  >= 4) && (SecondsCounter  < 9)) ? HIGH : LOW);
    digitalWrite(greenPin2, ((SecondsCounter  >= 9) && (SecondsCounter  < 11)) ? HIGH : LOW);
    digitalWrite(yellowPin2, ((SecondsCounter  >= 11) && (SecondsCounter  < 13)) ? HIGH : LOW);
    digitalWrite(redPin2, ((SecondsCounter  >= 13)) ? HIGH : LOW);
    SecondsCounter++;// Do this after to keep from skipping first second
  }
  // Other Code Here
}

Z

I have no idea how you got from the first code to the second one.

The below can be an approach to cycle through the led states.
Step 1:
I usually hate HIGH and LOW for the outputs as it depends on how the lights are wired and therefore I prefer ON and OFF. Place the below at the top of your code

#define ON HIGH

This simply defines an ‘alias’ for HIGH; you can change HIGH to LOW if your leds are wired from Vcc to pin.

Next we can define a two-dimensional array for the traffic light pins

byte trafficlightPins[][3] =
{
  {2, 3, 4},  // traffic light 1 r,g,y
  {5, 6, 7}   // traffic light 2 r,g,y
};

You can use a nested for loop to cycle through these pins to set the output mode; I will show that in setup().

Next we define a struct that can hold information about the status of the leds

struct LEDSTATES
{
  byte lightstates[2][3];   // states for r,g,y for each traffic light
  unsigned long duration;   // the duration that a 'situation' must be active
};

It defines a two-dimensional array to hold the status of the leds of traffic light 1 and traffic light 2 and a duration how long the leds must stay in this status.

Now you can populate an array of LEDSTATES

LEDSTATES ledStates[] =
{
  // traffic light1  traffic light2  duration
  {{{ON, !ON, !ON}, {ON, !ON, !ON}}, 2000},   // both traffic lights red
  {{{!ON, ON, !ON}, {ON, !ON, !ON}}, 3000},   // first one green
  {{{!ON, !ON, ON}, {ON, !ON, !ON}}, 4000},   // first one yellow

  {{{ON, !ON, !ON}, {ON, !ON, !ON}}, 5000},   // both traffic lights red
  {{{ON, !ON, !ON}, {!ON, ON, !ON}}, 6000},   // second one green
  {{{ON, !ON, !ON}, {!ON, !ON, ON}}, 7000},   // second one yellow
};

Note the use of ON and !ON; !ON means not ON (so OFF).

Now we can use the nested for loop to set the pins to output; the first part gives some debug information about how many pins there are in a traffic light and how many traffic lights.

void setup()
{
  Serial.begin(250000);
  Serial.print("Number of lights in a traffic light: ");
  Serial.println(sizeof(trafficlightPins[0]));
  Serial.print("Number of traffic lights: ");
  Serial.println(sizeof(trafficlightPins) / sizeof(trafficlightPins[0]));

  // loop through the pins and make them outputs
  for (int lightCnt = 0; lightCnt < sizeof(trafficlightPins) / sizeof(trafficlightPins[0]); lightCnt++)
  {
    for (int colorCnt = 0; colorCnt < sizeof(trafficlightPins[0]); colorCnt++)
    {
      pinMode(trafficlightPins[lightCnt][colorCnt], OUTPUT);
      Serial.print("Traffic light "); Serial.print(lightCnt);
      Serial.print(" pin "); Serial.print(trafficlightPins[lightCnt][colorCnt]);
      Serial.println(" set to output");
    }
  }

Next we give the leds their initial states based on the values in the first element of the ledStates array.

 // inital states (ledStates[0])
  for (int lightCnt = 0; lightCnt < sizeof(trafficlightPins) / sizeof(trafficlightPins[0]); lightCnt++)
  {
    for (int colorCnt = 0; colorCnt < sizeof(trafficlightPins[0]); colorCnt++)
    {
      digitalWrite(trafficlightPins[lightCnt][colorCnt], ledStates[0].lightstates[lightCnt][colorCnt]);
      Serial.print("Traffic light "); Serial.print(lightCnt);
      Serial.print(" pin "); Serial.print(trafficlightPins[lightCnt][colorCnt]);
      Serial.print(" set to "); Serial.println(ledStates[0].lightstates[lightCnt][colorCnt] == ON ? "ON" : "OFF");
    }
  }
} // end of setup

Don’t worry too much about the funny println at the end of each for loop; it’s a short notation for if/else

Next you can cycle though the ledStates in loop(); first we define the variables that we need

void loop()
{
  // get current time
  unsigned long currentTime = millis();

  // last time that something happened
  static unsigned long lastTime = 0;

  // keep track of the ledstate that we're in
  static byte ledstateIndex = 0;

Note the use of the static variables; their values will be remembered when loop() ends

And next we implement the timing

 // check if it's time
  if (currentTime - lastTime >= ledStates[ledstateIndex].duration)
  {
    Serial.println("------");
    Serial.print("current time = "); Serial.println(currentTime);
    lastTime = currentTime;
    ledstateIndex++;
    if (ledstateIndex == sizeof(ledStates) / sizeof(ledStates[0]))
    {
      ledstateIndex = 0;
    }
    for (int lightCnt = 0; lightCnt < sizeof(trafficlightPins) / sizeof(trafficlightPins[0]); lightCnt++)
    {
      for (int colorCnt = 0; colorCnt < sizeof(trafficlightPins[0]); colorCnt++)
      {
        digitalWrite(trafficlightPins[lightCnt][colorCnt], ledStates[ledstateIndex].lightstates[lightCnt][colorCnt]);
        Serial.print("Traffic light "); Serial.print(lightCnt);
        Serial.print(" pin "); Serial.print(trafficlightPins[lightCnt][colorCnt]);
        Serial.print(" set to "); Serial.println(ledStates[ledstateIndex].lightstates[lightCnt][colorCnt] == ON ? "ON" : "OFF");
      }
    }
  }
} // end of loop

The code checks if it’s time based on the duration in the currently active ledState.

I hope this gets you on the way; not sure if it’s 100% bugfree.