Avoid unnecessary reboots in admin edit transactions (#28)

RCGV1 · NomDeTom · web-flow · commit 6152aebbb7d1 · 2026-07-30T17:33:07.000+01:00
* 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 (#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 (#11146; same spiLock hazard as #10705/#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 #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 #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>
diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp
@@ -10,6 +10,7 @@
 #include "SPILock.h"
 #include "gps/RTC.h"
 #include "input/InputBroker.h"
+#include "mesh/mesh-pb-constants.h"
 #include "meshUtils.h"
 #include <ErriezCRC32.h>
 #include <FSCommon.h>
@@ -70,6 +71,10 @@
 
 AdminModule *adminModule;
 
+#ifdef PIO_UNIT_TESTING
+uint32_t disableBluetoothCallCountForTest = 0;
+#endif
+
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
 static bool licensedIdentityWillMigrate()
 {
@@ -94,6 +99,37 @@ static void writeSecret(char *buf, size_t bufsz, const char *currentVal)
     }
 }
 
+template <size_t MaxSize> static NOINLINE bool protobufsEqual(const pb_msgdesc_t *fields, const void *left, const void *right)
+{
+    // NOINLINE limits peak stack use to this frame: two MaxSize encode buffers.
+    uint8_t leftBytes[MaxSize], rightBytes[MaxSize];
+    const size_t leftSize = pb_encode_to_bytes(leftBytes, sizeof(leftBytes), fields, left);
+    const size_t rightSize = pb_encode_to_bytes(rightBytes, sizeof(rightBytes), fields, right);
+    return leftSize == rightSize && memcmp(leftBytes, rightBytes, leftSize) == 0;
+}
+
+static NOINLINE bool loraRadioConfigChanged(const meshtastic_Config_LoRaConfig &oldConfig,
+                                            const meshtastic_Config_LoRaConfig &newConfig)
+{
+    auto oldEffective = oldConfig;
+    auto newEffective = newConfig;
+    if (oldEffective.use_preset && newEffective.use_preset) {
+        oldEffective.bandwidth = newEffective.bandwidth = 0;
+        oldEffective.spread_factor = newEffective.spread_factor = 0;
+        oldEffective.coding_rate = newEffective.coding_rate = 0;
+    }
+    newEffective.hop_limit = oldEffective.hop_limit;
+    newEffective.tx_enabled = oldEffective.tx_enabled;
+    newEffective.override_duty_cycle = oldEffective.override_duty_cycle;
+    // The admin setter applies this immediately through RF95_FAN_EN where supported.
+    newEffective.pa_fan_disabled = oldEffective.pa_fan_disabled;
+    newEffective.ignore_incoming_count = oldEffective.ignore_incoming_count;
+    memcpy(newEffective.ignore_incoming, oldEffective.ignore_incoming, sizeof(newEffective.ignore_incoming));
+    newEffective.ignore_mqtt = oldEffective.ignore_mqtt;
+    newEffective.config_ok_to_mqtt = oldEffective.config_ok_to_mqtt;
+    return !protobufsEqual<meshtastic_Config_LoRaConfig_size>(&meshtastic_Config_LoRaConfig_msg, &oldEffective, &newEffective);
+}
+
 /**
  * @brief Handle received protobuf message
  *
@@ -482,14 +518,18 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
     }
     case meshtastic_AdminMessage_begin_edit_settings_tag: {
         LOG_INFO("Begin transaction for editing settings");
+        if (!hasOpenEditTransaction) {
+            deferredEditSegments = 0;
+            deferredShouldReboot = false;
+            deferredRadioAffected = false;
+        }
         hasOpenEditTransaction = true;
         editTransactionActivityMs = millis();
-        deferredShouldReboot = false;
-        deferredRadioAffected = false;
+        enabled = true;
+        setIntervalFromNow(EDIT_TRANSACTION_IDLE_MS);
         break;
     }
     case meshtastic_AdminMessage_commit_edit_settings_tag: {
-        disableBluetooth();
         LOG_INFO("Commit transaction for edited settings");
         hasOpenEditTransaction = false;
         deferredEditSegments = 0;
@@ -498,6 +538,9 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
         const bool commitReboot = deferredShouldReboot, commitRadio = deferredRadioAffected;
         deferredShouldReboot = false;
         deferredRadioAffected = false;
+        disable();
+        if (commitReboot)
+            disableBluetooth();
         saveChanges(SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS | SEGMENT_NODEDATABASE,
                     commitReboot, commitRadio);
         flushChannelWarnings(); // one coalesced message for everything edited in this transaction
@@ -1030,13 +1073,12 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
     case meshtastic_Config_network_tag: {
         LOG_INFO("Set config: WiFi");
         config.has_network = true;
+        auto incoming = c.payload_variant.network;
+        writeSecret(incoming.wifi_psk, sizeof(incoming.wifi_psk), config.network.wifi_psk);
         // No-op set must not reboot; any real change still restarts the network stack. memcmp fails safe.
-        if (memcmp(&config.network, &c.payload_variant.network, sizeof(config.network)) == 0)
+        if (memcmp(&config.network, &incoming, sizeof(config.network)) == 0)
             requiresReboot = false;
-        char prevPsk[sizeof(config.network.wifi_psk)];
-        memcpy(prevPsk, config.network.wifi_psk, sizeof(prevPsk));
-        config.network = c.payload_variant.network;
-        writeSecret(config.network.wifi_psk, sizeof(config.network.wifi_psk), prevPsk);
+        config.network = incoming;
         break;
     }
     case meshtastic_Config_display_tag:
@@ -1066,7 +1108,6 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
 
         LOG_INFO("Set config: LoRa");
         config.has_lora = true;
-        radioAffected = true; // the only sub-message that legitimately needs a live radio reconfigure
 
         if (validatedLora.coding_rate != clampCodingRate(validatedLora.coding_rate)) {
             LOG_WARN("Invalid coding_rate %d, setting to %d", validatedLora.coding_rate, LORA_CR_DEFAULT);
@@ -1213,6 +1254,7 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
         }
 #endif
 
+        radioAffected = loraRadioConfigChanged(oldLoraConfig, validatedLora);
         config.lora = validatedLora; // Finally, return the validated config back to the main config
         if (validatedLora.modem_preset != oldLoraConfig.modem_preset) {
             pendingOldLora = oldLoraConfig;
@@ -1232,6 +1274,7 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
         break;
     case meshtastic_Config_security_tag: {
         LOG_INFO("Set config: Security");
+        const auto oldSecurity = config.security;
         meshtastic_Config_SecurityConfig incoming = c.payload_variant.security;
         // Preserve our keypair when a SET omits the private key but we already hold one: regenerating would
         // change our NodeNum (== crc32(public_key)) and orphan us on the mesh. A SET without the key is a
@@ -1278,11 +1321,11 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
             LOG_WARN(warning);
             sendWarning(warning);
         }
+        requiresReboot = !protobufsEqual<meshtastic_Config_SecurityConfig_size>(&meshtastic_Config_SecurityConfig_msg,
+                                                                                &oldSecurity, &config.security);
 
         changes = SEGMENT_CONFIG | SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE;
 
-        requiresReboot = true;
-
         break;
     }
     case meshtastic_Config_device_ui_tag:
@@ -1304,12 +1347,6 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
 bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
 {
     bool shouldReboot = true;
-    // If we are in an open transaction or configuring MQTT or Serial (which have validation), defer disabling Bluetooth
-    // Otherwise, disable Bluetooth to prevent the phone from interfering with the config
-    if (!hasOpenEditTransaction && !IS_ONE_OF(c.which_payload_variant, meshtastic_ModuleConfig_mqtt_tag,
-                                              meshtastic_ModuleConfig_serial_tag, meshtastic_ModuleConfig_statusmessage_tag)) {
-        disableBluetooth();
-    }
 
     switch (c.which_payload_variant) {
     case meshtastic_ModuleConfig_mqtt_tag:
@@ -1321,14 +1358,13 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
         if (!MQTT::isValidConfig(c.payload_variant.mqtt)) {
             return false;
         }
-        // Disable Bluetooth to prevent interference during MQTT configuration
-        disableBluetooth();
         moduleConfig.has_mqtt = true;
         {
-            char prevPass[sizeof(moduleConfig.mqtt.password)];
-            memcpy(prevPass, moduleConfig.mqtt.password, sizeof(prevPass));
-            moduleConfig.mqtt = c.payload_variant.mqtt;
-            writeSecret(moduleConfig.mqtt.password, sizeof(moduleConfig.mqtt.password), prevPass);
+            auto incoming = c.payload_variant.mqtt;
+            writeSecret(incoming.password, sizeof(incoming.password), moduleConfig.mqtt.password);
+            shouldReboot = !protobufsEqual<meshtastic_ModuleConfig_MQTTConfig_size>(&meshtastic_ModuleConfig_MQTTConfig_msg,
+                                                                                    &moduleConfig.mqtt, &incoming);
+            moduleConfig.mqtt = incoming;
         }
 #endif
         break;
@@ -1340,9 +1376,10 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
             LOG_ERROR("Invalid serial config");
             return false;
         }
-        disableBluetooth(); // Disable Bluetooth to prevent interference during Serial configuration
 #endif
         moduleConfig.has_serial = true;
+        shouldReboot = !protobufsEqual<meshtastic_ModuleConfig_SerialConfig_size>(
+            &meshtastic_ModuleConfig_SerialConfig_msg, &moduleConfig.serial, &c.payload_variant.serial);
         moduleConfig.serial = c.payload_variant.serial;
         break;
     case meshtastic_ModuleConfig_external_notification_tag:
@@ -1362,6 +1399,9 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
         break;
     case meshtastic_ModuleConfig_telemetry_tag:
         LOG_INFO("Set module config: Telemetry");
+        shouldReboot = !moduleConfig.has_telemetry ||
+                       !protobufsEqual<meshtastic_ModuleConfig_TelemetryConfig_size>(
+                           &meshtastic_ModuleConfig_TelemetryConfig_msg, &moduleConfig.telemetry, &c.payload_variant.telemetry);
         moduleConfig.has_telemetry = true;
         moduleConfig.telemetry = c.payload_variant.telemetry;
         break;
@@ -1506,13 +1546,19 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
     }
 #endif
     }
+    if (shouldReboot && !hasOpenEditTransaction)
+        disableBluetooth();
     // No module config feeds RadioInterface::reconfigure() - that reads config.lora only.
     saveChanges(SEGMENT_MODULECONFIG, shouldReboot, /*radioAffected=*/false);
     return true;
 }
 
 void AdminModule::handleSetChannel(const meshtastic_Channel &cc)
 {
+    const ChannelIndex oldPrimaryIndex = channels.getPrimaryIndex();
+    char oldPrimaryName[32];
+    snprintf(oldPrimaryName, sizeof(oldPrimaryName), "%s", channels.getName(oldPrimaryIndex));
+
     channels.setChannel(cc);
     if (channels.ensureLicensedOperation()) {
         warnLicensedMode();
@@ -1537,8 +1583,13 @@ void AdminModule::handleSetChannel(const meshtastic_Channel &cc)
     }
     if (clamped)
         sendWarning(publicChannelPrecisionMessage);
-    saveChanges(SEGMENT_CHANNELS, false, /*radioAffected=*/true); // real frequency-slot / PSK change
-    warnOnChannelSet(channels.getByIndex(cc.index));              // passes the saved channel
+    const ChannelIndex newPrimaryIndex = channels.getPrimaryIndex();
+    const bool usesChannelNameForFrequency = config.lora.override_frequency == 0 && RadioInterface::uses_default_frequency_slot &&
+                                             getRegion(config.lora.region)->overrideSlot == OVERRIDE_SLOT_DEFAULT_CHANNEL_HASH;
+    const bool radioAffected = usesChannelNameForFrequency && (oldPrimaryIndex != newPrimaryIndex ||
+                                                               strcmp(oldPrimaryName, channels.getName(newPrimaryIndex)) != 0);
+    saveChanges(SEGMENT_CHANNELS, false, radioAffected);
+    warnOnChannelSet(channels.getByIndex(cc.index)); // passes the saved channel
     // Inside an edit transaction the queued warnings are flushed once at commit; otherwise emit now.
     if (!hasOpenEditTransaction)
         flushChannelWarnings();
@@ -1955,15 +2006,29 @@ void AdminModule::expireStaleEditTransaction()
     deferredEditSegments = 0;
     // Same consume-and-clear as a real commit: an abandoned transaction must not leave its
     // decisions behind for the next one.
-    const bool expiredRadio = deferredRadioAffected;
+    const bool expiredReboot = deferredShouldReboot, expiredRadio = deferredRadioAffected;
     deferredShouldReboot = false;
     deferredRadioAffected = false;
-    // No reboot: the settings are already live in RAM and the client that would expect one is gone.
+    disable();
+    // Boot-only changes must still take effect if the client disappears. Persisting without this
+    // reboot would leave runtime behavior out of sync with the stored configuration.
+    if (expiredReboot)
+        disableBluetooth();
     if (segments)
-        saveChanges(segments, false, expiredRadio);
+        saveChanges(segments, expiredReboot, expiredRadio);
     flushChannelWarnings();
 }
 
+int32_t AdminModule::runOnce()
+{
+    expireStaleEditTransaction();
+    if (!hasOpenEditTransaction)
+        return disable();
+
+    const uint32_t elapsed = millis() - editTransactionActivityMs;
+    return elapsed < EDIT_TRANSACTION_IDLE_MS ? EDIT_TRANSACTION_IDLE_MS - elapsed : 1;
+}
+
 // radioAffected is mandatory here - see the note on the declaration in AdminModule.h for why it
 // must not be allowed to default now that the deferred transaction flags accumulate it.
 void AdminModule::saveChanges(int saveWhat, bool shouldReboot, bool radioAffected)
@@ -1980,6 +2045,8 @@ void AdminModule::saveChanges(int saveWhat, bool shouldReboot, bool radioAffecte
         deferredRadioAffected |= radioAffected;
         LOG_INFO("Delay save of changes to disk until the open transaction is committed");
         editTransactionActivityMs = millis(); // still in use, so not the abandoned kind we time out
+        enabled = true;
+        setIntervalFromNow(EDIT_TRANSACTION_IDLE_MS);
         deferredEditSegments |= saveWhat;
         return;
     }
@@ -2060,8 +2127,10 @@ void AdminModule::handleSetHamMode(const meshtastic_HamParameters &p)
                 /*radioAffected=*/true);
 }
 
-AdminModule::AdminModule() : ProtobufModule("Admin", meshtastic_PortNum_ADMIN_APP, &meshtastic_AdminMessage_msg)
+AdminModule::AdminModule()
+    : ProtobufModule("Admin", meshtastic_PortNum_ADMIN_APP, &meshtastic_AdminMessage_msg), concurrency::OSThread("AdminTimeout")
 {
+    disable();
     // restrict to the admin channel for rx
     // boundChannel = Channels::adminChannel;
 }
@@ -2567,6 +2636,9 @@ void AdminModule::warnOnLoraPresetChange(const meshtastic_Config_LoRaConfig &old
 
 void disableBluetooth()
 {
+#ifdef PIO_UNIT_TESTING
+    disableBluetoothCallCountForTest++;
+#endif
 #if HAS_BLUETOOTH
 #ifdef ARCH_ESP32
     if (nimbleBluetooth)
diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h
@@ -3,6 +3,7 @@
 #include <esp_ota_ops.h>
 #endif
 #include "ProtobufModule.h"
+#include "concurrency/OSThread.h"
 #include "meshUtils.h"
 #include <sys/types.h>
 #if HAS_WIFI
@@ -21,7 +22,9 @@ struct AdminModule_ObserverData {
 /**
  * Admin module for admin messages
  */
-class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Observable<AdminModule_ObserverData *>
+class AdminModule : public ProtobufModule<meshtastic_AdminMessage>,
+                    public Observable<AdminModule_ObserverData *>,
+                    private concurrency::OSThread
 {
     friend class AdminModuleTestShim; // test/support/AdminModuleTestShim.h - native tests reach the private handlers/state
 
@@ -37,6 +40,7 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
     @return true if you've guaranteed you've handled this message and no other handlers should be considered for it
     */
     virtual bool handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshtastic_AdminMessage *p) override;
+    virtual int32_t runOnce() override;
 
   private:
     bool hasOpenEditTransaction = false;
@@ -49,6 +53,7 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
     void expireStaleEditTransaction();
 #ifdef PIO_UNIT_TESTING
     int lastSaveWhatForTest = 0;
+    unsigned long editTransactionTimerIntervalForTest() const { return interval; }
 #endif
 
     // While a transaction is open, saveChanges() defers the write - so the per-field reboot and
diff --git a/test/support/AdminModuleTestShim.h b/test/support/AdminModuleTestShim.h
@@ -29,14 +29,19 @@ class AdminModuleTestShim : public AdminModule
     {
         hasOpenEditTransaction = true;
         editTransactionActivityMs = millis();
+        enabled = true;
+        setIntervalFromNow(EDIT_TRANSACTION_IDLE_MS);
         deferredShouldReboot = false;
         deferredRadioAffected = false;
     }
     int savedSegments() const { return lastSaveWhatForTest; }
 
     bool editTransactionOpen() const { return hasOpenEditTransaction; }
+    bool editTransactionTimerEnabled() const { return enabled; }
+    unsigned long editTransactionTimerInterval() const { return editTransactionTimerIntervalForTest(); }
     // Backdate past the idle window so a test sees an abandoned transaction without waiting it out.
     void ageEditTransaction() { editTransactionActivityMs = millis() - EDIT_TRANSACTION_IDLE_MS - 1; }
+    int32_t runTransactionTimer() { return runOnce(); }
 
     // Setters may allocate an error reply from packetPool; drain it each iteration or the pool leaks.
     void drainReply()
diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp
