Unify data type for time variables

[PhirePhly]

The three variables used in the main loop were all different data types, which would likely cause roll-over issues at ~25 or ~50 days of run time.

This fix changes the api_mtbs and api_lasttime to match the unsigned long type used by the millis() function and tweaks the main loop comparison.

I will admit I haven't produced the original possible issue or tested the fix, since it would take several months to do so. At the very least, the time variables are now the same datatype.

[witnessmenow]

Hey @PhirePhly , very valid issue especially for something that is intended to be long running like this.

Your fix can also run into issues though, eventually millis() will start returning 0 again and once that happens your if loop will not return true again:

now - lastRunTime > timeBetweenRuns
// After Overflow of millis would be
0 - Bignumber > 2000 (this can't be true)

maybe this would work

unsigned long difference = now - lastRunTime;
if ( (difference > timeBetweenRuns) || (difference < 0 ))

that should handle the overflow case. The difference should never be less than 0 unless the overflow has happened

EDIT: just to mention that my current code also suffers from this! Just re-reading it makes it sound like I'm blaming you for it, not my intention at all! :)

[PhirePhly]

No worries. I'm used to terse PR feedback.

The issue with your counter-example is that difference is unsigned, so it will (by definition) never be less than 0. (difference < 0) will never be true.

I think my patch does still handle the over-flow case correctly:

1. Update happens and lastRunTime is set to something near ULONG_MAX
2. millis() rolls over and starts back at zero
3. The next calculation of millis() - lastRunTime is now a small number minus something near ULONG_MAX. This underflows the calculation and takes us all the way around to something slightly larger than the value of millis(), which is the result that is desired here

The issue is that you want everything to have the same data type so there's no weird promotions happening between data types while you're relying on this overflow/underflow behavior to handle rollover.

Example:

unsigned long lastRunTime = 4294967196;
unsigned long now = 100;
now - lastRunTime == 200;

(I believe ULONG_MAX is 4294967295 in this environment)

[witnessmenow]

Thanks for the explanation! Merged