From 55110ffa94ecfe7402fe928c09a48ac94c246eab Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 8 Jul 2026 19:53:20 -0700 Subject: [PATCH] fix(mqtt): migrate observer settings from legacy /com_prefs on upgrade The NodePrefs->MQTTPrefs split (2eb41bae) left the promised one-time migration of the old /com_prefs trailing block unimplemented, so users upgrading an observer node silently lost SNMP, radio-watchdog, and fault-alert configuration (alerts reset to off; PSK/hashtag/region wiped). loadPrefsInt now detects an old-format /com_prefs by its size, skips the legacy zero-filled MQTT gap (6-slot or 3-slot era), and recovers the trailing observer block into a LegacyObserverTail (reusing the old firmware's byte291/292 heuristic and per-field availability guards). loadMQTTPrefs applies those values when the loaded /mqtt_prefs predates the appended observer fields, and both files are rewritten once in the current layout. rx_boosted_gain/flood_max_* are also recovered from the correct offsets (previously read from inside the old gap and reset). Verified with a host-side harness that round-trips the real old-firmware savePrefs (from 2eb41bae^) through the new load path across upgrade, fresh-install, upstream-format, 3-slot-era, truncated, and legacy-variant cases, plus a non-MQTT-build variant. Updates the stale migration comments and documents the CommonCLI_Observer seam in MQTT_IMPLEMENTATION.md. Co-Authored-By: Claude Fable 5 --- MQTT_IMPLEMENTATION.md | 45 ++++++++-- src/helpers/CommonCLI.cpp | 185 ++++++++++++++++++++++++++++++++++---- src/helpers/CommonCLI.h | 26 +++++- 3 files changed, 232 insertions(+), 24 deletions(-) diff --git a/MQTT_IMPLEMENTATION.md b/MQTT_IMPLEMENTATION.md index 4ec105f4..0134ba04 100644 --- a/MQTT_IMPLEMENTATION.md +++ b/MQTT_IMPLEMENTATION.md @@ -140,15 +140,50 @@ The MQTT bridge uses a slot-based architecture with up to 6 concurrent connectio - `src/helpers/bridges/MQTTBridge.h` - MQTT bridge class definition - `src/helpers/bridges/MQTTBridge.cpp` - MQTT bridge implementation - `src/helpers/MQTTPresets.h` - Preset definitions, CA certificates, and lookup functions +- `src/helpers/MQTTDefaults.h` - Compile-time defaults for fresh `/mqtt_prefs` - `src/helpers/MQTTMessageBuilder.h` - JSON message formatting utilities - `src/helpers/MQTTMessageBuilder.cpp` - JSON message formatting implementation - `src/helpers/JWTHelper.h` - JWT token generation for Ed25519-based authentication +- `src/helpers/CommonCLI_Observer.cpp` - All observer CLI command handling (MQTT, WiFi, + timezone, NTP, OTA, SNMP, alerts) -#### Integration -- Updated `examples/simple_repeater/MyMesh.h` - Added MQTT bridge support -- Updated `examples/simple_repeater/MyMesh.cpp` - Added MQTT bridge integration and raw radio data capture -- Updated `src/helpers/CommonCLI.h` - MQTT slot preferences, WiFi, and timezone fields -- Updated `src/helpers/CommonCLI.cpp` - MQTT slot CLI commands, migration logic +#### Integration seams with upstream code + +The observer feature is kept out of upstream-tracked files through three mechanisms: + +- **CLI hook methods** — upstream `CommonCLI.cpp` delegates to three `CommonCLI` + methods defined in the fork-owned `CommonCLI_Observer.cpp`: `handleObserverCommand()`, + `handleObserverSetCmd()`, and `handleObserverGetCmd()`. Each returns `true` if it + consumed the command, otherwise the upstream parser runs. Only these three call + sites touch upstream CLI code. +- **Callback virtuals** — observer behaviour needed from the application is exposed + as default-no-op virtuals on `CommonCLICallbacks` (e.g. `restartBridgeSlot`, + `isMqttBridgeRunning`, `syncMqttNtp`, `onAlertConfigChanged`, `sendAlertText`, + `resolveAlertScope`, `beginDeferredOtaUpdate`). The example apps override them + behind `#ifdef WITH_MQTT_BRIDGE`. +- **Separate settings file** — observer settings (MQTT slots, WiFi, timezone, SNMP, + radio watchdog, fault alerts) live in the `MQTTPrefs` struct persisted to + `/mqtt_prefs`, keeping `NodePrefs` / `/com_prefs` aligned with the upstream layout. + +Remaining integration points in upstream files: +- `examples/simple_repeater/MyMesh.{h,cpp}`, `examples/simple_room_server/MyMesh.{h,cpp}` - + bridge/alerter/SNMP wiring and packet-feed hooks, guarded by `#ifdef WITH_MQTT_BRIDGE` +- `src/helpers/CommonCLI.{h,cpp}` - the three CLI hooks, `MQTTPrefs` load/save/migration +- `src/Dispatcher.{h,cpp}` - radio watchdog block, guarded by `#ifdef WITH_MQTT_BRIDGE` + +#### Settings upgrade / migration + +`loadPrefs()` handles all historical on-device formats one-time at boot: +- `/mqtt_prefs` written in the pre-slot or 3-slot layouts is field-copied into the + current 6-slot `MQTTPrefs` layout (size-based detection). +- A `/com_prefs` written by fork firmware that predates the `MQTTPrefs` split (which + contained a zero-filled MQTT gap plus a trailing observer block) is detected by its + size; the trailing SNMP / radio-watchdog / fault-alert settings and the + `rx_boosted_gain` / `flood_max_*` fields are recovered, carried into `/mqtt_prefs`, + and both files are rewritten in the current formats. +- Settings the pre-split firmware stored *inside* the `/com_prefs` MQTT gap (the MQTT + slot/WiFi config itself) are **not** recovered — users upgrading from firmware that + old must re-enter their MQTT and WiFi configuration. ## Build Configuration diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index c243c3f7..5f823707 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -39,6 +39,16 @@ static bool isValidName(const char *n) { return true; } +// Old fork firmware persisted the (since removed) NodePrefs MQTT fields to /com_prefs +// as a zero-filled gap between owner_info (which ends at offset 290) and a trailing +// observer block (rx_boosted_gain, flood_max_*, snmp/watchdog/alert settings). +// The gap size depended on MAX_MQTT_SLOTS at the time: 306 bytes of non-slot fields +// plus 186 bytes per slot (preset 24 + host 64 + port 2 + username 32 + password 64). +// loadPrefsInt() uses the file size to tell the eras apart and recover the tail. +static const size_t LEGACY_MQTT_GAP_6SLOT = 306 + 6 * 186; // 1422 +static const size_t LEGACY_MQTT_GAP_3SLOT = 306 + 3 * 186; // 864 +static const size_t LEGACY_OBS_TAIL_MAX = 124; // rx_boosted(1) + flood(2) + snmp(25) + watchdog(1) + alert block(95) + void CommonCLI::loadPrefs(FILESYSTEM* fs) { bool is_fresh_install = false; @@ -73,6 +83,15 @@ void CommonCLI::loadPrefs(FILESYSTEM* fs) { // the shorter /mqtt_prefs file won't contain it, so it keeps the default value (1 = on) // set by setMQTTPrefsDefaults(). No explicit migration needed. #endif + + if (_com_prefs_needs_upgrade) { + // Old-format /com_prefs (legacy MQTT gap + trailing observer block) was detected: + // rewrite the prefs files in the current layout, one time. This persists the + // recovered rx_boosted_gain/flood_max_* values and (on MQTT builds) the observer + // settings that loadMQTTPrefs carried over into /mqtt_prefs. + savePrefs(fs); + _com_prefs_needs_upgrade = false; + } } void CommonCLI::loadPrefsInt(FILESYSTEM* fs, const char* filename) { @@ -127,18 +146,116 @@ void CommonCLI::loadPrefsInt(FILESYSTEM* fs, const char* filename) { file.read((uint8_t *)&_prefs->adc_multiplier, sizeof(_prefs->adc_multiplier)); // 166 file.read((uint8_t *)_prefs->owner_info, sizeof(_prefs->owner_info)); // 170 // MQTT/observer settings are no longer stored in /com_prefs — they live in - // /mqtt_prefs (loaded by loadMQTTPrefs). The legacy zero-gap and trailing - // snmp/alert block that older firmware wrote here are gone; on upgrade those - // bytes are simply not read, so rx_boosted_gain/flood_max_* (which followed the - // gap) reset once and self-heal on the next save. - if (file.available() >= (int)sizeof(_prefs->rx_boosted_gain)) { - file.read((uint8_t *)&_prefs->rx_boosted_gain, sizeof(_prefs->rx_boosted_gain)); - } - if (file.available() >= (int)sizeof(_prefs->flood_max_unscoped)) { - file.read((uint8_t *)&_prefs->flood_max_unscoped, sizeof(_prefs->flood_max_unscoped)); - } - if (file.available() >= (int)sizeof(_prefs->flood_max_advert)) { - file.read((uint8_t *)&_prefs->flood_max_advert, sizeof(_prefs->flood_max_advert)); + // /mqtt_prefs (loaded by loadMQTTPrefs). Old fork firmware wrote a zero-filled + // MQTT gap here followed by a trailing observer block; detect that layout by the + // extra length, skip the gap, and recover the tail so those settings survive + // the upgrade (the file is rewritten in the new layout by loadPrefs afterwards). + size_t extra = file.available(); + if (extra > 3) { + _com_prefs_needs_upgrade = true; + size_t gap = 0; + if (extra > LEGACY_MQTT_GAP_6SLOT && extra <= LEGACY_MQTT_GAP_6SLOT + LEGACY_OBS_TAIL_MAX) { + gap = LEGACY_MQTT_GAP_6SLOT; + } else if (extra > LEGACY_MQTT_GAP_3SLOT && extra <= LEGACY_MQTT_GAP_3SLOT + LEGACY_OBS_TAIL_MAX) { + gap = LEGACY_MQTT_GAP_3SLOT; + } + // Unrecognized legacy sizes (e.g. pre-slot-era files) leave gap == 0: the tail + // is not read and everything past owner_info degrades to defaults. + if (gap > 0) { + uint8_t skip_buf[64]; + size_t remaining = gap; + while (remaining > 0) { + size_t n = remaining > sizeof(skip_buf) ? sizeof(skip_buf) : remaining; + file.read(skip_buf, n); + remaining -= n; + } + file.read((uint8_t *)&_prefs->rx_boosted_gain, sizeof(_prefs->rx_boosted_gain)); + // Tail layout: flood_max_unscoped, flood_max_advert, then the snmp fields — + // except legacy flex-branch files where snmp starts right after + // rx_boosted_gain (no flood_max_*). Same heuristic the old firmware used: + // snmp_enabled is 0/1 and the first community char is printable (> 64). + uint8_t b1 = 0, b2 = 0; + bool have_flood_bytes = file.available() >= 2; + if (have_flood_bytes) { + file.read(&b1, 1); + file.read(&b2, 1); + } +#ifdef WITH_MQTT_BRIDGE + // Pre-fill with the same defaults applyMQTTDefaults() uses, so fields a + // shorter (older) tail doesn't contain degrade to defaults when applied. + memset(&_legacy_tail, 0, sizeof(_legacy_tail)); + strncpy(_legacy_tail.snmp_community, "public", sizeof(_legacy_tail.snmp_community) - 1); + _legacy_tail.radio_watchdog_minutes = 5; + _legacy_tail.alert_wifi_minutes = 30; + _legacy_tail.alert_mqtt_minutes = 240; + _legacy_tail.alert_min_interval_min = 60; +#endif + if (have_flood_bytes && b1 <= 1 && b2 > 64) { + // Legacy variant: no flood_max_* — b1/b2 are snmp_enabled + community[0] +#ifdef WITH_MQTT_BRIDGE + _legacy_tail.snmp_enabled = b1; + _legacy_tail.snmp_community[0] = (char) b2; + if (file.available() >= (int)(sizeof(_legacy_tail.snmp_community) - 1)) { + file.read((uint8_t *)&_legacy_tail.snmp_community[1], sizeof(_legacy_tail.snmp_community) - 1); + } +#endif + } else if (have_flood_bytes) { + _prefs->flood_max_unscoped = b1; + _prefs->flood_max_advert = b2; +#ifdef WITH_MQTT_BRIDGE + if (file.available() >= (int)sizeof(_legacy_tail.snmp_enabled)) { + file.read((uint8_t *)&_legacy_tail.snmp_enabled, sizeof(_legacy_tail.snmp_enabled)); + } + if (file.available() >= (int)sizeof(_legacy_tail.snmp_community)) { + file.read((uint8_t *)&_legacy_tail.snmp_community, sizeof(_legacy_tail.snmp_community)); + } +#endif + } +#ifdef WITH_MQTT_BRIDGE + if (file.available() >= (int)sizeof(_legacy_tail.radio_watchdog_minutes)) { + file.read((uint8_t *)&_legacy_tail.radio_watchdog_minutes, sizeof(_legacy_tail.radio_watchdog_minutes)); + } + if (file.available() >= (int)sizeof(_legacy_tail.alert_enabled)) { + file.read((uint8_t *)&_legacy_tail.alert_enabled, sizeof(_legacy_tail.alert_enabled)); + } + if (file.available() >= (int)sizeof(_legacy_tail.alert_psk_hex)) { + file.read((uint8_t *)&_legacy_tail.alert_psk_hex, sizeof(_legacy_tail.alert_psk_hex)); + } + if (file.available() >= (int)sizeof(_legacy_tail.alert_wifi_minutes)) { + file.read((uint8_t *)&_legacy_tail.alert_wifi_minutes, sizeof(_legacy_tail.alert_wifi_minutes)); + } + if (file.available() >= (int)sizeof(_legacy_tail.alert_mqtt_minutes)) { + file.read((uint8_t *)&_legacy_tail.alert_mqtt_minutes, sizeof(_legacy_tail.alert_mqtt_minutes)); + } + if (file.available() >= (int)sizeof(_legacy_tail.alert_min_interval_min)) { + file.read((uint8_t *)&_legacy_tail.alert_min_interval_min, sizeof(_legacy_tail.alert_min_interval_min)); + } + if (file.available() >= (int)sizeof(_legacy_tail.alert_hashtag)) { + file.read((uint8_t *)&_legacy_tail.alert_hashtag, sizeof(_legacy_tail.alert_hashtag)); + } + if (file.available() >= (int)sizeof(_legacy_tail.alert_region)) { + file.read((uint8_t *)&_legacy_tail.alert_region, sizeof(_legacy_tail.alert_region)); + } + _legacy_tail.snmp_enabled = constrain(_legacy_tail.snmp_enabled, 0, 1); + _legacy_tail.radio_watchdog_minutes = constrain(_legacy_tail.radio_watchdog_minutes, 0, 120); + _legacy_tail.alert_enabled = constrain(_legacy_tail.alert_enabled, 0, 1); + _legacy_tail.snmp_community[sizeof(_legacy_tail.snmp_community) - 1] = '\0'; + _legacy_tail.alert_psk_hex[sizeof(_legacy_tail.alert_psk_hex) - 1] = '\0'; + _legacy_tail.alert_hashtag[sizeof(_legacy_tail.alert_hashtag) - 1] = '\0'; + _legacy_tail.alert_region[sizeof(_legacy_tail.alert_region) - 1] = '\0'; + _legacy_tail.valid = true; +#endif + } + } else { + if (file.available() >= (int)sizeof(_prefs->rx_boosted_gain)) { + file.read((uint8_t *)&_prefs->rx_boosted_gain, sizeof(_prefs->rx_boosted_gain)); + } + if (file.available() >= (int)sizeof(_prefs->flood_max_unscoped)) { + file.read((uint8_t *)&_prefs->flood_max_unscoped, sizeof(_prefs->flood_max_unscoped)); + } + if (file.available() >= (int)sizeof(_prefs->flood_max_advert)) { + file.read((uint8_t *)&_prefs->flood_max_advert, sizeof(_prefs->flood_max_advert)); + } } // sanitise bad pref values @@ -254,6 +371,11 @@ void CommonCLI::loadMQTTPrefs(FILESYSTEM* fs) { // Initialize with defaults first setMQTTPrefsDefaults(&_mqtt_prefs); + // 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 + // old-format /com_prefs trailing block below. + bool has_observer_fields = false; + bool file_existed = fs->exists("/mqtt_prefs"); if (file_existed) { // Load from separate MQTT prefs file @@ -369,12 +491,20 @@ void CommonCLI::loadMQTTPrefs(FILESYSTEM* fs) { saveMQTTPrefs(fs); } } else if (file_size > 0) { - // 6-slot format: read directly - size_t bytes_to_read = file_size < sizeof(_mqtt_prefs) ? file_size : sizeof(_mqtt_prefs); + // 6-slot format: read directly. The observer fields (snmp/watchdog/alert) + // count as present only when the file holds the full struct; shorter (older) + // files are read only up to the observer boundary, so trailing struct + // padding in an old-firmware file can't bleed into the new fields. + bool full_struct = file_size >= sizeof(_mqtt_prefs); + size_t obs_boundary = offsetof(MQTTPrefs, snmp_enabled); + size_t bytes_to_read = full_struct ? sizeof(_mqtt_prefs) + : (file_size < obs_boundary ? file_size : obs_boundary); size_t bytes_read = file.read((uint8_t *)&_mqtt_prefs, bytes_to_read); file.close(); if (bytes_read != bytes_to_read) { setMQTTPrefsDefaults(&_mqtt_prefs); + } else if (full_struct) { + has_observer_fields = true; } } else { file.close(); @@ -382,10 +512,31 @@ void CommonCLI::loadMQTTPrefs(FILESYSTEM* fs) { } } } else { - // No /mqtt_prefs file — defaults already set - // (Legacy /com_prefs migration removed: the old offset-based approach was fragile - // and the pre-MQTT firmware never wrote MQTT fields to /com_prefs anyway.) + // No /mqtt_prefs file — defaults already set. (MQTT slot/WiFi settings from + // pre-/mqtt_prefs-split fork firmware are NOT recovered from /com_prefs — that + // offset-based migration was fragile and was removed; those users re-enter + // their MQTT config. The observer trailing block IS recovered, below.) } + + // One-time upgrade path: if loadPrefsInt captured the trailing observer block of + // an old-format /com_prefs and this /mqtt_prefs predates the appended observer + // fields (or doesn't exist), carry the settings over so SNMP, radio-watchdog and + // fault-alert config survive the firmware upgrade. loadPrefs() persists both + // files in the new layout right after this. + if (_legacy_tail.valid && !has_observer_fields) { + _mqtt_prefs.snmp_enabled = _legacy_tail.snmp_enabled; + memcpy(_mqtt_prefs.snmp_community, _legacy_tail.snmp_community, sizeof(_mqtt_prefs.snmp_community)); + _mqtt_prefs.radio_watchdog_minutes = _legacy_tail.radio_watchdog_minutes; + _mqtt_prefs.alert_enabled = _legacy_tail.alert_enabled; + memcpy(_mqtt_prefs.alert_psk_hex, _legacy_tail.alert_psk_hex, sizeof(_mqtt_prefs.alert_psk_hex)); + _mqtt_prefs.alert_wifi_minutes = _legacy_tail.alert_wifi_minutes; + _mqtt_prefs.alert_mqtt_minutes = _legacy_tail.alert_mqtt_minutes; + _mqtt_prefs.alert_min_interval_min = _legacy_tail.alert_min_interval_min; + memcpy(_mqtt_prefs.alert_hashtag, _legacy_tail.alert_hashtag, sizeof(_mqtt_prefs.alert_hashtag)); + memcpy(_mqtt_prefs.alert_region, _legacy_tail.alert_region, sizeof(_mqtt_prefs.alert_region)); + MESH_DEBUG_PRINTLN("MQTT: Migrated observer settings from legacy /com_prefs trailing block"); + } + _legacy_tail.valid = false; } void CommonCLI::saveMQTTPrefs(FILESYSTEM* fs) { diff --git a/src/helpers/CommonCLI.h b/src/helpers/CommonCLI.h index fb3e1a78..944df3a1 100644 --- a/src/helpers/CommonCLI.h +++ b/src/helpers/CommonCLI.h @@ -149,8 +149,9 @@ struct MQTTPrefs { char mqtt_ntp_server[64]; // Custom NTP server; empty = pool.ntp.org // --- Observer non-MQTT settings, moved out of NodePrefs (Phase 2). Appended at - // the end so existing /mqtt_prefs files (which lack them) still load; values are - // migrated one-time from the old /com_prefs trailing block on first boot. --- + // the end so existing /mqtt_prefs files (which lack them) still load. On first + // boot after upgrade, loadPrefsInt() captures the old /com_prefs trailing block + // into LegacyObserverTail and loadMQTTPrefs() copies the values here. --- uint8_t snmp_enabled; // boolean char snmp_community[24]; // community string (default "public") uint8_t radio_watchdog_minutes; // 0=disabled, 1-120 minutes (observer-only radio recovery) @@ -195,6 +196,25 @@ struct ThreeSlotMQTTPrefs { char mqtt_slot_token[3][48]; char mqtt_slot_topic[3][96]; }; + +// 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 +// the values one-time if the loaded /mqtt_prefs predates the appended observer +// fields, so SNMP/watchdog/alert config survives the firmware upgrade. +struct LegacyObserverTail { + bool valid = false; + uint8_t snmp_enabled; + char snmp_community[24]; + uint8_t radio_watchdog_minutes; + uint8_t alert_enabled; + char alert_psk_hex[33]; + uint16_t alert_wifi_minutes; + uint16_t alert_mqtt_minutes; + uint16_t alert_min_interval_min; + char alert_hashtag[24]; + char alert_region[31]; +}; #endif class CommonCLICallbacks { @@ -307,7 +327,9 @@ class CommonCLI { char tmp[PRV_KEY_SIZE*2 + 4]; #ifdef WITH_MQTT_BRIDGE MQTTPrefs _mqtt_prefs; + LegacyObserverTail _legacy_tail; #endif + bool _com_prefs_needs_upgrade = false; // old-format /com_prefs detected; rewrite once after load mesh::RTCClock* getRTCClock() { return _rtc; } void savePrefs();