Fixing file deployent (nanoff or Visual Studio) for ChibiOS targets#3494
Fixing file deployent (nanoff or Visual Studio) for ChibiOS targets#3494Ellerbach wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 11 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLittleFS storage operations now normalize incoming paths, create intermediate directories safely, persist custom file attributes during writes and appends, and explicitly handle deletion results. ChangesLittleFS storage operations
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HAL_StorageOperation
participant create_directories
participant LittleFS
HAL_StorageOperation->>HAL_StorageOperation: Normalize storageName into lfsPath
HAL_StorageOperation->>create_directories: Create intermediate directories
create_directories->>LittleFS: Create or validate directories
HAL_StorageOperation->>LittleFS: Open file with NANO_LITTLEFS_ATTRIBUTE
HAL_StorageOperation->>LittleFS: Sync attributes
HAL_StorageOperation->>LittleFS: Write or append data
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp (1)
171-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite and Append branches duplicate the mkdir/opencfg/attribute-sync logic almost verbatim.
The two ~40-line blocks (directory extraction,
create_directoriescall,lfs_attr/lfs_file_configsetup,lfs_file_opencfg,lfs_file_sync) differ only in the open flags. Any future fix (e.g. the ignoredlfs_file_syncreturn above) has to be applied twice, and already risks drifting.♻️ Sketch of a shared helper
static uint32_t OpenLittleFsFileWithAttributes( lfs_t *lfsDrive, lfs_file_t *lfsFile, const char *lfsPath, char *dirPath, int openFlags) { if (dirPath[0] != '\0') { if (create_directories(lfsDrive, dirPath) != 0) { return StorageOperationErrorCode::WriteError; } } uint32_t nanoAttributes = kFileAttributesNormal; struct lfs_attr attr = {NANO_LITTLEFS_ATTRIBUTE, &nanoAttributes, NANO_LITTLEFS_ATTRIBUTE_SIZE}; struct lfs_file_config fileConfig = {.buffer = NULL, .attrs = &attr, .attr_count = 1}; if (lfs_file_opencfg(lfsDrive, lfsFile, lfsPath, openFlags, &fileConfig) != LFS_ERR_OK) { return StorageOperationErrorCode::WriteError; } if (lfs_file_sync(lfsDrive, lfsFile) != LFS_ERR_OK) { lfs_file_close(lfsDrive, lfsFile); return StorageOperationErrorCode::WriteError; } return StorageOperationErrorCode::NoError; }Call this once with
LFS_O_RDWR | LFS_O_CREATfor Write andLFS_O_RDWR | LFS_O_APPEND | LFS_O_CREATfor Append.🤖 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 `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp` around lines 171 - 217, Extract the duplicated directory creation, attribute configuration, file opening, and synchronization logic from the Write and Append branches into a shared helper such as OpenLittleFsFileWithAttributes. Have it return the appropriate StorageOperationErrorCode, close the file and report WriteError when lfs_file_sync fails, and invoke it with the existing Write and Append-specific open flags.
🤖 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.
Inline comments:
In `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp`:
- Around line 205-206: Handle the return value of lfs_file_sync in the
custom-attribute persistence path before the subsequent write. If syncing fails,
stop the operation, perform the same file-handle cleanup used by this function,
and propagate the failure so the write does not proceed with an unsaved
attribute.
- Around line 23-25: Rename the constant kFileAttributesNormal to follow the
repository’s c_ naming convention, and update both usage sites in the Write and
Append branches to reference the new name.
- Line 36: Change the file-scope helper create_directories to have internal
linkage by marking its definition static. Leave its signature and behavior
unchanged.
- Around line 126-149: The path normalization before create_directories() only
removes leading separators for drive-prefixed paths. Update the lfsPath
normalization in the storage operation flow so bare-rooted inputs such as \foo
and /foo also have all leading separators removed before directory creation,
while preserving drive-prefix stripping and separator conversion.
---
Outside diff comments:
In `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp`:
- Around line 171-217: Extract the duplicated directory creation, attribute
configuration, file opening, and synchronization logic from the Write and Append
branches into a shared helper such as OpenLittleFsFileWithAttributes. Have it
return the appropriate StorageOperationErrorCode, close the file and report
WriteError when lfs_file_sync fails, and invoke it with the existing Write and
Append-specific open flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17e7574c-5744-4e0f-9e7e-825efdd8566b
📒 Files selected for processing (1)
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp
josesimoes
added
the
Platform: ChibiOS
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)
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp (2)
160-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe fixed 256-byte directory buffer truncates paths in both branches.
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp#L160-L177: derive the directory path in place fromlfsPath, or allocate it based on the full path length.targets/ChibiOS/_common/targetHAL_StorageOperation.cpp#L227-L244: apply the same non-truncating directory extraction to Append.🤖 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 `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp` around lines 160 - 177, Replace the fixed 256-byte directory buffers in the directory-extraction logic for both the primary storage operation (lines 160-177) and Append (lines 227-244) with in-place extraction from lfsPath or storage sized to the complete path length; preserve root-path handling and pass the full directory path to create_directories without truncation.
215-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck
lfs_file_close()before reporting write success.LittleFS performs pending sync operations in
lfs_file_close(), so both the Write and Append paths can returnNoErroreven when persistence fails. Map failed closes at both sites toWriteErrorand follow the failure path instead of falling through to success.
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp:223: map Write close failures toWriteError.
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp:286: map Append close failures toWriteError.🤖 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 `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp` around lines 215 - 223, Update the Write path around lfs_file_close() at targets/ChibiOS/_common/targetHAL_StorageOperation.cpp:215-223 to check its return value, map a failed close to StorageOperationErrorCode::WriteError, and follow the existing failure path instead of reporting success; apply the same close-result handling to the Append path at targets/ChibiOS/_common/targetHAL_StorageOperation.cpp:276-284.
🤖 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 `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp`:
- Around line 160-177: Replace the fixed 256-byte directory buffers in the
directory-extraction logic for both the primary storage operation (lines
160-177) and Append (lines 227-244) with in-place extraction from lfsPath or
storage sized to the complete path length; preserve root-path handling and pass
the full directory path to create_directories without truncation.
- Around line 215-223: Update the Write path around lfs_file_close() at
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp:215-223 to check its
return value, map a failed close to StorageOperationErrorCode::WriteError, and
follow the existing failure path instead of reporting success; apply the same
close-result handling to the Append path at
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp:276-284.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: afa08a5c-94e4-4171-a007-4bf725e6caaf
📒 Files selected for processing (1)
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp
50fb1e7 to
01a07bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp`:
- Around line 23-25: Rename the constant c_kFileAttributesNormal to
c_FileAttributesNormal and update both nanoAttributes assignments to use the new
name, preserving the existing value and behavior.
- Around line 227-277: Update the Append branch around lfs_file_opencfg to
zero-initialize lfsFile before opening it, matching the Write branch. Check the
lfs_file_sync result before writing and closing; on failure, set the same
WriteError and follow the existing cleanup path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2daa1601-ad19-4879-a471-9b2b8a74e5b9
📒 Files selected for processing (1)
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp
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)
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp (2)
160-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid truncating paths before creating parent directories.
lfsPathcan exceed 255 bytes, but both branches silently truncate it indir_path; directories are created for the truncated prefix while LittleFS opens the original path, leaving its actual parent missing.
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp#L160-L177: derive the directory buffer from the fulllfsPathlength or reject overlong paths.targets/ChibiOS/_common/targetHAL_StorageOperation.cpp#L229-L246: apply the same full-length handling in Append.🤖 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 `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp` around lines 160 - 177, The directory extraction logic in the write path and the corresponding Append path must not truncate lfsPath before creating parent directories. Update both sites in targets/ChibiOS/_common/targetHAL_StorageOperation.cpp (lines 160-177 and 229-246) to use the full path length or reject overlong paths before directory creation, preserving the existing root-directory handling and LittleFS open behavior.
207-215: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate failed final file closes in the LittleFS write paths.
lfs_file_syncbefore the write only persists the new attribute, not the payload. After the write, a failinglfs_file_closecan leave the write uncommitted but the function still reports success. Update both write branches to check the finallfs_file_closereturn and setWriteErroron failure at lines 223 and 291.🤖 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 `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp` around lines 207 - 215, Update both LittleFS write branches in targets/ChibiOS/_common/targetHAL_StorageOperation.cpp at lines 207-215 and 274-280: check the final lfs_file_close return after writing, and set errorCode to StorageOperationErrorCode::WriteError when it fails so the write operation does not report success. The related close calls at lines 223 and 291 require this handling.
🤖 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 `@targets/ChibiOS/_common/targetHAL_StorageOperation.cpp`:
- Around line 160-177: The directory extraction logic in the write path and the
corresponding Append path must not truncate lfsPath before creating parent
directories. Update both sites in
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp (lines 160-177 and
229-246) to use the full path length or reject overlong paths before directory
creation, preserving the existing root-directory handling and LittleFS open
behavior.
- Around line 207-215: Update both LittleFS write branches in
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp at lines 207-215 and
274-280: check the final lfs_file_close return after writing, and set errorCode
to StorageOperationErrorCode::WriteError when it fails so the write operation
does not report success. The related close calls at lines 223 and 291 require
this handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b9f1ea7-d534-4f51-ab42-3d624680e4c0
📒 Files selected for processing (1)
targets/ChibiOS/_common/targetHAL_StorageOperation.cpp
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
I'm all for verbose comments when something requires explanation or is not obvious, but most of these comments look like a bit too much... if we keep this pace, we'll end up with as many comment lines as code lines with the explanation of every code block. 😅
Well noticed for next time :-D |
3 participants
Description
Fixing file deployent (nanoff or Visual Studio) for ChibiOS targets
Motivation and Context
How Has This Been Tested?
On real RP devices
Screenshots
Types of changes
Checklist