From e61ea9850a8f810e7c0eec360214cdb453ae45f8 Mon Sep 17 00:00:00 2001 From: Abubakr Eirabie Date: Tue, 13 Oct 2026 20:17:43 -0400 Subject: [PATCH] Quiesce Atom audio DMA before delayed Stop cleanup --- .../atom-volume-calibration-2026-10-11.md | 6 +++ tests/i2s_stubs/driver/i2s_std.h | 7 +-- tests/i2s_stubs/fake_i2s.h | 2 + tests/pyramid_volume_tests.cpp | 33 ++++++++++++- tests/test_volume_fence.py | 10 ++++ third_party/esphome-i2s-audio/UPSTREAM.md | 7 +++ .../i2s_audio/speaker/i2s_audio_speaker.cpp | 8 +++- .../i2s_audio/speaker/i2s_audio_speaker.h | 2 + .../speaker/i2s_audio_speaker_standard.cpp | 13 ++++++ .../esphome-i2s-audio/volume-fence.patch | 46 +++++++++++++++---- 10 files changed, 119 insertions(+), 15 deletions(-) diff --git a/docs/development/atom-volume-calibration-2026-10-11.md b/docs/development/atom-volume-calibration-2026-10-11.md index 45cabc1..ca6cd2d 100644 --- a/docs/development/atom-volume-calibration-2026-10-11.md +++ b/docs/development/atom-volume-calibration-2026-10-11.md @@ -41,6 +41,12 @@ proves older FIFO buffers drained. Logical volume readback and wall-clock delay cannot provide that proof; a fully stopped and cleaned-up speaker may complete immediately. +On Stop or natural completion, the opted-in speaker worker disables DMA before +publishing its exit. Main-loop cleanup then deletes the task and channel and +releases the bus. This prevents unserviced completion interrupts from turning +an ordinary stop into a latched overflow fault when that cleanup is delayed. +Active-stream overflow and channel-disable errors still fail closed. + Each distinct requested value gets a two-second fault deadline once hardware is ready. Duplicate requests and Stop/restart retries retain that deadline. It triggers failure, never permission to unmute. Allocation, partial writes, completion overflow, diff --git a/tests/i2s_stubs/driver/i2s_std.h b/tests/i2s_stubs/driver/i2s_std.h index 3a38d2d..48dff37 100644 --- a/tests/i2s_stubs/driver/i2s_std.h +++ b/tests/i2s_stubs/driver/i2s_std.h @@ -25,6 +25,7 @@ struct i2s_event_callbacks_t { namespace fake_i2s { // ESP-IDF 5.5.5 circular DMA: callbacks precede queue overflow and auto-clear. inline void advance() { + if(!channel_enabled){now+=10000;return;} assert(dma.size()==5); if(on_transmit)on_transmit(next_transmit,dma[next_transmit]); transmitted.push_back(dma[next_transmit]);now+=10000; @@ -39,9 +40,9 @@ inline void advance() { } inline int i2s_new_channel(const i2s_chan_config_t*,void **out,void*) {*out=fake_i2s::channel_failure?nullptr:reinterpret_cast(1);return fake_i2s::channel_failure?1:0;} inline int i2s_channel_init_std_mode(void*,const i2s_std_config_t*){return 0;} -inline int i2s_del_channel(void*){fake_i2s::dma.clear();fake_i2s::written=0;fake_i2s::free_descriptors.clear();fake_i2s::next_transmit=0;fake_i2s::current_valid=false;return 0;} -inline int i2s_channel_disable(void*){if(fake_i2s::on_disable)fake_i2s::on_disable();return 0;} -inline int i2s_channel_enable(void*){return fake_i2s::enable_failure?1:0;} +inline int i2s_del_channel(void*){fake_i2s::channel_enabled=false;fake_i2s::dma.clear();fake_i2s::written=0;fake_i2s::free_descriptors.clear();fake_i2s::next_transmit=0;fake_i2s::current_valid=false;return 0;} +inline int i2s_channel_disable(void*){if(fake_i2s::on_disable)fake_i2s::on_disable();if(fake_i2s::disable_failure)return ESP_ERR_INVALID_STATE;fake_i2s::channel_enabled=false;return 0;} +inline int i2s_channel_enable(void*){fake_i2s::channel_enabled=!fake_i2s::enable_failure;return fake_i2s::enable_failure?1:0;} inline int i2s_channel_get_info(void*,i2s_chan_info_t*out){out->total_dma_buf_size=5*fake_i2s::dma_bytes+(fake_i2s::invalid_dma_layout?1:0);return 0;} inline int i2s_channel_preload_data(void*,const void*d,size_t n,size_t*out){auto p=static_cast(d);fake_i2s::dma.emplace_back(p,p+n);*out=fake_i2s::preload_partial?n-1:n;return 0;} inline int i2s_channel_register_event_callback(void*,const i2s_event_callbacks_t*c,void*ctx){ diff --git a/tests/i2s_stubs/fake_i2s.h b/tests/i2s_stubs/fake_i2s.h index 59c5dd4..75d4c9f 100644 --- a/tests/i2s_stubs/fake_i2s.h +++ b/tests/i2s_stubs/fake_i2s.h @@ -18,6 +18,7 @@ inline bool defer_isr_bits{}; inline int64_t now{}; inline bool allocation_failure{}, partial_write{}, drop_events{}, callback_failure{}, channel_failure{}, invalid_dma_layout{}; inline bool ring_allocation_failure{}, source_allocation_failure{}, task_creation_failure{}, preload_partial{}, enable_failure{}; +inline bool channel_enabled{}, disable_failure{}; inline std::function on_write, on_fill, on_disable; inline std::function on_receive; inline void (*task)(void *); inline void *task_arg{}; @@ -32,6 +33,7 @@ inline bool (*callback)(void *, i2s_event_data_t *, void *); inline void *contex inline void reset() { now=0; dma_bytes=8; defer_isr_bits=false; allocation_failure=partial_write=drop_events=callback_failure=channel_failure=invalid_dma_layout=false;queues.clear(); ring_allocation_failure=source_allocation_failure=task_creation_failure=preload_partial=enable_failure=false; + channel_enabled=disable_failure=false; on_write={}; on_fill={}; on_disable={}; on_receive={}; on_transmit={}; free_descriptors.clear(); next_transmit=current_write=0; current_valid=false; overflow_callback=nullptr; task=nullptr; dma.clear(); transmitted.clear(); written=0; callback=nullptr; } } diff --git a/tests/pyramid_volume_tests.cpp b/tests/pyramid_volume_tests.cpp index 11fc109..fcf70bc 100644 --- a/tests/pyramid_volume_tests.cpp +++ b/tests/pyramid_volume_tests.cpp @@ -406,6 +406,37 @@ static void independent_dma_stalls() { try{fake_i2s::task(fake_i2s::task_arg);}catch(const fake_i2s::Exit&){} f.speaker.loop();f.audio.loop();assert(f.audio.is_failed()); } +static void stopped_task_dma_quiescence() { + // Real DMA keeps producing interrupts after the worker stops servicing it. + // Delay main-loop teardown for 200 ms, covering immediate and graceful Stop. + for(bool graceful:{false,true}) { + Fixture f;f.apply(70);unsigned writes=0,disables=0; + const int16_t pcm[4]={20000,20000,20000,20000}; + fake_i2s::on_fill=[&]{f.speaker.loop();if(writes<8)f.speaker.play(reinterpret_cast(pcm),sizeof(pcm));}; + fake_i2s::on_disable=[&]{++disables;}; + fake_i2s::on_write=[&]{if(++writes==8){if(graceful)f.speaker.finish();else f.audio.stop();}}; + f.speaker.start();f.speaker.loop(); + try{fake_i2s::task(fake_i2s::task_arg);}catch(const fake_i2s::Exit&){} + fake_i2s::on_write={}; + const auto transmitted=fake_i2s::transmitted.size(); + for(unsigned i=0;i<20;++i)fake_i2s::advance(); + assert(f.speaker.volume_fence_healthy()); + assert(!fake_i2s::channel_enabled&&disables==1); + assert(fake_i2s::transmitted.size()==transmitted); + f.speaker.loop();f.audio.loop(); + assert(f.audio.ready()&&f.audio.volume_percent()==70&&disables==1); + // Another recording must start without reboot or changing the saved gain. + assert(f.audio.start(::openathan::Track::NORMAL)); + writes=0;fake_i2s::on_write=[&]{if(++writes==8)f.audio.stop();}; + f.run();f.audio.loop(); + assert(f.audio.ready()&&f.audio.volume_percent()==70&&disables==2); + } + // A failed disable is a genuine teardown fault, not a successful Stop. + Fixture failed;failed.apply(70);unsigned writes=0;fake_i2s::disable_failure=true; + fake_i2s::on_write=[&]{if(++writes==8)failed.audio.stop();}; + failed.run();failed.audio.loop(); + assert(failed.audio.is_failed()&&!failed.enabled()&&failed.speaker.fenced_volume()==0); +} static void unfenced_transport_control() { Fixture f;f.speaker.set_volume_fence(false);unsigned writes=0; const int16_t pcm[4]={20000,20000,20000,20000}; @@ -484,4 +515,4 @@ int main() { quran_volume_continuity(); #endif curves(); storage(); interrupted_saves(); isolated_storage();amplifier_control(); - adapter_idle_and_faults();streaming_handoff();dma_fence();rapid_pause_stop_restart();superseded_deadline();dma_faults_and_deadline();independent_dma_stalls();unfenced_transport_control(); } + adapter_idle_and_faults();streaming_handoff();dma_fence();rapid_pause_stop_restart();superseded_deadline();dma_faults_and_deadline();independent_dma_stalls();stopped_task_dma_quiescence();unfenced_transport_control(); } diff --git a/tests/test_volume_fence.py b/tests/test_volume_fence.py index 9ee1424..9fb9fc1 100644 --- a/tests/test_volume_fence.py +++ b/tests/test_volume_fence.py @@ -28,6 +28,16 @@ def test_patch_reconstructs_every_upstream_file(self): self.assertEqual(set(paths), set(hashes)) for name, digest in hashes.items(): self.assertEqual(hashlib.sha256(paths[name].read_bytes()).hexdigest(), digest, name) + result = subprocess.run(["patch", "--batch", "--forward", "-p1", "-i", str(VENDOR / "volume-fence.patch")], + cwd=root, capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + expected = {str(p.relative_to(VENDOR / "components")): hashlib.sha256(p.read_bytes()).hexdigest() + for p in (VENDOR / "components").rglob("*") + if p.is_file() and p.suffix in (".py", ".h", ".cpp")} + rebuilt = {str(p.relative_to(root / "components")): hashlib.sha256(p.read_bytes()).hexdigest() + for p in (root / "components").rglob("*") + if p.is_file() and p.suffix in (".py", ".h", ".cpp")} + self.assertEqual(rebuilt, expected) metadata = json.loads((VENDOR / "upstream.json").read_text()) self.assertEqual(metadata["version"], "2026.9.0") self.assertEqual(metadata["commit"], "c6e4c87e525dd343e470d8dea368957a002f6505") diff --git a/third_party/esphome-i2s-audio/UPSTREAM.md b/third_party/esphome-i2s-audio/UPSTREAM.md index d2a791b..8835b2d 100644 --- a/third_party/esphome-i2s-audio/UPSTREAM.md +++ b/third_party/esphome-i2s-audio/UPSTREAM.md @@ -39,6 +39,13 @@ ISR queue failures latch under the same ESP-IDF critical-section lock immediatel without waiting for deferred event-group updates. Driver teardown rechecks faults before clearing events or allowing a stopped speaker to complete a fence. +The opted-in standard speaker disables its I2S channel in the worker before +publishing `TASK_STOPPING`/`TASK_STOPPED`. DMA must not keep producing completions +after the worker stops servicing descriptors while main-loop cleanup is delayed. +The main loop still owns task deletion, channel deletion and bus release, and +skips a second disable after verified worker quiescence. Existing faults and a +failed disable remain latched; this does not relax active-stream overflow checks. + No audio task, PCM buffer or queue is added. For an opted-in speaker the existing ten-entry record queue grows from 4 to 12 bytes per entry (+80 heap bytes). The synchronized state and lock add fixed object storage. DMA descriptors, source diff --git a/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.cpp b/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.cpp index d0cebe2..90f3560 100644 --- a/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.cpp +++ b/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.cpp @@ -328,6 +328,9 @@ void I2SAudioSpeakerBase::stop_(bool wait_on_empty) { esp_err_t I2SAudioSpeakerBase::init_i2s_channel_(const i2s_chan_config_t &chan_cfg, const i2s_std_config_t &std_cfg, size_t event_queue_size) { +#ifdef USE_OPENATHAN_VOLUME_FENCE + volume_dma_quiesced_ = false; +#endif esp_err_t err = i2s_new_channel(&chan_cfg, &this->tx_handle_, NULL); if (err != ESP_OK) { ESP_LOGE(TAG, "I2S channel allocation failed: %s", esp_err_to_name(err)); @@ -376,7 +379,10 @@ esp_err_t I2SAudioSpeakerBase::init_i2s_channel_(const i2s_chan_config_t &chan_c void I2SAudioSpeakerBase::stop_i2s_driver_() { if (this->tx_handle_ != nullptr) { - i2s_channel_disable(this->tx_handle_); +#ifdef USE_OPENATHAN_VOLUME_FENCE + if (!volume_fence_enabled_ || !volume_dma_quiesced_) +#endif + i2s_channel_disable(this->tx_handle_); i2s_del_channel(this->tx_handle_); this->tx_handle_ = nullptr; diff --git a/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.h b/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.h index 3afb1cf..c1195ed 100644 --- a/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.h +++ b/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker.h @@ -113,6 +113,8 @@ class I2SAudioSpeakerBase : public I2SAudioOut, public speaker::Speaker, public uint32_t volume_completion_capacity_{}; static bool i2s_on_send_q_ovf_cb(i2s_chan_handle_t handle, i2s_event_data_t *event, void *user_ctx); bool volume_fence_enabled_{}; + // Published by the worker before TASK_STOPPED; loop() owns channel deletion. + bool volume_dma_quiesced_{}; portMUX_TYPE volume_mutex_ = portMUX_INITIALIZER_UNLOCKED; VolumeFenceState volume_fence_; #endif diff --git a/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker_standard.cpp b/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker_standard.cpp index 93c21cf..5a6ce3d 100644 --- a/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker_standard.cpp +++ b/third_party/esphome-i2s-audio/components/i2s_audio/speaker/i2s_audio_speaker_standard.cpp @@ -469,6 +469,19 @@ void I2SAudioSpeaker::run_speaker_task() { } } +#ifdef USE_OPENATHAN_VOLUME_FENCE + if (volume_fence_enabled_ && successful_setup) { + // The worker no longer services DMA. Stop callbacks before publishing its + // exit, rather than overflowing during delayed main-loop task cleanup. + // Genuine faults raised before/during disable remain latched. + if (i2s_channel_disable(tx_handle_) == ESP_OK) { + volume_dma_quiesced_ = true; + } else { + fail_volume_fence_(); + ESP_LOGE(TAG, "Failed to quiesce fenced I2S channel"); + } + } +#endif xEventGroupSetBits(this->event_group_, SpeakerEventGroupBits::TASK_STOPPING); audio_source.reset(); diff --git a/third_party/esphome-i2s-audio/volume-fence.patch b/third_party/esphome-i2s-audio/volume-fence.patch index 8c2955f..ab89711 100644 --- a/third_party/esphome-i2s-audio/volume-fence.patch +++ b/third_party/esphome-i2s-audio/volume-fence.patch @@ -91,14 +91,24 @@ + portEXIT_CRITICAL(&volume_mutex_); + } +#endif -@@ -296 +357,5 @@ +@@ -269,0 +331,3 @@ ++#ifdef USE_OPENATHAN_VOLUME_FENCE ++ volume_dma_quiesced_ = false; ++#endif +@@ -296 +360,5 @@ - this->write_records_queue_ = xQueueCreate(event_queue_size, sizeof(uint32_t)); + size_t record_size = sizeof(uint32_t); +#ifdef USE_OPENATHAN_VOLUME_FENCE + if (volume_fence_enabled_) record_size = sizeof(VolumeDmaRecord); +#endif + this->write_records_queue_ = xQueueCreate(event_queue_size, record_size); -@@ -338,0 +404,12 @@ +@@ -314 +382,4 @@ +- i2s_channel_disable(this->tx_handle_); ++#ifdef USE_OPENATHAN_VOLUME_FENCE ++ if (!volume_fence_enabled_ || !volume_dma_quiesced_) ++#endif ++ i2s_channel_disable(this->tx_handle_); +@@ -338,0 +410,12 @@ +#ifdef USE_OPENATHAN_VOLUME_FENCE + if (this_speaker->volume_fence_enabled_) { + portENTER_CRITICAL_ISR(&this_speaker->volume_mutex_); @@ -111,7 +121,7 @@ + portEXIT_CRITICAL_ISR(&this_speaker->volume_mutex_); + } +#endif -@@ -344,0 +422,9 @@ +@@ -344,0 +428,9 @@ +#ifdef USE_OPENATHAN_VOLUME_FENCE + // Event-group updates from ISR are deferred to FreeRTOS's timer task. + // Latch under the same ISR-safe lock before any fence acknowledgement. @@ -121,7 +131,7 @@ + portEXIT_CRITICAL_ISR(&this_speaker->volume_mutex_); + } +#endif -@@ -352 +438,8 @@ +@@ -352 +444,8 @@ - xQueueSendToBackFromISR(this_speaker->i2s_event_queue_, &now, &need_yield3); + const BaseType_t sent = xQueueSendToBackFromISR(this_speaker->i2s_event_queue_, &now, &need_yield3); +#ifdef USE_OPENATHAN_VOLUME_FENCE @@ -131,7 +141,7 @@ + portEXIT_CRITICAL_ISR(&this_speaker->volume_mutex_); + } +#endif -@@ -356,0 +450,13 @@ +@@ -356,0 +456,13 @@ +#ifdef USE_OPENATHAN_VOLUME_FENCE +bool IRAM_ATTR I2SAudioSpeakerBase::i2s_on_send_q_ovf_cb(i2s_chan_handle_t handle, i2s_event_data_t *event, void *user_ctx) { + auto *speaker = static_cast(user_ctx); @@ -145,17 +155,17 @@ +} +#endif + -@@ -358 +464,5 @@ +@@ -358 +470,5 @@ - if (this->q31_volume_factor_ == INT32_MAX) { + apply_volume_factor_(data, bytes_read, q31_volume_factor_); +} + +void I2SAudioSpeakerBase::apply_volume_factor_(uint8_t *data, size_t bytes_read, int32_t factor) { + if (factor == INT32_MAX) { -@@ -365 +475 @@ +@@ -365 +481 @@ - esp_audio_libs::gain::apply(data, data, this->q31_volume_factor_, len, bytes_per_sample); + esp_audio_libs::gain::apply(data, data, factor, len, bytes_per_sample); -@@ -382,0 +493,70 @@ +@@ -382,0 +499,70 @@ +#ifdef USE_OPENATHAN_VOLUME_FENCE +VolumeFenceToken I2SAudioSpeakerBase::request_volume_with_fence(float level) { + if (!volume_fence_enabled_ || !std::isfinite(level) || level < 0 || level > 1) return {}; @@ -243,7 +253,7 @@ + float fenced_volume(); +#endif + -@@ -92,0 +105,14 @@ +@@ -92,0 +105,16 @@ +#ifdef USE_OPENATHAN_VOLUME_FENCE + VolumeGain volume_gain_snapshot_(); + void complete_volume_fence_(VolumeFenceToken token); @@ -255,10 +265,12 @@ + uint32_t volume_completion_capacity_{}; + static bool i2s_on_send_q_ovf_cb(i2s_chan_handle_t handle, i2s_event_data_t *event, void *user_ctx); + bool volume_fence_enabled_{}; ++ // Published by the worker before TASK_STOPPED; loop() owns channel deletion. ++ bool volume_dma_quiesced_{}; + portMUX_TYPE volume_mutex_ = portMUX_INITIALIZER_UNLOCKED; + VolumeFenceState volume_fence_; +#endif -@@ -134,0 +161 @@ +@@ -134,0 +163 @@ + void apply_volume_factor_(uint8_t *data, size_t bytes_read, int32_t factor); --- a/components/i2s_audio/speaker/i2s_audio_speaker_standard.cpp +++ b/components/i2s_audio/speaker/i2s_audio_speaker_standard.cpp @@ -439,6 +451,20 @@ + portEXIT_CRITICAL(&volume_mutex_); + } +#endif +@@ -347,0 +472,13 @@ ++#ifdef USE_OPENATHAN_VOLUME_FENCE ++ if (volume_fence_enabled_ && successful_setup) { ++ // The worker no longer services DMA. Stop callbacks before publishing its ++ // exit, rather than overflowing during delayed main-loop task cleanup. ++ // Genuine faults raised before/during disable remain latched. ++ if (i2s_channel_disable(tx_handle_) == ESP_OK) { ++ volume_dma_quiesced_ = true; ++ } else { ++ fail_volume_fence_(); ++ ESP_LOGE(TAG, "Failed to quiesce fenced I2S channel"); ++ } ++ } ++#endif --- /dev/null +++ b/components/i2s_audio/speaker/volume_fence.h @@ -0,0 +1,74 @@