millis() not in sync with 8MHz system clock?

Experimenting with an ATmega328P on a breadboard. Programming is via a USBtinyISP. Blinking one LED with millis() and another with Timer/Counter1. With a 16MHz system clock, the two LEDs stay in sync indefinitely. At 8MHz, after a few minutes, the LED driven with millis() is visibly behind the hardware-driven LED.

Is this the expected behavior? Or do I have a code problem?

//Toggle two LEDs once per second.
//One LED on pin 9, controlled by Timer1,
//another on pin 13, controlled by millis().
//ATmega328P @ 16MHz, external crystal, fuses L/H/E = 0xFF/0xDE/0x05
//ATmega328P @ 8MHz, internal RC oscillator, fuses L/H/E = 0xE2/0xD1/0x07

void setup(void)
{
    DDRB = _BV(DDB5);            //set PB5 as output (Arduino pin D13, DIP pin 19)
    DDRB |= _BV(DDB1);           //set OC1A/PB1 as output (Arduino pin D9, DIP pin 15)
    TCCR1B = 0;                  //stop the timer
    TCCR1A = _BV(COM1A0);        //toggle OC1A on compare match
    #if F_CPU == 16000000
    OCR1A = 62499;               //top value for counter
    #elif F_CPU == 8000000
    OCR1A = 31249;               //top value for counter
    #else
    #error F_CPU must be 16MHz or 8MHz only
    #endif    
    TCCR1B = _BV(WGM12) | _BV(CS12);   //CTC mode, prescaler clock/256, start the timer
}

void loop(void)
{
    static unsigned long msLast;
    
    unsigned long ms = millis();
    if (ms - msLast >= 1000) {
        msLast = ms;
        PINB = _BV(PINB5);
    }
}

msLast = msLast + 1000;

Thanks, that fixes it all right, but I don't see why. Even at 8MHz, I figure that loop() runs fast enough to oversample millis() by at least a factor of 50. Yet with a print statement I can clearly see that it's slipping a millisecond every two or three seconds. This does not occur at 16MHz. What am I missing?

I do the same for my timekeeping, with crystal equipped 328's is very accurate, and with 10ppm crystals it stays dead on.

The way Coding Badly I think it was explained it to me is this: you capture millis(), and you advance when you exceed some count. If you just increment from that exceeded count, it only gets worse. If you ignore the time when you exceeded and increment by a known amount, then you keep the count dead on, say at 1000-2000-3000-4000, and if your capture was at 1002, 2003, 3004, it doesn't matter because you are still looking for 1000, 2000, 3000, 4000 at the next time check, and not for 1002, 2003, 3004 and a slowly increasing off amount.

For the 16 MHz test, does this make any difference... (pointless test removed)

CrossRoads: The way Coding Badly I think it was explained it to me is this:

As long as the loop time is significantly less than the target (1000 ms) and a clean snapshot is used (only call millis once) either way should work the same.

Your suggestion is definitely the better choice when implementing something like a time-of-day clock or a stopwatch (technically it should be a while instead of an if but most folks don't have loop that badly overloaded to need a while).

Oh, I nearly forgot to add: that was an excellent suggestion!

Argh! You are absolutely correct @CrossRoads! Essentially, the problem is that millis does not always increment by 1.

Thanks. You were the one who explained it to me back in 2011, yes? I'd dig it up, but that's like 1900 pages of posts ago and the search tool is not the greatest in that regards.

I use micros() for time keeping instead of millis(), I think I've demonstrated to myself by letting the time track against the official US time that it stays right in step that way, letting a crystal equipped duemilanove run over several days. http://www.time.gov/

off to bed ...

Essentially, the problem is that millis does not always increment by 1.

The T0 overflow isr increments in 1.024ms intervals (tick). There is a calculus inside the isr which makes the adjusting ticks to millis(). That is not linear within an one second interval probably, but precise in long run. If you want a precise timekeeping you may consider the zero drift clock which works for any crystal oscillator frequency - see my topic in other sw dev.

Thanks for all the input, guys. I can see that at 16MHz, millis() increases by 1 most of the time, and occasionally by 2. At 8MHz, it increases by 2, and occasionally by 3. Can't quite put my thumb on it, but the way I had it coded never slips at 16MHz. Maybe the nature of the adjustment is that it comes out even at 16MHz, but not at 8MHz. Regardless, Crossroads' technique is obviously the correct one.

FYI, I wasn't concerned about synchronization with any external time standard, I was simply looking at millis() relative to the system clock. I suppose that all the same considerations, plus more, would apply if I were synchronizing with an external standard.

I realize this is an old thread, but someone might find useful the code I just wrote to make milllis() always accurately increment by 1 on an ATMega328P (the Arduino UNO processor) with an 8 Mhz clock. I basically made a special version of wiring.c that changes the way timer0 is used.

// This is a modified and simplified version of wiring.c, only for an ATMega328P 
// running at 8 Mhz, to make millis() be accurate to 1 msec instead of 2+ msec.
//   L. Shustek, 4/29/2020

#if F_CPU != 8000000
   This is only for 8 Mhz
#endif

#ifndef __AVR_ATmega328P__
   This is only for AtMega328P
#endif

/*
   wiring.c - Partial implementation of the Wiring API for the ATmega8.
   Part of Arduino - http://www.arduino.cc/

   Copyright (c) 2005-2006 David A. Mellis

   This library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2.1 of the License, or (at your option) any later version.

   This library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General
   Public License along with this library; if not, write to the
   Free Software Foundation, Inc., 59 Temple Place, Suite 330,
   Boston, MA  02111-1307  USA
*/

#include "wiring_private.h"

// the prescaler is set so that timer0 ticks every 64 8 Mhz clock cycles, and the
// overflow handler is now called every 125 ticks, or precisely every 1 msec.

#define TOP_LIMIT 124  // 0..124 is 125 states

volatile unsigned long timer0_micros = 0;
volatile unsigned long timer0_millis = 0;

ISR(TIMER0_COMPA_vect) { // Timer0 compare match A interrupt
   timer0_millis += 1; // register one more msec and 1000 more usecs
   timer0_micros += 1000; }

unsigned long millis() {
   unsigned long m;
   uint8_t oldSREG = SREG;
   // disable interrupts while we read timer0_millis or we might get an
   // inconsistent value (e.g. in the middle of a write to timer0_millis)
   cli();
   m = timer0_millis;
   SREG = oldSREG;
   return m; }

unsigned long micros() {
   unsigned long m;
   uint8_t oldSREG = SREG, t;
   cli(); // disable interrupts
   m = timer0_micros;
   t = TCNT0; // tick progress, each 8 usec, towards the next overflow
   if ((TIFR0 & _BV(OCF0A)) && (t < TOP_LIMIT))
      m += 1000; // an interrupt is pending but not yet taken
   SREG = oldSREG; // restore interrupts
   return m + ((unsigned int)t << 3); }

void delay(unsigned long ms) {
   #if 1 //the good (accurate) code
   uint32_t start = micros();
   while (ms > 0) {
      yield();
      while ( ms > 0 && (micros() - start) >= 1000) {
         ms--;
         start += 1000; } } }
   #else //the crude code (off by as much as 1 msec) that doesn't depend on micros()
   uint32_t start = millis();
   while (ms > 0) {
      yield();
      while (ms > 0 && (millis() - start) >= 1) {
         ms--;
         start += 1; } } }
   #endif

/* Delay for the given number of microseconds.  Assumes an 8 MHz clock. */
void delayMicroseconds(unsigned int us) {
   // call = 4 cycles + 2 to 4 cycles to init us(2 for constant delay, 4 for variable)

   // calling avrlib's delay_us() function with low values (e.g. 1 or
   // 2 microseconds) gives delays longer than desired.
   //delay_us(us);

   // for an 8 MHz clock

   // for a 1 and 2 microsecond delay, simply return.  the overhead
   // of the function call takes 14 (16) cycles, which is 2us
   if (us <= 2) return; //  = 3 cycles, (4 when true)

   // the following loop takes 1/2 of a microsecond (4 cycles)
   // per iteration, so execute it twice for each microsecond of
   // delay requested.
   us <<= 1; //x2 us, = 2 cycles

   // account for the time taken in the preceeding commands.
   // we just burned 17 (19) cycles above, remove 4, (4*4=16)
   // us is at least 6 so we can substract 4
   us -= 4; // = 2 cycles

   // busy wait
   __asm__ __volatile__ (
      "1: sbiw %0,1" "\n\t" // 2 cycles
      "brne 1b" : "=w" (us) : "0" (us) // 2 cycles
   );
   // return = 4 cycles
}

void init() {   // this needs to be called before setup()

   cli(); // disable interrupts

   // setup Timer0 for millis() interrupts at exactly 1.000 msec
   TCCR0B = 0;
   TCCR0A = 0;
   TCNT0 = 0; // zero count to avoid bogus wraparound
   OCR0A = TOP_LIMIT;  // count to 125, then restart and interrupt
   TIMSK0 = 0b00000010;  // enable OCRA interrupt
   TCCR0A = 0b00000010;  // no OCRA/B pin output; mode 2: CTC
   TCCR0B = 0b00000011;  // mode 2: CTC; clk/64 prescaling; GO!

   TCCR1B = 0; // turn off Timer1 and Timer2
   TCCR1A = 0;
   TCCR2B = 0;
   TCCR2A = 0;

   sbi(ADCSRA, ADPS2);  // set a2d prescaler so we are inside the desired 50-200 KHz range.
   sbi(ADCSRA, ADPS1);
   cbi(ADCSRA, ADPS0);
   sbi(ADCSRA, ADEN);    // enable a2d conversions

   // the bootloader connects pins 0 and 1 to the USART; disconnect them
   // here so they can be used as normal digital i/o; they will be
   // reconnected in Serial.begin()
   #if defined(UCSRB)
   UCSRB = 0;
   #elif defined(UCSR0B)
   UCSR0B = 0;
   #endif

   sei(); // enable interrupts
}

If you use this, you should put it in the sketch directory for your project as wiring.c, not in the general cores/arduino directory, because it will only work for an ATmega328P at 8 Mhz.

code I just wrote to make milllis() always accurately increment by 1 on an ATMega328P (the Arduino UNO processor) with an 8 Mhz clock.

Note that this will break PWM "analogWrite()" functionality on Pins 5 and 6, and probably vis-versa as well (ie if you use analogWrite() on one of those pins while this mod is in place.) How come you didn't do an exact 16MHz 1kHz while you were at it? It's an interesting tradeoff. I would think that a lot of applications might be happy to give up a couple of PWM outputs for the sake of a simplified millis(), and I really wish that the new ATmega4809 systems (which have a bunch of "extra" timers) has done something other than the overly-complex scheme that they seem to have actually implemented. Sigh.

LenShustek

The Len Shustek ??!! Wow! I thought I recognized the name from my old mainframe days, but I guess it's more likely it was from reading tSoaNM...

I've been thinking that the Arduino Ecosystem ought to be a good basis for something like an "Open Museum Electronics" project, making everything from touch-to-talk switches to relatively complex exhibit automation as lot more unified across the country.

westfw: Note that this will break PWM "analogWrite()" functionality on Pins 5 and 6, and probably vis-versa as well (ie if you use analogWrite() on one of those pins while this mod is in place.) How come you didn't do an exact 16MHz 1kHz while you were at it?

  • You are right about it breaking analogWrite() on pins 5 and 6, because that also uses timer 0. Thanks for pointing it out. PWM analog output on the other pins (3, 9, 10, and 11) will still work ok.
  • I forgot to subtract 1 from the TOP, so the frequency was off by 0.8%. I just fixed it above.
  • My application is for running on 3.3V with an 8 Mhz crystal. I'll let someone else implement and test a version for 16 Mhz.