Skip to content

Commit 8992a33

Browse files
committed
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
1 parent 0d19eab commit 8992a33

3 files changed

Lines changed: 47 additions & 17 deletions

File tree

docs/admin-config-save-gating.md

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
# AdminModule config-save side effects: radio-reload & reboot gating
22

3-
**Status:** Implemented
3+
**Status:** Implementation complete; hardware validation outstanding
44
**Date:** 2026-07-23
55
**Area:** `src/modules/AdminModule.cpp` (`handleSetConfig`, `saveChanges`), `src/mesh/MeshService.cpp` (`reloadConfig`), `src/mesh/NodeDB.h` (`SEGMENT_*`)
6-
**Commits:** `babeef08d` (radio-reload), `65c27f813` + `273fa8d0a` (segment docs / TODO markers), `71d711867` (reboot Tier 1), `cc9abdbbc` (reboot Tier 2)
6+
**Reference:** meshtastic/firmware#11181 (commit SHAs omitted - they do not survive rebase or squash merge)
77

88
Saving a config over the phone/BLE used to do more than persist bytes: it could re-init the
99
LoRa radio **and** reboot the device, on config that touched neither. This document records
@@ -161,21 +161,52 @@ is worth.
161161
Separating the few that plausibly don't need one (canned messages, ambient lighting,
162162
status message) needs a per-module audit - its own effort, not part of this work.
163163

164-
5. **On-device menu `reloadConfig` sites** - 27 `reloadConfig(SEGMENT_CONFIG)` /
165-
`reloadConfig(changes)` calls in `src/graphics/draw/MenuHandler.cpp` and
166-
`src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp` still reload on any Config
167-
save (they pass only `saveWhat`; the default `radioAffected=true` preserves this).
168-
**Why untouched:** this work was scoped to the AdminModule (client admin-message) crash class.
169-
The menu paths are a mix of genuinely-radio changes (region/preset) and incidental ones
170-
(role, display units), run on the UI thread. Each is marked with a
171-
`// TODO(radioAffected)` note (`audit` vs `radio-affecting`) for a future per-site pass.
164+
5. ~~**On-device menu `reloadConfig` sites**~~ - **now done, see "On-device menus" below.**
165+
These were initially left alone as out of scope, then migrated in the same PR.
172166

173167
For contrast, `set_channel` ([`:1445`](../src/modules/AdminModule.cpp#L1445)) and
174168
`restore_preferences` ([`:1911`](../src/modules/AdminModule.cpp#L1911)) also still reload -
175169
but that is _correct_, not conservative: both genuinely change LoRa/channel state.
176170

177171
---
178172

173+
## On-device menus (`applyConfigChange`)
174+
175+
Both UIs go through one entry point on `MeshService`, so a menu action states the same two
176+
decisions AdminModule does rather than inheriting defaults:
177+
178+
```cpp
179+
void applyConfigChange(int saveWhat, uint8_t flags, int32_t rebootSeconds = DEFAULT_REBOOT_SECONDS);
180+
181+
enum ConfigApplyFlags : uint8_t {
182+
CONFIG_APPLY_NONE = 0, // persist only
183+
CONFIG_APPLY_RADIO = 1 << 0, // region/preset/freq/channel/PSK - re-init the LoRa chip
184+
CONFIG_APPLY_REBOOT = 1 << 1, // field only takes effect after a restart
185+
};
186+
```
187+
188+
`flags` has no default: the point is that every call site says what it wants. A flags enum
189+
rather than two bools because `reboot` and `radioAffected` are both bools, sat in different
190+
positions across the helpers this replaced, and transposing them compiled silently. InkHUD's
191+
`applyConfigReload(changes, reboot)` took `reboot` exactly where `reloadConfig` and
192+
`saveChanges` take `radioAffected`.
193+
194+
Notes for anyone extending this:
195+
196+
- `reloadConfig(X, /*radioAffected=*/false)` is **exactly equivalent** to `saveToDisk(X)` - the
197+
save is unconditional, outside the guard. So never pair them: that writes the file twice.
198+
- **AdminModule keeps using `saveChanges`**, not `applyConfigChange` directly. It owns the
199+
edit-transaction deferral, without which a multi-field remote set writes flash per field.
200+
- Reboots go through `requestReboot()` (`src/main.h`). It carries no UI: BaseUI already renders
201+
the notice at draw time from `rebootAtMsec`, while InkHUD's e-ink only draws when pushed and so
202+
raises `notifyApplyingChanges()` explicitly.
203+
- `rebootSeconds` exists for one caller - InkHUD's wifi-recovery path uses a shorter delay than
204+
`DEFAULT_REBOOT_SECONDS`.
205+
- UI-only state stays out of this: BaseUI `uiconfig` fields use `saveUIConfig()`, and InkHUD has
206+
its own non-protobuf `Persistence::Settings` store. Neither is in the `/prefs` protobuf tree.
207+
208+
---
209+
179210
## Verification
180211
181212
Native coverage lives in `test/test_admin_radio/test_main.cpp`:
@@ -258,9 +289,8 @@ a cheap confidence test but not a gate.
258289
259290
- To stop a config field from reloading the radio on an **AdminModule/client config save**: it
260291
already doesn't, unless it's `lora`. Do **not** widen `radioAffected` to non-LoRa config -
261-
only `RadioInterface::reconfigure()` (which reads `config.lora`) consumes it. This does not
262-
apply to the on-device menu `reloadConfig` sites (item 5 above), which still reload on any
263-
Config save regardless of field - that is a separate, untouched code path.
292+
only `RadioInterface::reconfigure()` (which reads `config.lora`) consumes it. The on-device
293+
menus are now gated the same way, via `applyConfigChange` (see below).
264294
- To move a field off the reboot path: confirm it is consumed live (read from `config` at use
265295
time, no cached/driver state), add it to the live set in the relevant `handleSetConfig`
266296
case, and add a native case asserting no reboot on its change. For anything driver- or

src/modules/AdminModule.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -906,7 +906,7 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
906906
// only them takes effect with no restart. Everything else - GPS driver mode/timing and GPIO pin
907907
// assignments - stays on the reboot path. Fails safe: neutralize the known-live fields in a copy,
908908
// then reboot if any *other* byte differs, so a newly-added PositionConfig field reboots until it
909-
// is explicitly cleared as live here. See plan-narrow-reboot-trigger.
909+
// is explicitly cleared as live here. See docs/admin-config-save-gating.md.
910910
{
911911
meshtastic_Config_PositionConfig live = config.position, incoming = c.payload_variant.position;
912912
incoming.position_broadcast_secs = live.position_broadcast_secs;

test/test_admin_radio/test_main.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1519,7 +1519,7 @@ static void test_warn_license_transaction_coalescedToSingleMessage()
15191519
// (which drives the live SX126x/RadioInterface reconfigure) when saveWhat includes
15201520
// SEGMENT_CONFIG or SEGMENT_CHANNELS. Pure node-DB metadata saves - favorite, ignore,
15211521
// mute - must skip that reconfigure entirely: it's the crash in the WisMesh Tag
1522-
// favorite-node bug (see plan-decouple-nodedb-admin-saves.md). These tests watch
1522+
// favorite-node bug (see docs/admin-config-save-gating.md). These tests watch
15231523
// service->configChanged directly so a regression (someone widening the saveWhat mask,
15241524
// or reordering the check) is caught even though these run outside an edit transaction.
15251525
// -----------------------------------------------------------------------
@@ -2069,7 +2069,7 @@ static void test_moduleConfigTelemetryScreenFlags_liveInModuleConfig()
20692069
//
20702070
// position/network/bluetooth used to reboot on *every* set, including a client re-pushing
20712071
// byte-identical config. A no-op set must now leave rebootAtMsec unset; any real change still
2072-
// schedules the reboot exactly as before. See plan-narrow-reboot-trigger.md. reboot() only
2072+
// schedules the reboot exactly as before. See docs/admin-config-save-gating.md. reboot() only
20732073
// arms rebootAtMsec (it does not exit), so that is the signal we assert on.
20742074
// -----------------------------------------------------------------------
20752075

@@ -2148,7 +2148,7 @@ static void test_setConfigBluetooth_realChange_schedulesReboot()
21482148
//
21492149
// A position change that touches only live-appliable fields (broadcast cadence, smart-position,
21502150
// flags - read directly from config each cycle) must apply without a reboot. Boot-only fields
2151-
// (GPS driver mode/timing, GPIO pins) still reboot. See plan-narrow-reboot-trigger.md.
2151+
// (GPS driver mode/timing, GPIO pins) still reboot. See docs/admin-config-save-gating.md.
21522152
// -----------------------------------------------------------------------
21532153

21542154
static void test_setConfigPosition_liveFieldChange_doesNotReboot()

0 commit comments

Comments
 (0)