Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make elapsedSeconds millis() rollover safe #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pfeerick
Copy link
Owner

@pfeerick pfeerick commented Jul 9, 2024

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

@drf5n
Copy link

drf5n commented Jul 9, 2024

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
Copy link
Owner Author

pfeerick commented Jul 9, 2024

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

https://wokwi.com/projects/402972933691490305

@drf5n
Copy link

drf5n commented Jul 10, 2024

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
Copy link
Owner Author

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 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

elapsedSeconds doesn't seem to be rollover-safe.
2 participants