Make Safe File actually safe#10949
Conversation
📝 WalkthroughWalkthroughSafeFile now internally acquires spiLock during write operations and creates parent directories in openFile(). Numerous callers (MessageStore, GPS, NodeDB, WarmNodeStore, mesh-pb-constants, CannedMessageModule/Store, HopScalingModule, MotionSensor, FlashData) remove their own explicit spiLock guards and directory-creation calls, relying on SafeFile's internal handling. ChangesSPI Lock and Directory Creation Consolidation into SafeFile
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SafeFile
participant spiLock
participant FSCom
Caller->>SafeFile: openFile(path)
SafeFile->>FSCom: mkdir(parentDir) if path contains '/'
Caller->>SafeFile: write(data)
SafeFile->>spiLock: acquire LockGuard
SafeFile->>FSCom: write bytes to file
spiLock-->>SafeFile: release on scope exit
SafeFile-->>Caller: write complete
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (27)
Build artifacts expire on 2026-08-08. Updated for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/SafeFile.cpp (2)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant directory-creation comment.
The block is self-explanatory, so this can be dropped. As per coding guidelines, keep code comments minimal and avoid comments that restate the next line.
Suggested cleanup
- // Create the containing directory if it doesn't already exist size_t slashIndex = filenameTmp.find_last_of('/');🤖 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/SafeFile.cpp` at line 13, Remove the redundant directory-creation comment in SafeFile.cpp; the surrounding logic is already self-explanatory, so delete that standalone comment and keep the nearby code unchanged. Focus on the comment immediately before the directory existence check/creation block and leave the implementation intact.Source: Coding guidelines
8-9: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an explicit
<string>include
src/SafeFile.cppusesstd::string, butsrc/SafeFile.honly pulls inFSCommon.h,SPILock.h, andconfiguration.h. Include<string>directly to avoid depending on indirect includes.🤖 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/SafeFile.cpp` around lines 8 - 9, Add an explicit <string> include in SafeFile.cpp because std::string is used there but only indirect headers are currently relied on. Update the includes near SafeFile.cpp’s top so the translation unit does not depend on SafeFile.h or FSCommon.h to provide std::string.src/graphics/niche/Utils/FlashData.h (1)
110-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the 3-line comment block to comply with the comment-length guideline.
Lines 110-112 form a 3-line comment; the applicable guideline caps explanatory comments at one or two lines. The rest of the
save()change (dropping the manualmkdir/spiLock, two sequentialf.write()calls) is consistent withSafeFile's internal locking/mkdir contract.As per coding guidelines,
**/*.{c,cc,cpp,h,hpp}: "Keep code comments minimal: use at most one or two lines, only when the reason is not obvious; do not add multi-paragraph explanatory comments or restate what the next line does."✏️ Proposed trim
- // Save module's custom data (settings?) to flash. Doesn't use protobufs. - // SafeFile (constructor/write/close) takes the firmware's SPI lock for itself internally - // (relevant when files are on an SD card), so we must not hold it across those calls. + // Save module's custom data (settings?) to flash. Doesn't use protobufs. + // SafeFile takes the SPI lock itself internally; don't hold it across these calls. static void save(T *data, const char *label)🤖 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/graphics/niche/Utils/FlashData.h` around lines 110 - 134, The comment block above FlashData::save is too long for the guideline; trim the explanatory comments around the SafeFile usage to at most one or two concise lines. Keep the essential note about SafeFile handling atomic write, mkdir, and SPI locking internally, and remove the rest of the restated implementation details near save(), getFilename(), and the FSCom block.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/graphics/niche/Utils/FlashData.h`:
- Around line 110-134: The comment block above FlashData::save is too long for
the guideline; trim the explanatory comments around the SafeFile usage to at
most one or two concise lines. Keep the essential note about SafeFile handling
atomic write, mkdir, and SPI locking internally, and remove the rest of the
restated implementation details near save(), getFilename(), and the FSCom block.
In `@src/SafeFile.cpp`:
- Line 13: Remove the redundant directory-creation comment in SafeFile.cpp; the
surrounding logic is already self-explanatory, so delete that standalone comment
and keep the nearby code unchanged. Focus on the comment immediately before the
directory existence check/creation block and leave the implementation intact.
- Around line 8-9: Add an explicit <string> include in SafeFile.cpp because
std::string is used there but only indirect headers are currently relied on.
Update the includes near SafeFile.cpp’s top so the translation unit does not
depend on SafeFile.h or FSCommon.h to provide std::string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d6ebe0a-cd3d-4779-9ff7-7daf927eac7f
📒 Files selected for processing (11)
src/MessageStore.cppsrc/SafeFile.cppsrc/gps/GPS.cppsrc/graphics/niche/Utils/CannedMessageStore.cppsrc/graphics/niche/Utils/FlashData.hsrc/mesh/NodeDB.cppsrc/mesh/WarmNodeStore.cppsrc/mesh/mesh-pb-constants.cppsrc/modules/CannedMessageModule.cppsrc/modules/HopScalingModule.cppsrc/motion/MotionSensor.cpp
💤 Files with no reviewable changes (1)
- src/modules/HopScalingModule.cpp
|
In theory I agree with all of this. It's a risky 11th hour change I'd like to hold off on for now though |
Yes, agreed. It is worth noting that there are a couple instances where FSCom is currently getting called without taking the spiLock first. |
Maybe we can peel those off into a separate PR |
Firmware Size Report42 targets | vs
Show 37 more target(s)
Size budgets
Budgets live in Updated for b9679eb |
|
Hi @jp-bennett , Is this the right approach? If not I can change it for this (and any other sensor). |
3 participants
SafeFile has been something of a landmine, and several uses of it were failing to manage the spilock correctly. These changes eliminate the need to touch FSCom or spiLock directly for users of SafeFile.
These are pretty core changes, so lots of testing needed.
If we like this approach, I'd recommend adding file read routines to safe file, and work to eliminate raw FSCom access everywhere.
Summary by CodeRabbit