From ee15508494b23225fef78ea9a10d5310f4e87d62 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Mon, 14 Sep 2026 12:19:02 +0000 Subject: [PATCH] time: arm the remaining 0-means-unset stamps through the helpers (#11830) * time: add skipZero/safeMillis/timerEndsAtMillis helpers skipZero() steps a millis value past 0, since stored stamps and deadlines conventionally use 0 for "unset" and the one tick per ~49.7-day wrap that lands on 0 would otherwise read as never-set. safeMillis() covers a bare stamp; timerEndsAtMillis(delayMs) covers a deadline, where the sum is what has to dodge 0 - a non-zero read plus a delay lands there once per wrap - so it is not safeMillis() + delayMs. * time: replace hand-rolled zero-dodging with the UptimeClock helpers PacketHistory rxTimeMsec, EncryptedStorage s_lastFailMillis (stamps), and SGM41562 lastRefreshMs_ / NextHopRouter learnedAtMsec (ternary stamps) each hand-rolled skipZero() in place; swap in safeMillis()/skipZero() directly. HapticFeedback pulseOffAt/delayedPulseAt and GPS fixHoldEnds hand-rolled the deadline form - millis() + delay, then remap a 0 result to 1 - swap in timerEndsAtMillis(delay). No behavior change; each site keeps the value it already computed. * time: guard the remaining 0-means-unset deadline/stamp writes rebootAtMsec, shutdownAtMsec, and NotificationRenderer::alertBannerUntil are all read back with a bare == 0 / != 0 check for 'not scheduled', but every write site computed millis() + delay (or a bare millis() stamp) with no guard against landing exactly on 0 - the same wrap hazard skipZero() exists for, just never applied here. Route every rebootAtMsec/shutdownAtMsec/alertBannerUntil write through timerEndsAtMillis()/safeMillis(); RadioLibInterface's reboot-on-stuck-tx sums an already-captured stamp rather than "now", so it goes through skipZero() directly instead. No behavior change outside the ~1-in-2^32 wrap window each site was already exposed to. * time: guard three more 0-means-unset deadline writes ntp_renew (ethClient.cpp), suppressTouchTapUntilMs (Events.cpp), and tx_after (RadioLibInterface.cpp) all read back 0 as a real state - forced NTP renewal, no suppress window active, no TX delay armed, respectively - but each arm site wrote a bare millis()/getMillis() + delay with no guard against the sum landing exactly on 0. Route each through Time::timerEndsAtMillis(). No behavior change outside the wrap window each site was already exposed to. Refresh the Throttle.h TODO list to note ntp_renew is converted too. * motion: guard the calibration deadline and use Throttle::deadlinePassed endCalibrationAt's arm site wrote millis() + calibrateFor with no guard against landing on 0, the same value finishCalibrationIfExpired()/ drawFrameCalibration() treat as "not calibrating". Route it through Time::timerEndsAtMillis(). Also swap finishCalibrationIfExpired()'s hand-rolled (int32_t)(now - deadline) < 0 for Throttle::deadlinePassed(): same wrap-safe comparison the codebase already provides, without the signed-cast pattern Throttle.h documents as implementation-defined past INT32_MAX, and it drops the file's last direct millis() call in favor of the Time:: wrapper the rest of it already uses. * time: fix Throttle::execute()'s own zero-dodging Both places execute() writes *lastExecutionMs - the first-ever-run branch and the regular update - used bare Time::getMillis() with no guard against landing on 0, which is the exact sentinel this function reads back as "never run" one line above. A hit there makes the next call re-fire immediately instead of respecting minumumIntervalMs. Capture now via Time::safeMillis() once; every use downstream (the elapsed comparison, the stored value) is then safe by construction instead of needing the guard reapplied at each write. * revert some safeMillis cases where overflow is a bad thing * test(uptime): pin skipZero/safeMillis/timerEndsAtMillis at the wrap boundary Covers the zero case, an ordinary nonzero value, and a sum that lands exactly on 0 from a nonzero start - the case timerEndsAtMillis() exists for, and the one the prior suite had no direct coverage of. * time: restore the route-health write normalization and put it on one clock noteRouteLearned()/noteRouteSuccess() lost their `now ? now : 1` normalization, leaving learnedAtMsec able to store 0 - which getOrAllocRouteHealth() reads as an ever-growing age, making the slot the first eviction candidate and permanently stale. Normalize at the write, where the block comment already says it happens, so every caller is covered rather than just today's two. Both callers, the two isRouteStale() sites and doRetransmissions() now read Time::getMillis(), so the stamp and every comparison against it share a clock. doRetransmissions() goes back to getMillis(): its `now` feeds only comparisons, never a 0-sentinel field, so skipping zero there only cost accuracy. * time: read the haptic, InkHUD and calibration deadlines on the write's clock These three deadlines were converted to Time::timerEndsAtMillis() on the write side while their reads stayed on millis(), so each spanned two clocks and would fire immediately or never under an injected test clock. Convert the reads to match: HapticFeedback::scheduleNext()/runOnce(), the InkHUD tap-suppression window, and the calibration countdown's read-back of screen->getEndCalibration(). MotionSensor's sampledAtMs is left alone - its write and read are both millis() and consistent already. * time: correct the sentinel notes to match what the code actually does The Throttle.h enumeration claimed the remaining timerEndsAtMillis() callers "already dodge the sentinel", which reads as a completeness claim the same branch contradicts: RadioLibInterface's tx_after and activeReceiveStart are both 0=unarmed and both still arm from bare millis(). Name them instead, so the deadline-type conversion has the real list. The ntp_renew entry now separates a deliberate 0 ("due now", forced at link-up) from a computed one, which is what changed there. The three TODO(elapsed-stamp) blocks ran four and five lines against the repo's one-or-two rule, and two of them argued their case wrongly. Throttle.cpp implied safeMillis() simply doesn't help; in fact neither store is safe on the wrap tick - the 1 underflows a same-instant read, the 0 re-takes the never-run branch - which is the symmetry worth recording. PacketHistory.cpp called its dodge "reflecting the previous pattern" when it is load-bearing: rxTimeMsec 0 means "empty slot" (PacketHistory.h:21) and insert() drops a record stamped 0 outright, so without it a packet arriving on the wrap tick is never stored and loses its dedup. Also picks up trunk fmt's trailing-whitespace fix in Throttle.cpp and the comment realignment in SGM41562.cpp that this branch's added comment knocked out. * test(nexthop): pin the route-health stamp against the 0 sentinel The uptime suite covers skipZero/safeMillis/timerEndsAtMillis themselves, but nothing covered a call site, so the branch deleted noteRouteLearned()'s normalization and stayed green. None of the existing route-health tests pass 0 as `now` - they use 1000, learnAt, or millis() - (TTL + 5000) - which is exactly the gap the regression went through. Both new tests fail with "Expected 0 to be not equal to 0" when the skipZero() is backed out of NextHopRouter, and pass with it. noteRouteSuccess() only refreshes an existing record, so its twin learns a route first to reach the write. Also drops a self-referential assertion in the uptime suite: comparing getMillis() against safeMillis() passes even if safeMillis() does no dodge at all, so it now asserts the literal. * discard safemillis for skipzero (better semantics and therefore maintainability) and make consistent use of getmillis where it is called (to permit testing) * more wrapzero safety * STM gets some too * time: stop the next 0-means-unset deadline being armed from raw millis() The fields this branch armed through Time::timerEndsAtMillis() / Time::skipZero() are the kind that get added by copy-paste: `rebootAtMsec = millis() + N` appears at twenty-odd sites across six files, and the next module to defer a reboot will be written from one of them. Nothing catches the mistake afterwards - the sum lands on 0 for one tick per ~49.7-day wrap, so a test run, a soak and a bench session all pass while a pending reboot, shutdown, DFU jump or banner expiry is silently dropped. Two guards, at the two places it can go wrong. The helpers themselves: skipZero() is constexpr, so its contract is now pinned by static_assert in the header rather than only by test_uptime_clock. The asserts are chosen against the two plausible rewrites - `ms | 1` perturbs every even value and `ms + 1` turns the last tick of the wrap into the 0 the function exists to avoid. Both compile, and both pass a test that only checks skipZero(0); each trips a distinct assert here, naming the failure mode. The call sites: bin/lint-unset-sentinel-millis.sh flags a sentinel field in src/ assigned from a raw millis()/getMillis() read, and names the helper to use. It is name-driven because the 0 contract is declared in src/main.h and enforced in six other files, so no single-file scan can infer it; every one of the thirteen fields was checked to actually test against 0 before being listed. nagCycleCutoff and LinuxJoystick's nextRepeatX/nextRepeatY are deliberately absent - their unset state is a separate bool - and the nine remaining `millis() + x` sites in src/ are locals that never store 0 for anything to misread. Blocking, unlike its note-level neighbours: there is no run-time enforcer to pair with, and the tree has zero violations today, so gating costs nothing. Scoped to src/ so test_uptime_clock can keep building raw wrap values on purpose. bin/test-lint-unset-sentinel-millis.sh pins the scanner against 23 fixtures - reads, disarms, shadowing locals, comments, string literals and the already-fixed forms all have to stay quiet. Co-Authored-By: Claude Opus 5 * time: guard the four 0-means-unset stamps this branch had missed Sweeping src/ for the `if (stamp && )` idiom - the shape that makes 0 mean "unset" - turned up four stamps still armed from a raw clock read, so the new lint rule would have had to either ignore them or go red on checkout. Each is the same one-tick-per-wrap hole the rest of the branch closes: * TrackballInterruptBase lastInterruptTime, armed in all four ISR handlers and explicitly disarmed to 0 at the threshold reset. getMillis() is the ISR-safe read by construction - it compiles to millis() outside PIO_UNIT_TESTING - and skipZero() is pure, so neither adds anything to interrupt context. * NeighborInfoModule lastSentReply, read as `if (lastSentReply && ...)` before the 3-minute reply throttle. Needed the UptimeClock.h include. * PositionModule lastSentReply, same throttle; already on the injectable clock but still missing the guard. * NodeDB lastSort, whose own read spells the sentinel out as `lastSort == 0 ||`. On the wrap tick each would read as never-stamped: a trackball debounce window lost, a neighbour or position reply sent inside the throttle it was meant to respect, one extra NodeDB sort. Cheap individually, which is why they were missed. All four are now listed in bin/lint-unset-sentinel-millis.sh, so the rule covers every field in the tree that actually tests against 0 rather than a subset, and the header records the eight stamps left off for the opposite reason - their unset state is a separate flag (isNagging, busyTx, heldX/heldY, formatted_this_boot, heartbeat, gotwind, haveSample, lastIaqValid), so 0 is a value they may legally hold. The rule is silent across src/ on this tree. Co-Authored-By: Claude Opus 5 * lint: let a site opt out of the sentinel rule, with its reason on the record The rule is blocking, so it needs an escape hatch for the site where 0 genuinely is a legal timestamp - and the hatch should cost something, or it becomes the first thing anyone reaches for. `unset-sentinel-ok: ` in a comment on the write, or on a comment line above it, suppresses that one statement: // unset-sentinel-ok: busyTx carries the armed state, so 0 is a legal stamp here lastTxStart = Time::getMillis(); The reason is mandatory. A bare `unset-sentinel-ok`, or a colon with nothing after it, is reported instead of honoured - with a message saying so - so the only way to silence a site is to write down why it is safe. trunk-ignore still works, but this states the justification at the write and also applies when the script runs outside trunk. The marker is read from comment text collected during the same character-level pass that strips comments and literals, not by re-scanning the raw line. That is what keeps it out of reach of data: LOG_DEBUG("unset-sentinel-ok: ...") mutes nothing, because a string literal is not a comment. It is also consumed by the statement it was written for, so it cannot leak onto the next write - while still carrying across any number of intervening comment lines to the statement below, which is where a real justification wants to be written. Twelve fixtures added for the new behaviour: both comment styles, block and multi-line block comments, the bare form, the marker-in-a-string cases, and three leak cases. 35 total, all green, under bash 3.2 as well. Co-Authored-By: Claude Opus 5 * lint: watch the separate-flag stamps too, with their exemption stated at the write The nine stamps whose armed state lives in a companion boolean were previously just absent from the rule's list, which meant the reasoning for leaving them out existed only as prose in a shell script. They are now listed and individually opted out at the write, naming the flag that actually carries the armed state: // unset-sentinel-ok: haveSample carries the armed state, so 0 is a legal stamp lastSampleMs = Time::getMillis(); The point is what happens later. If someone rewrites `if (haveSample && ...)` as `if (lastSampleMs && ...)`, the field has silently acquired the 0 contract; with the opt-out sitting at the write, the claim to re-examine is in front of whoever makes that edit instead of buried in bin/. Every exemption was checked against its real read sites before being written, and three candidates did not survive that check. They stay off the list, because listing one would mean stamping an opt-out over a claim that does not hold: * nagCycleCutoff. handleInputEvent reads `if (nagCycleCutoff != UINT32_MAX)` without consulting isNagging, so at that read the field is its own armed flag with UINT32_MAX as the sentinel - and the arm at ExternalNotificationModule .cpp:521 can land exactly there. skipZero() cannot help: it lifts 0 to 1 and leaves UINT32_MAX alone, which UptimeClock.h's own static_assert pins. There is also a live boot-state bug behind this - the in-class initializer is 1 while isNagging starts false - and fixing the read is a behaviour change that belongs in its own PR. * TouchScreenBase::_start. Overloaded as an event stamp AND a `+ 30000` suppression deadline compared by signed subtraction, so a near-zero value reads as "long ago" rather than "armed 30s out" and LONG_PRESS re-fires. skipZero() does not fix this one either: 1 reads as long-ago exactly as 0 does. It needs the stamp and the deadline held separately. * StoreForwardModule::retry_delay. No reads at all today, so nothing misbehaves yet; exempting it now would pre-approve the raw arm for whoever implements the retry its own comment promises. The rule is silent across src/ on this tree, and the header records all three rejections so the next person does not have to re-derive them. The self-test's negative fixture no longer uses nagCycleCutoff as its example of a safely unlisted field - that would have encoded the opposite of what the header says. Co-Authored-By: Claude Opus 5 * fix(time,lint): guard the recomputed tx_after, and judge one write at a time Two review findings, both real. setTransmitDelay() recomputes p->tx_after from a clamp of three candidates, and that recomputation was still raw. Two lines above it, `if (p->tx_after)` is the read that takes 0 as "no delay wanted", so a clamp landing on 0 drops the CSMA backoff and the packet goes out immediately instead of after its computed delay. The first arm site in this function was already guarded; this one was missed because the value is not a plain `now + delay` and so does not fit timerEndsAtMillis() - it takes skipZero() instead. The narrowing order matters here and is spelled out at the site: add_delay is unsigned long, 64-bit on the portduino host, so the clamp can exceed UINT32_MAX there. skipZero() on the wide value would pass 0x100000000 through as non-zero and the store to this uint32_t field would then truncate it back to the 0 being avoided, so the cast comes first. The lint rule judged each write by the wrong text. rhs was taken from the write to the end of the accumulated statement, so a neighbour on the same line decided the verdict - and it was wrong in both directions: rebootAtMsec = millis() + 5; shutdownAtMsec = Time::timerEndsAtMillis(10); the later helper call suppressed a genuine raw arm rebootAtMsec = otherDeadline; shutdownAtMsec = millis(); the later millis() reported a safe copy rhs is now cut at its own semicolon. Six fixtures cover it, including both cases above, two raw writes on one line, two helper writes on one line, and a statement split across lines, which must still see its whole right-hand side. 41 fixtures total, green under bash 3.2. Co-Authored-By: Claude Opus 5 * time: arm the remaining 0-means-unset stamps through the helpers The follow-up sweep to the sixteen fields the previous commits covered. A field here uses 0 to mean "unset" - some read spells `if (f)`, `f != 0`, `f == 0 ||` or `f > 0`, or a site disarms it with `f = 0` - but it was armed from a raw clock read, so once per ~49.7-day wrap it stores the value its own readers treat as never-set. 34 fields, 58 arm sites. The rule could not have found most of them first. It treated `field = ` as inheriting whatever that variable did, which made the commonest shape in the tree invisible: one `now = millis()` at the top of a runOnce(), then several `xStartTime = now` below it. Listing those names would have bought no protection at all, so the scanner now tracks a local assigned from a clock and treats a write from it as the raw arm it is. One hop, one function, name-based, and it forgets a local reassigned from anything else; taint is dropped at each function boundary. Twelve fixtures pin it, including the negative cases - no leak across functions, `now` does not match `nowMs`, and neither `==` nor `+=` records anything. That pass immediately found a site the previous commits missed: setTransmitDelay() recomputes p->tx_after from a tainted `now`, two lines under the `if (p->tx_after)` read that takes 0 as "no delay wanted". Three of the fields are worth naming because the consequence is not cosmetic: * UpDownInterruptBase press/up/downStartTime - xDetected is only cleared INSIDE the block guarded by `xDetected && xStartTime > 0`, so a stored 0 makes both the entry and the exit condition unreachable and that button is dead for the rest of the boot, not for one tick. * PhoneAPI lastContactMsec - ServerAPI reads `lastContactMsec > 0` before the TCP idle close, and the field stays 0 until the next inbound packet, so a client that never speaks again leaks the socket for the life of the connection. * EInkDisplay lastDrawMsec - `if (lastDrawMsec)` gates every plain display() call on a keyframe having been shown, so a stored 0 stops the screen updating until something calls Screen::forceDisplay() again. TransmitHistory needed more than its arm sites. getLastSentToMeshMillis() returns 0 to mean "module has never sent", and besides the two stores, both reconstruction helpers end in `millis() - msAgo`, which can produce a 0 of their own. All three computed returns are guarded; the deliberate `return 0;` sentinels are untouched. Judged and deliberately not changed: * nRF54L15 connect_time_ms is armed from k_uptime_get_32(), not millis(). It is guarded with skipZero() but keeps its own clock - swapping in Time::getMillis() would have it compared against a k_uptime now at the watchdog read. The rule now recognises that clock too, so listing the field is not an empty gesture. * RotaryEncoderInterruptBase pressStartTime shares a name with the UpDown field and has a different contract: no read here tests the stamp against 0, pressDetected is the only armed flag. Opted out at the write. Its lastPressLongEventTime sibling IS a `== 0` latch and is fixed. * PositionModule line 38 copies a value the enclosing `if (restored != 0)` has already proven non-zero. Opted out. * pmMeasureStarted, adminKeyFallbackRefillMs, the two autosave stamps and scrollStartDelay are lazy initialisations whose wrap behaviour costs at most one interval and drops nothing. Left alone, and not listed. 47 lint fixtures green, the rule silent across src/, full native suite 1421/1421. Co-Authored-By: Claude Opus 5 * time: dodge the wrap at the clock read, not only at the store Review on #11830 made a point that was right and that this branch had wrong. Applying skipZero() at the STORE while a reader measures elapsed time against a raw clock splits the two sides apart for one tick per ~49.7-day wrap: the stamp becomes 1 while `now` is still 0, so `now - stamp` is UINT32_MAX and a brand new stamp reads as about 49.7 days old. Every elapsed-since guard then fires when it must not. Concretely, UpDownInterruptBase computed `now - pressStartTime` and emitted a long press for a fresh press, and TraceRouteModule read `now - lastTraceRouteTime < cooldownMs` as false and bypassed its cooldown. So the dodge moves to the read. Time::stampMillis() is getMillis() with the one 0 tick called 1; a site that both stores a stamp and measures against stamps reads the clock once through it and stores that value directly. Nine files, and the 1 ms skew is the same one skipZero() already documents. Where the clock arrives as a PARAMETER the store keeps its own skipZero() as well, because the function cannot assume the caller dodged anything. Removing that was a real regression and test_nexthop_routing caught it: noteRouteLearned() and noteRouteSuccess() are called with a literal 0 by test_health_learn_never_stores_zero_sentinel and test_health_success_never_stores_zero_sentinel, which assert the store normalises it - 0 is the empty-slot marker getOrAllocRouteHealth() evicts on. The two guards compose without shifting twice, since skipZero() of a non-zero value is itself. trySmartBroadcast() and directResponseAllowed() have the same parameter shape and keep their store-side guard for the same reason. Only stores fed by a stampMillis() local in the same function are bare. EInkParallelDisplay was missed the first time: the third class in the family, still storing skipZero(getMillis()) while rate-limiting against a raw millis() local. Normalised like its siblings. The lint rule gained three false positives with the class-scope tracking, all of them shapes that are not class bodies at all: template void f(T x) { uint32_t lastSort = millis(); } class Foo { void tick() { uint32_t lastSort = millis(); } }; void g(struct Bar *b) { uint32_t lastSort = millis(); } Two causes. pending_class matched class/struct anywhere on the line, so a template parameter list and an elaborated type in a parameter list both marked the following FUNCTION body as class scope; it is anchored to the start of the line now. And update_scope() runs at the end of a line, so a body opened earlier on the same line had not been counted when the statement was judged; is_declaration() now also counts unmatched braces earlier in the statement. The rule is blocking and `template ` is ordinary C++, so these would have reddened files nobody touched. note_taint() also never received the per-write `;` cut the judging path was given earlier in review, so on a line holding two statements it learned taint from the neighbour. Same cut applied. Five tests in test_uptime_clock pin the contract, including one that asserts the old store-only shape really does produce UINT32_MAX, and one that pins the 1 ms skew at 399 rather than 400 so nobody "corrects" it back into a raw read. Lint fixtures 53 -> 65. Full native suite 1426/1426, rule silent across src/. Known residual, deliberately not changed: a store that dodges zero while its reader measures through a Throttle:: helper still splits for that one tick, because those helpers read the clock internally and raw. About eight sites tree-wide, including PositionModule trySmartBroadcast and the lastContactMsec TCP idle check. Closing it means making Throttle read through the dodge, which was proposed on #11692 and declined there pending a caller audit, so fixing one site here would only make the tree inconsistent. The direction is also the same one the un-dodged code already took: a fresh stamp reads as old, and the guards involved were already passing on a 0 stamp. Co-Authored-By: Claude Opus 5 * lint: a dodged value is safe to copy, not to do arithmetic on Two more review findings on the rule, both real, both false negatives. Arithmetic on an already-dodged value was excused. stampMillis() guarantees only its own result, so `now + 5000` can carry a non-zero stamp straight back onto the sentinel - 0xFFFFEC78 + 5000 is exactly 0. That sum is precisely what Time::timerEndsAtMillis() exists to dodge, and the rule was waving it through because a helper name appeared somewhere in the expression. Worse, a fixture asserted that behaviour was correct, so the self-test was pinning the hole open. A local holding a dodged value is now tracked separately from a tainted one: it may be stored or copied straight through, but + or - applied at the OUTERMOST level is reported and the message points at timerEndsAtMillis(). Depth-aware, so the operator inside Time::skipZero(getMillis() - msAgo) is still fine, and so is the `(d == 0) ? 0 : timerEndsAtMillis(d)` arming form, which has no top-level operator at all. The wrong fixture is replaced by four: store-through, copy one more hop, arithmetic on a dodged local, and arithmetic on a direct helper call. A class body that opens and closes on one line was never recognised. The header check rejected it because the line ends in a semicolon, which a one-liner body always does, and even once armed the class brace counted as a function body and excused the member. Both halves fixed: the header arms on the brace rather than on the absence of a semicolon, and when the body opened on the statement being judged, one unmatched brace is class scope while two is a method body inside it. Getting that wrong first broke every multi-line class, because setting the per-statement flag without also arming pending_class meant update_scope() never registered the body - the three existing class fixtures caught it. 72 fixtures, green under bash 3.2, shellcheck clean, rule silent across src/. No src/ or test/ file changes, so the native suite is untouched by this commit. Co-Authored-By: Claude Opus 5 * style(time): trim comments to the house limit --------- Co-authored-by: Tom <116762865+Nestpebble@users.noreply.github.com> Co-authored-by: nomdetom Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com> Co-authored-by: Claude Opus 5 --- bin/lint-unset-sentinel-millis.sh | 201 ++++++++++++++++- bin/test-lint-unset-sentinel-millis.sh | 211 ++++++++++++++++++ src/Power.cpp | 3 +- src/UptimeClock.h | 7 + src/gps/RTC.cpp | 2 +- src/graphics/BaseUIEInkDisplay.cpp | 3 +- src/graphics/EInkDisplay2.cpp | 3 +- src/graphics/EInkParallelDisplay.cpp | 9 +- src/graphics/Screen.cpp | 2 +- src/graphics/draw/UIRenderer.cpp | 7 +- src/input/ExpressLRSFiveWay.cpp | 2 +- src/input/LinuxJoystick.cpp | 2 + src/input/RotaryEncoderInterruptBase.cpp | 4 +- src/input/TrackballInterruptBase.cpp | 8 +- src/input/UpDownInterruptBase.cpp | 3 +- src/mesh/IndicatorSerial.cpp | 3 +- src/mesh/NextHopRouter.cpp | 11 +- src/mesh/PhoneAPI.cpp | 4 +- src/mesh/ReliableRouter.cpp | 2 +- src/mesh/Router.cpp | 2 +- src/mesh/TransmitHistory.cpp | 13 +- src/mesh/api/PacketAPI.cpp | 3 +- src/mesh/eth/ethOTA.cpp | 5 +- src/mesh/http/WebServer.cpp | 2 +- src/mesh/wifi/WiFiAPClient.cpp | 3 +- src/modules/DropzoneModule.cpp | 5 +- src/modules/KeyVerificationModule.cpp | 3 +- src/modules/PositionModule.cpp | 7 +- src/modules/Telemetry/AirQualityTelemetry.cpp | 3 +- src/modules/Telemetry/DeviceTelemetry.cpp | 2 +- .../Telemetry/EnvironmentTelemetry.cpp | 3 +- src/modules/Telemetry/HealthTelemetry.cpp | 3 +- src/modules/Telemetry/PowerTelemetry.cpp | 3 +- src/modules/TraceRouteModule.cpp | 7 +- src/modules/TrafficManagementModule.cpp | 5 +- src/mqtt/MQTT.cpp | 3 +- .../extra_variants/t5s3_epaper/variant.cpp | 5 +- src/platform/nrf54l15/NRF54L15Bluetooth.cpp | 3 +- src/platform/stm32wl/main-stm32wl.cpp | 3 +- test/test_uptime_clock/test_main.cpp | 60 +++++ 40 files changed, 566 insertions(+), 64 deletions(-) diff --git a/bin/lint-unset-sentinel-millis.sh b/bin/lint-unset-sentinel-millis.sh index 1ff34da200..d3348cb8f4 100755 --- a/bin/lint-unset-sentinel-millis.sh +++ b/bin/lint-unset-sentinel-millis.sh @@ -85,7 +85,7 @@ set -uo pipefail # Millisecond fields this rule watches. See the notes above before editing. -SENTINELS='rebootAtMsec|shutdownAtMsec|enterDfuAtMsec|alertBannerUntil|pulseOffAt|delayedPulseAt|ntp_renew|tx_after|suppressTouchTapUntilMs|fixHoldEnds|lastChipRecoveryMs|activeReceiveStart|rxTimeMsec|lastInterruptTime|lastSentReply|lastSort|lastTxStart|lastHeartbeat|lastAveraged|lastSampleMs|lastIaqMs|last_format_ms|nextRepeatX|nextRepeatY|_cached_next_run' +SENTINELS='rebootAtMsec|shutdownAtMsec|enterDfuAtMsec|alertBannerUntil|pulseOffAt|delayedPulseAt|ntp_renew|tx_after|suppressTouchTapUntilMs|fixHoldEnds|lastChipRecoveryMs|activeReceiveStart|rxTimeMsec|lastInterruptTime|lastSentReply|lastSort|lastTxStart|lastHeartbeat|lastAveraged|lastSampleMs|lastIaqMs|last_format_ms|nextRepeatX|nextRepeatY|_cached_next_run|connect_time_ms|directionStartTime|downStartTime|fileage|keyDownStart|lastAuthFailure|lastContactMsec|lastDirectResponseMs|lastDiskSave|lastDownLongEventTime|lastDrawMsec|lastGpsSend|lastHeadingAtMs|lastHeapLogTime|lastHeapWarning|lastLfsFormatMs|lastMillis|lastPressLongEventTime|lastRemoteSessionMs|lastSentStatsToPhone|lastSentToPhone|lastSetFromPhoneNtpOrGps|lastTraceRouteTime|lastUpLongEventTime|lastUpdateMs|last_probe|last_report_to_map|lastrun_ntp|navBarLastShown|pressStartTime|startSendConditions|suppressFromMs|touchResumeAtMs|upStartTime' for target in "$@"; do [[ -f $target ]] || continue @@ -155,21 +155,200 @@ for target in "$@"; do return (c ~ /[A-Za-z0-9_]/) } + # Brace depth, and which depths are a class/struct BODY rather than a function body. Needed + # because a typed declaration means opposite things in the two places: inside a function it is a + # throwaway local that shadows the field, but at class scope it IS the field, with an initializer + # that can read the clock - src/modules/SerialModule.h does exactly that. Treating the second as a + # local silently excused a real arm site. + function update_scope(s, i, c) { + for (i = 1; i <= length(s); i++) { + c = substr(s, i, 1) + if (c == "{") { + depth++ + if (pending_class) { class_body[depth] = 1; pending_class = 0 } + } else if (c == "}") { + delete class_body[depth] + if (depth > 0) depth-- + } + } + } + + # Is the statement being judged sitting directly in a class body? `opened` is the net brace + # count seen earlier in this same statement, which update_scope() has not applied yet: a body + # opened on this very line (a one-line inline method) puts the statement inside a function, not + # in the class body. + function at_class_scope(opened) { + # The class body opened on this very statement, so its own brace is the class brace: exactly + # one unmatched brace means class scope, two or more means a method body inside it. + if (stmt_class_brace) return (opened == 1) + return ((depth in class_body) && opened <= 0) + } + + # Net unmatched `{` in the first `at` characters of the statement being judged. + function braces_before(s, at, i, c, n) { + n = 0 + for (i = 1; i < at && i <= length(s); i++) { + c = substr(s, i, 1) + if (c == "{") n++ + else if (c == "}") n-- + } + return n + } + # A local declaration that happens to reuse a sentinel name shadows the field and carries none # of its contract, so it is not this rule business. Detected by a type-ish token immediately # before the name - `uint32_t tx_after = millis() + d;` declares a local, `tx_after = ...` does # not. Kept narrow: only the spellings this tree actually uses for a millis value. + # + # Only honoured inside a function body; see update_scope() for why class scope is different. function is_declaration(s, at, head) { + if (at_class_scope(braces_before(s, at))) return 0 head = substr(s, 1, at - 1) sub(/[ \t]*(\*|&)?[ \t]*$/, "", head) return (head ~ /(^|[^A-Za-z0-9_])(uint32_t|uint64_t|int32_t|unsigned[ \t]+long|unsigned[ \t]+int|unsigned|long|int|auto|size_t|TickType_t)$/) } + # Does this statement read a clock directly? Matches millis(), Time::getMillis() and any wrapper + # whose name ends in millis, which is how almost every clock read in this tree spells itself, plus + # Zephyr k_uptime_get_32() - the nRF54L15 BLE code has no millis() at all and wraps at 32 bits just + # the same, so a sentinel armed from it needs the same guard. + function reads_clock(s) { return (s ~ /[Mm]illis[ \t]*\(/ || s ~ /k_uptime_get_32[ \t]*\(/) } + + # Already routed through a helper that dodges 0, so it is the fix rather than the defect. Note + # stampMillis() belongs here even though its name ends in millis: a local read through it holds a + # value that is already non-zero, so a write from that local is safe and must not be flagged. + function is_safe_arm(s) { return (s ~ /skipZero/ || s ~ /timerEndsAtMillis/ || s ~ /stampMillis/) } + + # Does the expression apply + or - to an already-dodged value at the OUTERMOST level? A dodged + # value is safe to store or copy, but not to do arithmetic on: stampMillis() guarantees only its + # own result, and `now + 5000` can carry a non-zero stamp straight back onto 0 - 0xFFFFEC78 + 5000 + # is exactly 0. That sum is what Time::timerEndsAtMillis() exists to dodge, so it has to be + # reported rather than excused. + # + # Depth-aware on purpose: the operator inside Time::skipZero(getMillis() - msAgo) is at depth 1 + # and is fine, because the helper wraps the result. So is the `? :` in the ternary arming form, + # which has no top-level + or - at all. + function toplevel_arith(s, i, n, c, d, prev) { + n = length(s); d = 0; prev = "" + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (c == "(") d++ + else if (c == ")") d-- + else if (d == 0 && (c == "+" || c == "-")) { + # not a unary sign, and not part of -> or ++/-- + if (prev != "" && prev != "(" && prev != "," && prev != "=" && prev != "+" && + prev != "-" && prev != "*" && prev != "/" && prev != "?" && prev != ":" && + substr(s, i + 1, 1) != ">") + return 1 + } + if (c != " " && c != "\t") prev = c + } + return 0 + } + + # Remember a local that was just assigned from a clock, so `field = now` a few lines later is + # recognised as the raw arm it really is. Without this the rule is blind to the commonest shape + # in the tree - `unsigned long now = millis();` at the top of a runOnce(), then half a dozen + # `xStartTime = now;` writes below it - and listing those fields would buy no protection at all. + # + # Deliberately shallow: one hop, within one function, name-based. It records ` = ` + # and ` = `, and it FORGETS the name when the same local is + # reassigned from anything else, so a variable reused for something unrelated stops matching. + # Taint is dropped at every function boundary (see the reset below), because a name that means a + # clock in one function usually means nothing in the next. + function note_taint(s, lhs, rhs, eqp, semi) { + eqp = index(s, "=") + if (eqp == 0) return + if (substr(s, eqp + 1, 1) == "=") return # `==` is a comparison + if (substr(s, eqp - 1, 1) ~ /[-+*\/%&|^!<>=]/) return # `+=`, `!=`, ... are not plain + lhs = substr(s, 1, eqp - 1) + rhs = substr(s, eqp + 1) + # This assignment only. Without the cut, a second statement on the same line teaches taint + # for the first - the same defect the judging path was fixed for. + semi = index(rhs, ";") + if (semi > 0) rhs = substr(rhs, 1, semi - 1) + # Take the last identifier on the left, which skips any type and `*`/`&` decoration. + if (!match(lhs, /[A-Za-z_][A-Za-z0-9_]*[ \t]*$/)) return + lhs = substr(lhs, RSTART, RLENGTH) + sub(/[ \t]+$/, "", lhs) + if (lhs == "") return + if (is_safe_arm(rhs) && !toplevel_arith(rhs)) { + delete tainted[lhs] + normalized[lhs] = 1 # holds a value that has already dodged 0 + } else if (reads_clock(rhs) || rhs_is_tainted(rhs)) { + tainted[lhs] = 1 + delete normalized[lhs] + } else { + delete tainted[lhs] # reused for something else - stop trusting the name + delete normalized[lhs] + } + } + + # Is any tainted local read in this expression, as a whole token? Token-bounded so a tainted + # `now` does not match `nowMs` or `snowfall`. Local names are plain identifiers, so using one + # as a match() pattern carries no regex metacharacters. + function rhs_is_normalized(s, name, t, p, before, after) { + for (name in normalized) { + t = s + while (match(t, name)) { + p = RSTART + before = (p == 1) ? " " : substr(t, p - 1, 1) + after = substr(t, p + length(name), 1) + if (before !~ /[A-Za-z0-9_]/ && after !~ /[A-Za-z0-9_]/) return 1 + t = substr(t, p + length(name)) + if (t == "") break + } + } + return 0 + } + + function rhs_is_tainted(s, name, t, p, before, after) { + for (name in tainted) { + t = s + while (match(t, name)) { + p = RSTART + before = (p == 1) ? " " : substr(t, p - 1, 1) + after = substr(t, p + length(name), 1) + if (before !~ /[A-Za-z0-9_]/ && after !~ /[A-Za-z0-9_]/) return 1 + t = substr(t, p + length(name)) + if (t == "") break + } + } + return 0 + } + BEGIN { LINE_CAP = 12 } # give up accumulating a statement after this many lines { code = strip_noncode($0) + # A class or struct header whose body opens on this line or the next. Anchored at the start + # of the line, because the keyword appears mid-line in shapes that are not class bodies at + # all: `template ` on a function, and an elaborated type in a parameter list such as + # `void g(struct Bar *b)`. Both used to mark the following FUNCTION body as class scope, which + # then reported every typed local in it. Not a forward declaration either, which ends in a + # semicolon with no brace. + stmt_class_brace = 0 + if (code ~ /^[ \t]*(class|struct)[ \t]+[A-Za-z_][A-Za-z0-9_]*/) { + # The body may open on this line or the next. `class Foo;` is a forward declaration and + # opens nothing; `class Foo { uint32_t t = millis(); };` opens AND closes here, so the + # trailing semicolon cannot be used to rule it out. + if (code ~ /\{/) { + # Body opens on this line. stmt_class_brace judges THIS statement (a one-liner + # whose member sits after the brace); pending_class is still needed so + # update_scope() registers the body for the lines that follow. + stmt_class_brace = 1 + pending_class = 1 + } else if (code !~ /;[ \t]*$/) { + pending_class = 1 # body opens on a later line + } + } + + # A closing brace in column 1 is the end of a function as this tree formats code, and a + # local called `now` there has nothing to do with the one in the next function. clang-format + # is enforced repo-wide, so this is reliable enough for a one-hop heuristic. + if ($0 ~ /^\}/) delete tainted + # An opt-out is sticky until the next statement that actually contains code is judged. That # is what lets it sit on its own line above the write, however many comment lines intervene, # without leaking past the statement it was written for. @@ -222,8 +401,15 @@ for target in "$@"; do # variable inherits whatever that one did, and anything already routed through # the helpers is the fix rather than the defect. Matching `millis` loosely # covers millis(), Time::getMillis() and any wrapper ending in millis. - if (rhs !~ /[Mm]illis[ \t]*\(/ || rhs ~ /skipZero/ || rhs ~ /timerEndsAtMillis/) - continue + # Arithmetic applied on top of an already-dodged value can wrap it back onto 0, + # so it is reported even though a helper appears in the expression. + if ((is_safe_arm(rhs) || rhs_is_normalized(rhs)) && !toplevel_arith(rhs)) { + continue # stored or copied straight through - safe + } + if (is_safe_arm(rhs) && !toplevel_arith(rhs)) + continue # already routed through the helpers + if (!reads_clock(rhs) && !rhs_is_tainted(rhs) && !rhs_is_normalized(rhs)) + continue # not a clock read, directly or via a local holding one if (pending_ok) continue # opted out, with a reason, at the write if (pending_bare) @@ -235,12 +421,21 @@ for target in "$@"; do hit_name[k] " is 0-means-unset - arm it with Time::timerEndsAtMillis(delay), or Time::skipZero(Time::getMillis()) for a stamp (see src/UptimeClock.h)", "unset-sentinel-millis" } + # Learn from this statement before dropping it: `now = millis()` here is what makes + # `field = now` below recognisable. Done after judging so a sentinel write cannot + # taint its own name. + if (nhits == 0) note_taint(stmt) + # Comment-only lines carry an opt-out toward the write below them, so they must not # clear it; a statement with real code in it consumes it. if (stmt ~ /[^ \t]/) { pending_ok = 0; pending_bare = 0 } stmt = "" nhits = 0 } + + # Last, so every brace on this line counts toward the scope of the NEXT line: a declaration + # sits at the depth its own line opened with. + update_scope(code) } ' "$target" done diff --git a/bin/test-lint-unset-sentinel-millis.sh b/bin/test-lint-unset-sentinel-millis.sh index 94c509f415..ab0923182f 100755 --- a/bin/test-lint-unset-sentinel-millis.sh +++ b/bin/test-lint-unset-sentinel-millis.sh @@ -41,6 +41,27 @@ run_case() { fi } +# run_case_h - same, but the fixture is a HEADER, so class-scope +# cases can be pinned. A typed declaration means opposite things in a class body and a function body. +run_case_h() { + local name="$1" expect="$2" body="$3" + local dir="$WORK/case_h" + rm -rf "$dir" + mkdir -p "$dir/src" + printf '%s\n' "$body" >"$dir/src/fixture.h" + + local got want + got=$(cd "$dir" && "$LINT" src/fixture.h | awk -F: '{print $2}' | paste -sd, -) + want=$(printf '%s' "$expect" | paste -sd, -) + + if [[ $got == "$want" ]]; then + echo "PASS $name" + else + echo "FAIL $name: expected lines [$want], got [$got]" + FAILURES=$((FAILURES + 1)) + fi +} + # --- must be reported --------------------------------------------------------- run_case "bare millis() sum" "2" 'void f() { @@ -209,6 +230,82 @@ run_case "opt-out covers both writes on its own line only" "3" 'void f() { rebootAtMsec = millis() + 5000; }' +# --- clock held in a local --------------------------------------------------- +# +# The commonest shape in the tree: one `now = millis()` at the top of a runOnce(), then several +# writes from it. Without these the rule is blind to every such field and listing one buys nothing. + +run_case "stamp copied from a tainted local" "3" 'void f() { + unsigned long now = millis(); + rebootAtMsec = now; +}' + +run_case "deadline built from a tainted local" "3" 'void f() { + uint32_t now = Time::getMillis(); + tx_after = now + delay; +}' + +run_case "several writes from one tainted local" "3 +4 +5" 'void f() { + unsigned long now = millis(); + pulseOffAt = now; + rebootAtMsec = now + 5000; + lastSort = now; +}' + +run_case "taint carried one hop through another local" "4" 'void f() { + uint32_t now = millis(); + uint32_t alsoNow = now; + lastSort = alsoNow; +}' + +run_case "tainted local still fixable via the helpers" "" 'void f() { + unsigned long now = millis(); + rebootAtMsec = Time::skipZero(now); +}' + +run_case "tainted local with an opt-out" "" 'void f() { + unsigned long now = millis(); + // unset-sentinel-ok: heldX carries the armed state + nextRepeatX = now + JOY_REPEAT_INTERVAL_MS; +}' + +# --- the taint must NOT spread further than one function, one name ----------- + +run_case "untainted local is not flagged" "" 'void f() { + uint32_t now = packet->rx_time; + rebootAtMsec = now; +}' + +run_case "similarly named local is not tainted" "" 'void f() { + uint32_t now = millis(); + rebootAtMsec = nowMs; +}' + +run_case "taint dropped when the local is reassigned from something else" "" 'void f() { + uint32_t now = millis(); + now = packet->rx_time; + rebootAtMsec = now; +}' + +run_case "taint does not cross a function boundary" "" 'void f() { + uint32_t now = millis(); +} +void g() { + rebootAtMsec = now; +}' + +run_case "taint from a comparison is not recorded" "" 'void f() { + if (now == millis()) {} + rebootAtMsec = now; +}' + +run_case "compound assignment does not taint" "" 'void f() { + now += millis(); + rebootAtMsec = now; +}' + # --- one write must not be judged by its neighbour on the same line ---------- # # rhs used to run to the end of the accumulated statement, so a neighbour decided this write. @@ -239,6 +336,120 @@ run_case "multi-line statement still sees its whole right-hand side" "2" 'void f millis() + 43200 * 1000; }' +# --- stampMillis() is the read-side dodge, not a raw clock ------------------- +# +# Its name ends in millis, so the clock-read test matches it. It must still count as safe, or every +# site that normalises at the read and then stores the local gets flagged. + +run_case "storing a dodged local straight through is safe" "" 'void f() { + uint32_t now = Time::stampMillis(); + lastSort = now; +}' + +# A dodged value is safe to store or copy, NOT to do arithmetic on: stampMillis() guarantees only its +# own result, and 0xFFFFEC78 + 5000 is exactly 0. That sum is what timerEndsAtMillis() is for. +run_case "arithmetic on a dodged local can wrap back onto 0" "3" 'void f() { + uint32_t now = Time::stampMillis(); + rebootAtMsec = now + 5000; +}' + +run_case "arithmetic on a direct helper call is reported too" "2" 'void f() { + rebootAtMsec = Time::stampMillis() + 5000; +}' + +run_case "an operator INSIDE the helper call is fine" "" 'void f() { + lastSort = Time::skipZero(Time::getMillis() - msAgo); +}' + +run_case "copying a dodged local one more hop stays safe" "" 'void f() { + uint32_t now = Time::stampMillis(); + uint32_t alsoNow = now; + lastSort = alsoNow; +}' + +run_case "stampMillis directly in the write is safe" "" 'void f() { + lastSort = Time::stampMillis(); +}' + +run_case "a raw read after a safe one re-taints the local" "5" 'void f() { + uint32_t now = Time::stampMillis(); + lastSort = now; + now = millis(); + rebootAtMsec = now; +}' + +# --- class scope versus function scope --------------------------------------- +# +# A typed declaration is a shadowing local inside a function, but AT CLASS SCOPE it is the field +# itself, with an initializer that can read the clock - src/modules/SerialModule.h does that today. +# Excusing the second as a local silently skipped a real arm site. + +run_case_h "class member initialised from the clock is reported" "2" 'class Foo { + uint32_t lastSort = millis(); +};' + +run_case_h "local inside an inline method is still excused" "6" 'class Foo { + void tick() + { + uint32_t lastSort = millis(); + } + uint32_t lastDrawMsec = millis(); +};' + +run_case_h "member already routed through the helpers is quiet" "" 'class Foo { + uint32_t lastSort = Time::stampMillis(); +};' + +run_case_h "forward declaration does not open a class body" "3" 'class Foo; +void f() { + lastSort = millis(); +}' + +# --- class scope must not swallow ordinary function bodies -------------------- +# +# The keyword appears mid-line in shapes that are not class bodies, and a body opened on the same +# line puts the statement inside a function. All three reported every typed local in the body. + +run_case "template on a function is not a class body" "" 'template void f(T x) { + uint32_t lastSort = millis(); +}' + +run_case "struct in a parameter list is not a class body" "" 'void g(struct Bar *b) { + uint32_t lastSort = millis(); +}' + +run_case_h "one-line inline method is a function body" "" 'class Foo { + void tick() { uint32_t lastSort = millis(); } +};' + +run_case_h "class with a multi-line method: member yes, local no" "6" 'class Foo { + void tick() + { + uint32_t lastSort = millis(); + } + uint32_t lastDrawMsec = millis(); +};' + +# note_taint gets the same per-write cut the judging path has. +run_case "taint is not learned from a neighbour on the same line" "" 'void f() { + uint32_t a = 0; uint32_t now = packet->rx_time; + rebootAtMsec = now; +}' + +# --- a class body that opens and closes on one line --------------------------- +# +# The trailing semicolon cannot be used to rule out a class header, because the whole body fits on +# the line; and that line\'s own brace is the CLASS brace, not a function body. + +run_case_h "one-line class body reports its member initialiser" "1" 'class Foo { uint32_t lastSort = millis(); };' + +run_case_h "one-line class with a one-line method excuses the local" "" 'class Foo { void tick() { uint32_t lastSort = millis(); } };' + +run_case_h "forward declaration opens nothing" "3" 'class Foo; +void f() { + lastSort = millis(); +}' + # --- scope ------------------------------------------------------------------- # test/ builds raw wrap values on purpose, so the rule must not reach into it. diff --git a/src/Power.cpp b/src/Power.cpp index f230318f67..ee14b3c639 100644 --- a/src/Power.cpp +++ b/src/Power.cpp @@ -19,6 +19,7 @@ #include "NodeDB.h" #include "PowerFSM.h" #include "Throttle.h" +#include "UptimeClock.h" #include "WaypointStore.h" #include "buzz/buzz.h" #include "configuration.h" @@ -1296,7 +1297,7 @@ void Power::logHeapUsage() memaudit::logBreakdown("periodic"); lastHeapLogFree = heapFree; - lastHeapLogTime = millis(); + lastHeapLogTime = Time::skipZero(Time::getMillis()); #endif } diff --git a/src/UptimeClock.h b/src/UptimeClock.h index 18895fd550..853fe7cbcf 100644 --- a/src/UptimeClock.h +++ b/src/UptimeClock.h @@ -58,6 +58,13 @@ inline uint32_t timerEndsAtMillis(uint32_t delayMs) return skipZero(getMillis() + delayMs); } +/// getMillis() for 0-means-unset stamps, with the 0 tick called 1. Use at the read when the value +/// is both stored and compared against stamps: skipZero() only at the store makes `now - stamp` wrap. +inline uint32_t stampMillis() +{ + return skipZero(getMillis()); +} + // skipZero() is the whole 0-means-unset contract in one expression, and it is constexpr, so pin it // here rather than only in test_uptime_clock: a build that breaks it stops at this header instead of // shipping a deadline that reads as never-set. The two obvious "simplifications" are what these diff --git a/src/gps/RTC.cpp b/src/gps/RTC.cpp index 58c00e12ee..ca5ebe146a 100644 --- a/src/gps/RTC.cpp +++ b/src/gps/RTC.cpp @@ -332,7 +332,7 @@ RTCSetResult perhapsSetRTC(RTCQuality q, const struct timeval *tv, bool forceUpd currentQuality = q; lastSetMsec = now; if (currentQuality >= RTCQualityNTP) { - lastSetFromPhoneNtpOrGps = now; + lastSetFromPhoneNtpOrGps = Time::skipZero(now); } // This delta value works on all platforms diff --git a/src/graphics/BaseUIEInkDisplay.cpp b/src/graphics/BaseUIEInkDisplay.cpp index 0506ba68dd..15d00f1ee7 100644 --- a/src/graphics/BaseUIEInkDisplay.cpp +++ b/src/graphics/BaseUIEInkDisplay.cpp @@ -1,6 +1,7 @@ #ifdef MESHTASTIC_INCLUDE_NICHE_GRAPHICS #include "./BaseUIEInkDisplay.h" +#include "UptimeClock.h" #include "configuration.h" #include "main.h" @@ -73,7 +74,7 @@ void BaseUIEInkDisplay::display() // Keyframe path. Returns true if a frame was pushed (sets lastDrawMsec). bool BaseUIEInkDisplay::forceDisplay(uint32_t msecLimit) { - const uint32_t now = millis(); + const uint32_t now = Time::stampMillis(); if (lastDrawMsec != 0 && (now - lastDrawMsec) < msecLimit) return false; diff --git a/src/graphics/EInkDisplay2.cpp b/src/graphics/EInkDisplay2.cpp index de058bc1f9..aec2c2caf2 100644 --- a/src/graphics/EInkDisplay2.cpp +++ b/src/graphics/EInkDisplay2.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #include "graphics/Backlight.h" @@ -59,7 +60,7 @@ bool EInkDisplay::forceDisplay(uint32_t msecLimit) // No need to grab this lock because we are on our own SPI bus // concurrency::LockGuard g(spiLock); - uint32_t now = millis(); + uint32_t now = Time::stampMillis(); uint32_t sinceLast = now - lastDrawMsec; if (adafruitDisplay && (sinceLast > msecLimit || lastDrawMsec == 0)) diff --git a/src/graphics/EInkParallelDisplay.cpp b/src/graphics/EInkParallelDisplay.cpp index a61b1bae97..d8d0f9475c 100644 --- a/src/graphics/EInkParallelDisplay.cpp +++ b/src/graphics/EInkParallelDisplay.cpp @@ -1,4 +1,5 @@ #include "EInkParallelDisplay.h" +#include "UptimeClock.h" #ifdef USE_EINK_PARALLELDISPLAY @@ -208,7 +209,7 @@ void EInkParallelDisplay::display(void) const uint16_t h = this->displayHeight; // Simple rate limiting: avoid very-frequent responsive updates - uint32_t nowMs = millis(); + uint32_t nowMs = Time::stampMillis(); if (lastUpdateMs != 0 && (nowMs - lastUpdateMs) < EPD_RESPONSIVE_MIN_MS) { LOG_DEBUG("rate-limited, skipping update"); return; @@ -367,11 +368,11 @@ void EInkParallelDisplay::display(void) startAsyncFullUpdate(forceFull ? CLEAR_SLOW : CLEAR_FAST); } - lastUpdateMs = millis(); + lastUpdateMs = Time::stampMillis(); previousImageHash = imageHash; // Keep same behavior as before - lastDrawMsec = millis(); + lastDrawMsec = Time::stampMillis(); } #ifdef EINK_LIMIT_GHOSTING_PX @@ -420,7 +421,7 @@ bool EInkParallelDisplay::forceDisplay(uint32_t msecLimit) if (!displayReady) return false; - uint32_t now = millis(); + uint32_t now = Time::stampMillis(); if (lastDrawMsec == 0 || (now - lastDrawMsec) > msecLimit) { display(); return true; diff --git a/src/graphics/Screen.cpp b/src/graphics/Screen.cpp index 6b2ed5b481..8d9c35ae42 100644 --- a/src/graphics/Screen.cpp +++ b/src/graphics/Screen.cpp @@ -493,7 +493,7 @@ float Screen::estimatedHeading(double lat, double lon) static double oldLat, oldLon; static float b = -1.0f; static uint32_t lastHeadingAtMs = 0; - const uint32_t now = millis(); + const uint32_t now = Time::stampMillis(); const uint32_t gpsUpdateIntervalSecs = Default::getConfiguredOrDefault(config.position.gps_update_interval, default_gps_update_interval); uint32_t effectiveUpdateIntervalSecs = gpsUpdateIntervalSecs; diff --git a/src/graphics/draw/UIRenderer.cpp b/src/graphics/draw/UIRenderer.cpp index 9a7638280c..58d759679b 100644 --- a/src/graphics/draw/UIRenderer.cpp +++ b/src/graphics/draw/UIRenderer.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #if HAS_SCREEN #include "CompassRenderer.h" @@ -2212,12 +2213,12 @@ void UIRenderer::drawNavigationBar(OLEDDisplay *display, OLEDDisplayUiState *sta if (navBarVisible && !navBarPrevVisible) { EINK_ADD_FRAMEFLAG(display, DEMAND_FAST); // Fast refresh when showing nav bar cosmeticRefreshDone = false; - navBarLastShown = millis(); + navBarLastShown = Time::skipZero(Time::getMillis()); } if (!navBarVisible && navBarPrevVisible) { - EINK_ADD_FRAMEFLAG(display, DEMAND_FAST); // Fast refresh when hiding nav bar - navBarLastShown = millis(); // Mark when it disappeared + EINK_ADD_FRAMEFLAG(display, DEMAND_FAST); // Fast refresh when hiding nav bar + navBarLastShown = Time::skipZero(Time::getMillis()); // Mark when it disappeared } if (!navBarVisible && navBarLastShown != 0 && !cosmeticRefreshDone) { diff --git a/src/input/ExpressLRSFiveWay.cpp b/src/input/ExpressLRSFiveWay.cpp index e9efeda52e..7d1e4de639 100644 --- a/src/input/ExpressLRSFiveWay.cpp +++ b/src/input/ExpressLRSFiveWay.cpp @@ -80,7 +80,7 @@ void ExpressLRSFiveWay::update(int *keyValue, bool *keyLongPressed) if (keyInProcess == NO_PRESS) { // New key down if (newKey != NO_PRESS) { - keyDownStart = Time::getMillis(); + keyDownStart = Time::skipZero(Time::getMillis()); // DBGLN("down=%u", newKey); } } else { diff --git a/src/input/LinuxJoystick.cpp b/src/input/LinuxJoystick.cpp index 8951a00b1c..d8f99ccec0 100644 --- a/src/input/LinuxJoystick.cpp +++ b/src/input/LinuxJoystick.cpp @@ -167,10 +167,12 @@ int32_t LinuxJoystick::runOnce() uint32_t now = millis(); if (heldX != 0 && (int32_t)(now - nextRepeatX) >= 0) { emitEvent((heldX < 0) ? INPUT_BROKER_LEFT : INPUT_BROKER_RIGHT); + // unset-sentinel-ok: heldX carries the armed state, so 0 is a legal deadline nextRepeatX = now + JOY_REPEAT_INTERVAL_MS; } if (heldY != 0 && (int32_t)(now - nextRepeatY) >= 0) { emitEvent((heldY < 0) ? INPUT_BROKER_UP : INPUT_BROKER_DOWN); + // unset-sentinel-ok: heldY carries the armed state, so 0 is a legal deadline nextRepeatY = now + JOY_REPEAT_INTERVAL_MS; } diff --git a/src/input/RotaryEncoderInterruptBase.cpp b/src/input/RotaryEncoderInterruptBase.cpp index c177403bf0..20e40d58e7 100644 --- a/src/input/RotaryEncoderInterruptBase.cpp +++ b/src/input/RotaryEncoderInterruptBase.cpp @@ -1,4 +1,5 @@ #include "RotaryEncoderInterruptBase.h" +#include "UptimeClock.h" #include "configuration.h" RotaryEncoderInterruptBase::RotaryEncoderInterruptBase(const char *name) : concurrency::OSThread(name) @@ -48,13 +49,14 @@ int32_t RotaryEncoderInterruptBase::runOnce() InputEvent e = {}; e.inputEvent = INPUT_BROKER_NONE; e.source = this->_originName; - unsigned long now = millis(); + unsigned long now = Time::stampMillis(); // Handle press long/short detection if (this->action == ROTARY_ACTION_PRESSED) { bool buttonPressed = !digitalRead(_pinPress); if (!pressDetected && buttonPressed) { pressDetected = true; + // unset-sentinel-ok: pressDetected is the armed flag; no read tests the stamp against 0 pressStartTime = now; pressAndTurnFired = false; } diff --git a/src/input/TrackballInterruptBase.cpp b/src/input/TrackballInterruptBase.cpp index 2be2495e12..e30cddd90e 100644 --- a/src/input/TrackballInterruptBase.cpp +++ b/src/input/TrackballInterruptBase.cpp @@ -203,20 +203,20 @@ int32_t TrackballInterruptBase::runOnce() if (e.inputEvent == INPUT_BROKER_NONE) { if (this->action == TB_ACTION_UP && !digitalRead(_pinUp) && !directionDetected) { directionDetected = true; - directionStartTime = millis(); + directionStartTime = Time::skipZero(Time::getMillis()); e.inputEvent = this->_eventUp; // send event first,will automatically trigger every 50ms * 3 after 500ms } else if (this->action == TB_ACTION_DOWN && !digitalRead(_pinDown) && !directionDetected) { directionDetected = true; - directionStartTime = millis(); + directionStartTime = Time::skipZero(Time::getMillis()); e.inputEvent = this->_eventDown; } else if (this->action == TB_ACTION_LEFT && !digitalRead(_pinLeft) && !directionDetected) { directionDetected = true; - directionStartTime = millis(); + directionStartTime = Time::skipZero(Time::getMillis()); e.inputEvent = this->_eventLeft; } else if (this->action == TB_ACTION_RIGHT && !digitalRead(_pinRight) && !directionDetected) { directionDetected = true; - directionStartTime = millis(); + directionStartTime = Time::skipZero(Time::getMillis()); e.inputEvent = this->_eventRight; } } diff --git a/src/input/UpDownInterruptBase.cpp b/src/input/UpDownInterruptBase.cpp index d597c8d8f4..5ad0d64ff0 100644 --- a/src/input/UpDownInterruptBase.cpp +++ b/src/input/UpDownInterruptBase.cpp @@ -1,4 +1,5 @@ #include "UpDownInterruptBase.h" +#include "UptimeClock.h" #include "configuration.h" UpDownInterruptBase::UpDownInterruptBase(const char *name) : concurrency::OSThread(name) @@ -50,7 +51,7 @@ int32_t UpDownInterruptBase::runOnce() { InputEvent e = {}; e.inputEvent = INPUT_BROKER_NONE; - unsigned long now = millis(); + unsigned long now = Time::stampMillis(); // Read all button states once at the beginning bool pressButtonPressed = !digitalRead(_pinPress); diff --git a/src/mesh/IndicatorSerial.cpp b/src/mesh/IndicatorSerial.cpp index 74608b5fa9..e146178954 100644 --- a/src/mesh/IndicatorSerial.cpp +++ b/src/mesh/IndicatorSerial.cpp @@ -1,6 +1,7 @@ #ifdef SENSECAP_INDICATOR #include "IndicatorSerial.h" +#include "UptimeClock.h" #include "concurrency/LockGuard.h" #include "mesh/comms/UARTProxy.h" #include @@ -61,7 +62,7 @@ void SensecapIndicator::probe_link() msg.data.ping = meshtastic_InterdeviceVersion_INTERDEVICE_VERSION_CURRENT; stamp_request(msg); send_uplink_unlocked(msg); - last_probe = millis(); + last_probe = Time::skipZero(Time::getMillis()); } // Read whatever is available on the link and process complete packets diff --git a/src/mesh/NextHopRouter.cpp b/src/mesh/NextHopRouter.cpp index f6c8857f46..00ccc814ef 100644 --- a/src/mesh/NextHopRouter.cpp +++ b/src/mesh/NextHopRouter.cpp @@ -201,7 +201,7 @@ void NextHopRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtast p->relay_node, wasAlreadyRelayer, weWereSoleRelayer); origTx->next_hop = p->relay_node; } - noteRouteLearned(p->from, p->relay_node, Time::getMillis()); // M3: anchor freshness (hot or overflow route) + noteRouteLearned(p->from, p->relay_node, Time::stampMillis()); // M3: anchor freshness (hot or overflow route) #if HAS_TRAFFIC_MANAGEMENT // Mirror the confirmed (and now unique-resolved) hop into the TMM overflow cache so it // survives even when the source isn't (or is no longer) in the hot NodeDB. @@ -313,7 +313,7 @@ std::optional NextHopRouter::getNextHop(NodeNum to, uint8_t relay_node) // a health record that still matches the stored byte; a next_hop set by another path (e.g. // TraceRouteModule) with no matching record is left authoritative. const RouteHealth *h = findRouteHealth(to); - if (h && h->lastNextHop == node->next_hop && isRouteStale(*h, Time::getMillis())) { + if (h && h->lastNextHop == node->next_hop && isRouteStale(*h, Time::stampMillis())) { LOG_INFO("Next hop 0x%x for 0x%08x stale (age/fails); flood and clear", node->next_hop, to); node->next_hop = NO_NEXT_HOP_PREFERENCE; // clear persisted route clearRouteHealth(to); // clear RAM health @@ -345,7 +345,7 @@ std::optional NextHopRouter::getNextHop(NodeNum to, uint8_t relay_node) uint8_t hint = trafficManagementModule->getNextHopHint(to); if (hint && hint != relay_node) { const RouteHealth *h = findRouteHealth(to); - if (h && h->lastNextHop == hint && isRouteStale(*h, Time::getMillis())) { + if (h && h->lastNextHop == hint && isRouteStale(*h, Time::stampMillis())) { LOG_INFO("TMM next hop 0x%x for 0x%08x stale (age/fails); flood and clear", hint, to); trafficManagementModule->clearNextHop(to); // clear overflow route (setNextHop won't store 0) clearRouteHealth(to); // clear RAM health @@ -444,7 +444,7 @@ int32_t NextHopRouter::doRetransmissions() { // Same clock Throttle reads, so setNextTx() deadlines and this test can't diverge under an // injected test clock. - uint32_t now = Time::getMillis(); + uint32_t now = Time::stampMillis(); int32_t d = INT32_MAX; // FIXME, we should use a better datastructure rather than walking through this map. @@ -617,6 +617,7 @@ void NextHopRouter::noteRouteLearned(NodeNum dest, uint8_t nextHop, uint32_t now h->lastNextHop = nextHop; h->consecutiveFailures = 0; } + // `now` is a parameter, so guard at the store too: 0 is the empty-slot marker. h->learnedAtMsec = Time::skipZero(now); } @@ -626,7 +627,7 @@ void NextHopRouter::noteRouteSuccess(NodeNum dest, uint32_t now) if (!h) return; // only routes we actually learned have health to refresh h->consecutiveFailures = 0; - h->learnedAtMsec = Time::skipZero(now); + h->learnedAtMsec = Time::skipZero(now); // a parameter, so guard at the store too } void NextHopRouter::noteRouteFailure(NodeNum dest) diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index 17f063a64f..aa5ff2d48e 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/src/mesh/PhoneAPI.cpp @@ -245,7 +245,7 @@ static void clearAuthSlot_LH(const PhoneAPI *p) PhoneAPI::PhoneAPI() { - lastContactMsec = millis(); + lastContactMsec = Time::skipZero(Time::getMillis()); std::fill(std::begin(recentToRadioPacketIds), std::end(recentToRadioPacketIds), 0); } @@ -437,7 +437,7 @@ bool PhoneAPI::checkConnectionTimeout() bool PhoneAPI::handleToRadio(const uint8_t *buf, size_t bufLength) { powerFSM.trigger(EVENT_CONTACT_FROM_PHONE); // As long as the phone keeps talking to us, don't let the radio go to sleep - lastContactMsec = millis(); + lastContactMsec = Time::skipZero(Time::getMillis()); memset(&toRadioScratch, 0, sizeof(toRadioScratch)); if (pb_decode_from_bytes(buf, bufLength, &meshtastic_ToRadio_msg, &toRadioScratch)) { diff --git a/src/mesh/ReliableRouter.cpp b/src/mesh/ReliableRouter.cpp index a3c86cd0ef..7be23ee810 100644 --- a/src/mesh/ReliableRouter.cpp +++ b/src/mesh/ReliableRouter.cpp @@ -181,7 +181,7 @@ void ReliableRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas // M3: an end-to-end ACK proves the directed route to the ACK's sender currently works, // so clear its failure count and refresh freshness (keeps a good route pinned). if (!isBroadcast(getFrom(p))) - noteRouteSuccess(getFrom(p), Time::getMillis()); + noteRouteSuccess(getFrom(p), Time::stampMillis()); } else { stopRetransmission(p->to, nakId); } diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index c4d670f830..55a10ebaa4 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1151,7 +1151,7 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p) JSONFile.close(); } JSONFile.open(portduino_config.JSONFilename + "_" + datetime, std::ios::out | std::ios::app); - fileage = millis(); + fileage = Time::skipZero(Time::getMillis()); } } if (portduino_config.JSONFilter == (_meshtastic_PortNum)0 || portduino_config.JSONFilter == p->decoded.portnum) { diff --git a/src/mesh/TransmitHistory.cpp b/src/mesh/TransmitHistory.cpp index 35144ec0d7..4b2ebc8a37 100644 --- a/src/mesh/TransmitHistory.cpp +++ b/src/mesh/TransmitHistory.cpp @@ -1,6 +1,7 @@ #include "TransmitHistory.h" #include "FSCommon.h" #include "SPILock.h" +#include "UptimeClock.h" #include "gps/RTC.h" #include @@ -81,7 +82,7 @@ void TransmitHistory::loadFromDisk() void TransmitHistory::setLastSentToMesh(uint16_t key) { - lastMillis[key] = millis(); + lastMillis[key] = Time::skipZero(Time::getMillis()); uint32_t now = getTime(); if (now >= 2) { const uint8_t flags = (getRTCQuality() == RTCQualityNone) ? ENTRY_FLAG_BOOT_RELATIVE : ENTRY_FLAG_NONE; @@ -94,7 +95,7 @@ void TransmitHistory::setLastSentToMesh(uint16_t key) // after boot so a crash-reboot loop can't avoid persisting. if (lastDiskSave == 0 || !Throttle::isWithinTimespanMs(lastDiskSave, SAVE_INTERVAL_MS)) { if (saveToDisk()) { - lastDiskSave = millis(); + lastDiskSave = Time::skipZero(Time::getMillis()); } } } @@ -151,7 +152,7 @@ uint32_t TransmitHistory::getLastSentAbsoluteMillis(uint32_t storedEpoch) const return 0; } - return millis() - msAgo; + return Time::skipZero(Time::getMillis() - msAgo); } uint32_t TransmitHistory::getLastSentBootRelativeMillis(uint32_t storedSeconds) const @@ -167,7 +168,7 @@ uint32_t TransmitHistory::getLastSentBootRelativeMillis(uint32_t storedSeconds) if (secondsAgo > BOOT_RELATIVE_RECOVERY_WINDOW_SEC) { return 0; } - return millis() - (secondsAgo * 1000); + return Time::skipZero(Time::getMillis() - (secondsAgo * 1000)); } uint32_t secondsAhead = storedSeconds - now; @@ -175,7 +176,7 @@ uint32_t TransmitHistory::getLastSentBootRelativeMillis(uint32_t storedSeconds) return 0; } - return millis(); + return Time::skipZero(Time::getMillis()); } uint32_t TransmitHistory::getLastSentToMeshMillis(uint16_t key) const @@ -286,7 +287,7 @@ void TransmitHistory::loadFromDisk() {} void TransmitHistory::setLastSentToMesh(uint16_t key) { - lastMillis[key] = millis(); + lastMillis[key] = Time::skipZero(Time::getMillis()); } uint32_t TransmitHistory::getLastSentToMeshEpoch(uint16_t key) const diff --git a/src/mesh/api/PacketAPI.cpp b/src/mesh/api/PacketAPI.cpp index c8adda5204..3590cfdb76 100644 --- a/src/mesh/api/PacketAPI.cpp +++ b/src/mesh/api/PacketAPI.cpp @@ -2,6 +2,7 @@ // First, in its own block so the include sorter keeps it there: configuration.h supplies the // variant defines mesh-pb-constants.h needs (portduino resolves MAX_NUM_NODES at runtime). +#include "UptimeClock.h" #include "configuration.h" #include "MeshService.h" @@ -59,7 +60,7 @@ bool PacketAPI::receivePacket(void) data_received = true; powerFSM.trigger(EVENT_INPUT); - lastContactMsec = millis(); + lastContactMsec = Time::skipZero(Time::getMillis()); meshtastic_ToRadio *mr; auto p = server->receivePacket()->move(); diff --git a/src/mesh/eth/ethOTA.cpp b/src/mesh/eth/ethOTA.cpp index b99ff73046..458b9cf5fc 100644 --- a/src/mesh/eth/ethOTA.cpp +++ b/src/mesh/eth/ethOTA.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #if HAS_ETHERNET && defined(HAS_ETHERNET_OTA) @@ -119,7 +120,7 @@ static bool authenticateClient(EthernetClient &client) uint8_t clientHash[OTA_HASH_SIZE]; if (!readExact(client, clientHash, OTA_HASH_SIZE)) { LOG_WARN("ETH OTA: Timeout reading auth response"); - lastAuthFailure = millis(); + lastAuthFailure = Time::skipZero(Time::getMillis()); return false; } @@ -136,7 +137,7 @@ static bool authenticateClient(EthernetClient &client) if (diff != 0) { LOG_WARN("ETH OTA: Authentication failed"); client.write(OTA_ERR_AUTH); - lastAuthFailure = millis(); + lastAuthFailure = Time::skipZero(Time::getMillis()); return false; } diff --git a/src/mesh/http/WebServer.cpp b/src/mesh/http/WebServer.cpp index 8a44895241..befc5d8776 100644 --- a/src/mesh/http/WebServer.cpp +++ b/src/mesh/http/WebServer.cpp @@ -111,7 +111,7 @@ static void handleWebResponse() static uint32_t lastHeapWarning = 0; if (lastHeapWarning == 0 || !Throttle::isWithinTimespanMs(lastHeapWarning, 30000)) { LOG_WARN("Low heap (%u bytes), not accepting HTTPS connections", freeHeap); - lastHeapWarning = millis(); + lastHeapWarning = Time::skipZero(Time::getMillis()); } } } diff --git a/src/mesh/wifi/WiFiAPClient.cpp b/src/mesh/wifi/WiFiAPClient.cpp index 35cb5653eb..8f53187279 100644 --- a/src/mesh/wifi/WiFiAPClient.cpp +++ b/src/mesh/wifi/WiFiAPClient.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #if HAS_WIFI #include "NodeDB.h" @@ -313,7 +314,7 @@ static int32_t reconnectWiFi() tv.tv_usec = 0; perhapsSetRTC(RTCQualityNTP, &tv); - lastrun_ntp = millis(); + lastrun_ntp = Time::skipZero(Time::getMillis()); } else { LOG_DEBUG("NTP Update failed"); } diff --git a/src/modules/DropzoneModule.cpp b/src/modules/DropzoneModule.cpp index 100b87662c..4adcf8c4a7 100644 --- a/src/modules/DropzoneModule.cpp +++ b/src/modules/DropzoneModule.cpp @@ -2,6 +2,7 @@ #include "DropzoneModule.h" #include "Meshservice->h" +#include "UptimeClock.h" #include "configuration.h" #include "gps/GeoCoord.h" #include "gps/RTC.h" @@ -39,13 +40,13 @@ ProcessMessage DropzoneModule::handleReceived(const meshtastic_MeshPacket &mp) snprintf(matchCompare, sizeof(matchCompare), "%s conditions", owner.short_name); if (received >= strlen(matchCompare) && strncasecmp(incomingMessage, matchCompare, strlen(matchCompare)) == 0) { LOG_DEBUG("Received dropzone conditions request"); - startSendConditions = millis(); + startSendConditions = Time::skipZero(Time::getMillis()); } snprintf(matchCompare, sizeof(matchCompare), "%s conditions", owner.long_name); if (received >= strlen(matchCompare) && strncasecmp(incomingMessage, matchCompare, strlen(matchCompare)) == 0) { LOG_DEBUG("Received dropzone conditions request"); - startSendConditions = millis(); + startSendConditions = Time::skipZero(Time::getMillis()); } return ProcessMessage::CONTINUE; } diff --git a/src/modules/KeyVerificationModule.cpp b/src/modules/KeyVerificationModule.cpp index eb6c496413..29594f64ab 100644 --- a/src/modules/KeyVerificationModule.cpp +++ b/src/modules/KeyVerificationModule.cpp @@ -3,6 +3,7 @@ #include "CryptoEngine.h" #include "HardwareRNG.h" #include "MeshService.h" +#include "UptimeClock.h" #include "gps/RTC.h" #include "graphics/draw/MenuHandler.h" #include "main.h" @@ -400,7 +401,7 @@ void KeyVerificationModule::resetToIdle() memset(hash1, 0, 32); memset(hash2, 0, 32); if (sessionFromRemote) - lastRemoteSessionMs = millis(); // start the cooldown when the session ends, not when it opened + lastRemoteSessionMs = Time::skipZero(Time::getMillis()); // start the cooldown when the session ends, not when it opened sessionFromRemote = false; currentNonce = 0; currentNonceTimestamp = 0; diff --git a/src/modules/PositionModule.cpp b/src/modules/PositionModule.cpp index 213237341a..68a7ccdf66 100644 --- a/src/modules/PositionModule.cpp +++ b/src/modules/PositionModule.cpp @@ -35,6 +35,7 @@ PositionModule::PositionModule() if (transmitHistory) { uint32_t restored = transmitHistory->getLastSentToMeshMillis(meshtastic_PortNum_POSITION_APP); if (restored != 0) { + // unset-sentinel-ok: the enclosing restored != 0 already rules out the unset value lastGpsSend = restored; LOG_INFO("Position: restored lastGpsSend from transmit history"); } @@ -550,7 +551,7 @@ int32_t PositionModule::runOnce() if (node == nullptr) return RUNONCE_INTERVAL; - uint32_t now = Time::getMillis(); + uint32_t now = Time::stampMillis(); // Local-only delivery, so it runs regardless of mesh opt-in state or channel utilization. // Only send while the queue is empty (phone assumed connected), like telemetry. The cadence @@ -709,7 +710,7 @@ void PositionModule::trySmartBroadcast(const meshtastic_PositionLite &selfPos, u if (!sendOurPosition()) return; - lastGpsSend = nowMs; + lastGpsSend = Time::skipZero(nowMs); // nowMs is a parameter, so guard at the store as well if (transmitHistory) transmitHistory->setLastSentToMesh(meshtastic_PortNum_POSITION_APP); LOG_DEBUG("Sent smart pos@%x:6 to mesh (distanceTraveled=%fm, minDistanceThreshold=%im, timeElapsed=%ims, " @@ -729,7 +730,7 @@ void PositionModule::handleNewPosition() meshtastic_PositionLite selfPos; if (!nodeDB->copyNodePosition(node->num, selfPos)) return; - trySmartBroadcast(selfPos, Time::getMillis()); + trySmartBroadcast(selfPos, Time::stampMillis()); } } diff --git a/src/modules/Telemetry/AirQualityTelemetry.cpp b/src/modules/Telemetry/AirQualityTelemetry.cpp index 2eb596bd96..bfb0ab73bb 100644 --- a/src/modules/Telemetry/AirQualityTelemetry.cpp +++ b/src/modules/Telemetry/AirQualityTelemetry.cpp @@ -1,4 +1,5 @@ #include "DebugConfiguration.h" +#include "UptimeClock.h" #include "configuration.h" #if HAS_TELEMETRY && !MESHTASTIC_EXCLUDE_AIR_QUALITY_SENSOR @@ -244,7 +245,7 @@ int32_t AirQualityTelemetryModule::runOnce() } else if (phoneDue && phoneAllowed) { // Mesh transmission isn't due yet, but we can still update the phone. if (sendTelemetry(NODENUM_BROADCAST, true)) { - lastSentToPhone = millis(); + lastSentToPhone = Time::skipZero(Time::getMillis()); // Correct the awake time, trimming to 0 const unsigned long elapsed = millis() - startAirQualityTelemetryCycle; awakeAheadOfTimeMs = elapsed >= awakeAheadOfTimeMs ? 0 : awakeAheadOfTimeMs - elapsed; diff --git a/src/modules/Telemetry/DeviceTelemetry.cpp b/src/modules/Telemetry/DeviceTelemetry.cpp index 8d3834137e..1b6b074e00 100644 --- a/src/modules/Telemetry/DeviceTelemetry.cpp +++ b/src/modules/Telemetry/DeviceTelemetry.cpp @@ -41,7 +41,7 @@ int32_t DeviceTelemetryModule::runOnce() sendTelemetry(NODENUM_BROADCAST, true); if (lastSentStatsToPhone == 0 || Throttle::hasElapsed(lastSentStatsToPhone, sendStatsToPhoneIntervalMs)) { sendLocalStatsToPhone(); - lastSentStatsToPhone = Time::getMillis(); + lastSentStatsToPhone = Time::skipZero(Time::getMillis()); } } return sendToPhoneIntervalMs; diff --git a/src/modules/Telemetry/EnvironmentTelemetry.cpp b/src/modules/Telemetry/EnvironmentTelemetry.cpp index a4143de299..e638a5db53 100644 --- a/src/modules/Telemetry/EnvironmentTelemetry.cpp +++ b/src/modules/Telemetry/EnvironmentTelemetry.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #if HAS_TELEMETRY && !MESHTASTIC_EXCLUDE_ENVIRONMENTAL_SENSOR @@ -466,7 +467,7 @@ int32_t EnvironmentTelemetryModule::runOnce() // Just send to phone when it's not our time to send to mesh yet // Only send while queue is empty (phone assumed connected) sendTelemetry(NODENUM_BROADCAST, true); - lastSentToPhone = millis(); + lastSentToPhone = Time::skipZero(Time::getMillis()); } } if (sleepOnNextExecution) { diff --git a/src/modules/Telemetry/HealthTelemetry.cpp b/src/modules/Telemetry/HealthTelemetry.cpp index 6ec316e701..10b8d68153 100644 --- a/src/modules/Telemetry/HealthTelemetry.cpp +++ b/src/modules/Telemetry/HealthTelemetry.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #if !MESHTASTIC_EXCLUDE_ENVIRONMENTAL_SENSOR && !MESHTASTIC_EXCLUDE_HEALTH_TELEMETRY && !defined(ARCH_PORTDUINO) @@ -90,7 +91,7 @@ int32_t HealthTelemetryModule::runOnce() // Just send to phone when it's not our time to send to mesh yet // Only send while queue is empty (phone assumed connected) sendTelemetry(NODENUM_BROADCAST, true); - lastSentToPhone = millis(); + lastSentToPhone = Time::skipZero(Time::getMillis()); } } if (sleepOnNextExecution) { diff --git a/src/modules/Telemetry/PowerTelemetry.cpp b/src/modules/Telemetry/PowerTelemetry.cpp index 684ce20133..f73fbf8120 100644 --- a/src/modules/Telemetry/PowerTelemetry.cpp +++ b/src/modules/Telemetry/PowerTelemetry.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #if !MESHTASTIC_EXCLUDE_ENVIRONMENTAL_SENSOR @@ -105,7 +106,7 @@ int32_t PowerTelemetryModule::runOnce() // Just send to phone when it's not our time to send to mesh yet // Only send while queue is empty (phone assumed connected) sendTelemetry(NODENUM_BROADCAST, true); - lastSentToPhone = millis(); + lastSentToPhone = Time::skipZero(Time::getMillis()); } } if (sleepOnNextExecution) { diff --git a/src/modules/TraceRouteModule.cpp b/src/modules/TraceRouteModule.cpp index 310cf4bc1b..4549f19df9 100644 --- a/src/modules/TraceRouteModule.cpp +++ b/src/modules/TraceRouteModule.cpp @@ -1,6 +1,7 @@ #include "TraceRouteModule.h" #include "MeshService.h" #include "NodeDB.h" +#include "UptimeClock.h" #include "graphics/Screen.h" #include "graphics/ScreenFonts.h" #include "graphics/SharedUIDisplay.h" @@ -534,7 +535,7 @@ const char *TraceRouteModule::getNodeName(NodeNum node) bool TraceRouteModule::startTraceRoute(NodeNum node) { LOG_INFO("TraceRoute startTraceRoute: node=0x%08x", node); - unsigned long now = millis(); + unsigned long now = Time::stampMillis(); if (node == 0 || node == NODENUM_BROADCAST) { LOG_ERROR("Invalid trace route node: 0x%08x", node); @@ -700,7 +701,7 @@ void TraceRouteModule::launch(NodeNum node) LOG_INFO("TraceRoute first init"); } - unsigned long now = millis(); + unsigned long now = Time::stampMillis(); if (initialized && lastTraceRouteTime > 0 && now - lastTraceRouteTime < cooldownMs) { unsigned long wait = (cooldownMs - (now - lastTraceRouteTime)) / 1000; bannerText = String("Wait for ") + String(wait) + String("s"); @@ -842,7 +843,7 @@ void TraceRouteModule::drawFrame(OLEDDisplay *display, OLEDDisplayUiState *state #endif // HAS_SCREEN int32_t TraceRouteModule::runOnce() { - unsigned long now = millis(); + unsigned long now = Time::stampMillis(); if (runState == TRACEROUTE_STATE_IDLE) { return INT32_MAX; diff --git a/src/modules/TrafficManagementModule.cpp b/src/modules/TrafficManagementModule.cpp index f12fbfbcdc..f0892e4590 100644 --- a/src/modules/TrafficManagementModule.cpp +++ b/src/modules/TrafficManagementModule.cpp @@ -1,4 +1,5 @@ #include "TrafficManagementModule.h" +#include "UptimeClock.h" #if HAS_TRAFFIC_MANAGEMENT @@ -1523,7 +1524,7 @@ bool TrafficManagementModule::shouldRespondToNodeInfo(const meshtastic_MeshPacke // request declined above never spends the budget). false forwards the request instead of consuming // it. Rationale in https://meshtastic.org/docs/development/reference/traffic-management-internals "Throttling direct // responses". - if (!directResponseAllowed(getFrom(p), p->to, clockMs())) { + if (!directResponseAllowed(getFrom(p), p->to, Time::skipZero(clockMs()))) { TM_LOG_DEBUG("NodeInfo direct response throttled for 0x%08x; forwarding request", getFrom(p)); return false; } @@ -1627,7 +1628,7 @@ bool TrafficManagementModule::directResponseAllowed(NodeNum requester, NodeNum t reqSlot->lastReplyMs = nowMs; tgtSlot->key = target; tgtSlot->lastReplyMs = nowMs; - lastDirectResponseMs = nowMs; + lastDirectResponseMs = Time::skipZero(nowMs); // a parameter, so guard at the store as well return true; } diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index 1c6cd57a0c..8108ba050c 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -3,6 +3,7 @@ #include "NodeDB.h" #include "PowerFSM.h" #include "ServiceEnvelope.h" +#include "UptimeClock.h" #include "configuration.h" #include "main.h" #include "mesh/Channels.h" @@ -870,5 +871,5 @@ void MQTT::perhapsReportToMap() packetPool.release(mp); // Update the last report time - last_report_to_map = millis(); + last_report_to_map = Time::skipZero(Time::getMillis()); } diff --git a/src/platform/extra_variants/t5s3_epaper/variant.cpp b/src/platform/extra_variants/t5s3_epaper/variant.cpp index 548fec421d..39ebcdc231 100644 --- a/src/platform/extra_variants/t5s3_epaper/variant.cpp +++ b/src/platform/extra_variants/t5s3_epaper/variant.cpp @@ -1,3 +1,4 @@ +#include "UptimeClock.h" #include "configuration.h" #ifdef T5_S3_EPAPER_PRO @@ -555,7 +556,7 @@ struct TouchLightSleepEndObserver { } touchStateEpoch++; - touchResumeAtMs = millis(); + touchResumeAtMs = Time::skipZero(Time::getMillis()); touchIndicatorRefreshPending = !isTouchInputEnabled(); #ifdef MESHTASTIC_INCLUDE_NICHE_GRAPHICS // Clear sleep-time touch overlay after wake. @@ -602,7 +603,7 @@ bool readTouch(int16_t *x, int16_t *y) LOG_DEBUG("touchscreen1: wakeup() on deferred resume"); touch.wakeup(); touchNeedsWake = false; - suppressFromMs = millis(); + suppressFromMs = Time::skipZero(Time::getMillis()); return false; } diff --git a/src/platform/nrf54l15/NRF54L15Bluetooth.cpp b/src/platform/nrf54l15/NRF54L15Bluetooth.cpp index 9ce0326316..8cef2fb59c 100644 --- a/src/platform/nrf54l15/NRF54L15Bluetooth.cpp +++ b/src/platform/nrf54l15/NRF54L15Bluetooth.cpp @@ -21,6 +21,7 @@ #include "BluetoothCommon.h" #include "BluetoothStatus.h" #include "PowerFSM.h" +#include "UptimeClock.h" #include "concurrency/OSThread.h" #include "configuration.h" #include "main.h" @@ -387,7 +388,7 @@ static void connected_cb(struct bt_conn *conn, uint8_t err) k_mutex_unlock(&ble_mutex); memset(lastToRadio, 0, sizeof(lastToRadio)); - connect_time_ms = k_uptime_get_32(); + connect_time_ms = Time::skipZero(k_uptime_get_32()); last_att_time_ms = connect_time_ms; char addr[BT_ADDR_LE_STR_LEN]; diff --git a/src/platform/stm32wl/main-stm32wl.cpp b/src/platform/stm32wl/main-stm32wl.cpp index 1f363edfa9..efeadedfeb 100644 --- a/src/platform/stm32wl/main-stm32wl.cpp +++ b/src/platform/stm32wl/main-stm32wl.cpp @@ -1,4 +1,5 @@ #include "FSCommon.h" +#include "UptimeClock.h" #include "configuration.h" #include "error.h" #include "gps/GPS.h" @@ -228,7 +229,7 @@ void preFSBegin() if (g_lfsCorruptMagic != LFS_CORRUPT_MAGIC) return; g_lfsCorruptMagic = 0; - lastLfsFormatMs = millis(); + lastLfsFormatMs = Time::skipZero(Time::getMillis()); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_FLASH_CORRUPTION_UNRECOVERABLE); fsFormat(); LOG_INFO("LittleFS format complete; restoring default settings"); diff --git a/test/test_uptime_clock/test_main.cpp b/test/test_uptime_clock/test_main.cpp index daf4cbed31..db0adacdb0 100644 --- a/test/test_uptime_clock/test_main.cpp +++ b/test/test_uptime_clock/test_main.cpp @@ -104,6 +104,61 @@ void test_timerEndsAtMillis_dodges_the_wrap_tick_itself() TEST_ASSERT_EQUAL_UINT32(1u, Time::timerEndsAtMillis(0)); // getMillis()==0, delayMs==0: sum is 0 } +// --- stampMillis(): one value for storing a stamp AND measuring against it --- + +void test_stampMillis_matches_getMillis_away_from_the_wrap() +{ + Time::setTestMillis(123456u); + TEST_ASSERT_EQUAL_UINT32(123456u, Time::stampMillis()); +} + +void test_stampMillis_calls_the_zero_tick_one() +{ + Time::setTestMillis(0); + TEST_ASSERT_EQUAL_UINT32(1u, Time::stampMillis()); +} + +// The regression this helper exists for. Applying skipZero() at the STORE while a reader measures +// elapsed time against a raw clock splits the two sides apart on the wrap tick: the stamp is 1 while +// now is still 0, so `now - stamp` is UINT32_MAX and a brand new stamp reads as ~49.7 days old. Any +// elapsed-since guard - a cooldown, a debounce, a long-press threshold - then fires when it must not. +void test_store_side_dodge_alone_makes_a_fresh_stamp_read_as_ancient() +{ + Time::setTestMillis(0); + const uint32_t rawNow = Time::getMillis(); // what an un-normalised reader holds: 0 + const uint32_t stampedAtStore = Time::skipZero(rawNow); // the old shape: dodge only at the store + TEST_ASSERT_EQUAL_UINT32(0u, rawNow); + TEST_ASSERT_EQUAL_UINT32(1u, stampedAtStore); + TEST_ASSERT_EQUAL_UINT32(UINT32_MAX, (uint32_t)(rawNow - stampedAtStore)); // the whole problem +} + +void test_reading_through_stampMillis_keeps_elapsed_at_zero_on_the_wrap_tick() +{ + Time::setTestMillis(0); + const uint32_t now = Time::stampMillis(); // read once, used for the store AND the comparison + const uint32_t stamp = now; // store it as-is; no second dodge needed + TEST_ASSERT_EQUAL_UINT32(1u, now); + TEST_ASSERT_EQUAL_UINT32(0u, (uint32_t)(now - stamp)); // fresh reads as fresh + + // Once the clock moves on, the measured age is short by exactly the 1 ms the dodge introduced: + // the stamp was taken at tick 0 and recorded as 1, so 400 ticks later it reads as 399 old. That + // is the whole cost of the scheme, and it is the same 1 ms skew skipZero() already documents - + // pinned here so nobody "corrects" it to 400 and reintroduces a raw read on one side. + Time::advanceTestMillis(400u); + TEST_ASSERT_EQUAL_UINT32(399u, (uint32_t)(Time::stampMillis() - stamp)); +} + +// A stamp stored one tick BEFORE the wrap, read one tick after, must still measure 1 ms - the dodge +// must not disturb ordinary wrap-crossing arithmetic. +void test_stampMillis_measures_across_the_wrap_boundary() +{ + Time::setTestMillis(0xFFFFFFFFu); + const uint32_t stamp = Time::stampMillis(); + TEST_ASSERT_EQUAL_UINT32(0xFFFFFFFFu, stamp); + Time::advanceTestMillis(1u); // wraps to 0, which stampMillis reports as 1 + TEST_ASSERT_EQUAL_UINT32(2u, (uint32_t)(Time::stampMillis() - stamp)); +} + // --- getMillisMonotonic(): the published wrap carry --- void test_monotonic_matches_millis_before_any_wrap() @@ -373,6 +428,11 @@ void setup() RUN_TEST(test_advanceTestMillis_wraps_like_millis); RUN_TEST(test_skipZero_maps_zero_to_one); RUN_TEST(test_skipZero_leaves_nonzero_values_alone); + RUN_TEST(test_stampMillis_matches_getMillis_away_from_the_wrap); + RUN_TEST(test_stampMillis_calls_the_zero_tick_one); + RUN_TEST(test_store_side_dodge_alone_makes_a_fresh_stamp_read_as_ancient); + RUN_TEST(test_reading_through_stampMillis_keeps_elapsed_at_zero_on_the_wrap_tick); + RUN_TEST(test_stampMillis_measures_across_the_wrap_boundary); RUN_TEST(test_timerEndsAtMillis_is_an_ordinary_sum_away_from_the_wrap); RUN_TEST(test_timerEndsAtMillis_dodges_a_sum_that_wraps_to_zero); RUN_TEST(test_timerEndsAtMillis_dodges_the_wrap_tick_itself);