millis not accurate?

After running for 4 hours it is about 3 mins behind. I assume this is a problem with my code:

#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();
    delay(10);
  }
  else if(!buttonPressed(hourButtonPin))
  {
    hourButtonIsPressed = false;
    delay(10);
  }
  
  /**********  UPDATE TIME  **********/
  // Move forward one second every 1000 milliseconds
  if(millis() - lastTick >= 1000) 
  {
    lastTick = millis();
    serialOutputTime();
    second++;
  }  
  // 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;
  }
  // 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
}

Everything looks OK to me, like using a variable to hold the value of millis() instead of using delay() at the end of each loop... but... I do see a little problem:

  /**********  UPDATE TIME  **********/
  // Move forward one second every 1000 milliseconds
  if(millis() - lastTick >= 1000) 
  {
    lastTick = millis();
    serialOutputTime();
    second++;
  }

Looks great at first until you realize that (millis() - lastTick) may actually be greater than 1000 (time it takes to run each loop), but is still treated functionally like 1000ms. So if it ends up being 1005, 1002, etc., it gets shaved off and you just lost 2-5 ms.

I'd say a quick fix may be to use a destination stamp instead, also so the CPU can do a quick "compare-to-zero" instead of comparing with a 16-bit integer ("1000"):

  /**********  UPDATE TIME  **********/
  // Move forward one second every 1000 milliseconds

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

Yeah, it calls millis() twice, but it might be the most accurate way to get the job done. Since millis() will always be greater than nextTick when the "if" is true, we can use millis() - nextTick to find the amount of error. Then, we assign nextTick as 1000 - (error) + millis(), which may end up being, say, 998ms from now (millis()) if there was a 2ms error.

It may seem like excessive accuracy, but micros() may prove to be a better count of time...

^ I should mention that I have been using a software real-time clock in my car's MPGuino (which uses a 20MHz crystal) and has been 100% accurate to the minute over the course of 3+ weeks running 24/7, so I think it's perfectly suitable for timekeeping, thank you very little.

FalconFour:
thank you very little.

lol

So you're saying that dedicated thru-hole mount crystals marked "16.000 MHz" (note the triple-digit precision marked on the tin) have the potential to be sloppier than a $1, vending-machine-purchased, Chinese-made piece-of-junk digital watch that also keeps near-perfect time? :%

What does the math say? 3 mins in 4 hours is 1.25% slow. That is a couple of magnitudes higher then the normal crystal tolerance error of 100-150 parts per million, so I would not look first at the crystal to be the problem.

My guess is you are dealing with a math problem in your code that is dropping mill counts. But I can only leave that to the software gurus around here, hardware is my thing. :wink:

Lefty

That doesn't change the facts. Ceramic resonators are even less precise than crystals, and you are seeing more and more microcontrollers (including Arduino boards and clones) using ceramic resonators because there is simply no expectation of long-term timekeeping.

I agee about the "long-term timekeeping" comment not being a good application for the millis() function on a arduino board. However I still say his 3 mins error over 4 hours (-1.25%) is way beyond the frequency tolerence of even ceramic resonators (typically +/- .1% ?) specifications. I still think the sketch has a problem causing the lose of time at that high of a rate.

Lefty

Resonators are more like ±0.5%. The OP didn't say what the hardware was? So I might think there could be something else going on. OTOH, my first reaction was that nothing was wrong, as I don't consider millis() suitable in general for long-term timekeeping. But lefty is right XD erm, correct in that the observed error is still over twice what we might expect from a resonator-driven clock.

Now the spooky part is that just before reading this post, just for fun, I'd started a sketch on a breadboarded "Arduino" with a ±10ppm crystal, and am comparing it with a NIST-synchronized RTC. I'll report back tomorrow. (So far it's right on, but has only been running 20 minutes, LOL.)

But lefty is right erm, correct

Lefty is left not right. However my correction tolerance is most likely way worst then even a ceramic resonator on it's worst night. :wink:

Try this:

// Move forward one second every 1000 milliseconds
duration = millis()-lastTick;
if(duration >= 1000)
{
lastTick = millis() + duration - 1000; //attempt to capture the time overage
serialOutputTime();
second++;
}

I still say his 3 mins error over 4 hours (-1.25%) is way beyond the frequency tolerence of even ceramic resonators (typically +/- .1% ?) specifications

That's actually worse than the internal oscillator. The one sitting on my desk is currently at 0.181%.

Regarding the code, this is a bit simpler and should solve any minor overrun problem. If necessary, the if can be changed to while to overcome serious overruns.

  if ( millis() - PreviousMS >= 10000 )  // Optionally use "while" instead of "if"
  {
    // Do your stuff here.
    PreviousMS = PreviousMS + 10000;
  }

CrossRoads:
Try this:

// Move forward one second every 1000 milliseconds
duration = millis()-lastTick;
if(duration >= 1000)
{
lastTick = millis() + duration - 1000; //attempt to capture the time overage
serialOutputTime();
second++;
}

I just noticed the slight overrun. I have this running and will report back tomorrow morning to see if it is still accurate. Thanks

My old Duemilanoves (Crystal) are good to single figure seconds over the millis() overrun period ( ~50 days ). My Uno (Ceramic) is OK to 20 seconds a day. These are long term figures, measurements to the single millisecond in the short term may be off.

dethredic:
After running for 4 hours it is about 3 mins behind. I assume this is a problem with my code:

I think the code has flaws, it isn't the crystal necessarily.

The test for >= 1000 ms and then taking whenever-that-happened-to-match as the next test point basically guarantees that you will eventually drift.

Then you take a different reading to synchronize the next second to:

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

At the very least, use the same reading, so the time taken to execute the "if" isn't counted, eg.

unsigned long now = millis ();

if(now - lastTick >= 1000) 
  {
    lastTick = now;
    serialOutputTime();
    second++;
  }

But better still, take some baseline time, and keep adding 1000 to it (or subtracting, whatever) so that you don't drift if an interrupt or something means your loop happens to execute at 1001 rather than 1000 mS.

Plus of course this:

delay(10);

If you happen to hit a button during the day, that means that your next match could be 10 mS out.

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