Merge branch 'fix/i2c_master_multi_read' into 'master'

fix(i2c_master): Fix that master multi-read failed

Closes IDFGH-15601

See merge request espressif/esp-idf!40336

Author: mythbuster5

components/esp_driver_i2c/i2c_master.c

@@ -7,6 +7,7 @@
 #include <string.h>
 #include <sys/param.h>
 #include <sys/lock.h>
+#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);
     }

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 */

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");