search

pfeerick / elapsedMillis

Arduino 'port' of the elapsedMillis library
MIT License
84 stars 26 forks source link

fix: make elapsedSeconds millis() rollover safe #17

Closed pfeerick closed 3 weeks ago

pfeerick commented 1 month ago

Fixes #16 by using modular arithmetic - edit: which will never complete, so fall back to using ms internally

drf5n commented 1 month ago

I'm not sure that's the right modulus.

millis rolls over at 0xFFFFFFFF or 4294967295, so the rollover for seconds happens at 4294967295/1000 = 4294967 seconds with remainder 295ms. Seconds (or any long) won't ever get big enough for & 0xFFFFFFFF to do anything.

I think the compiler would optimize away this patch and won't change the behavior.

pfeerick commented 1 month ago

Indeed... processing the wrong result. This version does appear to be working correctly now though

https://wokwi.com/projects/402972933691490305

drf5n commented 1 month ago

I wonder if the *1000 constants should use *1000UL..

For lines like:

elapsedSeconds operator + (int val) const           { elapsedSeconds r(*this); r.ms -= val * 1000; return r; }

...with an int val, val*1000 could overflow the int on an Uno before it gets assigned into the unsigned long ms.

Maybe a constant like:

const unsigned long msPerSec = 1000;
...
elapsedSeconds operator + (int val) const           { elapsedSeconds r(*this); r.ms -= val * msPerSec; return r; }
...
pfeerick commented 1 month ago

Yup, sounds good to me. It survived the test on wokwi and is working fine on a Atmega328 and ESP32-C6 (although I'm not sure of the direct timer reset there, so it is only a 'does the example run' test there). The changes in the assembly seem trivial... https://godbolt.org/z/dKPe61j7d ... plus anything that give the compiler hints on expected behaviour is good IMO ;)