Blink function with millis() for multiple LEDs

Hi Arduino folks,

I wrote a sketch that makes two (or more) LEDs blink a set number of times independantly. I need help cleaning it up, or making it more elegant / less clustered, since I know for a fact that my approach is very immature - especially the part with the state variable.

What I’m trying to do:
For example, for a puzzle in an Escape Room Game I need LED1 to blink 3 times, then 6 times and then 4 times. The gap (800) inbetween these “digits” is twice as long as the interval between the blinks (400). At the end there should be a longer pause (3000), and then it starts from the beginning.

What I was aiming for:
A function that I could use like this.
blink(pin, 3, 6, 4, blinkGap, pauseGap);
Ideally it would work for multiple LEDs, that each blink independantly with their own pattern.

Here’s what I have so far:
(For better readability I deleted the second function for the second LED - it’s just a copy of the first one with different variables ugh)

#define LED1 7
int state1 = 1;                         // Used to control the steps
unsigned long previousMillis1 = 0;
unsigned long counter1 = 0;             // Used to count the number of blinks


void setup() {
  Serial.begin(9600);
  pinMode(LED1, OUTPUT);
}

void loop() {
  blink1(LED1, 3, 6, 4, 400, 800);
}

void blink1(int pin, int num1, int num2, int num3, const long blinkGap, const long pauseGap) {
  if (state1 == 1) {                                           // Number 1
    if (counter1 < num1 * 2) {                                 // *2 because I'm counting on and off blinks
      for (int i = 0; i < num1 * 2; i++) {
        if (millis() - previousMillis1 >= blinkGap) {
          previousMillis1 = millis();
          digitalWrite(pin, !digitalRead(pin));
          counter1 += 1;
        }
      }
    } else {
      counter1 = 0;
      state1 = 2;   // Go to pause section                                           
    }
  }
  if (state1 == 2) {                                           // Pause 1
    if (millis() - previousMillis1 >= pauseGap) {
      previousMillis1 = millis();
      state1 = 3;   // Go to second "digit"
    }
  }
  if (state1 == 3) {                                           // Number 2
    if (counter1 < num2 * 2) {
      for (int i = 0; i < num2 * 2; i++) {
        if (millis() - previousMillis1 >= blinkGap) {
          previousMillis1 = millis();
          digitalWrite(pin, !digitalRead(pin));
          counter1 += 1;
        }
      }
    } else {
      counter1 = 0;
      state1 = 4;
    }
  }
  if (state1 == 4) {                                           // Pause 2
    if (millis() - previousMillis1 >= pauseGap) {
      previousMillis1 = millis();
      state1 = 5;
    }
  }
  if (state1 == 5) {                                           // Number 3
    if (counter1 < num3 * 2) {
      for (int i = 0; i < num3 * 2; i++) {
        if (millis() - previousMillis1 >= blinkGap) {
          previousMillis1 = millis();
          digitalWrite(pin, !digitalRead(pin));
          counter1 += 1;
        }
      }
    } else {
      counter1 = 0;
      state1 = 6;
    }
  }
  if (state1 == 6) {                                           // Pause 3 (long)
    if (millis() - previousMillis1 >= 3000) {
      previousMillis1 = millis();
      state1 = 1;
    }
  } 
}

PROBLEMS

  • Right now it only works for one LED, since I need different state, previousMillis and counter variables. I know that just copying and renaming the function would not be the smartest way. What else could I do?
  • Is there a way around the if state - structure? I tried a switch too, but essentially it does the same thing.

What I can’t wrap my head around:
How can I control the flow “1st digit, pause, 2nd digit, pause, 3rd digit, long pause” in a non-blocking way? I cannot use while-loops or delay() since I need multiple LEDs blinking independantly.

I appriciate any input or push in the right direction. Please excuse my noob coding, I’ve just started out and am eager to learn how to write better code.

Object base programming is the answer to problem one. Just wrap it into a class and you can do it multiple times.

And part two you can make it easier by splitting the task further! Basic, what you want to do is blink x times and wait which you want to do 3 times. See how the code for state == 1, 3 and 5 is kind of the same? Make life easy, make a function which does that and call that three times :wink:

I once made a library what kind of does what you want but I never finished it for real. I posted it here before but I attached it. I really need to release it :slight_smile:

BlinkX.cpp (2.93 KB)

BlinkX.h (1.28 KB)

  • Right now it only works for one LED, since I need different state, previousMillis and counter variables. I know that just copying and renaming the function would not be the smartest way. What else could I do?

This is not a complete solution to your question but might help you

//blink 3 LEDs, each with different periods.  Non blocking
const byte ledPins[] = {10, 11, 12};
const byte NUMBER_OF_LEDS = sizeof(ledPins) / sizeof(ledPins[0]);
unsigned long startTimes[3];
unsigned long periods[] = {300, 500, 700};

void setup()
{
  for (int led = 0; led < NUMBER_OF_LEDS; led++)
  {
    pinMode(ledPins[led], OUTPUT);
  }
}

void loop()
{
  unsigned long currentTime = millis();
  for (int led = 0; led < NUMBER_OF_LEDS; led++)
  {
    if (currentTime - startTimes[led] >= periods[led])
    {
      digitalWrite(ledPins[led], !digitalRead(ledPins[led]));
      startTimes[led] = currentTime;
    }
  }
}

While I had the sketch loaded to copy it into the previous reply I decided to extend it to allow for different on/off periods for each LED

//blink 3 LEDs, each with different periods on/off - non blocking

const byte ledPins[] = {10, 11, 12};
const byte NUMBER_OF_LEDS = sizeof(ledPins) / sizeof(ledPins[0]);
unsigned long startTimes[NUMBER_OF_LEDS];
unsigned long periods[][NUMBER_OF_LEDS] = { {500, 300, 100}, {500, 200, 450} };  //on/off periods

void setup()
{
  for (int led = 0; led < NUMBER_OF_LEDS; led++)
  {
    pinMode(ledPins[led], OUTPUT);
  }
}

void loop()
{
  unsigned long currentTime = millis();
  for (int led = 0; led < NUMBER_OF_LEDS; led++)
  {
    byte ledState = digitalRead(ledPins[led]);
    if (currentTime - startTimes[led] >= periods[ledState][led])
    {
      digitalWrite(ledPins[led], !ledState);
      startTimes[led] = currentTime;
    }
  }
}

@septillion
Thanks a bunch for bringing up the idea with a class and your library. I’ll definitely have to learn what a class is and how to use it. I looked at your library, and right now it’s still mostly alien language to me :smiley: Can’t wait to work through it though and try to understand what’s going on.

@UKHeliBob
Thanks for the hint with the arrays. In your code the LEDs do blink at different intervals. However, the tricky part for me was, how i can control the different “digits” and pauses after each one.

I worked out a soulution with arrays that works like a charm and somewhat looks and feels better than my first attempt. I used some of your code, UKHeliBob - thanks for that! Here goes.

const byte ledPins[] = {6, 7};
const byte nrLeds = sizeof(ledPins) / sizeof(ledPins[0]);
const int nrDigits = 3;
const long interval =   400;
const long shortPause = 800;
const long longPause =  3000;
unsigned long startTimes[nrLeds] = {0};
unsigned long counters[nrLeds] = {0};
int states[nrLeds] = {1, 1};
int blinks[nrLeds][nrDigits] = { {3, 6, 4}, {5, 2, 3} };

void setup() {
  Serial.begin(9600);
  for (int led = 0; led < nrLeds; led++)
  {
    pinMode(ledPins[led], OUTPUT);
  }
}

void loop() {
  blinkMaster(0);
  blinkMaster(1);
}

void blinkMaster(int led) {
  switch (states[led]) {
    case 1:                 // Digit 1
      {
        blinking(led, 0);
        break;
      }
    case 2:                 // Digit 2
      {
        blinking(led, 1);
        break;
      }
    case 3:                 // Digit 3
      {
        blinking(led, 2);
        break;
      }
    case 4:                 // Long Pause
      {
        if (millis() - startTimes[led] >= longPause) {
          startTimes[led] = millis();
          counters[led] = 0;
          states[led] = 1;
          break;
        }
      }
  }
}

void blinking(int led, int digit) {
  if (counters[led] < blinks[led][digit] * 2) {
    if (millis() - startTimes[led] >= interval) {
      startTimes[led] = millis();
      digitalWrite(ledPins[led], !digitalRead(ledPins[led]));
      counters[led] += 1;
    }
  } else {
    if (millis() - startTimes[led] >= shortPause) {
      startTimes[led] = millis();
      counters[led] = 0;
      states[led] += 1;
    }
  }
}

I used some of your code,

I will, of course, expect payment at the rates quoted in reply #2, but don't check back too soon as I have not edited it yet :slight_smile:

More seriously, I am glad you got it working the way you want.

UKHeliBob:
More seriously, I am glad you got it working the way you want.

I know OP got it working, but as pointed out above, delivering an Object Oriented solution may have been cleaner and easier (if you happen to know how to build a class from your example).

he's 1 yard from the goal line!

he's 1 yard from the goal line!

Agreed that an OOP approach would be better but he has achieved his goal. You and I have discussed this type of thing before, but my approach would be to get it working then make it work better.

UKHeliBob:
...my approach would be to get it working then make it work better.

how about a better way of just getting it working?

:wink:

Thanks BulldogLowell for the incentive to do it the OOP way.
I had no idea what OOP was, and I’m still not sure I did everything correctly. But it works just as well :slight_smile:

class Blinker
{
    byte ledPin;
    int digit1;
    int digit2;
    int digit3;
    long interval;
    long shortPause;
    long longPause;
    unsigned long startTime;
    int counter;
    int state;

  public: Blinker(int pin, int num1, int num2, int num3)
    {
      interval = 400;
      shortPause = 800;
      longPause = 3000;
      startTime = 0;
      counter = 0;
      state = 1;
      ledPin = pin;
      pinMode(ledPin, OUTPUT);
      digit1 = num1;
      digit2 = num2;
      digit3 = num3;
    }

    void flash(int digit) {
      if (counter < digit * 2) {
        if (millis() - startTime >= interval) {
          startTime = millis();
          digitalWrite(ledPin, !digitalRead(ledPin));
          counter += 1;
        }
      } else {
        if (millis() - startTime >= shortPause) {
          startTime = millis();
          counter = 0;
          state += 1;
        }
      }
    }

    void blinking()
    {
      switch (state) {
        case 1:                 // Digit 1
          {
            flash(digit1);
            break;
          }
        case 2:                 // Digit 2
          {
            flash(digit2);
            break;
          }
        case 3:                 // Digit 3
          {
            flash(digit3);
            break;
          }
        case 4:                 // Long Pause
          {
            if (millis() - startTime >= longPause) {
              startTime = millis();
              counter = 0;
              state = 1;
              break;
            }
          }
      }
    }
};

Blinker led1(6, 3, 6, 4);
Blinker led2(7, 5, 2, 3);

void setup() {
}

void loop() {
  led1.blinking();
  led2.blinking();
}

gandarufu:
Thanks BulldogLowell for the incentive to do it the OOP way.
I had no idea what OOP was, and I'm still not sure I did everything correctly. But it works just as well :slight_smile:

Good for you for going further with this but I think that it makes my point about getting it working first then improving it.

As a matter of interest, how easy do you think it would have been to write it using OOP if you had not got it working the other way first ?

I note that it works

just as well

and not any better, not that I would expect it to.

gandarufu:
Thanks BulldogLowell for the incentive to do it the OOP way.
I had no idea what OOP was, and I’m still not sure I did everything correctly. But it works just as well :slight_smile:

  public: Blinker(int pin, int num1, int num2, int num3)

{
     interval = 400;
     shortPause = 800;
     longPause = 3000;
     startTime = 0;
     counter = 0;
     state = 1;
     ledPin = pin;
     pinMode(ledPin, OUTPUT);
     digit1 = num1;
     digit2 = num2;
     digit3 = num3;
   }

It’s great to see you take the initiative.

One point to keep in mind. You are using pinMode in your constructor. Important to remember that the hardware is not guaranteed to be ready due to the nuances of the order of object creation. As you may know, with the Arduino IDE, you’re working with C++ where the main() and some other functions have been abstracted away. This is the real C++ main() function:

int main(void)
{
 init();

 setup();
   
 for (;;)
 loop();
       
 return 0;
}

You see, since init() does all of the hardware setup, objects may or may not be created before init(). So one way to accomplish this (and you see this a lot) is to add a new method, something like this:

void Blinker::begin(){
  pinMode(ledPin, OUTPUT);
}

or, you can pull out the ledPin from the constructor and add a method to attach the ledPin in setup() like this:

void Blinker::bindLedToPin(uint8_t pin){
  ledPin = pin;
  pinMode(ledPin, OUTPUT);
}

Just something to consider in case down the road you get snookered trying to figure out why your led won’t light!!

UKHeliBob:
…how easy do you think it would have been to write it using OOP if you had not got it working the other way first ?

a better question may be: “how much more was learned taking the extra step of making a class?”

as demonstrated by the OP, lots of people like the challenges that learning brings :wink:

as demonstrated by the OP, lots of people like the challenges that learning brings

Very true, but lots of people would baulk at the idea of writing an OOP solution as the first pass at solving a problem because the learning curve was too steep, hence my approach of creeping up on the solution. But where do you stop ? For instance, why not use direct port manipulation instead of the Arduino functions ?

I think both of you have a point. Honestly, at first I thought to myself "Ugh, why should I look into OOP when the previous solution just works fine?". But then I was glad that I looked into it and learned how to solve the same problem in three different ways. I enjoyed it immensly and I'm glad I got so much valuable input!
I think I'll pass on the port manipulation though :wink:

I think I’ll pass on the port manipulation though

Why ?

Come to that, why not write it in assembler :slight_smile:

UKHeliBob:
Why ?

Come to that, why not write it in assembler :slight_smile:

Or hex. :slight_smile: