refactor(m5stack-tab5): audio task on the shared I2S TX feed policy (follow-up to #869)

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;
     }

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<uint8_t> audio_tx_buffer;
   std::vector<uint8_t> 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<TaskHandle_t> 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<bool> recording_{false};
   std::function<void(const uint8_t *data, size_t length)> audio_rx_callback_{nullptr};

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,24 @@ 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 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);
   }
-  // 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;
 }