I2S: Wrong return codes for partial data transfers to/from DMA memory (IDFGH-6464)

[miketeachman]

Environment

• Development Kit: Lolin D32 Pro
• Kit version: 2.0
• Module or chip used: ESP32-WROVER
• IDF version: v4.4-beta1
• Build System: CMake
• Compiler version: 8.4.0
• Operating System: Windows 10
• (Windows only) environment type: WSL2/Ubuntu
• Using an IDE?: No
• Power Supply: USB

Problem Description

A bug has been introduced in ESP-IDF v4.4-beta1 release that changes the behaviour of some I2S functions. The i2s_write() routine now returns an ESP_ERR_TIMEOUT error when the transmit buffer has become full, after a portion of the supplied data has been written. Here is the problem: a partial data transfer is NOT an error situation and should return ESP_OK -- partial data transfers are expected behaviour and are described in the I2S API documentation. Similar problems exist in functions i2s_write_expand() and i2s_read().

Expected Behavior

i2s_write() returns ERR_OK when any amount of data (even 0 bytes) has been copied to the transmit buffer. This is the API behaviour prior to ESP-IDF v4.4-beta1.

Actual Behavior

i2s_write() returns ESP_ERR_TIMEOUT. This behaviour is introduced in ESP-IDF v4.4-beta1.

Steps to reproduce

1. call i2s_write() providing more data than can be copied into the transmit buffer during the timeout period provided. ESP_ERR_TIMEOUT is returned.

Code to reproduce this issue

rather than show code to reproduce the issue it's easiest to identify the code that was added which produces this bug.
By inspecting this code I think the bug will be obvious. The bug is found in the I2S driver file i2s.c in the functions i2s_write, i2s_write_expand(), and i2s_read(). The bug was introduced in commits b26da6f (for the write functions) and
commit 3c57a6a (for i2s_read())

Here is a code sample from i2s_write(), showing the problem line:

    while (size > 0) {
        if (p_i2s[i2s_num]->tx->rw_pos == p_i2s[i2s_num]->tx->buf_size || p_i2s[i2s_num]->tx->curr_ptr == NULL) {
            if (xQueueReceive(p_i2s[i2s_num]->tx->queue, &p_i2s[i2s_num]->tx->curr_ptr, ticks_to_wait) == pdFALSE) {
                ret = ESP_ERR_TIMEOUT;   // <==== the bug was introduced by adding this line.  Removing this line fixes the bug.
                break;
            }
            p_i2s[i2s_num]->tx->rw_pos = 0;

Debug Logs

Debug information comes from MicroPython

Traceback (most recent call last):
  File "uasyncio/core.py", line 1, in run_until_complete
  File "<stdin>", line 120, in continuous_play
  File "uasyncio/stream.py", line 1, in drain
OSError: [Errno 116] ETIMEDOUT: ESP_ERR_TIMEOUT

Other items if possible

• this bug was introduced in commits b26da6f and 3c57a6a

[L-KAYA]

Hi @miketeachman , thank you for your feedback!

We add this error code because i2s reading and writing functions could never return an error code before, it means even the operation failed (i.e. 0 byte is read or written), it would still return ESP_OK. We think it can not indicate the exact result of reading or writing.

But yes, in the case you mentioned, we can't just return timeout since sometimes incomplete reading or writing is allowed, so we might better to change the error line into ret = (*bytes_written) == 0 ? ESP_ERR_TIMEOUT : ESP_OK;

[miketeachman]

Hi @L-KAYA, thank you for looking at this issue.

I looked at the proposed change you suggested. I have some concerns with this change. 0 bytes written or read does not seem like a failure - it just means that all DMA buffers were full (i2s_write) or empty (i2s_read) during the specified timeout period. This will be a normal situation with I2S. The write and read functions return with either bytes_written or bytes_read - these return values can be used by the calling applications to determine that 0 bytes were copied.

Why do you consider 0 bytes written or read to be a failed operation? I'm struggling to understand this.

[L-KAYA]

Hi @miketeachman , from my consideration, if 0 byte is not a failure, how do users know their configuration is not working? For the case that no data to receive or write, just set the timeout ticks to the maximum value, it will be blocked here. And if the app needs to work in non-block mode (i.e. a short timeout period), the app needs to be able to know that there is a timeout event while receiving message from DMA queue, but the app can also choose to ignore this error code if it doesn't need this.

Therefore, instead of only receiving ESP_OK, the app now can receive the timeout event and decide whether to use this information. Looking forward to your reply!

[miketeachman]

I'm curious on the motivation to change the existing I2S API -- was this user driven? There must be a really good reason given that this is a breaking change (now returns an error code when previously ESP_OK was returned). I wonder how many other existing implementations will be affected by this breaking change.