From 9cf93bbb1b91ce7f46ca185f167cc9cebd94231b Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Fri, 2 Oct 2026 20:35:13 +0200 Subject: [PATCH 1/6] fix(i2s): don't append TX data to a buffer the DMA already sends When a write ends in the middle of a DMA buffer, the next write appends to that buffer. If the channel ran out of data in between, the DMA has already started sending (or even finished) that buffer, so the appended data is dropped or sent a full ring later, out of order. Bursty users, such as WS2812 pixel strips or sound effects, lose the beginning of each burst. Track in the TX EOF interrupt whether the DMA has reached the buffer the writer is filling. If so, the next write leaves the rest of that buffer silent and continues with the next free buffer, which the DMA plays next. Continuous streams that stay ahead of the DMA are unaffected. --- components/esp_driver_i2s/i2s_common.c | 52 ++++++++++++++++++++++--- components/esp_driver_i2s/i2s_private.h | 1 + 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/components/esp_driver_i2s/i2s_common.c b/components/esp_driver_i2s/i2s_common.c index bcf15a342afa..05de61b2f7c8 100644 --- a/components/esp_driver_i2s/i2s_common.c +++ b/components/esp_driver_i2s/i2s_common.c @@ -573,6 +573,35 @@ uint32_t i2s_get_source_clk_freq(i2s_clock_src_t clk_src, uint32_t mclk_freq_hz) return clk_freq; } +/** + * @brief Track whether the DMA has started sending the TX buffer that is being filled + * @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 appended to that buffer later would be dropped or sent out of order. + */ +static void IRAM_ATTR i2s_tx_update_curr_started(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); + 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 Make `buf` the TX buffer that is being filled + */ +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 = false; + portEXIT_CRITICAL(&g_i2s.spinlock); + handle->dma.rw_pos = 0; +} + /* Temporary ignore the deprecated warning of i2s_event_data_t::data */ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" @@ -622,6 +651,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_curr_started(handle, finish_desc); i2s_event_data_t evt = { .data = &(finish_desc->buf), .dma_buf = curr_buf, @@ -719,6 +749,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_curr_started(handle, finish_desc); evt.data = &(finish_desc->buf); evt.dma_buf = curr_buf; evt.size = handle->dma.buf_size; @@ -1206,6 +1237,7 @@ 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) { @@ -1246,17 +1278,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; @@ -1298,13 +1330,21 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si /* The binary semaphore can only be taken when the channel has been enabled and no other writing operation in progress */ 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; + /* The previous write may have left a partially filled buffer that the DMA has started sending + * since, for example because the writer was too slow and the channel ran out of data. Data + * appended 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.curr_started) { + handle->dma.rw_pos = handle->dma.buf_size; + } 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) { + void *next_buf; + if (xQueueReceive(handle->msg_queue, &next_buf, pdMS_TO_TICKS(timeout_ms)) == pdFALSE) { ret = ESP_ERR_TIMEOUT; break; } - handle->dma.rw_pos = 0; + i2s_tx_set_curr_ptr(handle, next_buf); } data_ptr = (char *)handle->dma.curr_ptr; data_ptr += handle->dma.rw_pos; diff --git a/components/esp_driver_i2s/i2s_private.h b/components/esp_driver_i2s/i2s_private.h index 792dfe2196eb..678bf19bac28 100644 --- a/components/esp_driver_i2s/i2s_private.h +++ b/components/esp_driver_i2s/i2s_private.h @@ -121,6 +121,7 @@ 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 appended to it */ lldesc_t **desc; /*!< dma descriptor array */ uint8_t **bufs; /*!< dma buffer array */ } i2s_dma_t; From 4e84cad22d12993529170573dc03246ba1f4c63d Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Fri, 2 Oct 2026 22:50:06 +0200 Subject: [PATCH 2/6] fix(i2s): raise the GDMA TX EOF once the buffer has been read By default a GDMA TX channel raises EOF only when the buffer's data has left the DMA FIFO. By then the DMA has already prefetched the start of the next buffer. The writer can therefore still append to a buffer the DMA has started to read, and that data is lost. On the ESP32-S3 at 10 kHz, 56 to 88 bytes at the start of bursts that followed a pause were dropped. Raise EOF once the DMA has read the buffer, as the legacy I2S DMA of the ESP32 does. --- components/esp_driver_i2s/i2s_common.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/components/esp_driver_i2s/i2s_common.c b/components/esp_driver_i2s/i2s_common.c index 05de61b2f7c8..6a1e4f14430a 100644 --- a/components/esp_driver_i2s/i2s_common.c +++ b/components/esp_driver_i2s/i2s_common.c @@ -833,6 +833,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"); From f82bc0f49278ada626e4524dbf48fdcbaa717562 Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Fri, 2 Oct 2026 22:59:04 +0200 Subject: [PATCH 3/6] fix(i2s): raise the legacy TX EOF once the buffer has been read The I2S DMA of the ESP32 and ESP32-S2 has the same default as GDMA: it raises EOF only once the buffer's data has left the FIFO, when the DMA has already read the start of the next buffer. On the ESP32 at 10 kHz, 4 to 8 bytes at the start of bursts that followed a pause were dropped. --- components/esp_driver_i2s/i2s_common.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/esp_driver_i2s/i2s_common.c b/components/esp_driver_i2s/i2s_common.c index 6a1e4f14430a..18dea62d59dd 100644 --- a/components/esp_driver_i2s/i2s_common.c +++ b/components/esp_driver_i2s/i2s_common.c @@ -858,6 +858,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); From c2af268b65a45b0a4ed99f2e0046ce714a7674ff Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Sat, 3 Oct 2026 23:51:13 +0200 Subject: [PATCH 4/6] fix(i2s): track the buffer that the TX DMA sends The writer could still write to a buffer that the DMA had started: - When preload left the first buffer partially filled, the DMA starts sending it when the channel is enabled, before any EOF. - The writer can take the next buffer of the ring from the queue just before the EOF interrupt that moves the DMA to it. Making it the current buffer then cleared the flag that the interrupt hadn't set. Record the buffer that the DMA sends, in the EOF interrupt and when the channel is enabled, and derive the flag from it when the writer selects a buffer. Check the flag before every copy, and don't let the writer be preempted between the check and the copy. --- components/esp_driver_i2s/i2s_common.c | 70 +++++++++++++++++++------ components/esp_driver_i2s/i2s_private.h | 3 +- 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/components/esp_driver_i2s/i2s_common.c b/components/esp_driver_i2s/i2s_common.c index 18dea62d59dd..370d98b7e33d 100644 --- a/components/esp_driver_i2s/i2s_common.c +++ b/components/esp_driver_i2s/i2s_common.c @@ -574,34 +574,61 @@ uint32_t i2s_get_source_clk_freq(i2s_clock_src_t clk_src, uint32_t mclk_freq_hz) } /** - * @brief Track whether the DMA has started sending the TX buffer that is being filled + * @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 appended to that buffer later would be dropped or sent out of order. + * data written to that buffer later would be dropped or sent out of order. */ -static void IRAM_ATTR i2s_tx_update_curr_started(i2s_chan_handle_t handle, lldesc_t *finish_desc) +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 = false; + 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" @@ -651,7 +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_curr_started(handle, finish_desc); + i2s_tx_update_dma_ptr(handle, finish_desc); i2s_event_data_t evt = { .data = &(finish_desc->buf), .dma_buf = curr_buf, @@ -749,7 +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_curr_started(handle, finish_desc); + i2s_tx_update_dma_ptr(handle, finish_desc); evt.data = &(finish_desc->buf); evt.dma_buf = curr_buf; evt.size = handle->dma.buf_size; @@ -1215,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, handle->dma.desc[0]->buf); + } handle->start(handle); handle->state = I2S_CHAN_STATE_RUNNING; if (handle->dir == I2S_DIR_RX) { @@ -1253,6 +1284,7 @@ esp_err_t i2s_channel_disable(i2s_chan_handle_t handle) 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); @@ -1342,21 +1374,19 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si /* The binary semaphore can only be taken when the channel has been enabled and no other writing operation in progress */ 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; - /* The previous write may have left a partially filled buffer that the DMA has started sending - * since, for example because the writer was too slow and the channel ran out of data. Data - * appended 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.curr_started) { - handle->dma.rw_pos = handle->dma.buf_size; - } while (size > 0 && handle->state == I2S_CHAN_STATE_RUNNING) { - if (handle->dma.rw_pos == handle->dma.buf_size || handle->dma.curr_ptr == NULL) { + /* 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; if (xQueueReceive(handle->msg_queue, &next_buf, pdMS_TO_TICKS(timeout_ms)) == pdFALSE) { ret = ESP_ERR_TIMEOUT; break; } i2s_tx_set_curr_ptr(handle, next_buf); + continue; } data_ptr = (char *)handle->dma.curr_ptr; data_ptr += handle->dma.rw_pos; @@ -1364,7 +1394,17 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si 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); + } + xTaskResumeAll(); + if (started) { + continue; + } #if SOC_CACHE_INTERNAL_MEM_VIA_L1CACHE esp_cache_msync(handle->dma.curr_ptr, handle->dma.buf_size, ESP_CACHE_MSYNC_FLAG_DIR_C2M); #endif diff --git a/components/esp_driver_i2s/i2s_private.h b/components/esp_driver_i2s/i2s_private.h index 678bf19bac28..621ef5f52e63 100644 --- a/components/esp_driver_i2s/i2s_private.h +++ b/components/esp_driver_i2s/i2s_private.h @@ -121,7 +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 appended to it */ + 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; From 540542b93f86d87373d3f2fd1d84a262bf6ed9aa Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Sun, 11 Oct 2026 22:42:57 +0200 Subject: [PATCH 5/6] test(i2s): cover skipped-buffer timeout accounting --- .../test_apps/i2s/main/CMakeLists.txt | 4 +- .../test_apps/i2s/main/test_i2s_tx_timeout.c | 79 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c diff --git a/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt b/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt index d90f51c9f47e..cfcbf111711b 100644 --- a/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt +++ b/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,6 @@ 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) diff --git a/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c b/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c new file mode 100644 index 000000000000..5e30e0cbedd9 --- /dev/null +++ b/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c @@ -0,0 +1,79 @@ +// Copyright (C) 2026 Toit contributors. +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#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); + } +} From b7a900d409643ef47d37528d70177888a8520421 Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Wed, 14 Oct 2026 19:44:41 +0200 Subject: [PATCH 6/6] fix(i2s): protect TX buffer adoption and cache publication Wait for free buffers before suspending scheduling, then dequeue and record the buffer without task preemption. Publish cached writes to DMA before resuming; use critical context to avoid the optional cache-sync mutex. Fix the native test lint failures and descriptor pointer warning. --- components/esp_driver_i2s/i2s_common.c | 24 ++++++++++++++----- .../test_apps/i2s/main/CMakeLists.txt | 3 ++- .../test_apps/i2s/main/test_i2s_tx_timeout.c | 1 + 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/components/esp_driver_i2s/i2s_common.c b/components/esp_driver_i2s/i2s_common.c index 370d98b7e33d..c739dcb538ed 100644 --- a/components/esp_driver_i2s/i2s_common.c +++ b/components/esp_driver_i2s/i2s_common.c @@ -1244,7 +1244,7 @@ esp_err_t i2s_channel_enable(i2s_chan_handle_t handle) #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, handle->dma.desc[0]->buf); + i2s_tx_set_dma_ptr(handle, (void *)handle->dma.desc[0]->buf); } handle->start(handle); handle->state = I2S_CHAN_STATE_RUNNING; @@ -1381,11 +1381,19 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si * (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; - if (xQueueReceive(handle->msg_queue, &next_buf, pdMS_TO_TICKS(timeout_ms)) == pdFALSE) { + /* 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; } - i2s_tx_set_curr_ptr(handle, next_buf); + 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; @@ -1400,14 +1408,18 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si bool started = i2s_tx_curr_started(handle); if (!started) { memcpy(data_ptr, src_byte, bytes_can_write); +#if SOC_CACHE_INTERNAL_MEM_VIA_L1CACHE + /* 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; } -#if SOC_CACHE_INTERNAL_MEM_VIA_L1CACHE - esp_cache_msync(handle->dma.curr_ptr, handle->dma.buf_size, ESP_CACHE_MSYNC_FLAG_DIR_C2M); -#endif size -= bytes_can_write; src_byte += bytes_can_write; handle->dma.rw_pos += bytes_can_write; diff --git a/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt b/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt index cfcbf111711b..b5fb4129a5a8 100644 --- a/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt +++ b/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt @@ -10,5 +10,6 @@ endif() idf_component_register(SRCS ${srcs} PRIV_INCLUDE_DIRS "../../.." - PRIV_REQUIRES unity esp_driver_pcnt spi_flash esp_driver_gpio esp_driver_i2s esp_driver_uart esp_pm + PRIV_REQUIRES unity esp_driver_pcnt spi_flash esp_driver_gpio + esp_driver_i2s esp_driver_uart esp_pm WHOLE_ARCHIVE) diff --git a/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c b/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c index 5e30e0cbedd9..dd6d59172cff 100644 --- a/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c +++ b/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c @@ -1,4 +1,5 @@ // Copyright (C) 2026 Toit contributors. +// SPDX-FileCopyrightText: 2026 Toit contributors // SPDX-License-Identifier: Apache-2.0 #include