From 0e44dcf2ffc93c85f7662df075c75e453b03c0f2 Mon Sep 17 00:00:00 2001 From: "C.S.M" Date: Thu, 3 Jul 2025 12:19:04 +0800 Subject: [PATCH 1/2] fix(i2c_master): Fix that master multi-read failed, Closes https://github.com/espressif/esp-idf/issues/16231 --- components/esp_driver_i2c/i2c_master.c | 81 +++++++++++++++---- .../esp_driver_i2c/include/driver/i2c_types.h | 3 +- 2 files changed, 69 insertions(+), 15 deletions(-) diff --git a/components/esp_driver_i2c/i2c_master.c b/components/esp_driver_i2c/i2c_master.c index 96a78b4a84..d5a45187aa 100644 --- a/components/esp_driver_i2c/i2c_master.c +++ b/components/esp_driver_i2c/i2c_master.c @@ -7,6 +7,7 @@ #include #include #include +#include "esp_rom_sys.h" #include "sdkconfig.h" #include "esp_types.h" #include "esp_attr.h" @@ -291,14 +292,62 @@ static bool s_i2c_read_command(i2c_master_bus_handle_t i2c_master, i2c_operation i2c_master->contains_read = true; #if !SOC_I2C_STOP_INDEPENDENT + /* + * If the remaining bytes is less than the hardware fifo length - 1, + * it means the read command can be sent out in one time. + * So, we set the status to I2C_STATUS_READ_ALL. + */ if (remaining_bytes < I2C_FIFO_LEN(i2c_master->base->port_num) - 1) { + atomic_store(&i2c_master->status, I2C_STATUS_READ_ALL); if (i2c_operation->hw_cmd.ack_val == I2C_ACK_VAL) { if (remaining_bytes != 0) { - i2c_ll_master_write_cmd_reg(hal->dev, hw_cmd, i2c_master->cmd_idx); - i2c_master->read_len_static = i2c_master->rx_cnt; - i2c_master->cmd_idx++; + // If the next transaction is a read command, and the ack value is I2C_ACK_VAL, + // that means we have multi read jobs. We send out this read command first. + // Otherwise, we can mix with other commands, like stop. + i2c_operation_t next_transaction = i2c_master->i2c_trans.ops[i2c_master->trans_idx + 1]; + if (next_transaction.hw_cmd.op_code == I2C_LL_CMD_READ && next_transaction.hw_cmd.ack_val == I2C_ACK_VAL) { + portENTER_CRITICAL_SAFE(&handle->spinlock); + i2c_master->read_len_static = i2c_master->rx_cnt; + i2c_ll_master_write_cmd_reg(hal->dev, hw_cmd, i2c_master->cmd_idx); + i2c_ll_master_write_cmd_reg(hal->dev, hw_end_cmd, i2c_master->cmd_idx + 1); + i2c_master->cmd_idx = 0; + i2c_master->read_buf_pos = i2c_master->trans_idx; + i2c_master->trans_idx++; + i2c_master->i2c_trans.cmd_count--; + portEXIT_CRITICAL_SAFE(&handle->spinlock); + if (i2c_master->async_trans == false) { + i2c_hal_master_trans_start(hal); + } else { + i2c_master->async_break = true; + } + } else { + portENTER_CRITICAL_SAFE(&handle->spinlock); + i2c_ll_master_write_cmd_reg(hal->dev, hw_cmd, i2c_master->cmd_idx); + i2c_master->read_len_static = i2c_master->rx_cnt; + i2c_master->cmd_idx++; + i2c_master->read_buf_pos = i2c_master->trans_idx; + i2c_master->trans_idx++; + i2c_master->i2c_trans.cmd_count--; + portEXIT_CRITICAL_SAFE(&handle->spinlock); + if (i2c_master->async_trans == false) { + if (xPortInIsrContext()) { + xSemaphoreGiveFromISR(i2c_master->cmd_semphr, do_yield); + } else { + xSemaphoreGive(i2c_master->cmd_semphr); + } + } + } + } else { + i2c_master->trans_idx++; + i2c_master->i2c_trans.cmd_count--; + if (i2c_master->async_trans == false) { + if (xPortInIsrContext()) { + xSemaphoreGiveFromISR(i2c_master->cmd_semphr, do_yield); + } else { + xSemaphoreGive(i2c_master->cmd_semphr); + } + } } - i2c_master->read_buf_pos = i2c_master->trans_idx; } else { i2c_ll_master_write_cmd_reg(hal->dev, hw_cmd, i2c_master->cmd_idx); // If the read position has not been marked, that means the transaction doesn't contain read-ack @@ -309,19 +358,20 @@ static bool s_i2c_read_command(i2c_master_bus_handle_t i2c_master, i2c_operation i2c_master->read_len_static = i2c_master->rx_cnt; } i2c_master->cmd_idx++; - } - i2c_master->trans_idx++; - i2c_master->i2c_trans.cmd_count--; - if (i2c_master->async_trans == false) { - if (xPortInIsrContext()) { - xSemaphoreGiveFromISR(i2c_master->cmd_semphr, do_yield); - } else { - xSemaphoreGive(i2c_master->cmd_semphr); + i2c_master->trans_idx++; + i2c_master->i2c_trans.cmd_count--; + if (i2c_master->async_trans == false) { + if (xPortInIsrContext()) { + xSemaphoreGiveFromISR(i2c_master->cmd_semphr, do_yield); + } else { + xSemaphoreGive(i2c_master->cmd_semphr); + } } } } else { atomic_store(&i2c_master->status, I2C_STATUS_READ); portENTER_CRITICAL_SAFE(&handle->spinlock); + i2c_master->read_buf_pos = i2c_master->trans_idx; i2c_ll_master_write_cmd_reg(hal->dev, hw_cmd, i2c_master->cmd_idx); i2c_ll_master_write_cmd_reg(hal->dev, hw_end_cmd, i2c_master->cmd_idx + 1); if (i2c_master->async_trans == false) { @@ -342,6 +392,7 @@ static bool s_i2c_read_command(i2c_master_bus_handle_t i2c_master, i2c_operation i2c_master->i2c_trans.cmd_count--; } } + i2c_master->read_buf_pos = i2c_master->trans_idx; i2c_ll_master_write_cmd_reg(hal->dev, hw_cmd, i2c_master->cmd_idx); i2c_ll_master_write_cmd_reg(hal->dev, hw_end_cmd, i2c_master->cmd_idx + 1); portEXIT_CRITICAL_SAFE(&handle->spinlock); @@ -672,7 +723,7 @@ I2C_MASTER_ISR_ATTR static void i2c_isr_receive_handler(i2c_master_bus_t *i2c_ma i2c_hal_context_t *hal = &i2c_master->base->hal; if (atomic_load(&i2c_master->status) == I2C_STATUS_READ) { - i2c_operation_t *i2c_operation = &i2c_master->i2c_trans.ops[i2c_master->trans_idx]; + i2c_operation_t *i2c_operation = &i2c_master->i2c_trans.ops[i2c_master->read_buf_pos]; portENTER_CRITICAL_ISR(&i2c_master->base->spinlock); i2c_ll_read_rxfifo(hal->dev, i2c_operation->data + i2c_operation->bytes_used, i2c_master->rx_cnt); /* rx_cnt bytes have just been read, increment the number of bytes used from the buffer */ @@ -686,6 +737,7 @@ I2C_MASTER_ISR_ATTR static void i2c_isr_receive_handler(i2c_master_bus_t *i2c_ma i2c_master->read_buf_pos = i2c_master->trans_idx; i2c_master->trans_idx++; i2c_operation->bytes_used = 0; + i2c_master->read_len_static = 0; } portEXIT_CRITICAL_ISR(&i2c_master->base->spinlock); } @@ -695,10 +747,11 @@ I2C_MASTER_ISR_ATTR static void i2c_isr_receive_handler(i2c_master_bus_t *i2c_ma portENTER_CRITICAL_ISR(&i2c_master->base->spinlock); i2c_ll_read_rxfifo(hal->dev, i2c_operation->data + i2c_operation->bytes_used, i2c_master->read_len_static); // If the read command only contain nack marker, no read it for the second time. - if (i2c_master->i2c_trans.ops[i2c_master->read_buf_pos + 1].data) { + if (i2c_master->i2c_trans.ops[i2c_master->read_buf_pos + 1].data && i2c_master->i2c_trans.ops[i2c_master->read_buf_pos + 1].hw_cmd.ack_val == I2C_NACK_VAL) { i2c_ll_read_rxfifo(hal->dev, i2c_master->i2c_trans.ops[i2c_master->read_buf_pos + 1].data, 1); } i2c_master->w_r_size = i2c_master->read_len_static + 1; + i2c_master->read_len_static = 0; i2c_master->contains_read = false; portEXIT_CRITICAL_ISR(&i2c_master->base->spinlock); } diff --git a/components/esp_driver_i2c/include/driver/i2c_types.h b/components/esp_driver_i2c/include/driver/i2c_types.h index 9c83bc5b30..ddcd0c3eaa 100644 --- a/components/esp_driver_i2c/include/driver/i2c_types.h +++ b/components/esp_driver_i2c/include/driver/i2c_types.h @@ -25,7 +25,8 @@ typedef int i2c_port_num_t; * @brief Enumeration for I2C fsm status. */ typedef enum { - I2C_STATUS_READ, /*!< read status for current master command */ + I2C_STATUS_READ, /*!< read status for current master command, but just partial read, not all data is read is this status */ + I2C_STATUS_READ_ALL, /*!< read status for current master command, all data is read is this status */ I2C_STATUS_WRITE, /*!< write status for current master command */ I2C_STATUS_START, /*!< Start status for current master command */ I2C_STATUS_STOP, /*!< stop status for current master command */ From 7d8d1fb98f173f6f78894411be35eca86e2ad5b5 Mon Sep 17 00:00:00 2001 From: "C.S.M" Date: Thu, 3 Jul 2025 12:19:50 +0800 Subject: [PATCH 2/2] test(i2c_master): Add test for master multi read job --- .../i2c_test_apps/main/test_i2c_multi.c | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c b/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c index a90d8e9bf2..aedb400c1e 100644 --- a/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c +++ b/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c @@ -387,6 +387,104 @@ static void slave_write_buffer_test_single_byte(void) TEST_CASE_MULTIPLE_DEVICES("I2C master read slave test single byte", "[i2c][test_env=generic_multi_device][timeout=150]", master_read_slave_test_single_byte, slave_write_buffer_test_single_byte); +static void master_read_slave_test_with_multi_read_job(void) +{ + uint8_t data_rd[DATA_LENGTH] = {0}; + i2c_master_bus_config_t i2c_mst_config = { + .clk_source = I2C_CLK_SRC_DEFAULT, + .i2c_port = TEST_I2C_PORT, + .scl_io_num = I2C_MASTER_SCL_IO, + .sda_io_num = I2C_MASTER_SDA_IO, + .flags.enable_internal_pullup = true, + }; + i2c_master_bus_handle_t bus_handle; + TEST_ESP_OK(i2c_new_master_bus(&i2c_mst_config, &bus_handle)); + + i2c_device_config_t dev_cfg = { + .dev_addr_length = I2C_ADDR_BIT_LEN_7, + .device_address = I2C_DEVICE_ADDRESS_NOT_USED, + .scl_speed_hz = 100000, + .scl_wait_us = 20000, + }; + + i2c_master_dev_handle_t dev_handle; + TEST_ESP_OK(i2c_master_bus_add_device(bus_handle, &dev_cfg, &dev_handle)); + + unity_wait_for_signal("i2c slave init finish"); + + uint8_t read_address = (ESP_SLAVE_ADDR << 1 | 1); + + i2c_operation_job_t i2c_ops[] = { + { .command = I2C_MASTER_CMD_START }, + { .command = I2C_MASTER_CMD_WRITE, .write = { .ack_check = true, .data = (uint8_t *) &read_address, .total_bytes = 1 } }, + { .command = I2C_MASTER_CMD_READ, .read = { .ack_value = I2C_ACK_VAL, .data = data_rd, .total_bytes = 1 }}, + { .command = I2C_MASTER_CMD_READ, .read = { .ack_value = I2C_ACK_VAL, .data = data_rd + 1, .total_bytes = 1 }}, + { .command = I2C_MASTER_CMD_READ, .read = { .ack_value = I2C_NACK_VAL, .data = data_rd + 2, .total_bytes = 1 }}, + { .command = I2C_MASTER_CMD_STOP }, + }; + + i2c_master_execute_defined_operations(dev_handle, i2c_ops, sizeof(i2c_ops) / sizeof(i2c_operation_job_t), 1000); + vTaskDelay(100 / portTICK_PERIOD_MS); + TEST_ASSERT(data_rd[0] == 6); + TEST_ASSERT(data_rd[1] == 8); + TEST_ASSERT(data_rd[2] == 10); + unity_send_signal("ready to delete master read test"); + + TEST_ESP_OK(i2c_master_bus_rm_device(dev_handle)); + TEST_ESP_OK(i2c_del_master_bus(bus_handle)); +} + +static void slave_write_buffer_test_single_byte_for_multi_read_job(void) +{ + i2c_slave_dev_handle_t handle; + uint8_t data_wr[DATA_LENGTH]; + event_queue = xQueueCreate(2, sizeof(i2c_slave_event_t)); + assert(event_queue); + + i2c_slave_config_t i2c_slv_config = { + .i2c_port = TEST_I2C_PORT, + .clk_source = I2C_CLK_SRC_DEFAULT, + .scl_io_num = I2C_SLAVE_SCL_IO, + .sda_io_num = I2C_SLAVE_SDA_IO, + .slave_addr = ESP_SLAVE_ADDR, + .send_buf_depth = DATA_LENGTH, + .receive_buf_depth = DATA_LENGTH, + .flags.enable_internal_pullup = true, + }; + + TEST_ESP_OK(i2c_new_slave_device(&i2c_slv_config, &handle)); + + i2c_slave_event_callbacks_t cbs = { + .on_receive = i2c_slave_receive_cb, + .on_request = i2c_slave_request_cb, + }; + + TEST_ESP_OK(i2c_slave_register_event_callbacks(handle, &cbs, NULL)); + + unity_send_signal("i2c slave init finish"); + + data_wr[0] = 6; + data_wr[1] = 8; + data_wr[2] = 10; + + i2c_slave_event_t evt; + uint32_t write_len; + while (true) { + if (xQueueReceive(event_queue, &evt, portMAX_DELAY) == pdTRUE) { + if (evt == I2C_SLAVE_EVT_TX) { + TEST_ESP_OK(i2c_slave_write(handle, data_wr, 3, &write_len, 1000)); + break; + } + } + } + + unity_wait_for_signal("ready to delete master read test"); + vQueueDelete(event_queue); + TEST_ESP_OK(i2c_del_slave_device(handle)); +} + +TEST_CASE_MULTIPLE_DEVICES("I2C master read slave test single byte 2", "[i2c][test_env=generic_multi_device][timeout=150]", master_read_slave_test_with_multi_read_job, slave_write_buffer_test_single_byte_for_multi_read_job); + static void i2c_slave_repeat_read(void) { unity_wait_for_signal("i2c master init first");