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 FlasherNote Building this pull request… the flash button, badges and supported-board |
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
3 participants
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