fix(i2s): don't append TX data to a buffer the DMA already sends

components/esp_driver_i2s/i2s_common.c

@@ -573,6 +573,62 @@ uint32_t i2s_get_source_clk_freq(i2s_clock_src_t clk_src, uint32_t mclk_freq_hz)
     return clk_freq;
 }
 
+/**
+ * @brief   Track which TX buffer the DMA is sending
+ * @note    Called from the TX EOF interrupt. When `finish_desc` is done, the DMA has moved on to the
+ *          next descriptor in the ring. If either of them holds the buffer that the writer is filling,
+ *          data written to that buffer later would be dropped or sent out of order.
+ */
+static void IRAM_ATTR i2s_tx_update_dma_ptr(i2s_chan_handle_t handle, lldesc_t *finish_desc)
+{
+    void *finished_buf = (void *)finish_desc->buf;
+    void *started_buf = (void *)STAILQ_NEXT(finish_desc, qe)->buf;
+    portENTER_CRITICAL_ISR(&g_i2s.spinlock);
+    handle->dma.dma_ptr = started_buf;
+    if (handle->dma.curr_ptr == finished_buf || handle->dma.curr_ptr == started_buf) {
+        handle->dma.curr_started = true;
+    }
+    portEXIT_CRITICAL_ISR(&g_i2s.spinlock);
+}
+
+/**
+ * @brief   Set the TX buffer that the DMA is sending, outside of the EOF interrupt
+ */
+static void i2s_tx_set_dma_ptr(i2s_chan_handle_t handle, void *buf)
+{
+    portENTER_CRITICAL(&g_i2s.spinlock);
+    handle->dma.dma_ptr = buf;
+    if (buf != NULL && handle->dma.curr_ptr == buf) {
+        handle->dma.curr_started = true;
+    }
+    portEXIT_CRITICAL(&g_i2s.spinlock);
+}
+
+/**
+ * @brief   Make `buf` the TX buffer that is being filled
+ * @note    The DMA may have started sending `buf` already: the writer can take the next buffer of
+ *          the ring from the queue just before the EOF interrupt that moves the DMA to it.
+ */
+static void i2s_tx_set_curr_ptr(i2s_chan_handle_t handle, void *buf)
+{
+    portENTER_CRITICAL(&g_i2s.spinlock);
+    handle->dma.curr_ptr = buf;
+    handle->dma.curr_started = buf != NULL && buf == handle->dma.dma_ptr;
+    portEXIT_CRITICAL(&g_i2s.spinlock);
+    handle->dma.rw_pos = 0;
+}
+
+/**
+ * @brief   Whether the DMA has started sending the TX buffer that is being filled
+ */
+static bool i2s_tx_curr_started(i2s_chan_handle_t handle)
+{
+    portENTER_CRITICAL(&g_i2s.spinlock);
+    bool started = handle->dma.curr_started;
+    portEXIT_CRITICAL(&g_i2s.spinlock);
+    return started;
+}
+
 /* Temporary ignore the deprecated warning of i2s_event_data_t::data */
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
@@ -622,6 +678,7 @@ static bool IRAM_ATTR i2s_dma_tx_callback(gdma_channel_handle_t dma_chan, gdma_e
 
     finish_desc = (lldesc_t *)event_data->tx_eof_desc_addr;
     void *curr_buf = (void *)finish_desc->buf;
+    i2s_tx_update_dma_ptr(handle, finish_desc);
     i2s_event_data_t evt = {
         .data = &(finish_desc->buf),
         .dma_buf = curr_buf,
@@ -719,6 +776,7 @@ static void IRAM_ATTR i2s_dma_tx_callback(void *arg)
     if (handle && (status & I2S_LL_EVENT_TX_EOF)) {
         i2s_hal_get_out_eof_des_addr(&(handle->controller->hal), (uint32_t *)&finish_desc);
         void *curr_buf = (void *)finish_desc->buf;
+        i2s_tx_update_dma_ptr(handle, finish_desc);
         evt.data = &(finish_desc->buf);
         evt.dma_buf = curr_buf;
         evt.size = handle->dma.buf_size;
@@ -802,6 +860,15 @@ esp_err_t i2s_init_dma_intr(i2s_chan_handle_t handle, int intr_flag)
         /* Register a new GDMA tx channel */
         ESP_RETURN_ON_ERROR(gdma_new_ahb_channel(&dma_cfg, &handle->dma.dma_chan), TAG, "Register tx dma channel error");
         ESP_GOTO_ON_ERROR(gdma_connect(handle->dma.dma_chan, trig), err1, TAG, "Connect tx dma channel error");
+        /* Raise the EOF event once the DMA has read the whole buffer, not once the data has left the DMA
+         * FIFO. By then the DMA has already prefetched the start of the next buffer, so the EOF would
+         * come too late to tell the writer that it can no longer append to that buffer. */
+        gdma_strategy_config_t strategy = {
+            .owner_check = false,
+            .auto_update_desc = false,
+            .eof_till_data_popped = false,
+        };
+        ESP_GOTO_ON_ERROR(gdma_apply_strategy(handle->dma.dma_chan, &strategy), err2, TAG, "Apply tx dma strategy failed");
         gdma_tx_event_callbacks_t cb = {.on_trans_eof = i2s_dma_tx_callback};
         /* Set callback function for GDMA, the interrupt is triggered by GDMA, then the GDMA ISR will call the  callback function */
         ESP_GOTO_ON_ERROR(gdma_register_tx_event_callbacks(handle->dma.dma_chan, &cb, handle), err2, TAG, "Register tx callback failed");
@@ -818,6 +885,9 @@ esp_err_t i2s_init_dma_intr(i2s_chan_handle_t handle, int intr_flag)
     intr_flag |= handle->intr_prio_flags;
     /* Initialize I2S module interrupt */
     if (handle->dir == I2S_DIR_TX) {
+        /* Raise the EOF interrupt once the DMA has read the whole buffer, not once the data has left the
+         * FIFO. See the GDMA strategy above. */
+        i2s_ll_dma_enable_eof_on_fifo_empty(handle->controller->hal.dev, false);
         esp_intr_alloc_intrstatus(i2s_periph_signal[port_id].irq, intr_flag,
                                   (uint32_t)i2s_ll_get_interrupt_status_reg(handle->controller->hal.dev), I2S_LL_TX_EVENT_MASK,
                                   i2s_dma_tx_callback, handle, &handle->dma.dma_chan);
@@ -1172,6 +1242,10 @@ esp_err_t i2s_channel_enable(i2s_chan_handle_t handle)
 #if CONFIG_PM_ENABLE
     esp_pm_lock_acquire(handle->pm_lock);
 #endif
+    if (handle->dir == I2S_DIR_TX) {
+        /* The DMA starts with the first buffer, which preload may have left partially filled */
+        i2s_tx_set_dma_ptr(handle, (void *)handle->dma.desc[0]->buf);
+    }
     handle->start(handle);
     handle->state = I2S_CHAN_STATE_RUNNING;
     if (handle->dir == I2S_DIR_RX) {
@@ -1206,9 +1280,11 @@ esp_err_t i2s_channel_disable(i2s_chan_handle_t handle)
     xSemaphoreTake(handle->binary, portMAX_DELAY);
     /* Reset the descriptor pointer */
     handle->dma.curr_ptr = NULL;
+    handle->dma.curr_started = false;
     handle->dma.rw_pos = 0;
     handle->stop(handle);
     if (handle->dir == I2S_DIR_TX) {
+        i2s_tx_set_dma_ptr(handle, NULL);
         /* TX queue is reset when the channel is disabled
            In case the queue is wrongly reset after preload the data */
         xQueueReset(handle->msg_queue);
@@ -1246,17 +1322,17 @@ esp_err_t i2s_channel_preload_data(i2s_chan_handle_t tx_handle, const void *src,
             ESP_GOTO_ON_FALSE(xQueueSend(tx_handle->msg_queue, &(tx_handle->dma.desc[i]->buf), 0) == pdTRUE,
                               ESP_FAIL, err, TAG, "Failed to push the descriptor to the queue");
         }
-        tx_handle->dma.curr_ptr = (void *)tx_handle->dma.desc[0]->buf;
-        tx_handle->dma.rw_pos = 0;
+        i2s_tx_set_curr_ptr(tx_handle, (void *)tx_handle->dma.desc[0]->buf);
     }
 
     /* Loop until no bytes in source buff remain or the descriptors are full */
     while (remain_bytes) {
         if (tx_handle->dma.rw_pos == tx_handle->dma.buf_size) {
-            if (xQueueReceive(tx_handle->msg_queue, &(tx_handle->dma.curr_ptr), 0) == pdFALSE) {
+            void *next_buf;
+            if (xQueueReceive(tx_handle->msg_queue, &next_buf, 0) == pdFALSE) {
                 break;
             }
-            tx_handle->dma.rw_pos = 0;
+            i2s_tx_set_curr_ptr(tx_handle, next_buf);
         }
         size_t bytes_can_load = remain_bytes > (tx_handle->dma.buf_size - tx_handle->dma.rw_pos) ?
                                 (tx_handle->dma.buf_size - tx_handle->dma.rw_pos) : remain_bytes;
@@ -1299,23 +1375,51 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si
     ESP_RETURN_ON_FALSE(xSemaphoreTake(handle->binary, pdMS_TO_TICKS(timeout_ms)) == pdTRUE, ESP_ERR_INVALID_STATE, TAG, "The channel is not enabled");
     src_byte = (char *)src;
     while (size > 0 && handle->state == I2S_CHAN_STATE_RUNNING) {
-        if (handle->dma.rw_pos == handle->dma.buf_size || handle->dma.curr_ptr == NULL) {
-            if (xQueueReceive(handle->msg_queue, &(handle->dma.curr_ptr), pdMS_TO_TICKS(timeout_ms)) == pdFALSE) {
+        /* Don't write to a buffer that the DMA has started sending. For example, the previous write
+         * may have left the buffer partially filled, and the channel ran out of data since. Data
+         * written to it would be dropped or sent out of order. Leave the rest of that buffer as is
+         * (silent when auto clear is enabled) and continue with the next free buffer. */
+        if (handle->dma.rw_pos == handle->dma.buf_size || handle->dma.curr_ptr == NULL || i2s_tx_curr_started(handle)) {
+            void *next_buf;
+            /* Wait without removing a buffer: a task switch after dequeue could let the DMA
+             * cycle through and requeue it before the writer records it as current. */
+            if (xQueuePeek(handle->msg_queue, &next_buf, pdMS_TO_TICKS(timeout_ms)) == pdFALSE) {
                 ret = ESP_ERR_TIMEOUT;
                 break;
             }
-            handle->dma.rw_pos = 0;
+            vTaskSuspendAll();
+            /* The ISR may have changed the queue since the peek. Only adopt a buffer that
+             * we actually dequeue now, and never block with the scheduler suspended. */
+            if (xQueueReceive(handle->msg_queue, &next_buf, 0) == pdTRUE) {
+                i2s_tx_set_curr_ptr(handle, next_buf);
+            }
+            xTaskResumeAll();
+            continue;
         }
         data_ptr = (char *)handle->dma.curr_ptr;
         data_ptr += handle->dma.rw_pos;
         bytes_can_write = handle->dma.buf_size - handle->dma.rw_pos;
         if (bytes_can_write > size) {
             bytes_can_write = size;
         }
-        memcpy(data_ptr, src_byte, bytes_can_write);
+        /* Check again and copy without being preempted in between. The DMA sends at the sample rate,
+         * so a copy that starts before the DMA has reached `rw_pos` stays ahead of it. */
+        vTaskSuspendAll();
+        bool started = i2s_tx_curr_started(handle);
+        if (!started) {
+            memcpy(data_ptr, src_byte, bytes_can_write);
 #if SOC_CACHE_INTERNAL_MEM_VIA_L1CACHE
-        esp_cache_msync(handle->dma.curr_ptr, handle->dma.buf_size, ESP_CACHE_MSYNC_FLAG_DIR_C2M);
+            /* Publish to DMA before allowing task preemption. Critical context also prevents
+             * the optional chunked cache-sync implementation from taking a blocking mutex. */
+            portENTER_CRITICAL(&g_i2s.spinlock);
+            esp_cache_msync(handle->dma.curr_ptr, handle->dma.buf_size, ESP_CACHE_MSYNC_FLAG_DIR_C2M);
+            portEXIT_CRITICAL(&g_i2s.spinlock);
 #endif
+        }
+        xTaskResumeAll();
+        if (started) {
+            continue;
+        }
         size -= bytes_can_write;
         src_byte += bytes_can_write;
         handle->dma.rw_pos += bytes_can_write;

components/esp_driver_i2s/i2s_private.h

@@ -121,6 +121,8 @@ typedef struct {
     bool                    auto_clear_before_cb;    /*!< Set to auto clear DMA TX descriptor before callback, i2s will always send zero automatically if no data to send */
     uint32_t                rw_pos;         /*!< reading/writing pointer position */
     void                    *curr_ptr;      /*!< Pointer to current dma buffer */
+    bool                    curr_started;   /*!< TX only: the DMA has started sending curr_ptr, so no more data can be written to it */
+    void                    *dma_ptr;       /*!< TX only: the buffer that the DMA is sending */
     lldesc_t                **desc;         /*!< dma descriptor array */
     uint8_t                 **bufs;         /*!< dma buffer array */
 } i2s_dma_t;

components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt

@@ -1,5 +1,6 @@
 set(srcs "test_app_main.c"
          "test_i2s.c"
+         "test_i2s_tx_timeout.c"
          "test_i2s_iram.c"
          "test_i2s_sleep.c")
 
@@ -8,5 +9,7 @@ if(CONFIG_SOC_I2S_SUPPORTS_ETM AND CONFIG_SOC_GPIO_SUPPORT_ETM)
 endif()
 
 idf_component_register(SRCS ${srcs}
-                       PRIV_REQUIRES unity esp_driver_pcnt spi_flash esp_driver_gpio esp_driver_i2s esp_driver_uart
+                       PRIV_INCLUDE_DIRS "../../.."
+                       PRIV_REQUIRES unity esp_driver_pcnt spi_flash esp_driver_gpio
+                                     esp_driver_i2s esp_driver_uart esp_pm
                        WHOLE_ARCHIVE)

components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c

@@ -0,0 +1,80 @@
+// Copyright (C) 2026 Toit contributors.
+// SPDX-FileCopyrightText: 2026 Toit contributors
+// SPDX-License-Identifier: Apache-2.0
+
+#include <stdlib.h>
+#include <string.h>
+#include "driver/i2s_std.h"
+#include "esp_rom_gpio.h"
+#include "soc/gpio_sig_map.h"
+#include "soc/i2s_periph.h"
+#include "unity.h"
+#include "i2s_private.h"
+
+/* Hold the slave clocks low so there are no EOFs and the free queue cannot
+ * refill. No GPIO is driven. This makes partial writes and timeouts deterministic
+ * without assuming a particular interrupt latency or requiring loopback wiring. */
+TEST_CASE("I2S TX timeout preserves an in-flight tail", "[i2s]")
+{
+    for (unsigned clear_mode = 0; clear_mode < 3; clear_mode++) {
+        i2s_chan_config_t cfg = I2S_CHANNEL_DEFAULT_CONFIG(I2S_NUM_AUTO, I2S_ROLE_SLAVE);
+        cfg.dma_desc_num = 2;
+        cfg.dma_frame_num = 240;
+        cfg.auto_clear_before_cb = clear_mode == 1;
+        cfg.auto_clear_after_cb = clear_mode == 2;
+        i2s_chan_handle_t tx = NULL;
+        TEST_ESP_OK(i2s_new_channel(&cfg, &tx, NULL));
+        i2s_std_config_t std = {
+            .clk_cfg = I2S_STD_CLK_DEFAULT_CONFIG(10000),
+            .slot_cfg = I2S_STD_MSB_SLOT_DEFAULT_CONFIG(I2S_DATA_BIT_WIDTH_16BIT, I2S_SLOT_MODE_STEREO),
+            .gpio_cfg = {
+                .mclk = I2S_GPIO_UNUSED, .bclk = I2S_GPIO_UNUSED,
+                .ws = I2S_GPIO_UNUSED, .dout = I2S_GPIO_UNUSED, .din = I2S_GPIO_UNUSED,
+            },
+        };
+        TEST_ESP_OK(i2s_channel_init_std_mode(tx, &std));
+        int port = tx->controller->id;
+        esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ZERO_INPUT, i2s_periph_signal[port].s_tx_bck_sig, false);
+        esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ZERO_INPUT, i2s_periph_signal[port].s_tx_ws_sig, false);
+
+        size_t size = tx->dma.buf_size;
+        uint8_t *old = malloc(2 * size);
+        uint8_t *next = malloc(size + 4);
+        TEST_ASSERT_NOT_NULL(old);
+        TEST_ASSERT_NOT_NULL(next);
+        memset(old, 0xa5, 2 * size);
+        memset(next, 0x3c, size + 4);
+        size_t loaded = 0;
+        TEST_ESP_OK(i2s_channel_preload_data(tx, old, 2 * size, &loaded));
+        TEST_ASSERT_EQUAL_UINT32(2 * size, loaded);
+        TEST_ESP_OK(i2s_channel_enable(tx));
+        TEST_ESP_OK(i2s_channel_disable(tx));
+
+        /* Reset retains DMA storage. Partially preload descriptor 0 again;
+         * its unwritten tail represents data from an earlier ring pass. */
+        TEST_ESP_OK(i2s_channel_preload_data(tx, next, 4, &loaded));
+        TEST_ASSERT_EQUAL_UINT32(4, loaded);
+        TEST_ESP_OK(i2s_channel_enable(tx));
+
+        size_t written = SIZE_MAX;
+        TEST_ASSERT_EQUAL(ESP_ERR_TIMEOUT, i2s_channel_write(tx, next, size + 4, &written, 0));
+        TEST_ASSERT_EQUAL_UINT32(size, written);
+        TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[0]->buf, 4);
+        TEST_ASSERT_EQUAL_MEMORY(old + 4, (const uint8_t *)tx->dma.desc[0]->buf + 4, size - 4);
+        TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[1]->buf, size);
+
+        /* A subsequent timed-out call must reset bytes_written to zero and
+         * must not overwrite either buffer. Auto-clear only runs at EOF. */
+        written = SIZE_MAX;
+        TEST_ASSERT_EQUAL(ESP_ERR_TIMEOUT, i2s_channel_write(tx, old, 4, &written, 20));
+        TEST_ASSERT_EQUAL_UINT32(0, written);
+        TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[0]->buf, 4);
+        TEST_ASSERT_EQUAL_MEMORY(old + 4, (const uint8_t *)tx->dma.desc[0]->buf + 4, size - 4);
+        TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[1]->buf, size);
+        TEST_ESP_OK(i2s_channel_disable(tx));
+        TEST_ESP_OK(i2s_del_channel(tx));
+        free(next);
+        free(old);
+        printf("TX timeout accounting and untouched tail: clear_mode=%u PASS\n", clear_mode);
+    }
+}