mirror of
https://github.com/meshtastic/firmware.git
synced 2026-09-16 00:10:11 -04:00
macos-27
7257
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matias DendaandJonathan Bennett
|
32eb1a1237 |
Honor an explicit -c config path when -s is given (#11348)
The simradio flag (-s) is the first branch of an if/else-if chain that also handles config loading, so it short-circuits every later branch -- including the one for an explicit -c <path>. Skipping config discovery under -s is intended, but a config path the user passed by hand is not discovery, and it is silently ignored today. Move the -s check after the -c branch so an explicit path is always parsed, and skip only the implicit discovery (./config.yaml, /etc/meshtasticd/config.yaml) when -s is given without -c. The radio override then runs after every config source, since -c and its ConfigDirectory entries can both set Lora.Module and -s has to win over them. Doing it there also fixes --check and --output-yaml, which reported the configured module rather than the simulated one because the old override sat behind an early return. Behaviour with a bare -s is unchanged: no YAML is loaded and the radio is the simulator. Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> |
||
|
|
ee15508494 |
time: arm the remaining 0-means-unset stamps through the helpers (#11830)
* time: add skipZero/safeMillis/timerEndsAtMillis helpers
skipZero() steps a millis value past 0, since stored stamps and deadlines
conventionally use 0 for "unset" and the one tick per ~49.7-day wrap that
lands on 0 would otherwise read as never-set.
safeMillis() covers a bare stamp; timerEndsAtMillis(delayMs) covers a
deadline, where the sum is what has to dodge 0 - a non-zero read plus a
delay lands there once per wrap - so it is not safeMillis() + delayMs.
* time: replace hand-rolled zero-dodging with the UptimeClock helpers
PacketHistory rxTimeMsec, EncryptedStorage s_lastFailMillis (stamps), and
SGM41562 lastRefreshMs_ / NextHopRouter learnedAtMsec (ternary stamps) each
hand-rolled skipZero() in place; swap in safeMillis()/skipZero() directly.
HapticFeedback pulseOffAt/delayedPulseAt and GPS fixHoldEnds hand-rolled the
deadline form - millis() + delay, then remap a 0 result to 1 - swap in
timerEndsAtMillis(delay).
No behavior change; each site keeps the value it already computed.
* time: guard the remaining 0-means-unset deadline/stamp writes
rebootAtMsec, shutdownAtMsec, and NotificationRenderer::alertBannerUntil are
all read back with a bare == 0 / != 0 check for 'not scheduled', but every
write site computed millis() + delay (or a bare millis() stamp) with no
guard against landing exactly on 0 - the same wrap hazard skipZero() exists
for, just never applied here.
Route every rebootAtMsec/shutdownAtMsec/alertBannerUntil write through
timerEndsAtMillis()/safeMillis(); RadioLibInterface's reboot-on-stuck-tx
sums an already-captured stamp rather than "now", so it goes through
skipZero() directly instead.
No behavior change outside the ~1-in-2^32 wrap window each site was
already exposed to.
* time: guard three more 0-means-unset deadline writes
ntp_renew (ethClient.cpp), suppressTouchTapUntilMs (Events.cpp), and tx_after
(RadioLibInterface.cpp) all read back 0 as a real state - forced NTP renewal,
no suppress window active, no TX delay armed, respectively - but each arm
site wrote a bare millis()/getMillis() + delay with no guard against the sum
landing exactly on 0.
Route each through Time::timerEndsAtMillis(). No behavior change outside the
wrap window each site was already exposed to.
Refresh the Throttle.h TODO list to note ntp_renew is converted too.
* motion: guard the calibration deadline and use Throttle::deadlinePassed
endCalibrationAt's arm site wrote millis() + calibrateFor with no guard
against landing on 0, the same value finishCalibrationIfExpired()/
drawFrameCalibration() treat as "not calibrating". Route it through
Time::timerEndsAtMillis().
Also swap finishCalibrationIfExpired()'s hand-rolled (int32_t)(now - deadline)
< 0 for Throttle::deadlinePassed(): same wrap-safe comparison the codebase
already provides, without the signed-cast pattern Throttle.h documents as
implementation-defined past INT32_MAX, and it drops the file's last direct
millis() call in favor of the Time:: wrapper the rest of it already uses.
* time: fix Throttle::execute()'s own zero-dodging
Both places execute() writes *lastExecutionMs - the first-ever-run branch
and the regular update - used bare Time::getMillis() with no guard against
landing on 0, which is the exact sentinel this function reads back as
"never run" one line above. A hit there makes the next call re-fire
immediately instead of respecting minumumIntervalMs.
Capture now via Time::safeMillis() once; every use downstream (the elapsed
comparison, the stored value) is then safe by construction instead of
needing the guard reapplied at each write.
* revert some safeMillis cases where overflow is a bad thing
* test(uptime): pin skipZero/safeMillis/timerEndsAtMillis at the wrap boundary
Covers the zero case, an ordinary nonzero value, and a sum that lands
exactly on 0 from a nonzero start - the case timerEndsAtMillis() exists
for, and the one the prior suite had no direct coverage of.
* time: restore the route-health write normalization and put it on one clock
noteRouteLearned()/noteRouteSuccess() lost their `now ? now : 1` normalization,
leaving learnedAtMsec able to store 0 - which getOrAllocRouteHealth() reads as an
ever-growing age, making the slot the first eviction candidate and permanently
stale. Normalize at the write, where the block comment already says it happens,
so every caller is covered rather than just today's two.
Both callers, the two isRouteStale() sites and doRetransmissions() now read
Time::getMillis(), so the stamp and every comparison against it share a clock.
doRetransmissions() goes back to getMillis(): its `now` feeds only comparisons,
never a 0-sentinel field, so skipping zero there only cost accuracy.
* time: read the haptic, InkHUD and calibration deadlines on the write's clock
These three deadlines were converted to Time::timerEndsAtMillis() on the write
side while their reads stayed on millis(), so each spanned two clocks and would
fire immediately or never under an injected test clock. Convert the reads to
match: HapticFeedback::scheduleNext()/runOnce(), the InkHUD tap-suppression
window, and the calibration countdown's read-back of screen->getEndCalibration().
MotionSensor's sampledAtMs is left alone - its write and read are both millis()
and consistent already.
* time: correct the sentinel notes to match what the code actually does
The Throttle.h enumeration claimed the remaining timerEndsAtMillis() callers
"already dodge the sentinel", which reads as a completeness claim the same branch
contradicts: RadioLibInterface's tx_after and activeReceiveStart are both 0=unarmed
and both still arm from bare millis(). Name them instead, so the deadline-type
conversion has the real list. The ntp_renew entry now separates a deliberate 0
("due now", forced at link-up) from a computed one, which is what changed there.
The three TODO(elapsed-stamp) blocks ran four and five lines against the repo's
one-or-two rule, and two of them argued their case wrongly. Throttle.cpp implied
safeMillis() simply doesn't help; in fact neither store is safe on the wrap tick -
the 1 underflows a same-instant read, the 0 re-takes the never-run branch - which
is the symmetry worth recording. PacketHistory.cpp called its dodge "reflecting
the previous pattern" when it is load-bearing: rxTimeMsec 0 means "empty slot"
(PacketHistory.h:21) and insert() drops a record stamped 0 outright, so without it
a packet arriving on the wrap tick is never stored and loses its dedup.
Also picks up trunk fmt's trailing-whitespace fix in Throttle.cpp and the comment
realignment in SGM41562.cpp that this branch's added comment knocked out.
* test(nexthop): pin the route-health stamp against the 0 sentinel
The uptime suite covers skipZero/safeMillis/timerEndsAtMillis themselves, but
nothing covered a call site, so the branch deleted noteRouteLearned()'s
normalization and stayed green. None of the existing route-health tests pass 0 as
`now` - they use 1000, learnAt, or millis() - (TTL + 5000) - which is exactly the
gap the regression went through.
Both new tests fail with "Expected 0 to be not equal to 0" when the skipZero() is
backed out of NextHopRouter, and pass with it. noteRouteSuccess() only refreshes
an existing record, so its twin learns a route first to reach the write.
Also drops a self-referential assertion in the uptime suite: comparing
getMillis() against safeMillis() passes even if safeMillis() does no dodge at
all, so it now asserts the literal.
* discard safemillis for skipzero (better semantics and therefore maintainability) and make consistent use of getmillis where it is called (to permit testing)
* more wrapzero safety
* STM gets some too
* time: stop the next 0-means-unset deadline being armed from raw millis()
The fields this branch armed through Time::timerEndsAtMillis() / Time::skipZero()
are the kind that get added by copy-paste: `rebootAtMsec = millis() + N` appears
at twenty-odd sites across six files, and the next module to defer a reboot will
be written from one of them. Nothing catches the mistake afterwards - the sum
lands on 0 for one tick per ~49.7-day wrap, so a test run, a soak and a bench
session all pass while a pending reboot, shutdown, DFU jump or banner expiry is
silently dropped.
Two guards, at the two places it can go wrong.
The helpers themselves: skipZero() is constexpr, so its contract is now pinned by
static_assert in the header rather than only by test_uptime_clock. The asserts are
chosen against the two plausible rewrites - `ms | 1` perturbs every even value and
`ms + 1` turns the last tick of the wrap into the 0 the function exists to avoid.
Both compile, and both pass a test that only checks skipZero(0); each trips a
distinct assert here, naming the failure mode.
The call sites: bin/lint-unset-sentinel-millis.sh flags a sentinel field in src/
assigned from a raw millis()/getMillis() read, and names the helper to use. It is
name-driven because the 0 contract is declared in src/main.h and enforced in six
other files, so no single-file scan can infer it; every one of the thirteen fields
was checked to actually test against 0 before being listed. nagCycleCutoff and
LinuxJoystick's nextRepeatX/nextRepeatY are deliberately absent - their unset state
is a separate bool - and the nine remaining `millis() + x` sites in src/ are locals
that never store 0 for anything to misread.
Blocking, unlike its note-level neighbours: there is no run-time enforcer to pair
with, and the tree has zero violations today, so gating costs nothing. Scoped to
src/ so test_uptime_clock can keep building raw wrap values on purpose.
bin/test-lint-unset-sentinel-millis.sh pins the scanner against 23 fixtures -
reads, disarms, shadowing locals, comments, string literals and the already-fixed
forms all have to stay quiet.
Co-Authored-By: Claude Opus 5 <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>
|
||
|
|
31f05ab057 |
fix(touch): stop LONG_PRESS repeating when the suppression deadline wraps (#11829)
* fix(touch): stop LONG_PRESS repeating when the suppression deadline wraps TouchScreenBase::_start was one field doing two incompatible jobs. It held the press-down timestamp, and then the LONG_PRESS handler overwrote it with `millis() + 30000` to stop the event repeating for the rest of the hold. Every read was a hand-rolled signed subtraction on time_t, and suppression worked only because `time_t(millis()) - _start` came out around -30000. Where time_t is 64 bits - the portduino host - that uint32_t sum wraps to a small number while millis() is still just under 0xFFFFFFFF. The subtraction then goes hugely positive instead of negative, the threshold test passes on every 20ms poll, and each pass re-arms to another wrapped value. It keeps firing until millis() itself wraps, up to ~30 s later: about 1500 TOUCH_ACTION_LONG_PRESS events injected into InputBroker for one finger that never moved. Modelling the old expression across press-start offsets puts the worst case at exactly 1500 for a 60 s hold, where three is correct. On a 32-bit time_t build the signed wrap happens to keep suppressing, so this is host-and-variant dependent rather than universal. The zero-dodging helpers in src/UptimeClock.h are no use here: they map 0 to 1, and 1 reads as "long ago" exactly as 0 does. The defect is the overload, not the zero, so the field is split by what it is actually asked: _pressStartMs a past event time - how long has the finger been down _longPressSuppressed is repeat suppression armed _longPressSuppressUntilMs when it expires, read only while the bool is set Two fields for the suppression rather than one, for the reason Throttle.h's TODO(deadline-type) gives: armed has to stay a separate question from passed. No single value can stand in for "unarmed" here either, since deadlinePassed() reads 0 as long past below ~24.8 days of uptime and as far future above it. Nothing new uses 0 as a sentinel, so bin/lint-unset-sentinel-millis.sh needs no entry. All three comparisons now go through Throttle - hasElapsed() for the two elapsed-since-press questions, which also buys the full ~49.7 day range that a stored event time gets, and deadlinePassed() for the suppression window. Behaviour is preserved deliberately, including the part that is easy to miss: the old `+ 30000` made a held finger re-report LONG_PRESS once every 30 s, not once per touch. A bool latch would have been simpler and quietly narrowed that, so the window is kept as LONG_PRESS_REPEAT_SUPPRESS_MS. Old and new were compared across five wrap scenarios and agree everywhere except the wrap window the old code got wrong. The tap-on-release suppression the old write also provided is not needed: a hold long enough to reach here has duration >= TIME_LONG_PRESS, so the tap branch already takes its else and clears _tapped. One guard added while here. The RAK14014 deferred-tap window is TIME_LONG_PRESS - 50 and that subtraction is unsigned now, so a variant lowering TIME_LONG_PRESS below 50 would underflow it into a ~49.7 day wait and the deferred TAP would never fire. The only override in the tree is t5s3_epaper at 500; a static_assert fails the build instead of the touch panel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(touch): trim comments to the house limit --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: nomdetom <nomdetom@protonmail.com> |
||
|
|
bef289ef42 |
fix(extnotif): make isNagging the only armed flag for the nag cycle (#11828)
* fix(extnotif): make isNagging the only armed flag for the nag cycle
ExternalNotificationModule kept the nag cycle's armed state in two places that
could disagree: the isNagging bool, and nagCycleCutoff reserving UINT32_MAX for
"not armed". handleInputEvent() read only the second one:
if (nagCycleCutoff != UINT32_MAX) { stopNow(); return 1; }
The field is declared `= 1`, while isNagging starts false, so at boot that test
said "armed" when nothing was nagging. The first input event of every boot was
therefore answered with stopNow() and a non-zero return - and a non-zero return
ends the observer chain (Observable::notifyObservers in src/Observer.h returns on
the first one), so that event was swallowed from every later observer. The handler
is registered whenever external_notification.enabled, and InputBroker only
short-circuits while nagging() is true, so the event does reach it.
The same read had a second failure mode once per ~49.7-day wrap: armNagCycle()
computes `millis() + durationMs`, which can land exactly on UINT32_MAX. When it
does, a real nag is running with isNagging true, but this read says "not armed" and
the module's own handler never stops it. Time::skipZero() cannot help here - it
lifts 0 to 1 and leaves UINT32_MAX alone, which src/UptimeClock.h static_asserts.
So the fix is not a zero guard, it is removing the second opinion. isNagging is
the armed flag - which is what the comment above the expiry check already claimed,
and what the other four reads already use - and nagCycleCutoff is now only ever a
deadline, read after isNagging has been checked. Nothing reserves a value, which
matters because an arm site spelled `millis() + interval` can produce any value
there is, so no value is safe to reserve. That is the shape the TODO(deadline-type)
note in src/mesh/Throttle.h is aiming at, and that note is updated to match rather
than keep describing the sentinel this removes.
Worth knowing for review, though not changed here: InputBroker::handleInputEvent
already calls stopNow() itself when nagging() is true, and returns without
notifying observers. Every path that starts a notification calls armNagCycle()
first, so isNagging is true for the whole life of any real nag. That makes this
handler reachable only when there is nothing to stop - its stopNow() was never
doing useful work. Gated rather than deleted, because removing a public handler
and its observer registration is a bigger call than fixing the defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(extnotif): trim comments to the house limit
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: nomdetom <nomdetom@protonmail.com>
|
||
|
github-actions[bot]andjp-bennett
|
644a43ca9b |
Update protobufs (#11841)
Co-authored-by: jp-bennett <5630967+jp-bennett@users.noreply.github.com> |
||
|
Matias DendaandThomas Göttgens
|
d05fbec64c |
Add AEAD (AES-CCM) authenticated encryption for PSK channels (#9749)
* Add AEAD (AES-CCM) authenticated encryption for PSK channels Extend PSK channel encryption with optional AES-CCM authenticated encryption (use_aead flag in ChannelSettings). When enabled, messages include a 12-byte authentication tag that prevents forgery, bit-flipping, and injection attacks by anyone with the channel PSK. Changes: - Add encryptPacketCCM/decryptPacketCCM to CryptoEngine with key promotion (16-byte keys zero-padded to 32 for AESSmall256 compat) - Move AES-CCM primitives (aes-ccm.h/cpp, aesSetKey, aesEncrypt) outside PKI guard so they're available unconditionally - Add isAEADEnabled() to Channels with hash differentiation (XOR 0xAE) - Add AEAD encrypt/decrypt branches in Router perhapsEncode/perhapsDecode with no CTR fallback on AEAD channels - Add use_aead field to channel.pb.h (bool, tag 8) - Add MESHTASTIC_AEAD_OVERHEAD constant to RadioInterface.h - Add comprehensive test suite: round-trip (AES-128/256), tamper detection (ciphertext, tag, sweep), wrong PSK, wrong sender, packet-too-small, deterministic output verification Addresses firmware#4030. * Apply clang-format to match project style * Guard AEAD path against empty PSK and check encrypt return value - Add early return in encryptPacketCCM/decryptPacketCCM when psk.length == 0, preventing null dereference in aesSetKey - Check encryptPacketCCM return value in Router::perhapsEncode (both PKI and non-PKI paths), returning BAD_REQUEST on failure instead of silently transmitting corrupt packets - Add unit test for empty PSK (encrypt and decrypt must return false without crashing) * Use true AES-128 for 16-byte PSKs instead of promoting to AES-256 aesSetKey now dispatches based on key length: 16 bytes creates AESSmall128, 32 bytes creates AESSmall256. The aes member type changes from AESSmall256 to BlockCipher (polymorphic base class). This removes the unnecessary key promotion that added two extra AES rounds (14 vs 12) with no security benefit since the entropy stays at 128 bits for 16-byte keys. encryptPacketCCM/decryptPacketCCM now pass psk.length directly to aes_ccm_ae/aes_ccm_ad instead of promoting to 32. New tests: ECB AES-128 with NIST vectors, AEAD test verifying AES-128 and AES-256 produce different ciphertexts with same key material and cross-key decryption fails. * Reject the invalid-key sentinel in the AEAD paths CryptoKey documents length == -1 as "invalid key - do not use", but the AEAD guards only tested for 0. Since length is int8_t and the aes_ccm_* key length parameter is size_t, a -1 would widen into a huge unsigned length and be handed to the cipher instead of being rejected. Both callers in Router.cpp are gated on a non-negative channel hash, and generateHash() already returns -1 exactly when getKey() yields an invalid key, so the sentinel cannot reach these functions today. Guard against it anyway rather than relying on callers to keep that invariant. * Tie MESHTASTIC_AEAD_OVERHEAD to CryptoEngine::AEAD_TAG_SIZE The packet-size boundary checks in perhapsEncode/perhapsDecode budget for MESHTASTIC_AEAD_OVERHEAD, but the tag actually written is AEAD_TAG_SIZE. Nothing tied the two together, so changing one would have silently produced oversized packets or truncated payloads. Assert they match instead of coupling RadioInterface.h to CryptoEngine. Also trims the sentinel comment to the two-line limit in AGENTS.md. * Add RFC 3610 known-answer vectors and widen the tamper sweep Packet Vectors #1, #2 and #7 pin aes_ccm_ae()/aes_ccm_ad() to published data rather than to their own output, covering M=8 and M=10, a trailing partial block in every case, and rejection of a modified AAD. Test 1 in test_AES_CCM_AEAD is relabelled as the smoke test it actually is. The per-byte tamper loop now walks the whole buffer including the tag, instead of only the first four ciphertext bytes. * Cover the second nonce input and tighten the AEAD test buffers Test 10 only ever varied fromNode, leaving packetId — the other half of the nonce — unexercised. It now checks each one wrong on its own, both wrong, and both right, so the negative assertions cannot pass vacuously. The undersized-packet test wrote into a one-byte buffer and only survived because decryptPacketCCM() returns before touching it; size it for the whole input so a regressed length guard fails an assertion instead of the stack. Also assert makePsk() cannot overrun CryptoKey::bytes. * Rewrite Unicode dashes to ASCII in AEAD comments The ascii-dash formatter that landed in develop rewrites U+2014/U+2013 to an ASCII hyphen. Three files on this branch still carried em dashes in comments, so Trunk Check went red once develop was merged in. Comments only, no code change. * Authenticate sender and destination IDs as AEAD associated data The nonce binds the sender and the packet id, but nothing bound the destination, so `to` could be rewritten in flight and the tag would still validate. Pass `from || to` as associated data to aes_ccm_ae/aes_ccm_ad so a redirected packet fails authentication. The hop fields stay out of the AAD on purpose: relays legitimately rewrite hop_limit, hop_start, relay_node and next_hop. Adds a sub-test covering redirection to another node and promotion of a unicast to a broadcast; both must be rejected, and the unmodified destination must still round-trip. This changes the on-the-wire format for AEAD packets. Nothing ships with use_aead yet, so there is no deployed traffic to stay compatible with. * fix(crypto): repair EXCLUDE_PKI builds and guard AEAD channel config aes-ccm.cpp is compiled in every build now and calls CryptoEngine::aesSetKey and CryptoEngine::aesEncrypt, whose definitions were still inside the !(MESHTASTIC_EXCLUDE_PKI) block in CryptoEngine.cpp, so MESHTASTIC_EXCLUDE_PKI=1 failed at the link step. Move both definitions outside the guard, and move the pending-public-key declarations back inside it next to the fields they read. fixupChannel() clears use_aead on a channel that resolves to no key material. That combination kept a valid-looking channel hash while every encode returned BAD_REQUEST and every decode dropped, with nothing in the config to show why. encryptPacketCCM/decryptPacketCCM are virtual, so a platform engine can back them with hardware CCM the way it already overrides encryptAESCtr. perhapsEncode() carries one copy of the AEAD/CTR branch instead of an identical copy in each arm of the MESHTASTIC_EXCLUDE_PKI ifdef. Tests: three use_aead cases in test_channel_keys covering the hash split, the no-key clear, and a secondary that borrows the primary's key. * fix(crypto): move CryptoEngine::hash out of the PKI guard hash() is plain SHA256, and PortduinoGlue calls it unguarded to derive a MAC address from the CH341 serial, so MESHTASTIC_EXCLUDE_PKI=1 failed to compile. With this and the previous commit that build links clean. * fix(channels): resolve primaryIndex before hashing in onConfigChanged A keyless secondary resolves its key through primaryIndex, so fixing up channels in the same pass that finds the primary hashed the early slots against the previous one and cleared their use_aead against a key they do in fact inherit. Split the pass, and re-run the fixups in the no-primary restore path, which moves the primary after the fact. Also splits the thirteen AES-CCM AEAD scenarios into separate test functions so a Unity failure names the one that broke. * chore(crypto): trim the AEAD maintainer commits Shortens three comments that outgrew the one-to-two line house rule, drops a truncated sentence and the braces around a single return in perhapsEncode(), and removes a channel test that the moved-primary regression test already covers. No behaviour change. --------- Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> |
||
|
|
80cfa52665 |
Add zero guards on time calculations where they were missing (#11692)
* 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>
---------
Co-authored-by: Tom <116762865+Nestpebble@users.noreply.github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Ben Meadors
|
df47f95ff3 |
fix(nodedb): build the fixed-GPS userprefs path again (#11825)
* fix(nodedb): build the fixed-GPS userprefs path again
The USERPREFS_FIXED_GPS block in the NodeDB constructor carries two defects
that only surface on vendor builds setting USERPREFS_FIXED_GPS_LAT and
USERPREFS_FIXED_GPS_LON. userPrefs.jsonc ships those keys commented out and no
CI target defines them, so the block is never compiled here and neither defect
was caught.
info has not existed in this scope since
|
||
|
Thomas Göttgens
|
2a01676227 |
fix(nodedb): track whether each node was heard on the current LoRa config (#11811)
* fix(nodedb): track whether each node was heard on the current LoRa config Set NODEINFO_BITFIELD_HEARD_ON_CURRENT_LORA on a genuine RF hear and clear it for every node when the LoRa slot config moves, so clients can tell which nodes went unreachable after a preset, region, slot or primary-channel-name change. Hooked into MeshService::reloadConfig(), the single funnel for the device menu, admin/CLI and scanned-URL paths, plus NodeDB::restorePreferences(), which reboots without passing through it. Fixes #11745 * fix(nodedb): store the slot each node was heard on instead of sweeping a bit A client scanning for traffic rolls through presets with live set_config writes, so every hop reached reloadConfig and the sweep cleared the marks on the way out and again on the way home. Each node now carries a 12-bit fingerprint of the slot it was heard on in spare bitfield bits, and heard_on_current_lora is derived by comparing that against the slot the radio is committed to. Config changes no longer touch the node database at all. * fix(nodedb): keep comments inside the two-line limit, rename a test Trunk read test_fingerprint_channelNumIsASlotChange as a Lob API key, since it is test_ followed by exactly 35 alphanumerics, so the tail is now shorter. The comments added under src/ are back within the one-or-two-line limit in AGENTS.md. * fix(nodedb): drop legacy bitfield bits above 10 during v24 migration v24 assigned bits 0..10, so a legacy record carrying anything higher would arrive claiming an RF hear with a stray slot fingerprint, and a never-heard node would read as reachable whenever that stray value matched ours. The migration now masks those bits off, and a new case in test_nodedb_legacy_migration pins it. |
||
|
Thomas GöttgensandClaude Opus 5
|
8a9e10d120 |
fix(power): stop a battery-less board deep-sleeping itself forever (#11821)
* fix(power): stop a battery-less board deep-sleeping itself forever The low-battery counter only reset inside its `hasBattery && !hasUSB` guard, so a board with no battery - whose floating divider drifts in and out of the battery-present window - ratcheted the count up across the gaps until it tripped `sds_secs`, which defaults to a ~24.8-day deep sleep. The button could not rescue it either, because `doDeepSleep()` force-holds `BUTTON_PIN` and a held pad ignores `ext1_wakeup_prepare()`'s re-route to RTC; `rtc_gpio_isolate()`'s pin list has the same effect on boards whose button is GPIO 2 or 34. Separately the cutoff now scales by `NUM_CELLS`, without which no multi-cell pack can ever read low enough to shut down at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(power): satisfy trunk check Apply the `ascii-dash` autoformat that `trunk fmt` wants on the comments this PR's file already carries, and rename the no-battery test so its `test_` prefix plus exactly 35 characters stops matching trufflehog's Lob API-key shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Thomas Göttgens
|
d4482a28a1 |
Fail the build when Telemetry no longer fits the packet payload (#11810)
* Fail the build when Telemetry no longer fits the packet payload * Trim the comments to the house limit |
||
|
Thomas Göttgens
|
34190aac07 |
fix(nodedb): drop satellite entries that no hot node owns (#11808)
* fix(nodedb): drop satellite entries that no hot node owns * test(nodedb): assert every persisted satellite key is owned |
||
|
Thomas Göttgens
|
546b678d50 |
fix(motion): drive screen wake from the accelerometer interrupt (#11758)
* fix(motion): drive screen wake from the accelerometer interrupt The BHI260AP ISR body was empty and BHI_IRQ was never set or read, so the attach only consumed a GPIO slot. ICM20948 could not reach its interrupt path at all: the ICM_20948_INT_PIN fallback in the header is guarded on ICM_20948_WOM_THRESHOLD, which the block above it always defines, so the pin was never defined and the config, attach and interrupt-driven runOnce() were dropped by the preprocessor on every board. BHI260AP now configures the FIFO interrupt, attaches an ISR that sets a flag, and enables the wrist tilt gesture so runOnce() can call wakeScreen(). BMA423 arms INT1 push-pull active-high, which the BMA4 reset default leaves disabled, and drains on the interrupt instead of every 50 ms. Both keep a slow keepalive drain so a pin that never asserts degrades to polling rather than losing tilt and tap wake. MOTION_WAKE_INT_PIN resolves whichever motion interrupt a variant declares. doLightSleep() arms it as a GPIO wake source and lsIdle() attributes the resulting wake to motion, which it previously charged to BUTTON_PIN and dropped. Both are gated on config.display.wake_on_tap_or_motion, matching MotionSensor::wakeScreen(). Closes #11755 * fix(motion): use the ICM20948 interrupt without dropping the compass The ICM_20948_INT_PIN build of runOnce() was a full replacement for the polled one and kept only wake-on-motion, so defining the pin would have dropped the magnetometer fusion that feeds screen->setHeading(), the calibration flow and the IMU sleep handling. providesHeading() returns true for this part, so that is the compass. Merge the two: the pin now selects the wake-on-motion mechanism only. The status register poll stays compiled in behind a keepalive, since no shipped firmware has exercised this line, so a pin that never asserts costs latency rather than wake-on-motion. Declare the pin on t-echo-card. Sensor_INT is P1.13, open drain with a 10K pullup to VDD3V3, matching the driver's active-low config and FALLING attach. The schematic's SCL P1.02 / SDA P1.04 match PIN_WIRE_SCL and PIN_WIRE_SDA. * Revert the t-echo-card ICM20948 interrupt pin Sensor_INT is not the IMU. In both T-Echo-Lite_V1.0 and T-Echo-Lite-Card_V1.0 it appears only on the unannotated 5-pin expansion header (P?, 5PIN_PA1.0) carrying SDA_P1.04, SCL_P1.02, VDD3V3, GND and Sensor_INT with its 10K pullup, and it leaves the sheet as an off-sheet port. Neither schematic contains an ICM20948 symbol at all, and the vendor pin map declares only ICM20948_SDA, ICM20948_SCL and ICM20948_ADDRESS for the part. The interrupt belongs to whatever plugs into that header, so the onboard IMU has no reason to drive it. The driver keeps polling. * Poll until an ICM20948 interrupt pin proves itself A variant that declares ICM_20948_INT_PIN is asserting routing no vendor firmware has ever exercised, so treat the line as unproven: keep polling the wake-on-motion status register at full rate, and only back off to the keepalive once the pin has actually fired. A wrong pin then behaves exactly as before rather than trading wake latency for the guess. * feat(t-impulse-plus): drive ICM20948 wake-on-motion from its INT pin The LilyGO pinmap documents the IMU's INT on P0.07, and variant.cpp already maps and names it as D27, but the pin was never handed to the driver, so wake-on-motion polled the status register every 50 ms. Use the D number: pinMode() and attachInterrupt() index g_ADigitalPinMap, where a raw 7 selects P1.13, the LoRa RF_VC1 TXEN line. The driver polls until the pin proves itself, so an ICM20948 that turns out not to drive it keeps working as before. * Derive MOTION_WAKE_INT_PIN after the build exclusions MESHTASTIC_MINIMIZE_BUILD defines MESHTASTIC_EXCLUDE_I2C further down the file, so the guard read as unset and a minimized build defined the pin anyway. doLightSleep() would then arm a GPIO no motion driver configures, since every driver is compiled out with I2C. Latent rather than live: nothing sets MESHTASTIC_MINIMIZE_BUILD today, and the variants that pass -DMESHTASTIC_EXCLUDE_I2C were already correct because a build flag is defined before this file is parsed. * fix(motion): keep the BMA423 INT1 config failure non-fatal Restores the resolution made when feature/sensorlib-0.4.1 was merged into this branch. That merge is gone after the rebase, and neither parent carried this: the interrupt path is an optimisation over the existing poll, so a pin-config failure should log and fall back rather than be ignored outright. |
||
|
Manuel
|
27afe1159b | fix vbus detection (#11801) | ||
|
github-actions[bot]andcaveman99
|
2dbc33e4d1 |
Update protobufs (#11809)
Co-authored-by: caveman99 <25002+caveman99@users.noreply.github.com> |
||
|
|
103463e26d |
fix(esp32): identify LilyGo T5 S3 ePaper Pro targets (#11368)
* fix(esp32): identify LilyGo T5 S3 ePaper Pro targets * fix(esp32): mark T5 S3 ePaper Pro targets actively supported --------- Co-authored-by: George <509474+giannoug@users.noreply.github.com> Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> Co-authored-by: rcarteraz <robert.l.carter2@gmail.com> |
||
|
Tom
|
777c79f6d8 |
revert a conflict regression and utilise the full power of the lr2021 lna (#10633)
* revert a conflict regression and introduce the DCDC workaround from semtech example code. * fix: Adjust DCDC workaround placement for * clod fixes stuff * clod fixes some more things |
||
|
github-actions[bot]andcaveman99
|
a495ef007c |
Update protobufs (#11806)
Co-authored-by: caveman99 <25002+caveman99@users.noreply.github.com> |
||
|
github-actions[bot]andcaveman99
|
a25ff005f0 |
Update protobufs (#11804)
Co-authored-by: caveman99 <25002+caveman99@users.noreply.github.com> |
||
|
Jonathan Bennett
|
81b3ce8fc2 | Alternate button handling for Muzi Base without screen (#11800) | ||
|
Benjamin FaershteinandManuel
|
2c595e935d |
feat(native): add macOS MUI simulator target (#11739)
Co-authored-by: Manuel <71137295+mverch67@users.noreply.github.com> |
||
|
SeanandBen Meadors
|
0c4bee7a7b |
fix(sx126x): let CalibrateImage settle before re-applying RX registers in resetAGC() (#11774)
* fix(sx126x): let CalibrateImage settle before re-applying RX registers in resetAGC() CalibrateImage returns as soon as the command is accepted and BUSY does not stay asserted for the rest of the calibration. resetAGC() then re-applies the RX boosted-gain and 0x8B5 registers immediately, and a register write landing in that window fails write-verify (RADIOLIB_ERR_SPI_WRITE_FAILED), leaving the chip needing a full re-init. On RAK3401 + RAK13302 (nRF52840, busy mesh) this hit ~69% of resets with no delay, ~3% at 10-20 ms, and 0 at 50 ms. * fix(sx126x): number the CalibrateImage settle as step 6 and re-wrap the comment Review feedback: the settle is the wait for step 5's image calibration, just as step 4 waits for step 3, so number it and wrap to the width of the other steps. Resume receiving becomes step 7. No functional change. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Thomas Göttgens
|
fa81b47ecb |
refactor(sensorlib): unify on 0.4.1 and move the PCF RTCs to PCF8xRTC (#11754)
* chore(deps): unify SensorLib on 0.4.1 and port the 0.4.x API changes The 19 SensorLib declarations were split across 0.3.1, 0.3.4 and 0.4.1. Pin all of them to 0.4.1 and fix the renovate datasource on ThinkNode-M9 (custom -> custom.pio). API changes in 0.4.x: - BMA423Sensor: the configAccelerometer/enableFeature/readIrqStatus surface is gone. SensorBMA423 now derives from SensorBMA4XX and dispatches tilt and tap through callbacks driven by update(). - BHI260APSensor: SensorRemap is a scoped enum in sensor/SensorDefs.hpp, and BoschSensorInfo members are protected, so read them through the accessors. - ExtensionIOXL9555 is renamed to IoExpanderXL9555 and the touch drivers moved under touch/. Point the includes at the current paths instead of the compatibility shims, which emit warnings on every build. Drop four lewisxhe/PCF8563_Library declarations. Nothing in the tree includes pcf8563.h; all RTC code goes through SensorLib. Drop the BMA423_INT block. No variant defines BMA423_INT (t-watch-s3 defines BMA4XX_INT), so it has never been compiled. Interrupt-driven wake on BMA423 is unimplemented rather than regressed by this change. Drop the T_WATCH_S3 branch in BHI260APSensor. That file requires HAS_BHI260AP, which T_WATCH_S3 does not define. SensorQMC6309.hpp does not exist in 0.3.4, so src/motion/QMC6309Sensor.cpp compiles for the first time on 0.4.1. * fix(motion): fail BMA423 init when the sensor rejects its configuration configAccelerometer, enableTiltDetector and enableTapDetector return false only on an I2C or driver-level failure, so treat them the way QMC6309Sensor treats configMagnetometer rather than initializing a sensor that never took its settings. Trim the t-watch-ultra placement comment to the two lines the coding guidelines allow. * refactor(rtc): drive the PCF clocks from PCF8xRTC instead of SensorLib SensorLib reaches its PCF8563 and PCF85063 drivers through a comm layer spanning Arduino, ESP-IDF, SPI and custom callbacks, which is a lot of code to link for four calls on an I2C RTC. Measured against develop, the boards that pull SensorLib grew about 10 KB moving from 0.3.4 to 0.4.1, while the nRF52 boards that do not pull it moved by 100-300 bytes. meshtastic/PCF8xRTC covers both parts in one class over Adafruit BusIO, which every board with a PCF part already links. Ten boards used SensorLib for nothing but the RTC and now drop it entirely; the remaining seven keep it for a BMA423, BHI260AP, QMI8658, XL9555 or touch controller and take the new driver for their RTC. Behaviour changes with it. Both parts latch an oscillator-stop flag on power loss, which the old path ignored: readFromRTC() now refuses a calendar the chip has marked invalid rather than feeding a plausible wrong date to BUILD_EPOCH, the result of begin() is checked, and a failed set is logged. The isBitSet workaround moves from configuration.h to MMC5983MASensor.h. It worked only because configuration.h pulled SensorLib.h in first, so the later include was a no-op and the macro stayed undefined; with the global include gone it has to sit where SensorLib and the SparkFun header actually meet. * fix(rtc): report a missing PCF chip separately from a stopped oscillator lostPower() reads a register, so it also returns true when the chip cannot be reached at all. Folding it into one warning meant a failed begin() reported "oscillator stopped", which is a different fault. * fix(t5s3): read GT911 touches through getTouchPoints 0.4.x dropped the default argument from getPoint(x, y, count) and marked it deprecated, so the two-argument call no longer resolves: variant.cpp:618:27: error: no matching function for call to 'TouchDrvGT911::getPoint(int16_t*, int16_t*)' Use getTouchPoints(), which is what the deprecation points at, rather than passing the count to a call that is on its way out. |
||
|
|
9f51963b42 |
fix(xmodem): return the phone-facing packet by const reference (#11781)
cppcheck reports returnByReference on XModemAdapter::getForPhone(): `meshtastic_XModem` carries a 128-byte payload buffer plus header fields, so returning it by value copied the whole struct on every call. Return `const meshtastic_XModem &` instead, and mark the method const - it is a pure read of xmodemStore, with resetForPhone() being what drains it. Every caller either copies into a value or reads a single field, so no call site changes. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com> |
||
|
Benjamin Faershtein
|
29a65aa13d | fix(mesh): use valid default packet history size (#11786) | ||
|
SeanandTom
|
42d32fcea6 |
fix(radio): make limitPower() idempotent so chip re-inits don't compound PA gain subtraction (#11782)
limitPower() converts the member `power` in place (regulatory clamp, then the TX_GAIN_LORA/FEM subtraction) and relied on applyModemConfig() having just re-seeded it. Since #10025 every driver calls it from both reinitChip() and programModemParams(), and the recovery paths added in #11676/#11678 run the two back-to-back, so each recovery re-converts an already-converted value. On a RAK13302 (22-entry gain table) one recovery walks a 30 dBm request 30 -> 22 -> 13 dBm and a second one down toward the -9 dBm floor, while config.lora.tx_power still reads 30. Seed `power` from config.lora.tx_power at the top of limitPower(); applyModemConfig() always writes the resolved value back there, so a single call is unchanged. Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com> |
||
|
|
382980637b |
perf(nodedb): bind encode-loop entries by const reference (#11780)
cppcheck (iterateByValue) flagged five range-for loops in the NodeDatabase pb_callbacks that copy a whole protobuf entry off the vector only to pass its address to pb_encode_submessage(), which takes a const void *. Bind by const reference instead. Drops one meshtastic_NodePositionEntry, NodeTelemetryEntry, NodeStatusEntry, NodeEnvironmentEntry and NodeInfoLite_Legacy copy per node per encode pass; these run on every nodes.proto save. The nodes_tag loop in NodeDB.cpp is intentionally left as a by-value copy: it mutates item.snr_q4/item.snr to the on-disk quantized form before encoding, so it is a working copy rather than a redundant one. cppcheck does not flag it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Thomas Göttgens
|
73c4110528 |
fix(phoneapi): resend my_info when the node num moves mid-session (#11732)
* fix(phoneapi): resend my_info when the node num moves mid-session The first region set mints the PKI key and moves my_node_num to crc32(public_key) live. my_info only went out during the want_config_id handshake, so an already-connected client kept addressing the old number and its admin packets NAKed PKI_SEND_FAIL_PUBLIC_KEY until it reconnected. PhoneAPI tracks the number it last reported and re-sends my_info from STATE_SEND_PACKETS when it no longer matches. createNewIdentity() nudges fromNum so clients poll. Fixes #11718 * fix(phoneapi): key the MyInfo re-announce off a one-shot state Review follow-up. The per-connection reportedNodeNum field is gone: adding per-instance members to PhoneAPI is documented as breaking USB-CDC enumeration on the nRF52 Adafruit framework, and the baseline was never set for SPECIAL_NONCE_ONLY_NODES, which skips STATE_SEND_MY_INFO and so emitted an unexpected my_info after config_complete_id. MeshService::identityMoved is set with the nudge and cleared once the notify pass has reached every observer, so PhoneAPI::onNotify arms STATE_RESEND_MY_INFO on each connected client in that single pass and stores nothing per connection. The test now drives NodeDB::createNewIdentity() and MeshService::loop() instead of writing my_node_num directly, and asserts the transport wake-up. Nodes-only sync asserts no trailing my_info. drainToIdle() honours its read cap. * fix(phoneapi): restart the dump when the node num moves mid-sync Review follow-up. A client still in its config dump has already been sent the old my_info and has no steady state for the one-shot to fall back from, so the notify pass cleared identityMoved without covering it and the client finished syncing on the obsolete number. PhoneAPI::onNotify now restarts such a client's dump, which is the existing re-handshake path. Skipped for a client that has not reached my_info yet and for SPECIAL_NONCE_ONLY_NODES, which never sends one. test_node_num_change_mid_dump_restarts_sync renumbers mid-dump and asserts the restart, the new number, and that no part of the config is lost. Verified to fail without the fix. * fix(phoneapi): make the identity-move signal survive a concurrent notify pass Review follow-up. The identity move can run off the loop task: a local admin set_config reaches AdminModule through Router::sendLocal() on whichever task delivered it. A bool cleared by MeshService::loop() could therefore be set and cleared without any client being armed, losing the re-announce. A generation counter replaces the bool. loop() snapshots it with fromNum before notifying and only advances the seen counter afterwards, so anything bumped during the pass is still pending. The same snapshot fixes a notify for a fromNum bump that arrived mid-pass being marked delivered. test_node_num_change_mid_dump_restarts_sync now asserts the whole restarted dump: header order, channels, both config sections, our node record, nonce. Also trims the MyInfo redaction comment to the two-line cap. * fix(nodedb): keep self at index 0 after a live renumber, restart nodes-only syncs Review follow-up. createNewIdentity() removed our old row and appended the new one, leaving index 0 pointing at some other node. PhoneAPI's own-nodeinfo read and the demote/evict scans that skip index 0 to protect us both rely on that slot being self, so a renumbered node handed every client a stranger's record as its own. Pinned the way nodeDBSelfCare() does it. onNotify no longer exempts SPECIAL_NONCE_ONLY_NODES from the mid-sync restart. That dump carries no my_info, but it does carry the self record, which the move invalidates the same way. Such a client also gets the re-announce once its sync lands in STATE_SEND_PACKETS, which it previously never did. The generation counters are atomic. Every interleaving was already safe, since observers read the live counter and the seen counter only advances to a pre-pass snapshot, but the concurrent plain accesses were a data race on paper. * fix(meshservice): make fromNum atomic Review follow-up. The counter is bumped from whichever task queued the packet and read by loop(). It is private to MeshService, so the type change covers every access. |
||
|
Jason P
|
125c4514b0 | Allow Spacebar to advance frames (#11771) | ||
|
Jason P
|
95f96439c6 |
Hide Navigation Bar when shutting down EInk (#11775)
* Hide Navigation Bar on EInk Shutdown
* Revert "Hide Navigation Bar on EInk Shutdown"
This reverts commit
|
||
|
Ben Meadors
|
784014e8a7 |
fix(ci): unbreak the ESP32 static analysis gate after the cppcheck 2.20 jump (#11777)
Every PR targeting develop has been red since 2026-09-07 on the seven ESP32 check jobs, while the nRF52, RP2040 and STM32 jobs pass on identical source. gh-action-firmware#61 moved the ESP32 container images onto the pioarduino core. Its esp32 platform ships its own tool-cppcheck 2.20.1 and reinstalls it over anything the repo pins, so ESP32 now analyses with cppcheck 2.20 while every other platform still resolves platformio/tool-cppcheck 1.21100.230717, i.e. 2.11. 2.20 parses far more of this tree than 2.11 ever managed, so checks that were always enabled fired for the first time: 388 defects, ESP32 only. #11776 cleared the two unknownMacro errors. Of the 386 left, two are worth acting on and are fixed rather than suppressed: * SerialModule dereferenced a null Position in NMEA/CALTOPO mode. `decoded` stays NULL when pb_decode_from_bytes() fails, but printWPL() was called with *decoded regardless, so a malformed position payload on our portnum crashed the node. Emit the waypoint only on a successful decode. * InkHUD's 12-hour clock passed a signed 12 to a %u conversion. The rest are style and performance suggestions - functionStatic and the const-correctness family account for 351 of them. Suppress those check ids so the gate means the same thing on every platform again, scoping the one-off ones to their file so a new occurrence elsewhere still fails. Burning them down is worth doing deliberately, not under a CI outage. Verified in the CI container images: all seven previously failing ESP32 environments pass, and rak4631, tracker-t1000-e and t-echo-plus still pass under cppcheck 2.11. |
||
|
AustinandClaude Opus 5
|
9ac0c2c75d |
fix(checks): silence cppcheck functionStatic on the no-screen Screen stub (#11778)
pioarduino's cppcheck 2.20 reports functionStatic for all 20 methods of the no-op graphics::Screen defined under !HAS_SCREEN: none of them touch a member, so it offers to make them static. The advice is wrong here - the stub exists only to mirror the real Screen's instance API so call sites like screen->setFrames(...) compile on screenless boards, so the methods have to stay non-static member functions. The header is included by 85 translation units, so this fired 1700 times on the two screenless esp32 boards in the check matrix (heltec-ht62-esp32c3-sx1262 and tlora-c6) - the entire src/graphics low-severity count for those boards. bin/check-all.sh passes --fail-on-defect=low, so it was failing those jobs. Wrap the stub in an inline cppcheck-suppress-begin/end block, matching the inline-suppression style already used elsewhere in the tree. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Austin
|
1251d31df6 |
fix(observer): suppress cppcheck warning for removeObserver method (#11779)
Accepting cppcheck's suggestion results in a no-compile, we cannot use a const here. Suppress the warning instead. |
||
|
github-actions[bot]andcaveman99
|
f36d7f11ec |
Update protobufs (#11762)
Co-authored-by: caveman99 <25002+caveman99@users.noreply.github.com> |
||
|
Thomas Göttgens
|
bd19fa8e48 |
feat(t-connect-pro): add LilyGo T-Connect-Pro variant (#11746)
* feat(t-connect-pro): add LilyGo T-Connect-Pro variant ESP32-S3R8, 16MB flash, 8MB octal PSRAM. SX1262 LoRa, 480x222 ST7796 LCD with CST226SE touch, W5500 ethernet and a 10A relay on EXT_NOTIFY_OUT. LoRa, display and ethernet share one SPI bus (SCK 12 / MISO 13 / MOSI 11), so every peripheral stays on SPI2_HOST. board_level is extra and HW_VENDOR falls through to PRIVATE_HW until a HardwareModel enum value is allocated. * fix(w5500): serialize shared-bus SPI access with spiLock Arduino's ETHClass reaches SPI through SPIClass, whose mutex is invisible to LovyanGFX. On a board where both share a bus the MAC reads glitched frame headers, and the resulting ESP_LOGE flood blocks the W5500 RX task on the console UART until the task watchdog reboots the device. SharedBusEthernet installs esp_eth directly so its custom_spi_driver callbacks can take spiLock, the mutex the radio, display, SD and sensors already share. It derives from NetworkInterface, so localIP(), connected(), config() and the GOT_IP events are unchanged. Selected by ETH_SHARED_SPI; boards without it keep the stock ETHClass path. Measured on T-Connect-Pro under a 150 x 1472 byte flood with the display active: 34948 truncated frames, 3 reboots and 20% packet loss before, none after. * fix(cst226se): honour reset pin, screen rotation and skip wrong-model probes Drive TOUCH_RST when the variant defines one, and stop passing I2C pins to begin() so SensorLib does not re-init a bus the scan already owns. Derive touch geometry from SCREEN_ROTATE the way TFTDisplay does, so a rotated panel maps to the landscape UI rather than the raw panel size. Use TouchDrvCST226 rather than the TouchDrvCSTXXX wrapper. Pinning the model does not stop the wrapper walking CST816 and CST92xx, whose retries cost about 3.5s of boot. T-Beam behaviour is unchanged. * style: trim comments to the two-line limit Follows the comment rule in .github/copilot-instructions.md, which the original commits missed. * feat(t-connect-pro): use the T_CONNECT_PRO hardware model Depends on meshtastic/protobufs#1062. Does not build until that merges and the generated headers are synced, since meshtastic_HardwareModel_T_CONNECT_PRO does not exist yet. Drops -D PRIVATE_HW, which becomes a no-op once HW_VENDOR resolves, and promotes board_level to release. * chore(t-connect-pro): mark as community supported Support level 3, matching the other unlicensed LilyGo boards. * fix(w5500): roll back partial init when begin() fails begin() returns early when ethHandle is set, so a failure after esp_eth_driver_install() left the handle populated and every later call returned true with no working driver. teardown() releases the event handler, netif glue, netif, driver, PHY and MAC in reverse creation order, and every failure path now uses it. * refactor(w5500): drop config the custom SPI driver never reads spi_devcfg and spi_host_id are only read by w5500_spi_init, which esp_eth skips when custom_spi_driver is set, so the device config fields were dead. Also drops the handle() accessor, its only caller is the class's own event handler, the eventRegistered flag, since unregistering an unregistered handler is safe, the redundant _esp_netif guard around destroyNetif(), and the TFT_CS indirection, which this panel path does not read. * fix(w5500): fail begin() when the event handler cannot register The return value was ignored, so a failed registration still started Ethernet and returned true while onEthEvent never fired. WiFiAPClient would then miss ETH_CONNECTED, GOT_IP and DISCONNECTED, leaving the link up with the firmware believing it was down. |
||
|
Thomas Göttgens
|
9fe0360f4e |
fix(position): stamp the broadcast cadence only when a position packet was actually sent (#11751)
* fix(position): stamp the broadcast cadence only when a position packet was actually sent * fix(position): honor the router verdict and always fall back to nodeinfo when no position goes out * fix(position): snapshot Time::getMillis() for the throttle stamps and trim the added comments * fix(position): consume the radio-generation change only on a position send that went out * refactor(position): share one smart-broadcast path and drop the duplicated fresh-position guard * fix(position): persist smart broadcasts to the transmit history |
||
|
|
51e45b3919 |
fix(telemetry): restore the noise floor feeder and stop shipping its default (#11749)
* fix(telemetry): don't broadcast the noise floor default as a real reading LocalStats.noise_floor has no has_/presence bit, so RadioLibInterface's NOISE_FLOOR_DEFAULT (-120 dBm) placeholder was indistinguishable on the wire from a genuine -120 dBm reading whenever no valid RSSI sample had ever been collected (radio not idle when sampled, or every reading falling outside the plausible bounds). Gate the assignment on hasNoiseFloorSamples() and log a warning instead of shipping the placeholder. The field stays at its zero-init default in that case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Address review: trim the noise_floor comment and demote the log to debug An empty sample buffer is expected during early boot, so LOG_WARN overstated it. * Restore the periodic noise floor feeder lost in merge |
||
|
Thomas Göttgens
|
ef4bfff092 | fix(nodeinfo): consume the radio-generation change only on a nodeinfo send that went out (#11752) | ||
|
github-actions[bot]andcaveman99
|
868604514a |
Update protobufs (#11750)
Co-authored-by: caveman99 <25002+caveman99@users.noreply.github.com> |
||
|
github-actions[bot]andcaveman99
|
fdb67309aa |
Update protobufs (#11741)
Co-authored-by: caveman99 <25002+caveman99@users.noreply.github.com> |
||
|
Jason P
|
0221fc8044 | Address TFT color overlaps in BaseUI (#11735) | ||
|
Andrew Yong
|
104923730f |
fix(metadata): report all compiled-out module configs (#11709)
* fix(metadata): report all compiled-out module configs Add MQTT_CONFIG, NEIGHBORINFO_CONFIG, STOREFORWARD_CONFIG and TELEMETRY_CONFIG bits to getDeviceMetadata().excluded_modules, guarded by the same macros that gate the modules in src/modules/Modules.cpp (MESHTASTIC_EXCLUDE_MQTT, MESHTASTIC_EXCLUDE_NEIGHBORINFO, MESHTASTIC_EXCLUDE_STOREFORWARD, HAS_TELEMETRY). Widen three existing conditions: PAXCOUNTER_CONFIG now also reports when an ESP32 build sets MESHTASTIC_EXCLUDE_PAXCOUNTER; BLUETOOTH_CONFIG now also reports when HAS_BLUETOOTH is 0 on nRF52/ESP32; NETWORK_CONFIG collapses the per-arch nRF52/RP2040 arms into a single !HAS_NETWORKING check. Clients read excluded_modules to decide which module config screens to show. Four bits were never set when the module was compiled out, and three were set only for a subset of the affected builds, so clients offered config screens for modules absent from the firmware: MQTT on every STM32WL target, TELEMETRY on nrf54l15 and minimize builds, NEIGHBORINFO on russell and several nRF52 RAK boards. The change is confined to getDeviceMetadata(). The bitmask is advisory: clients use it to hide menu entries and it does not touch the module config wire protocol. No PhoneAPI, NodeDB or AdminModule changes. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(metadata): exclude Store & Forward on unsupported architectures Define MESHTASTIC_EXCLUDE_STOREFORWARD for any build that is neither ARCH_ESP32 nor ARCH_PORTDUINO. StoreForwardModule registers only on those two architectures, but the macro was previously set only by minimize builds and a few variant flags, so getDeviceMetadata() still advertised STOREFORWARD_CONFIG on nRF52, RP2040 and STM32WL. Every other use of the macro is already nested in an ARCH_ESP32/ARCH_PORTDUINO block, so ESP32 and Portduino builds are unaffected. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> |
||
|
Garth Vander HouwenandThomas Göttgens
|
83198c1cbb |
fix(pki): reject a restored pre-2.8 low-entropy key at set time, explain the swap (#11686)
* fix(pki): reject a restored pre-2.8 low-entropy key at set time, explain the swap Restoring/setting a private key is a private-key change: the public key is *generated* from it. The low-entropy blacklist check in generateCryptoKeyPair runs against the stored public_key at entry, which is empty on a bare key restore — so a known pre-2.8 weak key derived from the provided private key was never caught at set time. It was only detected on the next boot (once the weak public key had been persisted and re-checked), which looks to the user like their saved key silently "did not stick", and their node number (== crc32(public_key)) had quietly changed too. - NodeDB::generateCryptoKeyPair: in the provided-private-key branch, re-check the *derived* public key against LOW_ENTROPY_HASHES. If it matches, replace it with a fresh secure keypair and set keyIsLowEntropy so the reason is surfaced. - AdminModule set-config(security): when the restore path regenerated a rejected low-entropy key, send a client warning at set time explaining the key can't be restored and the node number changed. Scoped to that branch so a stale flag from a boot-time regeneration can't fire on unrelated security sets. No protobuf changes; reuses the existing ClientNotification warning path. Signed-off-by: Garth Vander Houwen <garthvh@yahoo.com> * fix(pki): gate low-entropy restore warning on successful keygen generateCryptoKeyPair returns false on an unset LoRa region before resetting keyIsLowEntropy, so the set-time warning could fire on a stale flag. Capture the return value and require both. Shorten the rationale comments to two lines each. * fix(pki): clear key sizes when a restored private key derives nothing The provided-private-key branch sets private_key.size and public_key.size to 32 before regeneratePublicKey() runs. On failure it returned false with both sizes still set, and AdminModule persisted that pair; every later keygen then re-derived from the same dead key. Clear both on the failure path so the next keygen mints a fresh identity. Add test_admin_radio coverage for the set-time restore path: a derived low-entropy key warns and rotates, a stale keyIsLowEntropy flag with keygen blocked does not warn, and a failed derivation clears both sizes. * fix(pki): validate a restored public key that is itself blacklisted A restore supplying both private_key and public_key reached neither keygen branch, so a whole pre-2.8 low-entropy pair was accepted and persisted at set time and only caught on the next boot. Re-derive when the supplied public key is blacklisted, which routes it through the same rejection and warning as the bare-private-key restore. A non-blacklisted keypair import is unaffected. Install the test crypto stub through a helper and drop it in restoreAdminRadioGlobals(), so a failed assertion's longjmp cannot leak a freed engine into later tests. * fix(pki): only warn about a swapped key when one was actually swapped keyIsLowEntropy is set from the stored public key at function entry, so a restore whose supplied public key is blacklisted set it even when keygen merely re-derived the public key from a private key that was kept. The warning then claimed a new key had been generated and the node number changed, which was only half true. Gate it on the private key actually being replaced. * fix(pki): re-check a freshly minted keypair against the blacklist Both mint sites called crypto->generateKeyPair() once and trusted the result, so an entropy source still producing known-weak keys could persist another blacklisted identity. Route both through a helper that re-checks and retries a bounded number of times, then logs if it cannot do better. Pass the caller's own copy of the private key to generateCryptoKeyPair() instead of config.security.private_key.bytes, which aliased the memcpy destination inside it. * fix(pki): fail keygen when every replacement stays blacklisted generateBlacklistCheckedKeyPair() logged an error after exhausting its retries but left the compromised keypair in place and its callers marked the keygen successful, persisting exactly the identity the check exists to reject. Return a flag, clear both key sizes on exhaustion, and abort both callers so the next keygen starts clean. Match the declaration guard to the definition's, and derive the expected mint count in the retry test from the configured one. * refactor(pki): drop the keygen retry loop, fail on the first weak mint Retrying cannot help: an entropy source that lands on one of the twelve blacklisted keys is broken, and a second call to it produces the same result. With real entropy the odds are ~2^-250, so the loop never runs twice in practice either. Check once and fail, which is the same guarantee in a third of the code. * fix(pki): check the derived key on the stored-private-key path too factory_reset_config keeps the private key and clears the public one, so the entry check sees no stored key, reports "not low entropy" and takes the regenerate branch, which adopted whatever it derived. A preserved pre-2.8 key was therefore accepted for a whole boot cycle before the next boot caught it - the same silent revert this PR exists to remove. Hoist the post-derive blacklist check into a helper and use it on both derive paths. * fix(pki): clear key sizes when stored-private derivation fails too The stored-private-key path set public_key.size to 32 up front and left it there when regeneratePublicKey() failed, so config claimed a pair the node never got - the same defect already fixed on the provided-key path. Both paths now derive through one helper that clears on failure and vets the derived key, replacing the separate blacklist-replace helper. --------- Signed-off-by: Garth Vander Houwen <garthvh@yahoo.com> Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> |
||
|
Andrew Yong
|
3d1d1ef392 |
fix(stm32wl): advertise canShutdown if HAS_LSE (#11707)
* fix(stm32wl): advertise canShutdown if HAS_LSE Define HAS_CPU_SHUTDOWN on HAS_LSE STM32WL builds and, on that path, report canShutdown in getDeviceMetadata() from the runtime stm32wlRtcAvailable() check. canShutdown was always false on STM32WL: HAS_CPU_SHUTDOWN was never set for the architecture and pmu_found is never set there, so apps hid the shutdown control even though deep-sleep shutdown works on HAS_LSE builds via cpuDeepSleep() -> STM32LowPower::shutdown(). Reading the runtime check keeps the report accurate when the LSE crystal fails to lock, where cpuDeepSleep() resets instead of sleeping. The #else branch is untouched, so non-STM32WL and non-HAS_LSE builds report canShutdown exactly as before. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(stm32wl): reject HAS_CPU_SHUTDOWN without HAS_LSE Add an #error in architecture.h when an STM32WL build has HAS_CPU_SHUTDOWN set but HAS_LSE unset. getDeviceMetadata() takes the stm32wlRtcAvailable() branch under HAS_CPU_SHUTDOWN, but that function is compiled only under HAS_LSE, so a build forcing HAS_CPU_SHUTDOWN=1 with HAS_LSE=0 would reference it with no declaration or definition. No current variant does this, and architecture.h derives HAS_CPU_SHUTDOWN from HAS_LSE in the same block, so ordinary builds are unaffected. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> |
||
|
Jonathan BennettandClaude Opus 5
|
43155f3f90 |
feat: log heap watermark, largest free block and subsystem breakdown (#11660)
* feat: log heap watermark, largest free block and subsystem breakdown The periodic heap line reported only free/total, which cannot distinguish a leak from fragmentation, and the MemAudit per-subsystem breakdown was only ever printed at boot. Add ESP.getMinFreeHeap()/getMaxAllocHeap() wrappers to MemGet (0 on platforms that cannot report them) and include both in the 5-minute line, then log the MemAudit breakdown on the same tick. A falling watermark is a leak; a steady watermark with a shrinking largest block is fragmentation, and the breakdown names the tagged subsystem that moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9ZacpGtsA6DWavqr5Ty2i * docs: correct the watermark interpretation in logHeapUsage comment A single step down in the minimum-free watermark is a transient allocation, not proof of a leak; it takes repeated new lows across samples. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9ZacpGtsA6DWavqr5Ty2i --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Andrew Yong
|
3c04a79031 |
fix(stm32wl): improve reboot-to-DFU reliability (#11698)
- In enter_dfu, arm enterDfuAtMsec = millis() + 5s and return instead of resetting inline; the want_response ACK then goes out the normal path and Power::powerCommandsCheck() calls enterDfuMode() at the deadline. Nudge the deadline off 0 in the rare case the addition wraps to it, since powerCommandsCheck() reads 0 as unarmed. The delay is the client's detach window - and the margin a WebSerial web flasher needs (meshtastic/web-flasher#426). - In enterDfuMode(), stop the GPS and drain/end every configured UART before the reset. The ROM bootloader autobauds off the first byte on USART1 (PB6/PB7) or USART2 (PA2/PA3), and on every WL variant a console UART or the GPS stream sits on those pins. Factor the drain into quiesceSerial() and reuse it in cpuDeepSleep(). - Move earlyBootCheck from constructor(101) to .preinit_array, ahead of the core's premain()/SystemClock_Config() whatever the link order, and reset RCC before jumping to system memory. The handler used to reset the MCU inline, before the ACK was sent and while the client still held the console UART. The STM32WL ROM bootloader autobauds off the first byte received; a stray byte during the handoff (a trailing protobuf frame, a port-close DTR/RTS glitch) desynced it and left the device unreachable at any baud until a hard reset. STM32WL only: every hunk is behind #if defined(ARCH_STM32) or lives in main-stm32wl.cpp. nrf52, rp2040 and the rest are unchanged. Known limitation: gps->disable() only issues a UBX sleep command, so a non-u-blox or otherwise free-running GPS with no hardware enable/standby pin keeps transmitting on its UART past this point. If that UART is USART1 (PB6/PB7) or USART2 (PA2/PA3), the ROM bootloader can still autobaud onto the GPS stream instead of the host. New STM32WL hardware designs should keep GPS UARTs off those two bootloader-autobaud pins, or provide a way to power down or hold the GPS in reset before DFU. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> |
||
|
Lynxie
|
4cb912ff55 | fix(gps): remember valid fixes across search cycle (#11697) | ||
|
AustinandClaude Opus 5
|
1ed10f4883 |
fix(raspihttp): build against OpenSSL 4.0's const X509 name getters (#11523)
Ubuntu 26.10 ships OpenSSL 4.0, which const-qualified the return of X509_get_subject_name() and X509_get_issuer_name(): 3.5/3.6: X509_NAME *X509_get_subject_name(const X509 *a); 4.0: const X509_NAME *X509_get_subject_name(const X509 *a); generate_self_signed_x509() grabbed the certificate's own subject name and mutated it in place, so the assignment to a non-const X509_NAME * now fails to compile. Unlike notBefore/notAfter there is no X509_getm_ mutable variant to fall back on. Build the X509_NAME standalone instead and hand it to X509_set_subject_ name()/X509_set_issuer_name(), which take a const name and copy it on every OpenSSL from 1.1.0 through 4.0. The setters dup the name, so ours is freed on both the success and failure paths. This also lets the X509_NAME_add_entry_by_txt() calls be error-checked, which they were not before; the caller already X509_free()s the partially built cert when we return -1. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Ben MeadorsandTom
|
df34ef1081 |
fix(radio): recover from chip state loss in the RX/TX hot paths too (#11678)
* fix(radio): recover from chip state loss in the RX/TX hot paths too * fix(radio): address CodeRabbit findings on the hot-path recovery PR (#11680) * fix(radio): address CodeRabbit findings on the hot-path recovery PR SX128x: startReceive() still called the old asserting setStandby() before the new trySetStandby(). The assert fired first, so the recovery path added below it could never run - the exact chip-state-loss crash this PR exists to fix was still live on SX128x. Remove the stale call. LR11x0: resolvedTcxoVoltage was set once after the primary begin() attempts, but two later paths - firmware recovery and the one-shot firmware update - call begin() again with tcxoVoltage and never updated it. On a TCXO_OPTIONAL board that only came up via one of those paths, reinitChip() would recover with the wrong oscillator setting. Update resolvedTcxoVoltage after each of those begin() calls too. LR20x0: reconfigure() discarded RadioLibInterface::reconfigure()'s result - the band-hop path always returned true regardless, and the same-band path reused the same flag for chip-programming errors, so a base-class failure could both mask itself as success and wrongly trigger a full re-init. Track the base-class result (reconfigureSuccess) separately from the chip result (standbySuccess), and return the former. Also shortens the recovery-rationale comments in RadioLibInterface.h and SX126xInterface.cpp to 1-2 lines per the repo's comment convention, the rationale now covered once in the base class. * fix(radio): finish the recovery ladder and stop recovery from rebooting Follow-up to the CodeRabbit findings, plus two gaps found auditing the branch against its own intent (never reboot on chip state loss; recover in place). RX left off was unrecoverable on an idle node. Every startReceive() call site is event-driven - RX/TX ISR, the CAD-busy branch, startSend()'s failure path, init(), reconfigure() - and a radio with RX off cannot raise an RX interrupt, so nothing re-arms it unless the node happens to transmit or the user changes config. A listen-only or quiet node stayed deaf for good, which is worse than the reboot this replaced. main.cpp's existing 60 s AGC tick now calls periodicRadioMaintenance(), which re-arms RX when rxOffline is set and otherwise does the AGC reset as before. In-place repair now gives up rather than retrying forever. After MAX_CHIP_RECOVERY_FAILURES consecutive failures - a throttle window apart, so minutes of a provably dead chip - schedule rebootAtMsec, the same deliberate reboot Portduino already uses for LoRa_in_error. A reboot re-runs init(), which redoes the power-enable GPIOs, settle delays and TCXO probing that begin() alone skips. Both counters reset in RadioLibInterface::startReceive(), the one point every driver reaches only once the chip accepts the RX start. SX128x: reconfigure()'s recovery reached reinitChip()'s region-mismatch branch, which rewrites config.lora.region, saves, and calls ESP.restart() / NVIC_SystemReset(). A runtime recovery must never reboot - that is the crash this path exists to prevent, and it would fire with a config save pending. Gated to the boot-time call via a fromInit parameter. LR20x0: a rejected setRxBoostedGainMode cleared the success flag and so forced a full fullBegin() chip reset. It is a warn-level cosmetic setting, treated as warn-only in LR11x0's equivalent, and not a lost-state signature. Also logs suppressed recovery attempts at debug level; previously a chip that stayed dead recorded one critical error and then went completely silent. * fix(radio): count RX re-arms, not re-inits, in the recovery ladder LR20x0's recoverChipStateLoss() is fullBegin(), which re-arms RX itself but reports success on begin() alone. A re-init that came back with RX still dead therefore reset chipRecoveryFailures, so a chip that could be re-inited forever while never receiving again held the ladder at zero and never reached the reboot. The other drivers had the same hole from the other side: the caller's retry startReceive() runs after the reset, so a retry that failed again left the count cleared. RadioLibInterface::startReceive() is now the only place the ladder clears, and it only runs once the chip actually accepted RX. The threshold is judged at the top of the next attempt - a throttle window later, after that attempt's retry (the caller's, or fullBegin's own) has had its chance to clear it. That also drops the old false positive where the reboot was armed before the retry that would have succeeded. RF95Interface::startReceive() set isReceiving directly instead of calling the base, so on RF95 nothing ever cleared rxOffline or the ladder: the first failed RX start left periodicRadioMaintenance() re-initing forever, and with the count now advancing it would have rebooted a working radio. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> --------- Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com> |
||
|
Thomas Göttgens
|
14eaa5587d |
Honor mute when waking the screen for a received message (#11688)
* fix(ui): honor mute when waking the screen for a received message TextMessageModule fired powerFSM.trigger(EVENT_RECEIVED_MSG) for every text packet, gated only by shouldWakeOnReceivedMessage(), which checks external notification, device role and battery level but never the mute flags. A muted channel therefore suppressed the banner and still lit the screen. MessageRenderer::handleNewMessage() only computed mute for MessageType::BROADCAST, so a DM from a muted node produced a banner and a wake. Add isMutedForPacket() in Channels: a DM addressed to us reads the sender's NodeInfoLite mute bit, every other packet reads the mute bit of the channel it arrived on. This is the predicate ExternalNotificationModule already applied to the buzzer, vibra and LED outputs, hoisted so all three call sites share it. Bell and alert messages still break through mute on both paths, unchanged. No protobuf or config change: ChannelSettings.module_settings.is_muted and the NodeInfoLite mute bit already exist and are already settable from the device menu and via AdminMessage.toggle_muted_node. Closes #11674 * fix(ui): let an alert break through mute on the screen wake path In COLOR display mode TextMessageModule skips handleNewMessage(), so powerFSM.trigger(EVENT_RECEIVED_MSG) is the only wake an alert gets. Gating it on mute alone dropped that wake for a bell on a muted channel. Add MeshService::isAlertPayload(): an ASCII BEL in the payload while at least one alert_bell_* output is enabled. The wake gate is now "not muted, or an alert". MessageRenderer uses the same predicate instead of its own inline bell scan, which also lifts that scan's arbitrary 100 byte cap. Rename three test cases. Their names carried exactly 35 characters after the test_ prefix, which matches the Lob API key format and tripped trufflehog in the trunk check gate. |