fix(admin): persist TAK module config (team color / member role) - #11216
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 (31)
Build artifacts expire on 2026-08-24. Updated for |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughTAK module configuration now supports admin set/get handling and persistence. New native tests cover TAK values and all compiled-in module configuration submessages across save/load and readback flows. ChangesTAK module configuration lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Setting TAK team/role ACKed and rebooted but stored nothing: handleSetModuleConfig had no tak case, saveToDisk never set has_tak, and handleGetModuleConfig had no TAK_CONFIG case. Add all three, plus a native suite sweeping every ModuleConfig submessage through set -> save -> load -> get and a TAK value-fidelity suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jamesarich
force-pushed
the
fw-6430-handoff
branch
from
July 25, 2026 16:44
3d5bb7a to
bb9c72e
Compare
jamesarich
marked this pull request as ready for review
July 25, 2026 16:57
thebentern
approved these changes
Jul 25, 2026
madeofstown
pushed a commit
to madeofstown/meshtastic-firmware
that referenced
this pull request
Jul 26, 2026
…htastic#11216) Setting TAK team/role ACKed and rebooted but stored nothing: handleSetModuleConfig had no tak case, saveToDisk never set has_tak, and handleGetModuleConfig had no TAK_CONFIG case. Add all three, plus a native suite sweeping every ModuleConfig submessage through set -> save -> load -> get and a TAK value-fidelity suite. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 26, 2026
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
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 27, 2026
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
NomDeTom
pushed a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 30, 2026
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
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
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
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
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
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
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
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>
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Aug 12, 2026
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
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>
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.
Setting TAK Team Color or Member Role from a client appeared to save — the node ACKed and rebooted — but both values came back "Unspecified" after every reboot. The client apps were sending a correct
set_module_configwith theModuleConfig.taksubmessage populated; the firmware had no write path for it.Fixes #11182
Closes #9691
Fixes meshtastic/Meshtastic-Android#6430
Root cause — three gaps, each silent on its own
AdminModule::handleSetModuleConfig()had nomeshtastic_ModuleConfig_tak_tagcase. The submessage fell through the switch, and the handler still ransaveChanges(SEGMENT_MODULECONFIG, shouldReboot)and returnedtrue— so the node ACKed success and rebooted having stored nothing.NodeDB::saveToDisk()never setmoduleConfig.has_tak. Even a correctly populated struct would be omitted by nanopb on the flash write, sinceLocalModuleConfig.takis an optional field gated on its presence flag.AdminModule::handleGetModuleConfig()had noTAK_CONFIGcase, so remote admin reads returned an empty response. (The local PhoneAPI read path was already fixed in PhoneAPI: add missing tak_tag case + skip reserved gap in module-config iteration #10256; its PR description stated the admin set path was handled, but it was not — this PR completes that. enhancement: Add on-device TAK team/role configuration #9716 would have wired this up but was closed unmerged.)Changes
src/modules/AdminModule.cpp— add thetakcase tohandleSetModuleConfig()(copies the submessage, setshas_tak) and theTAK_CONFIGcase tohandleGetModuleConfig()src/mesh/NodeDB.cpp— setmoduleConfig.has_tak = truealongside the other presence flags in theSEGMENT_MODULECONFIGsave blockTests
Two new native suites (registered in
test/native-suite-count, docs updated):test_module_config— the bug class generalized: everyModuleConfigsubmessage is swept through the full lifecycle (admin set → encode, assaveToDiskwould → decode into the live global, as a reboot would → admin get answers with that submessage, for remote and local requesters), enumerating the generated proto constants so a future submessage is covered without registration. A companion test asserts sequential sets don't clobber earlier submessages.test_tak_config— TAK value fidelity: team/role stored verbatim, clearing back to Unspecified persists as an explicit edit, values survive the save/load round-trip, and both remote and localTAK_CONFIGreads return them.Both suites fail on the pre-fix code naming the offending submessage (
dropped on save, presence flag not set: ModuleConfig submessage tag 16) and pass with the fix. Full native suite: all 40 suites pass, including under ASan/LSan viabin/test-native-docker.sh.Notes for reviewers
saveToDisk()also never forceshas_remote_hardware/has_traffic_management. That is benign today (their admin set cases set the flags, so user-set values persist) and left untouched here to keep this change scoped to TAK.Capabilities.ktgatessupportsTakConfigon protobuf availability (>= 2.7.19) rather than on a firmware version that actually implements the write path, so the app offered a working-looking editor (plus a reboot) against firmware that ignored it. Worth a version-floor bump on the app side once this ships.Hardware verification (Seeed Wio Tracker L1, nRF52840)
2.8.0.ef1aedd):set_module_config(tak)with Team=Cyan / Role=Medic was ACKed and the node rebooted, but the values read back as Unspecified — the reported symptom, reproduced.2.8.0.bb9c72e, this branch): the same set lands in RAM (adminTAK_CONFIGget returns Cyan/Medic before the reboot), survives the save + reboot (post-reboot admin get returns Cyan/Medic), and the PhoneAPI config dump sendsmoduleConfig { tak { team: Cyan role: Medic } }on the wire.takandstatusmessagesections when accumulatingnode.moduleConfigfrom the config dump, so--get-style reads through that lib show stale zeros even though the wire traffic is correct.🤝 Attestations
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests