feat: S29.04 Platform/PlusFat adapter pack + 100% host TDD#524
Merged
Conversation
Stand up Platform/PlusFat/ (INTERFACE lib, gated on FREERTOS_PLUS_FAT_PATH) and the Tests/PlusFat host harness. The pool plumbing (Static.c) and its 9-test suite are copied + renamed verbatim from the FatFs sibling — proven, agreed coverage. The class seam (Initialise/Cleanup) is a minimal stub; the ff_* behaviour is driven by TDD from here. PlusFatFakes carries the test-local FreeRTOSFATConfig.h; the shared FreeRtosFakes FreeRTOSConfig.h gains configNUM_THREAD_LOCAL_STORAGE_POINTERS (Plus-FAT's ff_stdio requires >= 3). Plus-FAT is FreeRTOS-coupled and the test include path surfaces that rather than hiding it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drive the SolidSyslogFile lifecycle ops on the PlusFat adapter via TDD: Open tries ff_fopen "r+" then falls back to "w+" (open-or-create with no truncation, since Plus-FAT has no open-always mode); Close calls ff_fclose guarded on an open handle; IsOpen and the open-state both derive from the FF_FILE* sentinel (no separate bool); Cleanup closes any open handle before the NullFile overwrite. PlusFatFake grows ff_fopen (with mode-fail injection and mode/path spies) and ff_fclose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Read maps to ff_fread(buf, 1, count, fp) and succeeds only when the returned item count equals the requested count — a short read is the single failure mode (Plus-FAT's ff_fread has no separate status return). PlusFatFake grows ff_fread with source injection and size/items/call-count spies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Write maps to ff_fwrite(buf, 1, count, fp) and, only on a complete write, ff_fflush — so a power loss never loses a record the BlockStore was told had been stored. Fails on a short write (skipping the flush) or a flush error. PlusFatFake grows ff_fwrite (data/items spies + incomplete-write injection) and ff_fflush (call-count spy + fail injection). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SeekTo maps to ff_fseek(fp, offset, SEEK_SET); Size returns ff_filelength. PlusFatFake grows ff_fseek (offset/whence spies) and ff_filelength (length injection). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Truncate empties the file via ff_fseek(fp, 0, SEEK_SET) then ff_seteof — Plus-FAT has no truncate-to-zero call of its own. PlusFatFake grows ff_seteof. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Exists maps to ff_stat (true when it returns 0); Delete maps to ff_remove (true when it returns 0). Both ignore the file handle and act on the path. PlusFatFake grows ff_stat and ff_remove with path spies and fail injection. This completes the SolidSyslogFile vtable for the PlusFat adapter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Platform/PlusFat to the cppcheck / cppcheck-misra command lists in ci.yml
and scripts/misra_renumber.py (kept in lockstep); the freertos tidy/iwyu lanes
pick the pack up automatically via the configure. Suppress the one vtable
downcast under deviation D.002. Apply clang-format and clang-tidy fixes to the
new pack and fake (ff_filelength param name, openModes pointer-array init).
Add the SolidSyslogPlusFatFile{,Errors}.h rows to the CLAUDE.md header table
and the S29.04 DEVLOG entry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR implements the FreeRTOS-Plus-FAT file adapter pack (S29.04) for SolidSyslog. It adds a complete SolidSyslogFile implementation via ff_stdio, a host-side fake with full call tracking and failure injection, comprehensive behavioral and pool tests with 100% host line+branch coverage, and integrates the new adapter into CMake and CI lanes without modifying core or existing adapters. ChangesFreeRTOS-Plus-FAT Adapter & Testing
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 docstrings
🧪 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 |
Contributor
☀️ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Tests/PlusFat/SolidSyslogPlusFatFilePoolTest.cpp (2)
79-91: 💤 Low valueConsider using the CHECK_REPORTED macro for ErrorHandlerFake assertions.
For consistency with the established pattern, lines 87-90 could be replaced with:
CHECK_REPORTED(SOLIDSYSLOG_SEVERITY_ERROR, &PlusFatFileErrorSource, SOLIDSYSLOG_CAT_POOL_EXHAUSTED | PLUSFATFILE_ERROR_POOL_EXHAUSTED);However, this assumes the macro accepts the combined category|detail pattern. If not, the current explicit assertions are fine. Based on learnings,
CHECK_REPORTED(severity, source, code)wraps the exact pattern used here.🤖 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 `@Tests/PlusFat/SolidSyslogPlusFatFilePoolTest.cpp` around lines 79 - 91, The test SolidSyslogPlusFatFilePoolTest::ExhaustedCreateReportsError uses individual ErrorHandlerFake assertions; replace the four explicit checks after overflow = SolidSyslogPlusFatFile_Create() (the CALLED_FAKE, LONGS_EQUAL, POINTERS_EQUAL, UNSIGNED_LONGS_EQUAL calls) with a single CHECK_REPORTED(...) invocation referencing SOLIDSYSLOG_SEVERITY_ERROR, &PlusFatFileErrorSource and the combined code (SOLIDSYSLOG_CAT_POOL_EXHAUSTED | PLUSFATFILE_ERROR_POOL_EXHAUSTED); ensure the CHECK_REPORTED macro is used consistently and accepts the combined category|detail bit pattern before removing the original assertions.
145-173: 💤 Low valueConsider using the CHECK_REPORTED macro for ErrorHandlerFake assertions.
Lines 152-156 and 168-172 manually assert ErrorHandlerFake fields. For consistency with the established pattern, these could use the
CHECK_REPORTED(severity, source, code)macro. The current explicit assertions are correct but the macro provides a more concise, canonical form.🤖 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 `@Tests/PlusFat/SolidSyslogPlusFatFilePoolTest.cpp` around lines 145 - 173, Replace the manual ErrorHandlerFake assertions in the two tests (SolidSyslogPlusFatFilePoolTest::DestroyOfUnknownHandleReportsWarning and ::DestroyOfStaleHandleReportsWarning) with the canonical CHECK_REPORTED macro: after calling SolidSyslogPlusFatFile_Destroy and asserting CALLED_FAKE(ErrorHandlerFake_Handle, ONCE), remove the LONGS_EQUAL/POINTERS_EQUAL/UNSIGNED_LONGS_EQUAL calls that check ErrorHandlerFake_LastSeverity(), ErrorHandlerFake_LastSource(), and ErrorHandlerFake_LastCategory()/LastDetail() and instead invoke CHECK_REPORTED(SOLIDSYSLOG_SEVERITY_WARNING, &PlusFatFileErrorSource, PLUSFATFILE_ERROR_UNKNOWN_DESTROY) to assert severity, source and code in one concise call.
🤖 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 `@Tests/PlusFat/SolidSyslogPlusFatFilePoolTest.cpp`:
- Around line 79-91: The test
SolidSyslogPlusFatFilePoolTest::ExhaustedCreateReportsError uses individual
ErrorHandlerFake assertions; replace the four explicit checks after overflow =
SolidSyslogPlusFatFile_Create() (the CALLED_FAKE, LONGS_EQUAL, POINTERS_EQUAL,
UNSIGNED_LONGS_EQUAL calls) with a single CHECK_REPORTED(...) invocation
referencing SOLIDSYSLOG_SEVERITY_ERROR, &PlusFatFileErrorSource and the combined
code (SOLIDSYSLOG_CAT_POOL_EXHAUSTED | PLUSFATFILE_ERROR_POOL_EXHAUSTED); ensure
the CHECK_REPORTED macro is used consistently and accepts the combined
category|detail bit pattern before removing the original assertions.
- Around line 145-173: Replace the manual ErrorHandlerFake assertions in the two
tests (SolidSyslogPlusFatFilePoolTest::DestroyOfUnknownHandleReportsWarning and
::DestroyOfStaleHandleReportsWarning) with the canonical CHECK_REPORTED macro:
after calling SolidSyslogPlusFatFile_Destroy and asserting
CALLED_FAKE(ErrorHandlerFake_Handle, ONCE), remove the
LONGS_EQUAL/POINTERS_EQUAL/UNSIGNED_LONGS_EQUAL calls that check
ErrorHandlerFake_LastSeverity(), ErrorHandlerFake_LastSource(), and
ErrorHandlerFake_LastCategory()/LastDetail() and instead invoke
CHECK_REPORTED(SOLIDSYSLOG_SEVERITY_WARNING, &PlusFatFileErrorSource,
PLUSFATFILE_ERROR_UNKNOWN_DESTROY) to assert severity, source and code in one
concise call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7de7f79-e0ab-4947-b653-134e8c9971d9
📒 Files selected for processing (22)
.github/workflows/ci.ymlCLAUDE.mdCMakeLists.txtDEVLOG.mdPlatform/PlusFat/CMakeLists.txtPlatform/PlusFat/Interface/SolidSyslogPlusFatFile.hPlatform/PlusFat/Interface/SolidSyslogPlusFatFileErrors.hPlatform/PlusFat/Source/SolidSyslogPlusFatFile.cPlatform/PlusFat/Source/SolidSyslogPlusFatFilePrivate.hPlatform/PlusFat/Source/SolidSyslogPlusFatFileStatic.cTests/CMakeLists.txtTests/PlusFat/CMakeLists.txtTests/PlusFat/SolidSyslogPlusFatFilePoolTest.cppTests/PlusFat/SolidSyslogPlusFatFileTest.cppTests/PlusFat/main.cppTests/Support/FreeRtosFakes/Interface/FreeRTOSConfig.hTests/Support/PlusFatFakes/CMakeLists.txtTests/Support/PlusFatFakes/Interface/FreeRTOSFATConfig.hTests/Support/PlusFatFakes/Interface/PlusFatFake.hTests/Support/PlusFatFakes/Source/PlusFatFake.cmisra_suppressions.txtscripts/misra_renumber.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Closes #522 (S29.04, epic #512 / E29). Add a FreeRTOS-Plus-FAT
SolidSyslogFileadapter so the PlusTCP target can later run a Plus-FAT-backed store (S29.05),
proving the
SolidSyslogFileseam is FS-vendor-portable the same way E28 provedthe transport seam is TCP-vendor-portable. Host-tested pack only — media driver,
target swap,
@storeBDD, anddocs/integrating-plusfat.mdare S29.05.Change Description
New
Platform/PlusFat/pack mirroring the FatFs sibling, adapted to theff_stdioAPI. 3-TU split (*.c/*Private.h/*Static.c) + anErrorsheader — no Messages TU (the library is codes-only since S12.26; the epic
text predates that and has been corrected).
Key Plus-FAT vs FatFs differences:
ff_fopen(path, "r+")then falls back to"w+"—open-or-create without truncating existing data (Plus-FAT has no open-always
mode).
FF_FILE*sentinel is the open-state — no separateIsOpenbool(FatFs needs one because its
FILis embedded).ff_fread/ff_fwritewithxSize == 1so the returneditem count is the byte count; a short transfer is the single failure mode.
ff_fflushafter every complete write (durability — a powerloss never loses a record the BlockStore was told had been stored).
ff_fseek(0)+ff_seteof; Size =ff_filelength;Exists/Delete =
ff_stat/ff_remove.Plus-FAT is FreeRTOS-coupled (
ff_stdio.h→ff_headers.hpullsFreeRTOS.h/task.h/semphr.h). We surface that in the test harness rather than hide it:PlusFatFakescarries a test-localFreeRTOSFATConfig.h, the sharedFreeRtosFakes/FreeRTOSConfig.hgainsconfigNUM_THREAD_LOCAL_STORAGE_POINTERS(Plus-FAT keeps errno/CWD/ff_error in TLS slots), and the test include path adds
the kernel + Plus-FAT dirs.
The pool plumbing (
Static.c) and its 9-test suite are copied + renamed verbatimfrom FatFs — proven, agreed coverage. Every
ff_*op was driven by genuinered→green TDD.
Test Evidence
Tests/PlusFat/SolidSyslogPlusFatFileTest— 30 tests / 70 checks, 100%line+branch by construction:
config-lock acquire/probe/destroy, unknown- & stale-destroy warnings) — copied.
caught (IsOpen-false and Write-returns-true both pass for free through the
NullFilefallback) and replaced with reds that assert the call reached thefake.
Verified in the cpputest-freertos image: full host suite 22/22 green;
clang-tidy clean; cppcheck clean; cppcheck-misra exit 0 (one D.002 vtable-downcast
suppression added, mirroring every other adapter).
ff_seteof/ff_stat/ff_removeconfirmed present in the pinned Plus-FAT SHA8d38036.Areas Affected
Platform/PlusFat/(INTERFACE lib, gated onFREERTOS_PLUS_FAT_PATH),Tests/PlusFat/,Tests/Support/PlusFatFakes/.Tests/Support/FreeRtosFakes/FreeRTOSConfig.h— additive TLS-pointer count(no effect on the other 21 host tests).
Platform/PlusFatadded to the cppcheck / cppcheck-misra command lists(ci.yml +
scripts/misra_renumber.py, kept in lockstep); the freertostidy/iwyu lanes pick it up via the configure.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores