Address review feedback on the rollover fixes

- BME680Sensor: checkpoint lastStateSaveMs after a successful write instead of
  at the interval test. The first save (IAQ accuracy >= 2) left it at 0, timing
  the next save from boot, and stamping before the write deferred the retry a
  full period when the write failed. Reads Time::getMillis(), the same clock
  Throttle compares against.

- Throttle: add deadlinePassedAt(now, deadline) for loops that snapshot the
  clock once and test many deadlines; deadlinePassed() now delegates to it.
  NextHopRouter::doRetransmissions() uses it, replacing the inline half-range
  compare adopted from #10227 (nightjoker7) - same arithmetic, credited at the
  call site - and takes its snapshot from Time::getMillis() so setNextTx()
  deadlines and the due test cannot diverge under an injected test clock.

- test_native.yml: set -euo pipefail in the millis-deadline guard, matching the
  sibling suite-count job. Without -e a partially failed scan could report "no
  violations" from truncated output.

- test_packet_signing: build the not-due deadline from Time::getMillis() rather
  than millis(), so the test and the router read one clock.

- test_throttle: cover deadlinePassedAt(), and correct a wrapped-value comment
  (0xFFFFFF00 + 400 is 0x00000090, not 0x00000094).

Two review comments were declined: the AirTime mutex (every airTime-> caller
runs in the single cooperative loop, WebServerThread included) and the
MotionSensor 0-sentinel countdown (the calibration frame is only installed
while a window is open).

clod helped out here

Author: NomDeTom

.github/copilot-instructions.md

@@ -341,8 +341,9 @@ firmware/
   - `Throttle::hasElapsed(lastMs, intervalMs)` - its complement, true once the interval has passed (inclusive `>=`). Prefer this to spelling `!isWithinTimespanMs(...)`.
   - `Throttle::execute(&lastMs, intervalMs, func)` - function-pointer form that updates the timestamp on fire.
   - `Throttle::deadlinePassed(deadlineMs)` - for a stored absolute deadline that cannot be re-expressed as "interval since an event". Uses an unsigned half-range compare; reads deadlines more than ~24.8 days out as already passed, which no interval in this firmware approaches (the longest is 24 h).
+  - `Throttle::deadlinePassedAt(nowMs, deadlineMs)` - the same test against a caller-supplied `now`, for a loop that snapshots the clock once and then tests many deadlines (`NextHopRouter::doRetransmissions()`). Take the snapshot from `Time::getMillis()`, not `millis()`.
 
-  Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: for ~24 days after the 32-bit wrap it either stalls the action or fires it immediately. All four helpers subtract first, so unsigned wraparound cancels out. `Throttle` reads the clock through `Time::getMillis()` (`src/UptimeClock.h`), which means every one of its ~94 call sites is time-injectable - a native test can drive `Time::setTestMillis(0xFFFFFF00)` across the wrap. There is deliberately no 64-bit millis; see the note in `UptimeClock.h`.
+  Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: for ~24 days after the 32-bit wrap it either stalls the action or fires it immediately. All five helpers subtract first, so unsigned wraparound cancels out. `Throttle` reads the clock through `Time::getMillis()` (`src/UptimeClock.h`), which means every one of its ~94 call sites is time-injectable - a native test can drive `Time::setTestMillis(0xFFFFFF00)` across the wrap. There is deliberately no 64-bit millis; see the note in `UptimeClock.h`.
 
   **Sentinel hazard.** If a deadline variable also encodes "inactive" - `0` for `rebootAtMsec`, `shutdownAtMsec`, `alertBannerUntil`, `fixHoldEnds`; `UINT32_MAX` for `nagCycleCutoff` - test that sentinel _before_ the elapsed comparison. Every such value is arithmetically far in the past, so a correct comparison reads it as "expired" and fires immediately: `rebootAtMsec = -1` meaning "never" is what would have become a reboot loop. Write `if (deadline && Throttle::deadlinePassed(deadline))`, and never fold the sentinel into the helper.
 

.github/workflows/test_native.yml

@@ -72,7 +72,7 @@ jobs:
       - name: Reject millis() used directly in a deadline comparison
         shell: bash
         run: |
-          set -uo pipefail
+          set -euo pipefail
           allowlist=".github/millis-deadline-allowlist.txt"
 
           # Flag millis() directly adjacent to a comparison operator, in either order. The correct

AGENTS.md

@@ -86,8 +86,9 @@ Key rotation to never trigger casually: only the **full** factory reset (`factor
   - `Throttle::hasElapsed(lastMs, intervalMs)` - its complement, true once the interval has passed (inclusive `>=`). Prefer this to spelling `!isWithinTimespanMs(...)`.
   - `Throttle::execute(&lastMs, intervalMs, func)` - function-pointer form that updates the timestamp on fire.
   - `Throttle::deadlinePassed(deadlineMs)` - for a stored absolute deadline that cannot be re-expressed as "interval since an event".
+  - `Throttle::deadlinePassedAt(nowMs, deadlineMs)` - the same test against a caller-supplied `now`, for a loop that snapshots the clock once and tests many deadlines. Snapshot from `Time::getMillis()`.
 
-  Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: for ~24 days after the 32-bit wrap it either stalls the action or fires it immediately. All four helpers subtract first, so unsigned wraparound cancels out. `Throttle` reads the clock through `Time::getMillis()` (`src/UptimeClock.h`), so all ~94 of its call sites are time-injectable and a native test can drive the wrap with `Time::setTestMillis()`.
+  Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: for ~24 days after the 32-bit wrap it either stalls the action or fires it immediately. All five helpers subtract first, so unsigned wraparound cancels out. `Throttle` reads the clock through `Time::getMillis()` (`src/UptimeClock.h`), so all ~94 of its call sites are time-injectable and a native test can drive the wrap with `Time::setTestMillis()`.
 
   **Sentinel hazard.** If a deadline variable also encodes "inactive" (`0` for `rebootAtMsec`, `shutdownAtMsec`, `alertBannerUntil`, `fixHoldEnds`; `UINT32_MAX` for `nagCycleCutoff`), test that sentinel _before_ the elapsed comparison - every such value is arithmetically far in the past, so a correct comparison fires on it immediately. Write `if (deadline && Throttle::deadlinePassed(deadline))`.
 

src/mesh/NextHopRouter.cpp

@@ -1,6 +1,8 @@
 #include "NextHopRouter.h"
 #include "Default.h"
 #include "MeshTypes.h"
+#include "Throttle.h"
+#include "UptimeClock.h"
 #include "meshUtils.h"
 #if !MESHTASTIC_EXCLUDE_TRACEROUTE
 #include "modules/TraceRouteModule.h"
@@ -394,7 +396,9 @@ PendingPacket *NextHopRouter::startRetransmission(meshtastic_MeshPacket *p, uint
  */
 int32_t NextHopRouter::doRetransmissions()
 {
-    uint32_t now = millis();
+    // Same clock Throttle reads, so setNextTx() deadlines and this test can't diverge under an
+    // injected test clock.
+    uint32_t now = Time::getMillis();
     int32_t d = INT32_MAX;
 
     // FIXME, we should use a better datastructure rather than walking through this map.
@@ -405,16 +409,9 @@ int32_t NextHopRouter::doRetransmissions()
 
         bool stillValid = true; // assume we'll keep this record around
 
-        // Use unsigned half-range comparison so retransmission timing stays correct across the
-        // ~49.7 day millis() wraparound (previously this FIXME would stall all retx for the
-        // duration of the wrap or fire them all at once immediately after).
-        //
-        // Casting an unsigned difference to int32_t for a "time passed" test is
-        // implementation-defined in C++ when the value exceeds INT32_MAX. The unsigned
-        // half-range form below is fully well-defined: nextTxMsec is in the past (or is now)
-        // iff (now - nextTxMsec) has not wrapped past 2^31 ms. Anything further in the
-        // future wraps into the top half and reads as "not yet."
-        if ((uint32_t)(now - p.nextTxMsec) < 0x80000000u) {
+        // Half-range compare from #10227 (nightjoker7), now via Throttle: judged against the
+        // snapshot above, so one pass sees one instant and the 49.7 day wrap can't stall retx.
+        if (Throttle::deadlinePassedAt(now, p.nextTxMsec)) {
             if (p.numRetransmissions == 0) {
                 if (isFromUs(p.packet)) {
                     LOG_DEBUG("Reliable send failed, returning a nak for fr=0x%08x,to=0x%08x,id=0x%08x", p.packet->from,
@@ -510,7 +507,7 @@ void NextHopRouter::setNextTx(PendingPacket *pending)
 {
     assert(iface);
     auto d = iface->getRetransmissionMsec(pending->packet);
-    pending->nextTxMsec = millis() + d;
+    pending->nextTxMsec = Time::getMillis() + d;
     LOG_DEBUG("Setting next retransmission in %u msecs: ", d);
     printPacket("", pending->packet);
     setReceivedMessage(); // Run ASAP, so we can figure out our correct sleep time

src/mesh/Throttle.cpp

@@ -41,7 +41,5 @@ bool Throttle::isWithinTimespanMs(uint32_t lastExecutionMs, uint32_t timeSpanMs)
 /// See the header for the range limit and the sentinel requirement.
 bool Throttle::deadlinePassed(uint32_t deadlineMs)
 {
-    // Unsigned half-range rather than a cast to int32_t, which is implementation-defined once the
-    // difference exceeds INT32_MAX. Deadlines further ahead than 2^31 ms land in the top half.
-    return (uint32_t)(Time::getMillis() - deadlineMs) < 0x80000000u;
+    return deadlinePassedAt(Time::getMillis(), deadlineMs);
 }

src/mesh/Throttle.h

@@ -28,4 +28,13 @@ class Throttle
     /// for that separately, first: every such value is arithmetically far in the past, so it reads
     /// as passed.
     static bool deadlinePassed(uint32_t deadlineMs);
+
+    /// deadlinePassed() against a caller-supplied "now", for a loop that snapshots the time once and
+    /// tests many deadlines against it. Same range limit and sentinel rules as above.
+    static bool deadlinePassedAt(uint32_t nowMs, uint32_t deadlineMs)
+    {
+        // Passed iff now - deadline has not wrapped past 2^31 ms; further-ahead deadlines land in
+        // the top half. Not an int32_t cast, which is implementation-defined beyond INT32_MAX.
+        return (uint32_t)(nowMs - deadlineMs) < 0x80000000u;
+    }
 };

src/modules/Telemetry/Sensor/BME680Sensor.cpp

@@ -7,6 +7,7 @@
 #include "FSCommon.h"
 #include "SPILock.h"
 #include "TelemetrySensor.h"
+#include "UptimeClock.h"
 #include "mesh/Throttle.h"
 
 #if __has_include(<Adafruit_BME680.h>)
@@ -169,7 +170,6 @@ void BME680Sensor::updateState()
             LOG_DEBUG("%s state update every %d minutes", sensorName, STATE_SAVE_PERIOD / 60000);
             update = true;
             stateUpdateCounter++;
-            lastStateSaveMs = millis();
         }
     }
 
@@ -184,6 +184,9 @@ void BME680Sensor::updateState()
             file.write((uint8_t *)&bsecState, BSEC_MAX_STATE_BLOB_SIZE);
             file.flush();
             file.close();
+            // Checkpoint on success only: stamping at the interval test left the first save at 0
+            // (next save timed from boot) and deferred the retry a full period after a failed write.
+            lastStateSaveMs = Time::getMillis();
         } else {
             LOG_INFO("Can't write %s state (File: %s)", sensorName, bsecConfigFileName);
         }

test/test_packet_signing/test_main.cpp

@@ -22,6 +22,7 @@
 // compiled out unless both PKI and XEdDSA are enabled (e.g. stm32 sets MESHTASTIC_EXCLUDE_XEDDSA).
 #if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA)
 
+#include "UptimeClock.h"
 #include "mesh/Channels.h"
 #include "mesh/CryptoEngine.h"
 #include "mesh/MeshRadio.h"
@@ -1224,7 +1225,7 @@ void test_C3_invalid_repeated_packet_cannot_ack_or_change_retry_state(void)
     // "Far future, so no retransmission is due." Must be a representable future time, not
     // UINT32_MAX: doRetransmissions() compares with an unsigned half-range test, under which
     // UINT32_MAX is ~1ms in the *past* and would fire a retransmit and rewrite nextTxMsec.
-    const uint32_t notDueTxMsec = millis() + 3600000UL;
+    const uint32_t notDueTxMsec = Time::getMillis() + 3600000UL;
     pipelineRouter->addPending(prior, notDueTxMsec);
     const uint32_t lastHeard = mockNodeDB->getMeshNode(LOCAL_NODE)->last_heard;
 

test/test_throttle/test_main.cpp

@@ -107,7 +107,7 @@ void test_deadlinePassed_survives_millis_wrap()
     const uint32_t deadline = 0xFFFFFF00u + 500;
 
     TEST_ASSERT_FALSE(Throttle::deadlinePassed(deadline)); // not yet
-    Time::advanceTestMillis(400);                          // 0x00000094 - wrapped, still not due
+    Time::advanceTestMillis(400);                          // 0x00000090 - wrapped, still not due
     TEST_ASSERT_FALSE(Throttle::deadlinePassed(deadline));
     Time::advanceTestMillis(100); // exactly due, past the wrap
     TEST_ASSERT_TRUE(Throttle::deadlinePassed(deadline));
@@ -126,6 +126,22 @@ void test_deadlinePassed_does_not_fire_early_when_deadline_wraps()
     TEST_ASSERT_FALSE(Throttle::deadlinePassed(deadline));
 }
 
+// deadlinePassedAt() judges against a caller-supplied now, so a loop that snapshots the clock once
+// gets one instant for every entry - including across the wrap, where the clock has moved on.
+void test_deadlinePassedAt_uses_the_supplied_now()
+{
+    Time::setTestMillis(0xFFFFFF00u);
+    const uint32_t now = Time::getMillis();
+    const uint32_t deadline = 0xFFFFFF00u + 500; // wraps to 0x000000F4
+
+    TEST_ASSERT_FALSE(Throttle::deadlinePassedAt(now, deadline));
+    TEST_ASSERT_TRUE(Throttle::deadlinePassedAt(deadline, deadline));     // inclusive boundary
+    TEST_ASSERT_TRUE(Throttle::deadlinePassedAt(deadline + 1, deadline)); // past the wrap
+    Time::advanceTestMillis(60000);                                       // clock moved, snapshot did not
+    TEST_ASSERT_FALSE(Throttle::deadlinePassedAt(now, deadline));
+    TEST_ASSERT_TRUE(Throttle::deadlinePassed(deadline));
+}
+
 // deadlinePassed() cannot know about sentinels, so it reports them as passed. This pins that
 // contract, since callers relying on it must test armed-ness first.
 void test_deadlinePassed_reads_disarmed_sentinels_as_passed()
@@ -211,6 +227,7 @@ void setup()
     RUN_TEST(test_deadlinePassed_basic);
     RUN_TEST(test_deadlinePassed_survives_millis_wrap);
     RUN_TEST(test_deadlinePassed_does_not_fire_early_when_deadline_wraps);
+    RUN_TEST(test_deadlinePassedAt_uses_the_supplied_now);
     RUN_TEST(test_deadlinePassed_reads_disarmed_sentinels_as_passed);
     RUN_TEST(test_execute_runs_first_time_then_throttles);
     RUN_TEST(test_execute_survives_millis_wrap);