From d44b349d4d6ffcaadc9f0495f1c2fe9f838a477d Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Mon, 2 Nov 2026 09:05:41 -0600 Subject: [PATCH 1/2] refactor(m5stack-tab5): audio task on the shared I2S TX feed policy (codec/i2s_tx_feed.hpp) The follow-up promised in #869: the Tab5 was the one audio BSP still carrying its own copy of the feeding policy from #868. Its playback task now uses espp::i2s_tx_feed::Feeder like the other nine boards (attach_current_task / fill_block / notify_if_block_ready / detach_task), with the ring depth and static_assert taken from the header's constants. No behaviour change: same 12 x 240 ring, 24 ms bounded wait, whole-block writes, stream and trigger level; the Tab5's own specifics (full-duplex RX channel, microphone task, codec setup, frame-aligned producer) are untouched. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- .../m5stack-tab5/include/m5stack-tab5.hpp | 12 +- components/m5stack-tab5/src/audio.cpp | 109 +++++------------- 2 files changed, 33 insertions(+), 88 deletions(-) diff --git a/components/m5stack-tab5/include/m5stack-tab5.hpp b/components/m5stack-tab5/include/m5stack-tab5.hpp index b3fce29e16..52a99cbbc6 100644 --- a/components/m5stack-tab5/include/m5stack-tab5.hpp +++ b/components/m5stack-tab5/include/m5stack-tab5.hpp @@ -35,6 +35,7 @@ #include "es8388.hpp" #include "gt911.hpp" #include "i2c.hpp" +#include "i2s_tx_feed.hpp" #include "ili9881.hpp" #include "ina226.hpp" #include "interrupt.hpp" @@ -921,14 +922,9 @@ class M5StackTab5 : public BaseComponent { std::vector audio_tx_buffer; std::vector audio_rx_buffer; StreamBufferHandle_t audio_tx_stream; - // The playback task's FreeRTOS handle (set by the task itself on its first - // run): play_audio() notifies it when the stream reaches a whole block, so - // its bounded wait for a block is event-driven (see audio_task_callback). - std::atomic audio_tx_task_handle_{nullptr}; - // Whether the playback task's previous block carried audio (only the task - // touches this): a stream that momentarily holds less than a block while - // flowing is given time to complete it; an idle one gets silence at once. - bool audio_tx_was_flowing_{false}; + // Feeding policy shared by every audio BSP (whole blocks, bounded wait, + // ring depth): see codec/i2s_tx_feed.hpp. + espp::i2s_tx_feed::Feeder audio_tx_feed_; StreamBufferHandle_t audio_rx_stream; std::atomic recording_{false}; std::function audio_rx_callback_{nullptr}; diff --git a/components/m5stack-tab5/src/audio.cpp b/components/m5stack-tab5/src/audio.cpp index 2b59e2679e..9887d5b69a 100644 --- a/components/m5stack-tab5/src/audio.cpp +++ b/components/m5stack-tab5/src/audio.cpp @@ -44,24 +44,11 @@ static audio_hal_iface_samples_t audio_hal_samples_from_rate(uint32_t rate) { } } -// How long the audio task gives a producer that is mid-stream to complete a -// block before the block is written padded with silence. Longer than one block -// at every rate the board supports (a block is 1/60 s), so a producer whose -// next frame simply has not arrived yet is not padded; and shorter than the -// DMA ring minus one block, so the ring never drains while waiting (see -// AUDIO_TX_DMA_DESC_NUM). -static constexpr uint32_t AUDIO_TX_WHOLE_BLOCK_WAIT_MS = 24; - -// Depth of the TX channel's DMA ring, in descriptors of AUDIO_TX_DMA_FRAME_NUM -// frames (the I2S driver's default frame count). The audio task keeps this -// ring full: the I2S driver fills descriptors progressively and the DMA sends -// them in ring order, so a descriptor the task has only partly filled is -// safe only while the DMA is a full ring behind it. The ring therefore has to -// outlast the longest pause between writes (AUDIO_TX_WHOLE_BLOCK_WAIT_MS) plus -// the block written after it: 12 x 240 frames is 60 ms at 48 kHz / 65 ms at -// 44.1 kHz; the driver's default of 6 (about 30 ms) leaves no margin. -static constexpr uint32_t AUDIO_TX_DMA_FRAME_NUM = 240; -static constexpr uint32_t AUDIO_TX_DMA_DESC_NUM = 12; +// The playback task's feeding policy (whole blocks every iteration so the I2S +// TX DMA ring stays full, a bounded notification-driven wait for a block while +// a stream is flowing, the ring depth that makes the wait safe) is shared by +// every audio BSP: see codec/i2s_tx_feed.hpp for the mechanism and why it +// matters. This file keeps only what is specific to the Tab5. bool M5StackTab5::initialize_audio(uint32_t sample_rate, const espp::Task::BaseConfig &task_config) { @@ -77,12 +64,14 @@ bool M5StackTab5::initialize_audio(uint32_t sample_rate, i2s_chan_config_t chan_cfg = I2S_CHANNEL_DEFAULT_CONFIG(I2S_NUM_0, I2S_ROLE_MASTER); chan_cfg.auto_clear = true; // Auto clear the legacy data in the DMA buffer // The ring must outlast the audio task's whole-block wait plus one block at - // the highest rate the board supports (see AUDIO_TX_DMA_DESC_NUM). - static_assert(AUDIO_TX_DMA_DESC_NUM * AUDIO_TX_DMA_FRAME_NUM * 1000 / 48000 > - AUDIO_TX_WHOLE_BLOCK_WAIT_MS + 1000 / UPDATE_FREQUENCY, + // the highest rate the board supports (the policy and its rationale: + // codec/i2s_tx_feed.hpp). + static_assert(espp::i2s_tx_feed::ring_outlasts_wait(espp::i2s_tx_feed::DMA_DESC_NUM, + espp::i2s_tx_feed::DMA_FRAME_NUM, 48000, + UPDATE_FREQUENCY), "the I2S TX DMA ring must outlast the whole-block wait plus one block"); - chan_cfg.dma_desc_num = AUDIO_TX_DMA_DESC_NUM; - chan_cfg.dma_frame_num = AUDIO_TX_DMA_FRAME_NUM; + chan_cfg.dma_desc_num = espp::i2s_tx_feed::DMA_DESC_NUM; + chan_cfg.dma_frame_num = espp::i2s_tx_feed::DMA_FRAME_NUM; ESP_ERROR_CHECK(i2s_new_channel(&chan_cfg, &audio_tx_handle, &audio_rx_handle)); // Configure I2S for stereo output (needed for proper ES8388 operation). The @@ -123,6 +112,8 @@ bool M5StackTab5::initialize_audio(uint32_t sample_rate, // ever holding a receiver. audio_tx_stream = xStreamBufferCreate(tx_buf_size * 4, 1); xStreamBufferReset(audio_tx_stream); + audio_tx_feed_.stream = audio_tx_stream; + audio_tx_feed_.frame_bytes = NUM_CHANNELS * NUM_BYTES_PER_CHANNEL; // RX buffer for recording audio_rx_buffer.resize(tx_buf_size); @@ -272,7 +263,7 @@ bool M5StackTab5::initialize_audio(uint32_t sample_rate, logger_.error("Failed to start the microphone task"); // the playback task may already have registered its handle for // play_audio()'s notifications: forget it before the task goes away - audio_tx_task_handle_.store(nullptr, std::memory_order_release); + audio_tx_feed_.detach_task(); audio_task_->stop(); return false; } @@ -325,12 +316,9 @@ size_t M5StackTab5::play_audio(const uint8_t *data, uint32_t num_bytes) { } const size_t sent = xStreamBufferSend(audio_tx_stream, data, sendable, 0); // Wake the playback task if it is waiting for the stream to hold a whole - // block (see audio_task_callback); a notification nobody waits for is - // harmless (the task re-reads the stream after every wake). - if (sent > 0 && xStreamBufferBytesAvailable(audio_tx_stream) >= audio_tx_buffer.size()) { - if (TaskHandle_t task = audio_tx_task_handle_.load(std::memory_order_acquire)) { - xTaskNotifyGive(task); - } + // block (send -> notify -> return; see i2s_tx_feed.hpp). + if (sent > 0) { + audio_tx_feed_.notify_if_block_ready(audio_tx_buffer.size()); } return sent; } @@ -380,60 +368,21 @@ void M5StackTab5::stop_audio_recording() { bool M5StackTab5::audio_task_callback(std::mutex &m, std::condition_variable &cv, bool &task_notified) { - // Playback: write the next block to the I2S channel, ALWAYS a whole block. - // - // The I2S driver fills DMA descriptors progressively and the DMA transmits - // them in ring order, so the only safe steady state is a full ring with the - // write position a whole ring ahead of the DMA. Writing a whole block every - // iteration - audio when there is some, silence when idle - keeps it there: - // the write blocks on the ring and paces this loop. Letting the task sit on - // the stream instead (waiting for data, writing whatever arrived) drains the - // ring while idle; the next sound then starts in descriptors the DMA is - // about to send, half old and half new - a pop at the start of every click - // and playback, and more whenever a partial write is left behind. - // - // Producers push audio in bursts (a video frame at a time, with jitter), so - // when a stream that is flowing momentarily holds less than a block, give - // it a bounded chance to complete the block rather than padding the gap - // with silence (audible as stutter). The wait is event-driven: play_audio() - // notifies this task when the stream reaches a block. It is shorter than - // the ring minus one block, so the ring never drains while waiting; a - // stream that was already idle gets silence at once. - if (audio_tx_task_handle_.load(std::memory_order_relaxed) == nullptr) { - audio_tx_task_handle_.store(xTaskGetCurrentTaskHandle(), std::memory_order_release); - } + // Playback: write the next block to the I2S channel, ALWAYS a whole block, + // so the TX DMA ring stays full; a stream that is flowing but momentarily + // holds less than a block is given a bounded, notification-driven chance to + // complete it. The policy and its rationale live in codec/i2s_tx_feed.hpp. + audio_tx_feed_.attach_current_task(); const size_t block = audio_tx_buffer.size(); uint8_t *tx_buf = audio_tx_buffer.data(); - constexpr size_t frame_bytes = NUM_CHANNELS * NUM_BYTES_PER_CHANNEL; - size_t available = xStreamBufferBytesAvailable(audio_tx_stream); - if (available % frame_bytes != 0) { + bool misaligned = false; + audio_tx_feed_.fill_block(tx_buf, block, misaligned); + if (misaligned) { // Producers queue whole frames only (play_audio), so this is a bug - // somewhere. A stuck partial frame would misalign every frame queued after - // it for good, and merely skipping it here would leave it stuck: say so and - // start the stream over. - logger_.warn_rate_limited("audio stream misaligned ({} bytes queued); resetting it", available); - xStreamBufferReset(audio_tx_stream); - available = 0; - } - if (available < block && (available > 0 || audio_tx_was_flowing_)) { - // wrap-safe bounded wait (FreeRTOS keeps the remaining ticks up to date) - TimeOut_t timeout; - vTaskSetTimeOutState(&timeout); - TickType_t remaining = pdMS_TO_TICKS(AUDIO_TX_WHOLE_BLOCK_WAIT_MS); - while (available < block && xTaskCheckForTimeOut(&timeout, &remaining) == pdFALSE) { - // a notification left over from an earlier send wakes this at once; the - // stream is re-read and the wait resumes, still bounded by the deadline - ulTaskNotifyTake(pdTRUE, remaining); - available = xStreamBufferBytesAvailable(audio_tx_stream); - } - } - available = std::min(available, block); - if (available > 0) { - xStreamBufferReceive(audio_tx_stream, tx_buf, available, 0); + // somewhere; the feeder reset the stream rather than leave a partial frame + // misaligning everything queued after it. + logger_.warn_rate_limited("audio stream held a partial frame; reset it"); } - // silence only where there is no audio - memset(tx_buf + available, 0, block - available); - audio_tx_was_flowing_ = available > 0; i2s_channel_write(audio_tx_handle, tx_buf, block, NULL, portMAX_DELAY); return false; } From dc0a6d64b84bb74b365dbde78445ca06ab69ac46 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Mon, 2 Nov 2026 11:46:48 -0600 Subject: [PATCH 2/2] fix(m5stack-tab5): keep the queued-byte context in the misaligned-stream warning The shared feeder resets the stream when it finds a partial frame, so the BSP's warning had lost the byte count the old Tab5 code logged. The Feeder now records how many bytes the stream held (misaligned_bytes, additive, no API change for the other boards) and the Tab5 warning reports it together with the partial remainder and the frame size. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- components/codec/include/i2s_tx_feed.hpp | 8 +++++++- components/m5stack-tab5/src/audio.cpp | 5 ++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/components/codec/include/i2s_tx_feed.hpp b/components/codec/include/i2s_tx_feed.hpp index b491646a64..dd3ed820a0 100644 --- a/components/codec/include/i2s_tx_feed.hpp +++ b/components/codec/include/i2s_tx_feed.hpp @@ -90,6 +90,11 @@ struct Feeder { /// this): a stream that momentarily holds less than a block while flowing /// is given time to complete it; an idle one gets silence at once. bool was_flowing{false}; + /// Diagnostic: how many bytes the stream held the last time fill_block() + /// found it misaligned (before the reset), so the BSP's warning can say what + /// the producer left behind (`misaligned_bytes % frame_bytes` is the partial + /// frame). + size_t misaligned_bytes{0}; /// Called by the playback task (its first iteration suffices): records the /// calling task as the one to notify. @@ -122,12 +127,13 @@ struct Feeder { /// audio bytes in `buf`. Sets `misaligned` (and resets the stream, since a /// stuck partial frame would misalign every frame after it for good) when /// the stream held a partial frame - producers never queue one, so the - /// caller should warn. + /// caller should warn (misaligned_bytes says what it held). size_t fill_block(uint8_t *buf, size_t block, bool &misaligned) { misaligned = false; size_t available = stream != nullptr ? xStreamBufferBytesAvailable(stream) : 0; if (available % frame_bytes != 0) { misaligned = true; + misaligned_bytes = available; xStreamBufferReset(stream); available = 0; } diff --git a/components/m5stack-tab5/src/audio.cpp b/components/m5stack-tab5/src/audio.cpp index 9887d5b69a..fb71f32bc2 100644 --- a/components/m5stack-tab5/src/audio.cpp +++ b/components/m5stack-tab5/src/audio.cpp @@ -381,7 +381,10 @@ bool M5StackTab5::audio_task_callback(std::mutex &m, std::condition_variable &cv // Producers queue whole frames only (play_audio), so this is a bug // somewhere; the feeder reset the stream rather than leave a partial frame // misaligning everything queued after it. - logger_.warn_rate_limited("audio stream held a partial frame; reset it"); + logger_.warn_rate_limited( + "audio stream misaligned ({} bytes queued, {} bytes into a {}-byte frame); reset it", + audio_tx_feed_.misaligned_bytes, + audio_tx_feed_.misaligned_bytes % audio_tx_feed_.frame_bytes, audio_tx_feed_.frame_bytes); } i2s_channel_write(audio_tx_handle, tx_buf, block, NULL, portMAX_DELAY); return false;