Mirrors/MeshCore-Solo
1
0
Fork 0
mirror of https://github.com/MarekZegare4/MeshCore-Solo.git synced 2026-09-14 15:16:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(companion): prefs schema corruption, broken multi-scope, stale relay filter, cursor row

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Jakub
2026-08-21 11:02:05 +02:00
co-authored by Claude Opus 5
parent f589b9b2d1
commit 51c06b78c1
4 changed files with 77 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
examples/companion_radio/DataStore.cpp
+22 -16
View File
@@ -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
examples/companion_radio/MyMesh.cpp
+7 -1
View File
@@ -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();
}
examples/companion_radio/NodePrefs.h
+39 -14
View File
@@ -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).");
examples/companion_radio/ui-new/KeyboardWidget.h
+9 -9
View File
@@ -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) {