feat: S11.08 FreeRTOS adapters onto PoolAllocator#411
Merged
Conversation
3-TU split (vtable+Initialise/Cleanup, Private.h struct, Static.c pool) behind unchanged `_Create(void)` / `_Destroy(base)` shape. Public `SolidSyslogFreeRtosMutexStorage` typedef and `SOLIDSYSLOG_FREE_RTOS_MUTEX_SIZE` enum deleted. Pool-exhaustion fallback resolves to the shared `SolidSyslogNullMutex`. `Initialise` guards `xSemaphoreCreateMutexStatic` returning NULL by installing the NullMutex vtable, mirroring PosixMutex. Caller updates: `Bdd/Targets/FreeRtos/main.c` drops `mutexStorage` and `lifecycleMutexStorage`; BDD target CMakeLists + Tests/FreeRtos CMakeLists add the new `*Static.c`; the Mutex test exe gains `ConfigLockFake` and `ErrorHandlerFake` links for the new Pool TEST_GROUP. Tunable `SOLIDSYSLOG_FREE_RTOS_MUTEX_POOL_SIZE` default 1U. Error constants `SOLIDSYSLOG_ERROR_MSG_FREERTOSMUTEX_POOL_EXHAUSTED` / `_UNKNOWN_DESTROY` follow the established naming. Refs #410.
3-TU split (vtable+Initialise/Cleanup, Private.h struct, Static.c pool) behind unchanged `_Create(void)` / `_Destroy(base)` shape. Public `SolidSyslogFreeRtosDatagramStorage` typedef and `SOLIDSYSLOG_FREE_RTOS_DATAGRAM_SIZE` enum deleted. Pool-exhaustion fallback resolves to the shared `SolidSyslogNullDatagram`. Cleanup calls Close (idempotent socket cleanup) before overwriting the vtable with NullDatagram for use-after-destroy safety. ARP-priming / single-yield pattern preserved verbatim — refactor-only. Caller updates: `Bdd/Targets/FreeRtos/main.c` drops `datagramStorage`; BDD target CMakeLists + Tests/FreeRtos CMakeLists add the new `*Static.c`; the Datagram test exe gains `ConfigLockFake` and `ErrorHandlerFake` links for the new Pool TEST_GROUP. Tunable `SOLIDSYSLOG_FREE_RTOS_DATAGRAM_POOL_SIZE` default 1U. Error constants `SOLIDSYSLOG_ERROR_MSG_FREERTOSDATAGRAM_POOL_EXHAUSTED` / `_UNKNOWN_DESTROY` follow the established naming. Refs #410.
3-TU split (vtable+Initialise/Cleanup, Private.h struct, Static.c pool). Public `_Create` keeps its `ipv4Octets[4]` argument — that is the runtime state the resolver pins per slot — but loses the `SolidSyslogFreeRtosStaticResolverStorage*` parameter; storage moves into the library-internal pool. `SolidSyslogFreeRtosStaticResolverStorage` typedef and `SOLIDSYSLOG_FREE_RTOS_STATIC_RESOLVER_SIZE` enum deleted. Pool-exhaustion fallback resolves to the shared `SolidSyslogNullResolver` (Resolve returns false so the caller's unresolved-host error path runs naturally). Caller updates: `Bdd/Targets/FreeRtos/main.c` drops `resolverStorage`; BDD target CMakeLists + Tests/FreeRtos CMakeLists add the new `*Static.c`; the StaticResolver test exe gains `ConfigLockFake` and `ErrorHandlerFake` links for the new Pool TEST_GROUP. The test's `RecreateResolverWith` helper now Destroy/Create-cycles a slot rather than re-stomping caller storage. The previous `DestroyIsIdempotent` test is superseded by `DestroyOfStaleHandleReportsWarning` in the Pool group — double-Destroy now emits a WARNING per the bad-setup contract. Tunable `SOLIDSYSLOG_FREE_RTOS_STATIC_RESOLVER_POOL_SIZE` default 1U. Refs #410.
3-TU split (vtable+Initialise/Cleanup, Private.h struct, Static.c pool) behind unchanged `_Create(void)` / `_Destroy(base)` shape. Public `SolidSyslogFreeRtosTcpStreamStorage` typedef and `SOLIDSYSLOG_FREE_RTOS_TCP_STREAM_SIZE` enum deleted. Pool-exhaustion fallback resolves to the shared `SolidSyslogNullStream`. Cleanup closes any open socket before overwriting the vtable with NullStream for use-after-destroy safety. Refactor-only — the bounded-200 ms SO_SNDTIMEO/SO_RCVTIMEO connect dance, the post-connect timeout clear, the ARP-priming yield-once on cache miss, and the FreeRTOS_recv RCVTIMEO=0 "would-block as zero" contract are all preserved verbatim. Static helper functions stay where they were. Caller updates: `Bdd/Targets/FreeRtos/main.c` drops `tcpStreamStorage`; BDD target CMakeLists + Tests/FreeRtos CMakeLists add the new `*Static.c`; the TcpStream test exe gains `ConfigLockFake` and `ErrorHandlerFake` links for the new Pool TEST_GROUP. Pool size default 2U — matches the POSIX/Windows TcpStream pool sizes so a future TLS-via-mbedTLS path (S08.07) wrapping an underlying FreeRtosTcpStream does not silently fall back to NullStream on the second Create. Tunable `SOLIDSYSLOG_FREE_RTOS_TCP_STREAM_POOL_SIZE`. Error constants `SOLIDSYSLOG_ERROR_MSG_FREERTOSTCPSTREAM_POOL_EXHAUSTED` / `_UNKNOWN_DESTROY` follow the established naming. Refs #410.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughFour FreeRTOS stateful adapter classes migrate from caller-provided storage to library-managed pool allocation. Each class gains a ChangesFreeRTOS Adapters onto PoolAllocator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1263 passed, 🙈 2 skipped) 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.
Actionable comments posted: 6
🤖 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 `@DEVLOG.md`:
- Around line 92-95: Fix the inconsistent test-count numbers in the DEVLOG
section by reconciling the "Per-class Pool TEST_GROUPs add 36 tests overall (9 ×
4 classes)" statement with the later "total addition ~37" and the referenced "8
tests added to existing Mutex group / 0 to Datagram"; update the Pool, Mutex,
Datagram and overall totals so they sum correctly and match the per-class
breakdown (adjust the "9 × 4", the "8 tests" in Mutex, and the "~37" total as
needed) and ensure the three places ("Per-class Pool TEST_GROUPs..." line, the
Mutex/Datagram line, and the overall total) all show the same consistent
numbers.
In `@Platform/FreeRtos/Source/SolidSyslogFreeRtosDatagramStatic.c`:
- Around line 42-50: The destroy path flags the shared null sentinel as
"unknown" because SolidSyslogFreeRtosDatagram_Destroy only tries to free from
FreeRtosDatagram_Allocator; update Destroy to first detect the shared null
datagram returned by SolidSyslogNullDatagram_Get() (which
SolidSyslogFreeRtosDatagram_Create can return) and treat it as a valid no-op
destroy (return or mark released true) so the code does not call
SolidSyslog_Error with SOLIDSYSLOG_ERROR_MSG_FREERTOSDATAGRAM_UNKNOWN_DESTROY;
keep the existing pool-free logic using SolidSyslogPoolAllocator_IndexIsValid
and SolidSyslogPoolAllocator_FreeIfInUse/FreeRtosDatagram_CleanupAtIndex for
non-sentinel instances.
In `@Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.c`:
- Around line 8-10: The translation unit uses the bool type directly (declared
around line 12) but does not include <stdbool.h>; add a direct `#include`
<stdbool.h> at the top of SolidSyslogFreeRtosStaticResolver.c (near the existing
includes of "SolidSyslogFreeRtosStaticResolverPrivate.h",
"SolidSyslogNullResolver.h", and "SolidSyslogResolverDefinition.h") so the
symbol bool is provided by the translation unit itself and satisfies the IWYU CI
requirement.
In `@Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolverStatic.c`:
- Around line 28-35: SolidSyslogFreeRtosStaticResolver_Create must validate the
ipv4Octets pointer before passing it into FreeRtosStaticResolver_Initialise; if
ipv4Octets is NULL, do not touch the pool or call Initialise—immediately return
the fallback from SolidSyslogNullResolver_Get(). Update the function to check
ipv4Octets at the top (alongside the existing allocator/index checks), and only
call FreeRtosStaticResolver_Initialise(&FreeRtosStaticResolver_Pool[index].Base,
ipv4Octets) when ipv4Octets is non-NULL and the acquired index is valid.
In `@Platform/FreeRtos/Source/SolidSyslogFreeRtosTcpStream.c`:
- Around line 13-15: This file uses bool, size_t, uint16_t and uint32_t without
including their standard headers; add explicit includes for <stdbool.h>,
<stddef.h> and <stdint.h> near the top of SolidSyslogFreeRtosTcpStreamPrivate.h
consumer (i.e., in this translation unit) — place these three includes after the
copyright/license header and before any FreeRTOS includes so symbols used by
functions in this file (e.g., any functions in SolidSyslogFreeRtosTcpStream.c
that reference bool/size_t/uint16_t/uint32_t or types used by
SolidSyslogFreeRtosTcpStreamPrivate.h) have explicit, non-transitive
declarations.
In `@Tests/FreeRtos/SolidSyslogFreeRtosMutexTest.cpp`:
- Around line 145-152: The test
SolidSyslogFreeRtosMutexPool::FallbackLockUnlockAreNoOps currently has no
assertions; add NEVER checks to ensure fallback mutex does not call semaphore
APIs. After calling FillPool() and creating overflow via
SolidSyslogFreeRtosMutex_Create(), add NEVER(FreeRtosSemaphore_Take) and
NEVER(FreeRtosSemaphore_Give) (or the project’s equivalent NEVER macros for
those functions) before/around calling SolidSyslogMutex_Lock(overflow) and
SolidSyslogMutex_Unlock(overflow) so the test fails if the fallback
implementation calls FreeRtosSemaphore_Take or FreeRtosSemaphore_Give.
🪄 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: 316171c0-2ac8-4c7c-8978-9528b14c1d28
📒 Files selected for processing (28)
Bdd/Targets/FreeRtos/CMakeLists.txtBdd/Targets/FreeRtos/main.cBdd/Targets/FreeRtos/solidsyslog_user_tunables.hCLAUDE.mdCore/Interface/SolidSyslogTunablesDefaults.hCore/Source/SolidSyslogErrorMessages.hDEVLOG.mdPlatform/FreeRtos/Interface/SolidSyslogFreeRtosDatagram.hPlatform/FreeRtos/Interface/SolidSyslogFreeRtosMutex.hPlatform/FreeRtos/Interface/SolidSyslogFreeRtosStaticResolver.hPlatform/FreeRtos/Interface/SolidSyslogFreeRtosTcpStream.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosDatagram.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosDatagramPrivate.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosDatagramStatic.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosMutex.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosMutexPrivate.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosMutexStatic.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolverPrivate.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolverStatic.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosTcpStream.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosTcpStreamPrivate.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosTcpStreamStatic.cTests/FreeRtos/CMakeLists.txtTests/FreeRtos/SolidSyslogFreeRtosDatagramTest.cppTests/FreeRtos/SolidSyslogFreeRtosMutexTest.cppTests/FreeRtos/SolidSyslogFreeRtosStaticResolverTest.cppTests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cpp
Three classes of polish in response to PR #411 review: 1. **Direct stdlib includes** on the four migrated FreeRTOS `.c` files (`FreeRtosMutex.c`, `FreeRtosDatagram.c`, `FreeRtosStaticResolver.c`, `FreeRtosTcpStream.c`). Each TU now `#include`s `<stdbool.h>`, `<stddef.h>`, and/or `<stdint.h>` directly for the standard types it uses, rather than relying on transitive includes via FreeRTOS headers. CI's `analyze-iwyu` job runs in the cpputest container without `FREERTOS_KERNEL_PATH`, so the FreeRTOS adapter sources are not covered by the IWYU gate — adding explicit includes anyway as the project pattern (matches the `*Static.c` siblings). 2. **Tighter fallback-no-op test assertions** on all four classes' pool tests. The `FallbackLockUnlockAreNoOps` / `FallbackVtableMethodsAreNoOps` tests now reset the relevant fake between FillPool and the fallback exercise and assert `NEVER` on every platform-API call the fallback path could touch (`SemaphoreTake/Give` for Mutex; `Sendto`/`Closesocket` for Datagram; `Connect`/`Closesocket` for TcpStream). The Mutex test previously had no assertions at all — passing even if the fallback secretly hit the kernel. 3. **DEVLOG test-count fix** — the test-counts paragraph claimed "36 overall (9 × 4)" and "~37" and "8 tests added to Mutex / 0 to Datagram" inconsistently. The actual numbers are: 37 Pool-group tests total (9 each for Mutex / Datagram / TcpStream; 10 for StaticResolver — 9 canonical + 1 class-specific), net +36 once the superseded `DestroyIsIdempotent` is netted out. Skipped CodeRabbit findings: - **`SolidSyslogFreeRtosDatagram_Destroy` treating `NullDatagram_Get()` as a no-op destroy target** — declined. The WARNING-on-fallback-Destroy is the established E11 pattern across every migrated class (Posix, Windows, Core); diverging only on FreeRTOS would break the contract uniformity. If the cross-cutting shape should change, that's an epic-wide story, not a S11.08 deviation. - **NULL-validation on `ipv4Octets` in `SolidSyslogFreeRtosStaticResolver_Create`** — declined per the project rule that validation only belongs at true system boundaries (user input, external APIs); `_Create` is integrator setup wiring. Pre-migration the function also crashed on NULL — no regression. CodeRabbit itself quoted this rule in its comment. Refs #410, PR #411.
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1263 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
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.
Closes #410.
Summary
Third platform-adapter sweep in E11 (#29) — applies the canonical 3-TU
split (
<Class>.c+<Class>Private.h+<Class>Static.c) +SolidSyslogPoolAllocatorto all four stateful FreeRTOS adapterclasses. Every FreeRTOS adapter is storage-cast today, so the
public-header surgery is uniform: drop
<Class>Storagetypedef + sizeenum, drop
storageparameter from_Create.FreeRtosStaticResolverkeeps its
ipv4Octets[4]parameter — that is its runtime state, notstorage.
No new public GoF nulls: all four needed (
NullMutex_Get,NullDatagram,NullStream,NullResolver) landed in S11.06 (#407).61514e4FreeRtosMutex1U0bcdb70FreeRtosDatagram1Ubbd6321FreeRtosStaticResolver1Ucda8e2eFreeRtosTcpStream2U(matches POSIX/Windows for S08.07 mbedTLS pair)3c8aabaFreeRtosTcpStreamis refactor-only — bounded-200 msSO_SNDTIMEO/SO_RCVTIMEOconnect dance, post-connect timeout clear,ARP-priming yield-once on cache miss, and
FreeRTOS_recvRCVTIMEO=0"would-block-as-zero" contract all preserved verbatim.
FreeRtosMutexadds a
xSemaphoreCreateMutexStaticNULL-guard mirroringPosixMutex's defence againstpthread_mutex_initfailure.Notable BDD-target fix surfaced by the migration
The BDD target creates two
FreeRtosMutexinstances (bufferMutexlifecycleMutex). The library defaultSOLIDSYSLOG_FREE_RTOS_MUTEX_POOL_SIZE = 1Uwas silently falling thesecond
_Createback toNullMutex— Lock/Unlock would no-op and theset store filerebuild path could race the Service task. Surfacedas a
severity=4…UNKNOWN_DESTROYwarning onquitduring theQEMU smoke. Fixed by adding the override in
Bdd/Targets/FreeRtos/solidsyslog_user_tunables.h(folded into theMutex commit). The library default stays at
1U— the BDD target isthe unusual case.
This is exactly the E11 correctness motivation: pre-migration both
mutexes "worked" because each had its own storage blob; the pool
semantics made the multi-instance need explicit.
Per-class test additions
FreeRtosMutex: 13 tests (4 existing + 9 Pool).FreeRtosDatagram: 30 tests (21 existing + 9 Pool).FreeRtosStaticResolver: 18 tests (9 existing − 1 supersededDestroyIsIdempotent+ 10 Pool).FreeRtosTcpStream: 46 tests (37 existing + 9 Pool).Local gates
Validated across three devcontainers:
sanitize pass; coverage 99.5% (pre-existing FreeRTOS gap in the lcov
capture, same as POSIX/Windows); analyze-cppcheck clean;
analyze-tidy clean on every file touched; analyze-format clean.
freertos-crossARM build clean (all four new*Static.cfiles compile under arm-none-eabi-gcc and link into theSolidSyslogBddTarget.elf); QEMUmps2-an385boot smoke clean(
SolidSyslog>prompt,send 1succeeds,quitexits clean, noemissions).
msvc-debugclean;analyze-iwyuclean(confirmed by the user before push).
CI will run the full BDD-FreeRTOS-QEMU scenarios.
Test plan
freertos-crossARM cross-build linksmsvc-debugclean (user-validated)analyze-iwyuclean (user-validated)bdd-freertos-qemu(full scenarios — includingset store file)bdd-linux-syslog-ng/bdd-windows-otel(regression net)Summary by CodeRabbit
New Features
Documentation