Pr1 nodedb warmstore - #10705
Merged
Merged
Conversation
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (24)
Build artifacts expire on 2026-07-18. Updated for |
github-actions
Bot
added
needs-review
NomDeTom
added
2.8
ai-generated
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reworks NodeDB into a tiered storage model by adding a warm (“long-tail”) node tier that preserves minimal identity (notably PKI public keys) for nodes evicted from the hot NodeInfoLite store, and tightens retention/eviction rules for protected nodes (favorite/ignored/verified). It also adds a raw-flash persistence backend for the warm tier on nRF52840 and caps satellite-map growth to bound memory and nodes.db size.
Changes:
- Add
WarmNodeStore(RAM + persistence) and integrate it intoNodeDBload/evict/cleanup flows, plusRouterPKI key resolution viaNodeDB::copyPublicKey(). - Enforce satellite-map caps (
MAX_SATELLITE_NODES) and protected-node cap (MAX_NUM_NODES - 2) with user-facing warnings in some admin/UI paths. - Add unit tests for warm-tier policy and blocked/protected-node eviction/migration behavior; add nRF52840 linker/guard tooling to reserve flash for the warm ring.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| variants/nrf52840/nrf52840.ini | Force capped linker script for S140 v6 boards to keep the warm raw-flash region clear. |
| variants/nrf52840/nrf52.ini | Add post-link guard script to prevent images from overlapping the warm-store flash reservation. |
| extra_scripts/nrf52_warm_region.py | Post-link nm-based guard ensuring the firmware image ends below the reserved warm-store region. |
| src/platform/nrf52/nrf52840_s140_v6.ld | New linker script variant capping FLASH length to protect the warm-store ring area (S140 v6 layout). |
| src/platform/nrf52/nrf52840_s140_v7.ld | Cap FLASH length for S140 v7 layout to protect the warm-store ring area. |
| src/mesh/WarmNodeStore.h | Define warm-tier entry format, policies, and nRF52840 ring layout constants. |
| src/mesh/WarmNodeStore.cpp | Implement warm-tier admission/eviction, persistence (raw-flash ring vs /prefs/warm.dat), and replay. |
| src/mesh/NodeDB.h | Add warm-tier member + copyPublicKey(), setProtectedFlag(), and related helpers. |
| src/mesh/NodeDB.cpp | Integrate warm tier into eviction/migration/cleanup/reset flows; add satellite caps; add protected-cap enforcement. |
| src/mesh/Router.cpp | Resolve PKI keys via NodeDB::copyPublicKey() so long-tail nodes can still encrypt/decrypt DMs. |
| src/modules/AdminModule.cpp | Route favorite/ignore through setProtectedFlag(); allow blocking unknown nodes via getOrCreateMeshNode(). |
| src/graphics/draw/MenuHandler.cpp | Use setProtectedFlag() for ignore toggling (cap-aware). |
| src/mesh/mesh-pb-constants.h | Set new defaults for MAX_NUM_NODES, add MAX_SATELLITE_NODES, and define WARM_NODE_COUNT per platform. |
| src/mesh/generated/meshtastic/deviceonly.pb.h | Add persisted snr_q4 field to NodeInfoLite generated struct. |
| test/test_warm_store/test_main.cpp | New unit tests for warm-store admission/eviction/take/persistence behavior. |
| test/test_nodedb_blocked/test_main.cpp | New unit tests for hot-store migration and favorite/ignored retention + protected cap behavior. |
NomDeTom
force-pushed
the
pr1-nodedb-warmstore
branch
from
June 13, 2026 15:26
7c1e1c3 to
17ada5b
Compare
Contributor
Firmware Size Report22 targets | vs
Show 17 more target(s)
Updated for ca08228 |
NomDeTom
force-pushed
the
pr1-nodedb-warmstore
branch
from
June 16, 2026 09:47
0770379 to
20114e1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/mesh/NodeDB.cpp:3252
- NodeDB::getOrCreateMeshNode() will happily create an entry for NodeNum 0. Across the codebase, node num 0 is used as a sentinel for “local” (e.g. MeshPacket.from == 0), so allowing 0 into the hot store can create a bogus protected/ignored node, consume a slot, and potentially trigger avoidable evictions (now that AdminModule can create nodes by ID). It should defensively reject n==0 up front.
meshtastic_NodeInfoLite *NodeDB::getOrCreateMeshNode(NodeNum n)
{
meshtastic_NodeInfoLite *lite = getMeshNode(n);
if (!lite) {
…ty retention)
Introduces a tiered NodeDB so the device retains identity (public key,
last_heard) for far more nodes than fit in the full-record hot store,
without growing heap or the persisted nodes.proto unboundedly.
- Hot store: full NodeInfoLite, MAX_NUM_NODES (120 on nRF52).
- Satellite maps: position/telemetry/environment/status capped at
MAX_SATELLITE_NODES (40 freshest); eviction via enforceSatelliteCaps /
evictSatelliteOverCap.
- Warm tier (WarmNodeStore): 40 B {num,last_heard,public_key} records for
evicted nodes so DMs to/from long-tail nodes keep encrypting/decrypting.
Persisted to /prefs/warm.dat, or on nRF52840 a dedicated 12 KB raw-flash
record-ring below LittleFS (3x4 KB pages; see linker scripts + the
nrf52_warm_region.py post-link guard).
NodeDB::getOrCreateMeshNode now demotes evicted nodes into the warm tier and
re-admits them (restoring key/last_heard). Router PKI decrypt/encode resolve
the peer key via NodeDB::copyPublicKey (hot store, then warm tier).
NodeInfoLite gains snr_q4 (sint32, Q4-encoded dB); the float snr is zeroed on
disk. NodeInfoLite grows 105 -> 112 B; backup 2432 -> 2468 B.
Note: the snr_q4 .proto change still needs to land in the protobufs submodule
(generated header is updated here; submodule pointer left at upstream).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hardens how ignored/favourite nodes are received over admin and retained, closing paths where a block could be lost or accidentally cleared. - Blocking keeps the node's public key (admin set_ignored_node and addFromContact no longer zero it / drop the warm-tier key), so a blocked peer stays a verifiable identity. - set_ignored_node creates the node if absent, so a block by node ID sticks even for a node we've never heard from (e.g. pushed by a remote admin) with no NodeInfo or key. - Eviction protection (favourite/ignored/manually-verified) now also applies to the load-time hot-store migration and is never undone by cleanupMeshDB, which previously purged ignored nodes that lacked user info. - The hot-store migration leaves our own node (index 0) in place and prefers to demote non-protected nodes, like the runtime eviction scan. Caps the protected set (favourite + ignored + verified) at MAX_NUM_NODES-2 via NodeDB::setProtectedFlag(), so at least two evictable slots always remain and getOrCreateMeshNode can always make room — replacing the previous unconditional append that could run off the end of the node vector when every node was protected. A locally-set favourite/ignore that hits the cap reports back to the phone via a ClientNotification. Adds test_nodedb_blocked covering the migration, favourite/ignored eviction protection, ignored-survives-cleanup, and the protected-node cap. The maintenance methods stay private in production; the test reaches them through a PIO_UNIT_TESTING-guarded friend shim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> # Conflicts: # src/mesh/NodeDB.h
Zero-initialise `stranded[]` and `seqs[]/order[]` VLAs so cppcheck can verify there are no unguarded reads of uninitialised memory (the guards exist but are not visible to static analysis). Mark two local pointers `const` where the pointed-to entry is never mutated after assignment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cvaldess
added a commit
to cvaldess/firmware
that referenced
this pull request
Jun 20, 2026
…NT=0) The warmstore (meshtastic#10705) reboots RP2350/W5500 boards via the 8s hardware watchdog when a full NodeDB (120) save is followed by the extra warm.dat write. RP2350 has no dedicated branch in the per-platform WARM_NODE_COUNT selector (src/mesh/mesh-pb-constants.h) so it inherits the generic #else (320). Disable the warm tier on both W5500 variants via WARM_NODE_COUNT=0 (compiles clean thanks to the new #if WARM_NODE_COUNT > 0 guards). Validated on-hardware (wiznet_5500_evb_pico2_e22p): DB filled to 120, the exact eviction-at-full sequence fired, board survived with no watchdog reboot. See firmware#10746. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
getting hard reboots from this PR on ESP32-S3 nodes with no PSRAM like the Wireless Paper. Crash happens when connected to Bluetooth or attempting to connect. It's chocking heap @NomDeTom |
Contributor
|
clanker suggests
|
thebentern
pushed a commit
that referenced
this pull request
Jun 22, 2026
…10759) * fix: right-size warm tier for constrained platforms and feed RP2040 watchdog during NodeDB save * fix: size warm tier and traffic cache per-MCU RAM, lowering no-PSRAM ESP32 (classic/S2/C3) tiers * docs: document nRF52/RP2040 #else fall-through in warm tier and TM cache cascades
raghumad
pushed a commit
to raghumad/mezulla-firmware
that referenced
this pull request
Jun 25, 2026
* NodeDB: 3-tier node store with persistent warm tier (long-tail identity retention)
Introduces a tiered NodeDB so the device retains identity (public key,
last_heard) for far more nodes than fit in the full-record hot store,
without growing heap or the persisted nodes.proto unboundedly.
- Hot store: full NodeInfoLite, MAX_NUM_NODES (120 on nRF52).
- Satellite maps: position/telemetry/environment/status capped at
MAX_SATELLITE_NODES (40 freshest); eviction via enforceSatelliteCaps /
evictSatelliteOverCap.
- Warm tier (WarmNodeStore): 40 B {num,last_heard,public_key} records for
evicted nodes so DMs to/from long-tail nodes keep encrypting/decrypting.
Persisted to /prefs/warm.dat, or on nRF52840 a dedicated 12 KB raw-flash
record-ring below LittleFS (3x4 KB pages; see linker scripts + the
nrf52_warm_region.py post-link guard).
NodeDB::getOrCreateMeshNode now demotes evicted nodes into the warm tier and
re-admits them (restoring key/last_heard). Router PKI decrypt/encode resolve
the peer key via NodeDB::copyPublicKey (hot store, then warm tier).
NodeInfoLite gains snr_q4 (sint32, Q4-encoded dB); the float snr is zeroed on
disk. NodeInfoLite grows 105 -> 112 B; backup 2432 -> 2468 B.
Note: the snr_q4 .proto change still needs to land in the protobufs submodule
(generated header is updated here; submodule pointer left at upstream).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* NodeDB: robust receive + retention for blocked (ignored) nodes
Hardens how ignored/favourite nodes are received over admin and retained,
closing paths where a block could be lost or accidentally cleared.
- Blocking keeps the node's public key (admin set_ignored_node and
addFromContact no longer zero it / drop the warm-tier key), so a blocked
peer stays a verifiable identity.
- set_ignored_node creates the node if absent, so a block by node ID sticks
even for a node we've never heard from (e.g. pushed by a remote admin) with
no NodeInfo or key.
- Eviction protection (favourite/ignored/manually-verified) now also applies to
the load-time hot-store migration and is never undone by cleanupMeshDB, which
previously purged ignored nodes that lacked user info.
- The hot-store migration leaves our own node (index 0) in place and prefers to
demote non-protected nodes, like the runtime eviction scan.
Caps the protected set (favourite + ignored + verified) at MAX_NUM_NODES-2 via
NodeDB::setProtectedFlag(), so at least two evictable slots always remain and
getOrCreateMeshNode can always make room — replacing the previous unconditional
append that could run off the end of the node vector when every node was
protected. A locally-set favourite/ignore that hits the cap reports back to the
phone via a ClientNotification.
Adds test_nodedb_blocked covering the migration, favourite/ignored eviction
protection, ignored-survives-cleanup, and the protected-node cap. The
maintenance methods stay private in production; the test reaches them through a
PIO_UNIT_TESTING-guarded friend shim.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts:
# src/mesh/NodeDB.h
* fix copilot comments
* once again
* WarmNodeStore: fix cppcheck warnings (uninitvar, constVariablePointer)
Zero-initialise `stranded[]` and `seqs[]/order[]` VLAs so cppcheck can
verify there are no unguarded reads of uninitialised memory (the guards
exist but are not visible to static analysis). Mark two local pointers
`const` where the pointed-to entry is never mutated after assignment.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* self-care added to assist 2.7 and 2.8 nodedb migration
* Tidy warm-store/self-care: comments, guards, log + flash cleanup
Style/cleanup pass over the branch (no behavior change except the noted
preprocessor simplifications, which are semantically identical):
- Comments: move function descriptions to the headers, cap in-function
comments at ~3-4 lines, drop leading-number step markers, label stacked
#endif blocks, de-decorate banner comments.
- dumpToLog: fully gate decl + definition + AdminModule call site behind
MESHTASTIC_NODEDB_MIGRATION_VERBOSE so it compiles out when disabled
(~1.2 KB when off).
- mesh-pb-constants: drop the dead nRF52832 WARM_NODE_COUNT branch and trim
the macro docs.
- WarmNodeStore: simplify the redundant `ARCH_NRF52 && NRF52840_XXAA` guards
to `NRF52840_XXAA`, add a kNoPage sentinel for the ring page state.
- Shorten the always-on LOG_WARN strings (~120 B flash).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* more tidying up, aligning with docs and undoing other-arch regressions
* Update protobufs (meshtastic#19)
Co-authored-by: NomDeTom <116762865+NomDeTom@users.noreply.github.com>
* made the migration pathway cleareer
* address copilot review
* fixed a copilot review on a downstream PR.
* Address Copilot review comments for PR meshtastic#10705 (warmstore/nodedb)
- WarmNodeStore.h: default MIGRATION_VERBOSE to 0 (suppress info-level
chatter on production builds; opt in with =1)
- WarmNodeStore.cpp load(): move memset to top of function so all
failure paths (header-read fail, invalid header) leave entries clear
- WarmNodeStore.cpp save(): replace manual spiLock lock/unlock around
mkdir with LockGuard covering the full SafeFile sequence, matching
the lock discipline in load()
- Router.cpp: memcpy(&p->public_key.bytes, ...) -> memcpy(p->public_key.bytes,
...) — pass decayed uint8_t* rather than pointer-to-array
- AdminModule.cpp: check setProtectedFlag return for PKC auto-favorite;
log cap-refusal warning instead of unconditional "auto-favoriting"
- nrf52_warm_region.py: error message references both v6.ld and v7.ld
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* NodeDB: formatting cleanup (blank lines after preprocessor blocks)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Lukewarm store
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
raghumad
pushed a commit
to raghumad/mezulla-firmware
that referenced
this pull request
Jun 25, 2026
…meshtastic#10705) (meshtastic#10759) * fix: right-size warm tier for constrained platforms and feed RP2040 watchdog during NodeDB save * fix: size warm tier and traffic cache per-MCU RAM, lowering no-PSRAM ESP32 (classic/S2/C3) tiers * docs: document nRF52/RP2040 #else fall-through in warm tier and TM cache cascades
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 24, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 24, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 25, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 26, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 26, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 27, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
pushed a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 30, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 30, 2026
* reconfigure tests * Narrow radio-reload trigger to LoRa-affecting saves only reloadConfig() inferred "the radio needs reconfiguring" from whether saveWhat included SEGMENT_CONFIG. But Config is a monolithic segment (device/position/power/network/display/lora/bluetooth/security share one file), so every non-LoRa AdminModule save still fired the live SX126x reconfigure - the same live-SPI-on-the-admin-thread sequence that crashes on a favorite (meshtastic#11146), just reached via a Bluetooth toggle, WiFi PSK change, or keypair rotation. Separate the two concerns: add an explicit radioAffected flag threaded through AdminModule::saveChanges() into MeshService::reloadConfig(). The gate becomes `radioAffected && (saveWhat & (SEGMENT_CONFIG|SEGMENT_CHANNELS))` - the bitmask inference is kept as a backstop and only suppressible, and radioAffected defaults to true so the ~35 existing reloadConfig() callers (MenuHandler, MenuApplet, portduino) keep today's behavior untouched. Only handleSetConfig()'s lora_tag sets radioAffected; every other sub-message, plus set_fixed_position/remove_fixed_position and position's nested save, opt out. set_channel/commit_edit_settings/restore keep the default and still reload. Add native coverage in test_admin_radio for each non-LoRa sub-message (asserting no reconfigure), regression guards that lora/set_channel still reconfigure, and direct reloadConfig() guards pinning the fail-safe default. * mark up todo sites for checking reloadConfig use * added clarifying note * Don't reboot on no-op position/network/bluetooth config sets handleSetConfig() starts requiresReboot=true and device/power/display each gate it back down when nothing reboot-worthy changed, but position, network, and bluetooth never touched requiresReboot - so they rebooted on *every* set, including a client re-pushing byte-identical config (which happens routinely on connect/retry). Add a no-op gate to those three: if the incoming sub-message is byte-identical to the current one, skip the reboot. A whole-struct memcmp is the right tool here (unlike the field-by-field device/power/display gates, which must ignore benign fields) - it answers "did anything change?" and fails safe: its only error mode is padding-byte differences causing an unnecessary reboot, never a missed change. All three sub-messages are POD (no pb_callback_t fields). Any real change still reboots exactly as before. This is Tier 1 of plan-narrow-reboot-trigger; Tier 2 will further narrow position to reboot only on boot-only fields (GPIO/GPS). Adds native coverage asserting rebootAtMsec stays unset on a no-op set and is armed on a real change, for each of the three sub-messages. * Apply live position config changes without a reboot Tier 2 of plan-narrow-reboot-trigger. A position set that touches only fields the position module consumes live now applies without restarting. PositionModule reads position_broadcast_secs, position_broadcast_smart_enabled, broadcast_smart_minimum_distance, position_flags, and fixed_position directly from config on every send/schedule cycle (fixed_position also has dedicated live admin handlers) - changing only those needs no reboot. Everything else stays on the reboot path: GPS driver state (gps_mode/gps_enabled/ gps_update_interval/gps_attempt_time) and GPIO pin assignments (rx_gpio/ tx_gpio/gps_en_gpio), all of which touch subsystem/hardware init. The live set is deliberately limited to what static analysis proves is applied live, so this ships without hardware verification; GPS-timing fields that might also be live were left rebooting (fail toward current behavior). The gate neutralizes the live fields in a copy and reboots if any other byte differs, so a future PositionConfig field reboots until explicitly cleared as live - fail safe for schema growth. This supersedes the Tier 1 no-op memcmp gate for position (network/bluetooth keep theirs). Native coverage: a broadcast-interval change does not schedule a reboot; gps_mode and rx_gpio changes still do. * docs: config-save radio-reload & reboot gating Document the AdminModule config-save side-effect work: the radioAffected and requiresReboot axes, which operations now skip the radio reload or the reboot, and the operations deliberately left unchanged (commit_edit_settings, network/bluetooth live-apply, GPS-timing position fields, module config, and the on-device menu reloadConfig sites) with the reason for each. * docs: add hardware testing section to config-save gating The doc only mentioned the outstanding GPS-timing pass in one line and omitted the radio-reload/crash validation entirely, with no procedure. Add a Hardware testing section covering: the meshtastic-mcp setup (BLE connected throughout, nRF52840 SX126x reference board); the radio-reload/crash regression guard for the favorite-node fix (with a lora/set_channel positive control); and a per-field procedure with pass/fail criteria for deciding whether GPS-timing position fields can be reclassified as live - fail toward rebooting. * docs: clarify hardware tests run over serial, not menus/BLE The hardware section implied a BLE connection was required and that the crash ran on the BLE callback thread. On nRF52 the BLE onWrite only queues; handleToRadio -> saveChanges -> reloadConfig runs on the main FreeRTOS task, same context SerialConsole uses. So serial drives the exact code/thread under test, the on-device menus are unrelated (separate path), and the original crash was serial-proven. State that serial is sufficient and correct the thread references accordingly. * docs: correct crash mechanism to the off-main-thread spiLock lockup The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent). * docs: clarify config save behavior for GPS position updates * menu actions * menu actions reboot * Persist telemetry screen toggles from the frame menu The three moduleConfig.telemetry.*_screen_enabled toggles in the frame-toggle menu changed the value in memory and never wrote it, so they reverted on any reboot that did not happen to go through the reboot menu (which saves every segment). Their sibling entries in the same menu persist via Screen::toggleFrameVisibility -> saveFrameVisibility, so half the menu was durable and three entries were not. These three live in moduleConfig rather than the hiddenFrames blob, so they need their own SEGMENT_MODULECONFIG save. radioAffected is false: a screen preference has no business re-initialising the LoRa chip. clod helped too * Drop redundant and over-broad config writes from the menus reloadConfig() ends with an unconditional saveToDisk(saveWhat), outside the radioAffected guard, so nine sites in the InkHUD menu were writing the same proto file twice in a row: saveToDisk(X) immediately followed by reloadConfig(X, ...). One of the nine is the shared applyConfigReload() helper, so thirteen menu actions were affected. Five sites in the BaseUI menu called saveUIConfig() after changing a field that lives in config.proto. saveUIConfig() only writes /prefs/uiconfig.proto, so those writes could not persist the changed field and did nothing but cost a flash write. GPSFormatMenu had the mirror image: a uiconfig-only change that also called reloadConfig(), rewriting config.proto for a field not in it. Three bare saveToDisk() calls rewrote all five segments to change one bit - a channel mute flag and two NodeInfoLite bits - so they now pass the segment they actually touch. NodeInfoLite is written by saveNodeDatabaseToDisk() under SEGMENT_NODEDATABASE, not SEGMENT_DEVICESTATE. The reboot menu keeps its bare call: a full flush before a deliberate reboot is correct. reloadConfig() becomes virtual so tests can count calls; the accompanying test pins down the saveToDisk equivalence the nine deletions rely on. clod helped too * Route config saves through one MeshService::applyConfigChange helper Applying a config change involves three independent decisions: which proto files to persist, whether the LoRa chip needs re-initialising, and whether the field only takes effect after a restart. Those were spread across four helpers with three different parameter orders, two of which took adjacent bools that compile fine when transposed. InkHUD's applyConfigReload was the sharp edge: its second parameter was `reboot`, sitting exactly where reloadConfig and saveChanges take `radioAffected`. There is now a single entry point taking a flags enum, so each call site states its intent and cannot silently mean the opposite. Migrated 23 BaseUI sites, 20 InkHUD sites and the wasm glue. applyConfigReload is deleted and its five callers inlined; applyLoRaRegion and applyLoRaPreset stay, since they hold real domain logic beyond bundling, and just delegate. Adjacent `rebootAtMsec = millis() + ...` assignments fold into the reboot flag. The one caller that deliberately uses a shorter delay keeps it via the trailing rebootSeconds parameter. Reboot scheduling itself moves into requestReboot() in main.cpp, next to the global it sets: previously only AdminModule had a helper for this, and it was private, which is why every menu open-coded the deadline. requestReboot carries no UI, because BaseUI already renders the notice at draw time whenever rebootAtMsec is set. saveChanges keeps its signature and its edit-transaction deferral, so its fourteen call sites and the existing gating tests are untouched. clod helped too * Extract menu config actions into testable functions None of the menu save behaviour was reachable from a test: every action lived in a lambda assigned to BannerOverlayOptions.bannerCallback, which only ever runs via screen->showOverlayBanner(), so exercising one needed a live Screen. That is why the defects this series fixes went unnoticed - MenuHandler.cpp compiles in the native test build, but nothing in it could be called. Three actions move out into functions that own the whole decision: which segment to persist, whether the radio needs reconfiguring, and whether to reboot. Chosen because each covers a defect this series touched - the telemetry toggles that never persisted, the smart-position reboot that PR meshtastic#11181 removed, and a node-DB bit write that must not reach the radio. Deliberately picked extractions that also deduplicate: three telemetry call sites collapse onto one function and two smart-position sites onto another, so the promicro image is byte-identical to before. Worth knowing, because that board has under 1.4 KB of headroom before the warmstore region. toggleNodeMuted also gains a guard for an unknown node, so a stale pickedNodeNum can no longer cause a pointless flash write. clod helped too * Sort out InkHUD applying-changes notifications notifyApplyingChanges() was doing two jobs at its call sites in MenuApplet.cpp: signalling a live change that is applied without restarting, and warning of an imminent reboot. Audited every site. The finding is that none of them are redundant, so this records why rather than removing anything. The two live-change calls - LoRa region and modem preset - cover the seconds the e-ink takes to redraw while the radio reconfigures, and nothing else raises them. The reboot-path calls warn before the display goes. requestReboot() cannot absorb those: it deliberately carries no UI, because BaseUI renders its own notice at draw time from rebootAtMsec, while e-ink only draws when pushed. Also worth recording: the existing notifyReboot Observable (sleep.h, fired from Power::reboot) is a different moment - reboot execution, not scheduling - and InkHUD already observes it to save settings and shut applets down. A new "reboot scheduled" observable to centralise these calls would be a third reboot signal for no functional gain, so it is not added. clod helped too * stylee * Fix stale doc references and document the menu path Addresses review feedback. Four comments pointed at plan documents that were never committed (plan-narrow-reboot-trigger.md, plan-decouple-nodedb-admin-saves.md), so the rationale they cited was not discoverable. They now point at docs/admin-config-save-gating.md, which is in-repo. The doc listed the commit SHAs it described. All five had already been orphaned by the rebases this branch has been through, exactly as predicted, so it cites the PR instead. "Status: Implemented" also overstated things while hardware validation is still outstanding. The doc's biggest problem was that it had gone out of date within its own PR: it described the on-device menus as an untouched code path that still reloads the radio on any Config save, with per-site TODO markers. That stopped being true when the menus moved onto applyConfigChange. Replaced with a section covering the new entry point, the flags, why they are a flags enum rather than two bools, and the saveToDisk equivalence that makes pairing the two calls a double write. clod helped too * Fix smart-broadcast-interval reboot and carry transaction flags Four review findings. broadcast_smart_minimum_interval_secs is not read live, despite the comment this branch added claiming it was. PositionModule::minimumTimeThreshold is a const data member initialised when the module is constructed, so the value is captured at boot; only the sibling distance field is genuinely re-read (PositionModule.cpp :630). The InkHUD menu therefore applied it with no reboot and it silently did nothing until the next restart. It now reboots, matching the AdminModule path. Note the review suggested the opposite fix - adding the field to AdminModule's live-field list - which would have made the admin path silently ineffective too. While an edit transaction is open saveChanges() defers the write, which threw away the per-field reboot and radio decisions; the commit then used the parameter defaults and always rebooted and reconfigured the radio. Since phone apps write config through transactions, none of this branch's narrowing reached them. The deferred decisions now accumulate and the commit honours them. Two test fixes: the set_ignored_node test relied on an earlier RUN_TEST having created the node, which breaks under -f or a registration reorder. And the telemetry test asserted only values it had just assigned - replaced with one that drives the real toggle and checks the save path sets has_telemetry, the flag whose absence caused the TAK persistence bug fixed upstream in meshtastic#11216. clod helped too * Stop the edit transaction discarding the per-field save decisions Review of this branch's own output, plus the two findings Copilot raised on The headline defect is that none of this branch's narrowing reached phone apps. saveChanges() defers while an edit transaction is open, and radioAffected defaulted to true, so eight call sites that never thought about the radio - the five node-DB handlers, set_owner, set_module_config, and the nested save in the position case - accumulated a true into deferredRadioAffected. Outside a transaction that was harmless, because reloadConfig()'s saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS) bitmask independently blocks a node-DB-only save from reaching the radio whatever it asks for. Inside one the commit saves under a fixed full mask, so the bitmask always passes and radioAffected is the only thing left deciding it. Favouriting a node from the phone app therefore still ran the live SX126x reconfigure at commit - the The fix is to remove the default from saveChanges() rather than gate the deferred flag on the segment mask, which is what the review suggested. Gating would reinstate exactly the segment-to-radio inference this branch exists to delete - NodeDB.h now carries a comment telling the next person not to do that - and it fixes the symptom while leaving the wrong default in place for the next call site. With no default the compiler makes all fourteen sites state the answer. The commit also consumes and clears the deferred flags, so a stray second commit cannot inherit the previous transaction's answer. That gap existed because every radio test ran outside a transaction, which is the one arrangement where the bug is unreachable. Nine tests now cover the deferred path, asserting both axes independently across a commit. Verified they fail against the old behaviour: five go red while every pre-existing test stays green, which is the point. requestReboot()'s comment had the semantics backwards - it claimed a negative delay meant "now" and that rebootAtMsec == 0 was an immediate-reboot sentinel. Both are inverted: 0 means no reboot pending at every read site, and admin.proto documents reboot_seconds "<0 to cancel reboot". The expression was a faithful copy of AdminModule's, so only the comment was wrong, but it was wrong on the newly-created central helper. The negative branch now says so and logs it. Five sites still open-coded the deadline despite that helper existing; they are pure reboots with no config save, so applyConfigChange() does not fit but requestReboot() does exactly. SET_SMART_BROADCAST_INTERVAL was the only CONFIG_APPLY_REBOOT site in the InkHUD menu without a notifyApplyingChanges() beside it - re-adding its reboot last commit dropped the warning that applyConfigReload() used to raise, so the e-ink would go dark unannounced. And four channel actions carried CONFIG_APPLY_RADIO for uplink/downlink and position_precision, none of which touch the name, PSK or frequency slot the radio derives anything from; they had it only because the old reloadConfig(SEGMENT_CHANNELS) inferred it from the bitmask. The doc had gone stale inside its own PR again: it still listed commit_edit_settings under "intentionally left unchanged". Replaced with a section on why the bitmask stops protecting you inside a transaction, since that is the non-obvious part. Also dropped a commit SHA that had already been orphaned by rebase, exactly as its own note predicted, and a stale plan-doc reference in the tests that the last doc pass missed. Full native suite green, 810 cases across 40 suites. t-echo and t-echo-inkhud both build, covering the menu changes no native env compiles. * post rebase fixes * gps-toggle-noreboot * fix(admin): preserve live config transactions * fix(admin): avoid unnecessary config restarts * fix(admin): keep edit timeout dormant while idle * fix(admin): skip normalized no-op reboots * fix(admin): disable idle transaction timer --------- Co-authored-by: nomdetom <nomdetom@protonmail.com>
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 30, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 30, 2026
* reconfigure tests * Narrow radio-reload trigger to LoRa-affecting saves only reloadConfig() inferred "the radio needs reconfiguring" from whether saveWhat included SEGMENT_CONFIG. But Config is a monolithic segment (device/position/power/network/display/lora/bluetooth/security share one file), so every non-LoRa AdminModule save still fired the live SX126x reconfigure - the same live-SPI-on-the-admin-thread sequence that crashes on a favorite (meshtastic#11146), just reached via a Bluetooth toggle, WiFi PSK change, or keypair rotation. Separate the two concerns: add an explicit radioAffected flag threaded through AdminModule::saveChanges() into MeshService::reloadConfig(). The gate becomes `radioAffected && (saveWhat & (SEGMENT_CONFIG|SEGMENT_CHANNELS))` - the bitmask inference is kept as a backstop and only suppressible, and radioAffected defaults to true so the ~35 existing reloadConfig() callers (MenuHandler, MenuApplet, portduino) keep today's behavior untouched. Only handleSetConfig()'s lora_tag sets radioAffected; every other sub-message, plus set_fixed_position/remove_fixed_position and position's nested save, opt out. set_channel/commit_edit_settings/restore keep the default and still reload. Add native coverage in test_admin_radio for each non-LoRa sub-message (asserting no reconfigure), regression guards that lora/set_channel still reconfigure, and direct reloadConfig() guards pinning the fail-safe default. * mark up todo sites for checking reloadConfig use * added clarifying note * Don't reboot on no-op position/network/bluetooth config sets handleSetConfig() starts requiresReboot=true and device/power/display each gate it back down when nothing reboot-worthy changed, but position, network, and bluetooth never touched requiresReboot - so they rebooted on *every* set, including a client re-pushing byte-identical config (which happens routinely on connect/retry). Add a no-op gate to those three: if the incoming sub-message is byte-identical to the current one, skip the reboot. A whole-struct memcmp is the right tool here (unlike the field-by-field device/power/display gates, which must ignore benign fields) - it answers "did anything change?" and fails safe: its only error mode is padding-byte differences causing an unnecessary reboot, never a missed change. All three sub-messages are POD (no pb_callback_t fields). Any real change still reboots exactly as before. This is Tier 1 of plan-narrow-reboot-trigger; Tier 2 will further narrow position to reboot only on boot-only fields (GPIO/GPS). Adds native coverage asserting rebootAtMsec stays unset on a no-op set and is armed on a real change, for each of the three sub-messages. * Apply live position config changes without a reboot Tier 2 of plan-narrow-reboot-trigger. A position set that touches only fields the position module consumes live now applies without restarting. PositionModule reads position_broadcast_secs, position_broadcast_smart_enabled, broadcast_smart_minimum_distance, position_flags, and fixed_position directly from config on every send/schedule cycle (fixed_position also has dedicated live admin handlers) - changing only those needs no reboot. Everything else stays on the reboot path: GPS driver state (gps_mode/gps_enabled/ gps_update_interval/gps_attempt_time) and GPIO pin assignments (rx_gpio/ tx_gpio/gps_en_gpio), all of which touch subsystem/hardware init. The live set is deliberately limited to what static analysis proves is applied live, so this ships without hardware verification; GPS-timing fields that might also be live were left rebooting (fail toward current behavior). The gate neutralizes the live fields in a copy and reboots if any other byte differs, so a future PositionConfig field reboots until explicitly cleared as live - fail safe for schema growth. This supersedes the Tier 1 no-op memcmp gate for position (network/bluetooth keep theirs). Native coverage: a broadcast-interval change does not schedule a reboot; gps_mode and rx_gpio changes still do. * docs: config-save radio-reload & reboot gating Document the AdminModule config-save side-effect work: the radioAffected and requiresReboot axes, which operations now skip the radio reload or the reboot, and the operations deliberately left unchanged (commit_edit_settings, network/bluetooth live-apply, GPS-timing position fields, module config, and the on-device menu reloadConfig sites) with the reason for each. * docs: add hardware testing section to config-save gating The doc only mentioned the outstanding GPS-timing pass in one line and omitted the radio-reload/crash validation entirely, with no procedure. Add a Hardware testing section covering: the meshtastic-mcp setup (BLE connected throughout, nRF52840 SX126x reference board); the radio-reload/crash regression guard for the favorite-node fix (with a lora/set_channel positive control); and a per-field procedure with pass/fail criteria for deciding whether GPS-timing position fields can be reclassified as live - fail toward rebooting. * docs: clarify hardware tests run over serial, not menus/BLE The hardware section implied a BLE connection was required and that the crash ran on the BLE callback thread. On nRF52 the BLE onWrite only queues; handleToRadio -> saveChanges -> reloadConfig runs on the main FreeRTOS task, same context SerialConsole uses. So serial drives the exact code/thread under test, the on-device menus are unrelated (separate path), and the original crash was serial-proven. State that serial is sufficient and correct the thread references accordingly. * docs: correct crash mechanism to the off-main-thread spiLock lockup The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent). * docs: clarify config save behavior for GPS position updates * menu actions * menu actions reboot * Persist telemetry screen toggles from the frame menu The three moduleConfig.telemetry.*_screen_enabled toggles in the frame-toggle menu changed the value in memory and never wrote it, so they reverted on any reboot that did not happen to go through the reboot menu (which saves every segment). Their sibling entries in the same menu persist via Screen::toggleFrameVisibility -> saveFrameVisibility, so half the menu was durable and three entries were not. These three live in moduleConfig rather than the hiddenFrames blob, so they need their own SEGMENT_MODULECONFIG save. radioAffected is false: a screen preference has no business re-initialising the LoRa chip. clod helped too * Drop redundant and over-broad config writes from the menus reloadConfig() ends with an unconditional saveToDisk(saveWhat), outside the radioAffected guard, so nine sites in the InkHUD menu were writing the same proto file twice in a row: saveToDisk(X) immediately followed by reloadConfig(X, ...). One of the nine is the shared applyConfigReload() helper, so thirteen menu actions were affected. Five sites in the BaseUI menu called saveUIConfig() after changing a field that lives in config.proto. saveUIConfig() only writes /prefs/uiconfig.proto, so those writes could not persist the changed field and did nothing but cost a flash write. GPSFormatMenu had the mirror image: a uiconfig-only change that also called reloadConfig(), rewriting config.proto for a field not in it. Three bare saveToDisk() calls rewrote all five segments to change one bit - a channel mute flag and two NodeInfoLite bits - so they now pass the segment they actually touch. NodeInfoLite is written by saveNodeDatabaseToDisk() under SEGMENT_NODEDATABASE, not SEGMENT_DEVICESTATE. The reboot menu keeps its bare call: a full flush before a deliberate reboot is correct. reloadConfig() becomes virtual so tests can count calls; the accompanying test pins down the saveToDisk equivalence the nine deletions rely on. clod helped too * Route config saves through one MeshService::applyConfigChange helper Applying a config change involves three independent decisions: which proto files to persist, whether the LoRa chip needs re-initialising, and whether the field only takes effect after a restart. Those were spread across four helpers with three different parameter orders, two of which took adjacent bools that compile fine when transposed. InkHUD's applyConfigReload was the sharp edge: its second parameter was `reboot`, sitting exactly where reloadConfig and saveChanges take `radioAffected`. There is now a single entry point taking a flags enum, so each call site states its intent and cannot silently mean the opposite. Migrated 23 BaseUI sites, 20 InkHUD sites and the wasm glue. applyConfigReload is deleted and its five callers inlined; applyLoRaRegion and applyLoRaPreset stay, since they hold real domain logic beyond bundling, and just delegate. Adjacent `rebootAtMsec = millis() + ...` assignments fold into the reboot flag. The one caller that deliberately uses a shorter delay keeps it via the trailing rebootSeconds parameter. Reboot scheduling itself moves into requestReboot() in main.cpp, next to the global it sets: previously only AdminModule had a helper for this, and it was private, which is why every menu open-coded the deadline. requestReboot carries no UI, because BaseUI already renders the notice at draw time whenever rebootAtMsec is set. saveChanges keeps its signature and its edit-transaction deferral, so its fourteen call sites and the existing gating tests are untouched. clod helped too * Extract menu config actions into testable functions None of the menu save behaviour was reachable from a test: every action lived in a lambda assigned to BannerOverlayOptions.bannerCallback, which only ever runs via screen->showOverlayBanner(), so exercising one needed a live Screen. That is why the defects this series fixes went unnoticed - MenuHandler.cpp compiles in the native test build, but nothing in it could be called. Three actions move out into functions that own the whole decision: which segment to persist, whether the radio needs reconfiguring, and whether to reboot. Chosen because each covers a defect this series touched - the telemetry toggles that never persisted, the smart-position reboot that PR meshtastic#11181 removed, and a node-DB bit write that must not reach the radio. Deliberately picked extractions that also deduplicate: three telemetry call sites collapse onto one function and two smart-position sites onto another, so the promicro image is byte-identical to before. Worth knowing, because that board has under 1.4 KB of headroom before the warmstore region. toggleNodeMuted also gains a guard for an unknown node, so a stale pickedNodeNum can no longer cause a pointless flash write. clod helped too * Sort out InkHUD applying-changes notifications notifyApplyingChanges() was doing two jobs at its call sites in MenuApplet.cpp: signalling a live change that is applied without restarting, and warning of an imminent reboot. Audited every site. The finding is that none of them are redundant, so this records why rather than removing anything. The two live-change calls - LoRa region and modem preset - cover the seconds the e-ink takes to redraw while the radio reconfigures, and nothing else raises them. The reboot-path calls warn before the display goes. requestReboot() cannot absorb those: it deliberately carries no UI, because BaseUI renders its own notice at draw time from rebootAtMsec, while e-ink only draws when pushed. Also worth recording: the existing notifyReboot Observable (sleep.h, fired from Power::reboot) is a different moment - reboot execution, not scheduling - and InkHUD already observes it to save settings and shut applets down. A new "reboot scheduled" observable to centralise these calls would be a third reboot signal for no functional gain, so it is not added. clod helped too * stylee * Fix stale doc references and document the menu path Addresses review feedback. Four comments pointed at plan documents that were never committed (plan-narrow-reboot-trigger.md, plan-decouple-nodedb-admin-saves.md), so the rationale they cited was not discoverable. They now point at docs/admin-config-save-gating.md, which is in-repo. The doc listed the commit SHAs it described. All five had already been orphaned by the rebases this branch has been through, exactly as predicted, so it cites the PR instead. "Status: Implemented" also overstated things while hardware validation is still outstanding. The doc's biggest problem was that it had gone out of date within its own PR: it described the on-device menus as an untouched code path that still reloads the radio on any Config save, with per-site TODO markers. That stopped being true when the menus moved onto applyConfigChange. Replaced with a section covering the new entry point, the flags, why they are a flags enum rather than two bools, and the saveToDisk equivalence that makes pairing the two calls a double write. clod helped too * Fix smart-broadcast-interval reboot and carry transaction flags Four review findings. broadcast_smart_minimum_interval_secs is not read live, despite the comment this branch added claiming it was. PositionModule::minimumTimeThreshold is a const data member initialised when the module is constructed, so the value is captured at boot; only the sibling distance field is genuinely re-read (PositionModule.cpp :630). The InkHUD menu therefore applied it with no reboot and it silently did nothing until the next restart. It now reboots, matching the AdminModule path. Note the review suggested the opposite fix - adding the field to AdminModule's live-field list - which would have made the admin path silently ineffective too. While an edit transaction is open saveChanges() defers the write, which threw away the per-field reboot and radio decisions; the commit then used the parameter defaults and always rebooted and reconfigured the radio. Since phone apps write config through transactions, none of this branch's narrowing reached them. The deferred decisions now accumulate and the commit honours them. Two test fixes: the set_ignored_node test relied on an earlier RUN_TEST having created the node, which breaks under -f or a registration reorder. And the telemetry test asserted only values it had just assigned - replaced with one that drives the real toggle and checks the save path sets has_telemetry, the flag whose absence caused the TAK persistence bug fixed upstream in meshtastic#11216. clod helped too * Stop the edit transaction discarding the per-field save decisions Review of this branch's own output, plus the two findings Copilot raised on The headline defect is that none of this branch's narrowing reached phone apps. saveChanges() defers while an edit transaction is open, and radioAffected defaulted to true, so eight call sites that never thought about the radio - the five node-DB handlers, set_owner, set_module_config, and the nested save in the position case - accumulated a true into deferredRadioAffected. Outside a transaction that was harmless, because reloadConfig()'s saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS) bitmask independently blocks a node-DB-only save from reaching the radio whatever it asks for. Inside one the commit saves under a fixed full mask, so the bitmask always passes and radioAffected is the only thing left deciding it. Favouriting a node from the phone app therefore still ran the live SX126x reconfigure at commit - the The fix is to remove the default from saveChanges() rather than gate the deferred flag on the segment mask, which is what the review suggested. Gating would reinstate exactly the segment-to-radio inference this branch exists to delete - NodeDB.h now carries a comment telling the next person not to do that - and it fixes the symptom while leaving the wrong default in place for the next call site. With no default the compiler makes all fourteen sites state the answer. The commit also consumes and clears the deferred flags, so a stray second commit cannot inherit the previous transaction's answer. That gap existed because every radio test ran outside a transaction, which is the one arrangement where the bug is unreachable. Nine tests now cover the deferred path, asserting both axes independently across a commit. Verified they fail against the old behaviour: five go red while every pre-existing test stays green, which is the point. requestReboot()'s comment had the semantics backwards - it claimed a negative delay meant "now" and that rebootAtMsec == 0 was an immediate-reboot sentinel. Both are inverted: 0 means no reboot pending at every read site, and admin.proto documents reboot_seconds "<0 to cancel reboot". The expression was a faithful copy of AdminModule's, so only the comment was wrong, but it was wrong on the newly-created central helper. The negative branch now says so and logs it. Five sites still open-coded the deadline despite that helper existing; they are pure reboots with no config save, so applyConfigChange() does not fit but requestReboot() does exactly. SET_SMART_BROADCAST_INTERVAL was the only CONFIG_APPLY_REBOOT site in the InkHUD menu without a notifyApplyingChanges() beside it - re-adding its reboot last commit dropped the warning that applyConfigReload() used to raise, so the e-ink would go dark unannounced. And four channel actions carried CONFIG_APPLY_RADIO for uplink/downlink and position_precision, none of which touch the name, PSK or frequency slot the radio derives anything from; they had it only because the old reloadConfig(SEGMENT_CHANNELS) inferred it from the bitmask. The doc had gone stale inside its own PR again: it still listed commit_edit_settings under "intentionally left unchanged". Replaced with a section on why the bitmask stops protecting you inside a transaction, since that is the non-obvious part. Also dropped a commit SHA that had already been orphaned by rebase, exactly as its own note predicted, and a stale plan-doc reference in the tests that the last doc pass missed. Full native suite green, 810 cases across 40 suites. t-echo and t-echo-inkhud both build, covering the menu changes no native env compiles. * post rebase fixes * gps-toggle-noreboot * fix(admin): preserve live config transactions * fix(admin): avoid unnecessary config restarts * fix(admin): keep edit timeout dormant while idle * fix(admin): skip normalized no-op reboots * fix(admin): disable idle transaction timer --------- Co-authored-by: nomdetom <nomdetom@protonmail.com>
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 1, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 1, 2026
* reconfigure tests * Narrow radio-reload trigger to LoRa-affecting saves only reloadConfig() inferred "the radio needs reconfiguring" from whether saveWhat included SEGMENT_CONFIG. But Config is a monolithic segment (device/position/power/network/display/lora/bluetooth/security share one file), so every non-LoRa AdminModule save still fired the live SX126x reconfigure - the same live-SPI-on-the-admin-thread sequence that crashes on a favorite (meshtastic#11146), just reached via a Bluetooth toggle, WiFi PSK change, or keypair rotation. Separate the two concerns: add an explicit radioAffected flag threaded through AdminModule::saveChanges() into MeshService::reloadConfig(). The gate becomes `radioAffected && (saveWhat & (SEGMENT_CONFIG|SEGMENT_CHANNELS))` - the bitmask inference is kept as a backstop and only suppressible, and radioAffected defaults to true so the ~35 existing reloadConfig() callers (MenuHandler, MenuApplet, portduino) keep today's behavior untouched. Only handleSetConfig()'s lora_tag sets radioAffected; every other sub-message, plus set_fixed_position/remove_fixed_position and position's nested save, opt out. set_channel/commit_edit_settings/restore keep the default and still reload. Add native coverage in test_admin_radio for each non-LoRa sub-message (asserting no reconfigure), regression guards that lora/set_channel still reconfigure, and direct reloadConfig() guards pinning the fail-safe default. * mark up todo sites for checking reloadConfig use * added clarifying note * Don't reboot on no-op position/network/bluetooth config sets handleSetConfig() starts requiresReboot=true and device/power/display each gate it back down when nothing reboot-worthy changed, but position, network, and bluetooth never touched requiresReboot - so they rebooted on *every* set, including a client re-pushing byte-identical config (which happens routinely on connect/retry). Add a no-op gate to those three: if the incoming sub-message is byte-identical to the current one, skip the reboot. A whole-struct memcmp is the right tool here (unlike the field-by-field device/power/display gates, which must ignore benign fields) - it answers "did anything change?" and fails safe: its only error mode is padding-byte differences causing an unnecessary reboot, never a missed change. All three sub-messages are POD (no pb_callback_t fields). Any real change still reboots exactly as before. This is Tier 1 of plan-narrow-reboot-trigger; Tier 2 will further narrow position to reboot only on boot-only fields (GPIO/GPS). Adds native coverage asserting rebootAtMsec stays unset on a no-op set and is armed on a real change, for each of the three sub-messages. * Apply live position config changes without a reboot Tier 2 of plan-narrow-reboot-trigger. A position set that touches only fields the position module consumes live now applies without restarting. PositionModule reads position_broadcast_secs, position_broadcast_smart_enabled, broadcast_smart_minimum_distance, position_flags, and fixed_position directly from config on every send/schedule cycle (fixed_position also has dedicated live admin handlers) - changing only those needs no reboot. Everything else stays on the reboot path: GPS driver state (gps_mode/gps_enabled/ gps_update_interval/gps_attempt_time) and GPIO pin assignments (rx_gpio/ tx_gpio/gps_en_gpio), all of which touch subsystem/hardware init. The live set is deliberately limited to what static analysis proves is applied live, so this ships without hardware verification; GPS-timing fields that might also be live were left rebooting (fail toward current behavior). The gate neutralizes the live fields in a copy and reboots if any other byte differs, so a future PositionConfig field reboots until explicitly cleared as live - fail safe for schema growth. This supersedes the Tier 1 no-op memcmp gate for position (network/bluetooth keep theirs). Native coverage: a broadcast-interval change does not schedule a reboot; gps_mode and rx_gpio changes still do. * docs: config-save radio-reload & reboot gating Document the AdminModule config-save side-effect work: the radioAffected and requiresReboot axes, which operations now skip the radio reload or the reboot, and the operations deliberately left unchanged (commit_edit_settings, network/bluetooth live-apply, GPS-timing position fields, module config, and the on-device menu reloadConfig sites) with the reason for each. * docs: add hardware testing section to config-save gating The doc only mentioned the outstanding GPS-timing pass in one line and omitted the radio-reload/crash validation entirely, with no procedure. Add a Hardware testing section covering: the meshtastic-mcp setup (BLE connected throughout, nRF52840 SX126x reference board); the radio-reload/crash regression guard for the favorite-node fix (with a lora/set_channel positive control); and a per-field procedure with pass/fail criteria for deciding whether GPS-timing position fields can be reclassified as live - fail toward rebooting. * docs: clarify hardware tests run over serial, not menus/BLE The hardware section implied a BLE connection was required and that the crash ran on the BLE callback thread. On nRF52 the BLE onWrite only queues; handleToRadio -> saveChanges -> reloadConfig runs on the main FreeRTOS task, same context SerialConsole uses. So serial drives the exact code/thread under test, the on-device menus are unrelated (separate path), and the original crash was serial-proven. State that serial is sufficient and correct the thread references accordingly. * docs: correct crash mechanism to the off-main-thread spiLock lockup The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent). * docs: clarify config save behavior for GPS position updates * menu actions * menu actions reboot * Persist telemetry screen toggles from the frame menu The three moduleConfig.telemetry.*_screen_enabled toggles in the frame-toggle menu changed the value in memory and never wrote it, so they reverted on any reboot that did not happen to go through the reboot menu (which saves every segment). Their sibling entries in the same menu persist via Screen::toggleFrameVisibility -> saveFrameVisibility, so half the menu was durable and three entries were not. These three live in moduleConfig rather than the hiddenFrames blob, so they need their own SEGMENT_MODULECONFIG save. radioAffected is false: a screen preference has no business re-initialising the LoRa chip. clod helped too * Drop redundant and over-broad config writes from the menus reloadConfig() ends with an unconditional saveToDisk(saveWhat), outside the radioAffected guard, so nine sites in the InkHUD menu were writing the same proto file twice in a row: saveToDisk(X) immediately followed by reloadConfig(X, ...). One of the nine is the shared applyConfigReload() helper, so thirteen menu actions were affected. Five sites in the BaseUI menu called saveUIConfig() after changing a field that lives in config.proto. saveUIConfig() only writes /prefs/uiconfig.proto, so those writes could not persist the changed field and did nothing but cost a flash write. GPSFormatMenu had the mirror image: a uiconfig-only change that also called reloadConfig(), rewriting config.proto for a field not in it. Three bare saveToDisk() calls rewrote all five segments to change one bit - a channel mute flag and two NodeInfoLite bits - so they now pass the segment they actually touch. NodeInfoLite is written by saveNodeDatabaseToDisk() under SEGMENT_NODEDATABASE, not SEGMENT_DEVICESTATE. The reboot menu keeps its bare call: a full flush before a deliberate reboot is correct. reloadConfig() becomes virtual so tests can count calls; the accompanying test pins down the saveToDisk equivalence the nine deletions rely on. clod helped too * Route config saves through one MeshService::applyConfigChange helper Applying a config change involves three independent decisions: which proto files to persist, whether the LoRa chip needs re-initialising, and whether the field only takes effect after a restart. Those were spread across four helpers with three different parameter orders, two of which took adjacent bools that compile fine when transposed. InkHUD's applyConfigReload was the sharp edge: its second parameter was `reboot`, sitting exactly where reloadConfig and saveChanges take `radioAffected`. There is now a single entry point taking a flags enum, so each call site states its intent and cannot silently mean the opposite. Migrated 23 BaseUI sites, 20 InkHUD sites and the wasm glue. applyConfigReload is deleted and its five callers inlined; applyLoRaRegion and applyLoRaPreset stay, since they hold real domain logic beyond bundling, and just delegate. Adjacent `rebootAtMsec = millis() + ...` assignments fold into the reboot flag. The one caller that deliberately uses a shorter delay keeps it via the trailing rebootSeconds parameter. Reboot scheduling itself moves into requestReboot() in main.cpp, next to the global it sets: previously only AdminModule had a helper for this, and it was private, which is why every menu open-coded the deadline. requestReboot carries no UI, because BaseUI already renders the notice at draw time whenever rebootAtMsec is set. saveChanges keeps its signature and its edit-transaction deferral, so its fourteen call sites and the existing gating tests are untouched. clod helped too * Extract menu config actions into testable functions None of the menu save behaviour was reachable from a test: every action lived in a lambda assigned to BannerOverlayOptions.bannerCallback, which only ever runs via screen->showOverlayBanner(), so exercising one needed a live Screen. That is why the defects this series fixes went unnoticed - MenuHandler.cpp compiles in the native test build, but nothing in it could be called. Three actions move out into functions that own the whole decision: which segment to persist, whether the radio needs reconfiguring, and whether to reboot. Chosen because each covers a defect this series touched - the telemetry toggles that never persisted, the smart-position reboot that PR meshtastic#11181 removed, and a node-DB bit write that must not reach the radio. Deliberately picked extractions that also deduplicate: three telemetry call sites collapse onto one function and two smart-position sites onto another, so the promicro image is byte-identical to before. Worth knowing, because that board has under 1.4 KB of headroom before the warmstore region. toggleNodeMuted also gains a guard for an unknown node, so a stale pickedNodeNum can no longer cause a pointless flash write. clod helped too * Sort out InkHUD applying-changes notifications notifyApplyingChanges() was doing two jobs at its call sites in MenuApplet.cpp: signalling a live change that is applied without restarting, and warning of an imminent reboot. Audited every site. The finding is that none of them are redundant, so this records why rather than removing anything. The two live-change calls - LoRa region and modem preset - cover the seconds the e-ink takes to redraw while the radio reconfigures, and nothing else raises them. The reboot-path calls warn before the display goes. requestReboot() cannot absorb those: it deliberately carries no UI, because BaseUI renders its own notice at draw time from rebootAtMsec, while e-ink only draws when pushed. Also worth recording: the existing notifyReboot Observable (sleep.h, fired from Power::reboot) is a different moment - reboot execution, not scheduling - and InkHUD already observes it to save settings and shut applets down. A new "reboot scheduled" observable to centralise these calls would be a third reboot signal for no functional gain, so it is not added. clod helped too * stylee * Fix stale doc references and document the menu path Addresses review feedback. Four comments pointed at plan documents that were never committed (plan-narrow-reboot-trigger.md, plan-decouple-nodedb-admin-saves.md), so the rationale they cited was not discoverable. They now point at docs/admin-config-save-gating.md, which is in-repo. The doc listed the commit SHAs it described. All five had already been orphaned by the rebases this branch has been through, exactly as predicted, so it cites the PR instead. "Status: Implemented" also overstated things while hardware validation is still outstanding. The doc's biggest problem was that it had gone out of date within its own PR: it described the on-device menus as an untouched code path that still reloads the radio on any Config save, with per-site TODO markers. That stopped being true when the menus moved onto applyConfigChange. Replaced with a section covering the new entry point, the flags, why they are a flags enum rather than two bools, and the saveToDisk equivalence that makes pairing the two calls a double write. clod helped too * Fix smart-broadcast-interval reboot and carry transaction flags Four review findings. broadcast_smart_minimum_interval_secs is not read live, despite the comment this branch added claiming it was. PositionModule::minimumTimeThreshold is a const data member initialised when the module is constructed, so the value is captured at boot; only the sibling distance field is genuinely re-read (PositionModule.cpp :630). The InkHUD menu therefore applied it with no reboot and it silently did nothing until the next restart. It now reboots, matching the AdminModule path. Note the review suggested the opposite fix - adding the field to AdminModule's live-field list - which would have made the admin path silently ineffective too. While an edit transaction is open saveChanges() defers the write, which threw away the per-field reboot and radio decisions; the commit then used the parameter defaults and always rebooted and reconfigured the radio. Since phone apps write config through transactions, none of this branch's narrowing reached them. The deferred decisions now accumulate and the commit honours them. Two test fixes: the set_ignored_node test relied on an earlier RUN_TEST having created the node, which breaks under -f or a registration reorder. And the telemetry test asserted only values it had just assigned - replaced with one that drives the real toggle and checks the save path sets has_telemetry, the flag whose absence caused the TAK persistence bug fixed upstream in meshtastic#11216. clod helped too * Stop the edit transaction discarding the per-field save decisions Review of this branch's own output, plus the two findings Copilot raised on The headline defect is that none of this branch's narrowing reached phone apps. saveChanges() defers while an edit transaction is open, and radioAffected defaulted to true, so eight call sites that never thought about the radio - the five node-DB handlers, set_owner, set_module_config, and the nested save in the position case - accumulated a true into deferredRadioAffected. Outside a transaction that was harmless, because reloadConfig()'s saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS) bitmask independently blocks a node-DB-only save from reaching the radio whatever it asks for. Inside one the commit saves under a fixed full mask, so the bitmask always passes and radioAffected is the only thing left deciding it. Favouriting a node from the phone app therefore still ran the live SX126x reconfigure at commit - the The fix is to remove the default from saveChanges() rather than gate the deferred flag on the segment mask, which is what the review suggested. Gating would reinstate exactly the segment-to-radio inference this branch exists to delete - NodeDB.h now carries a comment telling the next person not to do that - and it fixes the symptom while leaving the wrong default in place for the next call site. With no default the compiler makes all fourteen sites state the answer. The commit also consumes and clears the deferred flags, so a stray second commit cannot inherit the previous transaction's answer. That gap existed because every radio test ran outside a transaction, which is the one arrangement where the bug is unreachable. Nine tests now cover the deferred path, asserting both axes independently across a commit. Verified they fail against the old behaviour: five go red while every pre-existing test stays green, which is the point. requestReboot()'s comment had the semantics backwards - it claimed a negative delay meant "now" and that rebootAtMsec == 0 was an immediate-reboot sentinel. Both are inverted: 0 means no reboot pending at every read site, and admin.proto documents reboot_seconds "<0 to cancel reboot". The expression was a faithful copy of AdminModule's, so only the comment was wrong, but it was wrong on the newly-created central helper. The negative branch now says so and logs it. Five sites still open-coded the deadline despite that helper existing; they are pure reboots with no config save, so applyConfigChange() does not fit but requestReboot() does exactly. SET_SMART_BROADCAST_INTERVAL was the only CONFIG_APPLY_REBOOT site in the InkHUD menu without a notifyApplyingChanges() beside it - re-adding its reboot last commit dropped the warning that applyConfigReload() used to raise, so the e-ink would go dark unannounced. And four channel actions carried CONFIG_APPLY_RADIO for uplink/downlink and position_precision, none of which touch the name, PSK or frequency slot the radio derives anything from; they had it only because the old reloadConfig(SEGMENT_CHANNELS) inferred it from the bitmask. The doc had gone stale inside its own PR again: it still listed commit_edit_settings under "intentionally left unchanged". Replaced with a section on why the bitmask stops protecting you inside a transaction, since that is the non-obvious part. Also dropped a commit SHA that had already been orphaned by rebase, exactly as its own note predicted, and a stale plan-doc reference in the tests that the last doc pass missed. Full native suite green, 810 cases across 40 suites. t-echo and t-echo-inkhud both build, covering the menu changes no native env compiles. * post rebase fixes * gps-toggle-noreboot * fix(admin): preserve live config transactions * fix(admin): avoid unnecessary config restarts * fix(admin): keep edit timeout dormant while idle * fix(admin): skip normalized no-op reboots * fix(admin): disable idle transaction timer --------- Co-authored-by: nomdetom <nomdetom@protonmail.com>
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 2, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 2, 2026
* reconfigure tests * Narrow radio-reload trigger to LoRa-affecting saves only reloadConfig() inferred "the radio needs reconfiguring" from whether saveWhat included SEGMENT_CONFIG. But Config is a monolithic segment (device/position/power/network/display/lora/bluetooth/security share one file), so every non-LoRa AdminModule save still fired the live SX126x reconfigure - the same live-SPI-on-the-admin-thread sequence that crashes on a favorite (meshtastic#11146), just reached via a Bluetooth toggle, WiFi PSK change, or keypair rotation. Separate the two concerns: add an explicit radioAffected flag threaded through AdminModule::saveChanges() into MeshService::reloadConfig(). The gate becomes `radioAffected && (saveWhat & (SEGMENT_CONFIG|SEGMENT_CHANNELS))` - the bitmask inference is kept as a backstop and only suppressible, and radioAffected defaults to true so the ~35 existing reloadConfig() callers (MenuHandler, MenuApplet, portduino) keep today's behavior untouched. Only handleSetConfig()'s lora_tag sets radioAffected; every other sub-message, plus set_fixed_position/remove_fixed_position and position's nested save, opt out. set_channel/commit_edit_settings/restore keep the default and still reload. Add native coverage in test_admin_radio for each non-LoRa sub-message (asserting no reconfigure), regression guards that lora/set_channel still reconfigure, and direct reloadConfig() guards pinning the fail-safe default. * mark up todo sites for checking reloadConfig use * added clarifying note * Don't reboot on no-op position/network/bluetooth config sets handleSetConfig() starts requiresReboot=true and device/power/display each gate it back down when nothing reboot-worthy changed, but position, network, and bluetooth never touched requiresReboot - so they rebooted on *every* set, including a client re-pushing byte-identical config (which happens routinely on connect/retry). Add a no-op gate to those three: if the incoming sub-message is byte-identical to the current one, skip the reboot. A whole-struct memcmp is the right tool here (unlike the field-by-field device/power/display gates, which must ignore benign fields) - it answers "did anything change?" and fails safe: its only error mode is padding-byte differences causing an unnecessary reboot, never a missed change. All three sub-messages are POD (no pb_callback_t fields). Any real change still reboots exactly as before. This is Tier 1 of plan-narrow-reboot-trigger; Tier 2 will further narrow position to reboot only on boot-only fields (GPIO/GPS). Adds native coverage asserting rebootAtMsec stays unset on a no-op set and is armed on a real change, for each of the three sub-messages. * Apply live position config changes without a reboot Tier 2 of plan-narrow-reboot-trigger. A position set that touches only fields the position module consumes live now applies without restarting. PositionModule reads position_broadcast_secs, position_broadcast_smart_enabled, broadcast_smart_minimum_distance, position_flags, and fixed_position directly from config on every send/schedule cycle (fixed_position also has dedicated live admin handlers) - changing only those needs no reboot. Everything else stays on the reboot path: GPS driver state (gps_mode/gps_enabled/ gps_update_interval/gps_attempt_time) and GPIO pin assignments (rx_gpio/ tx_gpio/gps_en_gpio), all of which touch subsystem/hardware init. The live set is deliberately limited to what static analysis proves is applied live, so this ships without hardware verification; GPS-timing fields that might also be live were left rebooting (fail toward current behavior). The gate neutralizes the live fields in a copy and reboots if any other byte differs, so a future PositionConfig field reboots until explicitly cleared as live - fail safe for schema growth. This supersedes the Tier 1 no-op memcmp gate for position (network/bluetooth keep theirs). Native coverage: a broadcast-interval change does not schedule a reboot; gps_mode and rx_gpio changes still do. * docs: config-save radio-reload & reboot gating Document the AdminModule config-save side-effect work: the radioAffected and requiresReboot axes, which operations now skip the radio reload or the reboot, and the operations deliberately left unchanged (commit_edit_settings, network/bluetooth live-apply, GPS-timing position fields, module config, and the on-device menu reloadConfig sites) with the reason for each. * docs: add hardware testing section to config-save gating The doc only mentioned the outstanding GPS-timing pass in one line and omitted the radio-reload/crash validation entirely, with no procedure. Add a Hardware testing section covering: the meshtastic-mcp setup (BLE connected throughout, nRF52840 SX126x reference board); the radio-reload/crash regression guard for the favorite-node fix (with a lora/set_channel positive control); and a per-field procedure with pass/fail criteria for deciding whether GPS-timing position fields can be reclassified as live - fail toward rebooting. * docs: clarify hardware tests run over serial, not menus/BLE The hardware section implied a BLE connection was required and that the crash ran on the BLE callback thread. On nRF52 the BLE onWrite only queues; handleToRadio -> saveChanges -> reloadConfig runs on the main FreeRTOS task, same context SerialConsole uses. So serial drives the exact code/thread under test, the on-device menus are unrelated (separate path), and the original crash was serial-proven. State that serial is sufficient and correct the thread references accordingly. * docs: correct crash mechanism to the off-main-thread spiLock lockup The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent). * docs: clarify config save behavior for GPS position updates * menu actions * menu actions reboot * Persist telemetry screen toggles from the frame menu The three moduleConfig.telemetry.*_screen_enabled toggles in the frame-toggle menu changed the value in memory and never wrote it, so they reverted on any reboot that did not happen to go through the reboot menu (which saves every segment). Their sibling entries in the same menu persist via Screen::toggleFrameVisibility -> saveFrameVisibility, so half the menu was durable and three entries were not. These three live in moduleConfig rather than the hiddenFrames blob, so they need their own SEGMENT_MODULECONFIG save. radioAffected is false: a screen preference has no business re-initialising the LoRa chip. clod helped too * Drop redundant and over-broad config writes from the menus reloadConfig() ends with an unconditional saveToDisk(saveWhat), outside the radioAffected guard, so nine sites in the InkHUD menu were writing the same proto file twice in a row: saveToDisk(X) immediately followed by reloadConfig(X, ...). One of the nine is the shared applyConfigReload() helper, so thirteen menu actions were affected. Five sites in the BaseUI menu called saveUIConfig() after changing a field that lives in config.proto. saveUIConfig() only writes /prefs/uiconfig.proto, so those writes could not persist the changed field and did nothing but cost a flash write. GPSFormatMenu had the mirror image: a uiconfig-only change that also called reloadConfig(), rewriting config.proto for a field not in it. Three bare saveToDisk() calls rewrote all five segments to change one bit - a channel mute flag and two NodeInfoLite bits - so they now pass the segment they actually touch. NodeInfoLite is written by saveNodeDatabaseToDisk() under SEGMENT_NODEDATABASE, not SEGMENT_DEVICESTATE. The reboot menu keeps its bare call: a full flush before a deliberate reboot is correct. reloadConfig() becomes virtual so tests can count calls; the accompanying test pins down the saveToDisk equivalence the nine deletions rely on. clod helped too * Route config saves through one MeshService::applyConfigChange helper Applying a config change involves three independent decisions: which proto files to persist, whether the LoRa chip needs re-initialising, and whether the field only takes effect after a restart. Those were spread across four helpers with three different parameter orders, two of which took adjacent bools that compile fine when transposed. InkHUD's applyConfigReload was the sharp edge: its second parameter was `reboot`, sitting exactly where reloadConfig and saveChanges take `radioAffected`. There is now a single entry point taking a flags enum, so each call site states its intent and cannot silently mean the opposite. Migrated 23 BaseUI sites, 20 InkHUD sites and the wasm glue. applyConfigReload is deleted and its five callers inlined; applyLoRaRegion and applyLoRaPreset stay, since they hold real domain logic beyond bundling, and just delegate. Adjacent `rebootAtMsec = millis() + ...` assignments fold into the reboot flag. The one caller that deliberately uses a shorter delay keeps it via the trailing rebootSeconds parameter. Reboot scheduling itself moves into requestReboot() in main.cpp, next to the global it sets: previously only AdminModule had a helper for this, and it was private, which is why every menu open-coded the deadline. requestReboot carries no UI, because BaseUI already renders the notice at draw time whenever rebootAtMsec is set. saveChanges keeps its signature and its edit-transaction deferral, so its fourteen call sites and the existing gating tests are untouched. clod helped too * Extract menu config actions into testable functions None of the menu save behaviour was reachable from a test: every action lived in a lambda assigned to BannerOverlayOptions.bannerCallback, which only ever runs via screen->showOverlayBanner(), so exercising one needed a live Screen. That is why the defects this series fixes went unnoticed - MenuHandler.cpp compiles in the native test build, but nothing in it could be called. Three actions move out into functions that own the whole decision: which segment to persist, whether the radio needs reconfiguring, and whether to reboot. Chosen because each covers a defect this series touched - the telemetry toggles that never persisted, the smart-position reboot that PR meshtastic#11181 removed, and a node-DB bit write that must not reach the radio. Deliberately picked extractions that also deduplicate: three telemetry call sites collapse onto one function and two smart-position sites onto another, so the promicro image is byte-identical to before. Worth knowing, because that board has under 1.4 KB of headroom before the warmstore region. toggleNodeMuted also gains a guard for an unknown node, so a stale pickedNodeNum can no longer cause a pointless flash write. clod helped too * Sort out InkHUD applying-changes notifications notifyApplyingChanges() was doing two jobs at its call sites in MenuApplet.cpp: signalling a live change that is applied without restarting, and warning of an imminent reboot. Audited every site. The finding is that none of them are redundant, so this records why rather than removing anything. The two live-change calls - LoRa region and modem preset - cover the seconds the e-ink takes to redraw while the radio reconfigures, and nothing else raises them. The reboot-path calls warn before the display goes. requestReboot() cannot absorb those: it deliberately carries no UI, because BaseUI renders its own notice at draw time from rebootAtMsec, while e-ink only draws when pushed. Also worth recording: the existing notifyReboot Observable (sleep.h, fired from Power::reboot) is a different moment - reboot execution, not scheduling - and InkHUD already observes it to save settings and shut applets down. A new "reboot scheduled" observable to centralise these calls would be a third reboot signal for no functional gain, so it is not added. clod helped too * stylee * Fix stale doc references and document the menu path Addresses review feedback. Four comments pointed at plan documents that were never committed (plan-narrow-reboot-trigger.md, plan-decouple-nodedb-admin-saves.md), so the rationale they cited was not discoverable. They now point at docs/admin-config-save-gating.md, which is in-repo. The doc listed the commit SHAs it described. All five had already been orphaned by the rebases this branch has been through, exactly as predicted, so it cites the PR instead. "Status: Implemented" also overstated things while hardware validation is still outstanding. The doc's biggest problem was that it had gone out of date within its own PR: it described the on-device menus as an untouched code path that still reloads the radio on any Config save, with per-site TODO markers. That stopped being true when the menus moved onto applyConfigChange. Replaced with a section covering the new entry point, the flags, why they are a flags enum rather than two bools, and the saveToDisk equivalence that makes pairing the two calls a double write. clod helped too * Fix smart-broadcast-interval reboot and carry transaction flags Four review findings. broadcast_smart_minimum_interval_secs is not read live, despite the comment this branch added claiming it was. PositionModule::minimumTimeThreshold is a const data member initialised when the module is constructed, so the value is captured at boot; only the sibling distance field is genuinely re-read (PositionModule.cpp :630). The InkHUD menu therefore applied it with no reboot and it silently did nothing until the next restart. It now reboots, matching the AdminModule path. Note the review suggested the opposite fix - adding the field to AdminModule's live-field list - which would have made the admin path silently ineffective too. While an edit transaction is open saveChanges() defers the write, which threw away the per-field reboot and radio decisions; the commit then used the parameter defaults and always rebooted and reconfigured the radio. Since phone apps write config through transactions, none of this branch's narrowing reached them. The deferred decisions now accumulate and the commit honours them. Two test fixes: the set_ignored_node test relied on an earlier RUN_TEST having created the node, which breaks under -f or a registration reorder. And the telemetry test asserted only values it had just assigned - replaced with one that drives the real toggle and checks the save path sets has_telemetry, the flag whose absence caused the TAK persistence bug fixed upstream in meshtastic#11216. clod helped too * Stop the edit transaction discarding the per-field save decisions Review of this branch's own output, plus the two findings Copilot raised on The headline defect is that none of this branch's narrowing reached phone apps. saveChanges() defers while an edit transaction is open, and radioAffected defaulted to true, so eight call sites that never thought about the radio - the five node-DB handlers, set_owner, set_module_config, and the nested save in the position case - accumulated a true into deferredRadioAffected. Outside a transaction that was harmless, because reloadConfig()'s saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS) bitmask independently blocks a node-DB-only save from reaching the radio whatever it asks for. Inside one the commit saves under a fixed full mask, so the bitmask always passes and radioAffected is the only thing left deciding it. Favouriting a node from the phone app therefore still ran the live SX126x reconfigure at commit - the The fix is to remove the default from saveChanges() rather than gate the deferred flag on the segment mask, which is what the review suggested. Gating would reinstate exactly the segment-to-radio inference this branch exists to delete - NodeDB.h now carries a comment telling the next person not to do that - and it fixes the symptom while leaving the wrong default in place for the next call site. With no default the compiler makes all fourteen sites state the answer. The commit also consumes and clears the deferred flags, so a stray second commit cannot inherit the previous transaction's answer. That gap existed because every radio test ran outside a transaction, which is the one arrangement where the bug is unreachable. Nine tests now cover the deferred path, asserting both axes independently across a commit. Verified they fail against the old behaviour: five go red while every pre-existing test stays green, which is the point. requestReboot()'s comment had the semantics backwards - it claimed a negative delay meant "now" and that rebootAtMsec == 0 was an immediate-reboot sentinel. Both are inverted: 0 means no reboot pending at every read site, and admin.proto documents reboot_seconds "<0 to cancel reboot". The expression was a faithful copy of AdminModule's, so only the comment was wrong, but it was wrong on the newly-created central helper. The negative branch now says so and logs it. Five sites still open-coded the deadline despite that helper existing; they are pure reboots with no config save, so applyConfigChange() does not fit but requestReboot() does exactly. SET_SMART_BROADCAST_INTERVAL was the only CONFIG_APPLY_REBOOT site in the InkHUD menu without a notifyApplyingChanges() beside it - re-adding its reboot last commit dropped the warning that applyConfigReload() used to raise, so the e-ink would go dark unannounced. And four channel actions carried CONFIG_APPLY_RADIO for uplink/downlink and position_precision, none of which touch the name, PSK or frequency slot the radio derives anything from; they had it only because the old reloadConfig(SEGMENT_CHANNELS) inferred it from the bitmask. The doc had gone stale inside its own PR again: it still listed commit_edit_settings under "intentionally left unchanged". Replaced with a section on why the bitmask stops protecting you inside a transaction, since that is the non-obvious part. Also dropped a commit SHA that had already been orphaned by rebase, exactly as its own note predicted, and a stale plan-doc reference in the tests that the last doc pass missed. Full native suite green, 810 cases across 40 suites. t-echo and t-echo-inkhud both build, covering the menu changes no native env compiles. * post rebase fixes * gps-toggle-noreboot * fix(admin): preserve live config transactions * fix(admin): avoid unnecessary config restarts * fix(admin): keep edit timeout dormant while idle * fix(admin): skip normalized no-op reboots * fix(admin): disable idle transaction timer --------- Co-authored-by: nomdetom <nomdetom@protonmail.com>
This was referenced Aug 12, 2026
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 12, 2026
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 12, 2026
* reconfigure tests * Narrow radio-reload trigger to LoRa-affecting saves only reloadConfig() inferred "the radio needs reconfiguring" from whether saveWhat included SEGMENT_CONFIG. But Config is a monolithic segment (device/position/power/network/display/lora/bluetooth/security share one file), so every non-LoRa AdminModule save still fired the live SX126x reconfigure - the same live-SPI-on-the-admin-thread sequence that crashes on a favorite (meshtastic#11146), just reached via a Bluetooth toggle, WiFi PSK change, or keypair rotation. Separate the two concerns: add an explicit radioAffected flag threaded through AdminModule::saveChanges() into MeshService::reloadConfig(). The gate becomes `radioAffected && (saveWhat & (SEGMENT_CONFIG|SEGMENT_CHANNELS))` - the bitmask inference is kept as a backstop and only suppressible, and radioAffected defaults to true so the ~35 existing reloadConfig() callers (MenuHandler, MenuApplet, portduino) keep today's behavior untouched. Only handleSetConfig()'s lora_tag sets radioAffected; every other sub-message, plus set_fixed_position/remove_fixed_position and position's nested save, opt out. set_channel/commit_edit_settings/restore keep the default and still reload. Add native coverage in test_admin_radio for each non-LoRa sub-message (asserting no reconfigure), regression guards that lora/set_channel still reconfigure, and direct reloadConfig() guards pinning the fail-safe default. * mark up todo sites for checking reloadConfig use * added clarifying note * Don't reboot on no-op position/network/bluetooth config sets handleSetConfig() starts requiresReboot=true and device/power/display each gate it back down when nothing reboot-worthy changed, but position, network, and bluetooth never touched requiresReboot - so they rebooted on *every* set, including a client re-pushing byte-identical config (which happens routinely on connect/retry). Add a no-op gate to those three: if the incoming sub-message is byte-identical to the current one, skip the reboot. A whole-struct memcmp is the right tool here (unlike the field-by-field device/power/display gates, which must ignore benign fields) - it answers "did anything change?" and fails safe: its only error mode is padding-byte differences causing an unnecessary reboot, never a missed change. All three sub-messages are POD (no pb_callback_t fields). Any real change still reboots exactly as before. This is Tier 1 of plan-narrow-reboot-trigger; Tier 2 will further narrow position to reboot only on boot-only fields (GPIO/GPS). Adds native coverage asserting rebootAtMsec stays unset on a no-op set and is armed on a real change, for each of the three sub-messages. * Apply live position config changes without a reboot Tier 2 of plan-narrow-reboot-trigger. A position set that touches only fields the position module consumes live now applies without restarting. PositionModule reads position_broadcast_secs, position_broadcast_smart_enabled, broadcast_smart_minimum_distance, position_flags, and fixed_position directly from config on every send/schedule cycle (fixed_position also has dedicated live admin handlers) - changing only those needs no reboot. Everything else stays on the reboot path: GPS driver state (gps_mode/gps_enabled/ gps_update_interval/gps_attempt_time) and GPIO pin assignments (rx_gpio/ tx_gpio/gps_en_gpio), all of which touch subsystem/hardware init. The live set is deliberately limited to what static analysis proves is applied live, so this ships without hardware verification; GPS-timing fields that might also be live were left rebooting (fail toward current behavior). The gate neutralizes the live fields in a copy and reboots if any other byte differs, so a future PositionConfig field reboots until explicitly cleared as live - fail safe for schema growth. This supersedes the Tier 1 no-op memcmp gate for position (network/bluetooth keep theirs). Native coverage: a broadcast-interval change does not schedule a reboot; gps_mode and rx_gpio changes still do. * docs: config-save radio-reload & reboot gating Document the AdminModule config-save side-effect work: the radioAffected and requiresReboot axes, which operations now skip the radio reload or the reboot, and the operations deliberately left unchanged (commit_edit_settings, network/bluetooth live-apply, GPS-timing position fields, module config, and the on-device menu reloadConfig sites) with the reason for each. * docs: add hardware testing section to config-save gating The doc only mentioned the outstanding GPS-timing pass in one line and omitted the radio-reload/crash validation entirely, with no procedure. Add a Hardware testing section covering: the meshtastic-mcp setup (BLE connected throughout, nRF52840 SX126x reference board); the radio-reload/crash regression guard for the favorite-node fix (with a lora/set_channel positive control); and a per-field procedure with pass/fail criteria for deciding whether GPS-timing position fields can be reclassified as live - fail toward rebooting. * docs: clarify hardware tests run over serial, not menus/BLE The hardware section implied a BLE connection was required and that the crash ran on the BLE callback thread. On nRF52 the BLE onWrite only queues; handleToRadio -> saveChanges -> reloadConfig runs on the main FreeRTOS task, same context SerialConsole uses. So serial drives the exact code/thread under test, the on-device menus are unrelated (separate path), and the original crash was serial-proven. State that serial is sufficient and correct the thread references accordingly. * docs: correct crash mechanism to the off-main-thread spiLock lockup The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent). * docs: clarify config save behavior for GPS position updates * menu actions * menu actions reboot * Persist telemetry screen toggles from the frame menu The three moduleConfig.telemetry.*_screen_enabled toggles in the frame-toggle menu changed the value in memory and never wrote it, so they reverted on any reboot that did not happen to go through the reboot menu (which saves every segment). Their sibling entries in the same menu persist via Screen::toggleFrameVisibility -> saveFrameVisibility, so half the menu was durable and three entries were not. These three live in moduleConfig rather than the hiddenFrames blob, so they need their own SEGMENT_MODULECONFIG save. radioAffected is false: a screen preference has no business re-initialising the LoRa chip. clod helped too * Drop redundant and over-broad config writes from the menus reloadConfig() ends with an unconditional saveToDisk(saveWhat), outside the radioAffected guard, so nine sites in the InkHUD menu were writing the same proto file twice in a row: saveToDisk(X) immediately followed by reloadConfig(X, ...). One of the nine is the shared applyConfigReload() helper, so thirteen menu actions were affected. Five sites in the BaseUI menu called saveUIConfig() after changing a field that lives in config.proto. saveUIConfig() only writes /prefs/uiconfig.proto, so those writes could not persist the changed field and did nothing but cost a flash write. GPSFormatMenu had the mirror image: a uiconfig-only change that also called reloadConfig(), rewriting config.proto for a field not in it. Three bare saveToDisk() calls rewrote all five segments to change one bit - a channel mute flag and two NodeInfoLite bits - so they now pass the segment they actually touch. NodeInfoLite is written by saveNodeDatabaseToDisk() under SEGMENT_NODEDATABASE, not SEGMENT_DEVICESTATE. The reboot menu keeps its bare call: a full flush before a deliberate reboot is correct. reloadConfig() becomes virtual so tests can count calls; the accompanying test pins down the saveToDisk equivalence the nine deletions rely on. clod helped too * Route config saves through one MeshService::applyConfigChange helper Applying a config change involves three independent decisions: which proto files to persist, whether the LoRa chip needs re-initialising, and whether the field only takes effect after a restart. Those were spread across four helpers with three different parameter orders, two of which took adjacent bools that compile fine when transposed. InkHUD's applyConfigReload was the sharp edge: its second parameter was `reboot`, sitting exactly where reloadConfig and saveChanges take `radioAffected`. There is now a single entry point taking a flags enum, so each call site states its intent and cannot silently mean the opposite. Migrated 23 BaseUI sites, 20 InkHUD sites and the wasm glue. applyConfigReload is deleted and its five callers inlined; applyLoRaRegion and applyLoRaPreset stay, since they hold real domain logic beyond bundling, and just delegate. Adjacent `rebootAtMsec = millis() + ...` assignments fold into the reboot flag. The one caller that deliberately uses a shorter delay keeps it via the trailing rebootSeconds parameter. Reboot scheduling itself moves into requestReboot() in main.cpp, next to the global it sets: previously only AdminModule had a helper for this, and it was private, which is why every menu open-coded the deadline. requestReboot carries no UI, because BaseUI already renders the notice at draw time whenever rebootAtMsec is set. saveChanges keeps its signature and its edit-transaction deferral, so its fourteen call sites and the existing gating tests are untouched. clod helped too * Extract menu config actions into testable functions None of the menu save behaviour was reachable from a test: every action lived in a lambda assigned to BannerOverlayOptions.bannerCallback, which only ever runs via screen->showOverlayBanner(), so exercising one needed a live Screen. That is why the defects this series fixes went unnoticed - MenuHandler.cpp compiles in the native test build, but nothing in it could be called. Three actions move out into functions that own the whole decision: which segment to persist, whether the radio needs reconfiguring, and whether to reboot. Chosen because each covers a defect this series touched - the telemetry toggles that never persisted, the smart-position reboot that PR meshtastic#11181 removed, and a node-DB bit write that must not reach the radio. Deliberately picked extractions that also deduplicate: three telemetry call sites collapse onto one function and two smart-position sites onto another, so the promicro image is byte-identical to before. Worth knowing, because that board has under 1.4 KB of headroom before the warmstore region. toggleNodeMuted also gains a guard for an unknown node, so a stale pickedNodeNum can no longer cause a pointless flash write. clod helped too * Sort out InkHUD applying-changes notifications notifyApplyingChanges() was doing two jobs at its call sites in MenuApplet.cpp: signalling a live change that is applied without restarting, and warning of an imminent reboot. Audited every site. The finding is that none of them are redundant, so this records why rather than removing anything. The two live-change calls - LoRa region and modem preset - cover the seconds the e-ink takes to redraw while the radio reconfigures, and nothing else raises them. The reboot-path calls warn before the display goes. requestReboot() cannot absorb those: it deliberately carries no UI, because BaseUI renders its own notice at draw time from rebootAtMsec, while e-ink only draws when pushed. Also worth recording: the existing notifyReboot Observable (sleep.h, fired from Power::reboot) is a different moment - reboot execution, not scheduling - and InkHUD already observes it to save settings and shut applets down. A new "reboot scheduled" observable to centralise these calls would be a third reboot signal for no functional gain, so it is not added. clod helped too * stylee * Fix stale doc references and document the menu path Addresses review feedback. Four comments pointed at plan documents that were never committed (plan-narrow-reboot-trigger.md, plan-decouple-nodedb-admin-saves.md), so the rationale they cited was not discoverable. They now point at docs/admin-config-save-gating.md, which is in-repo. The doc listed the commit SHAs it described. All five had already been orphaned by the rebases this branch has been through, exactly as predicted, so it cites the PR instead. "Status: Implemented" also overstated things while hardware validation is still outstanding. The doc's biggest problem was that it had gone out of date within its own PR: it described the on-device menus as an untouched code path that still reloads the radio on any Config save, with per-site TODO markers. That stopped being true when the menus moved onto applyConfigChange. Replaced with a section covering the new entry point, the flags, why they are a flags enum rather than two bools, and the saveToDisk equivalence that makes pairing the two calls a double write. clod helped too * Fix smart-broadcast-interval reboot and carry transaction flags Four review findings. broadcast_smart_minimum_interval_secs is not read live, despite the comment this branch added claiming it was. PositionModule::minimumTimeThreshold is a const data member initialised when the module is constructed, so the value is captured at boot; only the sibling distance field is genuinely re-read (PositionModule.cpp :630). The InkHUD menu therefore applied it with no reboot and it silently did nothing until the next restart. It now reboots, matching the AdminModule path. Note the review suggested the opposite fix - adding the field to AdminModule's live-field list - which would have made the admin path silently ineffective too. While an edit transaction is open saveChanges() defers the write, which threw away the per-field reboot and radio decisions; the commit then used the parameter defaults and always rebooted and reconfigured the radio. Since phone apps write config through transactions, none of this branch's narrowing reached them. The deferred decisions now accumulate and the commit honours them. Two test fixes: the set_ignored_node test relied on an earlier RUN_TEST having created the node, which breaks under -f or a registration reorder. And the telemetry test asserted only values it had just assigned - replaced with one that drives the real toggle and checks the save path sets has_telemetry, the flag whose absence caused the TAK persistence bug fixed upstream in meshtastic#11216. clod helped too * Stop the edit transaction discarding the per-field save decisions Review of this branch's own output, plus the two findings Copilot raised on The headline defect is that none of this branch's narrowing reached phone apps. saveChanges() defers while an edit transaction is open, and radioAffected defaulted to true, so eight call sites that never thought about the radio - the five node-DB handlers, set_owner, set_module_config, and the nested save in the position case - accumulated a true into deferredRadioAffected. Outside a transaction that was harmless, because reloadConfig()'s saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS) bitmask independently blocks a node-DB-only save from reaching the radio whatever it asks for. Inside one the commit saves under a fixed full mask, so the bitmask always passes and radioAffected is the only thing left deciding it. Favouriting a node from the phone app therefore still ran the live SX126x reconfigure at commit - the The fix is to remove the default from saveChanges() rather than gate the deferred flag on the segment mask, which is what the review suggested. Gating would reinstate exactly the segment-to-radio inference this branch exists to delete - NodeDB.h now carries a comment telling the next person not to do that - and it fixes the symptom while leaving the wrong default in place for the next call site. With no default the compiler makes all fourteen sites state the answer. The commit also consumes and clears the deferred flags, so a stray second commit cannot inherit the previous transaction's answer. That gap existed because every radio test ran outside a transaction, which is the one arrangement where the bug is unreachable. Nine tests now cover the deferred path, asserting both axes independently across a commit. Verified they fail against the old behaviour: five go red while every pre-existing test stays green, which is the point. requestReboot()'s comment had the semantics backwards - it claimed a negative delay meant "now" and that rebootAtMsec == 0 was an immediate-reboot sentinel. Both are inverted: 0 means no reboot pending at every read site, and admin.proto documents reboot_seconds "<0 to cancel reboot". The expression was a faithful copy of AdminModule's, so only the comment was wrong, but it was wrong on the newly-created central helper. The negative branch now says so and logs it. Five sites still open-coded the deadline despite that helper existing; they are pure reboots with no config save, so applyConfigChange() does not fit but requestReboot() does exactly. SET_SMART_BROADCAST_INTERVAL was the only CONFIG_APPLY_REBOOT site in the InkHUD menu without a notifyApplyingChanges() beside it - re-adding its reboot last commit dropped the warning that applyConfigReload() used to raise, so the e-ink would go dark unannounced. And four channel actions carried CONFIG_APPLY_RADIO for uplink/downlink and position_precision, none of which touch the name, PSK or frequency slot the radio derives anything from; they had it only because the old reloadConfig(SEGMENT_CHANNELS) inferred it from the bitmask. The doc had gone stale inside its own PR again: it still listed commit_edit_settings under "intentionally left unchanged". Replaced with a section on why the bitmask stops protecting you inside a transaction, since that is the non-obvious part. Also dropped a commit SHA that had already been orphaned by rebase, exactly as its own note predicted, and a stale plan-doc reference in the tests that the last doc pass missed. Full native suite green, 810 cases across 40 suites. t-echo and t-echo-inkhud both build, covering the menu changes no native env compiles. * post rebase fixes * gps-toggle-noreboot * fix(admin): preserve live config transactions * fix(admin): avoid unnecessary config restarts * fix(admin): keep edit timeout dormant while idle * fix(admin): skip normalized no-op reboots * fix(admin): disable idle transaction timer --------- Co-authored-by: nomdetom <nomdetom@protonmail.com>
Itzdavid01
pushed a commit
to Itzdavid01/firmware
that referenced
this pull request
Sep 5, 2026
* NodeDB: 3-tier node store with persistent warm tier (long-tail identity retention)
Introduces a tiered NodeDB so the device retains identity (public key,
last_heard) for far more nodes than fit in the full-record hot store,
without growing heap or the persisted nodes.proto unboundedly.
- Hot store: full NodeInfoLite, MAX_NUM_NODES (120 on nRF52).
- Satellite maps: position/telemetry/environment/status capped at
MAX_SATELLITE_NODES (40 freshest); eviction via enforceSatelliteCaps /
evictSatelliteOverCap.
- Warm tier (WarmNodeStore): 40 B {num,last_heard,public_key} records for
evicted nodes so DMs to/from long-tail nodes keep encrypting/decrypting.
Persisted to /prefs/warm.dat, or on nRF52840 a dedicated 12 KB raw-flash
record-ring below LittleFS (3x4 KB pages; see linker scripts + the
nrf52_warm_region.py post-link guard).
NodeDB::getOrCreateMeshNode now demotes evicted nodes into the warm tier and
re-admits them (restoring key/last_heard). Router PKI decrypt/encode resolve
the peer key via NodeDB::copyPublicKey (hot store, then warm tier).
NodeInfoLite gains snr_q4 (sint32, Q4-encoded dB); the float snr is zeroed on
disk. NodeInfoLite grows 105 -> 112 B; backup 2432 -> 2468 B.
Note: the snr_q4 .proto change still needs to land in the protobufs submodule
(generated header is updated here; submodule pointer left at upstream).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* NodeDB: robust receive + retention for blocked (ignored) nodes
Hardens how ignored/favourite nodes are received over admin and retained,
closing paths where a block could be lost or accidentally cleared.
- Blocking keeps the node's public key (admin set_ignored_node and
addFromContact no longer zero it / drop the warm-tier key), so a blocked
peer stays a verifiable identity.
- set_ignored_node creates the node if absent, so a block by node ID sticks
even for a node we've never heard from (e.g. pushed by a remote admin) with
no NodeInfo or key.
- Eviction protection (favourite/ignored/manually-verified) now also applies to
the load-time hot-store migration and is never undone by cleanupMeshDB, which
previously purged ignored nodes that lacked user info.
- The hot-store migration leaves our own node (index 0) in place and prefers to
demote non-protected nodes, like the runtime eviction scan.
Caps the protected set (favourite + ignored + verified) at MAX_NUM_NODES-2 via
NodeDB::setProtectedFlag(), so at least two evictable slots always remain and
getOrCreateMeshNode can always make room — replacing the previous unconditional
append that could run off the end of the node vector when every node was
protected. A locally-set favourite/ignore that hits the cap reports back to the
phone via a ClientNotification.
Adds test_nodedb_blocked covering the migration, favourite/ignored eviction
protection, ignored-survives-cleanup, and the protected-node cap. The
maintenance methods stay private in production; the test reaches them through a
PIO_UNIT_TESTING-guarded friend shim.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts:
# src/mesh/NodeDB.h
* fix copilot comments
* once again
* WarmNodeStore: fix cppcheck warnings (uninitvar, constVariablePointer)
Zero-initialise `stranded[]` and `seqs[]/order[]` VLAs so cppcheck can
verify there are no unguarded reads of uninitialised memory (the guards
exist but are not visible to static analysis). Mark two local pointers
`const` where the pointed-to entry is never mutated after assignment.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* self-care added to assist 2.7 and 2.8 nodedb migration
* Tidy warm-store/self-care: comments, guards, log + flash cleanup
Style/cleanup pass over the branch (no behavior change except the noted
preprocessor simplifications, which are semantically identical):
- Comments: move function descriptions to the headers, cap in-function
comments at ~3-4 lines, drop leading-number step markers, label stacked
#endif blocks, de-decorate banner comments.
- dumpToLog: fully gate decl + definition + AdminModule call site behind
MESHTASTIC_NODEDB_MIGRATION_VERBOSE so it compiles out when disabled
(~1.2 KB when off).
- mesh-pb-constants: drop the dead nRF52832 WARM_NODE_COUNT branch and trim
the macro docs.
- WarmNodeStore: simplify the redundant `ARCH_NRF52 && NRF52840_XXAA` guards
to `NRF52840_XXAA`, add a kNoPage sentinel for the ring page state.
- Shorten the always-on LOG_WARN strings (~120 B flash).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* more tidying up, aligning with docs and undoing other-arch regressions
* Update protobufs (meshtastic#19)
Co-authored-by: NomDeTom <116762865+NomDeTom@users.noreply.github.com>
* made the migration pathway cleareer
* address copilot review
* fixed a copilot review on a downstream PR.
* Address Copilot review comments for PR meshtastic#10705 (warmstore/nodedb)
- WarmNodeStore.h: default MIGRATION_VERBOSE to 0 (suppress info-level
chatter on production builds; opt in with =1)
- WarmNodeStore.cpp load(): move memset to top of function so all
failure paths (header-read fail, invalid header) leave entries clear
- WarmNodeStore.cpp save(): replace manual spiLock lock/unlock around
mkdir with LockGuard covering the full SafeFile sequence, matching
the lock discipline in load()
- Router.cpp: memcpy(&p->public_key.bytes, ...) -> memcpy(p->public_key.bytes,
...) — pass decayed uint8_t* rather than pointer-to-array
- AdminModule.cpp: check setProtectedFlag return for PKC auto-favorite;
log cap-refusal warning instead of unconditional "auto-favoriting"
- nrf52_warm_region.py: error message references both v6.ld and v7.ld
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* NodeDB: formatting cleanup (blank lines after preprocessor blocks)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Lukewarm store
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Itzdavid01
pushed a commit
to Itzdavid01/firmware
that referenced
this pull request
Sep 5, 2026
…meshtastic#10705) (meshtastic#10759) * fix: right-size warm tier for constrained platforms and feed RP2040 watchdog during NodeDB save * fix: size warm tier and traffic cache per-MCU RAM, lowering no-PSRAM ESP32 (classic/S2/C3) tiers * docs: document nRF52/RP2040 #else fall-through in warm tier and TM cache cascades
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NodeDB: 3-tier node store with warm tier + blocked-node retention
Reworks the NodeDB into a tiered store so the device retains identity for far more nodes than fit in the full-record store, and hardens how blocked (ignored) nodes are received and kept.
What it does
Load-time migration
Blocked-node handling
Tests:
test_warm_store,
test_nodedb_blocked (migration, favourite/ignored eviction protection, protected-cap).
Note: the snr_q4 field needs the matching deviceonly.proto change in the protobufs submodule before merge (generated header is updated here).
MCP server used to confirm migration behaviour - 150 nodes becomes 120 cleanly after startup.
🤝 Attestations
@alecperkins has tested on a heltec V4 - many thanks