mirror of
https://github.com/meshtastic/firmware.git
synced 2026-10-09 22:52:47 -04:00
1a5ddec04284c6ef644043fe3fb13c3c170bd8a2
196
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jonathan BennettandClaude Opus 5
|
8c0abbd522 |
feat(portduino): BLE peripheral support via BlueZ for meshtasticd (Raspberry Pi) (#11396)
* feat(portduino): BLE peripheral support via BlueZ for meshtasticd on Linux Adds the standard Meshtastic BLE service (toRadio/fromRadio/fromNum/logRadio) to the Linux native target, so a Raspberry Pi running meshtasticd can be paired and used over BLE like any other Meshtastic device. Implementation: a new LinuxBluetooth backend registers a GATT application, LE advertisement and pairing agent with bluetoothd over the org.bluez D-Bus APIs, using sdbus-c++ (both the 1.x and 2.x major versions, via a small compat shim - Debian bookworm/Ubuntu 24.04 ship 1.x, trixie/Fedora ship 2.x). When the sdbus-c++ dev package is absent the whole backend compiles out via __has_include, the same optional-dependency idiom as the ulfius webserver. Threading follows the NimbleBluetooth model, simplified: the sdbus event loop runs its own thread, and all PhoneAPI calls happen on the main thread. Writes queue to the main loop; reads park the D-Bus reply and are completed from the main thread after queued writes, so write-then-read clients see their answer without any busy-waiting. Enablement is a double opt-in: a new `Bluetooth:` config.yaml section (Enabled, default false; AdapterId, default hci0) must turn BLE on for the host, and the regular device config bluetooth.enabled must be on. The config-check schema and fixtures cover the new section. Pairing honors config.bluetooth.mode: NO_PIN maps to a NoInputNoOutput just-works agent; RANDOM_PIN to DisplayOnly with the kernel-generated passkey shown on screen/log via the existing BluetoothStatus plumbing. FIXED_PIN falls back to random-passkey semantics with a warning - BlueZ does not support forcing a passkey. PIN modes enforce encrypt-authenticated-read/write on all mesh characteristics. Packaging: install a D-Bus system policy so the meshtasticd user may talk to org.bluez, add it to the bluetooth group, order the unit after bluetooth.service, and add libsdbus-c++-dev to debian/rpm/docker/CI deps. Verified in-container against a mock bluetoothd: registration flow, GATT tree enumeration, advertisement properties, and a full config download (ToRadio wantConfig -> 47 FromRadio packets) through the D-Bus bridge. Real-hardware pairing/notify testing on a Pi still pending. Known limitations (v1): meshtasticd must be restarted if bluetoothd restarts; FIXED_PIN degrades to a random passkey; getRssi() returns 0 (same as nRF52). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): BLE fixes from first real-hardware pass (Pi CM5 + RAK6421) Findings from testing PR #11396 on a Raspberry Pi CM5 (Pi OS trixie, BlueZ/sdbus-c++ 2.1 - the v2 compat path) with a RAK6421 HAT and an Android phone: - getMacAddr() leaked its HCI socket on every call and never closed it, and on failure returned without touching the caller's buffer - which getDeviceName() passed in uninitialized. Close the socket on all paths and cache the MAC after the first successful read; it cannot change at runtime and this now runs on every bluetoothd property read. - getDeviceName() zero-initializes its MAC buffer, and LinuxBluetooth snapshots the name once at setup() on the main thread: the advertisement's LocalName getter runs on the D-Bus event-loop thread and getDeviceName()'s static buffer is not thread-safe. - Restore NimBLE-style config-phase packet prefetch (depth 3). The initial port answered every FromRadio read with a D-Bus -> main-loop round trip, which made the config download noticeably slow; ReadValue now answers straight from the prefetch queue on the event-loop thread, with NimBLE's safety rules (never in STATE_SEND_PACKETS, writes always observed before reads, queue cleared on disconnect). - Set advertising MinInterval/MaxInterval to 20-100ms (BlueZ >= 5.71; older versions ignore the properties). btmon showed the kernel default of 1.28s otherwise, and Android's background-connect scan windows are sparse enough that tap-to-connect took 8-14s; 20ms is the same floor NimBLE uses on ESP32. Verified on hardware: scan, passkey pairing, connect, config download, reconnect after bond wipe. Also diagnosed (no code change): the node identity MAC comes from the RAK HAT EEPROM by design, so the BLE name suffix follows the HAT rather than the BT adapter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): address CodeRabbit review on BLE support - setBluetoothEnable: handle disable before the config gate, so a running BLE stack is always stoppable even after the device config turns Bluetooth off underneath it - getMacAddr: read the adapter configured as Bluetooth.AdapterId instead of hardcoding hci0, falling back to hci0 for unparseable names - systemd unit: Wants=bluetooth.service so bluetoothd is pulled up when present (After= only orders, it does not start it) - debian/rpm: Recommends: bluez as the runtime contract for BLE - dbus policy: document why the org.bluez rule is destination-wide rather than a per-interface allowlist Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): show the BLE pairing code on BaseUI screens onDisplayPasskey published the passkey to bluetoothStatus and triggered PowerFSM, but never called screen->startAlert(), so on BaseUI the code only ever reached the log. BluetoothStatus has no BaseUI consumer -- only InkHUD's PairingApplet and StatusLEDModule read it -- so a Pi driving a HUB75/OLED panel showed nothing while BlueZ sat waiting for the user to type a code they could not see. NimBLE and nRF52 draw it via startAlert(); this adds the missing half for Linux. The agent callbacks run on the sdbus event-loop thread while the screen is owned by the main thread, so the passkey is handed over as a pending flag and drawn from runOnce(), matching the existing disconnectCleanupPending pattern rather than reaching into the screen from the event loop. Dismissed on all four exits, so a stale code cannot stick on an always-on panel: Paired -> true (newly watched in PropertiesChanged, which previously only looked at Connected), agent Cancel, peer disconnect (moved out of the lastGone branch so a peer leaving mid-pairing clears the code even when another device is still connected), and doDeinit() -- applied inline there because runOnce() may never be scheduled again after teardown. Verified on a Pi 5 + BlueZ 5.66 in RANDOM_PIN mode: the code renders on a HUB75 panel and clears once the phone completes pairing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: install libsdbus-c++-dev for the native test build setup-native-test landed on develop while this branch was adding libsdbus-c++-dev to setup-native, so the new action's "full setup-native list" of C libraries is missing it. Without the package the test job builds with HAS_BLUETOOTH 0 and never compiles LinuxBluetooth.cpp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): warn when a factory reset cannot clear BLE bonds factoryReset(eraseBleBonds) silently did nothing on Linux when the BLE backend was not running, so the reset reported success while the host's pairings stayed. Removing them needs a live connection to bluetoothd that a disabled backend never opened, so say so rather than imply they went. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): gate the factory-reset bond clear on an enabled backend setup() leaves linuxBluetooth allocated with its bus torn down when it throws, so a pointer check alone let factoryReset log "Clear bluetooth bonds" for a clear that clearBonds() then declined to perform. isEnabled() is only true after setup() completes, which routes that case to the warning that says the bonds were left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * style: reformat under clang-format 20 #11909 moved trunk from clang-format 16 to 20, which spaces C-style casts differently and reindents the comment above the HAS_WIFI block. Both files are ones this branch already touches, and trunk's fmt linter grades whole files, so its check fails until they are reformatted. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): three BLE config and lifecycle fixes from review Bluetooth config keys are now assigned individually rather than per section. loadConfig() runs once for every file in config.d, so reading an absent key as its default let a later file that named only one of them silently reset the other: `AdapterId: hci1` alone turned Bluetooth off, and `Enabled: true` alone dragged the adapter back to hci0. Only what a file actually states should override what an earlier one set. A backend that failed to come up is now retried. setup() can leave linuxBluetooth non-null but disabled - bluetoothd not ready, adapter missing, policy refusing - and every later enable then called resumeAdvertising(), which returns immediately while disabled. A transient failure at boot kept BLE off until the process restarted. doSetup() already opens with `if (enabled) return` and tears the bus down on every failure path, so calling it again is safe. Bluetooth.AdapterId is now checked for the hci<digits> form. LinuxBluetooth uses the value verbatim as the BlueZ object path while the MAC fallback reads only the leading hciN, so "hci1junk" looks plausible, yields a MAC, then finds no adapter and BLE never comes up. Covered by a new fixture and suite case, which is the kind of silent no-op that directory exists to catalogue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Tom
|
f90b48ea6c |
chore: trunk fmt --all (#11938)
Whitespace and comment-alignment only: the output of `trunk fmt --all` on
develop @
|
||
|
Thomas Göttgens
|
46e009d66d |
fix(portduino): detach the CH341 poll thread when it detaches its own interrupt on Windows (#11882)
* fix(portduino): detach the CH341 poll thread when it detaches its own interrupt on Windows * fix(portduino): stop a superseded CH341 poll thread on Windows * fix(portduino): wait for CH341 poll threads before deinit closes the device * fix(portduino): stop a superseded CH341 poll thread before it rewrites pin state * fix(portduino): keep a same-thread re-arm's CH341 pin state sentinel intact * fix(portduino): close the CH341 attach/deinit race and recognize a superseded poll thread * style(portduino): trim the new CH341 comments to the two-line house limit |
||
|
Jonathan BennettandClaude Opus 5
|
ee02cc3426 |
Games joystick input (#11917)
* fix(games): correct the high-score announcement argument order GAMES_HIGH_SCORE_STRING is "New %s high score %lu by %s!" but the arguments were passed as (name, initials, score): the initials string was formatted through %lu and the score integer through %s. That is a format/argument mismatch, so the announcement printed garbage at best and dereferenced the score as a pointer at worst. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(input): report which physical gamepad button produced an event A joystick event only carried the action it was mapped to, so a consumer could not tell two buttons apart once they shared one action, and games were limited to the handful of actions the broker defines. Carry the originating evdev button code in InputEvent::kbchar, encoded into a reserved 0xC0..0xDF range that misses printable ASCII and every INPUT_BROKER_MSG_ value (SystemCommands switches on kbchar without looking at inputEvent, so a collision there would reboot the node rather than move a paddle). D-pad events are axes, not buttons, and keep leaving kbchar at 0 -- which is exactly what lets a consumer tell stick from button. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(portduino): let one joystick action bind several buttons Input.JoystickButtons took a single evdev code per action, so a pad's A and Y could not both select, and the shoulder buttons could not sit alongside the D-pad. Accept a list of codes as well as a bare scalar; the config writer inverts its code->action map back out, emitting a list only where an action has more than one button. ConfigCheck gains a real checker for the section (it was previously waved through as free-form) covering the three ways a mapping silently does nothing: an action name the driver does not know, an evdev name where the numeric code belongs, and one code claimed by two actions. Two fixtures and shell-test cases cover the clean list form and those three faults. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(games): use the gamepad's extra buttons, and return home when idle Games now receive the physical button alongside the action, so a pad with more than two usable buttons controls more than two things: - Snake: a shoulder button mapped to left/right turns relative to the snake's heading (L counter-clockwise, R clockwise) while the D-pad keeps steering absolutely. The two are told apart by kbchar, not by hardcoding one pad's codes. - Breakout: the ball now rides the paddle after each serve until the player fires it with B or A, so a life is not lost to a ball already in flight when the player looks up. The paddle also keeps its position between lives. A game can claim BACK for the duration (Game::wantsBackButton) so B serves instead of pausing, and releases it once the ball is live. - Start (BTN_BASE4 / BTN_START) is mapped to select like any other button, so it launches games and drives the menus; inside a running game GamesModule picks it out of kbchar and pauses instead. Separately, the games frame no longer holds a walked-away device hostage: after 15 s with no input it returns to the home frame, so the device still reads as a Meshtastic node. The timer is suspended while a picker or banner is up (e.g. high-score initials entry, which the input handler never sees) so it cannot yank the user out mid-entry. Screen::isInteractionBusy() generalises the old module-intercept check -- modal module, intercepting module, game, or an open interactive overlay -- and MessageRenderer uses it before popping an incoming-message banner. A transient banner REPLACES an active overlay, so an arriving message could otherwise discard a half-entered high score. The message is still stored, its thread still selected, and the unread indicator still set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(ui): compose freetext on the on-screen keyboard from a gamepad A gamepad can drive the on-screen keyboard but cannot type, so on a host with a joystick and no configured keyboard device the OSK is the only way to compose freetext. Set osk_found there, and gate the "Freetext" menu entries on whether the device can enter text at all (physical keyboard, OSK, or touchscreen virtual keyboard) rather than on kb_found alone -- those entries were hidden on exactly the devices that needed them. The OSK prompt that CannedMessageModule already had inline in the message selector becomes showOnScreenKeyboard(), so the menu path can reach it too. Menus call in from a banner callback and the banner is torn down as soon as that callback returns, which would take the keyboard down with it, so the menu path defers the launch to runOnce(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(games): address review on frame fallback and joystick input gating Breakout: the paddle suppression was far too broad. aLinuxJoystick is constructed on every Linux host whether or not a gamepad is configured (InputBroker.cpp), so `aLinuxJoystick && kbchar == 0` was true everywhere and swallowed LEFT/RIGHT from the keyboard, trackball and ExpressLRS -- on a host with no joystick attached at all. Gate on the stick actually driving the paddle instead: LinuxJoystick assigns heldX before it emits and only auto-repeats while heldX is set, so every axis LEFT/RIGHT arrives with a zone held and nothing else does. kbchar == 0 still distinguishes an axis from a shoulder button mapped to left/right, which must keep nudging the paddle. Screen: showHomeFrame() did nothing when the home frame was hidden, since setFrames() only assigns positions.home for !hiddenFrames.home. That stranded the games inactivity bounce on the frame it was trying to leave. Fall back to the messages frame, which setFrames() always adds. Test: rename test_ballWaitsOnPaddleUntilLaunched to test_ball_waitsOnPaddleUntilLaunched, matching the repo convention and its neighbours in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(games): give games the whole InputEvent so Breakout can identify the source Follow-up to review on #11917. The previous narrowing still could not tell sources apart: kbchar == 0 is shared by the joystick's D-pad axis and by every other driver that sends a bare LEFT/RIGHT, so while the D-pad was held a keyboard or touchscreen press was still discarded. heldXZone() proves the axis is driving, not that this particular event came from it. Pass the event itself to Game::handleInput() rather than (ev, kbchar). Games that only care about the action read event->inputEvent; Snake keeps using kbchar for shoulder steering; Breakout now also checks event->source against LinuxJoystick's origin name, so only that driver's own axis repeats are suppressed. Chose the event over a third positional parameter so the signature does not have to grow again the next time a game needs something the event already carries. All three conditions in Breakout are load-bearing: source says it came from this gamepad, kbchar == 0 says it is the axis rather than a shoulder button mapped to left/right, and heldXZone() != 0 says the axis is what is driving right now so tick() already has it covered. LinuxJoystick::originName() exposes the name the driver stamps into InputEvent::source, alongside the existing heldXZone()/heldYZone() accessors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom
|
2d6dad9ee9 |
Portduino: Fix LR2021 switch tables, power ceilings and IRQ handling (#11382)
* fix(portduino): recognise the LR2021 power ceilings in --check loadConfig() has read Lora.LR2021_MAX_POWER and Lora.LR2021_MAX_POWER_HF since LR2021 support landed, but neither was listed in the config checker's schema. --check therefore reported both as "unknown key ... ignored by meshtasticd" -- false, and actively misleading: it tells the user to delete a key that is doing exactly what they wanted. This breaks the contract stated above schema(), that a key taught to loadConfig() is added there too. CI enforces that by running --check over bin/config.d/**, but no shipped config sets either key -- or mentions lr2021 at all -- so nothing ever tripped over the omission. It could only surface for someone hand-writing an LR2021 config. LR20x0 is the only module with two power ceilings, one per band, selected at runtime by region; every other family expresses the split as separate module names and needs a single key. That is the likely reason the pair was missed while every other *_MAX_POWER key was added. Also adds both to valueSpecs(), so a wrong-typed value is reported rather than silently replaced by the default. * feat(portduino): configurable IRQ DIO and a chip-neutral RF switch table for LR20x0 Two gaps found bringing an LR2021 up under meshtasticd on a Luckfox Lyra Zero W. Both sit in the LR20x0 support added in #11252, and they interact: the switch table has to be written slightly wrong to pass validation, and the interrupt lands on a pin that table is driving. Every symptom is silent, because begin() only exercises SPI and BUSY -- the radio reports init success and then receives nothing. IRQ DIO could not be set on Portduino ------------------------------------- LR20x0Interface picked the IRQ DIO purely at compile time, and neither LR2021_IRQ_DIO_NUM nor IRQ_DIO_NUM exists for a Portduino target, so meshtasticd always fell through to RadioLib's default of DIO5 -- which is also the first RF switch line on carriers using the DIO5-DIO8 table. A variant says this with a #define (the pro-micro DIY board uses DIO9); a carrier has only the YAML, and had no way to say it. Adds Lora.IRQ_DIO_NUM, and an ARCH_PORTDUINO branch after the two existing #define branches, so a variant that already sets one still wins. The switch table was parsed as LR11xx-only ------------------------------------------ Pin names resolved to RADIOLIB_LR11X0_DIOn whatever the radio, and the mode set was the LR11xx's, so MODE_RX_HF -- a mode the LR20x0 really has -- was rejected as an unknown key and had to be omitted. The two families are not interchangeable: an LR11xx has no DIO9, so its fifth switch slot is DIO10, while an LR20x0's fifth slot is DIO9 and DIO10 is its sixth. A table naming DIO10 was therefore driving the wrong pin on an LR20x0. The YAML layer now stores what was written -- a DIO number and a neutral mode id -- and each interface supplies its own DIO constants and OpMode_t map to a shared builder. Neither family's constants are assumed to coincide with the other's. This also fixes a round trip in the config writer, which decoded pins by comparing against RADIOLIB_LR11X0_* and always emitted five values per mode row: for a four-pin table it produced YAML that --check would reject for mismatched row lengths. --check ------- Findings are now judged against the resolved module rather than a fixed list, so a mode or pin the part does have can no longer be rejected, and one it does not have is named instead of silently accepted. The claim that the table "is only applied to LR11xx radios" was stale and is corrected, and the missing-table warning now covers both families. "auto" is excluded throughout: the module has not been probed yet, so absence cannot be judged. The IRQ/switch-pin collision is reported in both directions, including the harder case where no key is set and the radio default collides -- nothing in the file looks wrong. Note that listing a pin is what breaks it, not driving it: setRfSwitchTable() reassigns the DIO function for every pin in the list whatever the levels say, so an all-LOW column is still a collision. Seven fixtures cover these, including a false-positive guard: DIO5 as the interrupt is normal, and must stay silent when the table is elsewhere. * feat(portduino): let the YAML ask for a TCXO probe, across every family that has one A variant declares "a TCXO may or may not be fitted" at compile time with TCXO_OPTIONAL, because the board is known when the image is built. A Portduino carrier cannot: the same meshtasticd binary runs on hardware populated either way, so the statement has to arrive as YAML and be answered at runtime. Adds Lora.TCXO_OPTIONAL, and TCXO_OPTIONAL_ENABLED in RadioLibInterface.h to unify the two, so each driver asks the question once rather than growing a second, Portduino-shaped code path. On an embedded target it stays a compile-time constant, so `if (TCXO_OPTIONAL_ENABLED)` folds away exactly as the old `#if` did: the nrf52_promicro_diy_tcxo image, which defines TCXO_OPTIONAL and so exercises the converted branches, still ends at 0xDF1D0 -- the same address as before this change. It is defined there rather than in a header of its own because InterfacesTemplates.cpp includes all three interface .cpp files into one translation unit, where a per-file definition would collide. Covers every family that has a TCXO reference to probe for: SX126x (sx1262/sx1268/LLCC68), LR11xx and LR20x0. With no DIO3_TCXO_VOLTAGE given, the TCXO attempt uses RadioLib's own 1.6 V default rather than being skipped -- otherwise there is nothing to fall back FROM and the flag would silently do nothing. This is also the FIXME that sat on the Portduino branch in LR20x0Interface: an unset voltage now means "no TCXO" explicitly. Two things are deliberately left alone: Each family keeps its own probe order. LR11xx tries XTAL first, because a TCXO-first attempt hangs RadioLib's unbounded calibration wait on a module with no TCXO fitted, whereas XTAL fails fast and cleanly on a module that has one; LR20x0 and SX126x try the TCXO first. A carrier therefore behaves the same way in a Portduino build as in an embedded one, and changing an order stays a hardware-behaviour decision rather than a tidying-up one. The SX126x retry is Portduino-only. An embedded TCXO_OPTIONAL board already gets this from initLoRa(), which constructs a second SX126x interface with no Vref when the first fails; retrying inside init() as well would leave that ladder step unreachable and change how every existing t-echo-class board reports its oscillator. A Portduino build has no ladder to fall through, because the module is named in YAML rather than probed. --check learns the key, reports which Vref will actually be tried, and warns when it is set on a radio with no TCXO reference, where it is read, stored and inert. * docs(portduino): condense the comments on this branch The repo asks for one or two lines and no multi-paragraph blocks, on the grounds that the diff and the commit message carry the rationale while the code carries the behaviour. What landed here was well past that: 163 added comment lines, including a 26-line block above a single macro. Removes the rhetoric, the issue numbers and the before-and-after asides, and the notes on where a thing used to live. No added block is longer than three lines now. Two facts needed stating and are stated once each rather than repeated at every use: the slot/DIO divergence between the families, in PortduinoGlue.h, and the per-family TCXO probe order, in RadioLibInterface.h. The longest surviving explanation is why an all-LOW switch column still collides with the interrupt, which sits in the fixtures README because without it that pair of fixtures reads as contradictory. Comments only; no functional change. * address CodeRabbit review on #11382 - SX126xInterface: distinguish an explicit DIO3_TCXO_VOLTAGE from the TCXO_OPTIONAL probing default in the debug log instead of always claiming the config field was set. - ConfigCheck: modesFor() now reports an unresolved use_autoconf against the union of both radio families' modes, not the LR11xx subset - fixes a false "not a mode this part has" warning for valid LR2021-only modes (e.g. RFSW_RX_HF) before autodetection resolves the module. - PortduinoGlue loadConfig: build rfswitch_mode_high[m] as a fresh per-row bitmask instead of OR-accumulating onto a stale value, so a config re-parse can clear a slot back to LOW. - PortduinoGlue YAML serialization: gate rfswitch_table emission on has_rfswitch_table rather than rfswitch_dio_num[0] >= 0 (missed sparse pin lists), and track each emitted pin's original slot so row values line up correctly instead of shifting when a low slot is absent. - config-dist.yaml: document the per-family TCXO/XTAL probe order (SX126x/LR20x0 TCXO-first, LR11x0 XTAL-first). - Trim three overlong comments per the coding-guideline nitpicks. Left the SX126x XTAL-retry-on-oscillator-failure nitpick alone - RadioLib's begin() already does its own XOSC_START_ERR recovery internally, and narrowing our wrapper's retry condition on top of that needs hardware to verify it doesn't regress a real failure path. Verified: bin/test-config-check.sh GREEN 69/69 against an isolated native build; pio test -e native -f test_rtc PASSED. * fix CI: cppcheck duplicateValueTernary, harden kRfSwitchModes init LR11x0Interface::init(): work around cppcheck's duplicateValueTernary on `TCXO_OPTIONAL_ENABLED ? 0 : tcxoVoltage` (both branches fold to 0 on a board with no ARCH_PORTDUINO, no TCXO_OPTIONAL, and no explicit Vref, since tcxoVoltage already reduces to 0 via the same macro chain) by splitting it into a plain assignment + if, rather than suppressing the warning. The other TCXO_OPTIONAL_ENABLED ternaries in this PR (LR11x0Interface.cpp:75, SX126xInterface.cpp:76, LR20x0Interface.cpp: 86,240) pick between TCXO_OPTIONAL_DEFAULT_VOLTAGE (1.6f) and 0, which can never coincide, so they're unaffected and left as-is. ConfigCheck.cpp: kRfSwitchModes was a namespace-scope global with dynamic initialization (a lambda IIFE) reading kRfSwitchModeNames, which is defined in a different translation unit (PortduinoGlue.cpp). Currently safe only because kRfSwitchModeNames's initializer is constant-expression-only (string literals + enum constants), which the standard guarantees completes before any TU's dynamic initializers - but that safety is silent and would break if PortduinoGlue.cpp's array initializer ever stopped being a constant expression, with nothing to warn a future editor. Converted to a function-local static (Meyers' singleton), which is correct by construction regardless of the other TU's initializer, updating all 4 call sites (definition + 3 uses) from kRfSwitchModes to kRfSwitchModes(). Verified: pio test -e native -f test_radio PASSED; bin/test-config- check.sh GREEN 69/69 against an isolated native build. * refactor: simplify TCXO voltage handling across interfaces and improve comments * fix rfswitch_table cross-file merge; drop now-stale checker warning Three CodeRabbit findings on 09e0d390c, addressed together since #2 and #3 are the same root cause: 1. PortduinoGlue.cpp: require an exact "DIO<n>" match when parsing rfswitch_table.pins. sscanf's %d stops at the first non-digit, so "DIO5invalid" silently parsed as DIO5 at runtime even though ConfigCheck.cpp's static validator (exact match against kRfSwitchPins) already rejected it - checker and loader disagreed. 2. PortduinoGlue.cpp: reset all 5 pin slots and all 8 mode rows before applying a table, rather than only overwriting what the new table mentions. A later config.d file that omitted a mode a prior file had set (e.g. only redefining MODE_TX) let the earlier file's MODE_RX leak through, contradicting "last file wins" - the rule every other Lora: key already follows. 3. ConfigCheck.cpp: with #2 fixed, rfswitch_table behaves like any other cross-file key, so removed the special-cased ERROR in checkCrossFileOverlap ("These do NOT override each other... OR of every table") - it described the pre-fix OR-accumulation bug and is no longer accurate. Falls through to the generic "last file wins" INFO now. Renamed/repurposed the rfswitch-sticky fixture to rfswitch-last-wins and updated its assertion (was rc=1 asserting the old error text, now rc=0 asserting the generic info) and the fixtures README. Verified: bin/test-config-check.sh GREEN 69/69 against an isolated native build, including the renamed assertion. * Assert the effective rfswitch table, not just the overlap diagnostic The "last one wins" case checked that the cross-file info fires and that the result is clean. Neither observes the table the loader actually ended up with, so the merge bug ee9b5b81e fixed - a later table leaving an earlier file's pins and mode rows as carryover - would still have passed it. Raised by CodeRabbit. Asserting the value needs two things the existing case cannot supply. The winner has to be deterministic. Both files in rfswitch-last-wins/ sit in config.d/, which is walked with a bare directory_iterator and no sort, so which one lands last is up to the filesystem - the point configd-conflict/ exists to make, and the reason the checker warns rather than assuming alphabetical order. rfswitch-replace/ puts the losing table in config.yaml instead, which is always loaded before config.d/. The loser is the wider of the two, four pins and three all-HIGH mode rows against the winner's two pins and one all-LOW row, so carryover shows up as a surviving pin, a surviving mode row, or a HIGH that should be LOW. The table has to be observable. The check report says no more than "RF switch table : set", and check-yaml cannot help: it is --check --output-yaml, and --check wins and exits before the dump - which the case just below it asserts. emit_yaml() does serialise the effective table, so the assert helper grows a yaml mode that passes --output-yaml alone. Confirmed non-vacuous: with the reset loop in loadConfig() removed, the new assertion fails and the old one still passes. Config-check suite GREEN 70/70. Native suite GREEN 44/44, 968 cases. * Say nothing about rows the radio will never read Two of the RF-switch diagnostics judged a table against a family's mode list without first asking whether the module reads a table at all. modesFor() treated every module that was not an LR20x0 as LR11xx-like, so an sx1262 carrying a table was told which of its rows were "not a mode sx1262 has" and which modes it had omitted - alongside the correct warning that the whole table is inert. pinsFor() already returned an empty set for these parts and its caller already guarded on that; the mode path now matches. The missing-mode advice is dropped under "auto" as well. The module has not been probed, so the union of both families is all there is to compare against, and naming its absent modes would advise adding MODE_TX_HP, MODE_GNSS and MODE_WIFI rows to what may turn out to be an LR20x0. Fixtures for both silences, and an assert() needle prefixed with '!' to hold them: a line that is merely absent today is otherwise nobody's regression. Also corrects the kLr11x0SwitchDios/kLr20x0SwitchDios comments. They describe a slot mapping, but buildRfSwitchTable() searches them by value to find the parallel pin constant - and with 7 DIOs against 5 YAML pin slots, the LR20x0 array could not be positional. * Warn when the two TCXO keys ask for opposite things DIO3_TCXO_VOLTAGE written out as false or 0 asks for DIO3 to be left alone, and stores identically to the key being absent - so TCXO_OPTIONAL then probes DIO3 at the radio default anyway. Both keys behave exactly as documented; only together are they wrong, which is what makes the outcome surprising. loadConfig() now keeps the distinction that the store loses, and --check reports the contradiction and which key to drop. The flag is diagnostic only and is not serialized: an explicit false and an absent key both round-trip as absent, as they did before. Also fixes the Portduino TCXO log lines, which named the variant define SX126X_DIO3_TCXO_VOLTAGE on a path where the knob is the YAML key, and adds a TODO over the SX126x XTAL retry. RadioLib has autocorrected that case itself since 7.5.0 - SX126x::modSetup() retries config() on the XTAL when begin() fails with SPI_CMD_FAILED and XOSC_START_ERR - so the ordinary case never reaches our retry and what does is mostly invalid settings, logged as a TCXO fault. * feat(portduino): bound Lora.IRQ_DIO_NUM, accept the older spelling, document both An LR20x0 raises its interrupt on DIO5 through DIO11. Anything else was read straight out of the YAML and programmed into RadioLib, where it routes the IRQ nowhere: begin() touches only SPI and BUSY, so the radio reports init success and then never receives a packet - the same failure the switch-pin collision check already covers, reached by a typo instead. Refuse it in loadConfig() and again in the driver before it reaches RadioLib, warning both times. Because loadConfig() discards the value, the merged config cannot tell a rejected number from an absent key, so --check judges the range in its per-file pass where the offending line is still known. LR2021_IRQ_DIO_NUM, the spelling carried by the two earlier LR2021 branches, is read when IRQ_DIO_NUM is absent and reported as shadowed when it is not. Both keys, the DIO range and the collision that makes the setting matter are now described in config-dist.yaml. * fix(portduino): a rejected IRQ_DIO_NUM returns to the radio default loadConfig() runs once per file - the main config, then each file in config.d/ - and they all write the same portduino_config. An out-of-range Lora.IRQ_DIO_NUM warned that it was falling back to the radio default but left any valid value an earlier file had set, so LR20x0Interface went on programming that stale DIO. Reset it to -1, the unset sentinel every other reader already tests for. Pinned by a new fixture: DIO9 in the main config, out of range in config.d/. The main config is always read first, so the ordering is deterministic, unlike two files in config.d/ (see rfswitch-last-wins). The assertion requires the summary to name the radio default and not DIO9; with the reset removed and rebuilt, it is the only assertion that fails. |
||
|
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> |
||
|
|
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>
|
||
|
Clive BlackledgeandClaude Opus 5
|
389559bddb |
fix(NodeDB): re-derive my_node_num when ensurePkiKeys() mints the identity keypair (#11426)
* fix(pki): re-derive NodeNum when setting a region mints the identity key
A node's mesh address is derived from its identity key:
my_node_num == crc32Buffer(config.security.public_key.bytes, 32)
NodeDB::createNewIdentity() is what establishes that, and NodeDB::
generateCryptoKeyPair() is the only thing that called it.
CryptoEngine::ensurePkiKeys() generates or re-derives the keypair and writes
security.public_key, security.private_key and user.public_key - but never
re-derives my_node_num. Boot-time keygen is suppressed while the LoRa region is
UNSET (generateCryptoKeyPair()'s regionBlocksKeygen guard), so on a fresh device
my_node_num is still the MAC-derived value from pickNewNodeNum(). The user then
sets the region - the stock onboarding flow - ensurePkiKeys() mints a key, and
the invariant is broken.
The node then signs its broadcasts (Router.cpp signs when !pki_encrypted &&
(owner.is_licensed || isBroadcast(p->to))). Every receiver runs
verifyFirstContactNodeInfo, fails crc32Buffer(user.public_key) != p->from, and
drops the NodeInfo. The node's identity beacons are invisible to the mesh.
Nothing reboots to repair it: AdminModule sets requiresReboot = false for LoRa
changes ("All LoRa radio changes apply live via configChanged observer") and
MenuHandler ends at service->reloadConfig(changes).
Four call sites reached ensurePkiKeys():
1. AdminModule set_config LORA, region first set (phone app - the common path)
2. MenuHandler applyLoraRegion (on-device region picker)
3. InkHUD MenuApplet applyLoRaRegion (schedules a reboot, so it
self-healed at next boot)
4. portduino wasm wasm_set_region
The reference implementation was already in the tree: the *licensed* branch of
call site 1, thirteen lines below the broken unlicensed one, calls
nodeDB->generateCryptoKeyPair() (which reaches createNewIdentity()) and widens
the persisted mask with SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE.
Rather than repeat that at four call sites, the key-mint is routed through one
chokepoint that owns both halves of the identity: NodeDB::ensurePkiIdentity()
calls crypto->ensurePkiKeys() and then createNewIdentity(). It lives in NodeDB
because createNewIdentity() operates on the devicestate/node-DB globals, which
CryptoEngine deliberately does not touch - ensurePkiKeys() takes the security
config and user by reference precisely so it stays free of that dependency, and
it is unit-tested against a standalone CryptoEngine.
ensurePkiIdentity() returns true only when my_node_num actually moved
(createNewIdentity() early-returns when the key is unchanged, so a repeat region
change does not disturb the self entry or force a needless flash write). Callers
use that to widen their save mask; my_node_num lives in devicestate and the self
row moves in the node DB, so both segments must be persisted or the fix would
revert at the next boot. SEGMENT_CONFIG, which carries the key itself, is
already unconditional on all four paths.
The InkHUD reboot is left as-is. It is now redundant for this invariant, but it
covers the rest of that menu's behaviour and a redundant reboot is not a bug.
Adds test_handleSetConfig_persistsUnlicensedFirstRegionIdentity, the unlicensed
twin of the existing licensed test, asserting both the segment mask and
my_node_num == crc32(public_key).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(NodeDB): trim identity-recovery comments and guard the WASM nodeDB deref
Two review asks, no behaviour change on any built target.
Copilot flagged the unguarded nodeDB deref in the WASM region setter; it is the
only ensurePkiIdentity() call site that did not check the pointer first.
The rest is comment length. AGENTS.md:83 caps code comments at two lines, and the
identity-recovery comments across the four call sites plus the NodeDB.h doc block
ran to four and six lines. The rationale they carried is in the commit messages
and the PR body, which is where AGENTS.md says it belongs.
The PR's own fix in AdminModule.cpp is deliberately untouched.
* fix(NodeDB): keep the identity move authoritative when the self record cannot be created
createNewIdentity() removes the old node entry and assigns myNodeInfo.my_node_num
before it tries to create the row for the new number. If getOrCreateMeshNode()
came back null it returned false, so the first-region callers left
SEGMENT_DEVICESTATE and SEGMENT_NODEDATABASE out of the save mask.
The number had already moved in RAM at that point, and the freshly minted key
goes to flash under SEGMENT_CONFIG regardless. The next boot therefore reloads
the old number alongside the new key, which is exactly the
crc32(public_key) != my_node_num break this path exists to prevent, reached
through the error branch instead of the happy one.
Rolling the number back is not an option either, since the key has already been
replaced by the time this runs. So the move is now reported as the fact it is and
the missing self record is logged separately; getOrCreateMeshNode() will recreate
that row on the next contact. Reachable when the self record is absent and the
table is full of protected nodes.
Reported by CodeRabbit on #11426.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Ben Meadors
|
0ff10318ad |
refactor(net): unique_ptr for connection-lifecycle objects (#11459)
- 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. |
||
|
Ben Meadors
|
22079ca863 |
fix(platform): OOM null-write in stm32wl File and exception leak in portduino GPIO init (#11456)
- STM32_LittleFS File::_open_dir: the _dir_path allocation was the only unchecked malloc in the file, followed immediately by strcpy - an OOM became a NULL write, and the half-initialized state (open dir, null path) would later feed strlen(NULL) in openNextFile(). Check it and unwind the already-opened dir, matching the sibling failure path. - PortduinoGlue initGPIOPin: if setSilent()/gpioBind() threw after the LinuxGPIOPin was constructed, the pointer was lost in the catch block. Hold it in a unique_ptr and release only after the gpio table takes ownership. |
||
|
Jonathan BennettandClaude Fable 5
|
5baad2e2a8 |
logging: compile out LOG_TRACE by default, demote chatty DEBUG lines, drop redundant logs (#11391)
* logging: gate LOG_TRACE behind MESHTASTIC_TRACE_LOGGING, drop redundant reclock logs LOG_TRACE now compiles out by default so trace-level diagnostics cost no flash; enable with -DMESHTASTIC_TRACE_LOGGING. Portduino keeps it on for the traceFilename packet-trace feature. Remove the 66 caller-side I2C reclock/restore log lines in the telemetry sensors: ReClockI2C::setClock/restoreClock already log both frequencies internally (now at trace level, since they fire every sensor read). Also unify near-duplicate literals (colon/case/punctuation variants) so linker string dedup applies, and drop an information-free bare 'done'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: demote chatty per-packet/per-poll DEBUG lines to trace level With LOG_TRACE compiled out by default, per-iteration chatter (packet bookkeeping, sensor poll values, e-ink refresh reasons, GPS pin states, UI runState traces) now costs no flash on device builds while remaining one -DMESHTASTIC_TRACE_LOGGING away. 108 lines demoted, 4 information- free lines removed; failure paths, drop reasons, and one-time init logs all stay at debug level. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: address CodeRabbit review on trace-gate PR - GPS: pass serial-derived buffers as %s args, never as format strings (untrusted bytes could contain % directives) - 0x%08x for packet id / NodeNum per convention (Router, CannedMessage, NeighborInfo); unsigned casts for size_t args; %u for uint32_t delta - EInk: async full-refresh begin/complete back to DEBUG (rare state transitions); per-frame SKIPPED lines stay trace Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: gate trace on the flag's value, not its presence -DMESHTASTIC_TRACE_LOGGING=0 previously *enabled* trace logging because the gate tested definedness. The flag now defaults per-platform (portduino 1, else 0) and both backends test the value, so =0 disables, =1 or a bare -D enables. Also cast tx_after-millis() to uint32_t for %u (millis() is unsigned long on native). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: clang-format rewrap after specifier widening Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * Even fewer bytes! * logging: keep compile-gated debug lines at debug level; fix native-suite-count Lines already inside default-off #ifdef blocks (GPS_DEBUG, DEBUG_LOOP_TIMING) cost no flash and should stay visible at debug level when their gate is enabled, rather than also requiring MESHTASTIC_TRACE_LOGGING. test/native-suite-count lags the two test_event_channel_* suites added by #11045 (develop's Native Suite Count check has the same mismatch); bump 46 -> 47. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * gps: route GPS_DEBUG diagnostics through a LOG_DEBUG_GPS() macro (#11414) Replaces 27 log-only #ifdef GPS_DEBUG blocks across GPS.cpp, PositionModule, MeshService, and GPSStatus.h with a single-line LOG_DEBUG_GPS() call (src/gps/GPSLog.h, modeled on LOG_MIGRATION: value-gated, ((void)0) when off). Blocks containing declarations, control flow, hexDump, or nested conditionals keep an explicit '#if GPS_DEBUG' guard. RTC.cpp's per-reading raw time dumps and per-candidate rejection chatter fold under the same gate; quality transitions and boot-time seeding stay at debug. Also fixes the '// define GPS_DEBUG' missing-# typo in two variant headers and updates all seven commented examples to the value form ('#define GPS_DEBUG 1') required by the value-based gate. Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 Co-authored-by: Claude <noreply@anthropic.com> * gps: declare RTC gmtime result as pointer to const (cppcheck) With the setTime debug dump gated behind GPS_DEBUG, all remaining uses of t are reads; cppcheck (constVariablePointer) now flags it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
cd716fe384 |
logging: audit log strings for terseness, reclaiming ~6.8 KB of string data (#11374)
* 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> |
||
|
Tom
|
de6b23190a |
Test suite rebuild (#11322)
* docs(nodedb): make the native node cap unambiguous The native node cap was stated in four places that disagreed, and the disagreement already caused a wrong diagnosis: a saturated 200-node database looked arithmetically impossible because the cap had been read as 248, computed from a header that does not apply on this platform. The real value is 198. On portduino MAX_NUM_NODES is not a compile-time constant at all - the variant defines it as `portduino_config.MaxNodes`, resolved at runtime, default 200 and settable per host with `General: MaxNodes`. variant.h is reached before mesh-pb-constants.h, so that header's ARCH_PORTDUINO branch never fires and its plausible-looking 250 is dead code. - #error-guard the dead branch rather than leave a wrong number where people grep. The guard found a real defect: seven translation units reach mesh-pb-constants.h without configuration.h (SerialConsole.cpp, StreamAPI.cpp, PacketAPI.cpp, ServerAPI.cpp, PiWebServer.cpp, ServiceEnvelope.cpp, MeshtasticOTA.cpp, and test/TestUtil.cpp), so each was compiling with a different MAX_NUM_NODES - and therefore a different PACKETHISTORY_MAX - than the rest of the build. Each now includes configuration.h first. It cannot be included from mesh-pb-constants.h itself: that reaches SerialConsole.h through DebugConfiguration.h and closes a cycle. - Name the bare 250 in getMaxNodesAllocatedSize() NODEDB_MIGRATION_LOAD_CEILING. It is a decode allowance for files written by larger-cap firmware, not a cap, and it read like one. - Fix docs/node_info_stores.md, which named the wrong source and a "10-250" range that is wrong for native, and the copilot-instructions tunables line that said "portduino 250". * test(harness): give each suite its own scratch HOME and report leftovers Native suites shared one directory. Every suite that constructs a NodeDB loads and saves ~/.portduino/default/prefs/ - nodes.proto, config.proto, channels.proto, module.proto, device.proto, warm.dat, transmit_history.dat - and nothing cleared it, so state leaked suite -> suite within a run and run -> every run after it. A test run could also rewrite a real meshtasticd node database on the same machine. Per-run isolation does not fix this: the leak is generated inside a single run, so the boundary has to be per suite. bin/pio-test-isolate.sh runs each suite in its own scratch $HOME, registered as test_testing_command for env:native and env:coverage so a bare `pio test` and CI get the same boundary, not just bin/run-tests.sh. It runs the binary unchanged and exits with its exit code, so PlatformIO's pass/fail is untouched. Overriding HOME here rather than around `pio` also sidesteps the blocker that a bare HOME= breaks pio's own ~/.platformio/penv/bin/pio lookup. Leftovers are reported as a second axis, PASS/FAIL x CLEAN/DIRTY, because an unintended write has no matching assertion by definition - nobody writes TEST_ASSERT for a save they do not know is happening. The harness asserts it from outside, so it applies to every suite without the author opting in. - Only the *set of changed paths* is asserted, never contents. Hashes answer the boolean "did this change?" and nothing more; content baselines over protobuf bytes would churn on every NodeInfoLite field added, which is how snapshot suites become noise. - Deliberate writes are declared in test/state-manifest.tsv - one central file, suite / flags / mandatory reason. run-tests.sh prints the opt-out count on every run. - Granularity follows the state flag, so the two ship together: per-test by default (TestUtil redefines RUN_TEST to checkpoint after each test, naming the exact test that dirtied things), suite boundary for state=per-suite, where carrying state across test cases is the declared behaviour. - A declared write that does NOT happen is reported as MISSING, not folded into DIRTY. It catches silently broken persistence; a warning for now, since some are conditional. - Graded AMBER, not RED. With isolation in place DIRTY means "undeclared", not "dangerous", and a check that lands red on day one gets switched off. Guard the guard, both halves: state_assert_empty() refuses to run a suite against a sandbox that is not empty (otherwise the after-diff measures against the wrong baseline and reports CLEAN while meaning nothing), and bin/test-state-check.sh drives the real wrapper with fixtures asserting CLEAN / CLEAN / DIRTY / MISSING plus both directions of the empty assertion. A checker that silently matches everything would otherwise pass forever. --write-manifest proposes entries for a human to paste and justify; it never applies them, and neither does CI. * test(harness): stop reporting Unity's exit code as a signal A native suite ends in exit(UNITY_END()), and UNITY_END() returns the failure count. PlatformIO's native runner reads that non-zero exit code as a POSIX signal number, so four failures print "Program received signal SIGILL", five print "SIGTRAP", and the suite is classified [ERRORED] rather than [FAILED]. There is no crash. The signal name tracks the failure count and nothing else - it moved SIGILL -> SIGTRAP when a diagnostic probe added a fifth failure - and it cost hours of hunting a memory bug that did not exist, on an env (native) that carries no sanitizer at all. It also explains the phantom extra test case in the totals: the runner adds a synthetic entry for the signal it thinks it saw. run-tests.sh now says so inline whenever a signal line appears, and the three agent-facing docs say it too. * test(admin): isolate NodeDB and globals per test setUp() did `if (!nodeDB) nodeDB = new NodeDB();` and never deleted it, so 83 of the 85 tests shared one never-reset database and never restored config, owner, devicestate or channelFile. The fixture that does restore them was opt-in and armed by exactly two tests. The setUp comment claiming the rest "set their own config/region state and are unaffected" was not true - the admin handlers under test write all four globals. Route every test through the fixture instead: setUp saves the globals and installs a fresh NodeDB, tearDown restores and deletes it. The two tests that armed it themselves no longer need to. All 85 pass, so nothing was silently relying on the shared state. It costs about 7% of the suite's runtime (a NodeDB construction is a loadFromDisk plus, with a region set, key generation) - worth paying to write the phase 3 tests against a clean fixture rather than 83 tests' residue. Also cap the per-test attribution in the run summary at five entries; the full list stays in the suite's sandbox. * test(fs): cover the bounded file-manifest walk getFiles() runs on every phone sync via STATE_SEND_FILEMANIFEST, and nothing asserted any of its bounding behaviour. It does execute unasserted from test_stream_api's handshakes, but the cap, the depth limit, the wasLimited paths, overlong-path rejection and capacity release were all unguarded. Eight tests, all describing what the code does today: today's code is already correct here, since #10778 landed the by-reference collectFiles(), the 64-entry cap, the strlcpy bounds and the swap-idiom release. They pass on arrival, which is the point - this is the baseline a later change has to leave alone. Two things they do not cover, and cannot: - Moving reserve() outside the __cpp_exceptions guard. Exceptions are on natively, so the #else branch is not compiled. The suite's job there is to prove that change alters nothing observable. - The file.name() null guard. No in-tree backend returns null; the guard is defensive. The manifest-release test pins the swap idiom rather than calling PhoneAPI's releaseFilesManifest(), which is file-local. It asserts capacity() == 0, not just size() == 0 - a size-only check passes on clear(), which is the bug #7924 shipped. Suite count 43 -> 44, recounted against the directories rather than copied. * test(admin): assert node-DB metadata saves skip the radio reload set_favorite_node, set_ignored_node and toggle_muted_node each persist a NodeInfoLite bit and nothing else. MeshService::reloadConfig() gates its region re-derivation and configChanged notification on saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS), so a SEGMENT_NODEDATABASE-only save already skips the live radio reconfigure. Pure characterization - all three pass on develop. Worth pinning because that reconfigure is the path implicated in the WisMesh Tag favourite-node crash, and develop asserts nothing about it: widening the saveWhat mask or reordering the check would currently go unnoticed. Ported from the config-save series along with ConfigChangedCounter (an Observer<void *> counting configChanged notifications, the only externally visible signal that the reload branch was taken) and TEST_NODE_NUM. They join the existing suite, so no suite-count change. * refactor(menu): extract the mute toggle into a named function The node menu's mute action was inline in a banner-callback lambda, and that lambda only ever runs via screen->showOverlayBanner() - which is why nothing in MenuHandler.cpp was reachable from a test. Lift the `selected == Mute` branch into menuHandler::toggleNodeMuted(uint32_t) and call it from the lambda. Behaviour-neutral by construction: same statements, same order, same bare saveToDisk(). The null check moves into the function, so the call site no longer needs its own lookup. Verified by the native build and suite; the byte-identical-image check on a headroom-constrained nRF52 board was not run locally - CI's firmware-size comment covers it. Three tests come with it, all describing today's behaviour: - the bit flips both ways and no configChanged fires (develop never calls reloadConfig on this path); - an unknown node is a no-op rather than a write; - and the segment mask. Flipping one NodeInfoLite bit currently rewrites all five segments via bare saveToDisk(). That is asserted deliberately, with the comment naming it as characterization of a known defect: a pending fix narrows it to SEGMENT_NODEDATABASE, and when it lands this assertion is expected to change, which makes the improvement visible in the diff instead of silent. saveToDisk() is not virtual, so the mask is observed through its effect - remove the five prefs files, toggle, and see which reappear. * docs(test): make every suite count a pointer to the canonical one test/native-suite-count is the registered total and is machine-checked against test/test_* on every full run and by the suite-count-check CI job. Every other statement of the count is a copy that drifts: copilot-instructions said 12, AGENTS.md said 19, and the real number is 44. Replace both literals with a pointer to the file, say explicitly that no document should state the count as a literal, and reframe the two suite listings as descriptions rather than inventories - they carry per-suite information the count does not, so they stay, but nothing should infer completeness from their length. Register the new FS suite in both. * test(harness): randomise suite order, reproducibly Landed last, deliberately. Randomising an order-dependent suite set does not find bugs so much as convert a silent pass into intermittent red, and the first instinct is to revert the randomisation rather than fix the coupling. Phases 1-2 removed the coupling; this keeps it removed. Both runners previously hid order dependence behind a fixed order that happened to differ between them, and neither order was chosen: CI's area rules put admin first, PlatformIO's local discovery is reverse alphabetical and put it last. CI was green by accident. - bin/run-tests.sh --shuffle / --seed <n>. The seed defaults to HEAD's short SHA: one order per commit, so a red is replayable and attributable to the diff instead of flaky, while the project keeps exploring orders. Printed at the start and carried into the RESULT line, so a verdict is replayable from that line alone; the full order is printed on failure, because for an order-dependent failure the order is the diagnostic. - The shuffle is a Fisher-Yates over a MINSTD generator rather than awk's rand(), whose sequence differs between gawk and mawk. A seed that does not reproduce the same order on another machine is not a seed. - Shuffling needs one `pio test -f <suite>` invocation per suite - PlatformIO orders by its own os.walk() over test/ and filters only select - which measures at about 4.7s per suite of extra startup. - CI shuffles its area order, seeded from GITHUB_SHA and printed with the command to replay it locally. Intra-area order stays PlatformIO's; controlling it there would mean per-suite invocations, which is a cost worth deciding separately. Also records the 16 measured entries in test/state-manifest.tsv, each with its reason, taken from a full run's --write-manifest output rather than guessed. * test(default): cover the region-throttle interval overload getConfiguredOrDefaultMsScaled(configured, default, nodes, TrafficType) is the overload every telemetry and position module actually calls, and nothing referenced TrafficType anywhere under test/. All four of its behaviours were unguarded: the no-region guard, the throttle <= 1 short-circuit, the multiply, and the 64-bit overflow clamp. The throttles are real, not hypothetical - EU_866 carries PROFILE_LITE, which sets both positionThrottle and telemetryThrottle to 10, so a change here moves broadcast spacing in that region by an order of magnitude. Each test pins numOnlineNodes at the congestion threshold and uses ROUTER, which never congestion-scales, so the coefficient is 1 and the throttle is the only variable. The overflow case needs a base above INT32_MAX/10, hence three days rather than one. * ci(test): keep pull-request suite order fixed, seed the rest Shuffling the area order on every run - including pull_request - would turn a contributor's PR red for an ordering they did not choose, which is how a randomisation gets reverted instead of the coupling being fixed. That is the exact dynamic the ordering work was sequenced last to avoid, and the previous commit walked straight into it. - pull_request keeps the fixed declared area order. - push and schedule shuffle, seeded from the commit SHA: deterministic per commit, printed, attributable, and never blocking someone else's PR. - A suite_order_seed input on workflow_call and workflow_dispatch overrides both, so a specific failing order can be replayed anywhere, including on a PR. The run log prints which mode it took, the resulting order, and the local command to replay it. * ci(test): satisfy CKV_GHA_7 and yamllint on the seed input The seed is reachable through workflow_call, which callers can pass programmatically. The workflow_dispatch copy tripped checkov's "workflow_dispatch inputs MUST be empty" rule, and suppressing it was not worth it: replaying a specific order is a local operation, and the run log already prints the exact bin/run-tests.sh command to do it. * style(menu): apply the node-ID format convention RadioInterface.cpp documents the rule: 0x%08x in logs, !%08x in user-facing display. MenuHandler held every remaining exception - seven logs printing bare %08X, and two display labels doing the same. Repo-wide there are now no bare %08X node IDs left in log calls. * ci(test): pass workflow inputs through env, not shell interpolation suite_order_seed and github.event_name were spliced into the run: script as ${{ }} text, so a value carrying shell metacharacters would execute as code on the runner rather than being read as data. semgrep (run-shell-injection) and zizmor (template-injection) both flag it. Both now arrive as environment variables and are read as "$VAR". * refactor(test): share the seeded shuffle between the harness and CI bin/run-tests.sh and test_native.yml each carried a byte-identical copy of the MINSTD Fisher-Yates awk. The workflow prints "replay locally: ./bin/run-tests.sh --shuffle --seed $seed" after a shuffled CI run, and that instruction is only true while the two agree - drift would be announced by a replay quietly reproducing a different order than the one that failed. Extract shuffle_suites() to bin/lib/shuffle.sh and source it from both. Permutations verified identical across seeds before and after the move. * fix(test): correct the shared-state MISSING check and summary join Three defects in the new harness: state_classify() matched declarations two different ways - state_path_declared() for "undeclared", a hand-rolled regex for "missing". Interpolating an entry into an ERE also let a metacharacter in a manifest name match a file that is not the declared one. Both directions now go through the one helper. `paste -sd'; '` does not join with "; ": with -s, paste cycles through a multi-character delimiter one character per join, so paths rendered as "a;b c;d e". Replaced with an awk join. test-state-check.sh ran on after a failed cd instead of stopping (SC2164). ./bin/test-state-check.sh: 6/6 fixtures pass, MISSING included. * fix(portduino): bound General.MaxNodes MaxNodes was validated only for <= 0. Any positive value, including a typo'd or pasted-in one, propagates to MAX_NUM_NODES and scales both the node DB and the nodes.proto decode ceiling - failing at boot with no obvious cause. The ceiling is a sanity bound, not a capability limit; raise it if a host genuinely needs more. * docs(nodedb): reconcile the capacity tables The property matrix omitted the ESP32-S3 100-node flash tier that the platform table above it lists, and neither mentioned that the WASM build overrides MaxNodes to 80 in wasm_config_apply(). * fix(nodedb): make mesh-pb-constants.h self-sufficient on portduino The ARCH_PORTDUINO #error assumed it was unreachable in a normal build. It is not: the vendored device-ui sources include this header without configuration.h, which broke both native-tft docker builds. Include configuration.h here instead, ahead of every compile-time default - variant.h overrides MAX_RX_TOPHONE as well as MAX_NUM_NODES, so placing it lower in the file just moves the divergence to a redefinition. The #error stays as a backstop for the case where that include genuinely stops providing the cap. Verified with the native env's own flags: a TU including only this header now compiles, normal-order use of both macros compiles, and NodeDB.cpp compiles. * fix(portduino): raise the MaxNodes ceiling to 16000 Marked artificial: nothing in the node DB fails at 16001. 16000 sits just under the 16384 (128 x 128) population where HopScalingModule saturates its sampling denominator and starts dropping nodes, so a host inside the bound still gets meaningful hop recommendations. * lint(trunk): advise on node IDs logged as bare %08x RadioInterface.cpp documents the convention - 0x%08x in logs, !%08x in display - but nothing enforced it, which is how the MenuHandler cluster drifted. 22 call sites in PacketHistory, NodeInfoModule and PositionModule are still off it. A trunk linter rather than a CI grep job, because trunk checks changed files: new violations get flagged without a 22-site cleanup landing in an unrelated PR. Modelled on the existing too-many-defined definition. Scoped to values it can tell are IDs - an ID-shaped argument (->num, .from, getNodeNum) or message text naming one. A 32-bit hex that is not an ID is out of scope, so the CRC32 logs in ethOTA.cpp are correctly ignored. Emits "note", trunk's only non-blocking level: "warning" and "info" both exit non-zero and would gate CI, which is not what a log-format nit deserves. The pre-existing sites are line-scoped in the allowlist, so a new bad call in those same files is still caught. * lint(trunk): stop exempting the known node-id-format sites The seeded allowlist made the rule green by declaring the backlog acceptable. Empty it instead, so the 22 pre-existing sites are reported and get cleaned up by whoever next edits those files. Costs nothing to do: the rule emits "note", so these are non-blocking either way. The allowlist stays for its real purpose - a value the linter misreads as an ID. * style: log node and packet IDs as 0x%08x Clears the 22 sites the node-id-format linter reports, so the rule starts from zero rather than from a backlog nobody can see - trunk suppresses pre-existing findings by default, so left alone these would not have surfaced on edit the way an empty allowlist implies. Format strings only; no argument or control flow changes. The !%08x user-facing display forms are deliberately untouched - that is the other half of the same convention. * test(harness): build once up front, so suite timings mean something run-tests.sh fused build and run in a single pio invocation, so whichever suite PlatformIO's directory walk reached first absorbed the entire src compile and reported it as its own duration. On a real run that made a 0.03s suite report 13m21s, and hid the build cost from every other number in the summary. Do what .github/workflows/test_native.yml already does: one --without-testing build pass, then run with --without-building. Measured on a full 44-suite run - the build is now a single reported figure and 968 test cases execute in 1.9s, with no suite above 0.084s. Build output goes to its own log rather than $LOG: the outcome regexes match "error:" and "[ERRORED]", so a compiler diagnostic sharing that file would read as a test failure. Both red paths now keep the log they quote from. $LOG and the build log are mktemps the EXIT trap removes, so the three grepped lines were previously all anyone ever saw - and the cause is usually further up than the first [FAILED]. * test(harness): keep the run log on every red path bin/pio-test-isolate.sh already keeps a failing or DIRTY suite's sandbox and log under .pio/test-state/<suite>/. What was missing is the cross-suite view: $LOG is a mktemp the EXIT trap deletes, so run-tests.sh quoted three grepped lines from a file that no longer existed by the time anyone looked. Preserve it as .pio/build/<env>/test-failure.log from both red paths - including "no success summary found", which said "see log" while preserving nothing, and which is exactly the case where the build died before any suite ran and so left no per-suite sandbox either. Cleared at the start of every run, so a green run cannot leave a red one's log lying around looking current. * fix(test): report the real failure count on a shuffled red A shuffled run is one `pio test` invocation per suite, all appending to the same log, so the log carries one PlatformIO "N test cases:" summary per suite. verdict_red() took `tail -1`, which reports whatever the LAST suite did: a failure in suite 3 printed a "0 failed" summary from suite 44 directly under "RED - failures detected:". Sum the summaries instead. A single summary line - every unshuffled run - is passed through verbatim, so the familiar output is byte-identical. The patterns are passed to the awk helper as strings rather than /regex/ literals: awk evaluates a regex literal in argument position as `$0 ~ /re/`, so the callee would receive 0 or 1 and silently sum garbage. * fix(test): do not emit an empty suite name for an empty shuffle `printf '%s\n' "$@"` with no arguments still writes one empty line, and both callers read shuffle_suites through mapfile, so an empty suite list arrived as a single suite named "". Return before the printf when there is nothing to shuffle. * test(harness): state and enforce the Linux host requirement The native harness is a Linux tool: bash 4+ (mapfile), GNU coreutils and GNU find (-printf, md5sum, -executable). Most of that predates this branch - mapfile and both find predicates are already on develop - but none of it was written down, so the requirement was there to be discovered rather than read. Refuse to start on a non-Linux uname instead of degrading. On a BSD userland this would not fail cleanly: it would mis-hash the sandbox and mis-read the suite list, and still print a verdict. A state check that silently measures the wrong thing is worse than one that declines to run. Carrying a per-host fallback was the alternative, and it buys a second code path that nothing in CI exercises. bin/test-native-docker.sh already exists for macOS and non-Linux hosts, and the native-macos PlatformIO env is a build target for meshtasticd, not a test host - the isolation wrapper is registered for env:native and env:coverage only. Documented in the script header, test/README.md, and both agent docs. * fix(test): terminate every suite with exit(UNITY_END()) Two sites across two suites ended on a bare UNITY_END(). That ends the reporting, not the suite: setup() returns, the runtime goes on calling loop(), and the process runs forever. PlatformIO does not notice - it reports a suite from its Unity output, not from process exit - so the suite passes, the run goes green, and the binary stays resident. Thirteen of them had accumulated on one dev box, the oldest 19 hours old. The costs are quiet by construction: - the per-suite sandbox is deleted underneath a live process, so its CLEAN/DIRTY verdict describes what the suite had written when the harness stopped looking, not what it left behind; - .gcda coverage and LeakSanitizer's report both flush from atexit handlers, so a suite that never exits contributes no coverage and gets no leak check; - each survivor pins its own deleted 94 MB binary, which du cannot see. One of the two is the #else of an architecture guard, which is the easiest one to get wrong - it looks like there is nothing to clean up. test_mqtt has a correct exit(UNITY_END()) in its live branch, so a "does this file call exit() anywhere" check passes the file whole. test_serial had two more. develop's serial-config validation rework restructured that suite - the architecture guard is gone and both remaining branches now exit correctly - so this commit no longer has anything to change there; bin/lint-unity-exit.sh, added later on this branch, is what keeps it that way. test/README.md gets a section on it, since the skeleton showing the right shape had not stopped this happening. * test(harness): detect and reap suites that outlive their run A suite that never exits was invisible: PlatformIO reports a suite from its Unity output, so the run stayed green while the binary kept running. Two checks, because they fail differently. Runtime, in bin/pio-test-isolate.sh: the sandbox $HOME is mktemp-unique per suite, so any process still holding it is a survivor of that suite. Matching on the environment rather than a remembered PID identifies one whatever its parentage - a fork, a grandchild, a process already reparented to init - none of which a $! comparison catches. Reaped before the after-fingerprint is taken, so that fingerprint measures a tree nobody is still writing to, and so a run cannot leave processes accumulating on the host. Recorded as a sixth summary column and graded AMBER: the tests did pass, but the CLEAN verdict and the coverage were measured under a false assumption. Author-time, as bin/lint-unity-exit.sh, wired into trunk at "note" like node-id-format: every UNITY_END() must be wrapped in exit(). The rule is per occurrence, and that is the point - a file-level "calls exit() somewhere" check passes test_serial and test_mqtt, which have a correct one in their live branch and a bare one in the #else. Running it over the tree turned up test_mqtt, which the file-level pass had missed. It allows `int rc = UNITY_END(); ...; exit(rc)`, used by test_packet_signing to restore globals between the summary and the exit. That is where the rule gives ground: capturing and never exiting would leak and is not flagged. Flagging a correct idiom would push someone to "fix" working code. bin/test-state-check.sh gains a survivor fixture, asserting the wrapper both reports and reaps - a detector that only reports leaves the host accumulating processes, which is half the harm. 8/8. * fix(lint): make the unity-exit scanner statement-aware The rule judged one physical line at a time, which reports two kinds of correct code as bare: /* a comment that happens to mention UNITY_END() */ <- interior lines were never stripped exit( UNITY_END()); <- exit( and the macro never met On a probe of both, two of three findings were wrong. This is a note-level rule whose whole job is advice, and bin/lint-node-id-format.sh already says why that matters: a false positive costs more than a miss. One that cries wolf gets ignored, and the real finding goes with it. Carry /* ... */ state across lines and accumulate logical statements before testing, with a 12-line cap so one unclosed call cannot swallow the rest of the file - the same structure lint-node-id-format.sh uses, so the two custom linters in bin/ work alike rather than each having its own idea. Verified both directions: the develop-era sources still produce the same four findings, the fixed tree produces none, and a probe covering block-comment interiors, wrapped exit(), line comments, return UNITY_END() and capture-then- exit reports only the genuinely bare calls - including a complete block comment followed by real bare code on the same line, which the state machine has to keep live. Reported by CodeRabbit on #11322. * fix(lint): tokenise instead of pattern-matching, and self-test it Second round of review findings on the same scanner, all confirmed by direct test before changing anything. Six defects, one root cause: layered regexes cannot tokenise C++. False positives (correct code reported): - UNITY_END() inside a string literal read as code False negatives (real leaks missed): - a string containing "/*" opened comment state and swallowed later lines - greedy .* removed everything between two block comments on one line, taking a bare call with it - myexit(UNITY_END()) matched the exit() exemption as a substring - x == UNITY_END() and total += UNITY_END() matched the assignment exemption Replaced with a character-level scan carrying comment state, and token-bounded exemptions: exit must be a whole identifier, and the capture form must be a plain `=`. Raw string literals are still not modelled - there are none under test/, and delimiter tracking for a case that does not occur would be untested code guarding untested code, so it is documented rather than guessed at. Also drops the `return UNITY_END()` exemption. It only terminates from main(), there is no main() under test/, and from a helper it just returns a count. bin/test-lint-unity-exit.sh pins all fifteen cases, every false positive and false negative found in review among them. The rule has been wrong twice in a way that looked fine by inspection; it needed a self-test more than it needed another careful reading. Two further findings in the same review: - bin/run-tests.sh dropped PASSTHRU in shuffled mode, so `--shuffle -vvv` built verbosely and then ran quietly. The shuffled loop now forwards EXTRA_ARGS, which is PASSTHRU minus the -f pair it supplies per suite. - bin/run-tests.sh did not guard `cd "$ROOT_DIR"`. And one that did not reproduce: the survivor fixture's glob does find the pid file (verified with the lookup instrumented - the earlier failure was an artifact of running the script from /tmp, where SCRIPT_DIR cannot resolve). The assertion was still weak, because an empty pid took the "not running" branch and passed vacuously. It now fails if the pid was never recorded, and finds the file by search rather than assuming a directory depth. Reported by CodeRabbit on #11322. * fix(lint): report each UNITY_END occurrence at its own location The self-test only asked "did the linter say anything", so it could not have caught a wrong line, a wrong column, or a missing second finding. Fixtures now assert the exact diagnostics as line:col, and the first run of that assertion found two real problems. The caret pointed at the wrong occurrence. For `exit(UNITY_END()); UNITY_END();` the verdict was right but the column was 17 - the wrapped call - because the scanner stripped terminating forms out of the whole statement and then reported the first occurrence it had seen. Two bare calls on one line reported once. Judged per occurrence now, by looking back through whitespace at what wraps it, so both the count and the caret are right. That also needed a position map from strip_noncode(): removing a comment or collapsing a literal shifts every later column, and counting occurrences in the raw line does not recover it either - TEST_MESSAGE("... UNITY_END() ..."); UNITY_END(); has two occurrences in the raw text and one in the code. Four of the expected columns I wrote by hand were also wrong, off by one. The linter was right in every case; the assertions were not. They are computed from the fixture text now rather than pasted from output, because a baseline accepted from the tool it is testing asserts nothing. 17 fixtures, including the two-on-one-line case from review and its mirror. Reported by CodeRabbit on #11322. |
||
|
Thomas GöttgensandAustin
|
ecd59e3120 |
Package meshtasticd for Windows as an MSI (#11289)
* Package meshtasticd for Windows as an MSI Adds a --service flag connecting meshtasticd to the Service Control Manager, a WiX MSI installing it as an auto-start LocalSystem service with config in %ProgramData%\Meshtastic, and a CI step attaching the MSI to releases. * Address review comments Bind workflow expressions to env vars in run: bodies, and build the service status per call with an atomic checkpoint. * Fix service stop state and CI lint Latch the stop under a mutex so a startup report cannot walk the state back. Ignore the new workflows in semgrep and checkov, as main_matrix already is. * Drop the checkov ignore for the winget workflow Resolve the newest release inside the job instead of taking workflow_dispatch inputs, so CKV_GHA_7 no longer fires and checkov stays active on the file. * Carry the MSI architecture into the winget manifest Parse it from the asset name instead of defaulting to x64, and fail on a multi-arch release rather than validating one at random. * Restore release/.gitignore * Leave the main matrix alone Release attachment moves to the matrix rework in #11151. The MSI is still built and uploaded as a CI artifact. --------- Co-authored-by: Austin <vidplace7@gmail.com> |
||
|
TomandClaude Opus 5
|
21e3a583bd |
Yaml check for Meshtasticd (#11224)
* feat(portduino): add `meshtasticd --check` config validator Users hand-writing files in /etc/meshtasticd/config.d/ get no feedback when a key is misplaced, misspelled or duplicated: meshtasticd silently ignores what it does not read, so a broken config looks identical to a working one. Add a --check mode that loads the configuration exactly as startup does, then reports what it found and exits: - Duplicate keys, via the yaml-cpp Parser/EventHandler stream. The Node API cannot see them because the map is already collapsed by the time it exists, and yaml-cpp keeps the FIRST occurrence, so a later override is discarded. - Unknown or misnested keys, against a schema mirroring what loadConfig() reads, with a hint naming the section a stray key actually belongs to. - rfswitch_table validation: unrecognised pins, mode rows whose length does not match the pin list, values that are not HIGH/LOW, and unknown modes. - Cross-file overlap: every .yaml in the config directory merges into one portduino_config, so the file loaded LAST wins, the opposite of the within-file rule. Those files are read in filesystem order, not alphabetical. - A warning when more than one file defines a Lora section: spidev, spiSpeed, gpiochip, DIO2_AS_RF_SWITCH, DIO3_TCXO_VOLTAGE and USB_PID/VID/Serialnum are assigned unconditionally with a default every time one is seen, so any of them not repeated in the last file loaded is silently reset. - The resolved gpiochip/line for each pin, since a line that exists on the wrong chip is claimed successfully and then silently does nothing. Exits non-zero when errors were found so it can also gate CI over bin/config.d/**, keeping one implementation rather than a second schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(portduino): flag pins that resolve to -1 in --check A pin key whose value will not convert to a number falls back to RADIOLIB_NC (-1) while still being marked enabled, and initGPIOPin() then trips an assertion inside LinuxGPIOPin rather than failing cleanly. YAML indentation makes this easy to hit by accident: a stray line under "CS: 8" folds into the value as a multi-line scalar, so the file parses, the daemon crashes with a stack trace from a library file, and --check reported "Configuration looks good" while printing "pin -1" two lines above. Report it as an error naming the likely cause instead. Also correct a comment claiming unparseable config.d files are skipped silently. They are not: loadConfig() prints "*** Exception ..." with the line and column. It is the discarded return value, not the diagnostic, that makes the file's absence from the merged config easy to miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(portduino): cover `meshtasticd --check` with fixtures and a fuzz suite Adds the tests the config validator was missing, and the checks and fixes that writing them turned up. The theme throughout is configuration that the YAML parser accepts but that does not mean what it looks like it means. Tests ----- bin/test-config-check.sh - 57 assertions driving a built meshtasticd against test/fixtures/portduino-config (50 fixtures plus two config.d trees). A shell test rather than a Unity suite because both behaviours under test are properties of the process: --check is judged by its exit status and printed report, and the "a normal run rejects a bad config" path ends in exit() inside portduinoSetup(), neither of which is reachable from a suite that links one translation unit. Every fixture carries a comment header naming its planted fault and the expected finding, so it can be read on its own. Coverage: * a clean config for each of the ten radio module families (RF95, sx1262, sx1268, LLCC68, sx1280, lr1110, lr1120, lr1121, sim, auto), asserted both findings-free and resolving to that module, so a silent fallback to sim cannot pass * LR11xx rfswitch tables: unrecognised pins, rows longer and shorter than the pin count, levels that are not exactly HIGH, a missing pins list, more than five pins, a scalar table, unknown MODE_ keys, a MODE_ row stranded one level out, and a legal partial table * the PA gain table in both accepted shapes, entries outside the uint16 range it is stored in, and more than the 22 points that are kept * values of the wrong type, split by consequence: the two settings read with no fallback stop meshtasticd starting, everything else is silently replaced by its default * out-of-range and unit mistakes: TCXO voltage written in millivolts, ports outside their usable range, an over-long StatusMessage * MAC sources: both keys set at once, a malformed address, an interface that does not exist * structural faults: duplicate keys, non-mapping and unknown sections, a key left at the top level, a sequence at the document root, an empty file, unreadable pins, unparseable YAML * cross-file behaviour over a config.d directory, including the switch tables that do not override each other * five configs run WITHOUT --check, each of which must still be refused, so check mode cannot quietly make the normal path permissive test/test_fuzz_config - adversarial fuzzing of the checker itself, the "the tool meant to diagnose your config crashes on it" failure mode. Scope is deliberately narrow: yaml-cpp does the parsing and is fuzzed upstream, so what is exercised here is our code above the parse, above all the duplicate-key detector, which is the one hand-rolled piece and walks the raw parser event stream with its own stack. Groups: the checked-in fixtures as a seed corpus, 3000 byte mutations of them (flips, truncation, insertion, splicing, deletion), and structural torture (nesting to 4096 in flow and block style, duplicate keys at depth, anchors, aliases and merge keys, 64KB keys, 256KB scalars, multi-document files). A fourth group of random bytes is present but disabled behind FUZZ_CONFIG_RANDOM_BYTES: it was half the runtime for the least return, since uniform noise is rejected on the first token. The contract is crash-freedom and termination under AddressSanitizer, not any particular finding. CI runs the shell test in the existing native simulator job; the fuzz suite is picked up by the existing ^test_fuzz_ area rule. native-suite-count 40 -> 41. The fixtures are exempt from trunk in .trunk/trunk.yaml, since prettier rejects the duplicate keys and bad indentation that are the point of them. Checker fixes found while writing the tests ------------------------------------------- --check reported a clean exit 0 on configs meshtasticd then refuses to boot, the worst failure a diagnostic tool can have. Four hard exits inside loadConfig() killed the report before it printed: an unparseable file, an unknown Lora.Module, MACAddress and MACAddressSource both set, and HUB75 on a build without it. All are now reported as findings, and all are still refused on a normal run. New validation: Lora.Module against the accepted spellings, which are matched exactly and inconsistently cased, with a suggestion when only case differs; a per-key value type table covering ~85 keys, tested by asking yaml-cpp to perform the same conversion loadConfig() will so it cannot drift; the PA gain table; DIO3_TCXO_VOLTAGE, which is in volts and multiplied by 1000, so the millivolt value everything else uses silently asks for 1800V; APIPort and Webserver.Port ranges; MaxNodes; StatusMessage truncation; MAC address and source; and an unreadable ConfigDirectory. Also fixes a crash: a ConfigDirectory that cannot be read threw an uncaught filesystem_error from directory_iterator and aborted meshtasticd with SIGABRT, taking --check down with it. It now fails cleanly. Two smaller ones: cppcheck's uselessCallsSubstr on the ancestor walk, which was failing every check job; and the duplicate-key detector's stack pop, which was unguarded and relied on yaml-cpp emitting balanced events. Switch tables are the one place "the file loaded last wins" is false. The loader only ever writes HIGH and never writes LOW back, so a HIGH from an earlier file survives a later file that clears it and the radio drives the OR of every table loaded. Confirmed with --output-yaml. Reported as an error for now; the loader itself is left alone, as that changes RF behaviour. * fix(portduino): report CH341 pins as adapter indexes, not gpiochip lines --check printed "Resolved GPIO lines (what meshtasticd will try to claim)" for every config, listing a gpiochip and line for each Lora pin and advising they be confirmed against gpiodetect and gpioinfo. For spidev: ch341 every part of that is false. portduinoSetup() skips initGPIOPin() for every Lora pin when spidev is ch341 and hands the raw numbers to Ch341Hal, so nothing is claimed from a gpiochip -- and on Windows and macOS, where a USB adapter is the only way to attach a radio, there is no gpiochip, gpiodetect or gpioinfo to check against in the first place. The checker had no ch341 coverage at all: not one fixture used it, so the whole USB-SPI path went unexercised. The summary now splits on the transport. A ch341 device gets its pins listed as adapter indexes with the gpiod advice dropped, and a gpiochip or line mapping written alongside it is reported: those are read, stored, and never used. Also: "RF switch table: not set" read as a gap on an SX126x, where there is nothing to set. setRfSwitchTable() is only ever called for an LR11xx, so absence is now "not needed for this module" everywhere else, and "not resolved yet" for auto, which has no module to judge against. Fixtures: usb-ch341.yaml (clean, the meshstick shape) and ch341-gpiochip.yaml. CI fix ------ test-native was RED on "config.d overrides are reported", which wanted 2 warnings and got 1. The fixture's two config.d files name different modules, so which one wins -- and whether the LR11xx-without-a-switch-table warning fires -- depends on the order the filesystem returns them in. That is the very thing the fixture exists to demonstrate, so the count is no longer asserted; the report's own order caveat is asserted instead. Review fixes ------------ The unreadable-ConfigDirectory diagnostic was the one new print in PortduinoGlue.cpp not gated behind !configCheck, so it landed ahead of the report header and broke the clean output the rest of the change is careful to keep. Docs: rfswitch-valid.yaml carries seven modes, not eight, and empty-file.yaml is comments-only rather than zero bytes. * style(portduino): trim --check comment blocks and reconcile suite count Condense the multi-paragraph comment blocks in the --check validator to the one-to-two-line convention, and bump test/native-suite-count to 42 for the test_fuzz_config suite added here. 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
|
047c4e9feb |
add LR 2021 to portduino, and allow Framebuffer devices to rotate the screen from config (#11252)
* 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 |
||
|
Andrew YongandBen Meadors
|
abf217c38d |
fix(mesh): don't assert on malloc() failure in MemoryDynamic::alloc() (#11197)
* fix(mesh): don't assert on malloc() failure in MemoryDynamic::alloc() MemoryDynamic<T>::alloc() called assert(p) right after malloc(), instead of returning nullptr like the static MemoryPool<T,N>::alloc() already does on exhaustion. All of packetPool's callers were already hardened to null-check allocCopy()/allocZeroed() (#10948, #10951), but that path is unreachable on real OOM here: assert() fires first, one level down, before the caller ever gets a chance to check anything. On STM32WL (packetPool is MemoryDynamic there - not enough static RAM for the fixed pool), assert() failures are wrapped to an infinite `while(true);` loop rather than aborting or resetting, so the thread just hangs forever instead of returning nullptr. Reproduced on wio-e5 hardware under a burst of incoming DMs, with free heap draining toward zero right before the hang. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix: null-check the two allocUnique*() call sites missed by prior audits #10948/#10951 null-checked every raw-pointer packetPool.allocCopy()/ allocZeroed() call site, but missed the two spots using the UniquePacketPoolPacket (unique_ptr) wrapper: UdpMulticastHandler::onReceive() and MQTT::onReceive() both dereferenced the allocation result unconditionally. Previously unreachable in practice: MemoryDynamic::alloc() asserted before ever returning null, so nothing downstream saw it. Fixed alongside that assert removal (this branch) since these two are now reachable with a genuine null - same fix as everywhere else, just an unchecked pointer that turns up under OOM. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(SimRadio): don't leave isReceiving stuck true on allocation failure startReceive() set isReceiving = true before allocCopy(), so a failed allocation (now reachable with a real nullptr instead of hanging in assert()) left the simulated radio permanently marked as receiving: handleReceiveInterrupt() returns immediately on a null receivingPacket, before ever reaching the code that would clear isReceiving. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Jonathan Bennett
|
19ddadf3ff | Check for the SPI bufsize, and throw a useful error when set too small (#11072) | ||
|
Thomas Göttgens
|
a808e992a1 | Add native Windows build of meshtasticd (#11031) | ||
|
|
8d1fbbf55f |
Snake! (#10936)
* Snake! * Add spiLock to snake score saving * Check fixes * More careful locking * WIP: Big Display Node * Update src/graphics/HUB75Display.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Add HUB75 Native * Add Tetris game module with core logic and UI integration - Implement TetrisGame class for game logic, including piece movement, rotation, and line clearing. - Create TetrisModule class to manage game state, input handling, and rendering on OLED display. - Introduce high score tracking with persistence and optional mesh broadcasting. - Define UI states for title, playing, paused, game over, and high scores. - Implement input handling for game controls and state transitions. - Add rendering functions for the game board, high scores, and title screen. * feat(snake): add snake graphics and update display logic in SnakeModule * Prompt for Initials for Tetris, too * refactor games module * Games refactor * hub75 native double buffer * Games tuning * Make joystick repeat events on held button * Add clouds and colors * Fix breakout and more color * difficulty tuning * trunk * refactor game announcements, etc * Scale chirpy gravity as game speed increases * Portduino, check for hub75 display before reading in hub75 options * Final(?) games tuning * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Properly ignore input when games screen not shown * Fail gracefully when HUB75 is selected but not supported. --------- Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Ixitxachitl <kramerfm@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
James RichandClaude Opus 4.8
|
269b974e96 |
feat: populate MyNodeInfo.device_id on all platforms (#10995)
* feat: populate MyNodeInfo.device_id on all platforms RP2040/RP2350 use the 64-bit pico unique board id, STM32WL the 96-bit silicon UID, ESP32-S2 joins the existing OPTIONAL_UNIQUE_ID efuse branch, and everything else (classic ESP32 in particular) falls back to a deterministic factory-MAC-derived id, resolving the long-standing FIXME. Portduino keeps the config-supplied id preferred and now uses the MAC fallback when the config omits one. No proto or persistence changes; the id is re-read from silicon each boot and PhoneAPI still zeroes it for unauthenticated clients. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: declare zero_mac const to satisfy cppcheck Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address review feedback on device_id derivation Clear any disk-loaded device_id before the silicon derivation so a failed read leaves it unset rather than stale (Copilot), and size the portduino config copy with sizeof instead of a literal 16 (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor: extract device_id generation into per-arch getDeviceId() Per review feedback on #10995: move the platform-specific MyNodeInfo.device_id derivation out of the #if/#elif ladder in NodeDB.cpp into a getDeviceId() interface (target_specific.h) implemented per-architecture alongside each platform's getMacAddr(): - esp32: efuse OPTIONAL_UNIQUE_ID (C3/S2/S3/C6); classic ESP32 -> MAC - nrf52: FICR DEVICEID + DEVICEADDR - nrf54l15: FICR->INFO.DEVICEID + DEVICEADDR (NRF_FICR-guarded, MAC fallback) - rp2xx0: pico_get_unique_board_id() - stm32wl: HAL_GetUIDw0/1/2() - portduino: config-supplied id preferred, else MAC The shared MAC-derived fallback moves to meshUtils as getMacAddrDeviceId(). NodeDB.cpp now zero-inits the field and makes a single getDeviceId() call, dropping ~65 lines of platform boilerplate plus the esp_efuse/pico/stm32 includes that came with it. No behavior change: device_id is still re-read from silicon each boot and never persisted. Builds green: native-macos, tbeam, heltec-v3, rak4631, rak11310, rak3172. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: address review of device_id refactor - getMacAddrDeviceId(): zero-init the mac[6] buffer. getMacAddr() can return without writing (e.g. Portduino with no MAC source), so the old uninitialized buffer let stack garbage pass the all-zeros guard and become device_id. The pre-refactor code relied on the zero-initialized static ourMacAddr; restore that guarantee. - nrf54l15 getDeviceId(): drop the `#if defined(NRF_FICR)` guard and read FICR unconditionally (as the pre-refactor NodeDB code did). The guarded #else fell back to getMacAddr()'s hard-coded placeholder MAC, which would give every unit an identical device_id; a missing NRF_FICR should be a loud compile error. - NodeDB.cpp: `#include "target_specific.h"` instead of hand-copied externs for getMacAddr/getDeviceId; retire the stale FIXME. Same for meshUtils.cpp (whose extern comment wrongly claimed the TU was Arduino-free). - Delete the orphaned commented-out device_id hex-dump block in NodeDB.cpp. - Trim/de-duplicate the getDeviceId contract comments (single-sourced in target_specific.h). Builds green: native-macos, tbeam. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: trim device_id comments to the repo's 1-2 line guideline Addresses CodeRabbit review nitpick on #10995: shorten the getDeviceId() (target_specific.h), getMacAddrDeviceId() (meshUtils.h), device-id refresh (NodeDB.cpp), and nrf54l15 getDeviceId comments to two lines each, per the "one or two lines maximum" coding guideline. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Jonathan Bennett
|
9060ab4418 |
add linuxJoystick input module (#10970)
* add linuxJoystick input module * close epollfd to avoid leaks |
||
|
Andrew YongandBen Meadors
|
6768bd08df |
fix(modules): null-check packetPool/clientNotificationPool/mqttClientProxyMessagePool allocations (#10951)
* fix(modules): null-check packetPool/clientNotificationPool/mqttClientProxyMessagePool allocations Follow-up to fix/meshpacket-alloc-null-checks (PR meshtastic/firmware#10948), which fixed the core routing path (Router/NextHopRouter/ReliableRouter/ MeshService/RadioLibInterface) after reproducing a HardFault on STM32WL hardware under real mesh traffic. This covers the same allocCopy()/ allocZeroed() unchecked-return pattern in lower-frequency paths that were out of scope for that PR: SimRadio.cpp (portduino sim RX/TX), NodeInfoModule, the Telemetry modules' power-saving-sleep notifications, MQTT (map report, client-proxy messages, config-validation notifications), PositionModule, SerialModule, and KeyVerificationModule. Sites where the allocation result was already read back through an existing null check downstream (e.g. Telemetry's lastMeasurementPacket, SimRadio's receivingPacket, AdminModule::sendWarning) were left as-is - verified safe, not touched. Router::allocForSending() (and everything that funnels through it, e.g. allocDataProtobuf()) remains unguarded and out of scope here too - it's called from 30+ sites across the codebase and needs its own audit. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(KeyVerificationModule): bound cn->message writes with snprintf CodeRabbit flagged unbounded sprintf() writes into the fixed-size ClientNotification.message buffer as a static-analysis nitpick while reviewing this PR's allocation null-checks. Align these four sites with the snprintf(dest, sizeof(dest), ...) pattern already used for the same field elsewhere in this PR (SerialModule.cpp, MQTT.cpp). Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Ian McEwen
|
ade42830e0 |
Re-enable wantresponse and other fields in simradio when on the decoded path (#10934)
* Re-enable wantresponse and other fields in simradio when on the decoded path * Make the comments less verbose |
||
|
Tom
|
3becaf2d95 | emdashes begone (#10847) | ||
|
|
2fdb722e00 |
Add gpsd support to portduino/native (#10781)
* Add gpsd support to portduino * copilot fixes * emit gps config in emit_yaml * Fix formating to standard * Address coderabitai issues --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> |
||
|
Tom
|
fd62322876 |
Log tidy (#10798)
* clarify channel number formatting and fix the fixed-width padded nodeIDs * use the explicit 0x%08x for packets and nodeIDs throughout * LLM instructions |
||
|
Ben MeadorsandAustin
|
0baf8b1db6 |
Portduino WASM hello world (still experimental) (#10793)
* Add ARCH_PORTDUINO_WASM build: meshtasticd in WebAssembly over WebUSB
Compile the full portduino firmware to WebAssembly (Emscripten) so a real node runs in a browser tab or headless Node, driving a LoRa radio over WebUSB through a CH341 — the desktop Ch341Hal path with its libusb backend swapped for a WebUSB one. The native/desktop portduino build is unchanged.
New build env under src/platform/portduino/wasm/ (excluded from the native build_src_filter): WebUSB libpinedio backend, config/FS/region/MAC/PhoneAPI glue, wasm setup/loop, JS WebUSB runtime, and build stubs. bin/build-portduino-wasm.sh runs a standalone cached emcc build to build/wasm/meshnode.{mjs,wasm}.
Six firmware sources gain #ifdef ARCH_PORTDUINO_WASM guards (single-threaded cooperative emscripten_sleep loop, continuous RX, US region default, std RNG, no-popen exec); none affect non-wasm builds.
* PortDuino WASM: CI build job, cross-platform libdeps, configurable adapter
bin/build-portduino-wasm.sh: auto-detect native-macos (macOS) vs native (Linux/CI) libdeps with a NATIVE_ENV override, and use the SAME env's Crypto with an XEdDSA-present guard. Drops the heltec-v3/Crypto borrow — the meshtastic/Crypto pin already ships XEdDSA; the native libdeps cache was just stale. No env pin change.
Add .github/workflows/build_portduino_wasm.yml: build the ARCH_PORTDUINO_WASM target in CI (ubuntu + emsdk, native libdeps) so it can't silently bit-rot; asserts build/wasm/meshnode.{mjs,wasm}.
src/platform/portduino/wasm: add wasm_set_lora_* setters so the JS host can configure any CH341 LoRa adapter (module, USB ids, DIO/TCXO, SPI speed, pins); wasm_config_apply falls back to the MeshToad defaults when unset.
* WASM: build as a first-class [env:wasm] via platform-wasm (retire emcc script)
Replace the standalone emcc build (bin/build-portduino-wasm.sh) with a normal
PlatformIO env, `pio run -e wasm`, using the new meshtastic/platform-wasm
platform (emcc/em++ + Asyncify, the WASM sibling of platform-native). The
portduino WebAssembly node is now built the same way as every other target.
- variants/native/portduino/platformio.ini: add [env:wasm] (platform pinned to
platform-wasm, board wasm). Translates the script's source set into a curated
build_src_filter, the ~30 EXCLUDE_* defines, and the lib set; defines
ARCH_PORTDUINO_WASM in-repo so correctness doesn't hinge on the platform's
board.json.
- extra_scripts/wasm_link_flags.py: the firmware-specific emcc *link* settings
(EXPORT_NAME, EXPORTED_RUNTIME_METHODS, EXPORTED_FUNCTIONS, ASYNCIFY_IMPORTS).
PlatformIO feeds build_flags to compile only, so these must ride LINKFLAGS;
without it the WebUSB Asyncify seam and the JS host's runtime methods are
dropped (the _wasm_* exports survive only via EMSCRIPTEN_KEEPALIVE).
- PortduinoGlue.{h,cpp}: guard the yaml-cpp dependency out of the WASM build
(#ifndef ARCH_PORTDUINO_WASM around the include, emit_yaml/loadConfig/
readGPIOFromYaml). The browser node configures via the wasm_set_lora_* setters
and dead-strips the YAML path; this drops the host yaml-cpp build dependency
entirely. Native is unchanged (guards are inert there).
- portduino_glue_wasm.cpp / portduino_main_wasm.cpp: repair EM_ASM JS that a
formatter had mangled (!== -> != =, regex split) in the prior landing; the
emcc link succeeds regardless, so CI now runs `node --check meshnode.mjs`.
- .github/workflows/build_portduino_wasm.yml: build via `pio run -e wasm`
(artifacts under .pio/build/wasm/), trigger on the shared inputs the env
inherits (root platformio.ini, bin/platformio-*.py).
- NodeDB.cpp: drop the dead ARCH_PORTDUINO_WASM region-default branch (region
now defaults the same as native).
- Crypto renovate pins: add the missing gitBranch so they track upstream.
Output: .pio/build/wasm/meshnode.{mjs,wasm} (ES module, factory createMeshNode).
Verified: pio run -e wasm (against the published platform archive), node --check,
module instantiates in Node with all exports; native-macos + Docker native unit
tests (450/450) still pass.
* Fix name on the Piggystick
* wasm: pin platform-wasm at the GPL-3.0-relicensed commit
platform-wasm's LICENSE was always GPLv3 (matching this firmware), but its
platform.json/README still declared Apache-2.0 (mis-copied from platform-native).
That's fixed upstream in b83fa5b; bump the [env:wasm] pin to it. Build output is
unchanged (license metadata only). Verified: pio run -e wasm against b83fa5b.
* wasm: make reboot() actually restart the node (was a no-op)
In wasm the reboot path is live (main.cpp -> Power::powerCommandsCheck ->
Power::reboot), but Power::reboot's ARCH_PORTDUINO arm tore down SPI/Wire/Serial
and then called the no-op ::reboot() stub — leaving the node running with a dead
radio until the tab was manually reloaded. Triggers include an admin/phone
reboot, factory reset, the "reconfigure failed" path, and the 60 s stuck-TX
hardware watchdog (RadioLibInterface).
- Power::reboot(): add an ARCH_PORTDUINO_WASM arm (before ARCH_PORTDUINO, since
the wasm build defines both) that skips the host teardown and just calls
::reboot(). notifyReboot already let modules persist.
- ::reboot() (glue): hand off to the JS host — browser reloads the tab (NodeDB
state survives via IDBFS, same identity returns); headless calls Module.onReboot
if provided, else logs. Loose !=/== so clang-format doesn't mangle the EM_ASM JS.
- README: document the reboot handoff + the Module.onReboot hook.
Verified: pio run -e wasm + node --check (EM_ASM intact); native-macos unaffected.
* wasm: rename env to native-wasm and run it in the main CI matrix
Rename [env:wasm] -> [env:native-wasm] for consistency with the portduino
native family (native, native-macos, native-tft). The build dir follows to
.pio/build/native-wasm/ (artifact is still meshnode.{mjs,wasm}); the PIOENV
guard in extra_scripts/wasm_link_flags.py, the README, and the companion wrapper
move with it. The board stays `wasm`.
Also wire the build into normal CI: build_portduino_wasm.yml becomes a reusable
workflow (workflow_call) invoked as the `build-wasm` job of main_matrix.yml, so
the WebAssembly node is built like every other platform instead of on a separate
path trigger.
* native-wasm: auto-locate the Emscripten SDK (pre-build script)
`pio run -e native-wasm` failed with "emcc not found" whenever it was invoked
from a shell that hadn't sourced emsdk_env.sh — a VS Code task, an IDE build
button, a bare terminal. Add a pre: extra script that probes the usual emsdk
locations ($EMSDK_ENV, $EMSDK, ~/emsdk, ./.emsdk, the sibling companion
checkout), sources emsdk_env.sh, and imports the resulting environment so the
platform builder and emcc see PATH/EMSDK/EM_CONFIG. No-op when emcc is already
reachable (CI), silent when no SDK is found (the platform emits its own error).
* wasm: address PR review feedback
- js/bridge.js: import CH341 from "./ch341.js" (sibling in this layout), not
"../src/ch341.js" which doesn't resolve here.
- js/ch341.js: a zero-length transferIn while MISO bytes are still outstanding
now throws instead of breaking out with a partially-filled buffer — silent SPI
corruption becomes a loud error, matching the comment above it.
- libpinedio_webusb.c: webusb_set_auto_cs honors the AUTO_CS option (? 1 : 0)
instead of the always-on ? 1 : 1. Runtime behavior is unchanged — Ch341Hal sets
AUTO_CS=0 right after pinedio_init (RadioLib drives the active-low NSS); the
option just isn't set yet at init, so this now correctly defaults off.
- SX126xInterface.cpp: the RX-start error log now names the method actually
called (startReceive vs startReceiveDutyCycleAuto) instead of hardcoding the
duty-cycle name in the WASM branch.
* native-wasm: drop the emsdk bootstrap shim (now in platform-wasm)
The Emscripten SDK auto-location moved into the platform-wasm builder, so the
firmware no longer needs its own pre: extra script. Remove
extra_scripts/wasm_emsdk_env.py and bump the platform pin to the build that
carries the bootstrap. The wasm_link_flags.py post script stays — those exported
fns / runtime methods / Asyncify import seam are firmware-app-specific.
* wasm: use the canonical companion name (meshtasticd-wasm-node)
The companion repo was renamed meshtastic-web-node -> meshtasticd-wasm-node; fix
the stale name in the wasm README and bump the platform pin to the build that
promotes the canonical name in its emsdk auto-location.
* wasm: re-entrancy guard for the API/region entry points + flaky-open retry
Two robustness fixes for the browser node:
- Re-entrancy guard. The node is single-threaded + Asyncify: while setup()/loop()
is suspended inside a WebUSB transfer, the JS event loop is free, so a stray
DOM/timer callback that re-enters a wasm_* entry point starts a second Asyncify
unwind ("async operation already in flight" abort) or clobbers shared PhoneAPI
state (observed as a "PhoneAPI::available unexpected state" flood). Add a
g_wasm_in_firmware flag set around setup()/loop() (portduino_main_wasm.cpp); the
wasm_set_region / wasm_api_to_radio / wasm_api_from_radio / wasm_api_available
entry points now reject a mid-tick call (return busy) instead of corrupting or
aborting. The host must still call them between ticks — this is the safety net
the design lacked, not a substitute for the JS queue.
- CH341 open retry (js/bridge.js). First-connect WebUSB is flaky — the interface
is briefly unclaimable right after the grant, or held by a prior session,
giving a transient "Could not open SPI: -1". Retry the open with a short
backoff, closing the device between attempts so claimInterface starts clean.
* wasm: exclude emscripten-only sources from cppcheck
The `check` board matrix runs `pio check` (cppcheck) over all of src/,
including src/platform/portduino/wasm/. cppcheck can't parse the EM_ASYNC_JS/
EM_JS macros (Syntax Error: AST broken at libpinedio_webusb.c:39,
internalAstError) and these sources are not part of any checked board build
([env:native-wasm] is board_level=extra, compiled by the build-wasm CI job).
Suppress the wasm dir in suppressions.txt, the same way generated/ and .pio/
are already excluded.
* wasm: coalesce FS.syncfs so two never run at once
IDBFS syncfs is async; the explicit wasm_fs_sync (5s timer + post-save +
beforeunload) could overlap a prior in-flight sync, warning "2 FS.syncfs
operations in flight at once". Serialize: if a sync is running, mark a pending
re-sync and let the in-flight one chain it on completion — at most one in flight,
trailing writes still flushed. (Companion drops IDBFS autoPersist so this is the
single persistence path.)
* wasm: silence false-positive SAST on the emscripten glue
- extra_scripts/wasm_link_flags.py: restore the trunk-ignore-all(ruff/F821,
flake8/F821) header every other SCons extra_script carries; Import/env are
SConscript-injected globals, so ruff/flake8 flag them as undefined.
- .semgrepignore: exclude src/platform/portduino/wasm/js/ (browser WebUSB glue,
not part of the firmware binary). The unsafe-formatstring rule false-positives
on its benign retry/diagnostic console logs.
* Update .github/workflows/build_portduino_wasm.yml
Co-authored-by: Austin <vidplace7@gmail.com>
---------
Co-authored-by: Austin <vidplace7@gmail.com>
|
||
|
jessm33
|
ec82aa116d | Update native to exit when configured with an unknown module instead of going in to sim mode. | ||
|
Ben MeadorsandCopilot Autofix powered by AI
|
5ffd30c4a8 |
Fix PKC on portduino sim by working around region blocks keygen and packet length (#10730)
* Fix PKC on portduino sim by working around region blocks keygen and packet length * Reserve Compressed framing overhead in sim loopback capacity check The sim loopback re-encodes the Compressed wrapper back into decoded.payload.bytes (the same 233-byte field its data is copied from), so the carried bytes must leave room for protobuf framing. Checking against sizeof(c.data.bytes) (233) let near-max payloads overflow pb_encode_to_bytes(), which returns 0 and silently sends an empty loopback payload. Cap at sizeof(decoded.payload.bytes) minus the worst-case Compressed framing (meshtastic_Compressed_size - data size) for both the ciphertext and plaintext paths. Addresses Copilot review feedback on #10730. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Run trunk fmt --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
nomdetom
|
5d55353939 | Some fixes and tidies for testing both online and in unit_tests | ||
|
|
982440d21d |
Noise floor (#9347)
* add noise floor * Sliding window noise floor * Add getCurrentRSSI() to SimRadio for noise floor support * Remove sendLocalStatsToPhone call from runOnce * Change noise floor to int32_t type * Use int32_t for RSSI sample storage in noise floor * Remove float cast from noise floor assignment * Fix Copilot review issues: fix noise floor logic, types, and null pointer - Use robust busyTx/busyRx checks instead of simple isReceiving check - Initialize noiseFloorSamples to NOISE_FLOOR_MIN instead of 0 - Move noise_floor assignment inside null check to prevent potential crash - Change getNoiseFloor() and getAverageNoiseFloor() to return int32_t - Fix RSSI validation to check for positive values (rssi > 0) - Fix format specifier from %.1f to %d for int32_t - Update comments to accurately reflect the sampling logic * Fix RSSI condition to include zero value * Change noise floor initialization to zero * Disable noise floor for LR11x0 chips: getRSSI(bool) unsupported * Remove updateNoiseFloor call from onNotify to avoid radio queue overflow Per PR review feedback, calling updateNoiseFloor() in onNotify() for every ISR event (ISR_TX, ISR_RX, TRANSMIT_DELAY_COMPLETED) can cause the LoRa radio queue to get full. The noise floor sampling still happens in startReceive() and after transmitting. * fix lr11x0 current rssi * Address noise floor review comments * Address Copilot SimRadio noise floor comments * Fix RadioLibInterface formatting --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> |
||
|
Ben Meadors
|
784e3748e2 | Merge remote-tracking branch 'origin/master' into develop | ||
|
Ben Meadors
|
220bb4d186 |
Smart pointers and memory management cleanup (#10400)
* Refactor memory management in Syslog and StoreForwardModule * Implement destructor for Lock * Refactor RotaryEncoder and PacketHistory to use smart pointers for better memory management * CH341 should use unique_ptr for improved memory management * Fix checks in PH * Improve Syslog::vlogf to handle variable argument lists more safely * Fix initOk method to use nullptr for null pointer check |
||
|
Jonathan Bennett
|
bb86cf4e81 | Merge remote-tracking branch 'origin/master' into develop | ||
|
|
55f40ecdfd |
Add ulfius webserver support to macos native target (#10366)
* Add ulfius webserver support to macos native target * fix: update PiWebServer docs for macOS and add explicit cstring include Agent-Logs-Url: https://github.com/meshtastic/firmware/sessions/3ce82582-23e0-4afe-b22f-b24f81721488 Co-authored-by: thebentern <9000580+thebentern@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: add --cflags to openssl@3 pkg-config and fix apt package name Agent-Logs-Url: https://github.com/meshtastic/firmware/sessions/1a6c59aa-4393-4134-8cee-61eeee0e9127 Co-authored-by: thebentern <9000580+thebentern@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Ben Meadors
|
7066abbb86 |
Fix MAC_from_string to use input parameter instead of global config for MAC address parsing (#10356)
* Fix MAC_from_string to use input parameter instead of global config for MAC address parsing * Enhance MAC_from_string validation and error handling * Add missing include for <cctype> in PortduinoGlue.cpp |
||
|
|
9c72767c01 |
macOS: enable CH341 LoRa-hardware path (fix serial truncation, document setup) (#10320)
* macOS: enable CH341 LoRa-hardware path — fix serial truncation, document setup
Verified on Apple Silicon with a CH341A USB-SPI bridge (VID 0x1A86,
PID 0x5512) wired to an SX1262 (Meshstick variant) that the existing
`pine64/libch341-spi-userspace` lib_dep works on macOS as-is — Apple's
bundled CH34x driver only matches the CH340 *UART* variant
(PID 0x7523), so the CH341A's interface 0 is left unclaimed and
libusb opens / configures / claims it directly via IOUSBHostInterface.
End-to-end test: meshtasticd boots, libusb claim succeeds, SX1262 init
returns 0, TCP API serves the meshtastic CLI's --info / --sendtext flow.
Two changes:
1. **`PortduinoGlue.cpp:497`**: pass `sizeof(serial)` (= 9) instead of
the literal `8` to `Ch341Hal::getSerialString()`. The function in
`USBHal.h:61-68` treats `len` as buffer size and reserves one slot
for the null terminator (`bytesCopied = (len - 1) < 8 ? (len - 1) : 8`),
so passing 8 produced a 7-char serial — which then broke the
`strlen(serial) == 8` check at line 502, skipping the auto-MAC
derivation from serial + product string. On Linux this was masked
by the BlueZ HCI MAC fallback in `getMacAddr()` at lines 139-157,
but on macOS that fallback is `__linux__`-guarded so the serial path
is mandatory and the truncation left `mac_address` empty, causing
the daemon to exit with `*** Blank MAC Address not allowed!`.
2. **`variants/native/portduino/platformio.ini`**: expand the
`[env:native-macos]` comment block with a "Real LoRa hardware on
macOS" section. Documents:
- Why no upstream library change is needed (Apple kext targets
CH340/UART, not CH341A/SPI; libusb's `#ifdef __linux__` skip is
correct for macOS in this case).
- How to point `meshtasticd` at an existing platform-agnostic
`bin/config.d/lora-*.yaml` for CH341 hardware.
- The auto-MAC-derivation contract (now working with this fix).
- `ioreg` and `LIBUSB_DEBUG=4` diagnostic recipes for the failure
mode where a third-party WCH `CH34xVCPDriver` *would* claim
interface 0 (`kmutil unload -b <bundleID>` workaround).
No upstream library forks, no PR chain, no additional lib_deps —
the existing `pine64/libch341-spi-userspace` + libusb-1.0 stack does
the right thing on macOS already.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Apply suggestion from @Copilot
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
||
|
|
6c7ffa1054 |
macOS: enable CH341 LoRa-hardware path (fix serial truncation, document setup) (#10320)
* macOS: enable CH341 LoRa-hardware path — fix serial truncation, document setup
Verified on Apple Silicon with a CH341A USB-SPI bridge (VID 0x1A86,
PID 0x5512) wired to an SX1262 (Meshstick variant) that the existing
`pine64/libch341-spi-userspace` lib_dep works on macOS as-is — Apple's
bundled CH34x driver only matches the CH340 *UART* variant
(PID 0x7523), so the CH341A's interface 0 is left unclaimed and
libusb opens / configures / claims it directly via IOUSBHostInterface.
End-to-end test: meshtasticd boots, libusb claim succeeds, SX1262 init
returns 0, TCP API serves the meshtastic CLI's --info / --sendtext flow.
Two changes:
1. **`PortduinoGlue.cpp:497`**: pass `sizeof(serial)` (= 9) instead of
the literal `8` to `Ch341Hal::getSerialString()`. The function in
`USBHal.h:61-68` treats `len` as buffer size and reserves one slot
for the null terminator (`bytesCopied = (len - 1) < 8 ? (len - 1) : 8`),
so passing 8 produced a 7-char serial — which then broke the
`strlen(serial) == 8` check at line 502, skipping the auto-MAC
derivation from serial + product string. On Linux this was masked
by the BlueZ HCI MAC fallback in `getMacAddr()` at lines 139-157,
but on macOS that fallback is `__linux__`-guarded so the serial path
is mandatory and the truncation left `mac_address` empty, causing
the daemon to exit with `*** Blank MAC Address not allowed!`.
2. **`variants/native/portduino/platformio.ini`**: expand the
`[env:native-macos]` comment block with a "Real LoRa hardware on
macOS" section. Documents:
- Why no upstream library change is needed (Apple kext targets
CH340/UART, not CH341A/SPI; libusb's `#ifdef __linux__` skip is
correct for macOS in this case).
- How to point `meshtasticd` at an existing platform-agnostic
`bin/config.d/lora-*.yaml` for CH341 hardware.
- The auto-MAC-derivation contract (now working with this fix).
- `ioreg` and `LIBUSB_DEBUG=4` diagnostic recipes for the failure
mode where a third-party WCH `CH34xVCPDriver` *would* claim
interface 0 (`kmutil unload -b <bundleID>` workaround).
No upstream library forks, no PR chain, no additional lib_deps —
the existing `pine64/libch341-spi-userspace` + libusb-1.0 stack does
the right thing on macOS already.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Apply suggestion from @Copilot
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
||
|
|
126861fd16 |
Native MacOS hello world (#10309)
* Native MacOS hello world * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update variants/native/portduino/platformio.ini Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: ensure null-termination in getSerialString() and handle len==0 Agent-Logs-Url: https://github.com/meshtastic/firmware/sessions/e5647919-2255-48ad-bcaa-7a2c2fdbf212 Co-authored-by: thebentern <9000580+thebentern@users.noreply.github.com> --------- Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> |
||
|
|
06a6c3ee20 |
Native MacOS hello world (#10309)
* Native MacOS hello world * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update variants/native/portduino/platformio.ini Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: ensure null-termination in getSerialString() and handle len==0 Agent-Logs-Url: https://github.com/meshtastic/firmware/sessions/e5647919-2255-48ad-bcaa-7a2c2fdbf212 Co-authored-by: thebentern <9000580+thebentern@users.noreply.github.com> --------- Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> |
||
|
Jonathan BennettandBen Meadors
|
d47301defc |
Add PortduinoSetOptions to overwrite the realhardware bool (#10157)
Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
|
23321c4588 |
Reduce key duplication by enabling hardware RNG (#8803)
* Reduce key duplication by enabling hardware RNG * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Use micros() for worst case random seed for nrf52 Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Minor cleanup, remove dead code and clarify comment * trunk * Add useRadioEntropy bool, default false. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> |
||
|
Jonathan BennettandBen Meadors
|
0cab43fb43 |
Add PortduinoSetOptions to overwrite the realhardware bool (#10157)
Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Jonathan Bennett
|
125c1b7f13 | Make consoleInit() Reentrant, and initialize it earlier on native (#10156) | ||
|
|
77f378dd53 |
Reduce key duplication by enabling hardware RNG (#8803)
* Reduce key duplication by enabling hardware RNG * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Use micros() for worst case random seed for nrf52 Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Minor cleanup, remove dead code and clarify comment * trunk * Add useRadioEntropy bool, default false. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> |
||
|
Jonathan BennettandBen Meadors
|
0ed537a336 |
Add json file rotation option (#9783)
Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Jonathan BennettandBen Meadors
|
afd5e29bce |
More RAK6421 work (#9813)
* Add Portduino Enable pins * Add hat plus custom fields * Punt on the GPIO device detection for now * Simplify TX_GAIN_LORA for RAK13302 --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Jonathan BennettandBen Meadors
|
5a068431ed |
Add GPIO_DETECT_PA portduino config, and support 13302 detection with it (#9741)
* Add GPIO_DETECT_PA portduino config, and support 13302 detection with it. * Tweak PA detect gpio to use pinMapping * minor yaml output fixes --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Thomas Göttgens
|
56fd9c7813 |
fix a lot of low level cppcheck warnings (#9623)
* 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 |