millis not accurate?

@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.