Fix #421: Prevent timestamp jumps from tick counter overflow#447
Fix #421: Prevent timestamp jumps from tick counter overflow#447rishabhvaish wants to merge 3 commits intomatth-x:mainfrom
Conversation
…rflow The multiplication (ticks_now - mocpp_ticks_count) * 1000UL can overflow a 32-bit unsigned long when the tick delta exceeds ~4.3M ticks. On ESP-IDF with the default 1000 Hz tick rate, this happens if the function is not called for approximately 72 minutes, producing a truncated result that causes ~12-hour (49,250s) time jumps in Clock::now(). Fix: cast to unsigned long long before multiplying, then truncate back to unsigned long after dividing by configTICK_RATE_HZ. Refs matth-x#421
Add a safety cap of 1 hour on the time delta computed in Clock::now(). This guards against remaining edge cases where arithmetic overflow or concurrent access produces a delta that would cause the OCPP timestamp to jump forward by hours or days. If the real elapsed time exceeds the cap, the clock catches up incrementally on subsequent calls rather than applying a single large jump. Refs matth-x#421
src/MicroOcpp/Core/Time.cpp
Outdated
| // will catch up incrementally on subsequent calls. | ||
| const decltype(delta) MAX_DELTA_MS = 3600UL * 1000UL; // 1 hour | ||
| if (delta > MAX_DELTA_MS) { | ||
| delta = MAX_DELTA_MS; |
There was a problem hiding this comment.
Wouldn't this just cause an excessively large (12 hour) time jump to be a medium large (1 hour) jump?
Would it be more appropriate to simply have MicroOcpp skip an update where the computed delta was so large?
There was a problem hiding this comment.
Great point! You're absolutely right - capping still allows large jumps, just smaller ones.
Skipping the update entirely would be cleaner. Let me revise the approach to detect implausible deltas and skip the time update in those cases, logging a warning instead. This way the clock maintains consistency rather than making any sudden jumps.
I'll update the implementation shortly. Thanks for the feedback!
There was a problem hiding this comment.
Updated in aac9147 — now skips the update entirely when delta exceeds the plausible threshold (1 hour), and resets lastUpdate so the next call starts fresh. No time jump at all; the clock will resync on the next setTime() from the CSMS.
|
Thanks for helping us out here! ❤️ |
|
Please note that our jump is always 3 months, not much, not less... so not sure we're in the same boat. Also, to make sure we don't break something else for critical code, let's use a TDD approach here, try to reproduce it with a test first, I tried but didn't succeed with vibe-coding 🙁 |
Per reviewer feedback, instead of capping the delta to 1 hour (which still causes a potentially large time jump), skip the update entirely when the delta exceeds a plausible threshold. The clock will resync naturally on the next setTime() call from the CSMS. This ensures Clock::now() never produces time jumps, maintaining consistency for transaction timestamps and other time-sensitive operations.
Fixes #421
Problem
Chargers running MicroOcpp on ESP-IDF (and potentially Arduino) experience periodic ~12-hour (49,250s) timestamp jumps. This has been observed by multiple users (@devunwired, @razvanphp) and causes incorrect OCPP timestamps in transaction events, meter values, and heartbeats.
Root Cause
The multiplication overflow in
mocpp_tick_ms_espidf():On Xtensa (ESP32),
unsigned longis 32-bit. When the tick delta multiplied by 1000 exceedsULONG_MAX(4,294,967,295), the intermediate result is silently truncated. This produces a wrong millisecond delta that propagates throughClock::now()as a ~49,250-second time jump.For example, with the maximum possible tick difference of
0xFFFFFFFF:0xFFFFFFFF * 1000= 4,294,967,295,000 → truncated to 32-bit = 3,294,967,296configTICK_RATE_HZyields ~49,250 fewer seconds than expectedFix
1.
Platform.cpp— Use 64-bit intermediate arithmeticThis ensures the multiplication
tick_delta * 1000is performed in 64-bit precision before dividing and truncating back tounsigned long.2.
Time.cpp— Safety cap onClock::now()deltaAdded a 1-hour cap on the time delta in
Clock::now()as a defensive measure against any remaining edge cases (including the thread-safety concerns noted in the issue discussion). If the computed delta exceeds 1 hour, it is capped — the clock catches up incrementally on subsequent calls rather than applying a single large jump that would corrupt OCPP timestamps.Testing