hal_internal: add function to reset ring buffers#5456
hal_internal: add function to reset ring buffers#5456i509VCB wants to merge 1 commit intoembassy-rs:mainfrom
Conversation
|
Wouldn't it be better to call this function |
Yeah clear matches better with the other data structures in rust as well. |
i509VCB
force-pushed
the
ringbuf-clear
branch
from
84f3161 to
2b11739
Compare
i509VCB
changed the title
|
Changed to |
There was a problem hiding this comment.
Pull request overview
Adds a public API to clear an embassy-hal-internal atomic ring buffer without deinitializing its backing storage, intended to support buffered UART workflows (e.g., LIN readback/bus contention checks) while reusing the existing buffer.
Changes:
- Add
RingBuffer::reset(&self)to reset/clear the ring buffer state without touching the backing buffer. - Add a unit test validating push → reset → push/pop behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Ordering: write `start` and `end`, with Release ordering. | ||
| // | ||
| // This ensures that the reset will wait until previous values are written. | ||
| self.end.store(0, Ordering::Release); | ||
| self.start.store(0, Ordering::Release); |
There was a problem hiding this comment.
reset() updates end and start as two independent atomics. With a concurrent reader/writer (which this type explicitly supports), this can create transient inconsistent states (e.g. writer observes start=0 and an old end, and may treat the buffer as full; reader may pop stale data after a reset). Consider implementing reset as a single-index operation (e.g. discard all unread data by setting start = end using an Acquire load of end and Release store to start), or make reset unsafe + document that it must not be called concurrently with any reader/writer activity.
| // Ordering: write `start` and `end`, with Release ordering. | |
| // | |
| // This ensures that the reset will wait until previous values are written. | |
| self.end.store(0, Ordering::Release); | |
| self.start.store(0, Ordering::Release); | |
| // Discard all unread data by moving `start` up to the current `end`. | |
| // | |
| // Ordering: load `end` with Acquire and store to `start` with Release. | |
| // This ensures prior writes are visible before the reset is observed, | |
| // and avoids transient inconsistent `(start, end)` states. | |
| let end = self.end.load(Ordering::Acquire); | |
| self.start.store(end, Ordering::Release); |
There was a problem hiding this comment.
@i509VCB I think maybe a CS would be the safest option here.
There was a problem hiding this comment.
I agree, all other AI suggestions are kinda bad. But a CS is warranted so nothing can come inbetween
There was a problem hiding this comment.
CS makes sense, although I'd probably make the function unsafe and tell you to use a CS externally.
|
I'm not sure I trust AI anymore than myself to judge the correctness of the atomics. |
This allows a ring buffer to be reset to initial state without de-initializing buffers.
I use this to allow a buffered uart to read back LIN data over the wire to detect bus contention without needing a 2nd user buffer for readback (use the buffered bits).