Skip to content

Fix request handoff and lazy begin() races in Speaker_Class - #295

Merged
lovyan03 merged 2 commits into
m5stack:developfrom
ainyan03:spk_publish
Aug 1, 2026
Merged

lovyan03 merged 2 commits into
m5stack:developfrom
ainyan03:spk_publish

Conversation

@ainyan03

@ainyan03 ainyan03 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Problem

Requests reach the playback task by whole-struct assignment into a shared slot. That compiles to memcpy, which publishes the fields in no particular order, so the task can act on a half-written request. The volatile members give some accidental ordering on Xtensa, but on RISC-V volatile emits no barriers at all, and the ESP32-P4 is dual-core. On top of that, stop() and stop_current requests overwrite slots in place while the task may be reading them, and a writer that reads flip while the task moves it can publish into a slot the task no longer watches.

Separately, begin() runs lazily from whichever task plays first, and the setup starts by uninstalling the port: two first-use calls racing each other destroy the winner's live channel. Reproduced on hardware — i2s_channel_write gets a NULL handle and the board goes into a reboot loop.

Fix

First commit — each slot carries an atomic state byte (empty / writing / published / playing). A writer claims a slot by CAS before touching its payload (a preempting request may also take a queued one's place), and the task claims a published slot by CAS and plays from its own copy, so no one reads or writes a slot someone else holds. The flags that must be readable without the payload ride in the same byte: cutting the current sound, infinite repeat, pure-stop marker. Every handoff is a release/acquire on that byte.

Second commit — begin() is serialized, and its lock-free early return keys on a flag that is set only once the port and task are fully up. The microphone gets the same lock (its sample-rate rebuild runs under it too), and now records the rate the port was first built for, so a second record() at the same rate no longer tears the port down and rebuilds it.

Verified

  • Builds across ESP32 (IDF 4.4 / 5.x), S3, C6, P4, and the SDL simulator.
  • Disassembly on RISC-V shows payload writes, then fence rw,w, then the state store on publish; acquiring claims; nothing on the per-sample path got heavier (the playback loop now reads a plain local copy where it used to re-read volatile fields).
  • On CoreS3 and Tab5 hardware: a timing regression check (single 250 ms / queued 500 ms plays, all trials in range), a stress run (3000 preempting tones against 3000 stop() calls from another core, then 40 queued requests all accepted, clean convergence, no reboot with concurrent first-use begin()), and listening checks (single tones, gapless chaining, instant preempt cut, chord, stop).

Notes

A request used to reach the speaker task as a whole-struct assignment,
which compiles to memcpy and publishes the fields in no particular
order, so the task could act on a half-written slot. Stops and
preempting requests also overwrote a slot the task might be reading,
and could land in the wrong slot when flip moved between the read and
the write.

Each slot now carries an atomic state byte and every handoff is a
claim on it: writers take empty slots (a preempting request may also
take a queued one's place), the task takes published ones and plays
from its own copy, so no one touches a slot someone else holds. The
flags that must be readable without the payload ride in the same byte:
cutting the current sound, refusing to queue behind an endless
request, counting isPlaying().
Playback and recording both call begin() lazily from whichever task
touches them first, and setup starts by tearing the port down: two of
these racing rip the live channel out from under the running task,
which then feeds i2s a null handle and takes the whole board down.
One caller now goes through at a time; the rest wait and find the
work already done. The microphone gets the same lock around its
sample-rate rebuild, and now remembers the rate the port was built
for, so a second recording at the same rate no longer rebuilds it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the audio subsystem against multi-core races by changing the speaker request handoff to a slot-ownership protocol (atomic state + release/acquire publication) and by serializing lazy begin() initialization so concurrent first-use calls can’t tear down each other’s I2S channel setup.

Changes:

  • Reworks Speaker_Class request queueing so writers/task claim slots via CAS and the task plays from a private copy, avoiding torn whole-struct publishes.
  • Serializes lazy begin() for both speaker and microphone using an atomic lock plus a “fully begun” flag for safe lock-free fast paths.
  • Adjusts stop/preempt behavior to route through the new _set_next_wav() protocol.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/utility/Speaker_Class.hpp Introduces slot/state protocol types and updates isPlaying() to use slot occupancy instead of volatile fields.
src/utility/Speaker_Class.cpp Implements slot claiming/publication in the playback task and writers; adds serialized begin() and updates stop paths.
src/utility/Mic_Class.hpp Adds serialized begin() state (_begin_lock/_begun) for microphone initialization.
src/utility/Mic_Class.cpp Implements serialized mic begin() and factors setup into _begin_locked() with a safe fast path.
Suppressed comments (1)

src/utility/Speaker_Class.cpp:1064

  • stop(uint8_t) now becomes a no-op unless _play_channel_bits already has the channel bit set. Similar to stop(), this can miss a channel that has a request in-flight (e.g., wav_phase_writing) but hasn’t set the bit yet, leaving the just-queued request to play despite a stop(channel) call.
  void Speaker_Class::stop(uint8_t ch)
  {
    if ((size_t)ch >= sound_channel_max)
    {
      stop();
    }
    else if (_play_channel_bits.load() & (1 << ch))
    {
      wav_info_t tmp;
      tmp.stop_current = 1;
      _set_next_wav(ch, tmp);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/utility/Speaker_Class.cpp
@lovyan03
lovyan03 merged commit 43ce161 into m5stack:develop Aug 1, 2026
23 checks passed
@ainyan03
ainyan03 deleted the spk_publish branch August 1, 2026 21:28
lovyan03 pushed a commit that referenced this pull request Sep 2, 2026
…end()

Two record() callers could claim the same slot (their plain metadata
writes then race, and the capture task can pair one callers buffer with
the others length), and a record() racing a lifecycle change could
publish a request into a port that was just torn down while still
returning true. The default-rate record() overloads also read
_cfg.sample_rate outside any lock, and the capture task read it again
after startup, so a queued record() with a new rate could retune the
clock under a request already being captured.

All request publishing now happens under a dedicated _rec_lock, and
every lifecycle path joins the same order: record(), end() and the
public begin() each take _rec_lock before _begin_lock. The requested
sample rate is validated and committed inside _begin_raw() under
_begin_lock: a rate of 0, or one whose product with the effective
over_sampling would wrap uint32_t, is rejected without poisoning the
stored config. The default-rate overloads pass 0 (keep the current
rate) instead of reading the config outside the lock, and an explicit
sample_rate of 0 is rejected up front. The capture task uses
_rec_sample_rate - fixed before the task is created - instead of
re-reading _cfg.sample_rate, and _calc_rec_rate() clamps over_sampling
to the same 1..8 range the task itself uses.

The locks cover one begin plus one claim attempt. When both slots are
busy, or a rate change still has recordings draining, the caller
releases the locks, waits outside them, and retries, so end() never
waits behind a full queue or a stalled capture. The lock itself is an
unfair CAS: contending callers have no arrival order, which is
documented at the retry site.

Requests carry a wrap-safe sequence number stamped under _rec_lock,
and the capture task consumes the pending slot with the older stamp.
Slot identity cannot express publish order - a freed slot can be
refilled while the other still holds an earlier request, and cursor
parity schemes desynchronize across the tasks idle transitions. The
tasks two slot reads are not one snapshot, so an empty slot 0 is
re-read once slot 1 is known pending before the sequence compare. The
capture task itself takes no locks; slots still hand over through the
length release/acquire protocol.

begin() now reports honest results on every board: it waits for the
capture task to actually enable the I2S channel (previously only
boards with a post-start callback did, so an _i2s_start() failure left
record() returning true for requests that could never complete), and
it fails cleanly when the task semaphore cannot be allocated. The
teardown handshake is restructured: the capture task acknowledges its
I2S cleanup through a dedicated atomic, parks itself with vTaskSuspend
instead of self-deleting, and _end_locked() deletes it only after the
ack and the park - notifying a self-deleting task raced against its
TCB teardown, and the ack (rather than eTaskGetState alone) matters
because ESP-IDF 4.0-4.2 report an indefinite notification wait as
eSuspended, which would otherwise pass the check before cleanup ran. _task_running and _task_handle are atomics,
and the raw rx_update wait is bounded, with its timeout failing the
begin instead of starting the channel on an unlatched clock. A
persistently failing I2S read backs off one tick per retry so a
high-priority capture task cannot starve the stop request. The
effective rate is capped below 2^31 so the divider search never wraps
internally, and the slot-order compare uses an unsigned half-range
test with no signed-conversion dependence.

The callbacks run while these non-recursive locks are held; the
contract now spells out that they must not call record() either.
config() and setSampleRate() remain unsynchronized and are documented
as stopped-port operations, getters included. The locks are
per-instance: two Mic_Class instances driving the same I2S port stay
unsupported (the driver state is port-global).

Same family as the Speaker slot state machine fix (#295); the Mic side
keeps a writer lock plus sequence stamps instead of per-slot CAS
because its slots have a single consumer.
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.

3 participants