Reduce reboots and radio restarts - #11181
Open
NomDeTom wants to merge 24 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdmin configuration saves now separate disk persistence, live LoRa reloads, and reboot scheduling. Admin, menu, and WASM paths use explicit apply modes, with tests and documentation covering radio and reboot gating. ChangesAdmin configuration gating
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
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-25. Updated for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/admin-config-save-gating.md`:
- Around line 234-239: Rewrite the GPS-field validation text in the “Position
live-apply” section as one complete sentence, keeping gps_mode, gps_enabled,
gps_update_interval, and gps_attempt_time together in the parenthetical. Move
“item 3 above” outside the field list so it clearly references the broader
instruction.
- Around line 258-266: Clarify the radio-reload guidance in the “Extending this
safely” section to apply specifically to AdminModule/client configuration saves,
rather than all configuration paths. Account for the on-device menu flows
described earlier that call reloadConfig(SEGMENT_CONFIG) with default
radio-affecting behavior, either by documenting those exceptions here or
explicitly excluding them from the rule.
In `@src/mesh/MeshService.h`:
- Around line 168-175: Remove the stale `@return` documentation from
reloadConfig’s comment, keeping the parameter descriptions and behavior
documentation unchanged.
In `@src/modules/AdminModule.cpp`:
- Around line 901-931: In the meshtastic_Config_position_tag case, remove
SEGMENT_CONFIG from the nested saveChanges call after clearLocalPosition(),
leaving only the nodedb segment because config.position is updated afterward and
persisted by the final save. Update the trailing comment to accurately describe
the remaining nodedb save behavior, without implying fixed-position packets are
handled here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 20ec5f51-1ff3-4085-aebc-caebfea62d4b
📒 Files selected for processing (10)
docs/admin-config-save-gating.mdsrc/graphics/draw/MenuHandler.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/platform/portduino/wasm/portduino_glue_wasm.cpptest/test_admin_radio/test_main.cpp
NomDeTom
added
tech debt
NomDeTom
commented
Jul 24, 2026
Collaborator
Author
|
adjacent to #11202 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/admin-config-save-gating.md`:
- Around line 3-4: Update the Status entry in admin-config-save-gating.md from
“Implemented” to wording that explicitly indicates implementation is complete
but hardware validation remains outstanding, such as “Implementation complete;
hardware validation outstanding.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08c90b81-37ec-4a8e-a3e1-e91554cb6cf4
📒 Files selected for processing (10)
docs/admin-config-save-gating.mdsrc/graphics/draw/MenuHandler.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/platform/portduino/wasm/portduino_glue_wasm.cpptest/test_admin_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/modules/AdminModule.h
- src/mesh/MeshService.cpp
- src/platform/portduino/wasm/portduino_glue_wasm.cpp
- src/mesh/MeshService.h
- src/mesh/NodeDB.h
- src/modules/AdminModule.cpp
- test/test_admin_radio/test_main.cpp
- src/graphics/draw/MenuHandler.cpp
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces unnecessary LoRa radio reconfiguration and device reboots triggered by AdminModule config persistence, by separating “what to persist” (segment mask) from “does the radio need live reconfigure?” (new radioAffected flag) and tightening reboot gating for no-op / live-applicable updates.
Changes:
- Add
radioAffectedparameter toMeshService::reloadConfig()(defaulting to historical behavior) and thread it throughAdminModule::saveChanges()to suppress unnecessary live radio reconfigures. - Refine
AdminModule::handleSetConfig()reboot gating (notably for position/network/bluetooth) to avoid rebooting on byte-identical updates and to avoid rebooting when only “live-applicable” position fields change. - Add native unit tests covering radio-reload gating and reboot scheduling, and add documentation describing the new gating model.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/test_admin_radio/test_main.cpp |
Adds regression tests for radio reload suppression and reboot gating behavior. |
src/platform/portduino/wasm/portduino_glue_wasm.cpp |
Annotates a reload site for future radioAffected auditing (no behavior change yet). |
src/modules/AdminModule.h |
Extends saveChanges() signature with radioAffected. |
src/modules/AdminModule.cpp |
Implements radio reload gating via radioAffected and tightens reboot gating logic in handleSetConfig(). |
src/mesh/NodeDB.h |
Documents persistence segments vs semantic config domains (clarifies why SEGMENT_CONFIG can’t imply “radio changed”). |
src/mesh/MeshService.h |
Updates reloadConfig() signature/doc to include radioAffected with safe default. |
src/mesh/MeshService.cpp |
Applies radioAffected gate to live radio reconfigure while always persisting to disk. |
src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp |
Adds TODO markers to audit radioAffected correctness for menu-driven saves. |
src/graphics/draw/MenuHandler.cpp |
Adds TODO markers to audit radioAffected correctness for menu-driven saves. |
docs/admin-config-save-gating.md |
New documentation describing persistence vs radio reload vs reboot side effects and current gating rules. |
Comments suppressed due to low confidence (2)
test/test_admin_radio/test_main.cpp:1632
- This test header comment references "plan-narrow-radio-reload-trigger.md", but that file isn't present in the repo. Consider pointing to the in-repo documentation added by this PR instead.
// This is the same crash class as the WisMesh Tag favorite-node bug, reached through a
// different admin message (a WiFi PSK change, a Bluetooth toggle, a keypair rotation, ...).
// See plan-narrow-radio-reload-trigger.md. As with the node-DB tests above, these run
// outside an edit transaction so saveChanges() reaches reloadConfig() directly.
// -----------------------------------------------------------------------
test/test_admin_radio/test_main.cpp:1906
- This test header comment references "plan-narrow-reboot-trigger.md", but that file isn't present in the repo. It would be more maintainable to reference the doc added in this PR (or add the missing plan doc).
// A position change that touches only live-appliable fields (broadcast cadence, smart-position,
// flags - read directly from config each cycle) must apply without a reboot. Boot-only fields
// (GPS driver mode/timing, GPIO pins) still reboot. See plan-narrow-reboot-trigger.md.
// -----------------------------------------------------------------------
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp (1)
1085-1085: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not mark non-radio channel settings as radio-affecting.
uplink_enabled,downlink_enabled, andmodule_settings.position_precisiondo not change LoRa frequency, PSK, or slot configuration. PassingradioAffected=truecausesresetRadioConfig()andconfigChanged.notifyObservers()to run for each of these actions, unnecessarily restarting/reconfiguring the radio. Persist these changes without the radio flag; if a live module notification is needed, add a non-radio apply path instead of using a radio restart as a proxy.Based on
src/mesh/MeshService.cpp:138-155,radioAffected=trueexplicitly controls radio re-initialization while persistence remains independent.Proposed fix
- service->reloadConfig(SEGMENT_CHANNELS, /*radioAffected=*/true); + service->reloadConfig(SEGMENT_CHANNELS, /*radioAffected=*/false);Apply this to the four affected channel-setting call sites.
Also applies to: 1093-1093, 1109-1109, 1129-1129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp` at line 1085, Update the four affected channel-setting call sites in the menu actions around service->reloadConfig, including uplink_enabled, downlink_enabled, and module_settings.position_precision, to persist changes with radioAffected=false. Keep radioAffected=true only for settings that change LoRa frequency, PSK, or slot configuration, and use a separate non-radio notification path if live module updates are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp`:
- Line 1085: Update the four affected channel-setting call sites in the menu
actions around service->reloadConfig, including uplink_enabled,
downlink_enabled, and module_settings.position_precision, to persist changes
with radioAffected=false. Keep radioAffected=true only for settings that change
LoRa frequency, PSK, or slot configuration, and use a separate non-radio
notification path if live module updates are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aef6738c-f1c4-472c-8af1-7aaf2ef2ef98
📒 Files selected for processing (2)
src/graphics/draw/MenuHandler.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/graphics/draw/MenuHandler.cpp`:
- Around line 1906-1913: Remove the saveUIConfig() calls from both Smart
Position toggle branches in MenuHandler, which update
config.position.position_broadcast_smart_enabled. Keep the existing
service->reloadConfig(SEGMENT_CONFIG, /*radioAffected=*/false) calls so the
normal config persistence path handles the changes without writing uiconfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aab3cd52-268f-4d07-aa51-5eea0bc0ffc8
📒 Files selected for processing (10)
docs/admin-config-save-gating.mdsrc/graphics/draw/MenuHandler.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/platform/portduino/wasm/portduino_glue_wasm.cpptest/test_admin_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/modules/AdminModule.h
- src/platform/portduino/wasm/portduino_glue_wasm.cpp
- src/mesh/MeshService.h
- src/mesh/MeshService.cpp
- src/mesh/NodeDB.h
- test/test_admin_radio/test_main.cpp
- src/modules/AdminModule.cpp
- src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp
Collaborator
Author
|
@CodeRabbit did you review properly? Some of the comments are on untouched lines |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="meshtastic/firmware"
pr="11181"
echo "== PR review comments (with anchors) =="
gh api --paginate "repos/${repo}/pulls/${pr}/comments?per_page=100" \
--jq '.[] | {
id,
user: .user.login,
path,
line,
side,
start_line,
start_side,
original_line,
original_start_line,
original_position,
position,
in_reply_to_id,
body,
html_url
}'
echo "== Changed files and patches =="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | {filename, status, changes, patch}'Length of output: 96224
I should have applied the changed-code boundary more strictly rather than treating nearby unchanged statements as review scope. Sorry for the noise. 🐇 |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
(ᵕ—ᴗ—) 🐇 |
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 26, 2026
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
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 26, 2026
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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
test/test_admin_radio/test_main.cpp (2)
1515-1525: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the section banners.
Repository guidelines ask for one-to-two-line comments explaining only non-obvious reasons; these 8-10 line rationale blocks (plus references to a
plan-narrow-radio-reload-trigger.mdthat isn't in this PR) restate what the test names and asserts already say. Keeping the one-line "why" and dropping the narrative would read better and avoid stale doc pointers.As per coding guidelines: "Keep comments minimal—one or two lines—and comment only when the reason is not obvious."
Also applies to: 1633-1644, 1806-1808, 1858-1866, 1946-1953, 2067-2074, 2146-2152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_admin_radio/test_main.cpp` around lines 1515 - 1525, Trim the long rationale banners in the affected test sections to one or two lines stating only the non-obvious reason for the coverage. Remove duplicated explanations and references to external planning documents, while preserving the tests, names, and assertions unchanged.Source: Coding guidelines
1839-1856: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion doesn't test the stated equivalence.
reloadCallsis incremented by the mock's own override, sosavesBefore + 1is true by construction and says nothing aboutsaveToDiskbeing reached. The comment claims this "licenses deleting the ninesaveToDisk(X); reloadConfig(X, ...)double-writes", but nothing here observes a disk write. Either countsaveToDiskcalls (e.g. a NodeDB seam) or soften the comment to match what is actually asserted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_admin_radio/test_main.cpp` around lines 1839 - 1856, Update test_reloadConfig_radioAffectedFalse_isEquivalentToSaveToDisk so it observes actual saveToDisk activity through an appropriate NodeDB/mock seam, rather than relying on mockMeshService->reloadCalls, which is incremented by the override itself. Assert that the non-radio-affected reload performs the expected single disk write, and retain the existing no-reconfigure and in-memory value assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/AdminModule.cpp`:
- Around line 1856-1871: Update AdminModule::saveChanges and the open-edit
transaction commit path to accumulate deferred shouldReboot and radioAffected
decisions while hasOpenEditTransaction is true, preserving the strongest
requested values across multiple saves. When committing, pass the accumulated
flags explicitly to saveChanges instead of relying on its defaults, then clear
the accumulated state after the deferred save.
- Around line 901-932: Update the live-field neutralization block in the
meshtastic_Config_position_tag handler to also copy the existing
broadcast_smart_minimum_interval_secs value from live into incoming before the
memcmp check. Keep the field treated like broadcast_smart_minimum_distance so
changing it through admin or remote configuration does not require a reboot.
In `@test/test_admin_radio/test_main.cpp`:
- Around line 2051-2065: The
test_moduleConfigTelemetryScreenFlags_liveInModuleConfig test is tautological
because it only asserts values assigned directly by the test. Drive the
telemetry toggle through menuHandler::toggleTelemetryScreen, exercise the
relevant save path, and assert that moduleConfig.has_telemetry and the resulting
telemetry flags are populated by production behavior rather than set manually.
- Around line 1575-1587: Update test_setIgnoredNode_skipsRadioReload_butPersists
to explicitly create or retrieve TEST_NODE_NUM with getOrCreateMeshNode before
sending the admin message. Keep the existing configChanged assertion and
ignored-node verification unchanged.
---
Nitpick comments:
In `@test/test_admin_radio/test_main.cpp`:
- Around line 1515-1525: Trim the long rationale banners in the affected test
sections to one or two lines stating only the non-obvious reason for the
coverage. Remove duplicated explanations and references to external planning
documents, while preserving the tests, names, and assertions unchanged.
- Around line 1839-1856: Update
test_reloadConfig_radioAffectedFalse_isEquivalentToSaveToDisk so it observes
actual saveToDisk activity through an appropriate NodeDB/mock seam, rather than
relying on mockMeshService->reloadCalls, which is incremented by the override
itself. Assert that the non-radio-affected reload performs the expected single
disk write, and retain the existing no-reconfigure and in-memory value
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e6c7bbe6-fd9a-4f2f-a6bc-db75919699a1
📒 Files selected for processing (14)
docs/admin-config-save-gating.mdsrc/graphics/draw/MenuHandler.cppsrc/graphics/draw/MenuHandler.hsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/graphics/niche/InkHUD/InkHUD.hsrc/main.cppsrc/main.hsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/platform/portduino/wasm/portduino_glue_wasm.cpptest/test_admin_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/modules/AdminModule.h
- src/mesh/NodeDB.h
- src/mesh/MeshService.cpp
NomDeTom
added a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 26, 2026
Review of this branch's own output, plus the two findings Copilot raised on meshtastic#11181 after the last push. 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 meshtastic#11146 crash path, reached by the transport that matters most. 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.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/test_admin_radio/test_main.cpp (1)
2176-2190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the block comments to one or two lines.
This 15-line header (and the multi-line preambles at Lines 2203-2205, 2267-2268, 2287-2288, 2313-2314) restates rationale at length. Keep the non-obvious bit (transaction commits under a fixed full-segment mask, so only
radioAffectedgates the reload) and drop the rest; the test names carry the intent.As per coding guidelines: "Keep comments minimal—one or two lines—and comment only when the reason is not obvious."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_admin_radio/test_main.cpp` around lines 2176 - 2190, Trim the long explanatory block comments in the affected test sections, including the header before the edit-transaction deferral tests and the preambles near the referenced cases, to one or two lines. Preserve only the non-obvious rationale that transaction commits use a fixed full-segment mask, leaving radioAffected as the reload gate; rely on the test names for the remaining intent.Source: Coding guidelines
src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp (1)
677-678: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winResolve the unverified two-second reboot delay.
The comment explicitly says the reason for hard-coding
2seconds is unknown. Confirm that this delay is required for Wi-Fi recovery, or use and document the standard delay before merging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp` around lines 677 - 678, Resolve the hard-coded two-second reboot delay in the surrounding reboot logic: verify whether it is required for Wi-Fi recovery and retain it only with documentation of that requirement; otherwise replace it with DEFAULT_REBOOT_SECONDS and remove the unresolved TODO.src/modules/AdminModule.h (1)
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the comment-length convention consistently.
The changed explanatory blocks in both files exceed the repository’s one-or-two-line maximum. Keep only the essential behavioral contract and move detailed rationale into documentation.
src/modules/AdminModule.h#L54-L63: shorten thesaveChangesdocumentation.src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp#L592-L595: shorten the GPS rationale.src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp#L639-L642: shorten the reboot rationale.src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp#L1070-L1076: shorten the channel apply-mode rationale.As per coding guidelines, comments must be minimal—one or two lines maximum—and explain only non-obvious reasons.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/AdminModule.h` around lines 54 - 63, Shorten the saveChanges documentation in src/modules/AdminModule.h#L54-L63 to one or two lines covering only the deferred-commit behavior and mandatory radioAffected argument. Also shorten the GPS rationale at src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp#L592-L595, reboot rationale at `#L639-L642`, and channel apply-mode rationale at `#L1070-L1076` to one or two lines each, retaining only their non-obvious behavioral reasons and moving detailed rationale out of comments.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp`:
- Around line 677-678: Resolve the hard-coded two-second reboot delay in the
surrounding reboot logic: verify whether it is required for Wi-Fi recovery and
retain it only with documentation of that requirement; otherwise replace it with
DEFAULT_REBOOT_SECONDS and remove the unresolved TODO.
In `@src/modules/AdminModule.h`:
- Around line 54-63: Shorten the saveChanges documentation in
src/modules/AdminModule.h#L54-L63 to one or two lines covering only the
deferred-commit behavior and mandatory radioAffected argument. Also shorten the
GPS rationale at
src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp#L592-L595, reboot
rationale at `#L639-L642`, and channel apply-mode rationale at `#L1070-L1076` to one
or two lines each, retaining only their non-obvious behavioral reasons and
moving detailed rationale out of comments.
In `@test/test_admin_radio/test_main.cpp`:
- Around line 2176-2190: Trim the long explanatory block comments in the
affected test sections, including the header before the edit-transaction
deferral tests and the preambles near the referenced cases, to one or two lines.
Preserve only the non-obvious rationale that transaction commits use a fixed
full-segment mask, leaving radioAffected as the reload gate; rely on the test
names for the remaining intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 676f8944-3404-4da9-950d-c586a0eddb38
📒 Files selected for processing (9)
docs/admin-config-save-gating.mdsrc/graphics/draw/MenuHandler.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/main.cppsrc/main.hsrc/mesh/NodeDB.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.htest/test_admin_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main.cpp
- src/mesh/NodeDB.h
- src/modules/AdminModule.cpp
- docs/admin-config-save-gating.md
- src/graphics/draw/MenuHandler.cpp
reloadConfig() inferred "the radio needs reconfiguring" from whether saveWhat included SEGMENT_CONFIG. But Config is a monolithic segment (device/position/power/network/display/lora/bluetooth/security share one file), so every non-LoRa AdminModule save still fired the live SX126x reconfigure - the same live-SPI-on-the-admin-thread sequence that crashes on a favorite (meshtastic#11146), just reached via a Bluetooth toggle, WiFi PSK change, or keypair rotation. Separate the two concerns: add an explicit radioAffected flag threaded through AdminModule::saveChanges() into MeshService::reloadConfig(). The gate becomes `radioAffected && (saveWhat & (SEGMENT_CONFIG|SEGMENT_CHANNELS))` - the bitmask inference is kept as a backstop and only suppressible, and radioAffected defaults to true so the ~35 existing reloadConfig() callers (MenuHandler, MenuApplet, portduino) keep today's behavior untouched. Only handleSetConfig()'s lora_tag sets radioAffected; every other sub-message, plus set_fixed_position/remove_fixed_position and position's nested save, opt out. set_channel/commit_edit_settings/restore keep the default and still reload. Add native coverage in test_admin_radio for each non-LoRa sub-message (asserting no reconfigure), regression guards that lora/set_channel still reconfigure, and direct reloadConfig() guards pinning the fail-safe default.
handleSetConfig() starts requiresReboot=true and device/power/display each gate it back down when nothing reboot-worthy changed, but position, network, and bluetooth never touched requiresReboot - so they rebooted on *every* set, including a client re-pushing byte-identical config (which happens routinely on connect/retry). Add a no-op gate to those three: if the incoming sub-message is byte-identical to the current one, skip the reboot. A whole-struct memcmp is the right tool here (unlike the field-by-field device/power/display gates, which must ignore benign fields) - it answers "did anything change?" and fails safe: its only error mode is padding-byte differences causing an unnecessary reboot, never a missed change. All three sub-messages are POD (no pb_callback_t fields). Any real change still reboots exactly as before. This is Tier 1 of plan-narrow-reboot-trigger; Tier 2 will further narrow position to reboot only on boot-only fields (GPIO/GPS). Adds native coverage asserting rebootAtMsec stays unset on a no-op set and is armed on a real change, for each of the three sub-messages.
Tier 2 of plan-narrow-reboot-trigger. A position set that touches only fields the position module consumes live now applies without restarting. PositionModule reads position_broadcast_secs, position_broadcast_smart_enabled, broadcast_smart_minimum_distance, position_flags, and fixed_position directly from config on every send/schedule cycle (fixed_position also has dedicated live admin handlers) - changing only those needs no reboot. Everything else stays on the reboot path: GPS driver state (gps_mode/gps_enabled/ gps_update_interval/gps_attempt_time) and GPIO pin assignments (rx_gpio/ tx_gpio/gps_en_gpio), all of which touch subsystem/hardware init. The live set is deliberately limited to what static analysis proves is applied live, so this ships without hardware verification; GPS-timing fields that might also be live were left rebooting (fail toward current behavior). The gate neutralizes the live fields in a copy and reboots if any other byte differs, so a future PositionConfig field reboots until explicitly cleared as live - fail safe for schema growth. This supersedes the Tier 1 no-op memcmp gate for position (network/bluetooth keep theirs). Native coverage: a broadcast-interval change does not schedule a reboot; gps_mode and rx_gpio changes still do.
Document the AdminModule config-save side-effect work: the radioAffected and requiresReboot axes, which operations now skip the radio reload or the reboot, and the operations deliberately left unchanged (commit_edit_settings, network/bluetooth live-apply, GPS-timing position fields, module config, and the on-device menu reloadConfig sites) with the reason for each.
The doc only mentioned the outstanding GPS-timing pass in one line and omitted the radio-reload/crash validation entirely, with no procedure. Add a Hardware testing section covering: the meshtastic-mcp setup (BLE connected throughout, nRF52840 SX126x reference board); the radio-reload/crash regression guard for the favorite-node fix (with a lora/set_channel positive control); and a per-field procedure with pass/fail criteria for deciding whether GPS-timing position fields can be reclassified as live - fail toward rebooting.
The hardware section implied a BLE connection was required and that the crash ran on the BLE callback thread. On nRF52 the BLE onWrite only queues; handleToRadio -> saveChanges -> reloadConfig runs on the main FreeRTOS task, same context SerialConsole uses. So serial drives the exact code/thread under test, the on-device menus are unrelated (separate path), and the original crash was serial-proven. State that serial is sufficient and correct the thread references accordingly.
The previous edit, while right that the reconfigure is invoked on the main task (not the BLE callback thread), oversimplified the failure. The crash is cross-thread: the main-task setStandby()+SPI reprogram collides with the radio's off-main NotifiedWorkerThread over the shared non-recursive spiLock (FreeRTOS binary semaphore), locking it up and tripping the watchdog reboot (meshtastic#11146; same spiLock hazard as meshtastic#10705/meshtastic#10728). Restore that in both the Axis 1 rationale and the hardware test-1 description. Does not change the serial-is-sufficient conclusion (the collision is transport-independent).
The three moduleConfig.telemetry.*_screen_enabled toggles in the frame-toggle menu changed the value in memory and never wrote it, so they reverted on any reboot that did not happen to go through the reboot menu (which saves every segment). Their sibling entries in the same menu persist via Screen::toggleFrameVisibility -> saveFrameVisibility, so half the menu was durable and three entries were not. These three live in moduleConfig rather than the hiddenFrames blob, so they need their own SEGMENT_MODULECONFIG save. radioAffected is false: a screen preference has no business re-initialising the LoRa chip. clod helped too
reloadConfig() ends with an unconditional saveToDisk(saveWhat), outside the radioAffected guard, so nine sites in the InkHUD menu were writing the same proto file twice in a row: saveToDisk(X) immediately followed by reloadConfig(X, ...). One of the nine is the shared applyConfigReload() helper, so thirteen menu actions were affected. Five sites in the BaseUI menu called saveUIConfig() after changing a field that lives in config.proto. saveUIConfig() only writes /prefs/uiconfig.proto, so those writes could not persist the changed field and did nothing but cost a flash write. GPSFormatMenu had the mirror image: a uiconfig-only change that also called reloadConfig(), rewriting config.proto for a field not in it. Three bare saveToDisk() calls rewrote all five segments to change one bit - a channel mute flag and two NodeInfoLite bits - so they now pass the segment they actually touch. NodeInfoLite is written by saveNodeDatabaseToDisk() under SEGMENT_NODEDATABASE, not SEGMENT_DEVICESTATE. The reboot menu keeps its bare call: a full flush before a deliberate reboot is correct. reloadConfig() becomes virtual so tests can count calls; the accompanying test pins down the saveToDisk equivalence the nine deletions rely on. clod helped too
Applying a config change involves three independent decisions: which proto files to persist, whether the LoRa chip needs re-initialising, and whether the field only takes effect after a restart. Those were spread across four helpers with three different parameter orders, two of which took adjacent bools that compile fine when transposed. InkHUD's applyConfigReload was the sharp edge: its second parameter was `reboot`, sitting exactly where reloadConfig and saveChanges take `radioAffected`. There is now a single entry point taking a flags enum, so each call site states its intent and cannot silently mean the opposite. Migrated 23 BaseUI sites, 20 InkHUD sites and the wasm glue. applyConfigReload is deleted and its five callers inlined; applyLoRaRegion and applyLoRaPreset stay, since they hold real domain logic beyond bundling, and just delegate. Adjacent `rebootAtMsec = millis() + ...` assignments fold into the reboot flag. The one caller that deliberately uses a shorter delay keeps it via the trailing rebootSeconds parameter. Reboot scheduling itself moves into requestReboot() in main.cpp, next to the global it sets: previously only AdminModule had a helper for this, and it was private, which is why every menu open-coded the deadline. requestReboot carries no UI, because BaseUI already renders the notice at draw time whenever rebootAtMsec is set. saveChanges keeps its signature and its edit-transaction deferral, so its fourteen call sites and the existing gating tests are untouched. clod helped too
None of the menu save behaviour was reachable from a test: every action lived in a lambda assigned to BannerOverlayOptions.bannerCallback, which only ever runs via screen->showOverlayBanner(), so exercising one needed a live Screen. That is why the defects this series fixes went unnoticed - MenuHandler.cpp compiles in the native test build, but nothing in it could be called. Three actions move out into functions that own the whole decision: which segment to persist, whether the radio needs reconfiguring, and whether to reboot. Chosen because each covers a defect this series touched - the telemetry toggles that never persisted, the smart-position reboot that PR meshtastic#11181 removed, and a node-DB bit write that must not reach the radio. Deliberately picked extractions that also deduplicate: three telemetry call sites collapse onto one function and two smart-position sites onto another, so the promicro image is byte-identical to before. Worth knowing, because that board has under 1.4 KB of headroom before the warmstore region. toggleNodeMuted also gains a guard for an unknown node, so a stale pickedNodeNum can no longer cause a pointless flash write. clod helped too
notifyApplyingChanges() was doing two jobs at its call sites in MenuApplet.cpp: signalling a live change that is applied without restarting, and warning of an imminent reboot. Audited every site. The finding is that none of them are redundant, so this records why rather than removing anything. The two live-change calls - LoRa region and modem preset - cover the seconds the e-ink takes to redraw while the radio reconfigures, and nothing else raises them. The reboot-path calls warn before the display goes. requestReboot() cannot absorb those: it deliberately carries no UI, because BaseUI renders its own notice at draw time from rebootAtMsec, while e-ink only draws when pushed. Also worth recording: the existing notifyReboot Observable (sleep.h, fired from Power::reboot) is a different moment - reboot execution, not scheduling - and InkHUD already observes it to save settings and shut applets down. A new "reboot scheduled" observable to centralise these calls would be a third reboot signal for no functional gain, so it is not added. clod helped too
Addresses review feedback. Four comments pointed at plan documents that were never committed (plan-narrow-reboot-trigger.md, plan-decouple-nodedb-admin-saves.md), so the rationale they cited was not discoverable. They now point at docs/admin-config-save-gating.md, which is in-repo. The doc listed the commit SHAs it described. All five had already been orphaned by the rebases this branch has been through, exactly as predicted, so it cites the PR instead. "Status: Implemented" also overstated things while hardware validation is still outstanding. The doc's biggest problem was that it had gone out of date within its own PR: it described the on-device menus as an untouched code path that still reloads the radio on any Config save, with per-site TODO markers. That stopped being true when the menus moved onto applyConfigChange. Replaced with a section covering the new entry point, the flags, why they are a flags enum rather than two bools, and the saveToDisk equivalence that makes pairing the two calls a double write. clod helped too
Four review findings. broadcast_smart_minimum_interval_secs is not read live, despite the comment this branch added claiming it was. PositionModule::minimumTimeThreshold is a const data member initialised when the module is constructed, so the value is captured at boot; only the sibling distance field is genuinely re-read (PositionModule.cpp :630). The InkHUD menu therefore applied it with no reboot and it silently did nothing until the next restart. It now reboots, matching the AdminModule path. Note the review suggested the opposite fix - adding the field to AdminModule's live-field list - which would have made the admin path silently ineffective too. While an edit transaction is open saveChanges() defers the write, which threw away the per-field reboot and radio decisions; the commit then used the parameter defaults and always rebooted and reconfigured the radio. Since phone apps write config through transactions, none of this branch's narrowing reached them. The deferred decisions now accumulate and the commit honours them. Two test fixes: the set_ignored_node test relied on an earlier RUN_TEST having created the node, which breaks under -f or a registration reorder. And the telemetry test asserted only values it had just assigned - replaced with one that drives the real toggle and checks the save path sets has_telemetry, the flag whose absence caused the TAK persistence bug fixed upstream in meshtastic#11216. clod helped too
Review of this branch's own output, plus the two findings Copilot raised on The headline defect is that none of this branch's narrowing reached phone apps. saveChanges() defers while an edit transaction is open, and radioAffected defaulted to true, so eight call sites that never thought about the radio - the five node-DB handlers, set_owner, set_module_config, and the nested save in the position case - accumulated a true into deferredRadioAffected. Outside a transaction that was harmless, because reloadConfig()'s saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS) bitmask independently blocks a node-DB-only save from reaching the radio whatever it asks for. Inside one the commit saves under a fixed full mask, so the bitmask always passes and radioAffected is the only thing left deciding it. Favouriting a node from the phone app therefore still ran the live SX126x reconfigure at commit - the The fix is to remove the default from saveChanges() rather than gate the deferred flag on the segment mask, which is what the review suggested. Gating would reinstate exactly the segment-to-radio inference this branch exists to delete - NodeDB.h now carries a comment telling the next person not to do that - and it fixes the symptom while leaving the wrong default in place for the next call site. With no default the compiler makes all fourteen sites state the answer. The commit also consumes and clears the deferred flags, so a stray second commit cannot inherit the previous transaction's answer. That gap existed because every radio test ran outside a transaction, which is the one arrangement where the bug is unreachable. Nine tests now cover the deferred path, asserting both axes independently across a commit. Verified they fail against the old behaviour: five go red while every pre-existing test stays green, which is the point. requestReboot()'s comment had the semantics backwards - it claimed a negative delay meant "now" and that rebootAtMsec == 0 was an immediate-reboot sentinel. Both are inverted: 0 means no reboot pending at every read site, and admin.proto documents reboot_seconds "<0 to cancel reboot". The expression was a faithful copy of AdminModule's, so only the comment was wrong, but it was wrong on the newly-created central helper. The negative branch now says so and logs it. Five sites still open-coded the deadline despite that helper existing; they are pure reboots with no config save, so applyConfigChange() does not fit but requestReboot() does exactly. SET_SMART_BROADCAST_INTERVAL was the only CONFIG_APPLY_REBOOT site in the InkHUD menu without a notifyApplyingChanges() beside it - re-adding its reboot last commit dropped the warning that applyConfigReload() used to raise, so the e-ink would go dark unannounced. And four channel actions carried CONFIG_APPLY_RADIO for uplink/downlink and position_precision, none of which touch the name, PSK or frequency slot the radio derives anything from; they had it only because the old reloadConfig(SEGMENT_CHANNELS) inferred it from the bitmask. The doc had gone stale inside its own PR again: it still listed commit_edit_settings under "intentionally left unchanged". Replaced with a section on why the bitmask stops protecting you inside a transaction, since that is the non-obvious part. Also dropped a commit SHA that had already been orphaned by rebase, exactly as its own note predicted, and a stale plan-doc reference in the tests that the last doc pass missed. Full native suite green, 810 cases across 40 suites. t-echo and t-echo-inkhud both build, covering the menu changes no native env compiles.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/admin-config-save-gating.md`:
- Around line 288-297: Correct the transport-scope statements in the
documentation so they distinguish the AdminModule/client-protocol path covered
by serial or BLE testing from the separately changed on-device menu path. Update
the claims around the “On-device menus” section and the transport guidance to
state that menu behavior requires independent validation, without implying those
paths were untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8777fba9-92da-4122-a661-f3c1496696f1
📒 Files selected for processing (14)
docs/admin-config-save-gating.mdsrc/graphics/draw/MenuHandler.cppsrc/graphics/draw/MenuHandler.hsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/graphics/niche/InkHUD/InkHUD.hsrc/main.cppsrc/main.hsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/platform/portduino/wasm/portduino_glue_wasm.cpptest/test_admin_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- src/modules/AdminModule.h
- src/mesh/MeshService.cpp
- src/main.h
- src/graphics/niche/InkHUD/InkHUD.h
- src/graphics/draw/MenuHandler.h
- src/main.cpp
- src/platform/portduino/wasm/portduino_glue_wasm.cpp
- src/mesh/NodeDB.h
- src/mesh/MeshService.h
- src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp
- test/test_admin_radio/test_main.cpp
- src/modules/AdminModule.cpp
- src/graphics/draw/MenuHandler.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/support/AdminModuleTestShim.h`:
- Around line 23-27: Shorten the comment above the open-edit transaction
handling in the AdminModuleTestShim to one concise line explaining that it
mirrors begin_edit_settings by refreshing the timestamp and resetting deferred
decisions; remove the details that restate the method body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b45c53a8-611a-403b-80ca-23dca1d57e02
📒 Files selected for processing (4)
docs/admin-config-save-gating.mdsrc/modules/AdminModule.cpptest/support/AdminModuleTestShim.htest/test_admin_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_admin_radio/test_main.cpp
- src/modules/AdminModule.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Config saves currently conflate three independent decisions: which
/prefsfiles to persist, whether the LoRa chip needs re-initialising, and whether a reboot is required. They were spread across four helpers with three different parameter orders, two of which took adjacentbools. This narrows each side effect to the cases that need it, then unifies the entry point.Why
reloadConfig()ends with an unconditionalsaveToDisk(saveWhat)outside its radio-reload guard. Two consequences:SEGMENT_CONFIGre-initialised the SX126x, even for a display setting. That SPI reconfigure is the mechanism behind the WisMesh Tag favourite-node crash.saveToDisk(X); reloadConfig(X, …)writes the same proto file twice. Nine InkHUD menu sites did this, one of them a shared helper with five callers — thirteen menu actions affected.InkHUD's
applyConfigReload(changes, reboot)was the sharp edge: its second parameter sat exactly wherereloadConfigandsaveChangestakeradioAffected, sotruemeant the opposite of how it read.What changed
Radio reloads are now opt-in per save.
reloadConfiggainsradioAffected;handleSetConfigsets it only for the LoRa sub-message. Node-DB saves (favourite/ignore/mute) and every non-LoRa config sub-message no longer touch the radio.Reboots are dropped where the field is read live — position broadcast interval, smart-position toggle — and suppressed for no-op sets, so writing an unchanged Bluetooth config no longer drops the BLE link the phone is using. GPS-timing fields still reboot;
docs/admin-config-save-gating.mdrecords which fields need what and why.Redundant writes removed. Nine double-writes; five BaseUI
saveUIConfig()calls against fields that live inconfig.proto, notuiconfig.proto; one inverse case; three baresaveToDisk()calls that rewrote all five segments to change one bit.One entry point.
MeshService::applyConfigChange(saveWhat, flags, rebootSeconds)with a flags enum, so each of the 44 call sites states its intent and cannot silently mean the opposite.applyConfigReloadis deleted;applyLoRaRegion/applyLoRaPresetstay, since they hold real domain logic, and delegate.AdminModule::saveChangeskeeps its signature and its edit-transaction deferral. Reboot scheduling moves intorequestReboot()next torebootAtMsec— previously only AdminModule had a helper for this and it was private, which is why every menu open-coded the deadline.Menu actions are now testable. Every menu save lived in a lambda reachable only via
screen->showOverlayBanner(), so none of it could be exercised — which is why these defects went unnoticed. Three actions are lifted into plain functions.Bug fixed along the way
The three telemetry screen toggles in the frame-toggle menu changed
moduleConfigin memory and never persisted it, so they reverted on any reboot that didn't go through the reboot menu. Their sibling entries in the same menu persist viatoggleFrameVisibility, so half that menu was durable and three entries weren't.Testing
test/test_admin_radio— 117 cases, all passing (./bin/run-tests.sh -e native -f test_admin_radio). Covers per-sub-message radio-reload gating, reboot gating including no-op sets, the flags semantics and their composition, thereloadConfig(X, false) == saveToDisk(X)equivalence the deletions rely on, and the extracted menu actions.Flash,
nrf52_promicro_diy_tcxo(the tightest target, 98.3% used with ~1.3 KB real headroom before the warmstore region):Every intermediate commit passes the warm-region guard; peak was +48 B.
Not verified
MenuApplet.cppis compile-verified but not link-verified. All nRF52 InkHUD envs currently fail to link ondevelopwithError: offset out of rangein LTO codegen — reproduced on clean6d944e44dwith none of these commits, so it's a pre-existing upstream break, but it does mean the InkHUD changes here haven't been through a full link.🤝 Attestations
Summary by CodeRabbit
Summary