Correct handling of millis() rollover

[edgar-bonet]

This pull requests addresses multiple issues that stem from a misunderstanding or mishandling of the millis() rollover.

Specifically, the example sketches GPS_{Hardware,Software}Serial_LOCUS_Status.ino contained the test

if (millis() > updateTime)

which will not work as expected when millis() is close to a rollover event and updateTime is just past the event. On the other hand, several other examples use the form

if (millis() - timer > 2000)

which, owing to the fact that the subtraction is done modulo (ULONG_MAX+1), is immune to rollover events. The test is, however, preceded by a misguided attempt to manage the rollover:

// if millis() or timer wraps around, we'll just reset it
if (timer > millis()) timer = millis();

Those lines do more harm than good, as they create a glitch in the timing that would not exist without them.

In the same vein, the methods secondsSince{Fix,Time,Date}() are rollover-safe and their documentation should not state that they will fail on a rollover.

Finally, in NMEA_data.cpp, the expression

((nmea_float_t)millis() - val[idx].lastUpdate)

fails on a rollover because the subtraction is performed on a floating point data type, which does not obey the rules of modular arithmetics. The correct behavior is achieved if the subtraction is done before the cast to a floating type.

[sellensr]

Good catch in NMEA_data! Thanks!

On the secondsSinceFix() etc. calls, I think the documentation should still note the failure of accuracy on rollover. The function will return a small value if it has been more than one rollover period. Maybe instead of saying it fails:

The time returned is limited by the millis() wrap around and will restart from zero if there have been no events for one millis() wrap around time of 2^32 milliseconds, about 50 days.

[edgar-bonet]

@sellensr: I think I see your point. My point is that the rollover of millis() itself, which happens after about 50 days of uptime, is not an issue. There is a lot of misunderstanding about this rollover floating around the net, and I think it is important to avoid conveying the wrong idea that all timing functions will fail when this happens.

The method secondsSinceFix() fails after 50 days of not having a fix (which hopefully is not too common), irrespective of the uptime.

The time returned is limited by the millis() wrap around [...]

I do not like this sentence because it sounds like the issue is millis() wrapping around, which is exactly what I do not want to imply. The issue is not the wrap around of millis(), it's the wrap around of the quantity millis() - lastFix.

What do you think of the wording below?

The time returned is limited to 2^32 milliseconds, which is about 49.7 days. It will wrap around to zero if no position fix is received for this long.

The same sentence would be used for the other two functions, with “position fix” replaced by “GPS time” and “GPS date”.

[sellensr]

Your sentence is better than mine -- go for it

[edgar-bonet]

I pushed a commit with the added comments.

[drak7]

Github was having issues, I merged this but it's still showing as open. I'll close it and hope the commits stay where they are.