Timer: timeouts longer than 32 bits of ticks expire at the horizon, not the timeout

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<double>(microseconds) * US_TO_NT_MULTIPLIER };
+		if (ntDouble >= static_cast<double>(INT64_MAX)) {
+			// absurdly long timeout: the int64 cast below would be UB, and it cannot have elapsed
+			return false;
+		}
 		return delta > static_cast<efitick_t>(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<uint32_t>(delta) > static_cast<uint32_t>(USF2NT(microseconds));
 }
 

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