Skip to content

Mic: let record() from the release callback wait briefly for the request lock - #372

Merged
lovyan03 merged 1 commit into
m5stack:developfrom
ainyan03:mic_callback_record
Sep 20, 2026
Merged

lovyan03 merged 1 commit into
m5stack:developfrom
ainyan03:mic_callback_record

Conversation

@ainyan03

Copy link
Copy Markdown
Contributor

Summary

Mic.record() called from the buffer release callback returned false even though the queue had room and no begin()/end() was in progress, so the "two buffers alternately, re-queued from the callback" pattern described in the header stopped capturing after the first buffers.

The publisher notifies the capture task while it still holds the request lock. With a DMA backlog the task fills and releases the buffer before the publisher unlocks, the callback's record() finds the lock taken, and the in-task path returned false at once.

Reported in detail at ainyan03#1 (ESP32-S3 + ES7210, arduino-esp32 3.3.9).

Changes

  • Mic_Class.inl (_rec_raw()): from the capture task, a taken request lock is waited for a tick at a time instead of failing immediately. The wait ends with false when
    • _task_running has been cleared (an end() or sample-rate rebuild holds the lock while waiting for the task to exit; both clear the flag first), or
    • a few ticks have passed (in_task_lock_wait_ticks): the plain atomic has no priority inheritance, so a middle-priority task could otherwise keep the holder off the CPU indefinitely and the callback would never return.
      Both slots already taken is decided before touching the lock: only the capture task frees a slot, so waiting from its own callback cannot change that.
  • The notify stays under the lock on purpose: issuing it after the unlock would let an end() delete the task between the unlock and the notify.
  • Mic_Class.hpp: the release-callback note and record()'s @return now list every reason the callback's record() returns false (queue full, stop in progress, lock held too long, different rate), and the "never block in it" note states the exception.

Public signatures are unchanged. Callers outside the capture task are unaffected.

Behavior note: from the release callback, record() may now block for up to a few ticks while another task finishes publishing a request. Previously it never blocked there (and failed instead).

Verification

Test firmware re-queues each released buffer from the callback and counts record() failures, using the reporter's reproduction (600 ms of DMA backlog before the first two record() calls):

Build ESP32-S3 (ES7210) ESP32 (Core2, PDM mic)
develop first 2 callbacks fail, capture stops not measured
this PR 200 callbacks × 5 runs, 0 failures (2 buffers and 1 buffer) 200 callbacks × 5 runs, 0 failures

Also checked on both: end() called while the callback keeps re-queueing returns within 20 ms and the task exits; a sample-rate change from another task while the callback re-queues a single buffer completes (the callback's record() at the rebuild instant returns false once, as documented) and capture continues at the new rate.

Two rounds of independent adversarial review. The first found that an unbounded per-tick wait could hang the capture task under priority inversion (a middle-priority task starving the lock holder), which led to the bounded wait. CI run on the ainyan03 fork before this PR.

…est lock

record() called from the buffer release callback returned false whenever
the capture task reached the callback while another caller still held the
request lock. That is the common case: the publisher notifies the task
under the lock, and with a DMA backlog the buffer is filled and released
before the publisher gets to unlock. The header promised false only for a
full queue or a begin()/end() in progress, so alternating two buffers from
the callback silently stopped capturing.

The notify stays under the lock: moving it after the unlock would let an
end() delete the task between the unlock and the notify. Instead the
in-task path waits for the lock a tick at a time, bounded:

- it gives up at once when _task_running has been cleared (end() and the
  sample-rate rebuild both clear it before they wait for the task to
  exit, which is the only holder that never releases otherwise);
- it gives up after a few ticks, since the plain atomic has no priority
  inheritance and a middle-priority task could keep the holder off the
  CPU forever; a legitimate holder is a publisher about to unlock, so
  the bound only matters under that inversion;
- both slots taken is decided before touching the lock: only the capture
  task frees a slot, so from its own callback waiting cannot change that.

A busy spin is not an option: on one core the higher-priority capture
task would starve the very caller it waits for.

The header now lists every reason the callback's record() returns false
(full queue, stop in progress, lock held too long, other rate) in both
the callback note and record()'s @return, and states the exception to
"never block in it" instead of contradicting it.
@lovyan03
lovyan03 merged commit 258ab46 into m5stack:develop Sep 20, 2026
28 checks passed
@ainyan03
ainyan03 deleted the mic_callback_record branch September 20, 2026 00:06
lovyan03 added a commit that referenced this pull request Sep 21, 2026
Mic: let record() from the release callback wait briefly for the request lock
(cherry picked from commit 258ab46)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants