0

I wrote a function to blink an led using millis and want to blink multiple leds with the same function but with different delay intervals. When I call this function thrice to blink 3 different leds, all the leds blink with the shortest delay. As far my understanding is, functions can be reused if you want to perform same task but why doesn't it work in this case? It works incase I rename the function before calling it again.

Is it wrong to call single function more than once to perform same task but on different pins? How would you call a function without renaming it?

const int redLed = 10;
const int yellowLed = 11;
const int greenLed = 12;

void setup() {
  pinMode(redLed, OUTPUT);
  pinMode(yellowLed, OUTPUT);
  pinMode(greenLed, OUTPUT);
}

void loop() {
blinkLed(redLed, 1000);
blinkLed(yellowLed, 4000);
blinkLed(greenLed, 8000);
}

void blinkLed(int led, unsigned long timePeriod) {
  static unsigned long oldTime = 0;
  unsigned long newTime = millis();
  static boolean ledState = LOW;

  if (newTime - oldTime >= timePeriod) {
    oldTime = newTime;
    ledState = !ledState;
  }
  digitalWrite(led, ledState);
}

Edit: I have modified the function to not have shared variables but it just blinks redLed. why doesn't it work now? Here is the code:

const int redLed = 10;
const int yellowLed = 11;
const int greenLed = 12;

void setup() {
  pinMode(redLed, OUTPUT);
  pinMode(yellowLed, OUTPUT);
  pinMode(greenLed, OUTPUT);
}

void loop() {

  blinkLed(redLed, 1000);
  blinkLed(yellowLed, 4000);
  blinkLed(greenLed, 8000);
}

void blinkLed(int led, unsigned long timePeriod) {

  switch (led) {
    case 10:
      static unsigned long oldTime1 = 0;
      unsigned long newTime1 = millis();
      static boolean redState = LOW;

      if (newTime1 - oldTime1 >= timePeriod) {
        oldTime1 = newTime1;
        redState = !redState;
      }
      digitalWrite(led, redState);
          break;

    case 11:
      static unsigned long oldTime2 = 0;
      unsigned long newTime2 = millis();
      static boolean yellowState = LOW;


      if (newTime2 - oldTime2 >= timePeriod) {
        oldTime2 = newTime2;
        yellowState = !yellowState;
      }
      digitalWrite(led, yellowState);
          break;

    case 12:
      static unsigned long oldTime3 = 0;
      unsigned long newTime3 = millis();
      static boolean greenState = LOW;


      if (newTime3 - oldTime3 >= timePeriod) {
        oldTime3 = newTime3;
        greenState = !greenState;
      }
      digitalWrite(led, greenState);
      break;
  }
}
1

You want to let the LEDs blink concurrently.
Your static variables oldTime, and ledState are shared between the three LEDs.
So there is ony ONE ledState in your program but you need that state for each one.
Same applies for oldTime.

Resolve the this issue and your LEDs will blink as expected.

btw. you don't need the ledState as you can read its current state.

4
  • the led pin is defined as OUTPUT, so wouldn't I be reading what I am writing to the pin? Also how does the pin defined as output work as input at the same time?
    – Zaffresky
    Apr 17 '20 at 11:31
  • @Zaffresky if you are readin an output pin, you are reading, what you have written to it. But thats exactly, what you need, since you simply want to toggle the pin
    – chrisl
    Apr 17 '20 at 11:38
  • To resolve the shared variables issue I modified my function but there are other problems now. Scratching my head now about how to fix it. May be the easiest way without creating classes (need to learn it first) is just copy function with different names and calling them.
    – Zaffresky
    Apr 17 '20 at 12:14
  • 1
    @Zaffresky that is bruteforcing your idea into code. that is far from good coding habbits. Classes are easy if done primitively. Take a look at Majenko's Blinker Code. You really Need only to track three states (one for each LED) and those states consist of at most two variables. You can use a struct with those two variables if you feel that way. Just pass it as a pointer or reference to your blinkLed function.
    – Kwasmich
    Apr 17 '20 at 12:20
1

Yes, functions can be re-used if you want to perform the same task in different places or at different times. But your problem is that you only have one set of "time" variables.

You would be better off learning to create this as a class which can not only contain the "blink" function but also the variables that define the state for a specific instance of the blinking.

For example here is a small library I wrote ages ago as a demonstration.

2
  • I thought with each function call, a new copy of variables will be created and managed independently then previous function call and previous values of variables therein. I don't know anything about classes so will look it up
    – Zaffresky
    Apr 17 '20 at 11:19
  • 1
    No, one function = one set of variables. You could pass references to other local variables, but that gets messy. Simpler to use a class.
    – Majenko
    Apr 17 '20 at 11:32
0

Just putting other peoples' suggestions together:

void blinkLed(int led, unsigned long timePeriod) {
  static unsigned long oldTime[42];
  unsigned long newTime = millis();
  boolean ledState = digitalRead(led);

  if (newTime - oldTime[led] >= timePeriod) {
    oldTime[led] = newTime;
    ledState = !ledState;
  }
  digitalWrite(led, ledState);
}
4
  • Nah, sort of. it will work but still not a nice solution. The all known magic number 42 is really not what you want to do. You'd rather create a struct or class that encapsulates: pin, timePeriod, oldTime and ledState. The function will then take only the state object as its only argument.
    – Kwasmich
    Apr 17 '20 at 12:55
  • 1
    Other people excluding Kwasmich*
    – Abel
    Apr 17 '20 at 13:05
  • @Abel why did you chose 42 for OldTime? And are you defining oldTime as an array?
    – Zaffresky
    Apr 17 '20 at 20:09
  • Making a the singular value oldTime into an array is the easiest way to scale the code. Ideally 42 would be replaced by the max numbered possible digital out pin per the arduino header or simply the max of the used port +1 aka 13. Array because sacrificing mem for simplicity over implementing class or map.
    – Abel
    Apr 17 '20 at 22:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.