From c3b8633aa7f1d36ac0bb2ab1c30e64301b1c13a8 Mon Sep 17 00:00:00 2001 From: agessaman Date: Thu, 9 Jul 2026 12:16:20 -0700 Subject: [PATCH] fix(mqtt): hold newer-version /mqtt_prefs across saves + pin frozen layouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unknown-version path kept defaults at boot but any later savePrefs() (every CLI set command) rewrote /mqtt_prefs as v1 with defaults, destroying the newer config after a firmware downgrade. Latch _mqtt_prefs_hold when an unsupported version is seen and refuse to write while it is set — checked before the NRF52/STM32 open path, which deletes the file first. Also pin the frozen legacy /mqtt_prefs layouts (472/1464/2904 bytes + 8-byte header) with static_asserts so every target build re-verifies the deployed fleet's file offsets, and null-check _obs in AlertReporter::onLoop. --- MQTT_INTERNALS.md | 6 +++++- src/helpers/AlertReporter.cpp | 2 +- src/helpers/CommonCLI.cpp | 14 ++++++++++++-- src/helpers/CommonCLI.h | 15 +++++++++++++++ 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/MQTT_INTERNALS.md b/MQTT_INTERNALS.md index 1adb9713..ff65c8b6 100644 --- a/MQTT_INTERNALS.md +++ b/MQTT_INTERNALS.md @@ -47,7 +47,11 @@ Remaining integration points in upstream files: bytes of a legacy (headerless) file, whose payload begins with the `mqtt_origin` string. Bump `MQTT_PREFS_VERSION` when the payload layout changes incompatibly; a file whose version this firmware doesn't recognize is left untouched and the in-memory prefs -fall back to defaults (no downgrade, no misread). +fall back to defaults (no downgrade, no misread). `saveMQTTPrefs()` also refuses to +write while such a file is present (`_mqtt_prefs_hold`), so a `set` command after a +firmware downgrade can't clobber the newer config — observer settings changed in that +state simply don't persist. The frozen legacy layouts are pinned with `static_assert`s +in `CommonCLI.h`, so every target build re-verifies the fleet's file offsets. Adding a field to the current version stays backward compatible: append it to the end of `MQTTPrefs`. An older, shorter payload still loads and the missing tail keeps its diff --git a/src/helpers/AlertReporter.cpp b/src/helpers/AlertReporter.cpp index 793a8d79..a6841277 100644 --- a/src/helpers/AlertReporter.cpp +++ b/src/helpers/AlertReporter.cpp @@ -206,7 +206,7 @@ void AlertReporter::formatAge(unsigned long age_ms, char* out, size_t out_size) } void AlertReporter::onLoop(unsigned long now_ms) { - if (!_prefs || !_obs->alert_enabled) return; + if (!_prefs || !_obs || !_obs->alert_enabled) return; if (!_mesh) return; // Throttle: ~5 s cadence. The thresholds are minutes-scale so this is fine. diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index e11c17b5..f65983b3 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -378,6 +378,7 @@ static File openMqttPrefsRead(FILESYSTEM* fs) { void CommonCLI::loadMQTTPrefs(FILESYSTEM* fs) { // Initialize with defaults first setMQTTPrefsDefaults(&_mqtt_prefs); + _mqtt_prefs_hold = false; // Whether the loaded /mqtt_prefs already contained the observer fields (snmp/ // watchdog/alert) appended in Phase 2 — if not, they may be carried over from an @@ -411,8 +412,10 @@ void CommonCLI::loadMQTTPrefs(FILESYSTEM* fs) { } } else { // Unknown (newer) version: don't risk misreading a layout we don't know. - // Keep defaults for this boot and leave the file untouched (no downgrade). - MESH_DEBUG_PRINTLN("MQTT: /mqtt_prefs version unsupported, using defaults"); + // Keep defaults for this boot and hold the file so later savePrefs() + // calls can't overwrite the newer config (no downgrade). + _mqtt_prefs_hold = true; + MESH_DEBUG_PRINTLN("MQTT: /mqtt_prefs version unsupported, using defaults (file preserved)"); } } } @@ -611,6 +614,13 @@ void CommonCLI::loadMQTTPrefs(FILESYSTEM* fs) { } void CommonCLI::saveMQTTPrefs(FILESYSTEM* fs) { + if (_mqtt_prefs_hold) { + // /mqtt_prefs was written by newer firmware; overwriting it here (v1 header + + // this boot's defaults) would destroy that config. Observer settings changed + // this boot are not persisted until current-or-older firmware is flashed. + MESH_DEBUG_PRINTLN("MQTT: /mqtt_prefs from newer firmware, not overwriting"); + return; + } #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) fs->remove("/mqtt_prefs"); File file = fs->open("/mqtt_prefs", FILE_O_WRITE); diff --git a/src/helpers/CommonCLI.h b/src/helpers/CommonCLI.h index b5240571..48be39b0 100644 --- a/src/helpers/CommonCLI.h +++ b/src/helpers/CommonCLI.h @@ -236,6 +236,17 @@ struct Legacy6SlotMQTTPrefs { char mqtt_ntp_server[64]; }; +// The legacy layouts above describe files already written to the deployed fleet's +// flash, so their sizes are frozen forever — loadMQTTPrefs() tells the eras apart +// by file size and reads each file as a raw struct dump. These asserts pin the +// layouts on every target toolchain; if one fires, the compiler (or an edit to a +// legacy struct or MAX_MQTT_SLOTS) has changed a layout and fleet files would be +// read at wrong offsets. +static_assert(sizeof(MQTTPrefsHeader) == 8, "versioned /mqtt_prefs header must stay 8 bytes"); +static_assert(sizeof(OldMQTTPrefs) == 472, "frozen pre-slot /mqtt_prefs layout changed"); +static_assert(sizeof(ThreeSlotMQTTPrefs) == 1464, "frozen 3-slot /mqtt_prefs layout changed"); +static_assert(sizeof(Legacy6SlotMQTTPrefs) == 2904, "frozen deployed-fleet /mqtt_prefs layout changed"); + // Observer settings captured from the trailing block of an old-format /com_prefs // (fork firmware that predates the NodePrefs -> MQTTPrefs split). loadPrefsInt() // fills this in when it detects the old file layout; loadMQTTPrefs() then applies @@ -367,6 +378,10 @@ class CommonCLI { #ifdef WITH_MQTT_BRIDGE MQTTPrefs _mqtt_prefs; LegacyObserverTail _legacy_tail; + // /mqtt_prefs carries a version newer than this firmware understands (a downgrade). + // The in-memory prefs run on defaults and saveMQTTPrefs() must not overwrite the + // file, or the first `set` command would destroy the newer config. + bool _mqtt_prefs_hold = false; #endif bool _com_prefs_needs_upgrade = false; // old-format /com_prefs detected; rewrite once after load