Avoid unnecessary reboots in admin edit transactions - #28
Merged
Conversation
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.
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.
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.
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.
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.
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.
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).
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
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
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
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
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
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
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
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.
This was referenced Jul 30, 2026
RCGV1
changed the title
NomDeTom
reviewed
Jul 30, 2026
NomDeTom
left a comment
NomDeTom
left a comment
Owner
There was a problem hiding this comment.
Solid PR - the idempotency fix, the timeout thread and encode-then-compare are all the right shape, and the hardware evidence is appreciated.
Two things I checked that hold up, so nobody re-litigates them:
tx_enabledas live-applied is correct.RadioLibInterfacereadsconfig.lora.tx_enabledat send time (:159,:173,:761), not inreconfigure(), so suppressing the radio reload doesn't strand it.radioAffected=falsestarves nothing but the radio.configChangedhas exactly one observer,RadioInterface(RadioInterface.cpp:917), so the channel-PSK and no-op cases are safe on that axis.
Also confirmed disableBluetoothCallCountForTest = 0 is in setUp(), so the EQUAL_UINT32(0, ...) assertions aren't order-dependent.
Four things I'd want resolved before merge, left inline. Filing as a comment rather than changes-requested - the first one is a design call that's yours to make, not a defect.
One process note: this stacks on tidyups (78c9028de), not develop, so it moves with meshtastic#11181 if that gets reworked.
Owner
|
This PR's base ( Diffing this PR's head directly against current
Could you rebase (or merge Generated by Claude Code |
Author
|
Synced in 59240a1 by merging current tidyups at 4130653. The conflicts in AdminModule.cpp, AdminModuleTestShim.h, and test_admin_radio were resolved while preserving the no-op comparison, channel-slot detection, and dormant timeout work; the reviewable delta against tidyups is now limited to AdminModule.cpp/.h plus the two admin test files. Validation: 886/886 native tests, 169/169 focused admin-radio tests, and the T-Echo build with nRF warm-region/ISR/variant guards all pass. |
NomDeTom
added a commit
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
that referenced
this pull request
Jul 31, 2026
Three findings from re-reviewing the restart-narrowing work, none of which changes behaviour on a working device. protobufsEqual() read pb_encode_to_bytes()'s return value, which is 0 both for a failed encode and for a message whose every field sits at its default. Every caller uses equality to *skip* a reboot or a radio reload, so a failed encode read as "0 == 0, equal" would suppress exactly the work it could no longer prove unnecessary. Encode directly and read the stream result, which separates the two, and fail toward "changed". The all-defaults comparison is legitimate and still compares equal - a zeroed SecurityConfig encodes to zero bytes - so the obvious `size != 0` guard would have been wrong. handleSetChannel()'s name snapshot was a bare char[32]. getName() returns either the stored name (proto char[12]) or a modem-preset display name, and the longest of those is "MediumTurbo" at 11 - so 32 is right, but nothing said so and nothing checked it. Truncation would compare equal across a real rename and skip the frequency-slot reload, so record why the headroom is there and static_assert the proto side of it. runOnce()'s 1ms fallback looked like dead code: expireStaleEditTransaction() uses the same threshold, so a transaction that survives it should always have elapsed < the window. It doesn't, quite - that function read millis() a moment earlier, and the window can tip over in between, in which case the subtraction underflows to ~49.7 days. The guard is load-bearing; say so, because reading it as dead is what invites its removal. Also split the two bundled LoRa no-reload tests one field per set. Flipping six fields in a single message passes as long as none of them triggers a reload, so a field mis-classified later would have hidden behind its neighbours; each now names itself on failure. native test_admin_radio: 175/175. Test status, corrected: test_admin_radio is not green on this branch and was not made green by this commit. In a clean build it reports four pre-existing assertion failures (setFavoriteNode / setIgnoredNode / toggleMutedNode / toggleNodeMuted, all "did the bit get set") and then dies with SIGILL after the last case, so PlatformIO marks the suite ERRORED. Identical at this commit, at its parent, and at 4130653 before #28, filtered or full - and absent on develop, which does not carry these tests. Unrelated to this change; tracked separately. An earlier run of this commit reported 175/175 passing. That was a stale build tree - a clean build of the same source reports 176 cases - so that number was wrong and is retracted here rather than left in the record.
NomDeTom
added a commit
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
that referenced
this pull request
Aug 1, 2026
Three findings from re-reviewing the restart-narrowing work, none of which changes behaviour on a working device. protobufsEqual() read pb_encode_to_bytes()'s return value, which is 0 both for a failed encode and for a message whose every field sits at its default. Every caller uses equality to *skip* a reboot or a radio reload, so a failed encode read as "0 == 0, equal" would suppress exactly the work it could no longer prove unnecessary. Encode directly and read the stream result, which separates the two, and fail toward "changed". The all-defaults comparison is legitimate and still compares equal - a zeroed SecurityConfig encodes to zero bytes - so the obvious `size != 0` guard would have been wrong. handleSetChannel()'s name snapshot was a bare char[32]. getName() returns either the stored name (proto char[12]) or a modem-preset display name, and the longest of those is "MediumTurbo" at 11 - so 32 is right, but nothing said so and nothing checked it. Truncation would compare equal across a real rename and skip the frequency-slot reload, so record why the headroom is there and static_assert the proto side of it. runOnce()'s 1ms fallback looked like dead code: expireStaleEditTransaction() uses the same threshold, so a transaction that survives it should always have elapsed < the window. It doesn't, quite - that function read millis() a moment earlier, and the window can tip over in between, in which case the subtraction underflows to ~49.7 days. The guard is load-bearing; say so, because reading it as dead is what invites its removal. Also split the two bundled LoRa no-reload tests one field per set. Flipping six fields in a single message passes as long as none of them triggers a reload, so a field mis-classified later would have hidden behind its neighbours; each now names itself on failure. native test_admin_radio: 175/175. Test status, corrected: test_admin_radio is not green on this branch and was not made green by this commit. In a clean build it reports four pre-existing assertion failures (setFavoriteNode / setIgnoredNode / toggleMutedNode / toggleNodeMuted, all "did the bit get set") and then dies with SIGILL after the last case, so PlatformIO marks the suite ERRORED. Identical at this commit, at its parent, and at 4130653 before #28, filtered or full - and absent on develop, which does not carry these tests. Unrelated to this change; tracked separately. An earlier run of this commit reported 175/175 passing. That was a stale build tree - a clean build of the same source reports 176 cases - so that number was wrong and is retracted here rather than left in the record.
NomDeTom
added a commit
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>
NomDeTom
added a commit
that referenced
this pull request
Aug 2, 2026
Three findings from re-reviewing the restart-narrowing work, none of which changes behaviour on a working device. protobufsEqual() read pb_encode_to_bytes()'s return value, which is 0 both for a failed encode and for a message whose every field sits at its default. Every caller uses equality to *skip* a reboot or a radio reload, so a failed encode read as "0 == 0, equal" would suppress exactly the work it could no longer prove unnecessary. Encode directly and read the stream result, which separates the two, and fail toward "changed". The all-defaults comparison is legitimate and still compares equal - a zeroed SecurityConfig encodes to zero bytes - so the obvious `size != 0` guard would have been wrong. handleSetChannel()'s name snapshot was a bare char[32]. getName() returns either the stored name (proto char[12]) or a modem-preset display name, and the longest of those is "MediumTurbo" at 11 - so 32 is right, but nothing said so and nothing checked it. Truncation would compare equal across a real rename and skip the frequency-slot reload, so record why the headroom is there and static_assert the proto side of it. runOnce()'s 1ms fallback looked like dead code: expireStaleEditTransaction() uses the same threshold, so a transaction that survives it should always have elapsed < the window. It doesn't, quite - that function read millis() a moment earlier, and the window can tip over in between, in which case the subtraction underflows to ~49.7 days. The guard is load-bearing; say so, because reading it as dead is what invites its removal. Also split the two bundled LoRa no-reload tests one field per set. Flipping six fields in a single message passes as long as none of them triggers a reload, so a field mis-classified later would have hidden behind its neighbours; each now names itself on failure. native test_admin_radio: 175/175. Test status, corrected: test_admin_radio is not green on this branch and was not made green by this commit. In a clean build it reports four pre-existing assertion failures (setFavoriteNode / setIgnoredNode / toggleMutedNode / toggleNodeMuted, all "did the bit get set") and then dies with SIGILL after the last case, so PlatformIO marks the suite ERRORED. Identical at this commit, at its parent, and at 4130653 before #28, filtered or full - and absent on develop, which does not carry these tests. Unrelated to this change; tracked separately. An earlier run of this commit reported 175/175 passing. That was a stale build tree - a clean build of the same source reports 176 cases - so that number was wrong and is retracted here rather than left in the record.
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
2 participants
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.
Summary
This follow-up makes the restart reduction in meshtastic#11181 hold through the admin paths used by phone clients.
Configuration updates now make three independent decisions:
An edit transaction accumulates those decisions across every setting and applies them once at commit. Unchanged or live-applied settings no longer inherit a default reboot or radio restart merely because they were part of a larger transaction.
User-visible behavior
"sekrit"placeholder is treated as unchanged.If a client opens an edit transaction and disappears before committing, firmware commits the accepted changes after 60 seconds. The timeout thread is dormant while no transaction is open, so this recovery mechanism does not add a periodic idle wakeup.
Boot-only changes still reboot on timeout: saving them without applying them would leave runtime behavior inconsistent with the stored configuration until a later manual restart.
Root cause
saveChanges()defers writes while an edit transaction is open. The old transaction commit path did not retain the per-setting reboot and radio decisions, so committing a display-only or node-database-only batch could fall back to restart-enabled defaults.The transaction now accumulates
shouldRebootandradioAffectedindependently. Duplicate begin messages preserve the accumulated state, commit and timeout consume it once, and stale values cannot leak into a later transaction.Canonical protobuf comparison uses a
NOINLINEhelper so its temporary buffers do not accumulate in caller frames. The largest instantiation is MQTT: two 224-byte buffers, or 448 bytes of peak helper stack.BLE shutdown now happens after validation and mutation, and only when the accepted result requires a reboot. Admin messages are processed serially by the cooperative main loop, so another configuration write cannot interleave within that handler.
Validation
Current
tidyups(4130653d5) was merged at59240a171. The reviewable delta against that base remains limited toAdminModule.cpp/.hand the two admin test files.platformio test -e native-macos: 886/886 passedtest_admin_radio: 169/169 passedNarrowSlownames avoid redundant reloads, and US-to-ham transactions reload exactly once for both encodingspio run -e t-echo: passed at59240a171; 37.8% RAM, 94.4% flash, warm-region/ISR/variant guards passedtrunk fmt --ignore-git-stateon the four-file PR delta: passedgit diff --check: passed./bin/run-tests.shcould not start on macOS because it requires Bashmapfileand GNUfind -printf; the documented direct PlatformIO fallback above completed successfully.Hardware evidence
T-Echo was flashed through normal USB serial DFU with integration firmware
2.8.0.6dbd878./dev/cu.usbmodem313201: application PID0x4405, 1200-bps touch to bootloader PID0x002a, then the application returned on the same serial path.Set module config: TelemetryandSave changes to disk; no Bluetooth disable or reboot followed.Set config: SecurityandSave changes to disk; no Bluetooth disable or reboot followed.Primary-channel rename behavior is covered by native tests. The live Python client did not retain a custom rename on the default-channel T-Echo, so this does not claim on-device rename validation.