LED flashing (timing) question

[amazed]

Hello,

Why is this code results a sometime visible lag in the LED flash (mainly in the off state)?

bool ledActive = false;
unsigned long ledTimer = 0ul;

void setup()
{
  // put your setup code here, to run once:
  Serial.begin(57600);
  pinMode(LED_BUILTIN, OUTPUT);
}

void loop()
{
  if ( (ledActive == false) && ((millis() - ledTimer) >= 100ul) )
  {
    ledActive = true;
    digitalWrite(LED_BUILTIN, HIGH);
     //Serial.println( millis() - ledTimer);
    ledTimer = millis();
  }
  else if ( (millis() - ledTimer) >= 100ul )
  {
    ledActive = false;
    digitalWrite(LED_BUILTIN, LOW);
    //Serial.println( millis() - ledTimer);
    ledTimer = millis();
  }
}

Is it related with millis() correction?

If I check for ledActive in the else branch, the flashing is very consistent.

bool ledActive = false;
unsigned long ledTimer = 0ul;

void setup()
{
  // put your setup code here, to run once:
  Serial.begin(57600);
  pinMode(LED_BUILTIN, OUTPUT);
}

void loop()
{
  if ( (ledActive == false) && ((millis() - ledTimer) >= 100ul) )
  {
    ledActive = true;
    digitalWrite(LED_BUILTIN, HIGH);
     //Serial.println( millis() - ledTimer);
    ledTimer = millis();
  }
  else if ( (ledActive == true) && ((millis() - ledTimer) >= 100ul) )
  {
    ledActive = false;
    digitalWrite(LED_BUILTIN, LOW);
    //Serial.println( millis() - ledTimer);
    ledTimer = millis();
  }
}

Thanks in advance.

[alto777]

Step yourself through the two versions.

Or draw flowcharts of the two versions.

Or any way you can get your head around the structure of your statements and how if-else works.

The two versions are different, logically, and operate differently.

There is no mystery.

a7

[amazed]

You are right, I just had to make myself think about it. But writing about it also helped.

The "lag" with the first version occures if 100ms elapses just between the two condition check, right?
( millis() update is interrupt based )

[alto777]

Well, I tried to do what I recommended to you and it made my brain hurt.

The below is more typical structure:

void loop()
{
  if ((millis() - ledTimer) >= 100ul) {    // is it time to do something?

    if (ledActive == false)  {
      ledActive = true;
      digitalWrite(LED_BUILTIN, HIGH);
      //Serial.println( millis() - ledTimer);
    }
    else {             // ledActive == true here, because it's not false
      ledActive = false;
      digitalWrite(LED_BUILTIN, LOW);
      //Serial.println( millis() - ledTimer);
    }
  }


  ledTimer = millis();  // set timer for next action

}

So one decision that it is time to do something, then inside that some logic to decide what to do since it is time to do something.

Then you are checking with millis() only once, and always updating you ledTimer.

a7

[drmpf]

Have a look at my tutorial on Multi-tasking in Arduino
and How to write Timers and Delays in Arduino
Each loop you call every task and in the task check if there is something to do. e.g. has the timer timed out. Don't mix other logic conditions with the timer condition. Use the time to set the flags for this or some other task

[amazed]

Yes, I got to the same conclusion, that I overcomplicated the simple "Blink without delay" example.

But the sketch of yours also has a flaw.

Corrected version:

void loop()
{
  if ((millis() - ledTimer) >= 100ul) {    // is it time to do something?

    if (ledActive == false)  {
      ledActive = true;
      digitalWrite(LED_BUILTIN, HIGH);
    }
    else {             // ledActive == true here, because it's not false
      ledActive = false;
      digitalWrite(LED_BUILTIN, LOW);
    }

    //timestamp should be stored inside the timer if condition
    ledTimer = millis();  // set timer for next action**
  }
}

[alto777]

@amazed

But the sketch of yours also has a flaw.

Yes, yes it does. Good catch.

I could say I did that just to see if you've earned your millis() badge - finding and fixing that is good evidence that you have done.

a7

[amazed]

Well played.