Files
firmware/test
fdb644e0b7 Fix millis() rollover in deadline, interval, and timestamp handling (#11291)
* Add native test coverage for the UptimeClock monotonic seam

src/UptimeClock.{h,cpp} shipped without a dedicated test suite. Port the six
tests from the monotonic-time branch (test/test_time), retargeted to the
renamed header.

The wrap test crosses 0xFFFFFFFF via advanceTestMillis() rather than a second
setTestMillis(): setTestMillis() sets clockSourceChanged, which makes
getMillis64() rebase its accumulator and swallow the wrap.

* NextHopRouter: fix 49.7-day millis() rollover in retransmission timing

Resolves the "FIXME, handle 51 day rolloever here!!!" in
NextHopRouter::doRetransmissions() by switching the retransmission-due
comparison from plain unsigned <= to a signed-difference cast.

The previous p.nextTxMsec <= now comparison silently breaks across the
~49.7 day millis() wraparound: pending retransmissions either stall
for the remainder of the wrap window, or all fire simultaneously at
the rollover boundary. Long-running router/infrastructure nodes do hit
this in practice.

The replacement (int32_t)(p.nextTxMsec - now) <= 0 is the standard
Arduino/embedded idiom for rollover-safe deadline checks and behaves
identically to the original for any non-wrap timing.

* Address Copilot review: use unsigned half-range for rollover-safe retransmit check

Review feedback from @Copilot on PR #10227: casting a uint32_t
subtraction to int32_t is implementation-defined in C++ when the
unsigned value exceeds INT32_MAX (even though it works on typical
two's-complement targets).

Switch to the fully well-defined unsigned half-range form:
  nextTxMsec is in the past-or-equal iff (now - nextTxMsec) has not
  wrapped past 2^31 ms. Future offsets < 2^31 ms wrap into the top
  half and read as 'not yet'.

Same semantics as the signed-cast version on every two's-complement
platform we care about, but portable to any conforming C++ impl.

* Use monotonic time for airtime windows

* Document monotonic airtime windows

* Fix test_packet_signing sentinel that #10227's rollover fix inverts

test_C3_invalid_repeated_packet_cannot_ack_or_change_retry_state parked a
pending packet at nextTxMsec = UINT32_MAX to mean "never retransmit", then
asserted that a rejected repeated packet leaves the retry state untouched.

NextHopRouter::doRetransmissions() now tests whether a retransmit is due with
an unsigned half-range compare, (uint32_t)(now - nextTxMsec) < 0x80000000u,
so that retransmission timing survives the ~49.7 day millis() wrap. Under it
now - 0xFFFFFFFF == now + 1, a small positive delta, so UINT32_MAX reads as
~1ms in the past: the retransmit fires and rewrites nextTxMsec, and the test
failed with "Expected 4294967295 Was 6247".

Use a representable future time instead. Production is unaffected either way -
nextTxMsec is only ever written as millis() + d, and UINT32_MAX came from the
test harness alone - so the sentinel is what needs to go, not the comparison.
Special-casing UINT32_MAX in the retransmit path would keep a value that reads
as "expired" under any wrap-correct compare.

The value is held in a local because millis() advances across
runPipelineIngress(), so recomputing it at the assertion would compare against
a different number.

Reported upstream on meshtastic/firmware#10227, whose branch predates this test.

* Make Throttle time-injectable and add hasElapsed()

Throttle backs ~94 call sites, which makes it the highest-leverage place in
the tree to put the clock seam: reading Time::getMillis() instead of millis()
in its three call sites turns all of them into time-injectable code at once,
without touching any of them. The 32-bit millis() wrap is not otherwise
reachable from a native test.

The read is behaviour-preserving - Time::getMillis() returns millis() unless a
test injects a clock - and the full native suite passes with it live.

Also add hasElapsed(), the complement of isWithinTimespanMs(), because 51 of
the 94 call sites are spelled !isWithinTimespanMs and read poorly. Its
boundary is inclusive (>=) since isWithinTimespanMs uses <; both are
documented. It deliberately does not treat lastExecutionMs == 0 as "never
run": call sites pair that test with the interval check themselves, and
absorbing a sentinel into the one helper every module depends on is exactly
the value-overloading hazard being removed elsewhere.

Migrating the existing !isWithinTimespanMs sites is cosmetic and deliberately
left out of this commit.

test/test_throttle/ covers window semantics, both boundaries, the complement
identity, execute()'s first-run and throttled paths, and - the point of the
exercise - a window opened before the wrap closing correctly after it,
including at the 24h interval that is the longest in the tree.

* Stop disarmed deadline sentinels reaching the comparison

Two deadline variables encoded "inactive" as a magic value that only reads as
"never" because the comparison against it is a naive millis() compare. Under
any rollover-correct comparison both invert to "expired ~49 days ago", so they
have to be untangled before those comparisons can be fixed.

Power::reboot() set rebootAtMsec = -1 on platforms with no reboot
implementation, intending "never fire". Every reader already treats 0 as the
disarm value - powerCommandsCheck() tests `if (rebootAtMsec && ...)`, and
AdminModule writes 0 to cancel - so -1 was both wrong and unnecessary. Use 0.
Left as UINT32_MAX it would reboot-loop the moment the comparison is corrected.

ExternalNotificationModule's nag window compared against nagCycleCutoff, which
holds UINT32_MAX once stopped and 1 at boot. isNagging is the real armed flag,
so test it first and short-circuit: a disarmed cutoff can no longer reach the
arithmetic, while an idle module still takes the same sleep path that the
boot-time value of 1 was relying on.

Note this fixes the sentinel only. The comparison itself is still a naive
`nagCycleCutoff < millis()` and remains on the list to convert.

* Fix millis() rollover in every deadline and interval comparison

Roughly 20 sites compared against millis() directly - `millis() > deadline`,
`deadline < millis()`, `last + interval < millis()`. All of them break for
about 24 days after the 32-bit millis() wrap: depending on which side of the
wrap each value sits, the action either stalls for weeks or fires immediately
and repeatedly. The longest affected interval is the 12 hour NTP renewal, a
~50x margin against the wrap, so none of these needed the range - only the
correct comparison.

Add Throttle::deadlinePassed(deadlineMs) for sites that store an absolute
deadline they cannot re-express as "interval since an event". It uses the same
unsigned half-range test as NextHopRouter::doRetransmissions() rather than
introducing a competing signed-cast idiom, and unlike the signed cast it is
defined for every input. Sites that do store an event use the existing
isWithinTimespanMs / hasElapsed. Nothing gained new state.

Because both helpers read Time::getMillis(), every converted site is now
reachable from a native test that drives the clock across the wrap; the
comparison itself is covered directly in test/test_throttle/.

Sentinel handling is the reason this could not be a mechanical rewrite. The
disarm convention is not uniform: 0 means "inactive" for rebootAtMsec,
shutdownAtMsec, alertBannerUntil, fixHoldEnds, suppressUntilMs and
touchResumeBlockUntilMs; 0 means "due now" for ntp_renew, which is forced to 0
at link-up; UINT32_MAX means "inactive" for nagCycleCutoff; and
alertBannerUntil == 0 in isOverlayBannerShowing() means "show indefinitely".
Every inactive marker is arithmetically far in the past, so a correct
comparison fires on it - each site tests its sentinel before the arithmetic,
and keeps the meaning it had.

Two sites carried a second bug found on the way:

BME680Sensor tested (stateUpdateCounter * STATE_SAVE_PERIOD) < millis(). With
a 6 hour period and a uint16_t counter that product overflows uint32_t after
about 198 saves, independently of the millis() wrap. It now measures the
interval since the last save.

EInkDynamicDisplay had `if (previousRunMs > millis()) return;` as a millis()
overflow guard, which skipped rate limiting entirely for the whole post-wrap
period - the bug it meant to prevent. Every check below it already goes
through Throttle, so the guard is removed rather than fixed.

MotionSensor's calibration countdown is converted to a signed delta rather
than deadlinePassed, because it needs the remaining magnitude and not a
boolean; that matches the already-correct check in the same file.

* Remove getMillis64() and use Throttle for the NodeInfo reply window

getMillis64() had exactly one caller and no callers in tests. It also carried
obligations that made it the wrong shape for this firmware: a wrap accumulator
in mutable statics, which is not ISR-safe, and which must be polled at least
once every ~49.7 days or it silently misses a wrap and returns a time ~49 days
short.

Its one caller only wanted to know whether a 12 hour suppression window had
elapsed - which Throttle answers correctly across the wrap without any
accumulator. NodeInfoModule now stores Time::getMillis() in lastNodeInfoSeen
and tests the window with Throttle::isWithinTimespanMs, so the map holds
milliseconds rather than seconds derived from a 64-bit read.

USERPREFS_NODEINFO_REPLY_SUPPRESS_SECS is user-overridable and now feeds a
multiply by 1000, so a static_assert rejects any value too large to express in
milliseconds instead of letting it wrap.

clockSourceChanged goes too. It existed solely to rebase getMillis64()'s
accumulator when a test swapped clock sources, and it made the wrap untestable
through the injection API: setTestMillis() set the flag, so a wrap crossed by
two setTestMillis() calls was swallowed. With the accumulator gone the flag has
nothing to rebase, and the injection API is a plain settable clock.

The three getMillis64 tests are dropped as they no longer describe anything.
One test replaces them, pinning that advanceTestMillis() wraps past
0xFFFFFFFF rather than saturating, since the Throttle wrap tests rely on it.

Also fix eviction in pruneLastNodeInfoCache(): it picked the entry with the
smallest stored stamp, which is the wrong victim once some stamps sit on the
far side of the wrap. It now evicts the largest elapsed time.

* Add CI guard and docs rule against naive millis() comparisons

Fixing the existing sites does not stop the next one being added. The
millis-deadline-check job rejects millis() placed directly next to a comparison
operator, in either order, anywhere in src/. It lives in test_native.yml
alongside suite-count-check, which sets the precedent for a repo-hygiene guard
that CI enforces and bin/run-tests.sh does not.

The correct idioms all subtract before comparing, so none of them match the
pattern. Line comments are stripped first, so documentation is free to name the
broken form - as the guard's own comment and the coding conventions both do.

Writing the check before finishing the sweep turned out to be worth it: it
found roughly 14 sites that a by-hand audit of deadline variables had missed,
including two extra nagCycleCutoff compares, both boot-screen timeouts, and a
6 hour sensor save interval that was also overflowing a uint32_t multiply.

.github/millis-deadline-allowlist.txt covers the cases that are genuinely not
deadline tests. Both current entries are uptime thresholds - "has the device
been up N ms" - with no stored deadline and no event to measure from: a 30s
button holdoff against phantom shutdown from floating pins, and a 10s window
for the OEM boot logo. Each re-crosses its threshold once per wrap, which is
harmless for boot-holdoff logic and not worth new state to avoid. Entries are
keyed on file plus exact source text, without line numbers, so an edit above an
entry does not silently invalidate it.

Locally the guard reports 19 matches before the sweep and 2 after, both
allowlisted.

The Throttle bullet in the coding conventions is rewritten from "prefer
Throttle for rate limiting" to "never compare against millis() directly", lists
all four helpers with when to use which, names the CI guard, and documents the
sentinel hazard with the rebootAtMsec = -1 case that would have become a reboot
loop. Mirrored into AGENTS.md; CLAUDE.md gets a pointer row.

* Trim rollover comments to what the code needs

The comments added with the millis() rollover fixes carried too much of the
investigation that produced them: how many sites were found, which document
recorded them, what the old code used to do. That belongs in the commit history,
not in the source, and some of it was already stale - Power::reboot() still
described the check it disarms as "a naive millis() > deadline" when that
comparison had been fixed in the same series.

What stays is the non-obvious part at each site: which sentinel value the
variable overloads and what it means there, since that differs between call
sites and is what a correct comparison gets wrong. 0 means "not scheduled" for
rebootAtMsec, "renew now" for ntp_renew, and "show indefinitely" in
isOverlayBannerShowing().

Exposition is kept where it earns its place: the Throttle helpers, the uptime
clock's note on why there is no 64-bit variant, and the tests. The Throttle
docs lose only the site count and the "longest interval in the firmware"
statistic, both of which would age badly; the range trade-off between the two
forms is what a caller actually needs.

Comments only - no code changed, verified by diff.

* possible fixes

* 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

* Correct the described failure window of a naive millis() compare

The comments and agent docs said a bare `millis() > deadline` "breaks for ~24
days after the wrap". That figure belongs to the fix, not the bug: it is the
half-range limit of deadlinePassed(), which reads deadlines more than 2^31 ms
ahead as already passed, and the range over which a UINT32_MAX sentinel reads
as passed.

The naive compare's actual failure is an inversion lasting only while the
deadline sits on the far side of the wrap, so it is bounded by the interval:
the action fires immediately and loses its wait, or blocks for about the wait
it should have performed - days for the nRF52 flash-corruption backoff,
one skipped cycle for a seconds-long retransmit timer.

Comments and docs only; the ~24.8 day statements that correctly describe
deadlinePassed()'s own range are left as they were.

clod helped out here

* Restore a monotonic uptime clock and consolidate the wrap counters

Time::getMillisMonotonic() is the getMillis64() shape - a 32-bit wrap
counter carried across reads - promoted to the shared timebase, with
Time::getUptimeSecs() as the derived whole-seconds view. This deliberately
reverses the earlier removal of getMillis64(), and the distinction matters:
removal was right for a lazily-read accumulator with one rare caller, where
a 49.7-day gap between reads silently swallowed a wrap. Here every read is
the poll and AirTime::runOnce() guarantees one per second; the missed-wrap
contract is pinned by a test rather than left as a footnote.

Three private wrap counters collapse into it:

- AirTime::syncNow() takes its seconds from Time::getUptimeSecs() and drops
  its lastSyncMsec checkpoint; window rotation is unchanged.
- DeviceTelemetryModule loses refreshUptime()/uptimeWrapCount/uptimeLastMs;
  uptime_seconds comes from Time::getUptimeSecs(), which also removes the
  0.296s-per-wrap truncation of (0xFFFFFFFF / 1000) * wraps. Its two
  interval checks move to Throttle::hasElapsed().
- HostMetricsModule's copies of those members were never read (its uptime
  comes from /proc/uptime) - deleted.

Not ISR-safe (unguarded mutable carry): ISRs keep using getMillis(), which
stays a pure read. Audited: no interrupt-context file reads getTime(),
getValidTime(), or the new accessors.

test/native-suite-count 44 -> 45: the bump for test_uptime_clock was lost
in a branch history rewrite, leaving every later value off by one -
run-tests.sh reports AMBER and CI's suite-count-check fails on the current
push until this correction.

* Anchor the wall clock in monotonic milliseconds

getTime() computed elapsed-since-time-set as a 32-bit millis() delta, so a
node that took time once and stayed up past 49.7 days reported a wall clock
one full cycle in the past - and last_heard, rx_time, message and position
stamps all inherited it. The anchor is now the 64-bit monotonic count
(timeStartMsec -> timeStartMs64) and the elapsed term is computed in 64-bit,
so the wall clock is exact at any uptime.

All six anchor writers follow: the five hardware-RTC read branches and
perhapsSetRTC(), which keeps a truncated 32-bit copy of the same instant for
its Throttle-checked rate-limit stamps. The test seams anchor the same way.

Two native regression tests drive getTime() across the wrap through the
Time seam - one anchored before the wrap and read after it, one anchored
after a counted wrap - with the test epoch derived from BUILD_EPOCH so the
plausibility window cannot rot as the build date advances.

* Stamp the rx_time placeholder in monotonic uptime seconds

computeRxTimeStamp() stamped Time::getMillis() when the clock was untrusted,
and reconcilePendingRxTimes() back-calculated with a 32-bit millis() delta -
correct within one wrap, but a placeholder older than 49.7 days aliased to a
small elapsed value and reconciled to a plausible-but-wrong recent epoch:
the exact failure has_rx_time exists to prevent, reachable by an ordinary
unattended router whose phone connects two months in.

The placeholder is now Time::getUptimeSecs(). Both stamps come off the
monotonic counter, so the elapsed term is exact at any age and the aliasing
window is gone outright rather than widened. If elapsed somehow exceeds the
epoch itself, the packet stays un-dated (absent, never wrong) instead of
clamping to a pre-1970 value. Defence in depth: a placeholder that leaks
needs ~50 years of uptime to cross MIN_PLAUSIBLE_EPOCH, where milliseconds
took 18.3 days.

The stream-API reconciliation tests keep their scenarios with the placeholder
unit switched, and ScopedTimeFixture resets the monotonic carry so uptime
seconds are deterministic per case.

* Date nodes heard before the clock arrives, without polluting last_heard

A node first heard while the wall clock was untrusted got no last_heard at
all, and nothing backfilled it once time arrived - the phone showed "Last
heard: unknown" for a node it had just announced. The arrival instant now
waits in a RAM-only sidecar (NodeNum -> uptime seconds, 32 slots,
reuse-oldest - the RouteHealth shape) and is converted to a real epoch on
the clock-becoming-trusted transition, beside the existing rx_time
reconciliation. last_heard itself never holds anything but a real epoch or
0: it persists to flash and the warm tier, where an uptime-relative value
would be meaningless after reboot.

The sidecar's write sites are updateFrom()'s no-trusted-clock path (the
rx_time placeholder already carries the arrival instant, so this is a store,
not a second clock read) and addFromContact's anti-eviction stamps, which
previously wrote a bare getTime() - boot-relative seconds on a clockless
node, the exact value lastHeardIsWallClock() exists to catch. Eviction
ranking honours the stamps: heard-this-boot outranks every stored epoch,
ordered among themselves, so a stamped contact is not the first victim.

PhoneAPI re-reads last_heard at nodeinfo send time: a record prefetched
before the clock became trusted can carry 0 while the store has since been
backfilled, and re-reading at the pop makes handshake ordering (time-set vs
node-list download) irrelevant. Backfill never moves last_heard backwards
and skips the pathological elapsed-exceeds-epoch case. A node evicted to
the warm tier before time arrives is still absorbed with last_heard 0 -
same as before, bounded to the untrusted window.

* Update the agent docs for the monotonic timebase

The conventions bullet asserted there is deliberately no 64-bit millis; the
monotonic uptime clock restored for timestamps changes that contract. State
the split explicitly: Throttle for deadlines and intervals (no carry state),
Time::getMillisMonotonic()/getUptimeSecs() for timestamps, polled by
construction and not ISR-safe.

* Publish the monotonic wrap carry from a single writer

getMillisMonotonic() was a read-modify-write on two unguarded statics, and it
is reached off the main loop: the nRF52 Bluefruit task via
onFromRadioAuthorize() -> PhoneAPI::getFromRadio -> getValidTime(), and the
portduino civetweb workers via the same path. Two readers interleaving inside
the wrap window could each increment the carry, putting every uptime and
wall-clock reading 2^32 ms ahead for the rest of the boot - a permanent ~49.7
day jump in rx_time, last_heard and ClientNotification.time.

Readers no longer write. serviceMonotonic() publishes a snapshot behind a
seqlock and is the only writer; a reader adds its own unsigned elapsed time to
that snapshot, which is exact across the wrap, so it never inspects the
boundary and cannot miscount it. The main loop publishes every iteration, so
the once-per-49.7-days obligation now has the whole window of margin instead of
resting on an instruction-wide race.

AirTime was the guaranteed poller and is now a pure reader, so the two airtime
wrap tests step the clock the way loop() does. The test clock itself is atomic
so a suite can drive it from one thread while others read.

* Re-arm the GPS ephemeris hold when none is in force

The rollover sweep guarded the hold re-arm with `fixHoldEnds != 0 &&`, which
reads like the sentinel rule but inverts this site. The comparison it replaced,
`(fixHoldEnds + GPS_THREAD_INTERVAL) < millis()`, was always true when nothing
was armed - that was the point, since 0 means "not holding" and so is a reason
to arm. With the guard, a publish that cleared the hold without sleeping (the
`shouldPublish && !tooLong && !holdExpired` path, which does not call down())
left hasValidLocation set and prev_fixQual non-zero, so no disjunct held:
nothing re-armed, nothing published, and the receiver stayed powered at the
200ms poll until searchedTooLong() fired.

State the question positively instead. fixHoldInForce() is the only place the
sentinel is interpreted, and both of runOnce()'s decisions derive from it - the
asymmetry is now visible rather than implied, since arming does not require a
prior hold but expiring does. Its `!= 0` test is not redundant with the
arithmetic: deadlinePassed() is an unsigned half-range test, so past 2^31 ms of
uptime the sentinel reads as a deadline ~24.9 days in the future.

Kept beside its caller rather than in a header; the native test build compiles
GPS.cpp, so the suite declares the prototypes.

Also converts the getACK() wait to isWithinTimespanMs(start, interval): it has
both the start instant and the interval in hand, which gives the full 49.7-day
range instead of 24.8 days ahead, and takes its anchor from Time::getMillis()
so the wait is injectable.

* Date the NodeInfo reply window in uptime seconds

The 12h reply-suppression stamp regressed from wrap-immune 64-bit seconds to
raw 32-bit milliseconds, and pruneLastNodeInfoCache() evicts only by node count
and DB membership - never by age. A stable mesh under the node cap therefore
keeps every stamp indefinitely, and once uptime passes 49.7 days an old one
aliases back into the window: `now - stamp` computes as ~0 and a legitimate
NodeInfo request goes unanswered for up to 12h. It self-heals and repeats once
per wrap cycle.

Store Time::getUptimeSecs() instead, which does not wrap for 136 years, and
drop the millisecond conversion the previous shape needed. Entries past the
window are now evicted too: they can only ever decide "don't suppress".

N8-N11 cover the window from both sides, and N10 pins the regression - it needs
a full 2^32 ms of uptime to elapse, not merely a crossing of the boundary,
because that is when a millisecond stamp reads as "answered this instant".

tearDown() now restores the injected clock and C14's region and TX bucket. A
failing assertion aborts the test body, so restoring at the end of it leaked
that state into every later case.

* Update the agent docs for the single-writer clock and sentinel direction

Two rules the preceding three commits changed.

The monotonic clock is no longer maintained by whoever happens to read it:
serviceMonotonic() is the only writer, readers are pure, and calling it from
anywhere but the main loop reintroduces the double-count.

The sentinel guidance gained the half it was missing. It named UINT32_MAX as a
sentinel while prescribing an idiom that only covers 0, and it assumed the
sentinel always means "suppress" - at the GPS fix-hold site it meant "fire",
which is how that regression passed review looking like the rule.

* Name the fix-hold expiry predicate and arm it from the injected clock

holdJustExpired() gives the second reading of the fixHoldEnds sentinel a
name beside the first, so both are pinned by test/test_gps_fix_hold/ and
neither can be respelled at the call site. The old inline form could not
be tested: written as a literal, its guard folds at compile time and the
assertion asserts nothing.

The arm site used bare millis() while the evaluation reads the Throttle
clock; same value in production, but it kept that write out of reach of
Time::setTestMillis(). Remap a deadline that lands on 0, which would
otherwise read as no hold at all.

* Share the extend formula between the clock's reader and writer

getMillisMonotonic() and serviceMonotonic() carried byte-identical wrap
arithmetic. A one-sided edit to either would drift the published carry
from what readers report, so keep one copy.

* Trim the NodeInfo dedup comment to the house limit

* todo note for potential future imrpovments

* fix some simple deadlines

* Trim the hold-expiry test comment to the house limit

* Fix non-blocking uptime publication and pre-clock recency edges (#29)

* fix(time): avoid blocking monotonic readers

* test(time): make paused-publisher check deterministic

* fix(time): address review portability gaps

* Init the eviction sentinel to the newest possible recency

EvictionRecency{} is {0, false}, which evictionRecencyOlder() ranks as older than
every candidate: without the oldestIndex/oldestBoringIndex guards nothing would
ever be selected and a full node DB would stop evicting entirely.

Init to the genuine maximum instead, so the sentinel is correct on its own. The
index guards stay: two independent reasons the scan is right beats one.

* Keep the deadline-guard check name branch protection matches

The guard was widened to cover Time::getMillis() and unqualified getMillis(),
and renamed to suit. Upstream branch protection matches required checks by name,
so a rename means the old name never reports and merges block on a check that
will never arrive.

Widen the guard, keep the name; the descriptive text carries the broader scope.

* Correct native-suite-count to 47 after the develop merge

Upstream #11293 added test_nmea_wpl and took develop's count to 43; this branch
had independently reached 46. Merging develop resolved the counter textually,
keeping 46, while the directory set became the union of both sides at 47.

The suite-count CI gate fails on the mismatch, and it gates the native test jobs,
so the tests themselves were being skipped.

* test(uptime): make the wrap fall where the comment says it does

The concurrent-reader case started at 0xFFFFF000, leaving 0x1000 to the wrap, so
the 0x800 advance annotated "cross the wrap" fell short and the wrap actually
happened during the following 60s advance.

Start at 0xFFFFF800 instead, so the first advance lands exactly on the wrap while
the readers are running and the second is the ordinary time after it - the shape
both comments already described. Total elapsed is unchanged, so the closing
assertion still holds.

* Respond to human comments

* Did I ever tell you about the time I went to Shelbyville? I wore an onion on my belt, which was the style at the time.

* Convert the I2S nag deadline develop dragged in

The HAS_I2S_SPEAKER_NRF52 RTTTL block arrived from develop with a raw
nagCycleCutoff >= millis(), which the deadline guard rejects. Use the same
Throttle::deadlinePassed() form as the two sibling paths in this function.

* Arm the LittleFS format guard with a flag, not a zero timestamp

preFSBegin() runs in the first millisecond of boot, so millis() can legitimately
return 0 there. Both readers of last_format_ms treated 0 as "nothing formatted
this boot", which would skip the repeat-corruption escalation and let a dead
flash reformat-loop instead of reporting FLASH_CORRUPTION_UNRECOVERABLE.

* Note the single-thread contract on AirTime

* Note the AirTime locking TODO, and tighten the thread note

The two constant getters are not constrained, and getSilentMinutes() reads the
buckets without rotating them, so "the accessors mutate" was not accurate.

* trunk: ignore trufflehog false positives on millis-wrap test constants

test_throttle and test_uptime_clock pin dense clusters of hex boundary
constants (0xFFFFFF00u and neighbors) to exercise 32-bit millis()
rollover. trufflehog's Lob detector stitches nearby hex literals into
one candidate string, and the result happens to match a Lob API key
shape - not a secret, just test fixtures.

Same pattern already used for the gitleaks/nodedb-fixture false
positive in this file.

---------

Co-authored-by: nightjoker7 <mattdeering7@gmail.com>
Co-authored-by: Clive Blackledge <clive@ansible.org>
Co-authored-by: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com>
Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
2026-08-12 16:49:17 -05:00
..
2026-07-16 18:35:33 -05:00
2026-07-16 18:35:33 -05:00
2026-08-06 14:05:07 +00:00
2026-06-09 21:00:05 -05:00
2026-07-16 18:35:33 -05:00
2026-07-01 19:01:27 -05:00
2026-08-06 14:05:07 +00:00
2026-08-06 14:05:07 +00:00
2026-08-06 14:05:07 +00:00

Native Unit Tests - Authoring Guide

This directory contains C++ unit tests that run on the host machine via PlatformIO's native environment. Tests use the Unity framework.

Running Tests

Preferred: use bin/run-tests.sh - it defaults to the coverage env, cross-checks the number of suites that actually ran, and emits an unambiguous RED/AMBER/GREEN verdict:

./bin/run-tests.sh                          # all suites
./bin/run-tests.sh -f test_traffic_management  # single suite
./bin/run-tests.sh -f test_traffic_management > /tmp/test_out.txt 2>&1; tail -5 /tmp/test_out.txt

Exit codes: 0 = GREEN, 1 = RED, 2 = AMBER, 3 = FILTERED.

The harness is Linux-only, by choice. bin/run-tests.sh and the per-suite isolation it drives need bash 4+ and GNU coreutils/find (find -printf, md5sum), and the script refuses to start anywhere else rather than degrade quietly - a shared-state check that silently mis-hashes a sandbox still prints a verdict, and that verdict would be worthless. The native-macos PlatformIO env is a build target for meshtasticd, not a test host; the isolation wrapper is registered for env:native and env:coverage only. On macOS or Windows, run the suite in a container: ./bin/test-native-docker.sh.

-f is not a gate. A filtered run can pass while a full run fails, because filtering removes the suites that create the state a later suite trips over. Iterate with -f; gate on a full run.

Sanitizers are per env. coverage (the default) has ASan/LSan; native has none, verified. -e native runs are not sanitized.

A signal name in the output is not a crash. exit(UNITY_END()) returns the failure count and PlatformIO renders it as a signal number (4 -> SIGILL, 5 -> SIGTRAP), reporting the suite [ERRORED]. Match it against the failure count before assuming a fault.

Suite order is randomisable, and reproducible. --shuffle runs the suites in a seeded random order; --seed <n> replays an exact one. The seed defaults to the commit SHA - one order per commit, so a red is replayable and attributable rather than flaky - and is printed at the start of the run and on the RESULT: line. On failure the full order is printed, because for an order-dependent failure the order is the diagnostic. A single green seed is not evidence of order independence; vary it.

./bin/run-tests.sh --shuffle              # seed from HEAD, printed
./bin/run-tests.sh --seed 2855893161      # replay that exact order

Randomisation costs one pio invocation per suite (about 4.7s each), because PlatformIO orders suites by its own directory walk and -f only selects.

Copilot interface note: When running tests via the Copilot chat interface, edits made through the chat may not be reflected in the on-disk files that the test binary reads. If tests pass in chat but fail locally (or vice versa), verify the files on disk match what you expect before trusting the result. Always confirm with a local terminal run.

Raw pio test (no sanitizers, no verdict logic) - use when you need to override the env or inspect verbose Unity output:

# All test suites
pio test -e native

# Single suite
pio test -e native -f test_your_module

# Verbose (shows build errors in detail)
pio test -e native -f test_your_module -vvv

Never pipe through | tail -N to shorten output. PlatformIO prints build errors at the top of output and test results at the bottom; tail will show stale cached results from a prior successful build while hiding the compile error that caused the current run to fail.

Preferred pattern for raw pio - redirect to file, then grep:

# Redirect all output to a file; grep for errors and results after it exits
pio test -e native -f test_your_module > /tmp/test_out.txt 2>&1
echo "exit: $?"
grep -E 'error:|PASS|FAIL|succeeded|failed' /tmp/test_out.txt
tail -15 /tmp/test_out.txt

Why: piping through | grep line-buffers the output and suppresses all progress until the process exits, making it look hung. The redirect approach lets the build stream normally while still giving you filtered results afterwards.

Viewing verbose test output without truncation (e.g. TEST_MESSAGE group headers):

/tmp/meshtastic-pio-venv/bin/python -m platformio test -e coverage --filter test_mesh_beacon -vv 2>&1 | grep -v "[[:space:]]SKIPPED$"

The -vv flag makes Unity emit INFO: lines from TEST_MESSAGE calls; piping through grep -v SKIPPED removes the noise from platform feature gates while keeping all PASS/FAIL/INFO lines visible.

externally-managed-environment error on Ubuntu/Debian:

If pio test fails immediately with error: externally-managed-environment, the system pio binary is using the OS Python which newer distros lock down. Use PlatformIO's own venv instead:

~/.platformio/penv/bin/python -m platformio test -e native -f test_your_module > /tmp/test_out.txt 2>&1
grep -E 'error:|PASS|FAIL|succeeded|failed' /tmp/test_out.txt
tail -15 /tmp/test_out.txt

Helper Scripts (Useful Shortcuts)

These wrappers are handy when local host dependencies are missing or when you want repeatable commands.

# Run native tests in Docker (recommended on macOS / non-Linux hosts)
./bin/test-native-docker.sh

# Pass normal PlatformIO test args through to Dockerized test run
./bin/test-native-docker.sh -f test_your_module

# Force Docker image rebuild (after dependency changes)
./bin/test-native-docker.sh --rebuild

# Run simulator integration check (build native first)
pio run -e native && ./bin/test-simulator.sh

# Build and run meshtasticd natively
./bin/native-run.sh

# Build and run under gdbserver on localhost:2345
./bin/native-gdbserver.sh

# Build native release artifact into ./release/
./bin/build-native.sh native

Notes:

  • The repository script name is ./bin/test-simulator.sh (there is no test-native-simulator.sh).
  • ./bin/test-native-docker.sh is the closest match to CI behavior for native tests and avoids host package setup.

System Dependencies (Ubuntu/Debian)

The native build requires several system libraries. Install them all at once:

sudo apt-get install -y \
  libbluetooth-dev libgpiod-dev libyaml-cpp-dev libjsoncpp-dev openssl libssl-dev \
  libulfius-dev liborcania-dev libusb-1.0-0-dev libi2c-dev libuv1-dev

See .github/actions/setup-native/action.yml for the canonical list.

Creating a New Test Suite

1. Directory Structure

test/test_your_module/test_main.cpp

One file per suite. No per-test platformio.ini is needed - tests build under the [env:native] environment defined in the root platformio.ini.

2. File Skeleton

#include "MeshTypes.h"      // Include BEFORE TestUtil.h (provides NodeNum, etc.)
#include "TestUtil.h"        // initializeTestEnvironment(), testDelay()
#include <unity.h>

#if YOUR_FEATURE_GUARD       // Same #if guard as the module under test

#include "FSCommon.h"
#include "gps/RTC.h"
#include "mesh/NodeDB.h"
#include "modules/YourModule.h"
#include <cstdio>    // required for printf() - used for blank-line group separators
#include <cstring>
#include <memory>

// --- Test output helpers ---
// printf() writes directly to stdout and appears in -vv output as a plain line (no prefix).
// Use it for blank-line group separators: printf("\n");
// TEST_MESSAGE() emits a "file:line:INFO: <text>" line - visible at -vv and above.
// Use TEST_MSG_FMT for formatted diagnostic lines inside tests.
#define MSG_BUF_LEN 200
#define TEST_MSG_FMT(fmt, ...) do { \
    char _buf[MSG_BUF_LEN]; \
    snprintf(_buf, sizeof(_buf), fmt, __VA_ARGS__); \
    TEST_MESSAGE(_buf); \
} while(0)

// --- Tests ---

void test_example()
{
    TEST_MESSAGE("=== Example test ===");
    TEST_ASSERT_TRUE(true);
}

// --- Unity lifecycle ---

void setUp(void) { /* runs before every test */ }
void tearDown(void) { /* runs after every test */ }

void setup()
{
    initializeTestEnvironment();   // MUST call - sets up RTC, OSThread, console
    UNITY_BEGIN();

    printf("\n=== Example group ===\n");           // header line to help find tests

    RUN_TEST(test_example);
    exit(UNITY_END());             // REQUIRED - a bare UNITY_END() leaves the process running
}

void loop() {}

#else // !YOUR_FEATURE_GUARD

void setUp(void) {}
void tearDown(void) {}

void setup()
{
    initializeTestEnvironment();
    UNITY_BEGIN();
    exit(UNITY_END());
}

void loop() {}

#endif

3. Terminate with exit(UNITY_END()), on every branch

A bare UNITY_END() does not end the suite - it ends the reporting. setup() returns, the runtime goes on calling loop(), and the process runs forever. PlatformIO does not notice: it reads the Unity summary off stdout, reports the suite PASSED and moves to the next one, so the run is green while the binary is still resident. Nothing surfaces it, and the leak is one process per suite per run.

The consequences are worse than an idle process:

  • The per-suite sandbox is deleted underneath a live process, so its CLEAN/DIRTY verdict says what the suite had written by the time the harness stopped looking, not what it left behind.
  • .gcda coverage data and LeakSanitizer's report are both flushed by atexit handlers, so a suite that never exits contributes no coverage and gets no leak check - silently.
  • Each survivor pins its own deleted binary on disk (~94 MB), which du cannot see.

So: exit(UNITY_END()) in every setup() branch, including the #else of a feature or architecture guard where the suite does nothing. The empty-suite branch is the easiest one to get wrong, because it looks like there is nothing to clean up.

4. Feature Guard

Wrap the entire test body in the same #if guard the module uses (e.g. #if HAS_VARIABLE_HOPS, #if !MESHTASTIC_EXCLUDE_GPS). When the feature is disabled, the #else branch produces an empty passing suite.

Common Patterns

MockNodeDB

Most module tests need to inject nodes with controlled hop distances and ages:

class MockNodeDB : public NodeDB
{
  public:
    void clearTestNodes()
    {
        testNodes.clear();
        numMeshNodes = 0;
    }

    void addTestNode(NodeNum num, uint8_t hopsAway, bool hasHops,
                     uint32_t ageSecs, bool viaMqtt = false)
    {
        meshtastic_NodeInfoLite node = meshtastic_NodeInfoLite_init_zero;
        node.num = num;
        node.has_hops_away = hasHops;
        node.hops_away = hopsAway;
        nodeInfoLiteSetBit(&node, NODEINFO_BITFIELD_VIA_MQTT_MASK, viaMqtt);
        node.last_heard = getTime() - ageSecs;
        testNodes.push_back(node);
        meshNodes = &testNodes;
        numMeshNodes = testNodes.size();
    }

    std::vector<meshtastic_NodeInfoLite> testNodes;
};

static MockNodeDB *mockNodeDB = nullptr;

Set nodeDB = mockNodeDB; in setUp().

Test Shim (Exposing Protected/Private Members)

Subclass the module under test to make protected methods callable and private members writable:

class YourModuleTestShim : public YourModule
{
  public:
    // Pull protected methods into public scope via using.
    // IMPORTANT: using requires the method to be protected (or public) in the base -
    // friend alone does NOT satisfy this. See pitfall #6.
    using YourModule::runOnce;
    using YourModule::someProtectedMethod;

    // Wrap private members with setter methods (friend grants direct access here).
    void setPrivateField(int x) { privateField = x; }
};

For methods you want to expose via using, use the conditional access-specifier pattern in the header - not plain friend:

// In YourModule.h, inside the class body:
#ifdef PIO_UNIT_TESTING
  protected:
#else
  private:
#endif
    bool someMethod();

For private member variables that a shim setter needs to touch directly, friend is sufficient (no using involved):

// In YourModule.h, inside the class body:
#ifdef PIO_UNIT_TESTING
    friend class YourModuleTestShim;
#endif

Global Singleton Lifecycle

Most modules use a global pointer (extern YourModule *yourModule;). Manage it carefully:

void setUp(void) {
    // ... setup ...
}

void tearDown(void) {
    yourModule = nullptr;   // prevent dangling pointer between tests
}

void test_something() {
    auto shim = std::unique_ptr<YourModuleTestShim>(new YourModuleTestShim());
    yourModule = shim.get();
    // ... test ...
    yourModule = nullptr;
}

Pitfalls and How to Avoid Them

1. Persisted Filesystem State

You are handed a clean sandbox. Declare what you write.

Each suite runs inside its own scratch $HOME (bin/pio-test-isolate.sh), so state cannot reach the next suite. The files in play are wider than module state, and all but the last live under ~/.portduino/default/prefs/:

File Written by
nodes.proto any NodeDB save - including incidental ones from removeNodeByNum(), resetNodes(), nodeDBSelfCare(), and the constructor itself when the file is absent
config.proto, module.proto, channels.proto, device.proto config/channel saves, admin handlers
warm.dat WarmNodeStore::saveIfDirty(), on the node-DB save cadence
transmit_history.dat retransmission tracking
/prefs/<module>.bin per-module saveState()

NodeDB's constructor calls loadFromDisk(), so any suite that constructs one inherits whatever is there.

What you have to do:

  • Nothing, if your suite is self-contained. That is the default and what almost every suite wants.

  • If your suite mutates persisted state on purpose, add a line to test/state-manifest.tsv with a reason:

    test_nodedb_blocked	state=per-suite writes=nodes.proto,warm.dat	saturates the DB to test the protected-node cap
    

    An undeclared write is reported as DIRTY and grades the run AMBER. A declared write that never happens is reported as MISSING - a warning, and a useful one: it catches persistence that silently stopped working.

  • Use state=per-suite only if a test genuinely needs to observe the previous test's write (persistence round-trips, migration ladders). It relaxes per-test checking to the suite boundary, so make it a deliberate choice rather than an accident of setUp().

Deleting your own state in setUp() is still fine and still a good habit for intra-suite isolation - it is just no longer what stands between you and the next suite:

void setUp(void) {
    // ...
#ifdef FSCom
    FSCom.remove("/prefs/your_module.bin");
#endif
}

2. A Shared Fixture Is Not a Fixture

If your suite touches globals the code under test writes - nodeDB, config, owner, devicestate, channelFile - build and restore them in setUp/tearDown for every test, not just the ones that seem to need it. An opt-in fixture that only some tests arm leaves the rest sharing one never-reset object, and "the other tests set their own state and are unaffected" is a claim that quietly stops being true as tests are added.

test/test_admin_radio/test_main.cpp is the worked example:

void setUp(void) {
    // ...
    replaceAdminRadioGlobals();   // saves the globals, installs a fresh NodeDB
}
void tearDown(void) {
    restoreAdminRadioGlobals();   // restores them, deletes the NodeDB, re-runs initRegion()
    // ...
}

A fresh NodeDB per test costs real time (loadFromDisk() plus, when the region is set, key generation) - in that suite roughly 7% of a ~7½-minute run. Pay it. If a test genuinely needs to observe the previous test's state, that is what state=per-suite in test/state-manifest.tsv is for; say so there rather than achieving it by omission.

3. File-Scope Mutable Globals Persist Across Tests

Variables like static uint8_t someDenominator = 8; in the module .cpp file retain mutations from previous tests. This is distinct from member variables - it affects all instances.

Fix: Add a static void resetGlobal() method to the module and call it in setUp().

4. Randomness Breaks Determinism

If the module uses rand() for jitter or similar, test results become non-reproducible.

Fix: Add a static enable/disable flag:

// Module header:
static void setJitter(bool enabled) { s_jitterEnabled = enabled; }

// Test setUp:
YourModule::setJitter(false);

// Test tearDown:
YourModule::setJitter(true);

5. Time-Dependent Logic Produces Zeros

Rolling averages weighted by elapsedMs / ONE_HOUR_MS collapse to zero when tests complete in microseconds. Sample windows, EMA alphas, and interval-based accumulators all suffer from this.

Fix: Expose the timestamp via friend access and simulate realistic elapsed time:

// In test shim:
void setWindowStartMs(uint32_t ms) { windowStartMs = ms; }

// In test:
shim.setWindowStartMs(millis() - 3600000UL);  // pretend 1 hour elapsed

6. Capacity Limits Cause Cascading Failures

Fixed-size data structures (hash sets, ring buffers) overflow when tests inject more data than fits. This triggers early flushes with near-zero time fractions, compounding the time-dependent-zeros problem.

Fix: Simulate multiple realistic time windows rather than one massive burst. Let adaptive mechanisms (if any) self-tune over several rolls.

7. Granting test access to private/protected members

PlatformIO defines PIO_UNIT_TESTING during pio test builds. Several production headers (TransmitHistory.h, CryptoEngine.h, MQTT.h, RTC.h) use this to gate test-only visibility changes. PlatformIO also defines UNIT_TEST in the same builds for backward compatibility, but that spelling is deprecated - always use PIO_UNIT_TESTING in new code. The established pattern for exposing a private method to a test shim without widening production visibility:

#ifdef PIO_UNIT_TESTING
  protected:
#else
  private:
#endif
    bool myMethod();

Critical C++ rule: a using declaration in a derived class (e.g. using Base::myMethod) requires myMethod to be protected or public in the base - friend alone does not satisfy this. Adding friend class TestShim while leaving the method private will still fail to compile. Use the conditional access-specifier pattern above, not friend.

setUp/tearDown Checklist

  • Create and clear MockNodeDB (if needed)
  • Zero global configs: config, moduleConfig, myNodeInfo
  • Set nodeDB = mockNodeDB
  • Delete your own persisted state files (FSCom.remove(...)) for intra-suite isolation - cross-suite isolation is already guaranteed, see Pitfall 1
  • Declare deliberate writes to shared state in test/state-manifest.tsv, with a reason
  • Reset file-scope mutable globals
  • Reset mock clock to a safe base value (e.g. mockTime = ONE_HOUR_MS) - prevents unsigned subtraction underflow in time-dependent logic
  • Disable randomness/jitter flags
  • In tearDown: null the global singleton pointer, restore flags

Test Organization

A well-structured test suite follows this pattern:

  1. Topology/scenario builders - static helper functions that set up specific test conditions
  2. Injection helpers - simulate realistic traffic, time, or event patterns
  3. Scenario tests - each builds a scenario, runs the module, asserts on outcomes
  4. Lifecycle tests - state persistence, startup from blank, restart recovery
  5. Summary test (optional) - emits a scenario table into the log for quick CI review

Not a Unity suite: bin/test-config-check.sh

Portduino YAML validation is tested by driving a built meshtasticd rather than by a Unity suite, because what it asserts - the exit status and printed report of meshtasticd --check, and the fact that a normal run still refuses a bad config - are properties of the process, not of a linkable function. Fixtures live in test/fixtures/portduino-config/ (see the README there); CI runs it in test_native.yml. It is not a test_* directory, so it sits outside the suite count the harness derives from test/.

pio run -e native && ./bin/test-config-check.sh

Existing Test Suites

This table is a description, not an inventory. The canonical suite total is the number of test_* directories under test/, detected on the fly by bin/run-tests.sh on every full run and cross-checked against the suites that actually ran. That derived count is the only number that should be trusted or quoted. Entries below carry per-suite descriptions the count cannot; do not infer completeness from the row count.

Suite Module Under Test
test_admin_radio Admin + LoRa region config
test_fscommon_getfiles Bounded file-manifest walk
test_atak ATAK integration
test_crypto CryptoEngine
test_default Default configuration helpers
test_hop_scaling Hop scaling algorithm
test_http_content_handler HTTP handling
test_mac_from_string MAC address parsing
test_mesh_module Module framework
test_meshpacket_serializer Packet serialization
test_mqtt MQTT integration
test_packet_history Packet history tracking
test_position_precision Position precision helpers
test_radio Radio interface
test_serial Serial communication
test_module_config AdminModule module config
test_tak_config TAK (ATAK) team/role values
test_traffic_management Traffic management
test_transmit_history Retransmission tracking
test_type_conversions NodeDB v25 type conversions
test_utf8 UTF-8 utilities