feat: port ring_buf (sys/ring_buffer.h) + pre-public copyright audit (#45)#50
Merged
Conversation
Near-verbatim port of upstream's ring_buffer.h + lib/utils/ring_buffer.c (both Apache-2.0), the canonical companion to the UART IRQ pattern and the byte transport the direct-UART shell (#31) and a modbus serial backend want. Pure arithmetic + memcpy; no kernel dependencies, nothing blocks. Byte-mode API: ring_buf_init, RING_BUF_DECLARE, reset, size/space/ capacity_get, is_empty, put/get/peek, and the zero-copy claim/finish pairs. Indices use upstream's base-offset scheme (head/tail/base of ring_buf_idx_t, byte offset = head-base with one wrap correction, RING_BUFFER_MAX_SIZE caps size to half the index range so unsigned subtraction disambiguates full vs empty). CONFIG_RING_BUFFER_LARGE widens indices to 32-bit, matching upstream. Adaptations for Boreas (documented in the files): - include layout (zephyr/sys/util.h) and lowercase upstream min() -> MIN. - toolchain shims added to sys/util.h: __ASSERT_NO_MSG, likely/unlikely, __noinit (mapped to zero-init storage -- correct here, and avoids __NOINIT_ATTR wrongly surviving deep sleep), __deprecated. Also made __ASSERT self-contained (#include <stdlib.h> for abort()). Deliberate divergence: the upstream item-mode API (ring_buf_item_put/ _get, RING_BUF_ITEM_DECLARE*) is NOT ported -- it is @deprecated upstream ("use <zephyr/sys/ringq.h>") and unused by the byte-transport consumers this targets. Concurrency is unchanged: lock-free for single-producer/single-consumer in separate contexts; multiple producers/consumers must serialize externally. Tests (15): accounting, round-trip, capacity saturation, partial get, NULL-discard, peek-no-consume, reset, claim/finish (put+get), surplus return, over-claim -EINVAL, and the wrap-sensitive cases -- claim splits at the physical end, put/get spanning the wrap, and a 20000-cycle stream through a 7-byte (non-power-of-two) buffer exercising base advancement and the index arithmetic across many wraps. linux suite: 221/0 x3 (206 + 15). Closes #45 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tests Review fan-out (fidelity + conformance + adversarial) findings folded: - Restore the public RING_BUF_INIT macro (upstream API completeness; RING_BUF_DECLARE is now defined in terms of it, matching upstream structure). A downstream `struct ring_buf rb = RING_BUF_INIT(...)` now compiles. - Retain upstream's "Copyright (c) 2015 Intel Corporation" line on the two verbatim files alongside the Intercreate port line (Apache-2.0 4(c) preservation for a substantial verbatim derivative; the repo is going public). - Document the __noinit limitation: Boreas wires no .noinit section, so no-init/retained-RAM semantics are not available (storage is plain zero-init BSS). - Tests: assert ring_buf_put_claim returns 0 on a full buffer (the zero-copy path UART IRQ uses), and assert ring_buf_space_get across a wrap (the put.head - get.tail subtraction path). Adversarial index-arithmetic probe found zero constructible failures: the UINT16_MAX/2 cap is exact and inclusive (size=32768 would alias head-base to 0 mod 65536 and is rejected at build/init), and the non-power-of-two-size x uint16-index-wrap case is safe because only modular differences are used and tail-base is held in [0,size) by one base bump per wrap. The 20000-cycle test crosses the uint16 boundary, so it covers that class. linux suite: 221/0 x3. Refs #45 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lic) A pre-public licensing audit compared every Zephyr-API file against the upstream original (function bodies and macro expansions, not just names). Apache-2.0 4(c) requires retaining the upstream copyright only where actual copyrightable expression was copied. Retain upstream copyright (genuine verbatim/near-verbatim content): - sys/util.h: IS_ENABLED's _XXXX##/_YYYY token-paste trick is upstream verbatim -> Copyright (c) 2011-2014 Wind River Systems, Inc. - sys/byteorder.h: the 32-bit be/le composition mirrors upstream's canonical chaining -> Copyright (c) 2015-2016 Intel Corporation. - (sys/ring_buffer.h + ring_buf.c already carry Copyright (c) 2015 Intel Corporation from the port commit.) Deliberately NOT attributed (independent reimplementations -- adding an upstream notice would be false attribution): sys/dlist.h, sys/slist.h (upstream is macro-generated; ours is hand-written over a different struct layout), sys/atomic.h (inline __atomic wrappers vs upstream's extern decls), sys/time_units.h (us-stored k_timeout_t + FreeRTOS ticks vs upstream's Hz-based z_tmcvt), and all of zshell/zsys/zdevice (the shell, logging, and device model are independent implementations over ESP-IDF/FreeRTOS; only the public API surface matches Zephyr). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Folded findings from the code-review fan-out (no correctness bugs were found; these are robustness/coverage/doc improvements): - ring_buffer.h + ring_buf.c now #include "sdkconfig.h" explicitly. struct ring_buf's index width is selected by CONFIG_RING_BUFFER_LARGE, so the config must be visible wherever the struct is defined. It was safe before only by a transitive chain (util.h -> esp_attr.h -> sdkconfig.h); making it explicit removes a silent cross-TU ABI hazard if that chain ever changes, and matches the sibling .c convention. - util.h: corrected the __noinit comment. The prior rationale was wrong (ESP-IDF DOES ship __NOINIT_ATTR + a .noinit section, and that attribute survives warm reset, not deep sleep). The real reasons to map __noinit to plain BSS: ring_buf never reads unwritten bytes (so no-init vs zero-init is invisible), and plain BSS avoids a section attribute that does not port to the Mach-O host toolchain. The no-init limitation is documented. - util.h: dropped the unused __deprecated shim (no users; add toolchain shims when a real user lands). likely/unlikely kept (unlikely is used by ring_buf.c; both are #ifndef-guarded and identical in value to ESP-IDF's, so the include-order race is codegen-only). - test: added test_ring_buf_full_empty_across_wrap -- fills to full / drains to empty for 40 cycles (163 KB through a 4 KB buffer, crossing the uint16 index wrap ~2.5x), asserting the full (space==0, put refused) and empty discriminations AT high `allocated` across the wrap. The existing many-wrap test drains within a 7-byte span, so it never exercised full-vs-empty near the index wrap. linux suite: 222/0 x3. Refs #45 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
util.h was accreting symbols whose upstream homes are <zephyr/toolchain.h> (likely/unlikely, __weak, ALWAYS_INLINE, __noinit) and <zephyr/sys/__assert.h> (__ASSERT, __ASSERT_NO_MSG), while the repo otherwise mirrors upstream include paths exactly -- the devlog TODO already records a downstream smf port hand-editing upstream #includes because of this. Thin headers at the upstream paths fix that now, while they have 1-2 consumers; util.h re-includes both so existing ports keep working. Also two correctness fixes folded in: - __noinit now maps to __NOINIT_ATTR (real .noinit on hardware, no-op on the linux target via esp_attr.h's CONFIG_IDF_TARGET_LINUX gate in both IDF v5.4 and v5.5). The old empty shim silently gave zero-init BSS semantics to any future warm-reset-retention user, and its stated Mach-O blocker was false -- the macOS host build of the linux target compiles clean. - likely/unlikely now document the divergence from esp_compiler.h (ESP-IDF's are CONFIG_COMPILER_OPTIMIZATION_PERF-gated; Boreas matches upstream Zephyr's unconditional __builtin_expect; both are #ifndef-guarded so the include-order race is codegen-only). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The header advertises the byte API for thread + ISR use (the UART IRQ pattern), and the repo convention is that ISR-callable primitives are K_ISR_SAFE (k_sem, k_msgq, k_event, k_work): ESP-IDF ISRs registered with ESP_INTR_FLAG_IRAM fire during flash-cache-disabled windows (e.g. NVS commits), where calling flash-resident code panics with "Cache disabled but cached memory region accessed". All five functions are now IRAM-resident (verified via nm: 0x4037xxxx alongside k_sem_give). The internal __ASSERT_NO_MSGs are replaced with k_panic() checks: the upstream asserts compile out under its default CONFIG_ASSERT=n, but Boreas's __ASSERT is always on and its ESP_LOGE+abort failure path is itself flash-resident (util.h documents it as not IRAM-safe and prescribes k_panic() -- k_work.c already set the precedent of replacing upstream asserts on K_ISR_SAFE paths). memcpy is ROM-resident on ESP32-S3, so the copy loops remain safe. ring_buf_peek's NULL check is also hoisted above the copy loop so the documented "Cannot be NULL" contract is enforced on an empty buffer too, instead of silently returning 0 until data arrives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…retvals Doc-only fixes from review (code unchanged, upstream-verbatim): - Concurrency note rewritten: it overclaimed "lock-free SPSC" without stating that no field is volatile and no barriers are issued. Now documents that the query inlines read one index from each side (tear-free aligned loads, conservatively stale), forbids busy-waiting on them (the compiler may hoist the load out of a call-free loop), and requires per-handoff k_sem pairing on SMP. Also notes the implementation is K_ISR_SAFE. - RING_BUF_INIT: @warning that size8 > RING_BUFFER_MAX_SIZE is NOT checked on this path (unlike RING_BUF_DECLARE/ring_buf_init) and silently corrupts; macro body stays upstream-verbatim. - put_claim/get_claim/peek: @notes that finishing rewinds head to tail, so an outstanding claim must be finished before any other same-side call (including the copy-mode functions); restored upstream's dropped "with a non-zero `size`" qualifier on the peek doc. - put_finish/get_finish: -EINVAL condition corrected -- the code (here and upstream) rejects size > outstanding claimed bytes, not "exceeds free space"/"valid bytes" as upstream's doc has always misstated. - size_get: adopted current upstream main's "available data" wording (v3.7's "used space" miscounts outstanding claims); three @retval-with-prose fixed to @return (also fixed upstream post-v3.7). - Removed seven @warnings referencing the unported ring_buf_item_ API; RING_BUF_DECLARE gains a file-scope-only @note. - Kconfig: RING_BUFFER_LARGE costs 12 extra bytes per struct ring_buf on the 32-bit target (6 index fields x 2 bytes; measured 20 -> 32), not 6 as the help text claimed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two coverage gaps closed: - test_ring_buf_index_wrap_seeded: the traffic-volume stress tests can only reach the default uint16_t 65536 wrap; under CONFIG_RING_BUFFER_LARGE the 2^32 wrap was silently untested in exactly the config the option exists for. Seeding the indices just below the wrap via ring_buf_internal_reset (the technique upstream's ringbuffer test suite uses) makes the full/empty discrimination coverage index-width-independent. Verified locally: full suite passes on the linux target with CONFIG_RING_BUFFER_LARGE=y. - test_ring_buf_peek_across_wrap_multi_claim_get: peek was only tested contiguous-at-offset-0; now covers peek walking both physical segments of wrapped data and rewinding via its internal get_finish(0), plus multiple get-side claims completed by a single finish. Cleanups: per-byte TEST_ASSERT_EQUAL_UINT8 loops (~300k Unity calls across the two stress tests) replaced with per-chunk TEST_ASSERT_EQUAL_UINT8_ARRAY (better failure diagnostics: reports the element index); put_claim_wraps now drains and verifies its patterned writes landed at the correct physical offsets instead of leaving them unchecked; NULL-discard priming replaces drain-into-out + memset; dead sequence fill dropped from the saturates test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
Review hardening pass (xhigh
|
There was a problem hiding this comment.
Pull request overview
Ports Zephyr’s byte-mode ring_buf implementation into Boreas’ zkernel (header + C implementation), adds supporting Zephyr-compat toolchain/assert shims, and introduces a focused Unity test suite to validate wrap/index edge cases.
Changes:
- Add Zephyr-compatible
ring_bufAPI (sys/ring_buffer.h) and implementation (src/ring_buf.c) with ISR-safe placement. - Introduce toolchain/assert compatibility headers and update util/byteorder headers for licensing/attribution adjustments.
- Add comprehensive ring buffer tests and wire them into the test runner/build.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
components/zkernel/include/boreas/zephyr/sys/ring_buffer.h |
Adds the Zephyr-compatible byte ring buffer public API and inline helpers. |
components/zkernel/src/ring_buf.c |
Implements the ring buffer core logic (claim/finish + copy put/get/peek). |
components/zkernel/Kconfig |
Adds RING_BUFFER_LARGE option to widen indices to 32-bit. |
components/zkernel/include/boreas/zephyr/toolchain.h |
Adds Zephyr toolchain macro shims (likely/unlikely, ALWAYS_INLINE, __noinit, etc.). |
components/zkernel/include/boreas/zephyr/sys/__assert.h |
Adds Zephyr-compatible __ASSERT / __ASSERT_NO_MSG macros. |
components/zkernel/include/boreas/zephyr/sys/util.h |
Re-exports new compat headers and updates attribution commentary. |
components/zkernel/include/boreas/zephyr/sys/byteorder.h |
Updates attribution commentary for upstream-mirrored logic. |
components/zkernel/CMakeLists.txt |
Builds the new src/ring_buf.c. |
test/main/test_ring_buf.c |
Adds ring buffer unit tests (accounting, wrap, claim/finish, seeded index wrap). |
test/main/test_main.c |
Registers the new ring buffer test group. |
test/main/CMakeLists.txt |
Adds test_ring_buf.c to the test sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e (Copilot) The file-level comment claimed all definitions are #ifndef-guarded, but ALWAYS_INLINE is intentionally #undef'd and redefined unconditionally (the RISC-V -Werror=attributes fix). Call out the exception so the include-order contract isn't misleading. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Summary
Ports Zephyr's
ring_buf(byte-mode) — the canonical companion to the UART IRQ pattern and the byte transport the direct-UART shell (#31) and a modbus serial backend want — and folds in a small pre-public licensing audit that the port's attribution discussion surfaced.ring_buf port
Near-verbatim port of upstream
include/zephyr/sys/ring_buffer.h+lib/utils/ring_buffer.c(Apache-2.0). Pure arithmetic + memcpy; no kernel dependencies, nothing blocks.ring_buf_init,RING_BUF_INIT,RING_BUF_DECLARE,reset,size/space/capacity_get,is_empty,put/get/peek, and the zero-copy*_claim/*_finishpairs.ring_buf_idx_t; byte offset =head - basewith one wrap correction;RING_BUFFER_MAX_SIZEcaps size to half the index range so unsigned subtraction disambiguates full vs empty).CONFIG_RING_BUFFER_LARGEwidens indices to 32-bit, matching upstream.min()→MIN; new toolchain shims insys/util.h(__ASSERT_NO_MSG,likely/unlikely,__noinit→ zero-init storage with a documented limitation note,__deprecated);__ASSERTmade self-contained (#include <stdlib.h>).ring_buf_item_put/_get,RING_BUF_ITEM_DECLARE*) is not ported — it's@deprecatedupstream ("use<zephyr/sys/ringq.h>") and unused by the byte-transport consumers this targets.15 tests: accounting, round-trip, capacity saturation (incl.
put_claimreturning 0 when full), partial get, NULL-discard, peek-no-consume, reset, claim/finish (put+get), surplus return, over-claim-EINVAL,space_getacross a wrap, and the wrap-sensitive cases — claim splits at the physical end, put/get spanning the wrap, and a 20000-cycle stream through a 7-byte (non-power-of-two) buffer that crosses theuint16_tindex boundary and exercises base advancement.Pre-public copyright audit
A three-agent review (upstream fidelity, Boreas conformance, adversarial index arithmetic) plus a repo-wide licensing classification. The adversarial probe found zero constructible failures — the
UINT16_MAX/2cap is exact (size=32768 would aliashead-baseto 0 mod 65536 and is rejected at build), and the non-power-of-two × uint16-wrap case is safe (traced 10 boundary crossings, byte-perfect).The licensing pass compared every Zephyr-API file against upstream by actual code, not names, and retains the upstream copyright only where copyrightable expression was genuinely copied (Apache-2.0 §4(c)):
sys/ring_buffer.h+ring_buf.c→(c) 2015 Intel Corporationsys/util.h→(c) 2011-2014 Wind River Systems, Inc.(theIS_ENABLED_XXXX##/_YYYYtoken trick is verbatim)sys/byteorder.h→(c) 2015-2016 Intel Corporation(32-bit be/le chaining mirrors upstream)Files confirmed original and deliberately left un-attributed (false attribution avoided):
dlist.h,slist.h,atomic.h,time_units.h, and all of zshell/zsys/zdevice — independent implementations over ESP-IDF/FreeRTOS whose match to Zephyr is API-surface only.Test plan
Closes #45
🤖 Generated with Claude Code