Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lovyan03 merged 2 commits into from
Aug 1, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8178234
Publish playback requests through slot ownership
ainyan03 Aug 1, 2026
449aca7
Serialize begin() against concurrent lazy calls
ainyan03 Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions src/utility/Mic_Class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,18 +708,46 @@ if (_cfg.pin_bck < 0 || _cfg.pin_ws < 0) {

bool Mic_Class::begin(void)
{
// _rec_sample_rate was written before _begun was released, so the
// acquire load makes this pair of reads safe without the lock.
if (_begun.load(std::memory_order_acquire) && _rec_sample_rate == _calc_rec_rate()) { return true; }

// record() calls begin() lazily from whichever task gets there first,
// and both the setup and the sample-rate change tear the port down: two
// of these racing rip the live channel out from under the running task.
// One caller goes through at a time; the others wait for its outcome.
bool zero = false;
while (!_begin_lock.compare_exchange_strong(zero, true))
{
zero = false;
vTaskDelay(1);
}

bool res = true;
if (_task_running)
{
auto rate = _calc_rec_rate();
if (_rec_sample_rate == rate)
if (_rec_sample_rate != rate)
{
return true;
do { vTaskDelay(1); } while (isRecording());
end();
_rec_sample_rate = rate;
}
do { vTaskDelay(1); } while (isRecording());
end();
_rec_sample_rate = rate;
}
if (!_task_running)
{
// Record the rate the port is being built for; without this the next
// begin() at the very same rate tears the port down and rebuilds it.
_rec_sample_rate = _calc_rec_rate();
res = _begin_locked();
}
_begin_lock.store(false);

return res;
}

bool Mic_Class::_begin_locked(void)
{
if (_task_semaphore == nullptr) { _task_semaphore = xSemaphoreCreateBinary(); }

bool res = true;
Expand All @@ -743,13 +771,15 @@ if (_cfg.pin_bck < 0 || _cfg.pin_ws < 0) {
// end() takes the driver and the callback back down; it still sees the
// class as running, which is what lets it do that.
if (!res) { end(); }
else { _begun.store(true, std::memory_order_release); }
}

return res;
}

void Mic_Class::end(void)
{
_begun.store(false, std::memory_order_release);
if (!_task_running) { return; }
_task_running = false;
if (_task_handle)
Expand Down
7 changes: 7 additions & 0 deletions src/utility/Mic_Class.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ namespace m5
static void mic_task(void* args);

uint32_t _calc_rec_rate(void) const;
bool _begin_locked(void);
esp_err_t _setup_i2s(void);
bool _rec_raw(void* recdata, size_t array_len, bool flg_16bit, uint32_t sample_rate, bool stereo);

Expand All @@ -196,6 +197,12 @@ namespace m5

int32_t _offset = 0;
volatile bool _task_running = false;
/// begin() runs from whichever task records first, and setup starts by
/// tearing the port down - so only one call may go through.
std::atomic<bool> _begin_lock { false };
/// True only once begin() has fully finished; the lock-free early return
/// keys on this, so a caller can never see a half-built port as ready.
std::atomic<bool> _begun { false };
#if defined (SDL_h_)
SDL_Thread* _task_handle = nullptr;
#else
Expand Down
172 changes: 129 additions & 43 deletions src/utility/Speaker_Class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,40 +633,73 @@ namespace m5
int ch_diff = ch_info->diff;
size_t ch_index = ch_info->index;

wav_info_t* current_wav = &(ch_info->wavinfo[!ch_info->flip]);
wav_info_t* next_wav = &(ch_info->wavinfo[ ch_info->flip]);
wav_info_t* current_wav = &(ch_info->current);
bool flip = ch_info->flip.load(std::memory_order_relaxed);
uint8_t next_state = ch_info->wavinfo[flip].state.load(std::memory_order_acquire);

size_t idx = 0;

if (current_wav->repeat == 0 || next_wav->stop_current)
if (current_wav->repeat == 0
|| ((next_state & (wav_phase_mask | wav_state_stop_current)) == (wav_phase_published | wav_state_stop_current)))
{
label_next_wav:
bool clear_idx = (next_wav->repeat == 0
|| !next_wav->no_clear_index
|| (next_wav->data != current_wav->data));
current_wav->clear();
ch_info->flip = !ch_info->flip;
next_state = ch_info->wavinfo[flip].state.load(std::memory_order_acquire);
if ((next_state & wav_phase_mask) == wav_phase_published
&& ch_info->wavinfo[flip].state.compare_exchange_strong(next_state
, (uint8_t)((next_state & ~wav_phase_mask) | wav_phase_playing)
, std::memory_order_acquire, std::memory_order_relaxed))
{ // the claim above is what makes the payload of the slot readable.
wav_info_t& incoming = ch_info->wavinfo[flip].info;
bool clear_idx = ((next_state & wav_state_stop_marker)
|| !incoming.no_clear_index
|| (incoming.data != current_wav->data));
*current_wav = incoming;
if (next_state & wav_state_stop_marker)
{ // a pure stop: nothing to play. The slot itself is retired
// further below, once flip has moved off of it - freeing it
// here would let a writer claim it while flip still points at
// it, and the later retirement would wipe that claim out.
current_wav->clear();
}
// the finished (or cut) request goes back to the writers before
// flip moves, so a claim through the fresh flip cannot miss it;
// the wakeup comes last so a woken writer finds flip already moved.
ch_info->wavinfo[!flip].state.store(wav_phase_empty, std::memory_order_release);
ch_info->flip.store(!flip, std::memory_order_relaxed);
flip = !flip;
#if !defined (SDL_h_)
xSemaphoreGive(self->_task_semaphore);
xSemaphoreGive(self->_task_semaphore);
#endif
std::swap(current_wav, next_wav);

if (clear_idx)
{
ch_index = 0;
if (current_wav->repeat == 0)
if (clear_idx)
{
self->_play_channel_bits.fetch_and(~(1 << ch));
if (current_wav->repeat == 0)
{
ch_info->diff = 0;
ch_info->index = 0;
continue;
}
self->_play_channel_bits.fetch_or(1 << ch);
ch_index = 0;
}
}
else if (current_wav->repeat != 0)
{ // a writer snatched the request away first: the current sound
// stays until whatever they are publishing arrives.
goto label_play;
}
if (current_wav->repeat == 0)
{
ch_info->wavinfo[!flip].state.store(wav_phase_empty, std::memory_order_release);
#if !defined (SDL_h_)
xSemaphoreGive(self->_task_semaphore);
#endif
self->_play_channel_bits.fetch_and(~(1 << ch));
next_state = ch_info->wavinfo[flip].state.load(std::memory_order_acquire);
if ((next_state & wav_phase_mask) != wav_phase_published)
{ // nothing to do; a writer caught mid-publish raises the bit itself.
ch_info->diff = 0;
ch_info->index = 0;
continue;
}
self->_play_channel_bits.fetch_or(1 << ch);
goto label_next_wav;
}
}
label_play:
auto data = (const uint8_t*)current_wav->data;
const bool in_stereo = current_wav->is_stereo;
const int32_t in_rate = current_wav->sample_rate_x256;
Expand Down Expand Up @@ -915,7 +948,23 @@ namespace m5

bool Speaker_Class::begin(void)
{
if (_task_running) { return true; }
if (_begun.load(std::memory_order_acquire)) { return true; }

// Playback calls begin() lazily from whichever task gets there first,
// and _setup_i2s starts by uninstalling the port: two of these racing
// rip the live channel out from under the playback task. One caller
// goes through at a time; the others wait and find the work done.
bool zero = false;
while (!_begin_lock.compare_exchange_strong(zero, true))
{
zero = false;
#if defined (SDL_h_)
SDL_Delay(1);
#else
vTaskDelay(1);
#endif
}
if (_begun.load(std::memory_order_acquire)) { _begin_lock.store(false); return true; }

#if !defined (SDL_h_)
if (_task_semaphore == nullptr) { _task_semaphore = xSemaphoreCreateBinary(); }
Expand Down Expand Up @@ -948,18 +997,23 @@ namespace m5
// end() takes the driver and the callback back down; it still sees the
// class as running, which is what lets it do that.
if (!res) { end(); }
else { _begun.store(true, std::memory_order_release); }
}
_begin_lock.store(false);

return res;
}

void Speaker_Class::end(void)
{
_begun.store(false, std::memory_order_release);
if (_cb_set_enabled) { _cb_set_enabled(_cb_set_enabled_args, false); }
if (_task_running)
{
// No stop() here: it would publish markers and notify a task that may
// already be tearing its handle down. The slots are reset below once
// the task is gone, which is all the stop would have achieved.
_task_running = false;
stop();
if (_task_handle)
{
#if defined (SDL_h_)
Expand All @@ -975,8 +1029,11 @@ namespace m5
for (size_t ch = 0; ch < sound_channel_max; ++ch)
{
auto chinfo = &_ch_info[ch];
chinfo->wavinfo[0].clear();
chinfo->wavinfo[1].clear();
chinfo->wavinfo[0].info.clear();
chinfo->wavinfo[0].state.store(wav_phase_empty);
chinfo->wavinfo[1].info.clear();
chinfo->wavinfo[1].state.store(wav_phase_empty);
chinfo->current.clear();
}
#if !defined (SDL_h_)
_i2s_driver_uninstall(_cfg.i2s_port);
Expand All @@ -987,10 +1044,10 @@ namespace m5
{
wav_info_t tmp;
tmp.stop_current = 1;
uint8_t bits = _play_channel_bits.load();
for (size_t ch = 0; ch < sound_channel_max; ++ch)
{
auto chinfo = &_ch_info[ch];
chinfo->wavinfo[chinfo->flip] = tmp;
if (bits & (1 << ch)) { _set_next_wav(ch, tmp); }
}
}

Expand All @@ -1000,12 +1057,11 @@ namespace m5
{
stop();
}
else
else if (_play_channel_bits.load() & (1 << ch))
{
wav_info_t tmp;
tmp.stop_current = 1;
auto chinfo = &_ch_info[ch];
chinfo->wavinfo[chinfo->flip] = tmp;
_set_next_wav(ch, tmp);
}
}

Expand All @@ -1022,25 +1078,55 @@ namespace m5
{
auto chinfo = &_ch_info[ch];
uint8_t chmask = 1 << ch;
if (!wav.stop_current)
const uint8_t claimed = wav_phase_writing | ((wav.repeat == 0) ? wav_state_stop_marker : 0);
for (;;)
{
while ((_play_channel_bits.load() & chmask) && (chinfo->wavinfo[chinfo->flip].repeat))
bool f = chinfo->flip.load(std::memory_order_relaxed);
auto slot = &(chinfo->wavinfo[f]);
uint8_t st = slot->state.load(std::memory_order_relaxed);
uint8_t phase = st & wav_phase_mask;
// a preempting request may take a queued one's place; anything else
// needs the slot back from the task first.
if (phase == wav_phase_empty || (wav.stop_current && phase == wav_phase_published))
{
if (chinfo->wavinfo[!chinfo->flip].repeat == ~0u) { return false; }
if (slot->state.compare_exchange_strong(st, claimed
, std::memory_order_acquire, std::memory_order_relaxed))
{
// holding the claim pins flip: the task cannot adopt a slot in
// writing. So if flip already points elsewhere, this was the stale
// slot - put it back exactly as found and take the fresh target.
if (chinfo->flip.load(std::memory_order_relaxed) != f)
{
slot->state.store(st, std::memory_order_release);
continue;
}
slot->info = wav;
slot->state.store(wav_phase_published
| (wav.stop_current ? wav_state_stop_current : 0)
| (wav.repeat == ~0u ? wav_state_infinite : 0)
| (wav.repeat == 0 ? wav_state_stop_marker : 0)
, std::memory_order_release);
_play_channel_bits.fetch_or(chmask);
#if !defined (SDL_h_)
xSemaphoreTake(_task_semaphore, 1);
#else
SDL_Delay(1);
xTaskNotifyGive(_task_handle);
#endif
return true;
}
continue; // lost the claim to whoever changed the state; they made
// progress, so trying again right away cannot spin for long.
}
if (!wav.stop_current
&& ((chinfo->wavinfo[!f].state.load(std::memory_order_relaxed)
& (wav_phase_mask | wav_state_infinite)) == (wav_phase_playing | wav_state_infinite)))
{ // never a turn behind an endless request.
return false;
}
}
chinfo->wavinfo[chinfo->flip] = wav;
_play_channel_bits.fetch_or(chmask);

#if !defined (SDL_h_)
xTaskNotifyGive(_task_handle);
xSemaphoreTake(_task_semaphore, 1);
#else
SDL_Delay(1);
#endif
return true;
}
}

bool Speaker_Class::_play_raw(const void* data, size_t array_len, bool flg_16bit, bool flg_signed, float sample_rate, bool flg_stereo, uint32_t repeat_count, int channel, bool stop_current_sound, bool no_clear_index)
Expand Down
Loading
Loading