millis not accurate?

@ CrossRoads after letting your thing run overnight (10 hours, it was 8 mins behind).

Trying Coding Badly's code with the while.

As for my hardware, I have an Arduino Uno that was purchased about 7 months ago.

edit: @ Nick Gammon I thought millis would still count during the delay()

After almost 14 hours, my little test is within a second, maybe better, but I can't really measure closer than that. So it seems that millis() has the potential to be fairly accurate, but certainly care must be taken as either hardware or software can compromise it. I'd like to leave this running several days, but have other plans, so maybe another time. Wouldn't want to draw too many conclusions from a single test, but gut feel is that this one example is keeping time at least as well as the average free-running DS1307, in my experience.

dethredic:
edit: @ Nick Gammon I thought millis would still count during the delay()

Yes it will, but it depends where the delay ends up. eg.

-- about to tick (at 999)
delay (10);

-- notice tick now (it is 1009) 
if(millis() - lastTick >= 1000)

You have the potential to lose 10 mS of accuracy (admittedly you would be unlucky) if you press the button on a 1-second boundary.

Also this:

void serialOutputTime()
{
  Serial.print(hour, DEC); // the hour, sent to the screen in decimal format
  Serial.print(":"); // a colon between the hour and the minute
  Serial.print(minute, DEC); // the minute, sent to the screen in decimal format
  Serial.print(":"); // a colon between the minute and the second
  Serial.println(second, DEC); // the second, sent to the screen in decimal format
}

Now, as I read it, Serial.print is blocking (I may be wrong, but I think it is). Now you are printing something like 12:45:22 which is 8 bytes, which at 9600 baud will take about 8.33 mS. Now if you "tick over" during that 8.33 mS (and remember that you are doing this every second) then you are potentially going to miss the next "tick" by up to 8 mS. And because your tick handling just starts the next 1000 mS when the previous one ends (rather than when it should end) you are likely to gradually miss time.

After running for 4 hours it is about 3 mins behind.

By my reckoning you are losing 12.5 mS every tick, that's quite a lot.

print ((3 * 60) / (60 * 60 * 4)) --> (3 minutes) / (4 hours) = 0.0125

Using the suggestions in the thread I think I fixed the overflow issue. I also change it to only output the time every minute, so that if printing to the serial causes time loss then it will disappear. Let me know if there are any obvious problems with this, but otherwise I will let it run for a couple of hours.

#define minButtonPin 2
#define hourButtonPin 3

int second=0, minute=0, hour=1;
boolean minButtonIsPressed = false;
boolean hourButtonIsPressed = false;

void setup(){
  Serial.begin(9600);
  
  pinMode(minButtonPin, INPUT);
  pinMode(hourButtonPin, INPUT);
}

void loop(){
  static unsigned long lastTick = 0; // holds the last time we moved forward one second
  
  /**********  CHECK BUTTONS  **********/
  // Check Minute Button
  if(buttonPressed(minButtonPin) && !minButtonIsPressed)
  {
    //Serial.println("Minute was pressed");
    minute++;
    minButtonIsPressed = true;
    serialOutputTime();
    delay(10);
  }
  else if(!buttonPressed(minButtonPin))
  {
    minButtonIsPressed = false;
    delay(10);
  }
  // Check Hour Button
  if(buttonPressed(hourButtonPin) && !hourButtonIsPressed)
  {
    //Serial.println("Hour was pressed");
    hour++;
    hourButtonIsPressed = true;
    serialOutputTime();
  }
  else if(!buttonPressed(hourButtonPin))
  {
    hourButtonIsPressed = false;
  }
  
  /**********  UPDATE TIME  **********/
  // Move forward one second every 1000 milliseconds
  unsigned long now = millis ();
  if(now - lastTick >= 1000) 
  {
    lastTick = now;
    second++;
  } 
  // Move forward one minute every 60 seconds
  if(second > 59)
  {
    minute++;
    second = 0;
    serialOutputTime();
  }
  // Move forward one hour every 60 minutes
  if(minute > 59)
  {
    hour++;
    minute = 0;
  }
  // Reset the hour
  if(hour > 12)
  {
    hour = 1;
  }
}

boolean buttonPressed(int aPin){
  if(digitalRead(aPin) == HIGH)
  {
    return true;
  }
  
  return false;
}

void serialOutputTime()
{
  Serial.print(hour, DEC); // the hour, sent to the screen in decimal format
  Serial.print(":"); // a colon between the hour and the minute
  Serial.print(minute, DEC); // the minute, sent to the screen in decimal format
  Serial.print(":"); // a colon between the minute and the second
  Serial.println(second, DEC); // the second, sent to the screen in decimal format
}

Hm, couple of things to recap...

  • Serial.print() is blocking, and VERY, VERY much so. During debugging of a program with AVR Studio, I actually had to jump over the Serial.print() commands, because they were taking so long in the debugger. That's why I figured the loops may be taking longer than 1ms and "overflowing" over the 1000ms conditional. It actually takes a significantly alarming amount of time to run a Serial.print() statement.
  • I have the sudden urge to write an accurately coded clock, and upload it to 10 Arduino boards and just prove to these gddamn knuckleheads that Arduino clock crystals and timing are just as accurate, if not more so, than a cheap chinese watch. There's a reason they have that number on the tin, and that's that THEY are calibrated for that frequency at THEIR factory, not the device they run on... it's not the responsibility of the device manufacturer to calibrate a device for crystal error; that's the crystal's job: to BE 100% accurate to within 3 decimal places. That's why you buy a crystal. I was asking a rhetorical question back there.
  • I also posted code that should've fixed the overflow issue, but did it work? I dunno, I never even read any acknowledgement that I even posted any code. Also a rhetorical question here; I don't expect anyone to actually test it now that others have posted the same exact concept under their own nickname and it also worked :roll_eyes:

Eh, minor disappointments, but whatever works...

@FalconFour I did try your code but it work about the same as the others. I think that the serial.Print was the culprit, as when I changed it to display every minute things worked out much better. I have now changed it to every 15 mins and things are working out better still.

Sorry, FalconFour - I saw your code but wasn't totally certain. So I just expressed the idea (which you had) which was to count up from when it should have matched rather than when it did match, but without giving code.

But now I look at your code I'm not convinced:

if(millis() >= nextTick) {
    nextTick = 1000 - (millis() - nextTick) + millis();
    serialOutputTime();
    second++;
  }

Basically this will fail when millis() wraps around. Imagine once it becomes zero again (after 49.7 days) then it will stall because you are waiting for millis() (which is small) to become >= nextTick (which is large).

I think you need to subtract, something like this:

unsigned long now = millis()
unsigned long diff = now - lastTick;  // handles wrap-around
if (diff >= 1000)
  {
  unsigned long overshoot = diff - 1000;  // how many mS over are we?
  second++;  // one second later
  serialOutputTime();   // show the time
  lastTick = now - overshoot;   // last tick was actually potentially a bit earlier   
  }

Untested.

True... I wasn't 100% on it myself but I was just a little ruffled to be kinda glazed-over being practically the first person to acknowledge that accurate timing even could be done, given accurately-timed code :wink:

Just mostly ruffled that the topic transitioned from "it can't be done" to "here's how to do it" without even recognizing the "oh, well, I guess it can be done!" aspect :sweat_smile:

No biggie, I'm just glad it works as I wrote from the start... give a crystal-based Arduino good code, and it'll give you good time! Only thing to worry about is the millis() overflow every 49 days :slight_smile:

BTW, here's why millis() is so accurate... millis() runs in TIMER0, which overflows every 1.024 milliseconds on a 16MHz clock (every 16,384 clock cycles, with 256 values and a clock/64 prescaler):

SIGNAL(TIMER0_OVF_vect)
{
	// copy these to local variables so they can be stored in registers
	// (volatile variables must be read from memory on every access)
	unsigned long m = timer0_millis;
	unsigned char f = timer0_fract;

	m += 1; // add 1 to timer0_millis
	f += 3; // add 3 to timer0_fract (24 >> 3 to save bits)
	if (f >= FRACT_MAX) { // if timer0_fract >= 125, add another 1ms since all the 0.024's added up
		f -= FRACT_MAX; // but don't lose precision.
		m += 1;
	}

	timer0_fract = f; // apply all we've counted so far
	timer0_millis = m;
	timer0_overflow_count++; //used to count cycles since this is incremented every 16,384 cycles
}

That's from wiring.c, modified with the values calculated in the macros, and with my comments describing what it does :slight_smile:

And millis() just pauses interrupts, reads timer0_millis, resumes interrupts, and returns timer0_millis. :slight_smile:

FWIW I agree with you about the crystal. A crystal marked 16.000 is implying 62 ppm error (1 / 16000), whereas the observed accuracy was more like 12500 ppm.

Don't worry too much about being "blanked" - often people are happy when something is fixed, and don't comment, or unhappy when it isn't fixed, and don't acknowledge the intermediate steps that might have worked (ones they may not have even tried).

@dethredic - the other thing you can do is compensate, if it turns out the crystal is a bit fast or a bit slow (assuming it is consistent). Say you regularly are 1% slow, just shave 1% off the number of milliseconds you are checking for.

Coding_Badly's solution (page 1) is the correct one.

Only read millis() once per loop. If you're reading millis() more than once in your calculation, chances are you are accumulating overrun.

The essential idea is to only keep track of when you last should have triggered, not when you last actually did, that way any overrun is ignored in the calculation of the time at which you next should trigger, and therefore doesn't accumulate. You can't avoid a certain amount of overrun on each trigger, the best you can do is avoid its accumulation.

Set your first trigger time in setup(), then in your loop check to see if the current time exceeds this by your delay amount (1000). If so, add your delay (1000) to the previous trigger time and do your required work. That's all:

unsigned long trigger;
unsigned long delay = 1000;

setup()
{
  trigger = millis();
}

loop()
{
  if ( (millis() - trigger) > delay )  // Optionally use "while" instead of "if"
  {
    // Do your stuff here.

    trigger += delay;
  }
}

By using subtraction in the comparison you can ensure that rollover is handled correctly (see millis() rollover - #5 by crimony - Syntax & Programs - Arduino Forum)

This method also has the benefit that it will "catch up" if the processing is delayed for some reason, because you are only adding 1 second to the trigger each time through the loop, eg. if there is a 10 second delay for some reason (perhaps a very slow print every 10 mins or so), millis() will be 10000 counts ahead of the value of "trigger" so the loop body will execute 10 times in a row quickly until "trigger" is eventually ahead of millis().

I agree, that looks neat and simple.

Talk about thinking outside the box... I really really like that solution! Here, stuck on calculating per loop, I never even thought of looking at millis() on the outside. Very good idea. It also accounts for the overflow, while the "trigger" will also overflow just the same!

Okay, after using the following code (more of a timer), my clock has fallen behind 40 seconds after 10 hours.

#define displayPin 2
#define goPin 3

int second = 0, minute = 0, hour = 0;
boolean displayIsPressed = false;
boolean goPinIsPressed = false;
boolean startClock = false;
unsigned long aTime = 0;

unsigned long trigger;
unsigned long secondDelay = 1000;

void setup(){
  Serial.begin(9600);
  
  pinMode(displayPin, INPUT);
  pinMode(goPin, INPUT);
  
  trigger = millis();
}

void loop(){  
  /**********  CHECK BUTTONS  **********/
  // Display time Button
  if(buttonPressed(displayPin) && !displayIsPressed)
  {
    displayIsPressed = true;
    serialOutputTime();
  }
  else if(!buttonPressed(displayPin))
  {
    displayIsPressed = false;
  }
  // Go button
  if(buttonPressed(goPin) && !goPinIsPressed)
  {
    goPinIsPressed = true;
    startClock = true;
    aTime = millis();
  }
  else if(!buttonPressed(goPin))
  {
    goPinIsPressed = false;
  }
  
  
  if(startClock == true)
  {
    updateTime(aTime);
  }
}

void updateTime(unsigned long adjustment){
  /**********  UPDATE TIME  **********/
  // Move forward one second every 1000 milliseconds
  while(millis() - adjustment - trigger >= secondDelay) 
  {
    second++;
    trigger += secondDelay;
  } 
  // Move forward one minute every 60 seconds
  if(second > 59)
  {
    minute++;
    second = 0;
  }
  // Move forward one hour every 60 minutes
  if(minute > 59)
  {
    hour++;
    minute = 0;
  }
}

boolean buttonPressed(int aPin){
  if(digitalRead(aPin) == HIGH)
  {
    return true;
  }
  
  return false;
}

void serialOutputTime()
{
  Serial.print(hour, DEC); // the hour, sent to the screen in decimal format
  Serial.print(":"); // a colon between the hour and the minute
  Serial.print(minute, DEC); // the minute, sent to the screen in decimal format
  Serial.print(":"); // a colon between the minute and the second
  Serial.println(second, DEC); // the second, sent to the screen in decimal format
}

This is a much better error, but I would still like to make it a little closer, so I will try adding an adjustment.
By my calculations It seems my crystal is off by 40 seconds every 10 hours or 36000 seconds, so to compensate for this:

40 / (10 * 60 * 60) = 0.00111111111

Now:

0.00111111111 * 1 000 = 1.11111111

So I will adjust my "secondDelay" from 1000 to 998.99999.

Let me know if there are any errors in my math.

Let me know if there are any errors in my math.

It doesn't matter. You need to fix the bug in your program first and then rerun the test. I believe removing this line of code solves the problem...

  trigger = millis();

I think what he is saying is that, if you take 40 seconds to press the "start" button the clock starts at zero but the trigger is already 40 seconds in.

I don't think that would be correct as trigger() is called just once in setup(), so it would be near zero, if not zero.

I think what he is saying is that...

You have the right idea.

I believe I was wrong in my assessment. I think the code is actually unstable. If "trigger = millis();" results in a non-zero value assigned to trigger, I believe the application gets stuck in the while loop until trigger wraps around. I think the end result is that second is set to zero, minute jumps ahead by one, and trigger is left at "random" value lower than "millis() - adjustment".

But the details are irrelevant given the fact that aTime / adjustment serve no useful purpose and can be easily eliminated thus eliminating any possibility of a problem.

Well stare at it as I might, I can't quite get my head around it. But this isn't right:

if(buttonPressed(goPin) && !goPinIsPressed)
  {
    goPinIsPressed = true;
    startClock = true;
    aTime = millis();
  }

Whenever you press "goPin" it resets aTime, but it doesn't reset the hour/minute/second variables. So I think that pressing the button during the day (if you indeed do that) will have an undesirable effect.