Modified Blink demo - Problems with millis() and % (modulo)

I have spent hours / days trying to figure out why this doesn’t work correctly to no avail.

All I am trying to do is to blink the LED at 50% duty cycle once every second, without using delay() so that loop() doesn’t block, thereby allowing other things to occur asynchronously in said loop() function… ie if(test1) dothing(1), if(test2) dothing(2). etc…
Whether or not I can code the other aspects of my project using the asynchronous concept and use the return values of millis() reliably will make or break my project.
This should be absolutely trivial to do, but doesn’t work as expected.

The problem:
millis() % 1000 == 0 doesn’t seem to be calculated correctly, therefore ledState = !ledState is evaluated, at the wrong times as the system clock continues to increment.
As a result the duty cycle of the LED varies. sometimes as short as 1ms, others as long as 1999 ms, and sometimes 1000ms (likely 999 or 1001ms). The times are my guesstimate based on visual observation.

I think that the registers that __udivmodsi4 (the routine that the compiler uses to implement % ) uses are getting clobbered by the timer0 ISR, but I’m not sure…

I am using Arduino 22 and a ladyada Boarduino Boarduino - Breadboard-compatible Arduino Clone (Doemalenove ATmega328p).

Non-Blocking Blink sketch

unsigned char ledState;

void setup()
{
  pinMode(13,OUTPUT);
  ledState = 0;
}

void loop()
{
  if((millis() % 1000) == 0)
    ledState = !ledState;
    
   digitalWrite(13,(ledState != 0));
}

I have also pulled and built the avr tool chain gcc 4.5.2, binutils 2.21, and avrlibc 1.6.8 and built a from scratch code project that doesn’t use the Arduino environment, with the same results.

The only way that I can get this to work correctly is to put the modulo calculation test for ledState inside of the timer0 ISR, which then works flawlessly, lending credence to my theory that the registers are getting clobbered by the ISR.

#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <util/atomic.h>
        
#define BIT(x) (1<<(x))
#define BITSET(a,b) ((a) |= (b))
#define BITCLR(a,b) ((a) &= ~(b))
#define BITIS(a,b) ((a) & (b) != 0)
                
volatile unsigned char ledState = 0;

// follow the arduino model for timer 0 frequencies 
#define CLOCK_CYCLES_TO_US(cc) ( ((cc) * 1000L) / (F_CPU / 1000L) )
#define US_TO_CLOCK_CYCLES(us) ( ((us) * (F_CPU / 1000L)) / 1000L )
#define TIMER0_PRESCALE 64
#define TIMER0_US_PER_OVERFLOW ( CLOCK_CYCLES_TO_US(TIMER0_PRESCALE * 256) )
#define TIMER0_INC_MS_PER_TICK ( TIMER0_US_PER_OVERFLOW / 1000 )
#define TIMER0_INC_FRAC_MS_PER_TICK ( (TIMER0_US_PER_OVERFLOW % 1000) >> 3)
#define TIMER0_INC_FRAC_MAX_MS_PER_TICK ( 1000 >> 3 )
volatile unsigned long timer0_ticks = 0; 
volatile unsigned long timer0_ms = 0;
volatile unsigned char timer0_fracms = 0;
ISR(TIMER0_OVF_vect)
{
        unsigned long ms = timer0_ms;
        unsigned char fms = timer0_fracms;
        
        ms += TIMER0_INC_MS_PER_TICK;
        fms += TIMER0_INC_FRAC_MS_PER_TICK;
        if(fms >= TIMER0_INC_FRAC_MAX_MS_PER_TICK)
        { 
                fms -= TIMER0_INC_FRAC_MAX_MS_PER_TICK;
                ms ++;
        }
        
        timer0_ms = ms;
        timer0_fracms = fms;
      timer0_ticks ++; 

//      if(timer0_ms % 1000 == 0) 
//              ledState = 1 - ledState; 
}

unsigned int ismillis(unsigned int modulo)
{       unsigned isEven;

        cli();
        isEven = ((timer0_ms % modulo) == 0);
        sei();

        return isEven;
}

int main(void)  
{
        DDRB |=  _BV(5);  // PORTB0 is output 

        // increment timer every 64 clock cycles 
        BITSET(TCCR0B, BIT(CS01) | BIT(CS00));
        BITSET(TIMSK0, BIT(TOIE0)); // turn on overlow interrupt for timer 0 
        sei(); // turn on interrupts 

        while(1)
        {
                if(ismillis(1000))   
                        ledState = 1 - ledState;
        
                if(ledState)
                        PORTB |=  _BV(5);       // LED on 
                else
                        PORTB &= ~_BV(5);       // LED off 
        }
          
        return 0;
}

Does anyone know how to best deal with this?
Any help at all would very greatly appreciated.

Thanks!

Why it will never work…

This is a good example of how to do it correctly…

This:
if((millis() % 1000) == 0)
ledState = !ledState;

will be true for 1ms every second. During that 1ms, loop() might be run several times, once, or maybe not at all, depending on how complex it is. Given that your loop is fairly simple I expect it is run several times. Each time it is run ledState is inverted. The result would be pretty much random.

Look at the BlinkWithoutDelay example to see the correct way of doing it.

Wow, me stupid monkey...

will be true for 1ms every second

hit the nail on the head.
I am very embarrassed given the obviousness of that...

Thank you stimmer for the clue hammer.

with head down, walks back to corner, mumbles, stupid monkey...

You can achieve this using:

void loop()
{
  if((millis() % 2000) > 1000){
    ledState = 1;
  } else {
    ledState = 0;
  } 
  digitalWrite(13, ledState);
}