Report a recording as pending from the moment it is requested - #293
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a race where Mic_Class::isRecording() could report “not recording” immediately after record() queued a request but before the background task set an internal flag, causing callers’ polling loops to exit early and consume stale/partially-written audio buffers.
Changes:
- Make
Mic_Class::isRecording()report based solely on queued recording lengths (and mark itvolatile) so pending requests are visible immediately. - Remove the
_is_recordingflag from the mic task path, and clear pending mic requests inMic_Class::end()to avoid stale “recording” status after stopping. - Propagate task/thread creation failures from
Mic_Class::begin()andSpeaker_Class::begin()via their boolean return values.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/utility/Speaker_Class.cpp | Return failure from begin() when speaker task/thread creation fails. |
| src/utility/Mic_Class.hpp | Update isRecording() to be const volatile and rely on queued lengths instead of a task-set flag; remove _is_recording member. |
| src/utility/Mic_Class.cpp | Remove _is_recording writes, propagate mic task creation failure, and clear pending requests in end(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+741
to
+743
| res = (pdPASS == xTaskCreate(mic_task, "mic_task", stack_size, this, _cfg.task_priority, &_task_handle)); | ||
| } | ||
| if (!res) { _task_running = false; } |
| res = (pdPASS == xTaskCreate(spk_task, "spk_task", stack_size, this, _cfg.task_priority, &_task_handle)); | ||
| } | ||
| #endif | ||
| if (!res) { _task_running = false; } |
isRecording() gated the pending length check on a flag that only the
recording task sets, and only once it has been scheduled and found work
to do. record() stores the request and returns without touching it, so
between the two the function answers 0 while a request is pending. The
usual idiom
if (M5.Mic.record(...)) { while (M5.Mic.isRecording()) { delay(1); } }
therefore falls straight through on the first call after an idle
period, and the buffer is read while it still holds the previous
contents or a partially written frame. begin() waits the same way
before restarting the task at a new sample rate.
The lengths alone already answer the question: record() fills one in
before notifying the task, and the task zeroes it only after writing
the last sample. Drop the flag from the condition, and with it the
member, whose only reader this was. end() now clears the requests, so
one left unfinished by a stopped task cannot keep reporting a
recording. Marking the function volatile keeps the reads in a polling
loop from being optimized away, as the speaker side already does.
The speaker side reaches the same guarantee the other way around, by
setting its channel bits in the calling context before notifying its
task, and re-checking them after clearing.
The result of the task creation was discarded, so a failure left the class believing it was running with no task to serve it: begin() and record() both reported success, and the request stayed queued forever. Report the failure instead, which turns it into the false that record() is already documented to return, and take the half-finished start back down so a failed begin() leaves nothing configured behind it.
The result of the task creation was discarded here as well, so a failure left the class reporting that it had started although nothing would ever serve the queue. Report it, and take the half-finished start back down the same way. Playback itself already survives a failure: _play_raw() returns before queueing when there is no task handle, so nothing is left pending and isPlaying() keeps answering false. What this corrects is the answer begin() gives, and the driver it used to leave configured behind it.
ainyan03
force-pushed
the
mic_isrecording_race
branch
from
August 1, 2026 06:26
afba860 to
7917eba
Compare
This was referenced Aug 1, 2026
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.
Fixes #222.
The race
Mic_Class::isRecording()gated the pending-length check on_is_recording, a flag that onlymic_tasksets, and only once it has been scheduled and found work to do.record()stores the request and notifies the task without touching the flag, so between the two the function answers 0 although a request is pending. The idiom the examples usetherefore falls straight through on the first call after an idle period, and the caller reads a buffer that still holds the previous recording — which is what the reporter heard as a metallic/robotic artifact.
Mic_Class::begin()waits the same way before restarting the task at a new sample rate.The fix
The lengths alone already answer the question:
record()fills one in before notifying the task, and the task zeroes it only after writing the last sample. So the flag is dropped from the condition, and with it the member, whose only reader this was.This is how the speaker side has always worked —
Speaker_Class::isPlaying(uint8_t channel)looks at itswavinfoentries with no flag involved — so the two sides now answer the same way. (The speaker reaches the same guarantee for its channel bits from the other direction:_set_next_wav()sets them in the calling context before notifying, andspk_taskre-checks the data after clearing one.)Two supporting changes come with it:
end()now clears the requests, so one left unfinished by a stopped task cannot keep reporting a recording — the same thingSpeaker_Class::end()does for itswavinfoentries.isRecording()is markedvolatile, so the reads in a polling loop cannot be optimized away, matching the qualification the speaker's status methods already carry.The documented return values (0 / 1 / 2) are unchanged.
begin() reporting a failure
Dropping the flag exposed an existing gap: the result of the task creation was discarded, so a failure left the class believing it was running. Under the old condition
isRecording()answered 0 in that state; under the new one the request would stay queued and the wait would never end.begin()now reports the failure, which turns it into thefalsethatrecord()is already documented to return.The speaker had the same gap in its
begin(), and the last commit closes it there too. Playback itself already survives a failed task creation —_play_raw()returns before queueing when there is no task handle — so on that side it only corrects the answerbegin()gives.Verification
Measured on an M5Stack CoreS3, filling the destination buffer with a sentinel before each
record()and counting how much of it survives the wait loop (200 trials per case):isRecording()==0right afterrecord()A worst frame of 512/512 means the wait returned before a single sample had been written. The 50% rate in the second case comes from the two states alternating: the request that was missed is still recorded afterwards, so the next call finds the task busy and waits correctly, and the one after that meets an idle task again.
Built for ESP32 (ESP-IDF 4.4), ESP32-S3 (ESP-IDF 5.0 and 5.5) and the SDL build.