refactor: S29.03 extract shared FreeRTOS BDD-target pipeline#521
Conversation
The DRY pass S29.01 deferred. The two FreeRTOS target main.c files
(Plus-TCP + lwIP) shared ~600 lines of near-verbatim code: the store /
security-policy lifecycle, the OnSet handler, the SD set, the Service
drain task, the console glue, ErrorHandlerEx. Extract all of it into
Bdd/Targets/Common/BddTargetFreeRtosPipeline.{c,h}; each main.c now keeps
only its network backend behind an injection seam.
The seam (BddTargetFreeRtosPipelineConfig): DefaultHost, a BuildSender
thunk (platform network bring-up + Switching sender, runs on the
interactive task so lwIP's adapters touch a started core), a GetHostname
callback, and a TeardownNetwork thunk. The pipeline owns the SolidSyslog
lifecycle / store / SD / both tasks and exports GetEndpoint /
GetEndpointVersion / Sleep / Exit / InitConsole. The Service task
self-registers its handle.
FreeRtos/main.c 1258 -> 291, FreeRtosLwip/main.c 1111 -> 320; the shared
surface (956 lines incl. header) now exists once (net -800 lines).
Pure refactor, no behaviour change. Both targets cross-build clean on
sha-0b93766; lwIP ELF has zero Platform/PlusTcp symbols; clang-format
clean; full BDD suite green on both targets (44/44 each: UDP/TCP/TLS/mTLS
+ the full @store suite incl. @hmac/@aesgcm/capacity/power-cycle).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 34 minutes and 3 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 (2)
📝 WalkthroughWalkthroughThis PR extracts 600+ lines of duplicated FreeRTOS BDD-target logic from two ChangesShared FreeRTOS Pipeline Extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 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 `@Bdd/Targets/Common/BddTargetFreeRtosPipeline.c`:
- Around line 363-387: The code currently uses pendingSecurityPolicy (modified
by OnSet("security-policy")) when destroying policies, which can change after
CreateSecurityPolicy() built the actual policy; modify CreateSecurityPolicy() to
record the immutable active policy kind into a new variable (e.g.,
installedSecurityPolicy or activeSecurityPolicy) at creation time, and update
DestroySecurityPolicy() to consult that installedSecurityPolicy instead of
pendingSecurityPolicy so teardown/destroy dispatches the correct branch; ensure
OnSet still updates pendingSecurityPolicy for future creates but does not affect
the installed/active value used for destruction.
In `@Bdd/Targets/FreeRtos/main.c`:
- Around line 152-160: The code sets interactiveTaskCreated = pdTRUE
unconditionally even if xTaskCreate fails; change it to capture and check
xTaskCreate's return (BaseType_t), only set interactiveTaskCreated = pdTRUE when
xTaskCreate returns pdPASS, and on failure leave interactiveTaskCreated false
(and optionally log/error) so the next eNetworkUp will retry creating
BddTargetFreeRtosPipeline_ServiceTask; update the block that calls
xTaskCreate(BddTargetFreeRtosPipeline_ServiceTask, ...) to perform this
conditional check.
🪄 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: eb5dad38-199d-4723-9bb9-cf90b078a428
📒 Files selected for processing (7)
Bdd/Targets/Common/BddTargetFreeRtosPipeline.cBdd/Targets/Common/BddTargetFreeRtosPipeline.hBdd/Targets/FreeRtos/CMakeLists.txtBdd/Targets/FreeRtos/main.cBdd/Targets/FreeRtosLwip/CMakeLists.txtBdd/Targets/FreeRtosLwip/main.cDEVLOG.md
…e latch Two pre-existing latent bugs the refactor surfaced (carried verbatim from the old Plus-TCP main.c): - DestroySecurityPolicy dispatched on pendingSecurityPolicy, which a later `set security-policy` can change after the store (and its policy) were built — a subsequent rebuild/teardown could then run the wrong destroy path. Latch the kind into installedSecurityPolicy at CreateSecurityPolicy time and destroy against that. Behaviour-preserving for all current BDD flows (the harness sets security-policy before store file). - The Plus-TCP eNetworkUp hook latched interactiveTaskCreated even if the Service task's xTaskCreate failed, stranding a half-started pipeline with no retry. Only latch when both succeed; on Service failure delete the interactive task (safe — the hook runs on the higher-priority IP task, so the idle+1 interactive task has not been scheduled yet) so the next eNetworkUp retries cleanly. Both targets cross-build clean; @store suite (all security policies + teardown) 12/12 green on Plus-TCP. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1428 passed) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
☀️ 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
Closes #520. The DRY pass S29.01 deferred. The two FreeRTOS target
main.cfiles (Plus-TCP + lwIP) had drifted to ~600 lines of near-verbatim shared code. Extract it so future changes (e.g. S29.05's Plus-FAT swap) are made once, not twice-in-sync. Pure refactor — no behaviour change.Change Description
New
Bdd/Targets/Common/BddTargetFreeRtosPipeline.{c,h}owns everything platform-independent: the SolidSyslog lifecycle, the FatFs-backed store + security-policy machinery (crc16 / hmac-sha256 / aes-256-gcm / null), the SD set, the interactivesethandler, the CircularBuffer + Service drain task,ErrorHandlerEx, and the console glue.Each
main.ckeeps only its network backend behind the injection seamBddTargetFreeRtosPipelineConfig:DefaultHost—"10.0.2.2"(Plus-TCP, no DNS) /"syslog-ng"(lwIP, DNS alias);BuildSender— a thunk that brings up the platform network and returns the Switching sender, default transport selected. Runs on the interactive task so lwIP's adapters touch a started lwIP core (netif bring-up + ARP warm-up moved inside the seam);GetHostname— reads the platform IP stack;TeardownNetwork— releases the sender + adapters.The pipeline owns both FreeRTOS tasks and exports
GetEndpoint/GetEndpointVersion(the sender configs in eachmain.cwire these — they read the shared host/port), plusSleep/Exit/InitConsole. The Service task self-registers its handle, so neither task needs the caller to plumb anything.FreeRtos/main.c1258 → 291,FreeRtosLwip/main.c1111 → 320; the shared surface (956 lines incl. header) now exists once. Net −800 lines.Test Evidence
sha-0b93766image (zero warnings under the strict bar).Platform/PlusTcpsymbols (arm-none-eabi-nm).@storesuite: store_and_forward, store_capacity ×4, power_cycle_replay ×3 incl.@hmac/@aesgcm, capacity_threshold ×2, block_lifecycle ×2).Areas Affected
Bdd/Targets/): new shared pipeline TU + bothmain.cslimmed + both target CMakeLists. No library / Tier-1/2 source, no behaviour change.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests