Publish a recording request with its length, not before it - #294
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
isRecording() uses relaxed loads, so polling-based completion does not synchronize with the task’s release store that signals recording completion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a cross-core race in the microphone request handover by preventing the consumer task from observing a “ready” slot (non-zero length) before the rest of the slot fields are fully published.
Changes:
- Make
recording_info_t::lengthastd::atomic<size_t>and publish it last withmemory_order_release. - Consume the slot by loading
lengthwithmemory_order_acquireinmic_task, and clear it with a release store on completion. - Replace whole-struct assignment/clearing with field-wise initialization plus a new
recording_info_t::clear()helper.
File summaries
| File | Description |
|---|---|
| src/utility/Mic_Class.hpp | Introduces atomic slot length and declares recording_info_t::clear(); updates isRecording() to use atomic loads. |
| src/utility/Mic_Class.cpp | Applies acquire/release semantics in producer/consumer paths, avoids struct memcpy publication, and clears slots via clear(). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| /// now in recording or not. | ||
| /// @return 0=not recording / 1=recording (There's room in the queue) / 2=recording (There's no room in the queue.) | ||
| size_t isRecording(void) const volatile { return ((bool)_rec_info[0].length) + ((bool)_rec_info[1].length); } | ||
| size_t isRecording(void) const volatile { return ((bool)_rec_info[0].length.load(std::memory_order_relaxed)) + ((bool)_rec_info[1].length.load(std::memory_order_relaxed)); } |
There was a problem hiding this comment.
🟡 Human review recommended
The change is correctness-critical cross-core synchronization using atomics and memory ordering across multiple embedded targets, which warrants final human review despite the focused diff.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The request was handed to the task by assigning the whole struct, which the compiler turns into a memcpy: the length reaches memory somewhere in the middle of it. The task acts on a slot the moment it sees a length there, so it could start on one whose data pointer and sample format were still those of the previous request, and write the wrong width past the end of the buffer. Fill the fields in first and let the length publish the slot, which is what the length already means to the task. It becomes an atomic, stored with a release and loaded with an acquire, so that the fields written before it are what the task sees after it. Ordinary stores carry no such promise between the two cores, and a volatile marker would not either: it leaves the surrounding accesses unordered, and on the RISC-V targets it produces no barrier at all. The same pairing carries the samples back the other way. The task clears the length once it has written the last one, and isRecording() loads it the same way, so a caller that waits for a recording to finish is reading a buffer the task has finished filling. Which slot a request goes into was read twice, once to wait for it to be free and once to write into it, and the task can move it in between: the descriptor could land in a slot whose length was never checked. Read it once and keep that slot for both. A slot can no longer be copied as a whole, so end() clears one through a clear() of its own.
There was a problem hiding this comment.
🟢 Ready to approve
The atomic acquire/release publication pattern is applied consistently at the producer and consumer sites and removes the identified partial-copy race without introducing new unsynchronized accesses in the changed regions.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Follow-up to #293, on the same slot handover.
What goes wrong
record()hands its request tomic_taskby assigning the wholerecording_info_tinto the slot the task polls. The compiler turns that into amemcpy(14 bytes, confirmed by disassembly), and the length sits in the middle of it. The task treats a non-zero length as "this slot is ready", so it can pick a slot up while the rest is still being copied — with the data pointer and the sample format of the previous request. A 16-bit format left over from before makes it write twice as far as the caller's buffer allows.Nothing about this is theoretical for the ordering either:
memcpygives no guarantee about which bytes land first, and the two cores give none about the order they become visible in.The fix
The fields describing the request go in first, and the length publishes the slot last — which is what the length already means to the task.
The length becomes a
std::atomic<size_t>, stored withmemory_order_releaseand loaded withmemory_order_acquire. That is what makes the preceding stores visible to the task once it observes the length; plain stores promise nothing between cores. Avolatilemarker would not be enough either: it leaves the surrounding ordinary accesses unordered, and on the RISC-V targets it emits no barrier at all (on Xtensa it happens to emitmemw, which is where the assumption usually survives). The generated code was checked on both:fence rw,w/fence r,rwon RISC-V,memwon Xtensa, with the accesses inlined — no calls into libatomic.A slot can no longer be copied as a whole, so
end()clears one through aclear()of its own.Verification
On an M5Stack CoreS3, with the recording buffer filled with a sentinel before each
record()and the survivors counted after the wait loop, 200 trials per case: no false completions, no dirty frames, no timeouts — the same as before this change, which is what is expected. This corrects a window that is hard to hit on purpose rather than one that shows up in a loop.Built for ESP32 (ESP-IDF 4.4), ESP32-S3 (ESP-IDF 5.5), ESP32-C6 (RISC-V) and the SDL build.
Not included
The speaker side hands its requests over the same way and has the same problem. Fixing it properly means more than reordering the stores:
spk_taskreads several fields of a slot before deciding to take it, and a stop request skips the wait for the slot to be free, so a "the slot is filled" marker alone does not hold there. That needs its own change to the handover protocol, so it is left out of this one.