Using millis() to toggle an LED every 500 milliseconds

Question

I thought the code below would toggle an LED every 500 milliseconds. But it does not work. I'm not able to catch the mistake, which must be very silly.

void setup() {
  pinMode(3, OUTPUT);
}

void loop() {
  unsigned long cnt=millis();
  if(!(cnt % 500)) {
    PORTD^= (1<<PD3);
  }
}

Anyhow the code below works. But why not the previous code?

void loop() {
  static unsigned long prev=0;
  // Note: First time case is not handled. As soon as we want to toggle,
  // we must store millis() in prev. In this case this happens after reset and may not be an issue.
  unsigned long current = millis();
  if((unsigned long)(current - prev) >= 500) {
    PORTD^= (1<<PD3);
    prev = current;
  }
}

Answer 1

if (!(millis() % 500)) ...

There are two issues with this. The first, and most obvious, is that the condition will be true for a full millisecond. During that millisecond you will be toggling the LED on and off very fast. Whether you end up doing an odd or even number of toggles is anyone's guess.

The second and less obvious problem is that millis() skips some values. Roughly one integer every 42 is skipped over. This is because the millis() counter is incremented once every 1024 µs, getting 24 µs late on each increment. Once all those delays add up to a full millisecond, millis() catches up by incrementing the counter by two milliseconds instead of one. This is why you should never condition something on millis() having one specific value.

The almost correct solution to this problem is shown in Juraj's answer:

if (millis() > next) ...

This is not quite correct because it fails each time millis() rolls over back to zero. The correct solution is

static unsigned long last;
if (millis() - last >= 500) {
    last += 500;
    ...
}

Because the subtraction follows the rules of modular arithmetic, this is guaranteed to work across the rollover events.

Answer 2

Your timing doesn't work, because it has a low probability to read a millisecond that is an exact multiple of 500.

Do it this way:

  static unsigned long next = millis();
  if (millis() - next > 500) {
    next += 500;

Answer 3

void loop() {
  unsigned long cnt=millis();
  if(!(cnt % 500)) {
    PORTD^= (1<<PD3);
  }
}

At every iteration, cnt is set with the number of milliseconds elapsed since you turned on the Arduino... So this is not gonna work. At all. What you need is two variables. One that holds the last millis() count when you blinked the LED, and for the current millis() count:

unsigned long lastCount;
void setup() {
  pinMode(3, OUTPUT);
  lastCount=millis();
}

void loop() {
  unsigned long cnt=millis();
  if((cnt-lastCount)>499) {
    PORTD^= (1<<PD3);
    lastCount=cnt;
  }
}

Answer 4

I always use this

if((millis()/500)%2==0)) 
{
    PORTD |= (1<<PD3);
}
else
{
    PORTD &= ~(1<<PD3);
}

It first divides millis by 500ms. So you get the number of 500ms chucks (since the Arduino started). Then you check to see if this number is even. If so, turn the led pin on. Else turn the led pin off.