I2C slave version 2 - ISR not waking task on request, while stretch keeps SDA low (IDFGH-14490)

[HackXIt]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v5.4.0

Espressif SoC revision.

ESP32-S3 (ESP32-S3FN8) (revision 0.2)

Operating System used.

Linux

How did you build your project?

Command line with idf.py:

idf.py -p /dev/ttyACM0 build flash monitor

If you are using Windows, please specify command line type.

None

Development Kit.

Custom board

Power Supply used.

External 5V

What is the expected behavior?

Receive callback waking the task after sending the corresponding event to the event queue, and the requested data being written back to the master.

What is the actual behavior?

ESP32S3 Controller holds SDA low and causes I²C stretch behavior, since the underlying task dealing with responding is never woken as it waits for the event from the queue.

This means no data reaches the FIFO, causing the stretch behavior.

Steps to reproduce.

TA_lex_i2c_slave.zip

I have attached my full component above. Since it is a bit complicated and related to our proprietary goals, I do not want to go into detail.

But in essence, here are the relevant code parts which I expect to be working, but don't:

IRAM_ATTR static bool i2c_slave_request_cb(i2c_slave_dev_handle_t i2c_slave, const i2c_slave_request_event_data_t *evt_data, void *arg)
{
    i2c_slave_context_t *context = (i2c_slave_context_t *)arg;
    static i2c_slave_event_t evt = I2C_SLAVE_EVT_TX;
    BaseType_t xTaskWoken = 0;
    ESP_DRAM_LOGD(DRAM_STR("i2c_isr_tx"), "Request");
    xQueueSendFromISR(context->event_queue, &evt, &xTaskWoken);
    ESP_DRAM_LOGD(DRAM_STR("i2c_isr_tx"), "Request done (xTaskWoken: %d)", xTaskWoken);
    portYIELD_FROM_ISR();
    return xTaskWoken;
}

IRAM_ATTR static bool i2c_slave_receive_cb(i2c_slave_dev_handle_t i2c_slave, const i2c_slave_rx_done_event_data_t *evt_data, void *arg)
{
    i2c_slave_context_t *context = (i2c_slave_context_t *)arg;
    static i2c_slave_event_t evt = I2C_SLAVE_EVT_RX;
    BaseType_t xTaskWoken = 0;
    // Save bytes
    for (size_t i = 0; i < evt_data->length; ++i)
    {
        ESP_DRAM_LOGD(DRAM_STR("i2c_isr_rx"), "Receive 0x%02X", evt_data->buffer[i]);
        context->bytes[i] = evt_data->buffer[i];
    }
    context->byte_count = evt_data->length;
    xQueueSendFromISR(context->event_queue, &evt, &xTaskWoken);
    ESP_DRAM_LOGD(DRAM_STR("i2c_isr_rx"), "Receive done (xTaskWoken: %d)", xTaskWoken);
    return xTaskWoken;
}

// ... 
void TA_i2c_slave_init()
{
// ...
ESP_ERROR_CHECK(i2c_new_slave_device(&i2c_slv_config, &i2c_slave_ctx->handle));
// ...
}

// ...
void TA_i2c_slave_start()
{
// ...
    i2c_slave_ctx->event_queue = xQueueCreate(1, sizeof(i2c_slave_event_t));
    if (!i2c_slave_ctx->event_queue)
    {
        ESP_LOGE(TAG, "Failed to create event queue");
        return;
    }
    ESP_ERROR_CHECK(i2c_slave_register_event_callbacks(i2c_slave_ctx->handle, &i2c_callbacks, i2c_slave_ctx));
    if (xTaskCreatePinnedToCore(_i2c_slave_task, "I2C Slave Task", 4096, i2c_slave_ctx, configMAX_PRIORITIES - 1, &i2c_task_handle, 0) != pdPASS)
    {
        ESP_LOGE(TAG, "Failed to create I2C slave task");
        return;
    }
// ...
}

// ...
void _i2c_slave_task(void *pvParameters)
{
    // Task initialization
    i2c_slave_context_t *context = (i2c_slave_context_t *)pvParameters;
    int8_t reg_size = -1;
    uint8_t converted_reg_size = 0;
    while (1)
    {
        i2c_slave_event_t evt;
        if (xQueueReceive(context->event_queue, &evt, portMAX_DELAY) != pdTRUE)
        {
            ESP_LOGE(TAG, "Timeout/Failed to receive event");
            continue;
        }
        ESP_LOGD(TAG, "Processing I2C event: %u", evt);
        reg_size = _i2c_get_register_size(context->bytes[0]);
        if (reg_size < 0)
        {
            ESP_LOGE(TAG, "Invalid register address: 0x%02X", context->bytes[0]);
            continue;
        }
        else
        {
            ESP_LOGD(TAG, "%s Register 0x%02X has size %u", evt == I2C_SLAVE_EVT_RX ? "RX" : "TX", context->bytes[0], i2c_register_sizes[context->bytes[0]]);
            converted_reg_size = (uint8_t)reg_size;
        }
        switch (evt)
        {
        case I2C_SLAVE_EVT_RX:
            // Receive data
            if (is_in_range(context->bytes[0], I2C_MEM_REG_CMD_RANGE_START, I2C_MEM_REG_CMD_RANGE_END))
            {
                if (_i2c_verify_command_parameters(GET_COMMAND_INDEX(context->bytes[0]), context->byte_count - 1))
                {
                    _i2c_command_handler(context->bytes[0], context->bytes + 1, context->byte_count - 1);
                }
            }
            else
            {
                _i2c_receive_handler(context->bytes[0], converted_reg_size, context->bytes + 1, context->byte_count - 1);
            }
            break;
        case I2C_SLAVE_EVT_TX:
            // Transmit data
            _i2c_response_handler(context->bytes[0], converted_reg_size);
            break;
        default:
            ESP_LOGW(TAG, "Unknown I2C event: %u", evt);
            break;
        }
    }
    // Task Cleanup
    vTaskDelete(NULL);
}

// This function never gets called, because the task never goes out of sleep, since no I2C_SLAVE_EVT_TX event was sent
void _i2c_response_handler(uint8_t register_byte, uint32_t length)
{
    static uint32_t total_written = 0;
    static uint32_t written = 0;
    while (total_written < length)
    {
        ESP_ERROR_CHECK(i2c_slave_write(i2c_slave_ctx->handle, i2c_slave_ctx->memory + register_byte + total_written, length - total_written, &written, 10));
        total_written += written;
        if (written == 0)
        {
            ESP_LOGE(TAG, "Failed to write to I2C master (timeout or write error)");
            break;
        }
    }
    log_hex_bytes(i2c_slave_ctx->memory + register_byte, total_written); // Uncomment if necessary
    if (total_written != length)
    {
        ESP_LOGE(TAG, "Failed to write all bytes to I2C master (expected: %lu, written: %lu)", length, total_written);
    }
    total_written = 0;
    written = 0;
}

I am sending the data from a raspberry pi as master via the following commands:

# Works fine according to logs, but still gives write failed for unknown reasons, possibly related to the i2cset tool
i2cset -y 1 0x10 0x7A 0x11 i
# Does not work 
i2cget -y 1 0x10 0x7A i 1

Debug Logs.

More Information.

I am using I2C slave version 2, and I have a task handling both receive and request.

Receiving data works fine and can be stored. The handling task is woken with the event.

Requesting data always fails, with SDA held low by the controller (I²C stretch behavior I believe) after the first attempt.

I have recorded the interaction on a logic analyzer:

I am still unaware as to why i2cset gives me a write failed error, despite the acknowledgment by the controller.

However, my monitor log shows a successful operation, until the moment of the request:

# after i2cset:
D i2c_isr_rx: Receive 0x7A
D i2c_isr_rx: Receive 0x11
D i2c_isr_rx: Receive done (xTaskWoken: 1)
D (25468) I2CSlave: Processing I2C event: 0
D (25468) I2CSlave: RX Register 0x7A has size 6

# after i2cget:
D i2c_isr_rx: Receive 0x7A
D i2c_isr_rx: Receive done (xTaskWoken: 1)
D i2c_isr_tx: Request
D i2c_isr_tx: Request done (xTaskWoken: 0) # <-- This should not be 0
D (28048) I2CSlave: Processing I2C event: 0 # <-- Only the receive is processed
D (28048) I2CSlave: RX Register 0x7A has size 6

[HackXIt]

Since the attached component above is dependent on other components, it probably is not straightforward to get it working.
I just provided it for clarity as to where I'm encountering the issue.

Here is an example to reproduce, modified from the hello world example, using all necessary code stripped down to the essentials of the component we're making, bundled into a single project.
(I wanted to create the minimal example from the component's code, to reproduce the behavior without introducing side effects)

I could create an even more minimal example, but this was the quickest way to demonstrate for me, without writing new code.

minimal_i2c_slave_bug.zip

It uses I2C_NUM_0 and GPIO_NUM_2 for SCL and GPIO_NUM_1 for SDA (can be modified in ì2c_slave.c)

I tried to strip everything unrelated to the issue, but kept most of my code structure.
I don't believe it is related to my code, I just did not want to write a completely fresh example from scratch, just to replicate the issue.

EDIT:

Had to re-upload due to broken zip because I forgot the recursive flag when creating it.
Also, please ignore the zip in the initial bug, it also does not contain the source for the same reason, but it is also not necessary anymore due to the example above.

[mythbuster5]

Thanks I will take an eye on it.. BTW, may you try my example called i2c_slave_network_sensor. I want to know if the example also fails

[mythbuster5]

Ok, I might know why you encounter this issue. becasue in one i2c write-read transaction, the request and receive event occur very fast. So I think you need to change your code. 1). please create queue at least two item i2c_slave_ctx->event_queue = xQueueCreate(1, sizeof(i2c_slave_event_t)); in order to contain both rx and tx event. 2). Don't use switch-case: break branch, instead, please use if (rx) {...} if (tx) {....} try it!

[HackXIt]

Ok, I might know why you encounter this issue. becasue in one i2c write-read transaction, the request and receive event occur very fast. So I think you need to change your code. 1). please create queue at least two item i2c_slave_ctx->event_queue = xQueueCreate(1, sizeof(i2c_slave_event_t)); in order to contain both rx and tx event. 2). Don't use switch-case: break branch, instead, please use if (rx) {...} if (tx) {....} try it!

1. Nice catch - Now it at least responds properly.

I am wondering, if the i2c_slave version 2 writes all bytes into the FIFO when using i2c_slave_write, because there might be an additional issue for me.

In my i2c slave component, I implemented a 128-byte memory with varying register sizes. The code checks the size of a register according to a table and returns only bytes up to the maximum size of that register. However, when writing back, I always use the full register size to write, since requested byte-length is unknown in I²C.

For a register of size 6, the master could only request 1 byte of the 6 put into FIFO, leaving 5 remaining bytes in the FIFO after the call to i2c_slave_write. On a secondary request to the same register, I'll again put 6 bytes in the FIFO, however, since there are already 5 in it, wouldn't the master then read back the remaining 5 and the first of the additional 6?

Should I clear the FIFO in this?

2. Due to how the queue works, I'll only ever access one queue item, so the switch-case is valid in my opinion, otherwise I'll always check both if-statements, were only 1 is ever true. Or am I mistaken here?

To clarify, the reason I have a switch-case to begin with, is the fact that I also want to use the queue to send events to the task from another task, not just the ISR. I just haven't gotten to that part yet.
An example of what the task has to do additionally: react to a GPIO interrupt to change its slave address, restarting the initialization.

[mythbuster5]

Ok, sorry, to root cause is queue size indeed, switch-case logic is not that matter, my bad..

As for For a register of size 6, the master could only request 1 byte of the 6 put into FIFO, leaving 5 remaining bytes in the FIFO after the call to i2c_slave_write if your master have already take one byte, the content in fifo will be self cleared. I'd like to suggest to refer to this example, https://github.com/espressif/esp-idf/tree/master/examples/peripherals/i2c/i2c_slave_network_sensor which contains small buffer write and large buffer write.

I don't know whether this is valid for you.

[HackXIt]

Ok, sorry, to root cause is queue size indeed, switch-case logic is not that matter, my bad..

As for For a register of size 6, the master could only request 1 byte of the 6 put into FIFO, leaving 5 remaining bytes in the FIFO after the call to i2c_slave_write if your master have already take one byte, the content in fifo will be self cleared. I'd like to suggest to refer to this example, https://github.com/espressif/esp-idf/tree/master/examples/peripherals/i2c/i2c_slave_network_sensor which contains small buffer write and large buffer write.

I don't know whether this is valid for you.

I mostly looked at this example already when implementing this. The main issue was the queue size, a minor detail easily overlooked.

If the FIFO is cleared, than that's completely enough for me.

Closing this, since solved.