* CH341: fewer waits on the USB bus per radio command
Every RadioLib command to a radio behind a CH341 waits on the USB bus about
five times: the BUSY read before it, CS low, the command, CS high, and the
BUSY read after, with a 1 us settle between the last two that usleep() turns
into ~50 us of timer slack.
- A BUSY read that follows the read that found BUSY low after a command is
answered from it, once, within 1 ms and only if nothing else has been sent
to the adapter in between: RadioLib's wait before a command then costs
nothing after the wait that ended the last one.
- Delays under 100 us spin on the steady clock instead of sleeping.
- With a libch341 that has pinedio_transceive_select(), and where Lora.CS is
D0, the CS levels travel with the SPI transfer and RadioLib's separate CS
writes are dropped. Against an older libch341 this part compiles out.
The HAL learns the radio's CS and BUSY pins from initLoRa(), since USBHal.h
is included by PortduinoGlue.h ahead of portduino_config.
Measured on an SX1262 over a CH341 (SHORT_TURBO, 6 v 6 runs of 300 s): the
three-command CAD setup fell 1.01 -> 0.62 ms, CAD arm to air 6.60 -> 5.23 ms,
and total packet loss 1.51 -> 1.13% (p=0.048), with collisions 4.83 -> 0.67
per run (p=0.002).
Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9
* Bump libch341-spi-userspace to 027cde5 for pinedio_transceive_select()
Picks up libch341#6 (1 ms interrupt poll, poll thread woken on detach) and
#7 (pinedio_transceive_select()), so the packed-CS path in USBHal.h builds
in rather than compiling out.
Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Whitespace and comment-alignment only: the output of `trunk fmt --all` on
develop @ d49cf21c3 with the pinned clang-format@20.1.0 and prettier. 48
files had drifted; trunk-action only checks the files a PR touches, so the
drift never fails CI and instead lands as noise in the next PR to edit any of
them. No code change.
* 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>
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>
* fix(radio): put the beacon restore back inside completeSending's if (p)
Reverts the RadioLibInterface and RadioInterface changes from #11573
(ac330e6a6). Hoisting MeshBeaconModule::reconfigureForBeaconTX() out of the
if (p) block changed its meaning from "a send completed" to "the radio went to
standby, for any reason" - and every driver's setStandby() calls
completeSending() unconditionally: on the pre-TX LBT scan, on startReceive(),
and inside reconfigure().
Two shipping faults followed, both confirmed on hardware the next day.
Every beacon transmitted on the wrong preset. isChannelActive() standbys the
radio immediately before each transmit, so the restore ran between the switch
and the key-up. The packet went out carrying the beacon channel hash with home
modem settings - inaudible to listeners on the target preset, an unknown hash
to listeners on the home one. Inert in both directions.
And unbounded recursion: the restore calls iface->reconfigure(), which
standbys, which calls completeSending(), which restores again, each level
running a full applyModemConfig(). It terminated in a HardFault and a silent
reboot (Reset reason 0x4 on nRF52, no panic output). The crash masked the
misdirection - the node died before Started Tx, so the wrong preset was
invisible until the recursion was fixed.
completeSending() clears sendingPacket at the top, so any nested call sees
p == NULL. The if (p) block was an accidental re-entrancy guard, and nothing
named it as such; removing it created both faults at once. Name it now.
This also reverts the beginSending() failure return that motivated the move,
and the startSend() scaffolding built to reach the restore on that path. The
payload bounds check it replaced is reinstated in the next commit, at a point
where refusing a packet is already a supported outcome.
* fix(radio): bound the payload at the radio queue, not mid-transmit
#11573 replaced beginSending()'s assert with a runtime check that logged,
released the packet and returned 0. beginSending() had never returned 0
before, so startSend() gained a failure path it had to unwind - and the
release moved ownership of the packet out of the caller that held it. That
new return value is what made hoisting the beacon restore look necessary.
The check itself is worth keeping. MeshPacket.encrypted has a nanopb maximum
of 256 bytes against a 240-byte radio buffer, and beginSending() is on the
path for relayed frames and phone-sourced packets, neither under our control.
Asserts are commonly compiled out in release builds, so what shipped was an
unchecked 256-into-240 memcpy driven by remote input.
Move it to Router::send(), immediately before iface->send(p) - the single
funnel for every over-the-air transmit. Refusing a packet there is already a
supported outcome: it returns TOO_LARGE, which is what perhapsEncode() already
returns for the same condition on the decoded path, and releases or NAKs
exactly as the duty-cycle limit above it does. Nothing radio-side has happened
at that point, so there is no half-started transmit to tear back down.
perhapsEncode()'s existing check does not cover this case: relayed and
phone-sourced frames arrive already encrypted and never reach it.
beginSending() keeps a last line of defence, but clamps rather than failing,
so it stays a call that always succeeds. Adds MAX_RADIO_PAYLOAD_LEN so both
sites name the same number instead of recomputing it.
Nothing about a beacon can trigger any of this - broadcast_message is
admin-truncated to 100 bytes, the whole MeshBeacon protobuf tops out at 180,
and observed beacons run to 106 - which is why this is separated from the
beacon changes rather than carried with them.
Tests: Router::send() refuses an oversized payload and still sends one that
exactly fills the buffer; beginSending() clamps instead of rejecting, and
leaves ordinary traffic whole.
* fix(beacon): guard the radio switch/restore against re-entry and early restore
Two checks in reconfigureForBeaconTX(), both independent of radio state, so
the switch/restore state machine no longer rests on sendingPacket's lifetime -
which is exactly the implicit coupling that let #11573 through.
A re-entrancy guard. Both branches end in iface->reconfigure(), whose
setStandby() runs completeSending(), which calls straight back in here. While
one call is applying a config, a nested call returns false and leaves it
alone. This covers the switch branch too, which had the same exposure with a
quieter symptom: a second switch before the restore would take the re-entrant
call as a restore and undo the switch still being applied, sending the beacon
on the home channel instead of its target.
A restore gate. The restore now waits for the packet that armed the switch to
actually finish, tracked by id against our own target table rather than by
asking the radio. Every caller that completes or abandons a beacon clears that
packet's target settings first, so a live entry means the TX has not happened
yet. cancelSending() now clears too, which is what keeps a cancelled beacon
from pinning the radio on the beacon config.
Together these make explicit the invariant completeSending()'s if (p) block
was carrying by accident: a future hoist of that call gets a logged no-op
instead of a crash and a misdirected beacon.
Also sets radioSwitched before reconfigure() rather than after, in both
branches, so the flag never describes a radio state that is not yet true.
Diagnostics, because every step of this dance was previously silent about its
own state. Count consecutive switches with no restore between them and log the
depth on both sides, so a change-change-change-restore run reads off the log;
switch #2 onwards prints the held home snapshot, which is the value that has
to survive a second switch. The restore names the config it is restoring to,
so a stale snapshot is visible directly. The re-entrancy guard logs when it
fires - expected exactly twice per beacon, so a burst means something new is
re-entering rather than a silent reboot. And setTargetRadioSettings() now
warns on the slot eviction that previously left a packet to key up on whatever
config was running - no crash, no log, wrong channel.
Reachable only with beacon broadcast enabled (the default flags are
LISTEN_ENABLED | LEGACY_SPLIT, so broadcast is off) and a target differing
from the running config; an identical target takes the early return and never
switches.
Tests: three re-entrancy cases against a RadioInterface whose reconfigure()
re-enters exactly as completeSending() does - bounded, so a regression fails
an assertion instead of overflowing the stack and taking the runner with it -
plus a restore that must defer until the beacon it switched for completes.
* fix(beacon,radio): address review findings on #11596
Payload ceiling was one byte too generous. RadioBuffer::payload is 240 bytes
because the buffer reserves MAX_LORA_PAYLOAD_LEN + 1, but the PHY caps a whole
frame at 255 and beginSending() adds a 16-byte header - so a 240-byte payload
produced a 256-byte frame. Define the ceiling as MAX_LORA_PAYLOAD_LEN -
sizeof(PacketHeader), matching what perhapsEncode() already enforces, with a
static_assert that it still fits the buffer.
Target-table eviction could unblock the restore gate. With every slot live,
setTargetRadioSettings() overwrote slot 0 - and if that slot held the packet the
outstanding switch is gated on, the restore came unblocked and put the home
config back under a beacon that had not keyed up. Skip that entry when choosing
a victim, and refuse the target outright if every slot is in flight. Needs
radioSwitched/switchedForId at file scope so the setter can see them.
Restore on every abandon path, not just the clear. cancelSending() dropped a
queued packet's target without restoring, so a beacon pre-switched by onNotify()
and then cancelled left the radio receiving on the beacon config;
removePendingTXPacket() did neither. Both now route through
abandonBeaconTarget(), as does startSend()'s tx-disabled branch. The restore
gate makes it a no-op when the abandoned packet is not the one we switched for.
No NAK on the oversize drop. p->channel is a wire hash by that point, not an
index, and Channels::getIndexByHash() is declared but never defined. Only
already-encrypted ingress can reach the gate anyway - perhapsEncode() bounds
everything it encodes - and those carry no index to answer on. Release and log.
Tests clear sendingPacket before releasing their packet, and assert against the
payload ceiling rather than the buffer size.
* fix(beacon): route the invalid-target drop through abandonBeaconTarget
onNotify()'s invalid-config drop was the one packet-abandonment path still
clearing the target directly instead of going through abandonBeaconTarget(),
so a packet that armed the radio switch and then failed validation would be
released with the radio left on the beacon config and nothing to restore it.
The helper's restore gate (targetRadioSettingsLive(switchedForId)) makes the
call a no-op for any packet that did not arm the switch, so this closes the
gap without risking a premature restore.
Also trims the switch-state comment to the two-line limit.
* fix(radio): take the abandoned packet as a pointer to const
cppcheck's constParameterPointer failed the check matrix on every board:
abandonBeaconTarget() only forwards the packet to clearTargetRadioSettings(),
which already takes a const pointer, so the parameter should be const too.
* refactor(radio): drive the beacon radio switch through TX hooks
RadioLibInterface named MeshBeaconModule at six call sites behind
MESHTASTIC_EXCLUDE_BEACON guards, so the driver carried per-packet beacon
state: when to switch preset, when a target config was invalid mid-transmit,
and when not to listen on a busy channel. Review on #11596 asked for the
module dependency to come out.
RadioTxHook is what the driver knows instead - beforeTransmit() returning
send/defer/drop, holdsRadio(), packetReleased() - on a self-registering
intrusive list, so nothing is allocated and a build without the beacon module
registers nothing and every call is a no-op. The four abandon paths (cancel,
remove-pending, TX disabled, completeSending) collapse onto one
packetReleased(), and the tri-state means the driver no longer has to know why
a packet wanted a re-delay or a drop.
MeshBeaconTxHook wraps the existing statics; the switch/restore logic, its
re-entrancy guard and its restore gate are untouched. It is created in
Modules.cpp inside the existing exclusion guard, so MESHTASTIC_EXCLUDE_BEACON
now works by nothing registering rather than by #ifdefs in the driver.
Behaviour is unchanged. The invalid-config LOG_DEBUG moves into the module and
the driver logs a generic refusal. Four tests cover the send/defer/drop mapping
and that an empty hook list is a no-op; native:test_mesh_beacon is 59/59.
Also notes in sendBeaconPacket that beacons uplink to MQTT on the primary
slot's uplink_enabled, and that the topic follows the beacon channel under the
crypto-override swap - both intentional.
* fix(beacon): restore the home config for a packet that jumps the queue
The restore gate added in 9cb7b96c9 refused to put the home config back while
the beacon that armed the switch was still live. That is right for a release -
completeSending() runs on every setStandby(), and restoring there would undo
the switch before the beacon had keyed up - but it also caught the case where
the driver is asking about a different packet it is about to transmit.
MeshPacketQueue::enqueue() inserts by priority (std::upper_bound over
CompareMeshPacketFunc), so an ACK or routing packet queued during the beacon's
deferred transmit delay lands ahead of it. beforeTransmit() then saw an
untagged packet, found the beacon still queued, skipped the restore and
returned PRETX_SEND - and the packet transmitted on the beacon's preset, slot
and region. It was encrypted and hashed for the home channel, so no receiver
on either preset could use it.
Apply the gate only to a null p. A non-null untagged packet is the driver
about to key up, which always restores; the restore returns PRETX_DEFER, so
the driver re-runs the delay and the channel scan on the config it will
actually transmit on. beforeTransmit() is the only caller that passes a
non-null untagged packet, so nothing else changes.
Found by CodeRabbit on #11596. native:test_mesh_beacon 60/60, including a
regression test for the queue transition; the four re-entrancy tests still
cover the null-p gate.
---------
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
* Fix for MeshBeacon packet leakage
* fix: add runtime payload size check against radiobuffer
* review fix for PR#11573: clear target radio settings before MeshBeacon packet release
* add unit test for radio buffer capacity check, removing related assert for the test
* review fix for PR#11573: add explicit verifaction against rejected packets
* feat(lora): state a pinned userPrefs preset as the unset region's intent
A vendor build can pin USERPREFS_LORACONFIG_MODEM_PRESET while leaving the
region unset, so a fresh flash comes up as region UNSET plus a deliberate
preset. Stock installs come up as region UNSET plus the LONG_FAST placeholder,
and nothing in FromRadio told the two apart - so clients treat every
unset-region node as factory-fresh and replace its preset with the region
default as soon as the user picks a region. A mesh pinned to SHORT_TURBO loses
every new node to LONG_FAST or LONG_TURBO, silently.
getRegionPresetMap() now emits an UNSET entry when, and only when, the build
pins a preset, stating that preset as both the group's sole entry and its
default. Stock builds are unchanged on the wire: no UNSET entry, which clients
already read as unconstrained.
This is intent, not enforcement. supportsPreset() still accepts any known
preset while the region is unset (#11496) and the radio is held silent either
way, so the device continues to honour whatever the user or an admin sets.
Costs one group slot and one region slot on pinned builds only (6->7 of 8,
34->35 of 38); exhaustion is logged and degrades to the existing unconstrained
behaviour.
* Trim comments to the project's one-to-two-line limit
- WiFiServerAPI/ethServerAPI apiPort and ethApiServer's listener are
create/destroy cycles that repeat across WiFi teardown and W5500
chip resets; the manual delete+null bookkeeping becomes reset().
(ethTlsApiServer's listener is left for a follow-up: that file is
already touched by the partial-init fix PR and converting it here
would conflict.)
- ContentHandler::handleFormUpload held its body parser raw with
delete on four separate exit paths of a per-request handler; any
future early return was a silent leak. unique_ptr removes all four.
- The portduino ch341Hal global becomes unique_ptr. The LoRa-error
recovery loop's delete/null/new sequence was correct only by
hand-preserved ordering; it becomes reset()/make_unique. RadioLibHAL
keeps a non-owning raw pointer, as before.
No behavior change.
* logging: strip redundant punctuation, level prefixes, and 'successfully' from log strings
The logger already appends a newline and prints the level tag, so
trailing '.', '!', '...', literal \n, and 'Error:'/'Warning:' prefixes
inside format strings are wasted flash bytes. Same for 'successfully'
(the affirmative form already implies it).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
* logging: tighten verbose log strings in modules, radio, platform, and system code
Rewrite wordy log messages to terser equivalents - drop filler words
(articles, 'attempting', 'due to', 'please'), use 'Can't X'/'X failed'
phrasing, and abbreviate where the codebase already does (config, init,
msg, BT). Format specifiers and argument lists are unchanged; distinctive
greppable tokens are preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
* logging: tighten verbose log strings in telemetry sensors and GPS
Same terseness pass: drop filler, 'Can't X'/'X failed' phrasing, common
abbreviations (temp, msg). Specifiers, arguments, and sensor-name
prefixes unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
* logging: tighten verbose log strings in mesh core
Same terseness pass over NodeDB, Router, MeshService, PhoneAPI,
RadioInterface, NextHopRouter, and PacketHistory: 'X failed'/'Can't X'
phrasing, imperative verbs, dropped filler. Specifiers and arguments
unchanged; duplicate literals kept identical to preserve linker string
dedup.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
* logging: 'Unable to/Could not/Cannot' -> "Can't" in log strings
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
* logging: clang-format rewrap after string shortening
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
* logging: restore boot-logo trailing newline and progress-dot strings
The terseness pass over-trimmed: the Meshtastic ASCII boot logo kept its
blank line via a trailing \n, and three bare "." progress ticks were
reduced to empty strings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1
* Update src/mesh/wifi/WiFiAPClient.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* logging: address review feedback on the terseness audit
- Node/packet IDs use the repo's 0x%08x convention in NextHopRouter,
NodeDB, AdminModule and CannedMessageModule. The sibling log in each
if/else pair is converted too, so a pair isn't split across two formats.
next_hop stays 0x%x - it's the last-byte relay hint, not a NodeNum.
- RTC: the read-path and set-path "not found" warnings were byte-identical,
so the linker deduped them and the log couldn't say which one fired.
Split into "RTC read:" / "RTC set:". (The four sites live in mutually
exclusive #ifdef branches, so the RTC family was never ambiguous.)
- SCD4X getAmbientPressure()/setAmbientPressure() logged "altitude", and
SCD30 getASC() logged "Can't send command" for a read. Both now name the
operation they actually perform.
- LOG_ERROR already carries the level: ". Error: %u" -> ", rc=%u" (matching
the existing rc=%d house style) and "Error executing X()" -> "X() failed".
- Typos and wording: "OTA partiton. (Reason" -> "OTA partition (reason",
"CST3530 not response ~" -> "CST3530 no response", "Packet received with
to: of 0" -> "to=0", HostMetrics "Error decoding" -> "Can't decode", and
the dangling ": " on the NextHopRouter retransmission line.
Printf specifier sequences are byte-identical on all 37 touched lines apart
from the 6 deliberate %x/%u -> %08x node-ID widenings, all on uint32_t args.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
* add LR 2021 to portduino, and allow Framebuffer devices to rotate the screen from config. Requires https://github.com/meshtastic/device-ui/pull/355 and supersedes https://github.com/meshtastic/firmware/pull/10567 and https://github.com/meshtastic/firmware/pull/11138
Many thanks to the original authors https://github.com/a-li3n and https://github.com/jessm33
* Build LR2021Interface.cpp in the wasm env
initLoRa() constructs LR2021Interface for Lora.Module: lr2021, so excluding
the file from the native-wasm source filter left the constructor undefined at
link time. LR20x0Interface.cpp stays excluded; it is template-only and comes in
via the InterfacesTemplates.cpp amalgamation.
Also replace the non-UTF-8 degree signs in the framebuffer rotation comment and
correct the rfswitch alias cleanup comment.
* Trim comments
* Report setenv failure for the framebuffer rotation
* Add explicit presence for MeshPacket.rx_time (arrival time)
rx_time is now proto3 optional with a has_rx_time presence bit, matching
the rx_rssi treatment. A node with no GPS and no phone connected yet has
no time source at all, so a bare 0 was indistinguishable from a genuine
1970-01-01 reading; downstream consumers (replay packets, JSON
serialization) now check has_rx_time instead of the value.
* Dedupe rx_time stamping into a shared helper; trim a debug log string
Extract the repeated haveTime/rx_time/has_rx_time stamp logic (5 call
sites across Router.cpp, MeshBeaconModule.cpp, MeshService.cpp) into
Router::computeRxTimeStamp()/stampRxTime(). Also shorten the new RTC.cpp
LOG_DEBUG string. Saves 48 bytes of flash on rak4631 (measured), no
behavior change.
* Fix has_rx_rssi presence carried unconditionally through StoreForward replay
preparePayload() set has_rx_rssi = true unconditionally on replay, regardless
of whether the packet's rx_rssi at store time was a genuine measurement (e.g.
MQTT-relayed packets carry no real RSSI). Store the presence bit alongside
rx_rssi in PacketHistoryStruct and restore it on replay instead.
Flagged by Copilot on #11271 (same root cause the has_rx_time explicit
presence work fixes) but never addressed before that PR merged.
* Trim comment blocks to the repo's 1-2 line guideline
.github/copilot-instructions.md:338 caps code comments at 1-2 lines; several
blocks added across the rx_time explicit-presence work ran well past that.
Also consolidates Time.cpp's file-level doc comment into Time.h, where the
rest of the Time:: API contract already lives.
No behavior change.
* Add rx_time explicit-presence test coverage
- test_meshpacket_serializer: has_rx_time=false fixture plus tests asserting
JsonSerialize/JsonSerializeEncrypted emit 0 rather than leaking the
millis() placeholder, alongside the has_rx_time=true baseline.
- test_stream_api: two tests driving a real PhoneAPI handshake (want_config_id
through STATE_SEND_PACKETS) that simulate a phone time-giving transaction
arriving before vs. after a queued packet is drained - covering both the
reconciled and the ships-with-placeholder-absent paths of
MeshService::reconcilePendingRxTimes().
* Fix three correctness issues flagged in review
- Time.h: drop the reserved-identifier include guard (_MT_TIME_H); pragma
once already covers it, matching convention elsewhere (e.g. RTC.h).
- Time.cpp: rebase getMillis64()'s wrap accumulator when the test seam
swaps clock sources, so a real<->injected clock jump isn't miscounted
as a genuine 32-bit wrap.
- NodeInfoModule: the 12h reply-suppression window is a local dedup
duration, not a wall-clock reading - switch it to Time::getMillis64()
so RTC-quality jumps and replayed packets' stale rx_time can't perturb
it.
- StoreForwardModule: has_rx_time was derived from *current* RTC quality
at replay time rather than stored at capture time, so a history entry
saved while time-blind could be misreported as a valid epoch once the
clock later improved. Persist the presence bit in PacketHistoryStruct
instead.
* tryfix CI
* post review fixes
* more test fixes
Decree of the Cabinet of Ministers of Ukraine dated February 25, 2026,*
No. 262, on amendments to Section 2 of the Plan for the Allocation and
Use of the Radio Frequency Spectrum in Ukraine, has harmonised
Ukrainian regulations on the 868 MHz spectrum with the EU's, thereby
making UA_868 legally obsolete.
* https://zakon.rada.gov.ua/laws/show/262-2026-п
Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com>
* Protobufs
* Wire up MEDIUM_TURBO modem preset
MEDIUM_TURBO (500 kHz, SF9, CR 4/5) already existed in the protobuf enum but
was never wired into firmware, so selecting it silently fell through to the
LONG_FAST default and rendered an "Invalid" display name.
Add its bw/sf/cr mapping (modemPresetToParams), display name (MediumTurbo/MedT),
PRESETS_STD membership (standard regions only — 500 kHz does not fit EU868's
250 kHz band, so it stays out of PRESETS_EU_868 and is rejected/clamped there),
and the MEDIUM SNR-grading bucket. Includes positive coverage in test_radio,
EU868-reject + US-accept coverage in test_admin_radio and test_mesh_beacon,
the STD preset count 9->10, an extended fuzz range, and the client-spec doc.
* Address review feedback on MEDIUM_TURBO tests
- test_mesh_beacon: assert has_mesh_beacon before checking the invalid preset was
cleared, so the EU868-cleared test can't pass on a dropped message (matches the
existing SHORT_TURBO test).
- test_fuzz_packets: draw modem presets from _ModemPreset_ARRAYSIZE instead of a
hard-coded 17 so the fuzz range tracks future enum additions automatically.
* Add software LoRa IRQ polling and an MCP23017 expander HAL for radios behind an I2C expander
Some boards route the SX126x control lines (NRESET/DIO1/BUSY) through an I2C
GPIO expander instead of native GPIOs, and don't wire the expander's /INT to
the MCU, so RadioLib cannot take a hardware DIO1 interrupt.
Two reusable pieces, both gated behind USE_MCP23017 / LORA_DIO1_SOFTWARE_POLL
so builds that don't define them are unaffected:
- ExtensionIOMCP23017 + MCP23017LockingArduinoHal: a RadioLib HAL that maps
virtual pins (MCP23017_VPIN_BASE .. +15) onto an MCP23017's GPIO, so
RESET/BUSY/DIO1 reads and writes become I2C transactions while the real SPI
GPIOs (SCK/MOSI/MISO/CS) pass straight through. A failed I2C read skips the
read-modify-write rather than clobbering the rest of the bank.
- LORA_DIO1_SOFTWARE_POLL: with no DIO1 interrupt available, the SX126x
interface polls the radio IRQ status register from the radio thread and
synthesizes the ISR_TX/ISR_RX events, filtering noisy preamble/header IRQs so
they can't starve TX. A 1 ms ISR_POLL_TICK drives the poll; TX timers may
overwrite the pending tick (pollMissedIrqs() is the backup that bounds the
latency of the busy-Rx contention window). Behaviour is unchanged on all
boards that keep the hardware interrupt.
* Add Meshnology W10 AIOT Dev Kit variant (SX1262 via MCP23017 expander)
ESP32-S3R8 + EBYTE E22-900MM22S (SX1262) + AXP2101 PMIC + Quectel L76KB GPS +
SPI TFT, with the radio's RESET/DIO1/BUSY and the LCD reset routed through an
MCP23017 I2C expander at 0x20. The expander's /INT is not wired to the ESP32,
so DIO1 uses the software poll. Pins come from the board schematic and the
vendor firmware; the expander is an MCP23017 despite the V1.1 schematic's stale
'TCA9555' label (the V1.2 placement diagram and all vendor code use the MCP23017
register map).
A local pins_arduino.h shadows the generic esp32s3 variant to drop RGB_BUILTIN,
which otherwise pulls the Arduino RMT HAL into the link and fails against this
build's trimmed FreeRTOS config.
Reports HardwareModel 140 (MESHNOLOGY_W10, already present in the protobufs).
Hardware-verified on a real W10 AIOT Dev Kit: AXP2101 PMU, MCP23017, SX1262
init + a full over-the-air TX/RX round trip through the expander, PCF85063 RTC,
SHT41 and QMI8658 auto-detected, and an L76KB GPS fix.
* meshnology-w10: enable ES8311 speaker for notification tones
Bring up the ES8311 codec (I2C 0x18) -> NS4150 amp -> speaker so the board can
play notification tones / ringtones over the I2S buzzer path (the
use_i2s_as_buzzer external-notification option). Codec2 voice stays out of
scope: it is SX1280-only and this is a sub-GHz board.
- variant.h: HAS_I2S + DAC_I2S pins (MCLK=1, BCK=2, WS=4, DOUT=5, DIN=3)
- platformio.ini: arduino-audio-driver + ESP8266Audio + ESP8266SAM
- extra_variants/meshnology_w10/variant.cpp: lateInitVariant() configures the
ES8311, mirroring the other Meshtastic ES8311 boards. Kept codec-only (no
main.h) so the audio driver's 'using namespace audio_driver' does not pull a
conflicting GpioPin into scope alongside Meshtastic's class GpioPin.
- AudioThread.h: toggle the NS4150 amp (MCP23017 EXIO_PA_CTRL) around playback.
Verified on hardware: a test tune played audibly through the speaker.
* meshnology-w10: address review feedback on the MCP23017 driver
- ExtensionIOMCP23017: serialize register access with a mutex, since the radio
HAL (BUSY/DIO1/RESET) and AudioThread (amp enable) now reach the expander from
different threads and the read-modify-write paths are not atomic.
- digitalRead: return a fail-safe HIGH on a failed read instead of LOW, so a
transient I2C error on the LoRa BUSY line can't look like 'ready' and let
RadioLib start an SPI transaction early. (DIO1 is polled via the radio IRQ
register, not this pin.)
- enablePinChangeInterrupt: use the checked readReg() overload and skip on a
failed read, matching the other read-modify-write helpers.
- meshnology_w10 variant.cpp: log a warning if an ES8311 register write NACKs
instead of silently leaving the codec half-configured.
- RadioInterface.cpp: guard the MCP23017 HAL branch with ARCH_ESP32 to match the
include and driver-file guards.
* Replace board-model audio/sleep ifdefs with reusable variant capability macros
Two shared-code spots keyed off specific hardware models; move the board-specific
detail into opt-in macros the variants define, so the core code stays generic and
new boards can opt into the behavior class without touching shared files.
- AudioThread amp control: drop the T_LORA_PAGER / MESHNOLOGY_W10 ifdefs. A board
with an I2S amp now defines AUDIO_AMP_ENABLE(on) in its variant.h (T-LoRa Pager
and Meshnology W10 do), and AudioThread just calls it around playback. The
expander includes are keyed on USE_XL9555 / USE_MCP23017 (capabilities) instead
of the model name.
- Light-sleep / DIO1 wakeup: drop the SENSECAP_INDICATOR check. Boards whose
LORA_DIO1 is an expander pin (not an ESP32 GPIO) define LORA_DIO1_EXTENDED_IO
(SenseCAP Indicator and Meshnology W10), and sleep.cpp keys off that for both
the light-sleep skip and the DIO1 GPIO-wakeup skip. This also fixes a latent
issue on the SenseCAP: it previously reached the GPIO-wakeup path and called
gpio_pulldown_en() on a virtual expander pin; it now skips that cleanly.
Builds verified on meshnology_w10, tlora-pager, and seeed-sensecap-indicator.
* meshnology-w10: silence cppcheck constParameterPointer on the ISR-callback helper
isIsrTxCallback() takes a function pointer it only compares; cppcheck's
constParameterPointer wants it 'pointer to const', which is meaningless for a
function pointer (the codebase already globally suppresses the sibling
constParameterCallback). Inline-suppress it to keep the check green.
* HopScaling: qualify the member assignment as this->count
cppcheck (CI's version) flags 'count = newCount;' at the end of trimIfNeeded()
as uselessAssignmentArg ('assignment of function parameter has no effect') - a
false positive, since count is a member that outlives the call. Write it as
this->count (matching the Step-1 assignment above) so the analyzer sees a member
write. This finding comes in via the develop merge, not this board; fixing it
here to unblock the PR's cppcheck check.
* first pass tests
* more tests
* Fix two crafted-admin-packet crashes found by the E5 fuzzer
Both are reachable from an authorized admin (local from==0, admin channel,
or PKC) - remote DoS:
1. SIGFPE in LoRa config validation. A set_config LoRaConfig with
use_preset=false and bandwidth=0 makes freqSlotWidth 0, so numFreqSlots
is 0 and `hash(name) % numFreqSlots` (RadioInterface.cpp) divides by
zero. Guard the modulo; the existing channel_num check then rejects/
clamps the config.
2. Stack overflow in Channels::getKey. A SECONDARY channel at the primary
slot with an empty PSK recursed into getKey(primaryIndex) forever. Skip
the primary-key borrow when chIndex == primaryIndex.
Re-enable the E5 admin fuzzer to hit both triggers again (use_preset both
ways incl. bandwidth 0, plus the set_channel tag) as regression guards.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Correct fuzz-test invariants after the crash fixes
- E5 admin fuzz: node eviction under a filling NodeDB is legitimate, so
assert only the bounded-count invariant, not that a specific seed node
survives 6000 mutating ops.
- TMM blitz: scope off the nodeinfo direct-response send path (it needs a
fully-wired MeshService/phone queue the fixture doesn't provide; the
deterministic directResponse tests cover it). The crafted-nodenum
rate/unknown/position cache stress is unchanged.
clod helped too
* realistic tests
* test: dedup fuzz RNG into shared test/support/DeterministicRng.h
The four in-tree fuzz suites (test_fuzz_decode, test_fuzz_packets,
test_hop_scaling, test_traffic_management) each carried a byte-identical
copy of the seeded 64-bit LCG (rngSeed/rngNext/rngByte/rngRange). Hoist
it into one shared header so there is a single generator to reason about
and no risk of the copies drifting. static inline keeps per-suite state
per translation unit and avoids -Wunused-function for suites that don't
use every helper. Also corrects a stale comment in test_traffic_management
(the blitz's nodeinfo direct-response path is intentionally left off).
No behavioral change: same constants, same per-suite seeds.
clod helped too
* test: fuzz uncovered ProtobufModule handlers and the MQTT downlink ingress
Extend the in-tree fuzz coverage to packet sources that previously had
none:
- test_fuzz_packets E8/E9/E10: drive PositionModule, DeviceTelemetryModule
and NeighborInfoModule at handleReceivedProtobuf directly (via using-shims,
bypassing the ProtobufModule reply/send path so no router is needed). The
fixture already stands up nodeDB/service/channels, and nodeStatus/powerStatus
are auto-initialized in main.cpp, so no new globals are required. Adds a
shared fuzzRxHeader() helper for crafting adversarial RX packet headers.
- test_fuzz_decode: add meshtastic_KeyVerification to the decode table. The
KeyVerification and StoreForward handler paths are documented as decode-level
only, with the concrete reason each is intrinsic (private-state gating /
PSRAM + self-pointer wiring), not a fixture gap.
- test_mqtt: test_receiveFuzzServiceEnvelope blitzes the non-RF broker-push
ingress (onReceiveProto) two ways - raw garbage bytes that must fail envelope
decode cleanly, and a well-formed ServiceEnvelope wrapping a crafted inner
MeshPacket over crafted channel_id/gateway_id - exercising the channel match,
isFromUs, XEdDSA receive policy and perhapsDecode chain. Adds a deliverRaw()
passthrough to MQTTUnitTest.
All under the coverage env (ASan/LSan). No firmware/src changes. Full sweep
GREEN 27/27, 544 cases.
clod helped too
* Harden LoRa/channel config against crafted admin messages; consolidate test helpers
Production (review findings on the hot-fuzz crash fixes):
- Clamp bandwidth at the source (clampBandwidthKHz) in checkOrClampConfigLora
and applyModemConfig so numFreqSlots can never be 0 for any consumer; a
bandwidth-0 set_config previously passed validation and re-armed the SIGFPE
on the next applyModemConfig.
- Guard applyModemConfig's hash % numFreqSlots (the validator's sibling modulo
was fixed earlier but this one was still unguarded).
- Enforce the primary-channel invariant in Channels::onConfigChanged: a config
demoting every slot now re-promotes the stale SECONDARY slot (keeping its
key) or restores the default channel if the slot is DISABLED, instead of
leaving every getPrimaryIndex() reader on a non-primary slot. The getKey
recursion guard stays as defense-in-depth.
Tests:
- New test/support/MockMeshService.h and AdminModuleTestShim.h replace four
byte-identical mocks and three divergent admin shims (test_mqtt's capturing
mock is genuinely different and stays).
- DeterministicRng.h: add rngFill() (replaces 14 hand-rolled fill loops) and
rngEdgeNodeNum() (unifies the three NodeNum boundary pools).
- Extract fuzzChannelSettings() shared by the set_channel case and fuzzBeacon.
- fuzzBeacon: the un-terminated branch now fills the whole buffer with non-NUL
bytes so the strnlen bound is actually stressed (~50% of iterations, not ~4%).
- E6 beacon fuzz: replace the TEST_ASSERT_TRUE(true) tautology with real
invariants (handler never consumes; offers land in lastReceivedOffer keyed
to the sender).
- Trim the seven over-long comment blocks flagged against the 1-2 line rule;
the FINDINGS trailer moves to this commit message (see production notes).
Full native suite GREEN 27/27 under the coverage (ASan/LSan) env.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Clamp UTF-8 char length in the emote walkers; add test_fuzz_emotes
A TEXT_MESSAGE payload is opaque protobuf bytes, so PB_VALIDATE_UTF8 never
screens it - invalid UTF-8 and truncated multi-byte lead bytes reach the
emote/width render path verbatim. EmoteRenderer's walkers advanced by
utf8CharLen(lead) without clamping to the bytes actually remaining, so a
truncated lead (e.g. a lone 0xF0, which claims 4 bytes) near the end of the
buffer made getUtf8ChunkWidth's memcpy read past the string. ASan confirms a
heap-buffer-overflow READ from measureStringWithEmotes.
Add utf8CharLenClamped() and use it at every walk site (width measure,
truncation cut-loop, and the draw-path text-run/chunk builders); the one
already-guarded site (matchAtIgnoringModifiers) is unchanged.
New test/test_fuzz_emotes drives measureStringWithEmotes and truncateToWidth
over adversarial byte strings (biased to embed/end in truncated multi-byte
leads) in exact-sized heap buffers so any over-read is a hard ASan fault. Its
headless display uses a synthetic font (firstChar 0, fontData centered in a
large buffer) so the stock OLEDDisplay::getStringWidth - which indexes the
font jump table with a signed char and over-reads for any byte >= 0x80 - does
not mask the finding. native-suite-count bumped 27 -> 28.
Full native suite GREEN 28/28 under the coverage (ASan/LSan) env.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Keep emote width measurement in-bounds for non-ASCII bytes
OLEDDisplay::getStringWidth (the utf8=false path EmoteRenderer uses on default
builds) indexes the font jump table by (c - firstChar) with a signed char and
no bounds check, so any byte outside printable ASCII - high bytes from UTF-8
text, but also a stray control byte like 0x0A - reads outside the font array.
On-device this reads adjacent flash and returns a garbage width; under ASan the
test_fuzz_emotes fuzzer flags it as a global-buffer-overflow, and it made the
non-ASCII width measurement meaningless either way.
The OLED driver is a pinned upstream dependency, so guard it firmware-side in
EmoteRenderer's getStringWidth helper: measure a sanitized copy where any byte
outside [0x20, 0x7E] counts as a '?' placeholder. Printable ASCII is unchanged
and the UA/RU lookup path is untouched.
test_fuzz_emotes now drives a real ArialMT font instead of the synthetic
in-bounds font it needed before this fix, so the suite exercises the true
production width path (utf8CharLen clamp + this sanitizer) end to end. The same
fuzzer tripped the global-buffer-overflow before this change.
Full native suite GREEN 28/28 under the coverage (ASan/LSan) env.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* clarify channel number formatting and fix the fixed-width padded nodeIDs
* use the explicit 0x%08x for packets and nodeIDs throughout
* LLM instructions
* Added lora region and preset maps
* Protos
* Address PR review feedback
- Log (and break/skip) when the region preset map exceeds its array bounds
instead of silently dropping regions
- Derive test bounds from the generated nanopb array sizes via sizeof()
instead of hard-coded magic numbers
- Fix want_config sequence comment (missing comma, STATE_SEND_MODULECONFIG)
- Specify a language on the spec's fenced code blocks (markdownlint MD040)
* Fix want_config stall: handle STATE_SEND_REGION_PRESETS in PhoneAPI::available()
available() had a separate per-state switch that wasn't updated for the new
state, so it returned false ('unexpected state 5') and getFromRadio() was
never called - the config handshake stalled after metadata and the client
timed out. Verified via the native simulator integration test.
Move default frequency (slot) override from RegionProfile to RegionInfo (set per-region).
This is usually set to `0`, but will be especially useful for Ham modes where each region default must fit within a band plan.
Co-authored-by: Copilot <copilot@github.com>
* Enable Lite and Narrow regions and introduce getEffectiveDutyCycle for Lite profiles
* Add TrafficType enum and extend getConfiguredOrDefaultMsScaled to manage based on regionProfile settings
* Refactor telemetry modules to include TrafficType in getConfiguredOrDefaultMsScaled calls
* Update submodule protobufs to latest commit
* Add support for new region presets and modem presets in menu options
* Add new LoRa region codes and modem presets for EU bands
* boof
* Add modem presets for LITE and NARROW configurations
* Update subproject commit reference in protobufs
* Update protobufs
* Refactor modem preset definitions to use macro for consistency and clarity
* Refactor modem preset cases to use PRESET macro for consistency
* fix: update LoRa region code for EU 868 narrowband configuration
Co-authored-by: Copilot <copilot@github.com>
* Fix test suite failure
Co-authored-by: Copilot <copilot@github.com>
* Add override slot override - for when one override isn't enough.
Co-authored-by: Copilot <copilot@github.com>
* address copilot comments
---------
Co-authored-by: Copilot <copilot@github.com>
* LR2021 radio on NRF_Promicro
Co-authored-by: Copilot <copilot@github.com>
* Refactor LR2021 interface includes and conditional compilation for improved clarity
Co-authored-by: Copilot <copilot@github.com>
* Refactor LR20x0 interface: remove unused includes and update comments for clarity
* Fix LR2021 max power definitions and add radio type detection tests
* remove potato radio type detection tests
* Include placeholder for DCDC - currently requires godmode
* Added godmode features - not enabled by default
---------
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
* Add powerlimits to reconfigured radio settings as well as init settings.
* Refactor preamble length handling for wide LoRa configurations
* Moved the preamble setting to the main class and made the references static
* Enhance LoRa configuration with modem presets and validation logic
* Rename bootstrapLoRaConfigFromPreset tests to validateModemConfig for clarity and consistency
* additional tidy-ups to the validateModemConfig - still fundamentally broken at this point
* Enhance region validation by adding numPresets to RegionInfo and implementing validateRegionConfig in RadioInterface
* Add validation for modem configuration in applyModemConfig
* Fix region unset handling and improve modem config validation in handleSetConfig
* Refactor LoRa configuration validation methods and introduce clamping method for invalid settings
* Update handleSetConfig to use fromOthers parameter to either correct or reject invalid settings
* Fix some of the copilot review comments for LoRa configuration validation and clamping methods; add tests for region and preset handling
* Redid the slot default checking and calculation. Should resolve the outstanding comments.
* Add bandwidth calculation for LoRa modem preset fallback in clampConfigLora
* Remove unused preset name variable in validateConfigLora and fix default frequency slot check in applyModemConfig
* update tests for region handling
* Got the synthetic colleague to add mock service for testing
* Flash savings... hopefully
* Refactor modem preset handling to use sentinel values and improve default preset retrieval
* Refactor region handling to use profile structures for modem presets and channel calculations
* added comments for clarity on parameters
* Add shadow table tests and validateConfigLora enhancements for region presets
* Add isFromUs tests for preset validation in AdminModule
* Respond to copilot github review
* address copilot comments
* address null poointers
* fix build errors
* Fix the fix, undo the silly suggestions from synthetic reviewer.
* we all float here
* Fix include path for AdminModule in test_main.cpp
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* More suggestion fixes
* admin module merge conflicts
* admin module fixes from merge hell
* fix: initialize default frequency slot and custom channel name; update LNA mode handling
* save some bytes...
* fix: simplify error logging for bandwidth checks in LoRa configuration
* Update src/mesh/MeshRadio.h
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* refactor: update throttling factor calculation and add unit tests for scaling behavior
* refactor: adjust throttling factor calculation for improved accuracy in different configurations
* Update src/mesh/Default.h
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update src/mesh/Default.h
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* refactor: enhance throttling factor calculation and introduce pow_of_2 utility function
* refactor: improve expected ms calculation in unit tests for Default::getConfiguredOrDefaultMsScaled
* refactor: improve scaling logic for routers and sensors in computeExpectedMs function
---------
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* simplify the observer pattern, since all the called functions are const getters.
* use arduino macro over std: for numerical values and refactor local variables in drawScrollbar()
* oh, so Cppcheck actually complained about const pointers not being const.
* slowly getting out of ifdef hell
* fix inkHUD warnings as well
* last 2 check warnings
* git checks should fail on low defects from now on
