feat: preserve normal radio profiles across event firmware - #11110
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEvent-mode radio profiles now use dedicated persistence files. Boot loading initializes missing event configuration from the standard profile, tracks storage availability and config readiness, defers unsafe writes, and includes event files in encrypted-storage migrations. Unity tests cover the new helpers and storage rules. ChangesEvent profile persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NodeDB
participant Storage
participant StandardConfig
participant EventConfig
NodeDB->>Storage: Check event profile file and available space
NodeDB->>StandardConfig: Load standard configuration when event config is missing
StandardConfig-->>NodeDB: Return configuration and decode status
NodeDB->>EventConfig: Derive event LoRa configuration
NodeDB->>EventConfig: Persist initial event configuration
NodeDB->>Storage: Defer or suppress boot writes when recovery is incomplete
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (31)
Build artifacts expire on 2026-08-25. Updated for |
b5919c4 to
65f92fc
Compare
|
Review follow-up: force-pushed
The draft remains blocked only on hardware verification and the existing macOS native-test |
65f92fc to
0bc1176
Compare
|
Updated to The remaining |
|
macOS native simulator verification on
The stable PKI-derived node number also remained |
|
Follow-up verification found and fixed a real degraded-boot persistence bug. When the normal config was corrupt on the first event boot, the original change kept config.proto and channels.proto intact but still rewrote device.proto and nodes.proto. That could change the saved node identity despite the intended "freezing identity" behavior. This update defers automatic protobuf writes during initialization until config loading is verified. A decode-failed boot now remains degraded and radio-silent without writing normal files, event profile files, device state, or the node database. Explicit runtime configuration saves remain available after initialization. Verification:
The event-only userPrefs file used for the simulator remains a local test fixture and is not part of the PR. |
|
@jp-bennett @vidplace7 Please review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/mesh/NodeDB.h (2)
115-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense code comments to comply with maximum length guidelines.
As per coding guidelines, code comments in C++ files should be minimal—one or two lines maximum.
src/mesh/NodeDB.h#L115-L117: Condense this 3-line comment to one or two lines.src/mesh/NodeDB.h#L137-L139: Condense this 3-line comment to one or two lines.src/mesh/NodeDB.h#L583-L585: Condense this 3-line comment to one or two lines.src/mesh/NodeDB.cpp#L2172-L2174: Condense this 3-line comment to one or two lines.src/mesh/NodeDB.cpp#L2399-L2402: Condense this 4-line comment to one or two lines.src/mesh/NodeDB.cpp#L2610-L2612: Condense this 3-line comment to one or two lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NodeDB.h` around lines 115 - 117, Condense the comments at src/mesh/NodeDB.h lines 115-117, 137-139, and 583-585, and src/mesh/NodeDB.cpp lines 2172-2174, 2399-2402, and 2610-2612 to one or two lines each while preserving their existing meaning; no code behavior changes are needed.Source: Coding guidelines
118-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
UPPER_SNAKE_CASEfor constants.As per coding guidelines, C++ constants and defines must use
UPPER_SNAKE_CASE.
src/mesh/NodeDB.h#L118-L123: Rename these filename constants toUPPER_SNAKE_CASE(e.g.,STANDARD_CONFIG_FILE_NAME).src/mesh/NodeDB.h#L140-L140: RenameeventProfileStorageReservationBytestoEVENT_PROFILE_STORAGE_RESERVATION_BYTES.src/mesh/NodeDB.h#L161-L165: RenameradioProfileStoragetoRADIO_PROFILE_STORAGEon lines 162 and 164.(Note: If you decide to rename
eventProfileStorageReservationBytes, you will also need to update its downstream usages, such as intest/test_event_profile_storage/test_main.cpp)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NodeDB.h` around lines 118 - 123, Rename the filename constants in src/mesh/NodeDB.h lines 118-123 to UPPER_SNAKE_CASE, including STANDARD_CONFIG_FILE_NAME and corresponding channel, event, and backup names. Rename eventProfileStorageReservationBytes to EVENT_PROFILE_STORAGE_RESERVATION_BYTES at line 140 and update all downstream references, including test/test_event_profile_storage/test_main.cpp. Rename radioProfileStorage to RADIO_PROFILE_STORAGE at lines 161-165 and update its references.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/NodeDB.h`:
- Around line 115-117: Condense the comments at src/mesh/NodeDB.h lines 115-117,
137-139, and 583-585, and src/mesh/NodeDB.cpp lines 2172-2174, 2399-2402, and
2610-2612 to one or two lines each while preserving their existing meaning; no
code behavior changes are needed.
- Around line 118-123: Rename the filename constants in src/mesh/NodeDB.h lines
118-123 to UPPER_SNAKE_CASE, including STANDARD_CONFIG_FILE_NAME and
corresponding channel, event, and backup names. Rename
eventProfileStorageReservationBytes to EVENT_PROFILE_STORAGE_RESERVATION_BYTES
at line 140 and update all downstream references, including
test/test_event_profile_storage/test_main.cpp. Rename radioProfileStorage to
RADIO_PROFILE_STORAGE at lines 161-165 and update its references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 61751f2f-a470-48cb-b8d8-0ce8f8aa548a
📒 Files selected for processing (3)
src/mesh/NodeDB.cppsrc/mesh/NodeDB.htest/test_event_profile_storage/test_main.cpp
|
In general I really think this is the right approach. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mesh/NodeDB.cpp (2)
2183-2202: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReset
eventProfileStorageUnavailableon every load.The flag is assigned only when
eventConfigMissingis true. After a prior capacity failure, a repeatedloadFromDisk()can retaintrueeven after the event config exists, causing later config/channel writes to remain suppressed.Proposed fix
`#if` USERPREFS_EVENT_MODE bool eventConfigMissing = false; + eventProfileStorageUnavailable = false; `#ifdef` FSCom🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NodeDB.cpp` around lines 2183 - 2202, Reset eventProfileStorageUnavailable at the start of each load in the USERPREFS_EVENT_MODE initialization flow, before checking eventConfigMissing, so a successful reload clears any prior capacity-failure state. Preserve the existing storage-space calculation and error logging for missing configurations.
2618-2644: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPropagate
EncryptedStorage::migrateFile()failures here. It returnsfalseon read/encrypt/write errors; ignoring that can leave these files plaintext while lockdown stays active.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NodeDB.cpp` around lines 2618 - 2644, Check the return value of EncryptedStorage::migrateFile() in both the backup migration and inactive radio profile loop, and propagate failure through the surrounding save/migration flow. Do not continue as if migration succeeded when it returns false, ensuring lockdown cannot proceed while any targeted file remains plaintext.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mesh/NodeDB.cpp`:
- Around line 2183-2202: Reset eventProfileStorageUnavailable at the start of
each load in the USERPREFS_EVENT_MODE initialization flow, before checking
eventConfigMissing, so a successful reload clears any prior capacity-failure
state. Preserve the existing storage-space calculation and error logging for
missing configurations.
- Around line 2618-2644: Check the return value of
EncryptedStorage::migrateFile() in both the backup migration and inactive radio
profile loop, and propagate failure through the surrounding save/migration flow.
Do not continue as if migration succeeded when it returns false, ensuring
lockdown cannot proceed while any targeted file remains plaintext.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 312c0ef7-e6dc-490c-a27f-6ae20c79bd0a
📒 Files selected for processing (1)
src/mesh/NodeDB.cpp
There was a problem hiding this comment.
Pull request overview
This PR introduces an “event firmware” radio-profile isolation mechanism so event builds use separate on-device config/channel/backup files, allowing users to flash back to normal firmware and automatically resume their prior LoRa + channel configuration without app intervention.
Changes:
- Added radio-profile–scoped storage paths (standard vs event) for config/channels/backups, and helpers for seeding an event config from the standard identity while applying event LoRa defaults.
- Implemented event-profile creation + storage-space preflight and event-file cleanup in normal firmware.
- Added a focused native test suite covering storage-path selection, identity preservation, persistence deferral, and the reservation-space check.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/test_event_profile_storage/test_main.cpp |
Adds unit tests validating path isolation, event config derivation, persistence deferral logic, and storage-space preflight. |
src/mesh/NodeDB.h |
Defines standard/event storage filenames and helper utilities to select paths, seed event config, and gate boot persistence. |
src/mesh/NodeDB.cpp |
Wires profile selection into load/save flows, seeds missing event config from standard config, migrates backups for encrypted storage, and adds boot-write deferral + event-space gating. |
…tems The USERPREFS_EVENT_MODE storage preflight called FSCom.totalBytes() and FSCom.usedBytes(), which only exist on ESP32's LittleFS wrapper. Every other backend failed to compile once event mode was enabled: error: 'class InternalFileSystem' has no member named 'totalBytes' (nRF52) error: no member named 'totalBytes' in 'fs::FS' (Portduino) This was not caught earlier because the event block is behind #if USERPREFS_EVENT_MODE, and the existing unit tests only cover the pure helpers, which compile identically either way. Add fsTotalBytes()/fsUsedBytes() to FSCommon and implement them per backend: littlefs v1 traversal for nRF52 (Adafruit_LittleFS) and STM32 (STM32_LittleFS), FSInfo for RP2040, statvfs for Portduino, and the native methods for ESP32 and nRF54L15. The nRF52/STM32 path reports "full" if the traversal errors so the capacity check fails safe. Verified with USERPREFS_EVENT_MODE=1: native-macos test_event_profile_storage passes 5/5, and heltec-v3, rak4631, rak11310 and rak3172 all build clean.
…uite-count Two CI fixes: - native-windows has no <sys/statvfs.h>, so the Portduino branch of fsTotalBytes()/fsUsedBytes() broke the Windows build. Switch to std::filesystem::space(), which is already used under ARCH_PORTDUINO in HostMetrics.cpp and works on both POSIX and MinGW. Errors report "full" so the capacity check still fails safe. - This PR adds test/test_event_profile_storage, taking test/ from 40 to 41 suite directories, which trips the native-suite-count reconciliation check.
…eview Review feedback: - Copilot: saveProto()'s boot-write deferral applied to every file, so loadFromDisk()'s recovery writes (e.g. restoring owner fields into devicestate) were silently dropped and never retried, because the ctor CRC baselines are computed after loadFromDisk(). Deferral now applies only to the radio-profile files, via isRadioProfileFile(). - jp-bennett: eventConfigFromStandard() wrapped a struct copy plus one field assignment in a header helper with its own unit test. Inlined at its single call site and removed; the behaviour is covered end-to-end by hardware validation instead (RAK4631 normal -> event -> normal preserves NodeNum and public key while swapping LoRa, and restores the original channel byte-identically). - jp-bennett: the active-backup encrypted-storage migration ran for normal builds too, which changed non-event behaviour and contradicted the PR's stated event-only scope. Scoped to USERPREFS_EVENT_MODE; the adjacent block already covers the inactive standard backup in event builds. New tests (all verified to fail under mutation, not vacuous): - test_event_paths_never_collide_with_standard_or_shared_files: the core safety property. A path-table slip would make event firmware overwrite the user's real config, channels or backup, or capture a shared file like devicestate/nodedb. Nothing asserted this before. - test_only_radio_profile_files_defer_boot_writes: guards the deferral fix above so re-widening it fails loudly. - test_event_reservation_fits_smallest_supported_filesystem: the reservation is compile-time but must fit filesystems as small as 14 KiB (STM32WL) and 28 KiB (nRF52840). Protobuf growth pushing it past those would silently stop event profiles persisting - the exact failure mode confirmed by fault injection on a RAK4631. Verified with USERPREFS_EVENT_MODE both on and off: 7/7 tests pass, and rak4631, heltec-v3, rak11310 and rak3172 all build clean.
|
Benjamin Faershtein seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
45cb8e7
Event-only scope
This PR does not add or change channel QR behavior. Existing QR
ReplaceandAddremain the app workflow.What changes
/prefs/config.proto,/prefs/channels.proto, and/backups/backup.proto/prefs/event-config.proto,/prefs/event-channels.proto, and/backups/event-backup.protoStorage safety
saveToDisk()formatting the filesystem on a failed writeSafety
Verification
pio test -e native-macos -f test_event_profile_storagepio run -e native-macospio run -e rak3172(the 14 KiB STM32WL filesystem family)Design background: meshtastic/design#120 (comment)
Summary by CodeRabbit