feat: S29.05 PlusFAT-backed store on the FreeRTOS-Plus-TCP BDD target#526
Conversation
Lift the FatFs-specific mount / format-on-first-use / unmount and the SolidSyslogFile create/destroy out of BddTargetFreeRtosPipeline into a new BddTargetFatFsMount shim, reached through four function pointers on BddTargetFreeRtosPipelineConfig (MountStore / UnmountStore / CreateStoreFile / DestroyStoreFile). The pipeline no longer includes ff.h or SolidSyslogFatFsFile.h and is now FS-vendor-agnostic. Both targets still use FatFs via the shim — no behaviour change; sets up the PlusFAT swap on the Plus-TCP target.
Lift the BKPT 0xAB trap, the open/read/write/seek/length/close primitives,
the 8 MiB FAT16 geometry, and the open-or-create-sparse image logic out of
diskio.c into a new Bdd/Targets/Common/SemihostingDisk TU. diskio.c now maps
the ChaN-FatFs disk_* contract onto SemihostingDisk_{EnsureReady,IsReady,
Read,Write}, preserving its RES_NOTRDY / RES_PARERR / RES_ERROR distinctions.
This lets the upcoming FreeRTOS-Plus-FAT FF_Disk_t driver share byte-for-byte
the same host image and trap. No behaviour change. Verified both FreeRTOS
cross targets still build.
BLOCKED at link on a pre-existing S29.04 adapter defect — see below. - FFSemihostingDisk: FF_Disk_t media driver over the shared SemihostingDisk (mount-or-format-on-first-use, FAT16, IO-manager cache from heap_4). - BddTargetPlusFatMount: the Plus-FAT FS-mount seam impl. - Plus-TCP target main.c: install the PlusFat seam (was FatFs). - Plus-TCP CMakeLists: drop FatFs (ff.c/ffsystem.c/diskio.c/staging), add the 14 Plus-FAT core sources to the relaxed OBJECT lib (+ -Wno-cpp for ff_fat.c's one-FAT-table #warning), add the adapter/driver/shim to the strict executable (-Wno-sign-conversion only on FFSemihostingDisk.c for Plus-FAT's unsigned FF_ERR_* macros). - FreeRTOSConfig.h: configNUM_THREAD_LOCAL_STORAGE_POINTERS = 3 (ff_stdio TLS). - New integrator FreeRTOSFATConfig.h. - Shared pipeline: STORE_PATH_PREFIX "STORE" -> "/STORE" (Plus-FAT ff_stdio needs absolute paths with ffconfigHAS_CWD=0; valid for ChaN too). lwIP FatFs target still builds green. The Plus-TCP target COMPILES fully but does NOT LINK: SolidSyslogPlusFatFile.c calls ff_fflush(), which FreeRTOS-Plus-FAT (SHA 8d38036) only DECLARES in ff_stdio.h and never defines. PlusFatFake defines it, so S29.04 host tests pass and masked the gap. Real durability primitive is FF_FlushCache(Fp->pxIOManager). Fixing it touches the merged S29.04 Tier-2 adapter (+ the fake + host tests), which the S29.05 AC says to leave untouched — needs David's decision.
ff_fflush is declared in FreeRTOS-Plus-FAT's ff_stdio.h but never defined in the library (SHA 8d38036) — PlusFatFake defined it, so the S29.04 host tests passed while the real Plus-TCP target failed to link. Switch the adapter's per-write durability to FF_FlushCache(Fp->pxIOManager), the actual Plus-FAT cache-flush primitive (FF_FILE carries pxIOManager; ff_stdio.h pulls ff_ioman.h + ff_file.h, so no new includes). The directory-entry size is still committed on Close. Update PlusFatFake (FF_FlushCache spy replaces ff_fflush) and the three durability tests. Approved expansion of the S29.05 scope (the adapter was the one place the host fake masked a real-library gap). Host: SolidSyslogPlusFatFileTest 30/30 green. Plus-TCP cross target now links; nm confirms FF_FlushCache present, no ff_fflush, no FatFs symbols.
- docs/integrating-plusfat.md: integrator guide (FF_Disk_t media-driver contract, FreeRTOSFATConfig.h + kernel-config requirements, absolute-path convention, FF_FlushCache durability + dirent-on-close nuance, heap-is-vendor-scoped note, the BDD target as reference integration). - DEVLOG: S29.05 session entry (prepended). - clang-format reflow of the two new seam headers' pointer alignment. misra_suppressions.txt left untouched: misra_renumber proposed no changes; the AMBIGUOUS pairs it reported are pre-existing cross-run cppcheck variance in Core headers this story never touched (per the cppcheck-misra-invariant note).
|
Warning Review limit reached
More reviews will be available in 51 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR introduces a filesystem-mount seam in the shared FreeRTOS BDD pipeline, enabling per-target filesystem vendor selection. FatFs is retained for the lwIP target; PlusFAT is introduced for the PlusTCP target. Both use a shared semihosting disk abstraction backed by BKPT 0xAB trap code and 8 MiB FAT16 geometry. ChangesPipeline FS-mount seam and refactoring
Semihosting disk layer
FatFs disk layer and mount shim
Plus-FAT media driver and mount shim
Plus-FAT cache-flush update
Build and runtime configuration
Documentation and changelog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1428 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Bdd/Targets/Common/FFSemihostingDisk.c (1)
145-151: ⚡ Quick winExtract disk validation expression into a static inline predicate.
The same 3-part composite condition is duplicated in both callbacks; a named predicate clarifies intent and keeps both paths in sync.
💡 Proposed refactor
static int32_t ReadBlocks(uint8_t* buffer, uint32_t sector, uint32_t count, FF_Disk_t* disk); static int32_t WriteBlocks(uint8_t* buffer, uint32_t sector, uint32_t count, FF_Disk_t* disk); static FF_Error_t PartitionAndFormat(void); +static inline bool FFSemihostingDisk_IsInvalidDisk(const FF_Disk_t* disk); + +static inline bool FFSemihostingDisk_IsInvalidDisk(const FF_Disk_t* disk) +{ + return (disk == NULL) || (disk->ulSignature != DISK_SIGNATURE) || (disk->xStatus.bIsInitialised == pdFALSE); +} @@ - if ((disk == NULL) || (disk->ulSignature != DISK_SIGNATURE) || (disk->xStatus.bIsInitialised == pdFALSE)) + if (FFSemihostingDisk_IsInvalidDisk(disk)) @@ - if ((disk == NULL) || (disk->ulSignature != DISK_SIGNATURE) || (disk->xStatus.bIsInitialised == pdFALSE)) + if (FFSemihostingDisk_IsInvalidDisk(disk))As per coding guidelines: “Extract composite boolean conditions into intent-naming static inline predicates ... rather than inlining them into
if/return.”Also applies to: 170-176
🤖 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 `@Bdd/Targets/Common/FFSemihostingDisk.c` around lines 145 - 151, Extract the duplicated 3-part disk validation into a single static inline predicate (e.g., static inline BaseType_t DiskIsInvalid(const FF_Disk_t* disk) or DiskIsValid) and use it from ReadBlocks and the corresponding write callback (the function that contains the same check at 170-176) instead of repeating the composite if expression; implement the predicate to check for NULL, ulSignature != DISK_SIGNATURE and xStatus.bIsInitialised == pdFALSE (returning true for invalid or false for valid depending on your naming) and replace the original composite condition in ReadBlocks and the other callback with a call to that predicate to keep intent clear and both paths consistent.Bdd/Targets/Common/diskio.c (1)
38-44: ⚡ Quick winExtract repeated readiness check into a named predicate.
The same composite readiness condition is inlined in both read and write paths; a small predicate keeps intent explicit and removes duplication.
💡 Proposed refactor
`#include` "SemihostingDisk.h" +#include <stdbool.h> + +static inline bool Diskio_IsNotReady(BYTE pdrv) +{ + return (pdrv != 0) || (!SemihostingDisk_IsReady()); +} @@ - if ((pdrv != 0) || !SemihostingDisk_IsReady()) + if (Diskio_IsNotReady(pdrv)) @@ - if ((pdrv != 0) || !SemihostingDisk_IsReady()) + if (Diskio_IsNotReady(pdrv))As per coding guidelines: “Extract composite boolean conditions into intent-naming static inline predicates ... rather than inlining them into
if/return.”Also applies to: 63-69
🤖 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 `@Bdd/Targets/Common/diskio.c` around lines 38 - 44, Extract the repeated composite readiness check ((pdrv != 0) || !SemihostingDisk_IsReady()) used in disk_read and the corresponding check in disk_write into a clearly named static inline predicate (e.g., static inline bool SemihostingDisk_NotReady(BYTE pdrv) or SemihostingDisk_IsInvalidDriveOrNotReady) and replace the inlined conditionals with calls to that predicate; update both disk_read and disk_write to use the new predicate so intent is explicit and duplication is removed while keeping existing return values (RES_NOTRDY) and behavior intact.Bdd/Targets/Common/SemihostingDisk.c (1)
40-47: ⚡ Quick winMove helper definitions directly below first use.
The semihosting helper definitions are grouped at the file end instead of being placed immediately beneath their first caller.
As per coding guidelines: “Functions in source files ordered top-down ... helpers forward-declared at top and defined immediately beneath first caller”.
Also applies to: 153-222
🤖 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 `@Bdd/Targets/Common/SemihostingDisk.c` around lines 40 - 47, The helper functions (Semihosting, SemihostingOpen, SemihostingRead, SemihostingWrite, SemihostingSeek, SemihostingFlen, SemihostingClose) are forward-declared at the top but defined at the end of the file; move each helper's full definition to immediately follow the first caller (i.e., place the Semihosting* function implementations directly beneath the function that uses them first) and remove unnecessary forward declarations so the file follows the top-down ordering guideline and helpers appear right after their first use.
🤖 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 `@Bdd/Targets/Common/SemihostingDisk.c`:
- Around line 95-105: The current range check in SemihostingDisk_Read (and the
analogous check in SemihostingDisk_Write) can overflow because sector + count is
computed in 32-bit and may wrap; change the check to compute the sum in a wider
type or use a subtraction-based test to avoid wrap: either cast to uint64_t and
do uint64_t end = (uint64_t)sector + (uint64_t)count; if (end >
(uint64_t)SEMIHOSTING_DISK_SECTOR_COUNT) return SEMIHOSTING_DISK_OUT_OF_RANGE;
or use the safe check if (count != 0 && sector >
(uint32_t)(SEMIHOSTING_DISK_SECTOR_COUNT - count)) return
SEMIHOSTING_DISK_OUT_OF_RANGE; apply the same change inside
SemihostingDisk_Write to fix both bounds checks.
In `@docs/integrating-plusfat.md`:
- Line 17: The fenced code block containing "SolidSyslogBlockStore",
"SolidSyslogFileBlockDevice", and "your FF_Disk_t media driver" needs a language
label to satisfy markdownlint MD040; update the unlabeled triple-backtick fence
to include a language token (e.g., use "text") so the block becomes ```text
before the lines and the closing ``` remains unchanged; locate the block by
searching for the lines "SolidSyslogBlockStore" or "SolidSyslogFileBlockDevice"
and add the language label.
In `@Platform/PlusFat/Source/SolidSyslogPlusFatFile.c`:
- Line 101: Extract the composite return condition into a clearly named static
inline predicate, e.g. IsWriteFlushed(...), instead of returning wroteAll &&
(FF_FlushCache(self->Fp->pxIOManager) == FF_ERR_NONE) inline; add a file-scoped
static inline function that takes the same context needed (the current self
pointer and the wroteAll boolean), calls FF_FlushCache(self->Fp->pxIOManager)
and returns a bool comparing to FF_ERR_NONE combined with wroteAll, then replace
the original return expression with a call to that predicate to make the intent
self-documenting.
---
Nitpick comments:
In `@Bdd/Targets/Common/diskio.c`:
- Around line 38-44: Extract the repeated composite readiness check ((pdrv != 0)
|| !SemihostingDisk_IsReady()) used in disk_read and the corresponding check in
disk_write into a clearly named static inline predicate (e.g., static inline
bool SemihostingDisk_NotReady(BYTE pdrv) or
SemihostingDisk_IsInvalidDriveOrNotReady) and replace the inlined conditionals
with calls to that predicate; update both disk_read and disk_write to use the
new predicate so intent is explicit and duplication is removed while keeping
existing return values (RES_NOTRDY) and behavior intact.
In `@Bdd/Targets/Common/FFSemihostingDisk.c`:
- Around line 145-151: Extract the duplicated 3-part disk validation into a
single static inline predicate (e.g., static inline BaseType_t
DiskIsInvalid(const FF_Disk_t* disk) or DiskIsValid) and use it from ReadBlocks
and the corresponding write callback (the function that contains the same check
at 170-176) instead of repeating the composite if expression; implement the
predicate to check for NULL, ulSignature != DISK_SIGNATURE and
xStatus.bIsInitialised == pdFALSE (returning true for invalid or false for valid
depending on your naming) and replace the original composite condition in
ReadBlocks and the other callback with a call to that predicate to keep intent
clear and both paths consistent.
In `@Bdd/Targets/Common/SemihostingDisk.c`:
- Around line 40-47: The helper functions (Semihosting, SemihostingOpen,
SemihostingRead, SemihostingWrite, SemihostingSeek, SemihostingFlen,
SemihostingClose) are forward-declared at the top but defined at the end of the
file; move each helper's full definition to immediately follow the first caller
(i.e., place the Semihosting* function implementations directly beneath the
function that uses them first) and remove unnecessary forward declarations so
the file follows the top-down ordering guideline and helpers appear right after
their first use.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 861f4d4c-c256-4205-af4e-edb8c0403905
📒 Files selected for processing (23)
Bdd/Targets/Common/BddTargetFatFsMount.cBdd/Targets/Common/BddTargetFatFsMount.hBdd/Targets/Common/BddTargetFreeRtosPipeline.cBdd/Targets/Common/BddTargetFreeRtosPipeline.hBdd/Targets/Common/BddTargetPlusFatMount.cBdd/Targets/Common/BddTargetPlusFatMount.hBdd/Targets/Common/FFSemihostingDisk.cBdd/Targets/Common/FFSemihostingDisk.hBdd/Targets/Common/SemihostingDisk.cBdd/Targets/Common/SemihostingDisk.hBdd/Targets/Common/diskio.cBdd/Targets/FreeRtos/CMakeLists.txtBdd/Targets/FreeRtos/FreeRTOSConfig.hBdd/Targets/FreeRtos/FreeRTOSFATConfig.hBdd/Targets/FreeRtos/main.cBdd/Targets/FreeRtosLwip/CMakeLists.txtBdd/Targets/FreeRtosLwip/main.cDEVLOG.mdPlatform/PlusFat/Source/SolidSyslogPlusFatFile.cTests/PlusFat/SolidSyslogPlusFatFileTest.cppTests/Support/PlusFatFakes/Interface/PlusFatFake.hTests/Support/PlusFatFakes/Source/PlusFatFake.cdocs/integrating-plusfat.md
CodeRabbit + review feedback on the S29.05 sources (no behaviour change): - SemihostingDisk.c: overflow-safe SemihostingDisk_IsRangeValid (subtraction form, no sector+count wrap), shared by Read/Write; reorder the Semihosting* primitives beneath their first callers (top-down reading). - FFSemihostingDisk.c: extract the duplicated disk-validation check into FFSemihostingDisk_IsValidDisk. - diskio.c: extract the readiness check into Diskio_IsDriveReady. - SolidSyslogPlusFatFile.c: reorder so Close sits beneath its first caller (Cleanup); add PlusFatFile_IsFileOpen used at every open-state site; extract the write durability flush into PlusFatFile_Flush (honest action verb; parenthesise the wroteAll equality). - docs/integrating-plusfat.md: label the architecture fence 'text' (MD040). Verified: clang-format clean tree-wide; host SolidSyslogPlusFatFileTest 30/30; both FreeRTOS cross targets build.
|
Thanks for the review — all six addressed in
|
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1428 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
The intent-named-predicate reorder shifted SolidSyslogPlusFatFile.c's SelfFromBase cast from line 45 to 47, leaving the misra-c2012-11.3 suppression stale and the (deviated) cast unsuppressed — failing analyze-cppcheck. Point the suppression at the new line. cppcheck-misra clean on Platform/PlusFat again.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1428 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Last functional story of E29 (#525, parent #512). Gives the FreeRTOS-Plus-TCP QEMU BDD target a real FreeRTOS-Plus-FAT-backed store, proving the
SolidSyslogFileseam is FS-vendor-portable — ChaN-FatFs on the lwIP target, FreeRTOS-Plus-FAT on the Plus-TCP target — the same way E28 proved the transport seam is TCP-vendor-portable. FatFs relocated to lwIP in S29.01; PlusFAT lands here.Closes #525.
Change Description
BddTargetFreeRtosPipelineConfiggains four hooks (MountStore/UnmountStore/CreateStoreFile/DestroyStoreFile). FatFs logic moved toBddTargetFatFsMount, PlusFAT toBddTargetPlusFatMount; the pipeline no longer includesff.h/SolidSyslogFatFsFile.h.diskio.cinto a sharedSemihostingDiskTU, consumed by both the ChaNdiskio.cglue and the newFFSemihostingDiskFF_Disk_t driver — so FatFs and PlusFAT runs hit byte-for-byte the same host image.FFSemihostingDisk— FF_Disk_t driver modelled on Plus-FAT'sff_ramdisk.c, but mount-or-format-on-first-use (triesFF_Mountfirst so a power cycle keeps data; partitions +FF_Format(..., xPreferFAT16)only a fresh image).main.cinstalls the PlusFAT seam; CMakeLists drops all FatFs and compiles the 13 Plus-FAT core sources (relaxed OBJECT lib,-Wno-cppforff_fat.c's one-FAT-table#warning); new integratorFreeRTOSFATConfig.h;configNUM_THREAD_LOCAL_STORAGE_POINTERS = 3(ff_stdio TLS).FFSemihostingDisk.cstays in the strict executable with only-Wno-sign-conversion(Plus-FAT'sFF_ERR_*macros carry the high flag bit).ff_fflushdefect fix (approved scope expansion). The S29.04 adapter flushed viaff_fflush, which Plus-FAT declares inff_stdio.hbut never defines (PlusFatFakedefined it, so host tests passed while the real target failed to link). Switched toFF_FlushCache(Fp->pxIOManager), the real cache-flush primitive (dirent size still committed on Close); fake + the three durability tests updated. This is the one place the host fake masked a real-library gap.ff_stdioaccepts only absolute paths atffconfigHAS_CWD=0, so the shared store prefix became/STORE(valid for ChaN too).docs/integrating-plusfat.md; DEVLOG entry.Test Evidence
SolidSyslogPlusFatFileTest30/30 green after theff_fflush→FF_FlushCacheswap (fake spy + the three durability tests retargeted: flush-once-on-write, skip-on-incomplete-write, fail-on-flush-error).nmconfirmsFF_FlushCachepresent, noff_fflush, no FatFs symbols.@storesuite green: 5 features / 12 scenarios passed, 0 failed (store_and_forward,store_capacity,power_cycle_replay,capacity_threshold,block_lifecycle).power_cycle_replaypassing confirms flush-data + commit-size-on-close durability across a graceful restart./STOREprefix and sharedSemihostingDisk.misra_suppressions.txtunchanged (analysis lanes — tidy/iwyu/cppcheck — left to CI per the local-checks budget).Areas Affected
Bdd/Targets/Common/— newSemihostingDisk,FFSemihostingDisk,BddTargetFatFsMount,BddTargetPlusFatMount;diskio.c+ pipeline refactor.Bdd/Targets/FreeRtos/(Plus-TCP target) — FatFs→PlusFAT swap;Bdd/Targets/FreeRtosLwip/unchanged behaviour (FatFs via the new shim).Platform/PlusFat/Source/(Tier 2) — one-line durability fix (FF_FlushCache);Platform/FatFs/untouched. Core/Tier-1 untouched.Tests/— PlusFatFake + PlusFat durability tests. Docs.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores