S28.05: SolidSyslogLwipRawTcpStream#466
Conversation
Lays the three-TU pool scaffolding for the upcoming SolidSyslogLwipRawTcpStream adapter — Interface header, Errors header, Private header, .c (Initialise/Cleanup only — vtable methods land in commits 2/3 alongside Open/Send/Read), Messages.c (ErrorSource), Static.c (pool + Create/Destroy with the ConfigLock-wrapped slot walks). Adds three new tunables (SOLIDSYSLOG_LWIP_RAW_TCP_STREAM_POOL_SIZE default 2, SOLIDSYSLOG_LWIP_RAW_TCP_CONNECT_POLL_MS default 10, SOLIDSYSLOG_LWIP_RAW_TCP_RX_QUEUE_SIZE default 8). Bad-config (NULL config or NULL Sleep) resolves silently to the shared SolidSyslogNullStream — matches the sibling-backend contract; only POOL_EXHAUSTED and UNKNOWN_DESTROY are reported. 13 tests: CreateReturnsNonNullStream, CreatedStreamIsNotTheNullStreamSingleton, DestroyReleasesSlotToPool, CreateWithNullConfigReturnsFallback, CreateWithNullSleepReturnsFallback, plus the 9-test pool block mirroring SolidSyslogPlusTcpTcpStream's shape. Coverage: 100% line on TcpStream.c + TcpStreamStatic.c; Messages.c exercised only via integrator-installed error handlers (matches S28.04 Datagram). Part of #465. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… bridge
Wires Open / Close against lwIP Raw's tcp_new / tcp_arg / tcp_recv /
tcp_sent / tcp_err / tcp_connect / tcp_close / tcp_abort. Bridges
the lwIP callback Raw TCP API to SolidSyslogStream's synchronous
Open(addr) contract via a bounded spin loop driven by the
integrator-injected SolidSyslogSleepFunction — each iteration sleeps
SOLIDSYSLOG_LWIP_RAW_TCP_CONNECT_POLL_MS so lwIP's timer / RX paths
keep getting cycles to advance the SYN/SYN-ACK exchange. Deadline
is the existing per-instance GetConnectTimeoutMs getter (S12.17
pattern), with a Null Object falling back to SOLIDSYSLOG_TCP_CONNECT_TIMEOUT_MS.
Encapsulates lwIP's tcp_close-after-tcp_err gotcha: the wrapper's
ErrCallback nulls self->Pcb when lwIP releases the pcb upstream,
and Close checks Pcb != NULL before calling tcp_close — integrators
never see the rule. Sets SOF_KEEPALIVE on every pcb (idle / intvl /
cnt come from the integrator's lwipopts.h — LWIP_TCP_KEEPALIVE=1
is the recommended-on setting, documented in S28.05 commit 4's
docs/integrating-lwip.md).
Send / Read / RX queue land in the next commit; for now Recv / Sent
callback slots hold no-op stubs (lwIP requires them set, but the
synchronous API surface doesn't exercise their behaviour yet).
LwipTcpFake.{h,c} added — captures registered callbacks so tests
can drive connected_cb / err_cb directly; default tcp_connect
behaviour synchronously fires connected_cb with ERR_OK so happy-path
Open tests don't loop. OutstandingPcbCount leak invariant pinned in
shared teardown.
26 new tests across two TEST_GROUP_BASE groups (created-only +
Open-already), covering: tcp_new fail, keepalive, callback
registration, immediate connect-error path, errored-callback path,
timeout-with-abort path, sleep-between-polls behaviour, runtime-tunable
deadline, idempotency, close-after-tcp_err, destroy-after-tcp_err.
Coverage: 100% line on TcpStream.c + TcpStreamStatic.c (35 tests
in the exe; full debug suite 17/17 green).
Part of #465.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires Send / Read against lwIP Raw's tcp_write / tcp_output / tcp_recved + a bounded pbuf ring drained by Stream_Read. Send takes the TCP_WRITE_FLAG_COPY path — lwIP copies the caller's buffer into its own pbufs before tcp_write returns, so the synchronous Stream_Send(buf, len) lifetime contract is honoured. ERR_MEM from tcp_output after a successful tcp_write is queued-not- failed (matches POSIX's "kernel accepted into the send buffer" semantics); anything else closes internally. RX queue is a fixed ring sized by SOLIDSYSLOG_LWIP_RAW_TCP_RX_QUEUE_SIZE (default 8). The tcp_recv callback enqueues incoming pbufs, returning ERR_MEM when full so lwIP retains the pbuf and replays the callback later (flow control). Read drains bytes from the head pbuf, calls tcp_recved(pcb, n) to ACK back to lwIP's receive window, and pbuf_free's the head when fully drained. Peer FIN (tcp_recv with p == NULL) sets Errored but the wrapper drains any already-queued bytes before signalling EOF — next Read with an empty queue returns -1 and ClosePcb's the connection internally per the Stream contract. tcp_err's Pcb-already-nulled state is handled by IsOpen() — Read returns -1 without a double-tcp_close. ClosePcb now drains the queue unconditionally before tcp_close (under the existing Pcb != NULL guard) — pbufs we accepted via tcp_recv are ours to free regardless of pcb state. 20 new tests across the Connected group: Send happy/fail/COPY-flag/ ERR_MEM-queued/ERR_CONN-closes, post-tcp_err Send-false. Read queue-empty/drain-head/recved-ACK/partial-then-finish/multi-pbuf- advance/peer-FIN-after-drain/post-tcp_err. RX backpressure when queue full. Destroy-drains-queue + Close-drains-queue leak invariants. Coverage: 100% line + function on TcpStream.c + TcpStreamStatic.c (55 tests in the exe; full debug suite 17/17 green). Part of #465. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, auto-balanced pushIncomingPbuf, CHECK_FORWARDED_PCB
Three test-side cleanups, no production code change:
1. sendBytes() / readBytes() on the shared TEST_BASE fixture wrap the
abstract Stream Send/Read against the existing sendBuffer/readBuffer
fields. Matches the Datagram precedent's sendBytes helper. Removes
~18 repetitions of SolidSyslogStream_{Send,Read}(stream, buffer, N).
2. pushIncomingPbuf now returns the err_t the wrapper's tcp_recv
callback returned and only bumps LwipPbufFake_NoteIncomingPbuf when
that return was ERR_OK. The semantic "if the wrapper accepted the
pbuf the test counts it; otherwise lwIP retains it and the count
stays put" is now encoded in one place, not duplicated in each
call site. RecvCallbackBackpressuresWhenQueueFull drops its inline
fabrication + manual drain, both now subsumed by the helper's
auto-balancing (queue drain runs in shared teardown via Destroy).
3. CHECK_FORWARDED_PCB(getter) macro replaces the recurring
POINTERS_EQUAL(LwipTcpFake_LastTcpNewReturned(), getter()) intent
("the lwIP call received our pcb"). Used across Open/Send/Read/
Close pcb-forwarding asserts.
Coverage unchanged: 100% line + function on TcpStream.c + Static.c.
All 55 tests still green; full debug suite 17/17 green.
Part of #465.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #465. Housekeeping bundle closing the S28.05 PR: - docs/integrating-lwip.md new file. Covers NO_SYS=1 vs NO_SYS=0, the tcpip_callback() threading rule, lwipopts.h expectations (LWIP_RAW / LWIP_UDP / LWIP_TCP / ARP_QUEUEING / LWIP_TCP_KEEPALIVE / TCP_MSS / PBUF_POOL_SIZE / MEMP_NUM_TCP_PCB), a bare-metal NO_SYS=1 wiring example, per-adapter design notes (pbuf-strategy split UDP REF / TCP COPY, synchronous-Open spin, RX queue, tcp_close-after-tcp_err encapsulation), and the SolidSyslog tunables relevant to lwIP integrators. - CLAUDE.md gains two rows for SolidSyslogLwipRawTcpStream.h and SolidSyslogLwipRawTcpStreamErrors.h, slotted next to the other TCP-stream rows. - Top-level CMakeLists.txt STATUS strings for SOLIDSYSLOG_FREERTOS_NET =LWIP and =BOTH update to "Address+Resolver+Datagram+TcpStream only; BDD arrives in S28.06". The comment block above (the deferred PR #464 nit per project_s28_04_pr464_deferred_cmake_comment) is brought into sync. - DEVLOG entry capturing the five locked-in design calls (TCP_WRITE_FLAG_COPY, spin-with-injected-sleep, tcp_close-after-tcp_err encapsulation, bounded RX queue, SOF_KEEPALIVE), the commit shape, the LwipTcpFake's default-fires-cb behaviour that simplifies happy-path tests, and the new tunables/error source. - Pre-PR three-step: - IWYU on touched files (clang-19, freertos-host preset). Two findings applied: LwipTcpFake.h drops transitive lwip/arch.h + lwip/err.h; TcpStream.c drops SolidSyslogStreamDefinition.h + lwip/ip_addr.h and adds the public SolidSyslogLwipRawTcpStream.h header. Test file adjusts include set. - clang-format -i on every touched .c/.h/.cpp. - cppcheck-misra surfaced eight findings on the new TcpStream code. Six were resolved by code refactor (READ_FAILED moved into Read, RxQueueCount > 0U, pointer-arith → array indexing in DrainHeadBytes, == true on the IndexIsValid predicate, plus a new LwipRawTcpStream_SelfFromArg helper that concentrates the three lwIP-callback void*-arg casts into a single named site). Two new suppressions added under D.002 — 11.3 on SelfFromBase and 11.5 on SelfFromArg — matching the sibling-backend precedent. - docs/misra-deviations.md D.002 gets a sub-section (c) explicitly documenting the third-party-callback void*-arg pattern that the suppression file has been treating as D.002 since S08.07's MbedTls landed. Closes a pre-existing audit-trail gap and gives my new SelfFromArg entry the documented backing it needs. - scripts/misra_renumber.py extended to include the Platform/LwipRaw tree (-IPlatform/LwipRaw/Interface + Platform/LwipRaw/Source/). Oversight from S28.03; the script was previously blind to LwipRaw findings, which made the AMBIGUOUS-noise output during this PR's pre-push checks impossible to interpret. Pre-existing AMBIGUOUS entries on other rules are unchanged — they're orthogonal stale- suppression noise. - S28-05-HANDOFF.md removed (planning artefact carried from the session start; its job is done now that #465 is closing). Coverage: 100% line + function on Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c and SolidSyslogLwipRawTcpStreamStatic.c (Messages.c stays at 0%, matching the S28.04 Datagram established pattern — exercised only via integrator-installed error handlers). Full debug suite 17/17 green; the 55 TcpStream tests are inside SolidSyslogLwipRawTcpStreamTest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 13 minutes and 25 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds SolidSyslogLwipRawTcpStream: public headers and tunables, core stream (bounded connect, tcp_write/tcp_output send semantics, bounded RX pbuf ring, guarded close), static pool factory, comprehensive host-side lwIP fakes and CppUTest suite, plus integration guide and MISRA/script updates. ChangeslwIP Raw TCP Stream Adapter
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 (✔️ 1344 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: 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 `@Tests/Lwip/SolidSyslogLwipRawTcpStreamTest.cpp`:
- Around line 34-59: Wrap the custom test macros CHECK_IS_FALLBACK,
CHECK_REPORTED, and CHECK_FORWARDED_PCB with the required NOLINTBEGIN/NOLINTEND
block so linter macro-usage checks are disabled for their definitions; locate
the macro definitions (the CHECK_IS_FALLBACK do { ... } while (0) block, the
CHECK_REPORTED do { ... } while (0) block, and the single-line
CHECK_FORWARDED_PCB) and add NOLINTBEGIN before each macro definition block and
the corresponding NOLINTEND after each block (or wrap all three consecutively
with one NOLINTBEGIN/NOLINTEND pair) while leaving the existing do { ... } while
(0) structure intact.
In `@Tests/Support/LwipFakes/Source/LwipTcpFake.c`:
- Line 355: Extract the composite condition "if ((tcpConnectError == ERR_OK) &&
connectCallbackFires && (connected != NULL))" into a self-documenting static
inline predicate, e.g., static inline bool should_fire_connect_callback(int
tcpConnectError, bool connectCallbackFires, void *connected), and replace the
original if expression with a call to that predicate
(should_fire_connect_callback(tcpConnectError, connectCallbackFires,
connected)); ensure the helper is declared static inline near the top of the C
file (or above the caller) and uses the same symbols tcpConnectError, ERR_OK,
connectCallbackFires, and connected so the call-site reads as the documented
intent.
🪄 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: ed73dec9-14fe-4ba4-8f40-d30bd85a7219
📒 Files selected for processing (20)
CLAUDE.mdCMakeLists.txtCore/Interface/SolidSyslogTunablesDefaults.hDEVLOG.mdPlatform/LwipRaw/Interface/SolidSyslogLwipRawTcpStream.hPlatform/LwipRaw/Interface/SolidSyslogLwipRawTcpStreamErrors.hPlatform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.cPlatform/LwipRaw/Source/SolidSyslogLwipRawTcpStreamMessages.cPlatform/LwipRaw/Source/SolidSyslogLwipRawTcpStreamPrivate.hPlatform/LwipRaw/Source/SolidSyslogLwipRawTcpStreamStatic.cTests/Lwip/CMakeLists.txtTests/Lwip/SolidSyslogLwipRawTcpStreamTest.cppTests/Support/LwipFakes/Interface/LwipPbufFake.hTests/Support/LwipFakes/Interface/LwipTcpFake.hTests/Support/LwipFakes/Source/LwipPbufFake.cTests/Support/LwipFakes/Source/LwipTcpFake.cdocs/integrating-lwip.mddocs/misra-deviations.mdmisra_suppressions.txtscripts/misra_renumber.py
Three review-feedback fixes bundled into one commit:
- IWYU: restore the `struct SolidSyslogAddress;` forward declaration
in `Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c`. The
earlier IWYU pass dropped it while adding `struct SolidSyslogStream;`
— I misread "should add" as "replace" instead of additive. CI's
IWYU lane caught it; local re-run on the freertos-host preset now
clean. Both forward decls are needed (Open takes Address, the
vtable methods take Stream).
- analyze-tidy-freertos-plustcp (5 errors on
Tests/Lwip/SolidSyslogLwipRawTcpStreamTest.cpp):
- `[[nodiscard]]` on `sendBytes` and `readBytes` per
modernize-use-nodiscard. Two test sites that didn't capture the
return get explicit `(void)` casts.
- `static` on `pushIncomingPbuf` / `pushPeerFin` / `pushTcpErr`
per readability-convert-member-functions-to-static — none
access `this`, all delegate to the LwipTcpFake/LwipPbufFake
free functions.
- `NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)` on the
one `const_cast<void*>(data)` inside `pushIncomingPbuf` — pbuf's
payload field is `void*`; callers pass string literals, so the
cast is structural, not a const-strip.
- CodeRabbit two inline comments, both valid:
- NOLINTBEGIN/NOLINTEND(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while)
wrapper around the three test macros (`CHECK_IS_FALLBACK`,
`CHECK_REPORTED`, `CHECK_FORWARDED_PCB`). Matches the established
project convention in DEVLOG ("wrap with NOLINTBEGIN and use
do { ... } while (0) for safe single-statement use"). I forgot
the wrapper.
- Extract the composite condition
`(tcpConnectError == ERR_OK) && connectCallbackFires && (connected != NULL)`
in `tcp_connect` (LwipTcpFake.c) into a `ShouldFireConnectCallback`
static-inline predicate. Matches the project's "intent-naming
static-inline predicates" pattern.
Tests: 55/55 TcpStream tests green; full debug suite 17/17 green;
clang-tidy now clean on the freertos-plustcp preset locally.
Part of #465.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Actionable comments posted: 0 |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Two CI follow-ups on top of 5d579c2: - analyze-format: clang-format in the cpputest CI container reformatted `CHECK_FORWARDED_PCB` from a one-line `#define` onto two lines with a line-continuation backslash. My local clang-format-19 thought the one-liner was fine; the cpputest image's version disagrees. Apply the two-line form by hand — matches the visual style of the other CHECK_* macros above. - analyze-cppcheck: the IWYU fwd-decl I added in 5d579c2 shifted the SelfFromBase + SelfFromArg lines down by one. Bump the matching `misra_suppressions.txt` entries (11.3 :110→:111, 11.5 :119→:120) to match. Tests still 55/55 green; cppcheck-misra clean on `Platform/LwipRaw/Source/` locally. Part of #465. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Actionable comments posted: 0 |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Workaround for a divergent clang-format binary between containers: my freertos-host devcontainer (cpputest-freertos image) ships clang-format-19 and is happy with both the one-line and two-line forms of the macro; CI's analyze-format runs in the cpputest image and ships a different clang-format that wants a different alignment (first try: failed at col 95 on the one-liner; second try: failed at col 36 on the backslash position of the two-line form). Bracket the macro with `// clang-format off` / `// clang-format on` markers so neither version reformats it. The macro is a single expression — formatting it manually is fine. Follow-up to investigate: align the clang-format binaries across cpputest / cpputest-clang / cpputest-freertos images so dev and CI agree, or run analyze-format inside cpputest-freertos so the lwIP-touching files don't see a tooling mismatch. Part of #465. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Summary
tcp_new/tcp_connect/tcp_write/tcp_output/tcp_recv/tcp_recved/tcp_close/tcp_abort) to SolidSyslog's synchronousStream_Open/Send/Read/Closecontract via a bounded spin-with-injected-sleep Open loop and a bounded RX pbuf queue drainingtcp_recvintoStream_Read.docs/integrating-lwip.mdintegrator guide coveringNO_SYS=1vsNO_SYS=0, thetcpip_callback()threading rule,lwipopts.hexpectations, and per-adapter design notes (deferred since S28.03).docs/misra-deviations.mdD.002 gains a sub-section (c) documenting the third-party-callbackvoid*arg pattern — closes a pre-existing audit-trail gap that S08.07 mbedTLS already leaned on.Design decisions (locked in upfront in #465)
tcp_writeusesTCP_WRITE_FLAG_COPY— diverges from S28.04 Datagram's PBUF_REF because TCP defers transmission and the synchronousStream_Sendlifetime can't honour zero-copySolidSyslogSleepFunctionticks lwIP timers/RX underNO_SYS=1, yields underNO_SYS=0. Semaphore-wakeup ruled out (doesn't coverNO_SYS=1single-thread case)tcp_close-after-tcp_errgotcha encapsulated — wrapper nullsPcbin the err callback;ClosePcbchecksPcb != NULLbeforetcp_close. Integrators never see the ruleSOLIDSYSLOG_LWIP_RAW_TCP_RX_QUEUE_SIZEdefault 8 (sized for typical mTLS handshake flight). Backpressure via non-ERR_OKfrom the recv callback when full; lwIP retains the pbuf and replaysSOF_KEEPALIVEset on every pcb in Open — matchesSolidSyslogPosixTcpStream. lwipopts.h sets the idle/intvl/cnt valuesTest plan
SolidSyslogLwipRawTcpStreamTestgreen (debug + coverage presets)Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c(155 lines, 29 fns) andSolidSyslogLwipRawTcpStreamStatic.c(29 lines, 5 fns)Platform/LwipRaw/Source/(two new suppressions under D.002 — 11.3 SelfFromBase + 11.5 SelfFromArg)Commit shape
chore: S28.05 LwipRawTcpStream pool plumbing + tunables + tests— Three-TU split, three new tunables (POOL_SIZE=2,CONNECT_POLL_MS=10,RX_QUEUE_SIZE=8), 13 testsfeat: S28.05 LwipRawTcpStream Open/Close lifecycle + connect callback bridge—LwipTcpFake+ the four callback bridge + the bounded synchronous-Open spin. 22 new testsfeat: S28.05 LwipRawTcpStream Send/Read + RX queue + tcp_err handling—tcp_writeCOPY-path Send, RX-ring Read withtcp_recvedACK, peer-FIN-drain-before-EOF, RX-queue drain on Close/Destroy. 20 new testsrefactor: S28.05 LwipRawTcpStream tests — sendBytes/readBytes helpers, auto-balanced pushIncomingPbuf, CHECK_FORWARDED_PCB— Three test cleanups suggested mid-reviewfeat: S28.05 SolidSyslogLwipRawTcpStream + integrating-lwip.md— Housekeeping bundle: integrator guide, CLAUDE.md rows, CMake STATUS strings, DEVLOG, MISRA fixes/suppressions,scripts/misra_renumber.pyextended to scan LwipRaw, D.002 (c) sub-section,S28-05-HANDOFF.mddeletedBreaking changes
None — entirely new files behind two new public headers (
SolidSyslogLwipRawTcpStream.h+SolidSyslogLwipRawTcpStreamErrors.h).Closes #465.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tunable Defaults
Documentation
Tests