From 72f1ef235bb85e92fc8eb7f8319ec5f56c60327c Mon Sep 17 00:00:00 2001 From: TokenGoblin Date: Sat, 15 Aug 2026 00:10:53 -0600 Subject: [PATCH] Timer: timeouts longer than 32 bits of ticks expired at the horizon, not the timeout hasElapsedUs() short-circuits "delta >= UINT32_MAX -> expired" before it ever looks at how long the requested timeout is. That is only valid when the timeout itself fits in 32 bits of ticks. For longer timeouts the timer reported elapsed the moment the delta crossed the 32-bit tick horizon: US_TO_NT_MULTIPLIER horizon 4 (stm32) ~1073 s 100 (simulator/tests) ~43 s 168 (kinetis/cypress) ~25.6 s So hasElapsedSec(3600) on stm32 fired at ~18 minutes, and on kinetis at ~26 seconds. The "Timer fortify" commit (293f070) added a double-precision path for huge timeouts, but placed it after the shortcut, where it cannot help once the delta has crossed the horizon. There was a second problem in the same zone: the fast path truncates USF2NT(microseconds) to uint32_t. For timeouts whose microsecond count fits in 32 bits but whose tick count does not (43 s..71 min at multiplier 100), that float-to-uint32 conversion is out of range - undefined behavior - so the comparison was garbage even before the horizon was reached. Fix: route any timeout that does not fit 32 bits AS TICKS through the 64-bit double-precision compare, before the delta shortcut. The fast path is unchanged for every timeout that fits, which is every existing sub-horizon caller. Also guard the double->int64 cast against absurd (>int64 ticks) timeouts, which would be UB; such a timeout simply never elapses. Behavior notes: - brand-new (never reset) timers still report elapsed for any sane timeout, InitialState keeps the delta enormous; pinned by a new test. - getElapsedSeconds() still saturates at the horizon as its header documents; callers comparing that against long thresholds should use hasElapsedSec(), which this commit makes exact. Tests: two new tests fail against the previous implementation (60 s timeout checked at 50 s reported elapsed; 3600 s at 3599 s reported elapsed) and pass now. Full suite: 42 tests pass. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UUaUv4kBBZhtCkMpZnh8So --- util/src/timer.cpp | 22 ++++++++++++++----- util/test/test_timer.cpp | 47 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/util/src/timer.cpp b/util/src/timer.cpp index 9d3284c..982432a 100644 --- a/util/src/timer.cpp +++ b/util/src/timer.cpp @@ -37,18 +37,28 @@ bool Timer::hasElapsedUs(float const microseconds) const { return false; } - // If larger than 32 bits, timer has certainly expired - if (delta >= UINT32_MAX) { - return true; - } - constexpr float max_32_bit_fit_float{ 4294967295.f }; - if (microseconds >= max_32_bit_fit_float) { + // The fast path below truncates both sides to 32 bits, so it is only valid when the timeout + // fits in 32 bits AS TICKS - and US_TO_NT_MULTIPLIER can be as large as 168, shrinking the + // 32-bit tick horizon to as little as 25 seconds. Longer timeouts compare the full 64-bit + // delta, in double so the tick conversion keeps precision. This check must come before the + // "delta larger than 32 bits" shortcut: sitting after it, long timeouts were reported as + // elapsed the moment the delta crossed the horizon. + if (microseconds >= max_32_bit_fit_float / US_TO_NT_MULTIPLIER) { auto const ntDouble{ static_cast(microseconds) * US_TO_NT_MULTIPLIER }; + if (ntDouble >= static_cast(INT64_MAX)) { + // absurdly long timeout: the int64 cast below would be UB, and it cannot have elapsed + return false; + } return delta > static_cast(ntDouble); } + // The timeout fits in 32 bits of ticks, so a delta beyond 32 bits has certainly expired + if (delta >= UINT32_MAX) { + return true; + } + return static_cast(delta) > static_cast(USF2NT(microseconds)); } diff --git a/util/test/test_timer.cpp b/util/test/test_timer.cpp index 3befe8d..4123e04 100644 --- a/util/test/test_timer.cpp +++ b/util/test/test_timer.cpp @@ -33,3 +33,50 @@ TEST(util, timerResetStampedInTheFuture) { setTimeNowNt(0); } + +/** + * US_TO_NT_MULTIPLIER is 100 in this test build, so 32 bits of ticks is just under 43 seconds. + * On real ports the horizon ranges from ~25 seconds (kinetis/cypress, multiplier 168) to + * ~1073 seconds (stm32, multiplier 4). Timeouts longer than the horizon used to be reported + * as elapsed the moment the delta crossed it, because the "delta larger than 32 bits" shortcut + * ran before the long-timeout path. + */ +TEST(util, timerTimeoutLongerThan32BitsOfTicks) { + Timer timer; + timer.reset(0); + + // well inside the fast path: nothing surprising + setTimeNowNt(USF2NT((efitick_t)20'000'000)); // 20 seconds + EXPECT_FALSE(timer.hasElapsedSec(60)); + + // past the 32-bit tick horizon but before the requested timeout: NOT elapsed + setTimeNowNt(USF2NT((efitick_t)50'000'000)); // 50 seconds + EXPECT_FALSE(timer.hasElapsedSec(60)); + + setTimeNowNt(USF2NT((efitick_t)61'000'000)); // 61 seconds + EXPECT_TRUE(timer.hasElapsedSec(60)); + + setTimeNowNt(0); +} + +TEST(util, timerHourLongTimeoutStaysExact) { + Timer timer; + timer.reset(0); + + setTimeNowNt(USF2NT((efitick_t)3'599'000'000)); // 3599 seconds + EXPECT_FALSE(timer.hasElapsedSec(3600)); + + setTimeNowNt(USF2NT((efitick_t)3'601'000'000)); // 3601 seconds + EXPECT_TRUE(timer.hasElapsedSec(3600)); + + setTimeNowNt(0); +} + +TEST(util, brandNewTimerHasElapsedEvenForLongTimeouts) { + // "Brand new instances have most recent reset time far in the past" - that promise from the + // header must survive the long-timeout path too + Timer timer; + + setTimeNowNt(0); + EXPECT_TRUE(timer.hasElapsedSec(3600)); +}