From 51c06b78c1f7fbfe1b579def9b7e80eae276d3a3 Mon Sep 17 00:00:00 2001 From: Jakub <106778416+MarekZegare4@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:02:05 +0200 Subject: [PATCH] fix(companion): prefs schema corruption, broken multi-scope, stale relay filter, cursor row Follow-up review of f589b9b2 -- five defects in that commit's own changes. - repeat_scope_only + repeat_extra_scopes were read/written in the MIDDLE of the prefs stream, beside their repeat_* siblings. loadPrefsInt()'s rd() is a plain sequential reader gated only on file.available(), with no per-field versioning, so on any pre-existing file those 25 bytes were taken from the fields that follow, shifting EVERY later field: repeater profile (incl. a float freq), track_shared_loc, all of loc_share_*, trail, bot, GPIO modes. Moved to the struct/file tail, sentinel bumped to 0xC0DE0027 with 0xC0DE0026 marked burned. sizeof stays 2752 (confirmed by build); the tripwire procedure now spells out the append-only rule that "in struct order" left implicit. - rebuildRepeatScopes() called getAutoKeyFor() with id 0 for every entry, but that cache is keyed on the id alone and ignores the name on a hit -- so every extra scope after the first silently got the first one's key, making the comma-separated list do nothing. Distinct id per scope now. - interference_threshold had no load clamp, so an upgrader read 0x23 (35) out of the old file's sentinel tail instead of 0. - CMD_SET_DEFAULT_FLOOD_SCOPE wrote default_scope_key without rebuilding the relay filter, so setting or clearing the scope from the app left the repeater filtering on the previous key until reboot. The on-device path already did. - The keyboard preview derived the cursor's row a second time from byte offsets, disagreeing with the cursor_line the scroll window already computes: it pinned the cursor to the end of a full line (drawing '_' one character past the display width) at every wrap boundary. Use cursor_line directly. Co-Authored-By: Claude Opus 5 --- examples/companion_radio/DataStore.cpp | 38 +++++++------ examples/companion_radio/MyMesh.cpp | 8 ++- examples/companion_radio/NodePrefs.h | 53 ++++++++++++++----- .../companion_radio/ui-new/KeyboardWidget.h | 18 +++---- 4 files changed, 77 insertions(+), 40 deletions(-) diff --git a/examples/companion_radio/DataStore.cpp b/examples/companion_radio/DataStore.cpp index a5f37aee..2698240f 100644 --- a/examples/companion_radio/DataStore.cpp +++ b/examples/companion_radio/DataStore.cpp @@ -383,18 +383,8 @@ void DataStore::loadPrefsInt(const char *filename, NodePrefs& _prefs, double& no (_prefs.repeat_min_snr < -20 || _prefs.repeat_min_snr > 10)) _prefs.repeat_min_snr = NodePrefs::REPEAT_SNR_DISABLED; // match the UI's -20..10 range if (_prefs.repeat_suppress_dup > 1) _prefs.repeat_suppress_dup = 0; - - // → 0xC0DE0025: append repeat_scope_only right after the other repeater - // forwarding filters. A pre-0x25 file has no byte here; clamp to 0 (off, - // unchanged forwarding behaviour for upgraders). - rd(&_prefs.repeat_scope_only, sizeof(_prefs.repeat_scope_only)); - if (_prefs.repeat_scope_only > 1) _prefs.repeat_scope_only = 0; - - // → 0xC0DE0026: append repeat_extra_scopes right after it. A pre-0x26 file - // has no bytes here; rd() zero-inits, which is already an empty string. - rd(_prefs.repeat_extra_scopes, sizeof(_prefs.repeat_extra_scopes)); - _prefs.repeat_extra_scopes[sizeof(_prefs.repeat_extra_scopes) - 1] = '\0'; - + // NOTE: repeat_scope_only/repeat_extra_scopes are read at the TAIL, not here + // beside their siblings — see the append-only rule in NodePrefs.h. rd(&_prefs.repeater_use_profile, sizeof(_prefs.repeater_use_profile)); rd(&_prefs.repeater_freq, sizeof(_prefs.repeater_freq)); rd(&_prefs.repeater_bw, sizeof(_prefs.repeater_bw)); @@ -582,12 +572,27 @@ void DataStore::loadPrefsInt(const char *filename, NodePrefs& _prefs, double& no if (_prefs.keyboard_cardkb_compact > 1) _prefs.keyboard_cardkb_compact = 0; // → 0xC0DE0024: append interference_threshold + cad_enabled at the tail. - // A pre-0x24 file has no bytes here; clamp to 0 (both off, matching the - // getters' previous hardcoded behaviour for upgraders). + // A pre-0x24 file has no bytes here, so these read that file's own 4-byte + // sentinel tail (0xC0DE0023 → 23 00 DE C0) rather than zeroes; clamp both to + // 0/off, matching the getters' previous hardcoded behaviour for upgraders. + // interference_threshold is dB above the measured noise floor (see + // RadioLibWrapper::isChannelActive()); anything past ~30 dB would never + // trigger anyway, so treat it as a stray byte and fall back to off. rd(&_prefs.interference_threshold, sizeof(_prefs.interference_threshold)); + if (_prefs.interference_threshold > 30) _prefs.interference_threshold = 0; rd(&_prefs.cad_enabled, sizeof(_prefs.cad_enabled)); if (_prefs.cad_enabled > 1) _prefs.cad_enabled = 0; + // → 0xC0DE0027: append repeat_scope_only + repeat_extra_scopes at the tail. + // (0xC0DE0026 had them mid-stream, which shifted every later field by 25 + // bytes when reading an older file — see NodePrefs.h.) A pre-0x27 file has + // no bytes here; clamp the flag to 0 (off, unchanged forwarding behaviour) + // and leave the name list zero-initialised, which is already an empty string. + rd(&_prefs.repeat_scope_only, sizeof(_prefs.repeat_scope_only)); + if (_prefs.repeat_scope_only > 1) _prefs.repeat_scope_only = 0; + rd(_prefs.repeat_extra_scopes, sizeof(_prefs.repeat_extra_scopes)); + _prefs.repeat_extra_scopes[sizeof(_prefs.repeat_extra_scopes) - 1] = '\0'; + // Schema sentinel: bumped on layout changes. Mismatch means an older file // (or a different schema); rd() already zero-inits any fields not present, // so we just log it — next savePrefs writes the current sentinel. @@ -748,8 +753,7 @@ void DataStore::savePrefs(const NodePrefs& _prefs, double node_lat, double node_ file.write((uint8_t *)&_prefs.repeat_delay_boost, sizeof(_prefs.repeat_delay_boost)); file.write((uint8_t *)&_prefs.repeat_min_snr, sizeof(_prefs.repeat_min_snr)); file.write((uint8_t *)&_prefs.repeat_suppress_dup, sizeof(_prefs.repeat_suppress_dup)); - file.write((uint8_t *)&_prefs.repeat_scope_only, sizeof(_prefs.repeat_scope_only)); - file.write((uint8_t *)_prefs.repeat_extra_scopes, sizeof(_prefs.repeat_extra_scopes)); + // repeat_scope_only/repeat_extra_scopes are written at the TAIL — see loadPrefsInt(). file.write((uint8_t *)&_prefs.repeater_use_profile, sizeof(_prefs.repeater_use_profile)); file.write((uint8_t *)&_prefs.repeater_freq, sizeof(_prefs.repeater_freq)); file.write((uint8_t *)&_prefs.repeater_bw, sizeof(_prefs.repeater_bw)); @@ -804,6 +808,8 @@ void DataStore::savePrefs(const NodePrefs& _prefs, double node_lat, double node_ file.write((uint8_t *)&_prefs.keyboard_cardkb_compact, sizeof(_prefs.keyboard_cardkb_compact)); file.write((uint8_t *)&_prefs.interference_threshold, sizeof(_prefs.interference_threshold)); file.write((uint8_t *)&_prefs.cad_enabled, sizeof(_prefs.cad_enabled)); + file.write((uint8_t *)&_prefs.repeat_scope_only, sizeof(_prefs.repeat_scope_only)); + file.write((uint8_t *)_prefs.repeat_extra_scopes, sizeof(_prefs.repeat_extra_scopes)); // Tail sentinel — must be last. See NodePrefs::SCHEMA_SENTINEL. Its write is // the one we check: once the flash fills, writes return 0, so a good diff --git a/examples/companion_radio/MyMesh.cpp b/examples/companion_radio/MyMesh.cpp index 9f3d4776..5d6033a7 100644 --- a/examples/companion_radio/MyMesh.cpp +++ b/examples/companion_radio/MyMesh.cpp @@ -691,7 +691,11 @@ void MyMesh::rebuildRepeatScopes() { if (*tok != '\0') { char hashtag[1 + sizeof(_prefs.repeat_extra_scopes)]; snprintf(hashtag, sizeof(hashtag), "#%s", tok); - temp.getAutoKeyFor(0, hashtag, repeat_scopes[repeat_scope_count++]); + // Distinct id per scope: getAutoKeyFor() keys its cache on the id ALONE + // and ignores the name on a hit, so reusing one id here would hand every + // scope after the first the first one's key. + temp.getAutoKeyFor(repeat_scope_count, hashtag, repeat_scopes[repeat_scope_count]); + repeat_scope_count++; } tok = strtok(NULL, ","); } @@ -2807,6 +2811,7 @@ void MyMesh::handleCmdFrame(size_t len) { if (n > 0 && n < 31) { strcpy(_prefs.default_scope_name, (char *) &cmd_frame[1]); memcpy(_prefs.default_scope_key, &cmd_frame[1+31], 16); + rebuildRepeatScopes(); // slot 0 of the relay filter tracks this key savePrefs(); writeOKFrame(); } else { @@ -2815,6 +2820,7 @@ void MyMesh::handleCmdFrame(size_t len) { } else { memset(_prefs.default_scope_name, 0, sizeof(_prefs.default_scope_name)); // set default scope to null memset(_prefs.default_scope_key, 0, sizeof(_prefs.default_scope_key)); + rebuildRepeatScopes(); // drop it from the relay filter too, not just from sends savePrefs(); writeOKFrame(); } diff --git a/examples/companion_radio/NodePrefs.h b/examples/companion_radio/NodePrefs.h index f24cb101..991f813b 100644 --- a/examples/companion_radio/NodePrefs.h +++ b/examples/companion_radio/NodePrefs.h @@ -208,24 +208,15 @@ struct NodePrefs { // persisted to file // fringe traffic isn't re-flooded. REPEAT_SNR_DISABLED (-128) = off. // repeat_suppress_dup: 1 = cancel a queued retransmit when the same flood is // overheard from another node first (less redundant airtime in dense mesh). - // repeat_scope_only: 1 = only forward flood packets matching this device's - // own scope (Settings > Radio > Scope, default_scope_key) or one of the - // repeat_extra_scopes below — drops unscoped floods and floods tagged for - // a different community. A no-op (forwards everything, unchanged) while - // no scope is configured at all, so enabling this on an unconfigured - // device can't silently blackhole all flood traffic. + // (repeat_scope_only / repeat_extra_scopes belong to this group too, but live + // at the struct tail — the file format is strictly append-only, so a new + // field can never be slotted in next to its logical siblings.) uint8_t repeat_skip_adverts; uint8_t repeat_max_hops; uint8_t repeat_delay_boost; int8_t repeat_min_snr; static const int8_t REPEAT_SNR_DISABLED = -128; uint8_t repeat_suppress_dup; - uint8_t repeat_scope_only; - // Extra region names this repeater also relays for, beyond its own - // Settings > Radio > Scope (comma-separated, e.g. "eu,de") — see - // MyMesh::rebuildRepeatScopes(). Relay-only: never affects what scope the - // companion's own messages send under, only what repeat_scope_only accepts. - char repeat_extra_scopes[24]; // Optional dedicated radio profile for repeater mode. When repeater_use_profile // is 1, enabling the repeater switches the radio to repeater_freq/bw/sf/cr and @@ -463,6 +454,22 @@ struct NodePrefs { // persisted to file uint8_t interference_threshold; uint8_t cad_enabled; + // Repeater scope filter — logically part of the repeat_* group far above, but + // the on-disk format is append-only (see the tripwire note at the bottom of + // this file), so it has to live here at the tail. + // repeat_scope_only: 1 = only forward flood packets matching this device's + // own scope (Settings > Radio > Scope, default_scope_key) or one of the + // repeat_extra_scopes below — drops unscoped floods and floods tagged for + // a different community. A no-op (forwards everything, unchanged) while + // no scope is configured at all, so enabling this on an unconfigured + // device can't silently blackhole all flood traffic. + uint8_t repeat_scope_only; + // Extra region names this repeater also relays for, beyond its own + // Settings > Radio > Scope (comma-separated, e.g. "eu,de") — see + // MyMesh::rebuildRepeatScopes(). Relay-only: never affects what scope the + // companion's own messages send under, only what repeat_scope_only accepts. + char repeat_extra_scopes[24]; + // Single source of truth for the live-share option tables (shared by the Map // UI labels and the auto-send engine in UITask). static const uint8_t LOC_SHARE_MOVE_COUNT = 4; @@ -525,7 +532,12 @@ struct NodePrefs { // persisted to file // adding/removing/reordering fields in DataStore::savePrefs/loadPrefsInt so // older saves are detected on load and skipped (zero-init defaults kept). // High 24 bits identify the file format; low byte is the schema revision. - static const uint32_t SCHEMA_SENTINEL = 0xC0DE0026; + // 0xC0DE0026 is BURNED — it briefly named a layout that put repeat_scope_only + // + repeat_extra_scopes in the middle of the stream (next to the other + // repeat_* fields) instead of at the tail, which shifted every field after + // them by 25 bytes when loading an older file. Never released, but a dev + // build wrote it, so the number must not be reused for anything else. + static const uint32_t SCHEMA_SENTINEL = 0xC0DE0027; // Bit-index for each home page. Used by page_order (entries store bit+1) and // by home_pages_mask. Single source of truth — both HomeScreen::pageBit/bitToPage @@ -617,9 +629,18 @@ struct NodePrefs { // persisted to file // // This assert is the manual checkpoint. Changing a data member changes sizeof // and trips it. When it trips, do ALL of the following, then update the number: +// 0. put the new field at the TAIL of the struct, even when it logically +// belongs beside older siblings. loadPrefsInt()'s rd() is a plain +// sequential reader gated only on file.available() — there is no per-field +// versioning — so a field inserted mid-stream is read out of an older +// file's bytes and shifts EVERY field after it (repeat_scope_only + +// repeat_extra_scopes did exactly that in the burned 0xC0DE0026 layout). +// "In struct order" below therefore means "appended in both places". // 1. add the field's rd(...) in DataStore::loadPrefsInt(), in struct order // 2. add the field's write(...) in DataStore::savePrefs(), in struct order -// 3. clamp it on load (an upgrader's file lacks it → stray bytes) +// 3. clamp it on load (an upgrader's file lacks it → stray bytes; the first +// few are that file's own 4-byte sentinel tail, so a plausible-looking +// value like 0x23 shows up rather than 0) // 4. bump SCHEMA_SENTINEL's low byte // (Padding can also shift sizeof; a "false" trip just means re-check + rebump.) // keyboard_cardkb_compact (0xC0DE0023) also landed in existing tail padding -- @@ -637,6 +658,10 @@ struct NodePrefs { // persisted to file // 0xC0DE0024 bump -- confirmed via a real build, sizeof unchanged at 2728. // repeat_extra_scopes[24] (0xC0DE0026) added exactly 24 bytes, no leftover // padding this time -- confirmed via a real build, sizeof now 2752. +// 0xC0DE0027 moved those same two fields out of the repeat_* group and down to +// the struct tail (the 0xC0DE0026 mid-stream layout was unreadable for older +// files, see the sentinel comment) -- padding worked out identically either +// way, so sizeof stays 2752. Confirmed via a real Heltec_v3 build. static_assert(sizeof(NodePrefs) == 2752, "NodePrefs layout changed — sync DataStore save/load + clamp, bump " "SCHEMA_SENTINEL, then update this size (see steps above)."); diff --git a/examples/companion_radio/ui-new/KeyboardWidget.h b/examples/companion_radio/ui-new/KeyboardWidget.h index 7d637bee..6f606b15 100644 --- a/examples/companion_radio/ui-new/KeyboardWidget.h +++ b/examples/companion_radio/ui-new/KeyboardWidget.h @@ -577,18 +577,18 @@ struct KeyboardWidget { // ...and the byte offset that line starts at. int ps = 0; for (int n = first_line * cpl; n > 0 && ps < len; n--) ps += kbUtf8CharBytesAt(buf, ps, len); - bool cursor_drawn = false; // draw it on exactly one line, even once the text itself runs out for (int pl = 0; pl < prev_lines; pl++) { int pe = ps; // byte offset cpl codepoints further along (or end of text) for (int k = 0; k < cpl && pe < len; k++) pe += kbUtf8CharBytesAt(buf, pe, len); - // cursor_pos == len == pe is the common "typing at the end" case: that's - // this line's cursor only if THIS is where the text actually ends (pe == - // len), not just whichever line happens to be the bottom of the preview - // area -- short text (fitting in fewer than prev_lines rows) would - // otherwise always show the cursor stranded on the last blank row - // instead of right after what was just typed. - bool cursor_here = !cursor_drawn && ps <= cursor_pos && (cursor_pos < pe || pe == len); - if (cursor_here) cursor_drawn = true; + // Which row the cursor is on is already decided above, by the same + // cursor_chars / cpl the scroll window uses -- just ask it. Deriving it a + // second time from byte offsets here is what put the cursor in the wrong + // place: "cursor_pos >= ps and this is the bottom row" stranded it on the + // last blank row whenever the text was shorter than the preview area, + // and "text ends here" instead pinned it to the end of a full line + // (drawing the '_' one character past the display width) at every + // wrap boundary, where cursor_line has already moved to the next row. + bool cursor_here = (first_line + pl == cursor_line); int line_end = (len < pe) ? len : pe; char linebuf[KB_PREVIEW_BYTES + 2]; // cpl codepoints + cursor '_' + NUL if (cursor_here) {