* 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 <noreply@anthropic.com>
* time: guard the four 0-means-unset stamps this branch had missed
Sweeping src/ for the `if (stamp && <deadline check>)` 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 <noreply@anthropic.com>
* 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: <reason>` 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 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
* 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 = <variable>`
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 <noreply@anthropic.com>
* 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 <class T> 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 <class T>` 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 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
* style(time): trim comments to the house limit
---------
Co-authored-by: Tom <116762865+Nestpebble@users.noreply.github.com>
Co-authored-by: nomdetom <nomdetom@protonmail.com>
Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Never add --without-building to a test run. PlatformIO links every native test program to the single $BUILD_DIR/$PROGNAME path and attributes Unity output by text alone, so a run that only builds beforehand executes whichever suite was linked last under every suite's name - all reporting PASSED. Build once with --without-testing to warm the shared src objects if you like; the run itself must still build. bin/check-test-attribution.py grades the JUnit reports for exactly this and is wired into both bin/run-tests.sh (RED) and CI.
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 notest-native-simulator.sh). ./bin/test-native-docker.shis 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.
.gcdacoverage data and LeakSanitizer's report are both flushed byatexithandlers, 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
ducannot 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.tsvwith a reason:test_nodedb_blocked state=per-suite writes=nodes.proto,warm.dat saturates the DB to test the protected-node capAn 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-suiteonly 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 ofsetUp().
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:
- Topology/scenario builders - static helper functions that set up specific test conditions
- Injection helpers - simulate realistic traffic, time, or event patterns
- Scenario tests - each builds a scenario, runs the module, asserts on outcomes
- Lifecycle tests - state persistence, startup from blank, restart recovery
- 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 |