From 27690e09902ec8948a1b96ef2b5c97c75a08f3e5 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Thu, 14 Mar 2024 17:40:25 -0700 Subject: [PATCH 1/6] fix(i2c_master): fix deadlock on s_i2c_transaction_start failure As pointed out in PR #13134 by @MatthiasKunnen, there is a deadlock in `s_i2c_synchronous_transaction()` if `s_i2c_transaction_start()` should fail because, on error, s_i2c_synchronous_transaction() returns before releasing the lock. This commit fixes the deadlock without any other changes. Closes: #13387 Signed-off-by: Eric Wheeler --- components/driver/i2c/i2c_master.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/components/driver/i2c/i2c_master.c b/components/driver/i2c/i2c_master.c index 342fe2c63f..ea618ae419 100644 --- a/components/driver/i2c/i2c_master.c +++ b/components/driver/i2c/i2c_master.c @@ -761,6 +761,7 @@ static esp_err_t s_i2c_asynchronous_transaction(i2c_master_dev_handle_t i2c_dev, static esp_err_t s_i2c_synchronous_transaction(i2c_master_dev_handle_t i2c_dev, i2c_operation_t *i2c_ops, size_t ops_dim, int timeout_ms) { + esp_err_t ret = ESP_OK; i2c_dev->master_bus->trans_done = false; TickType_t ticks_to_wait = (timeout_ms == -1) ? portMAX_DELAY : pdMS_TO_TICKS(timeout_ms); if (xSemaphoreTake(i2c_dev->master_bus->bus_lock_mux, ticks_to_wait) != pdTRUE) { @@ -777,9 +778,11 @@ static esp_err_t s_i2c_synchronous_transaction(i2c_master_dev_handle_t i2c_dev, i2c_dev->master_bus->sent_all = false; i2c_dev->master_bus->trans_finish = false; i2c_dev->master_bus->queue_trans = false; - ESP_RETURN_ON_ERROR(s_i2c_transaction_start(i2c_dev, timeout_ms), TAG, "I2C transaction failed"); + ESP_GOTO_ON_ERROR(s_i2c_transaction_start(i2c_dev, timeout_ms), err, TAG, "I2C transaction failed"); + +err: xSemaphoreGive(i2c_dev->master_bus->bus_lock_mux); - return ESP_OK; + return ret; } esp_err_t i2c_new_master_bus(const i2c_master_bus_config_t *bus_config, i2c_master_bus_handle_t *ret_bus_handle) From f93ebedcb6ff7233e0bcdcf17725b4e8361db20e Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Wed, 6 Mar 2024 19:04:12 +0800 Subject: [PATCH 2/6] fix(i2c): Fix I2C synchronous transaction cost so much CPU source, Closes https://github.com/espressif/esp-idf/issues/13137, Closes https://github.com/espressif/esp-idf/pull/13322 --- components/driver/i2c/i2c_master.c | 36 ++++++++++++++----- components/driver/i2c/i2c_private.h | 1 + .../driver/i2c/include/driver/i2c_types.h | 7 ++-- 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/components/driver/i2c/i2c_master.c b/components/driver/i2c/i2c_master.c index ea618ae419..94889e6f48 100644 --- a/components/driver/i2c/i2c_master.c +++ b/components/driver/i2c/i2c_master.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -392,6 +392,9 @@ static void s_i2c_send_commands(i2c_master_bus_handle_t i2c_master, TickType_t t uint8_t fifo_fill = 0; uint8_t address_fill = 0; + // Initialise event queue. + xQueueReset(i2c_master->event_queue); + i2c_master->event = I2C_EVENT_ALIVE; while (i2c_master->i2c_trans.cmd_count) { if (xSemaphoreTake(i2c_master->cmd_semphr, ticks_to_wait) != pdTRUE) { // Software timeout, clear the command link and finish this transaction. @@ -429,10 +432,17 @@ static void s_i2c_send_commands(i2c_master_bus_handle_t i2c_master, TickType_t t } i2c_hal_master_trans_start(hal); - // For blocking implementation, we must wait `done` interrupt to update the status. - while (i2c_master->trans_done == false) {}; - if (i2c_master->status != I2C_STATUS_ACK_ERROR && i2c_master->status != I2C_STATUS_TIMEOUT) { - atomic_store(&i2c_master->status, I2C_STATUS_DONE); + // For blocking implementation, we must wait event interrupt to update the status. + // Otherwise, update status to timeout. + i2c_master_event_t event; + if (xQueueReceive(i2c_master->event_queue, &event, ticks_to_wait) == pdTRUE) { + if (event == I2C_EVENT_DONE) { + atomic_store(&i2c_master->status, I2C_STATUS_DONE); + } + } else { + i2c_master->cmd_idx = 0; + i2c_master->trans_idx = 0; + atomic_store(&i2c_master->status, I2C_STATUS_TIMEOUT); } xSemaphoreGive(i2c_master->cmd_semphr); } @@ -573,15 +583,19 @@ static void IRAM_ATTR i2c_master_isr_handler_default(void *arg) return; } - if (int_mask == I2C_LL_INTR_NACK) { + if (int_mask & I2C_LL_INTR_NACK) { atomic_store(&i2c_master->status, I2C_STATUS_ACK_ERROR); i2c_master->event = I2C_EVENT_NACK; - } else if (int_mask == I2C_LL_INTR_TIMEOUT || int_mask == I2C_LL_INTR_ARBITRATION) { + } else if (int_mask & I2C_LL_INTR_TIMEOUT || int_mask & I2C_LL_INTR_ARBITRATION) { atomic_store(&i2c_master->status, I2C_STATUS_TIMEOUT); - } else if (int_mask == I2C_LL_INTR_MST_COMPLETE) { + i2c_master->event = I2C_EVENT_TIMEOUT; + } else if (int_mask & I2C_LL_INTR_MST_COMPLETE) { i2c_master->trans_done = true; i2c_master->event = I2C_EVENT_DONE; } + if (i2c_master->event != I2C_EVENT_ALIVE) { + xQueueSendFromISR(i2c_master->event_queue, (void *)&i2c_master->event, &HPTaskAwoken); + } if (i2c_master->contains_read == true) { i2c_isr_receive_handler(i2c_master); } @@ -672,6 +686,9 @@ static esp_err_t i2c_master_bus_destroy(i2c_master_bus_handle_t bus_handle) vSemaphoreDeleteWithCaps(i2c_master->cmd_semphr); i2c_master->cmd_semphr = NULL; } + if (i2c_master->event_queue) { + vQueueDeleteWithCaps(i2c_master->event_queue); + } if (i2c_master->queues_storage) { free(i2c_master->queues_storage); } @@ -817,6 +834,9 @@ esp_err_t i2c_new_master_bus(const i2c_master_bus_config_t *bus_config, i2c_mast i2c_master->cmd_semphr = xSemaphoreCreateBinaryWithCaps(I2C_MEM_ALLOC_CAPS); ESP_GOTO_ON_FALSE(i2c_master->cmd_semphr, ESP_ERR_NO_MEM, err, TAG, "no memory for i2c semaphore struct"); + i2c_master->event_queue = xQueueCreateWithCaps(1, sizeof(i2c_master_event_t), I2C_MEM_ALLOC_CAPS); + ESP_GOTO_ON_FALSE(i2c_master->event_queue, ESP_ERR_NO_MEM, err, TAG, "no memory for i2c queue struct"); + portENTER_CRITICAL(&i2c_master->base->spinlock); i2c_ll_clear_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); portEXIT_CRITICAL(&i2c_master->base->spinlock); diff --git a/components/driver/i2c/i2c_private.h b/components/driver/i2c/i2c_private.h index 8d4688562d..7ceec24da2 100644 --- a/components/driver/i2c/i2c_private.h +++ b/components/driver/i2c/i2c_private.h @@ -118,6 +118,7 @@ struct i2c_master_bus_t { i2c_operation_t i2c_ops[I2C_STATIC_OPERATION_ARRAY_MAX]; // I2C operation array _Atomic uint16_t trans_idx; // Index of I2C transaction command. SemaphoreHandle_t cmd_semphr; // Semaphore between task and interrupt, using for synchronizing ISR and I2C task. + QueueHandle_t event_queue; // I2C event queue uint32_t read_buf_pos; // Read buffer position bool contains_read; // Whether command array includes read operation, true: yes, otherwise, false. uint32_t read_len_static; // Read static buffer length diff --git a/components/driver/i2c/include/driver/i2c_types.h b/components/driver/i2c/include/driver/i2c_types.h index 81b3c239c1..7c5e4c1720 100644 --- a/components/driver/i2c/include/driver/i2c_types.h +++ b/components/driver/i2c/include/driver/i2c_types.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -34,11 +34,14 @@ typedef enum { I2C_STATUS_TIMEOUT, /*!< I2C bus status error, and operation timeout */ } i2c_master_status_t; - +/** + * @brief Enumeration for I2C event. + */ typedef enum { I2C_EVENT_ALIVE, /*!< i2c bus in alive status.*/ I2C_EVENT_DONE, /*!< i2c bus transaction done */ I2C_EVENT_NACK, /*!< i2c bus nack */ + I2C_EVENT_TIMEOUT, /*!< i2c bus timeout */ } i2c_master_event_t; /** From 8567102be40e57f83d2d049c3c70181cf825de34 Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Tue, 2 Jan 2024 17:55:09 +0800 Subject: [PATCH 3/6] fix(i2c_master): Fix issue that use callback may cause memory leak, Closes https://github.com/espressif/esp-idf/issues/12878 --- components/driver/i2c/i2c_master.c | 175 +++++++++--------- components/driver/i2c/i2c_private.h | 10 +- .../i2c_test_apps/main/test_i2c_common.c | 83 ++++++++- 3 files changed, 171 insertions(+), 97 deletions(-) diff --git a/components/driver/i2c/i2c_master.c b/components/driver/i2c/i2c_master.c index 94889e6f48..f4c30846a9 100644 --- a/components/driver/i2c/i2c_master.c +++ b/components/driver/i2c/i2c_master.c @@ -149,7 +149,7 @@ static bool s_i2c_write_command(i2c_master_bus_handle_t i2c_master, i2c_operatio i2c_master->trans_idx++; } portENTER_CRITICAL_SAFE(&handle->spinlock); - if (i2c_master->asnyc_trans == false) { + if (i2c_master->async_trans == false) { i2c_hal_master_trans_start(hal); } else { i2c_master->async_break = true; @@ -163,7 +163,7 @@ static bool s_i2c_write_command(i2c_master_bus_handle_t i2c_master, i2c_operatio i2c_ll_master_write_cmd_reg(hal->dev, hw_end_cmd, i2c_master->cmd_idx + 1); portEXIT_CRITICAL_SAFE(&handle->spinlock); i2c_master->cmd_idx = 0; - if (i2c_master->asnyc_trans == false) { + if (i2c_master->async_trans == false) { i2c_hal_master_trans_start(hal); } else { i2c_master->async_break = true; @@ -172,7 +172,7 @@ static bool s_i2c_write_command(i2c_master_bus_handle_t i2c_master, i2c_operatio i2c_master->cmd_idx++; i2c_master->trans_idx++; i2c_master->i2c_trans.cmd_count--; - if (i2c_master->asnyc_trans == false) { + if (i2c_master->async_trans == false) { if (xPortInIsrContext()) { xSemaphoreGiveFromISR(i2c_master->cmd_semphr, do_yield); } else { @@ -230,7 +230,7 @@ static bool s_i2c_read_command(i2c_master_bus_handle_t i2c_master, i2c_operation } i2c_master->trans_idx++; i2c_master->i2c_trans.cmd_count--; - if (i2c_master->asnyc_trans == false) { + if (i2c_master->async_trans == false) { if (xPortInIsrContext()) { xSemaphoreGiveFromISR(i2c_master->cmd_semphr, do_yield); } else { @@ -242,7 +242,7 @@ static bool s_i2c_read_command(i2c_master_bus_handle_t i2c_master, i2c_operation portENTER_CRITICAL_SAFE(&handle->spinlock); 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->asnyc_trans == false) { + if (i2c_master->async_trans == false) { i2c_hal_master_trans_start(hal); } else { i2c_master->async_break = true; @@ -265,7 +265,7 @@ static bool s_i2c_read_command(i2c_master_bus_handle_t i2c_master, i2c_operation portEXIT_CRITICAL_SAFE(&handle->spinlock); i2c_master->status = I2C_STATUS_READ; portENTER_CRITICAL_SAFE(&handle->spinlock); - if (i2c_master->asnyc_trans == false) { + if (i2c_master->async_trans == false) { i2c_hal_master_trans_start(hal); } else { i2c_master->async_break = true; @@ -370,7 +370,7 @@ static void s_i2c_start_end_command(i2c_master_bus_handle_t i2c_master, i2c_oper *address_fill += sizeof(addr_write); portEXIT_CRITICAL_SAFE(&i2c_master->base->spinlock); } - if (i2c_master->asnyc_trans == false) { + if (i2c_master->async_trans == false) { if (xPortInIsrContext()) { xSemaphoreGiveFromISR(i2c_master->cmd_semphr, do_yield); } else { @@ -457,7 +457,10 @@ static void s_i2c_send_command_async(i2c_master_bus_handle_t i2c_master, BaseTyp i2c_master->trans_finish = true; i2c_master->in_progress = false; if (i2c_master->queue_trans) { + xSemaphoreTakeFromISR(i2c_master->bus_lock_mux, do_yield); i2c_master->new_queue = true; + i2c_master->ops_cur_size--; + xSemaphoreGiveFromISR(i2c_master->bus_lock_mux, do_yield); xQueueSendFromISR(i2c_master->trans_queues[I2C_TRANS_QUEUE_COMPLETE], &i2c_master->i2c_trans, do_yield); } i2c_master->sent_all = true; @@ -513,7 +516,7 @@ static esp_err_t s_i2c_transaction_start(i2c_master_dev_handle_t i2c_dev, int xf i2c_ll_rxfifo_rst(hal->dev); i2c_ll_enable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); portEXIT_CRITICAL(&i2c_master->base->spinlock); - if (i2c_master->asnyc_trans == true) { + if (i2c_master->async_trans == true) { s_i2c_send_command_async(i2c_master, NULL); } else { s_i2c_send_commands(i2c_master, ticks_to_wait); @@ -600,7 +603,7 @@ static void IRAM_ATTR i2c_master_isr_handler_default(void *arg) i2c_isr_receive_handler(i2c_master); } - if (i2c_master->asnyc_trans) { + if (i2c_master->async_trans) { i2c_master_dev_handle_t i2c_dev = NULL; i2c_master_device_list_t *device_item; @@ -692,22 +695,7 @@ static esp_err_t i2c_master_bus_destroy(i2c_master_bus_handle_t bus_handle) if (i2c_master->queues_storage) { free(i2c_master->queues_storage); } - if (i2c_master->i2c_anyc_ops) { - for (int i = 0; i < i2c_master->index; i++) { - if (i2c_master->i2c_anyc_ops[i]) { - free(i2c_master->i2c_anyc_ops[i]); - } - } - free(i2c_master->i2c_anyc_ops); - } - if (i2c_master->anyc_write_buffer) { - for (int i = 0; i < i2c_master->index; i++) { - if (i2c_master->anyc_write_buffer[i]) { - free(i2c_master->anyc_write_buffer[i]); - } - } - free(i2c_master->anyc_write_buffer); - } + free(i2c_master->i2c_async_ops); for (int i = 0; i < I2C_TRANS_QUEUE_MAX; i++) { if (i2c_master->trans_queues[i]) { vQueueDelete(i2c_master->trans_queues[i]); @@ -726,50 +714,70 @@ static esp_err_t i2c_master_bus_destroy(i2c_master_bus_handle_t bus_handle) static esp_err_t s_i2c_asynchronous_transaction(i2c_master_dev_handle_t i2c_dev, i2c_operation_t *i2c_ops, size_t ops_dim, int timeout_ms) { - if (i2c_dev->master_bus->sent_all == true && i2c_dev->master_bus->num_trans_inqueue == 0) { - memcpy(i2c_dev->master_bus->i2c_ops, i2c_ops, sizeof(i2c_operation_t) * ops_dim); - i2c_dev->master_bus->addr_10bits_bus = i2c_dev->addr_10bits; - i2c_dev->master_bus->i2c_trans = (i2c_transaction_t) { + i2c_master_bus_t *i2c_master = i2c_dev->master_bus; + if (i2c_master->sent_all == true && i2c_master->num_trans_inqueue == 0) { + memcpy(i2c_master->i2c_ops, i2c_ops, sizeof(i2c_operation_t) * ops_dim); + i2c_master->addr_10bits_bus = i2c_dev->addr_10bits; + i2c_master->i2c_trans = (i2c_transaction_t) { .device_address = i2c_dev->device_address, - .ops = i2c_dev->master_bus->i2c_ops, + .ops = i2c_master->i2c_ops, .cmd_count = ops_dim, }; - i2c_dev->master_bus->sent_all = false; - i2c_dev->master_bus->trans_finish = false; - i2c_dev->master_bus->queue_trans = false; + i2c_master->sent_all = false; + i2c_master->trans_finish = false; + i2c_master->queue_trans = false; ESP_RETURN_ON_ERROR(s_i2c_transaction_start(i2c_dev, timeout_ms), TAG, "I2C transaction failed"); } else { - i2c_dev->master_bus->i2c_anyc_ops[i2c_dev->master_bus->index]=(i2c_operation_t(*))heap_caps_calloc(1, sizeof(i2c_operation_t) * 6, I2C_MEM_ALLOC_CAPS); - memcpy(i2c_dev->master_bus->i2c_anyc_ops[i2c_dev->master_bus->index], i2c_ops, sizeof(i2c_operation_t) * ops_dim); + xSemaphoreTake(i2c_master->bus_lock_mux, portMAX_DELAY); + // Check whether operation pool has extra space. + bool ops_pool = (i2c_master->ops_cur_size != i2c_master->queue_size); + i2c_operation_t *ops_current; + + if (ops_pool) { + i2c_master->ops_cur_size++; + memcpy(&i2c_master->i2c_async_ops[i2c_master->ops_prepare_idx], i2c_ops, sizeof(i2c_operation_t) * ops_dim); + // Clear unused memory + uint8_t unused_dim = I2C_STATIC_OPERATION_ARRAY_MAX - ops_dim; + if (unused_dim != 0) { + memset(&i2c_master->i2c_async_ops[i2c_master->ops_prepare_idx] + sizeof(i2c_operation_t) * ops_dim, 0, sizeof(i2c_operation_t) * unused_dim); + } + // Record current operation and feed to transaction queue. + ops_current = &i2c_master->i2c_async_ops[i2c_master->ops_prepare_idx][0]; + i2c_master->ops_prepare_idx = (i2c_master->ops_prepare_idx + 1) % i2c_master->queue_size; + } + + xSemaphoreGive(i2c_master->bus_lock_mux); + ESP_RETURN_ON_FALSE(ops_pool == true, ESP_ERR_INVALID_STATE, TAG, "ops list is full, please increase your trans_queue_depth"); + i2c_transaction_t i2c_queue_pre; - if (i2c_dev->master_bus->num_trans_inflight < i2c_dev->master_bus->queue_size) { - ESP_RETURN_ON_FALSE(xQueueReceive(i2c_dev->master_bus->trans_queues[I2C_TRANS_QUEUE_READY], &i2c_queue_pre, portMAX_DELAY) == pdTRUE, ESP_FAIL, TAG, "no transaction in the ready queue"); + if (i2c_master->num_trans_inflight < i2c_master->queue_size) { + ESP_RETURN_ON_FALSE(xQueueReceive(i2c_master->trans_queues[I2C_TRANS_QUEUE_READY], &i2c_queue_pre, portMAX_DELAY) == pdTRUE, ESP_FAIL, TAG, "no transaction in the ready queue"); } else { - ESP_RETURN_ON_FALSE(xQueueReceive(i2c_dev->master_bus->trans_queues[I2C_TRANS_QUEUE_COMPLETE], &i2c_queue_pre, portMAX_DELAY) == pdTRUE, ESP_FAIL, TAG, "recycle transaction from done queue failed"); - i2c_dev->master_bus->num_trans_inflight--; + ESP_RETURN_ON_FALSE(xQueueReceive(i2c_master->trans_queues[I2C_TRANS_QUEUE_COMPLETE], &i2c_queue_pre, portMAX_DELAY) == pdTRUE, ESP_FAIL, TAG, "recycle transaction from done queue failed"); + i2c_master->num_trans_inflight--; } i2c_queue_pre = (i2c_transaction_t) { .device_address = i2c_dev->device_address, - .ops = i2c_dev->master_bus->i2c_anyc_ops[i2c_dev->master_bus->index], + .ops = ops_current, .cmd_count = ops_dim, }; - i2c_dev->master_bus->index++; - if (xQueueSend(i2c_dev->master_bus->trans_queues[I2C_TRANS_QUEUE_PROGRESS], &i2c_queue_pre, portMAX_DELAY) == pdTRUE) { - i2c_dev->master_bus->num_trans_inflight++; - i2c_dev->master_bus->num_trans_inqueue++; - if (i2c_dev->master_bus->sent_all == true) { + if (xQueueSend(i2c_master->trans_queues[I2C_TRANS_QUEUE_PROGRESS], &i2c_queue_pre, portMAX_DELAY) == pdTRUE) { + i2c_master->num_trans_inflight++; + i2c_master->num_trans_inqueue++; + if (i2c_master->sent_all == true) { // Oh no, you cannot get the queue from ISR, so you get queue here. - ESP_RETURN_ON_FALSE(xQueueReceive(i2c_dev->master_bus->trans_queues[I2C_TRANS_QUEUE_PROGRESS], &i2c_queue_pre, portMAX_DELAY) == pdTRUE, ESP_FAIL, TAG, "get trans from progress queue failed"); - i2c_dev->master_bus->num_trans_inflight--; - i2c_dev->master_bus->num_trans_inqueue--; - i2c_dev->master_bus->sent_all = false; - i2c_dev->master_bus->trans_finish = false; - i2c_dev->master_bus->queue_trans = false; + ESP_RETURN_ON_FALSE(xQueueReceive(i2c_master->trans_queues[I2C_TRANS_QUEUE_PROGRESS], &i2c_queue_pre, portMAX_DELAY) == pdTRUE, ESP_FAIL, TAG, "get trans from progress queue failed"); + i2c_master->ops_cur_size--; + i2c_master->num_trans_inflight--; + i2c_master->num_trans_inqueue--; + i2c_master->sent_all = false; + i2c_master->trans_finish = false; + i2c_master->queue_trans = false; ESP_RETURN_ON_ERROR(s_i2c_transaction_start(i2c_dev, timeout_ms), TAG, "I2C transaction failed"); } } else { - ESP_RETURN_ON_FALSE(xQueueSend(i2c_dev->master_bus->trans_queues[I2C_TRANS_QUEUE_READY], &i2c_queue_pre, 0) == pdTRUE, ESP_ERR_INVALID_STATE, TAG, "ready queue full"); + ESP_RETURN_ON_FALSE(xQueueSend(i2c_master->trans_queues[I2C_TRANS_QUEUE_READY], &i2c_queue_pre, 0) == pdTRUE, ESP_ERR_INVALID_STATE, TAG, "ready queue full"); } } @@ -848,10 +856,9 @@ esp_err_t i2c_new_master_bus(const i2c_master_bus_config_t *bus_config, i2c_mast xSemaphoreTake(i2c_master->bus_lock_mux, portMAX_DELAY); SLIST_INIT(&i2c_master->device_list); xSemaphoreGive(i2c_master->bus_lock_mux); - // Initialize the queue if (bus_config->trans_queue_depth) { - i2c_master->asnyc_trans = true; + i2c_master->async_trans = true; i2c_master->sent_all = true; i2c_master->trans_finish = true; i2c_master->new_queue = true; @@ -873,8 +880,10 @@ esp_err_t i2c_new_master_bus(const i2c_master_bus_config_t *bus_config, i2c_mast ESP_ERR_INVALID_STATE, TAG, "ready queue full"); } - i2c_master->i2c_anyc_ops = (i2c_operation_t(**))heap_caps_calloc(bus_config->trans_queue_depth, sizeof(i2c_operation_t*), I2C_MEM_ALLOC_CAPS); - i2c_master->anyc_write_buffer = (uint8_t**)heap_caps_calloc(bus_config->trans_queue_depth, sizeof(uint8_t*), I2C_MEM_ALLOC_CAPS); + i2c_master->i2c_async_ops = (i2c_operation_t(*)[I2C_STATIC_OPERATION_ARRAY_MAX])heap_caps_calloc(bus_config->trans_queue_depth, sizeof(*i2c_master->i2c_async_ops), I2C_MEM_ALLOC_CAPS); + ESP_RETURN_ON_FALSE(i2c_master->i2c_async_ops, ESP_ERR_NO_MEM, TAG, "no mem for operations"); + i2c_master->ops_prepare_idx = 0; + } int isr_flags = I2C_INTR_ALLOC_FLAG; if (bus_config->intr_priority) { @@ -980,21 +989,15 @@ esp_err_t i2c_master_transmit(i2c_master_dev_handle_t i2c_dev, const uint8_t *wr ESP_RETURN_ON_FALSE(i2c_dev != NULL, ESP_ERR_INVALID_ARG, TAG, "i2c handle not initialized"); ESP_RETURN_ON_FALSE((write_buffer != NULL) && (write_size > 0), ESP_ERR_INVALID_ARG, TAG, "i2c transmit buffer or size invalid"); - if (i2c_dev->master_bus->asnyc_trans == false) { - i2c_operation_t i2c_ops[] = { - {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, - {.hw_cmd = I2C_TRANS_STOP_COMMAND}, - }; + i2c_operation_t i2c_ops[] = { + {.hw_cmd = I2C_TRANS_START_COMMAND}, + {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, + {.hw_cmd = I2C_TRANS_STOP_COMMAND}, + }; + + if (i2c_dev->master_bus->async_trans == false) { ESP_RETURN_ON_ERROR(s_i2c_synchronous_transaction(i2c_dev, i2c_ops, DIM(i2c_ops), xfer_timeout_ms), TAG, "I2C transaction failed"); } else { - i2c_dev->master_bus->anyc_write_buffer[i2c_dev->master_bus->index] = (uint8_t*)heap_caps_calloc(1, sizeof(uint8_t)*write_size, I2C_MEM_ALLOC_CAPS); - memcpy(i2c_dev->master_bus->anyc_write_buffer[i2c_dev->master_bus->index], write_buffer, write_size); - i2c_operation_t i2c_ops[] = { - {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)i2c_dev->master_bus->anyc_write_buffer[i2c_dev->master_bus->index], .total_bytes = write_size}, - {.hw_cmd = I2C_TRANS_STOP_COMMAND}, - }; ESP_RETURN_ON_ERROR(s_i2c_asynchronous_transaction(i2c_dev, i2c_ops, DIM(i2c_ops), xfer_timeout_ms), TAG, "I2C transaction failed"); } return ESP_OK; @@ -1006,28 +1009,18 @@ esp_err_t i2c_master_transmit_receive(i2c_master_dev_handle_t i2c_dev, const uin ESP_RETURN_ON_FALSE((write_buffer != NULL) && (write_size > 0), ESP_ERR_INVALID_ARG, TAG, "i2c transmit buffer or size invalid"); ESP_RETURN_ON_FALSE((read_buffer != NULL) && (read_size > 0), ESP_ERR_INVALID_ARG, TAG, "i2c receive buffer or size invalid"); - if (i2c_dev->master_bus->asnyc_trans == false) { - i2c_operation_t i2c_ops[] = { - {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, - {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_READ_COMMAND(ACK_VAL), .data = read_buffer, .total_bytes = read_size - 1}, - {.hw_cmd = I2C_TRANS_READ_COMMAND(NACK_VAL), .data = (read_buffer + read_size - 1), .total_bytes = 1}, - {.hw_cmd = I2C_TRANS_STOP_COMMAND}, - }; + i2c_operation_t i2c_ops[] = { + {.hw_cmd = I2C_TRANS_START_COMMAND}, + {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, + {.hw_cmd = I2C_TRANS_START_COMMAND}, + {.hw_cmd = I2C_TRANS_READ_COMMAND(ACK_VAL), .data = read_buffer, .total_bytes = read_size - 1}, + {.hw_cmd = I2C_TRANS_READ_COMMAND(NACK_VAL), .data = (read_buffer + read_size - 1), .total_bytes = 1}, + {.hw_cmd = I2C_TRANS_STOP_COMMAND}, + }; + + if (i2c_dev->master_bus->async_trans == false) { ESP_RETURN_ON_ERROR(s_i2c_synchronous_transaction(i2c_dev, i2c_ops, DIM(i2c_ops), xfer_timeout_ms), TAG, "I2C transaction failed"); } else { - i2c_dev->master_bus->anyc_write_buffer[i2c_dev->master_bus->index] = (uint8_t*)heap_caps_calloc(1, sizeof(uint8_t)*write_size, I2C_MEM_ALLOC_CAPS); - memcpy(i2c_dev->master_bus->anyc_write_buffer[i2c_dev->master_bus->index], write_buffer, write_size); - - i2c_operation_t i2c_ops[] = { - {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)i2c_dev->master_bus->anyc_write_buffer[i2c_dev->master_bus->index], .total_bytes = write_size}, - {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_READ_COMMAND(ACK_VAL), .data = read_buffer, .total_bytes = read_size - 1}, - {.hw_cmd = I2C_TRANS_READ_COMMAND(NACK_VAL), .data = (read_buffer + read_size - 1), .total_bytes = 1}, - {.hw_cmd = I2C_TRANS_STOP_COMMAND}, - }; ESP_RETURN_ON_ERROR(s_i2c_asynchronous_transaction(i2c_dev, i2c_ops, DIM(i2c_ops), xfer_timeout_ms), TAG, "I2C transaction failed"); } return ESP_OK; @@ -1045,7 +1038,7 @@ esp_err_t i2c_master_receive(i2c_master_dev_handle_t i2c_dev, uint8_t *read_buff {.hw_cmd = I2C_TRANS_STOP_COMMAND}, }; - if (i2c_dev->master_bus->asnyc_trans == false) { + if (i2c_dev->master_bus->async_trans == false) { ESP_RETURN_ON_ERROR(s_i2c_synchronous_transaction(i2c_dev, i2c_ops, DIM(i2c_ops), xfer_timeout_ms), TAG, "I2C transaction failed"); } else { ESP_RETURN_ON_ERROR(s_i2c_asynchronous_transaction(i2c_dev, i2c_ops, DIM(i2c_ops), xfer_timeout_ms), TAG, "I2C transaction failed"); @@ -1097,7 +1090,7 @@ esp_err_t i2c_master_register_event_callbacks(i2c_master_dev_handle_t i2c_dev, c { ESP_RETURN_ON_FALSE(i2c_dev != NULL, ESP_ERR_INVALID_ARG, TAG, "i2c handle not initialized"); - if (i2c_dev->master_bus->asnyc_trans == false) { + if (i2c_dev->master_bus->async_trans == false) { ESP_LOGE(TAG, "I2C transaction queue is not initialized, so you can't use callback here, please resister the bus again with trans_queue_depth != 0"); return ESP_ERR_INVALID_STATE; } diff --git a/components/driver/i2c/i2c_private.h b/components/driver/i2c/i2c_private.h index 7ceec24da2..4e14b26757 100644 --- a/components/driver/i2c/i2c_private.h +++ b/components/driver/i2c/i2c_private.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -124,7 +124,7 @@ struct i2c_master_bus_t { uint32_t read_len_static; // Read static buffer length uint32_t w_r_size; // The size send/receive last time. bool trans_over_buffer; // Data length is more than hardware fifo length, needs interrupt. - bool asnyc_trans; // asynchronous transaction, true after callback is installed. + bool async_trans; // asynchronous transaction, true after callback is installed. volatile bool trans_done; // transaction command finish SLIST_HEAD(i2c_master_device_list_head, i2c_master_device_list) device_list; // I2C device (instance) list // asnyc trans members @@ -139,11 +139,11 @@ struct i2c_master_bus_t { bool trans_finish; // true if current command has been sent out. bool queue_trans; // true if current transaction is in queue bool new_queue; // true if allow a new queue transaction - size_t index; // transaction index QueueHandle_t trans_queues[I2C_TRANS_QUEUE_MAX]; // transaction queues. StaticQueue_t trans_queue_structs[I2C_TRANS_QUEUE_MAX]; // memory to store the static structure for trans_queues - i2c_operation_t **i2c_anyc_ops; // pointer to asynchronous operation. - uint8_t **anyc_write_buffer; // pointer to asynchronous write buffer. + i2c_operation_t (*i2c_async_ops)[I2C_STATIC_OPERATION_ARRAY_MAX]; // pointer to asynchronous operation(s). + uint32_t ops_prepare_idx; // Index for the operations can be written into `i2c_async_ops` array. + uint32_t ops_cur_size; // Indicates how many operations have already put in `i2c_async_ops`. i2c_transaction_t i2c_trans_pool[]; // I2C transaction pool. }; diff --git a/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c b/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c index ee307c32a3..af6aa29285 100644 --- a/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c +++ b/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -148,3 +148,84 @@ TEST_CASE("I2C master probe device test", "[i2c]") TEST_ESP_ERR(i2c_master_probe(bus_handle, 0x55, -1), ESP_ERR_NOT_FOUND); TEST_ESP_OK(i2c_del_master_bus(bus_handle)); } + +#define LENGTH 48 + +static IRAM_ATTR bool test_master_tx_done_callback(i2c_master_dev_handle_t i2c_dev, const i2c_master_event_data_t *evt_data, void *arg) +{ + return true; +} + +/******************************************************************************* + * + * This test aim to test I2C non-blocking transaction function. Several things have been + * done in this test for testing its memory/concurrency issues. + * + * 1. See the depth of queue is 37, but the number of transaction is 42, that means some + * queue must be reused. + * 2. There are some delay randomly set there, for testing the concurency or any I2C state + * might be met. + ******************************************************************************* +*/ +TEST_CASE("I2C master transaction non-blocking mode with large amount of transaction", "[i2c]") +{ + i2c_master_bus_config_t i2c_bus_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, + .glitch_ignore_cnt = 7, + .trans_queue_depth = 37, + .flags.enable_internal_pullup = true, + }; + + i2c_master_bus_handle_t bus_handle; + + TEST_ESP_OK(i2c_new_master_bus(&i2c_bus_config, &bus_handle)); + + i2c_device_config_t dev_cfg = { + .dev_addr_length = I2C_ADDR_BIT_LEN_7, + .device_address = 0x58, + .scl_speed_hz = 400000, + }; + + i2c_master_dev_handle_t dev_handle; + TEST_ESP_OK(i2c_master_bus_add_device(bus_handle, &dev_cfg, &dev_handle)); + + i2c_master_event_callbacks_t cbs = { + .on_trans_done = test_master_tx_done_callback, + }; + i2c_master_register_event_callbacks(dev_handle, &cbs, NULL); + + uint32_t cnt = 7; + uint8_t *buf[6]; + for (int i = 0; i < 6; i++) { + buf[i] = (uint8_t*)heap_caps_calloc(1, LENGTH, MALLOC_CAP_8BIT); + for (int j = 0; j < LENGTH; j++) { + buf[i][j] = i + j; + } + } + while (cnt--) { + i2c_master_transmit(dev_handle, buf[0], LENGTH, -1); + esp_rom_delay_us(1000); + i2c_master_transmit(dev_handle, buf[1], LENGTH, -1); + esp_rom_delay_us(500); + i2c_master_transmit(dev_handle, buf[2], LENGTH, -1); + esp_rom_delay_us(200); + i2c_master_transmit(dev_handle, buf[3], LENGTH, -1); + esp_rom_delay_us(400); + i2c_master_transmit(dev_handle, buf[4], LENGTH, -1); + esp_rom_delay_us(700); + i2c_master_transmit(dev_handle, buf[5], LENGTH, -1); + esp_rom_delay_us(200); + } + + i2c_master_bus_wait_all_done(bus_handle, -1); + for (int i = 0; i < 6; i++) { + if (buf[i]) { + free(buf[i]); + } + } + TEST_ESP_OK(i2c_master_bus_rm_device(dev_handle)); + TEST_ESP_OK(i2c_del_master_bus(bus_handle)); +} From 90afc33cd8deedf5bb5b62c9d50b0eb2885f9681 Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Fri, 15 Mar 2024 14:06:52 +0800 Subject: [PATCH 4/6] fix(i2c_master): Fix issue that initialize esp32 and using i2c_master_probe issue, and probe might failed. Fixed I2C cannot return err code when nack detected Closes https://github.com/espressif/esp-idf/issues/13213, Closes https://github.com/espressif/esp-idf/issues/12929, Closes https://github.com/espressif/esp-idf/issues/13398, --- components/driver/i2c/i2c_master.c | 53 ++++++++--- components/driver/i2c/i2c_private.h | 2 + .../driver/i2c/include/driver/i2c_master.h | 18 +++- .../i2c_test_apps/main/test_i2c_common.c | 89 ++++++++++++++++++- .../i2c_test_apps/main/test_i2c_multi.c | 54 +++++++++++ docs/en/api-reference/peripherals/i2c.rst | 4 + 6 files changed, 204 insertions(+), 16 deletions(-) diff --git a/components/driver/i2c/i2c_master.c b/components/driver/i2c/i2c_master.c index f4c30846a9..115f7b830c 100644 --- a/components/driver/i2c/i2c_master.c +++ b/components/driver/i2c/i2c_master.c @@ -42,7 +42,7 @@ static const char *TAG = "i2c.master"; static esp_err_t s_i2c_master_clear_bus(i2c_bus_handle_t handle) { -#if !SOC_I2C_SUPPORT_HW_FSM_RST +#if !SOC_I2C_SUPPORT_HW_CLR_BUS const int scl_half_period = 5; // use standard 100kHz data rate int i = 0; gpio_set_direction(handle->scl_num, GPIO_MODE_OUTPUT_OD); @@ -340,7 +340,7 @@ static void s_i2c_start_end_command(i2c_master_bus_handle_t i2c_master, i2c_oper } #endif i2c_ll_hw_cmd_t hw_write_cmd = { - .ack_en = false, + .ack_en = i2c_master->ack_check_disable ? false : true, .op_code = I2C_LL_CMD_WRITE, .byte_num = 1, }; @@ -407,11 +407,13 @@ static void s_i2c_send_commands(i2c_master_bus_handle_t i2c_master, TickType_t t s_i2c_hw_fsm_reset(i2c_master); i2c_master->cmd_idx = 0; i2c_master->trans_idx = 0; + ESP_LOGE(TAG, "I2C hardware timeout detected"); xSemaphoreGive(i2c_master->cmd_semphr); return; } if (i2c_master->status == I2C_STATUS_ACK_ERROR) { + ESP_LOGE(TAG, "I2C hardware NACK detected"); const i2c_ll_hw_cmd_t hw_stop_cmd = { .op_code = I2C_LL_CMD_STOP, }; @@ -466,6 +468,20 @@ static void s_i2c_send_command_async(i2c_master_bus_handle_t i2c_master, BaseTyp i2c_master->sent_all = true; return; } + + // Stop the transaction when invalid event is detected + if (i2c_master->event == I2C_EVENT_NACK || i2c_master->event == I2C_EVENT_TIMEOUT) { + i2c_master->sent_all = true; + i2c_master->trans_finish = true; + i2c_master->in_progress = false; + if (i2c_master->queue_trans) { + i2c_master->ops_cur_size--; + xQueueSendFromISR(i2c_master->trans_queues[I2C_TRANS_QUEUE_COMPLETE], &i2c_master->i2c_trans, do_yield); + } + i2c_master->i2c_trans.cmd_count = 0; + i2c_master->event = I2C_EVENT_ALIVE; + return; + } while(i2c_ll_is_bus_busy(hal->dev)){} while (i2c_master->i2c_trans.cmd_count && !needs_start) { @@ -524,6 +540,8 @@ static esp_err_t s_i2c_transaction_start(i2c_master_dev_handle_t i2c_dev, int xf if (i2c_master->status != I2C_STATUS_DONE) { ret = ESP_ERR_INVALID_STATE; } + // Interrupt can be disabled when on transaction finishes. + i2c_ll_disable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); } if (i2c_master->base->pm_lock) { @@ -643,7 +661,6 @@ static void IRAM_ATTR i2c_master_isr_handler_default(void *arg) i2c_master->read_len_static = 0; i2c_ll_txfifo_rst(hal->dev); i2c_ll_rxfifo_rst(hal->dev); - i2c_ll_enable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); i2c_master->i2c_trans = t; memcpy(i2c_master->i2c_ops, t.ops, t.cmd_count * sizeof(i2c_operation_t)); @@ -803,9 +820,12 @@ static esp_err_t s_i2c_synchronous_transaction(i2c_master_dev_handle_t i2c_dev, i2c_dev->master_bus->sent_all = false; i2c_dev->master_bus->trans_finish = false; i2c_dev->master_bus->queue_trans = false; + i2c_dev->master_bus->ack_check_disable = i2c_dev->ack_check_disable; ESP_GOTO_ON_ERROR(s_i2c_transaction_start(i2c_dev, timeout_ms), err, TAG, "I2C transaction failed"); err: + // When error occurs, reset hardware fsm in case not influence following transactions. + s_i2c_hw_fsm_reset(i2c_dev->master_bus); xSemaphoreGive(i2c_dev->master_bus->bus_lock_mux); return ret; } @@ -833,6 +853,10 @@ esp_err_t i2c_new_master_bus(const i2c_master_bus_config_t *bus_config, i2c_mast i2c_master->base->scl_num = bus_config->scl_io_num; i2c_master->base->sda_num = bus_config->sda_io_num; i2c_master->base->pull_up_enable = bus_config->flags.enable_internal_pullup; + + if (i2c_master->base->pull_up_enable == false) { + ESP_LOGW(TAG, "Please check pull-up resistances whether be connected properly. Otherwise unexpected behavior would happen. For more detailed information, please read docs"); + } ESP_GOTO_ON_ERROR(i2c_param_master_config(i2c_master->base, bus_config), err, TAG, "i2c configure parameter failed"); i2c_master->bus_lock_mux = xSemaphoreCreateBinaryWithCaps(I2C_MEM_ALLOC_CAPS); @@ -858,6 +882,7 @@ esp_err_t i2c_new_master_bus(const i2c_master_bus_config_t *bus_config, i2c_mast xSemaphoreGive(i2c_master->bus_lock_mux); // Initialize the queue if (bus_config->trans_queue_depth) { + ESP_LOGW(TAG, "Please note i2c asynchronous is only used for specific scenario currently. It's experimental for other users because user cannot get bus error from API. And It's not compatible with ``i2c_master_probe``. If user makes sure there won't be any error on bus and tested with no problem, this message can be ignored."); i2c_master->async_trans = true; i2c_master->sent_all = true; i2c_master->trans_finish = true; @@ -893,7 +918,6 @@ esp_err_t i2c_new_master_bus(const i2c_master_bus_config_t *bus_config, i2c_mast ESP_GOTO_ON_ERROR(ret, err, TAG, "install i2c master interrupt failed"); atomic_init(&i2c_master->status, I2C_STATUS_IDLE); - i2c_ll_enable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); i2c_ll_master_set_filter(hal->dev, bus_config->glitch_ignore_cnt); xSemaphoreGive(i2c_master->cmd_semphr); @@ -925,6 +949,7 @@ esp_err_t i2c_master_bus_add_device(i2c_master_bus_handle_t bus_handle, const i2 i2c_dev->scl_speed_hz = dev_config->scl_speed_hz; i2c_dev->addr_10bits = dev_config->dev_addr_length; i2c_dev->master_bus = i2c_master; + i2c_dev->ack_check_disable = dev_config->flags.disable_ack_check; i2c_master_device_list_t *device_item = (i2c_master_device_list_t *)calloc(1, sizeof(i2c_master_device_list_t)); ESP_GOTO_ON_FALSE((device_item != NULL), ESP_ERR_NO_MEM, err, TAG, "no memory for i2c device item`"); @@ -991,7 +1016,7 @@ esp_err_t i2c_master_transmit(i2c_master_dev_handle_t i2c_dev, const uint8_t *wr i2c_operation_t i2c_ops[] = { {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, + {.hw_cmd = I2C_TRANS_WRITE_COMMAND(i2c_dev->ack_check_disable ? false : true), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, {.hw_cmd = I2C_TRANS_STOP_COMMAND}, }; @@ -1011,7 +1036,7 @@ esp_err_t i2c_master_transmit_receive(i2c_master_dev_handle_t i2c_dev, const uin i2c_operation_t i2c_ops[] = { {.hw_cmd = I2C_TRANS_START_COMMAND}, - {.hw_cmd = I2C_TRANS_WRITE_COMMAND(false), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, + {.hw_cmd = I2C_TRANS_WRITE_COMMAND(i2c_dev->ack_check_disable ? false : true), .data = (uint8_t *)write_buffer, .total_bytes = write_size}, {.hw_cmd = I2C_TRANS_START_COMMAND}, {.hw_cmd = I2C_TRANS_READ_COMMAND(ACK_VAL), .data = read_buffer, .total_bytes = read_size - 1}, {.hw_cmd = I2C_TRANS_READ_COMMAND(NACK_VAL), .data = (read_buffer + read_size - 1), .total_bytes = 1}, @@ -1053,6 +1078,7 @@ esp_err_t i2c_master_probe(i2c_master_bus_handle_t bus_handle, uint16_t address, if (xSemaphoreTake(bus_handle->bus_lock_mux, ticks_to_wait) != pdTRUE) { return ESP_ERR_TIMEOUT; } + esp_err_t ret = ESP_OK; bus_handle->cmd_idx = 0; bus_handle->trans_idx = 0; @@ -1073,17 +1099,22 @@ esp_err_t i2c_master_probe(i2c_master_bus_handle_t bus_handle, uint16_t address, // This will not influence device transaction. i2c_hal_set_bus_timing(hal, 100000, bus_handle->base->clk_src, bus_handle->base->clk_src_freq_hz); i2c_ll_master_set_fractional_divider(hal->dev, 0, 0); + i2c_ll_enable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); i2c_ll_update(hal->dev); s_i2c_send_commands(bus_handle, ticks_to_wait); if (bus_handle->status == I2C_STATUS_ACK_ERROR) { - // Reset the status to done, in order not influence next time transaction. - bus_handle->status = I2C_STATUS_DONE; - xSemaphoreGive(bus_handle->bus_lock_mux); - return ESP_ERR_NOT_FOUND; + ret = ESP_ERR_NOT_FOUND; + } else if (bus_handle->status == I2C_STATUS_TIMEOUT) { + ESP_LOGE(TAG, "probe device timeout. Please check if xfer_timeout_ms and pull-ups are correctly set up"); + ret = ESP_ERR_TIMEOUT; } + + // Reset the status to done, in order not influence next time transaction. + bus_handle->status = I2C_STATUS_DONE; + i2c_ll_disable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR); xSemaphoreGive(bus_handle->bus_lock_mux); - return ESP_OK; + return ret; } esp_err_t i2c_master_register_event_callbacks(i2c_master_dev_handle_t i2c_dev, const i2c_master_event_callbacks_t *cbs, void *user_data) diff --git a/components/driver/i2c/i2c_private.h b/components/driver/i2c/i2c_private.h index 4e14b26757..4e4bf1076d 100644 --- a/components/driver/i2c/i2c_private.h +++ b/components/driver/i2c/i2c_private.h @@ -125,6 +125,7 @@ struct i2c_master_bus_t { uint32_t w_r_size; // The size send/receive last time. bool trans_over_buffer; // Data length is more than hardware fifo length, needs interrupt. bool async_trans; // asynchronous transaction, true after callback is installed. + bool ack_check_disable; // Disable ACK check volatile bool trans_done; // transaction command finish SLIST_HEAD(i2c_master_device_list_head, i2c_master_device_list) device_list; // I2C device (instance) list // asnyc trans members @@ -152,6 +153,7 @@ struct i2c_master_dev_t { uint16_t device_address; // I2C device address uint32_t scl_speed_hz; // SCL clock frequency i2c_addr_bit_len_t addr_10bits; // Whether I2C device is a 10-bits address device. + bool ack_check_disable; // Disable ACK check i2c_master_callback_t on_trans_done; // I2C master transaction done callback. void *user_ctx; // Callback user context }; diff --git a/components/driver/i2c/include/driver/i2c_master.h b/components/driver/i2c/include/driver/i2c_master.h index fb78ef47b8..c0ca2e5ea5 100644 --- a/components/driver/i2c/include/driver/i2c_master.h +++ b/components/driver/i2c/include/driver/i2c_master.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -38,6 +38,9 @@ typedef struct { i2c_addr_bit_len_t dev_addr_length; /*!< Select the address length of the slave device. */ uint16_t device_address; /*!< I2C device raw address. (The 7/10 bit address without read/write bit) */ uint32_t scl_speed_hz; /*!< I2C SCL line frequency. */ + struct { + uint32_t disable_ack_check: 1; /*!< Disable ACK check. If this is set false, that means ack check is enabled, the transaction will be stoped and API returns error when nack is detected. */ + } flags; /*!< I2C device config flags */ } i2c_device_config_t; /** @@ -160,6 +163,19 @@ esp_err_t i2c_master_receive(i2c_master_dev_handle_t i2c_dev, uint8_t *read_buff * @param[in] bus_handle I2C master device handle that created by `i2c_master_bus_add_device`. * @param[in] address I2C device address that you want to probe. * @param[in] xfer_timeout_ms Wait timeout, in ms. Note: -1 means wait forever (Not recommended in this function). + * + * @attention Pull-ups must be connected to the SCL and SDA pins when this function is called. If you get `ESP_ERR_TIMEOUT + * while `xfer_timeout_ms` was parsed correctly, you should check the pull-up resistors. If you do not have proper resistors nearby. + * `flags.enable_internal_pullup` is also acceptable. + * + * @note The principle of this function is to sent device address with a write command. If the device on your I2C bus, there would be an ACK signal and function + * returns `ESP_OK`. If the device is not on your I2C bus, there would be a NACK signal and function returns `ESP_ERR_NOT_FOUND`. `ESP_ERR_TIMEOUT` is not an expected + * failure, which indicated that the i2c probe not works properly, usually caused by pull-up resistors not be connected properly. Suggestion check data on SDA/SCL line + * to see whether there is ACK/NACK signal is on line when i2c probe function fails. + * + * @note There are lots of I2C devices all over the world, we assume that not all I2C device support the behavior like `device_address+nack/ack`. + * So, if the on line data is strange and no ack/nack got respond. Please check the device datasheet. + * * @return * - ESP_OK: I2C device probe successfully * - ESP_ERR_NOT_FOUND: I2C probe failed, doesn't find the device with specific address you gave. diff --git a/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c b/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c index af6aa29285..b3e5a15e6f 100644 --- a/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c +++ b/components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c @@ -26,6 +26,24 @@ static const char TAG[] = "test-i2c"; +// Make it as a local test, because don't know where there happened to be any pull-up on CI. +TEST_CASE("I2C master initialize without pins pull-up ", "[i2c][ignore]") +{ + i2c_master_bus_config_t i2c_mst_config_1 = { + .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 = false, // no pull-up + }; + i2c_master_bus_handle_t bus_handle; + + TEST_ESP_OK(i2c_new_master_bus(&i2c_mst_config_1, &bus_handle)); + TEST_ESP_ERR(ESP_ERR_TIMEOUT, i2c_master_probe(bus_handle, 0x22, 20)); + vTaskDelay(1000); + TEST_ESP_OK(i2c_del_master_bus(bus_handle)); +} + TEST_CASE("I2C bus install-uninstall test", "[i2c]") { i2c_master_bus_config_t i2c_mst_config_1 = { @@ -142,10 +160,10 @@ TEST_CASE("I2C master probe device test", "[i2c]") i2c_master_bus_handle_t bus_handle; TEST_ESP_OK(i2c_new_master_bus(&i2c_mst_config_1, &bus_handle)); - TEST_ESP_ERR(i2c_master_probe(bus_handle, 0x22, -1), ESP_ERR_NOT_FOUND); - TEST_ESP_ERR(i2c_master_probe(bus_handle, 0x33, -1), ESP_ERR_NOT_FOUND); - TEST_ESP_ERR(i2c_master_probe(bus_handle, 0x44, -1), ESP_ERR_NOT_FOUND); - TEST_ESP_ERR(i2c_master_probe(bus_handle, 0x55, -1), ESP_ERR_NOT_FOUND); + TEST_ESP_ERR(ESP_ERR_NOT_FOUND, i2c_master_probe(bus_handle, 0x22, -1)); + TEST_ESP_ERR(ESP_ERR_NOT_FOUND, i2c_master_probe(bus_handle, 0x33, -1)); + TEST_ESP_ERR(ESP_ERR_NOT_FOUND, i2c_master_probe(bus_handle, 0x44, -1)); + TEST_ESP_ERR(ESP_ERR_NOT_FOUND, i2c_master_probe(bus_handle, 0x55, -1)); TEST_ESP_OK(i2c_del_master_bus(bus_handle)); } @@ -187,6 +205,7 @@ TEST_CASE("I2C master transaction non-blocking mode with large amount of transac .dev_addr_length = I2C_ADDR_BIT_LEN_7, .device_address = 0x58, .scl_speed_hz = 400000, + .flags.disable_ack_check = true, }; i2c_master_dev_handle_t dev_handle; @@ -229,3 +248,65 @@ TEST_CASE("I2C master transaction non-blocking mode with large amount of transac TEST_ESP_OK(i2c_master_bus_rm_device(dev_handle)); TEST_ESP_OK(i2c_del_master_bus(bus_handle)); } + +static void _test_i2c_new_bus_device(i2c_master_bus_handle_t *bus_handle, i2c_master_dev_handle_t *dev_handle) +{ + 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, + }; + + 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 = 0x58, + .scl_speed_hz = 100000, + }; + + TEST_ESP_OK(i2c_master_bus_add_device(*bus_handle, &dev_cfg, dev_handle)); +} + +static void _test_i2c_del_bus_device(i2c_master_bus_handle_t bus_handle, i2c_master_dev_handle_t dev_handle) +{ + TEST_ESP_OK(i2c_master_bus_rm_device(dev_handle)); + TEST_ESP_OK(i2c_del_master_bus(bus_handle)); +} + +TEST_CASE("I2C master transaction transmit check nack return value", "[i2c]") +{ + uint8_t data_wr[DATA_LENGTH] = { 0 }; + + i2c_master_bus_handle_t bus_handle; + i2c_master_dev_handle_t dev_handle; + _test_i2c_new_bus_device(&bus_handle, &dev_handle); + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_transmit(dev_handle, data_wr, DATA_LENGTH, -1)); + _test_i2c_del_bus_device(bus_handle, dev_handle); +} + +TEST_CASE("I2C master transaction transmit receive check nack return value", "[i2c]") +{ + uint8_t data_wr[DATA_LENGTH] = { 0 }; + uint8_t data_rd[DATA_LENGTH] = { 0 }; + + i2c_master_bus_handle_t bus_handle; + i2c_master_dev_handle_t dev_handle; + _test_i2c_new_bus_device(&bus_handle, &dev_handle); + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_transmit_receive(dev_handle, data_wr, DATA_LENGTH, data_rd, DATA_LENGTH, -1)); + _test_i2c_del_bus_device(bus_handle, dev_handle); +} + +TEST_CASE("I2C master transaction receive check nack return value", "[i2c]") +{ + uint8_t data_rd[DATA_LENGTH] = { 0 }; + + i2c_master_bus_handle_t bus_handle; + i2c_master_dev_handle_t dev_handle; + + _test_i2c_new_bus_device(&bus_handle, &dev_handle); + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_receive(dev_handle, data_rd, DATA_LENGTH, -1)); + _test_i2c_del_bus_device(bus_handle, dev_handle); +} diff --git a/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c b/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c index 2d372905bf..e66625e5b0 100644 --- a/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c +++ b/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c @@ -472,6 +472,60 @@ static void slave_write_buffer_1b_test(void) TEST_CASE_MULTIPLE_DEVICES("I2C master read slave 1 byte test", "[i2c][test_env=generic_multi_device][timeout=150]", master_read_slave_1b_test, slave_write_buffer_1b_test); +static void master_probe_slave(void) +{ + 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)); + + unity_wait_for_signal("i2c slave init finish"); + + esp_err_t ret = ESP_OK; + for (uint8_t i = 0x01; i < 0x7F; i++) { + ret = i2c_master_probe(bus_handle, i, -1); + if (ret == ESP_OK) { + printf("The slave has been found, the address is %x\n", i); + TEST_ASSERT(i == 0x58); + break; + } + TEST_ASSERT(ret == ESP_ERR_NOT_FOUND); + } + + unity_send_signal("probe finish"); + + TEST_ESP_OK(i2c_del_master_bus(bus_handle)); +} + +static void slave_init_for_probe(void) +{ + i2c_slave_config_t i2c_slv_config = { + .addr_bit_len = I2C_ADDR_BIT_LEN_7, + .clk_source = I2C_CLK_SRC_DEFAULT, + .i2c_port = TEST_I2C_PORT, + .send_buf_depth = 256, + .scl_io_num = I2C_SLAVE_SCL_IO, + .sda_io_num = I2C_SLAVE_SDA_IO, + .slave_addr = 0x58, + }; + + i2c_slave_dev_handle_t slave_handle; + TEST_ESP_OK(i2c_new_slave_device(&i2c_slv_config, &slave_handle)); + + unity_send_signal("i2c slave init finish"); + + unity_wait_for_signal("probe finish"); + + TEST_ESP_OK(i2c_del_slave_device(slave_handle)); +} + +TEST_CASE_MULTIPLE_DEVICES("I2C master probe slave test", "[i2c][test_env=generic_multi_device][timeout=150]", master_probe_slave, slave_init_for_probe); + #if SOC_I2C_NUM > 1 // Now chips with mutiple I2C controllers are up to 2, can change this to interation when we have more I2C controllers. static void i2c_master_write_test_more_port(void) diff --git a/docs/en/api-reference/peripherals/i2c.rst b/docs/en/api-reference/peripherals/i2c.rst index 127b7e0aed..7adc7dae42 100644 --- a/docs/en/api-reference/peripherals/i2c.rst +++ b/docs/en/api-reference/peripherals/i2c.rst @@ -297,6 +297,10 @@ I2C Master Probe I2C driver can use :cpp:func:`i2c_master_probe` to detect whether the specific device has been connected on I2C bus. If this function return ``ESP_OK``, that means the device has been detected. +.. important:: + + Pull-ups must be connected to the SCL and SDA pins when this function is called. If you get `ESP_ERR_TIMEOUT` while `xfer_timeout_ms` was parsed correctly, you should check the pull-up resistors. If you do not have proper resistors nearby, setting `flags.enable_internal_pullup` as true is also acceptable. + .. figure:: ../../../_static/diagrams/i2c/i2c_master_probe.png :align: center :alt: I2C master probe From 2291ded9a15cdbbf1938fe360a824dc6fa092399 Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Wed, 3 Jan 2024 16:49:12 +0800 Subject: [PATCH 5/6] fix(i2c): Use hardware fsm reset on esp32c6/h2/p4 --- components/driver/i2c/i2c_master.c | 5 +++++ components/soc/esp32c6/include/soc/Kconfig.soc_caps.in | 4 ++++ components/soc/esp32c6/include/soc/soc_caps.h | 2 +- components/soc/esp32h2/include/soc/Kconfig.soc_caps.in | 4 ++++ components/soc/esp32h2/include/soc/soc_caps.h | 2 +- components/soc/esp32p4/include/soc/Kconfig.soc_caps.in | 4 ++++ components/soc/esp32p4/include/soc/soc_caps.h | 2 +- 7 files changed, 20 insertions(+), 3 deletions(-) diff --git a/components/driver/i2c/i2c_master.c b/components/driver/i2c/i2c_master.c index 115f7b830c..ee7b18d8ac 100644 --- a/components/driver/i2c/i2c_master.c +++ b/components/driver/i2c/i2c_master.c @@ -83,6 +83,7 @@ static esp_err_t s_i2c_master_clear_bus(i2c_bus_handle_t handle) static esp_err_t s_i2c_hw_fsm_reset(i2c_master_bus_handle_t i2c_master) { i2c_hal_context_t *hal = &i2c_master->base->hal; +#if !SOC_I2C_SUPPORT_HW_FSM_RST i2c_hal_timing_config_t timing_config; uint8_t filter_cfg; @@ -99,6 +100,10 @@ static esp_err_t s_i2c_hw_fsm_reset(i2c_master_bus_handle_t i2c_master) i2c_ll_clear_intr_mask(hal->dev, I2C_LL_INTR_MASK); i2c_hal_set_timing_config(hal, &timing_config); i2c_ll_master_set_filter(hal->dev, filter_cfg); +#else + i2c_ll_master_fsm_rst(hal->dev); + s_i2c_master_clear_bus(i2c_master->base); +#endif return ESP_OK; } diff --git a/components/soc/esp32c6/include/soc/Kconfig.soc_caps.in b/components/soc/esp32c6/include/soc/Kconfig.soc_caps.in index de9120b45a..702c31dbac 100644 --- a/components/soc/esp32c6/include/soc/Kconfig.soc_caps.in +++ b/components/soc/esp32c6/include/soc/Kconfig.soc_caps.in @@ -531,6 +531,10 @@ config SOC_I2C_SUPPORT_SLAVE bool default y +config SOC_I2C_SUPPORT_HW_FSM_RST + bool + default y + config SOC_I2C_SUPPORT_HW_CLR_BUS bool default y diff --git a/components/soc/esp32c6/include/soc/soc_caps.h b/components/soc/esp32c6/include/soc/soc_caps.h index a5263b8168..64adb95636 100644 --- a/components/soc/esp32c6/include/soc/soc_caps.h +++ b/components/soc/esp32c6/include/soc/soc_caps.h @@ -237,7 +237,7 @@ #define SOC_I2C_CMD_REG_NUM (8) /*!< Number of I2C command registers */ #define SOC_I2C_SUPPORT_SLAVE (1) -// FSM_RST only resets the FSM, not using it. So SOC_I2C_SUPPORT_HW_FSM_RST not defined. +#define SOC_I2C_SUPPORT_HW_FSM_RST (1) #define SOC_I2C_SUPPORT_HW_CLR_BUS (1) #define SOC_I2C_SUPPORT_XTAL (1) diff --git a/components/soc/esp32h2/include/soc/Kconfig.soc_caps.in b/components/soc/esp32h2/include/soc/Kconfig.soc_caps.in index 1e037b674d..6b1bfc6257 100644 --- a/components/soc/esp32h2/include/soc/Kconfig.soc_caps.in +++ b/components/soc/esp32h2/include/soc/Kconfig.soc_caps.in @@ -535,6 +535,10 @@ config SOC_I2C_SUPPORT_SLAVE bool default y +config SOC_I2C_SUPPORT_HW_FSM_RST + bool + default y + config SOC_I2C_SUPPORT_HW_CLR_BUS bool default y diff --git a/components/soc/esp32h2/include/soc/soc_caps.h b/components/soc/esp32h2/include/soc/soc_caps.h index 1f03aacdeb..43f650daa5 100644 --- a/components/soc/esp32h2/include/soc/soc_caps.h +++ b/components/soc/esp32h2/include/soc/soc_caps.h @@ -241,7 +241,7 @@ #define SOC_I2C_CMD_REG_NUM (8) /*!< Number of I2C command registers */ #define SOC_I2C_SUPPORT_SLAVE (1) -// FSM_RST only resets the FSM, not using it. So SOC_I2C_SUPPORT_HW_FSM_RST not defined. +#define SOC_I2C_SUPPORT_HW_FSM_RST (1) #define SOC_I2C_SUPPORT_HW_CLR_BUS (1) #define SOC_I2C_SUPPORT_XTAL (1) diff --git a/components/soc/esp32p4/include/soc/Kconfig.soc_caps.in b/components/soc/esp32p4/include/soc/Kconfig.soc_caps.in index 9f1f9b8abb..cd18e7103f 100644 --- a/components/soc/esp32p4/include/soc/Kconfig.soc_caps.in +++ b/components/soc/esp32p4/include/soc/Kconfig.soc_caps.in @@ -479,6 +479,10 @@ config SOC_I2C_SUPPORT_SLAVE bool default y +config SOC_I2C_SUPPORT_HW_FSM_RST + bool + default y + config SOC_I2C_SUPPORT_HW_CLR_BUS bool default y diff --git a/components/soc/esp32p4/include/soc/soc_caps.h b/components/soc/esp32p4/include/soc/soc_caps.h index 4a6d2cfe79..519673b956 100644 --- a/components/soc/esp32p4/include/soc/soc_caps.h +++ b/components/soc/esp32p4/include/soc/soc_caps.h @@ -246,7 +246,7 @@ #define SOC_I2C_CMD_REG_NUM (8) /*!< Number of I2C command registers */ #define SOC_I2C_SUPPORT_SLAVE (1) -// FSM_RST only resets the FSM, not using it. So SOC_I2C_SUPPORT_HW_FSM_RST not defined. +#define SOC_I2C_SUPPORT_HW_FSM_RST (1) #define SOC_I2C_SUPPORT_HW_CLR_BUS (1) #define SOC_I2C_SUPPORT_XTAL (1) From ae94c0134b43bd90b9d110947ddf079590587908 Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Mon, 18 Mar 2024 20:21:40 +0800 Subject: [PATCH 6/6] fix(i2c_master): Fix issue that i2c clock got wrong after reset, Closes https://github.com/espressif/esp-idf/issues/13397 --- components/driver/i2c/i2c_master.c | 2 + .../i2c_test_apps/main/test_i2c_multi.c | 96 +++++++++++++++++++ components/hal/esp32/include/hal/i2c_ll.h | 40 +++++++- components/hal/esp32c2/include/hal/i2c_ll.h | 46 ++++++++- components/hal/esp32c3/include/hal/i2c_ll.h | 46 ++++++++- components/hal/esp32s2/include/hal/i2c_ll.h | 41 +++++++- components/hal/esp32s3/include/hal/i2c_ll.h | 46 ++++++++- components/hal/i2c_hal.c | 11 ++- components/hal/include/hal/i2c_hal.h | 16 +++- components/hal/include/hal/i2c_types.h | 3 +- 10 files changed, 339 insertions(+), 8 deletions(-) diff --git a/components/driver/i2c/i2c_master.c b/components/driver/i2c/i2c_master.c index ee7b18d8ac..1bbfc80a6f 100644 --- a/components/driver/i2c/i2c_master.c +++ b/components/driver/i2c/i2c_master.c @@ -827,6 +827,8 @@ static esp_err_t s_i2c_synchronous_transaction(i2c_master_dev_handle_t i2c_dev, i2c_dev->master_bus->queue_trans = false; i2c_dev->master_bus->ack_check_disable = i2c_dev->ack_check_disable; ESP_GOTO_ON_ERROR(s_i2c_transaction_start(i2c_dev, timeout_ms), err, TAG, "I2C transaction failed"); + xSemaphoreGive(i2c_dev->master_bus->bus_lock_mux); + return ret; err: // When error occurs, reset hardware fsm in case not influence following transactions. diff --git a/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c b/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c index e66625e5b0..be4c97cddf 100644 --- a/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c +++ b/components/driver/test_apps/i2c_test_apps/main/test_i2c_multi.c @@ -26,6 +26,13 @@ #include "esp_log.h" #include "test_utils.h" #include "test_board.h" +// For clock checking +#include "hal/uart_hal.h" +#include "soc/uart_periph.h" +#include "hal/clk_tree_hal.h" +#include "esp_private/gpio.h" +#include "hal/uart_ll.h" +#include "esp_clk_tree.h" void disp_buf(uint8_t *buf, int len) { @@ -609,3 +616,92 @@ static void i2c_slave_read_test_more_port(void) TEST_CASE_MULTIPLE_DEVICES("I2C master write slave test, more ports", "[i2c][test_env=generic_multi_device][timeout=150]", i2c_master_write_test_more_port, i2c_slave_read_test_more_port); #endif + +#if CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3 +// For now, we tested the chip which has such problem. +// This test can be extended to all chip when how uart baud rate +// works has been figured out. + +#if SOC_RCC_IS_INDEPENDENT +#define HP_UART_BUS_CLK_ATOMIC() +#else +#define HP_UART_BUS_CLK_ATOMIC() PERIPH_RCC_ATOMIC() +#endif + +//Init uart baud rate detection +static void uart_aut_baud_det_init(int rxd_io_num) +{ + gpio_ll_func_sel(&GPIO, rxd_io_num, PIN_FUNC_GPIO); + gpio_set_direction(rxd_io_num, GPIO_MODE_INPUT); + gpio_pullup_en(rxd_io_num); + esp_rom_gpio_connect_in_signal(rxd_io_num, UART_PERIPH_SIGNAL(1, SOC_UART_RX_PIN_IDX), 0); + HP_UART_BUS_CLK_ATOMIC() { + uart_ll_enable_bus_clock(1, true); + uart_ll_reset_register(1); + } + /* Reset all the bits */ + uart_ll_disable_intr_mask(&UART1, ~0); + uart_ll_clr_intsts_mask(&UART1, ~0); + uart_ll_set_autobaud_en(&UART1, true); +} + +static void i2c_master_write_fsm_reset(void) +{ + uint8_t data_wr[3] = { 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 = 0x58, + .scl_speed_hz = 10000, // Not a typical value for I2C + }; + + i2c_master_dev_handle_t dev_handle; + TEST_ESP_OK(i2c_master_bus_add_device(bus_handle, &dev_cfg, &dev_handle)); + + // Nack will reset the bus + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_transmit(dev_handle, data_wr, 3, -1)); + + unity_send_signal("i2c transmit fail--connect uart"); + unity_wait_for_signal("uart connected"); + + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_transmit(dev_handle, data_wr, 3, -1)); + + unity_send_signal("i2c transmit after fsm reset"); + TEST_ESP_OK(i2c_master_bus_rm_device(dev_handle)); + + TEST_ESP_OK(i2c_del_master_bus(bus_handle)); +} + +static void uart_test_i2c_master_freq(void) +{ + unity_wait_for_signal("i2c transmit fail--connect uart"); + uart_aut_baud_det_init(I2C_MASTER_SCL_IO); + + unity_send_signal("uart connected"); + unity_wait_for_signal("i2c transmit after fsm reset"); + int pospulse_cnt = uart_ll_get_pos_pulse_cnt(&UART1); + int negpulse_cnt = uart_ll_get_neg_pulse_cnt(&UART1); + // Uart uses XTAL as default clock source + int freq_hz = (clk_hal_xtal_get_freq_mhz() * 1 * 1000 * 1000) / (pospulse_cnt + negpulse_cnt); + printf("The tested I2C SCL frequency is %d\n", freq_hz); + TEST_ASSERT_INT_WITHIN(500, 10000, freq_hz); + uart_ll_set_autobaud_en(&UART1, false); + HP_UART_BUS_CLK_ATOMIC() { + uart_ll_enable_bus_clock(1, false); + } +} + +TEST_CASE_MULTIPLE_DEVICES("I2C master clock frequency test", "[i2c][test_env=generic_multi_device][timeout=150]", uart_test_i2c_master_freq, i2c_master_write_fsm_reset); + +#endif // CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3 diff --git a/components/hal/esp32/include/hal/i2c_ll.h b/components/hal/esp32/include/hal/i2c_ll.h index 9a2e89ebea..73d85a802e 100644 --- a/components/hal/esp32/include/hal/i2c_ll.h +++ b/components/hal/esp32/include/hal/i2c_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -147,6 +147,44 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d // Not supported on ESP32 } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + // Not supported on ESP32 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + // Not supported on ESP32 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + // Not supported on ESP32 +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32c2/include/hal/i2c_ll.h b/components/hal/esp32c2/include/hal/i2c_ll.h index 8958be0d2e..58d6da0f43 100644 --- a/components/hal/esp32c2/include/hal/i2c_ll.h +++ b/components/hal/esp32c2/include/hal/i2c_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -157,6 +157,50 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_b, div_b); } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + /* Set div_a and div_b to 0, as it's not necessary to use them */ + *div_a = hw->clk_conf.sclk_div_a; + *div_b = hw->clk_conf.sclk_div_b; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + *div_num = HAL_FORCE_READ_U32_REG_FIELD(hw->clk_conf, sclk_div_num); + *clk_sel = hw->clk_conf.sclk_sel; + *clk_active = hw->clk_conf.sclk_active; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_num, div_num); + hw->clk_conf.sclk_sel = clk_sel; + hw->clk_conf.sclk_active = clk_active; +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32c3/include/hal/i2c_ll.h b/components/hal/esp32c3/include/hal/i2c_ll.h index bce34f325f..3dcedf8f5a 100644 --- a/components/hal/esp32c3/include/hal/i2c_ll.h +++ b/components/hal/esp32c3/include/hal/i2c_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -171,6 +171,50 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_b, div_b); } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + /* Set div_a and div_b to 0, as it's not necessary to use them */ + *div_a = hw->clk_conf.sclk_div_a; + *div_b = hw->clk_conf.sclk_div_b; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + *div_num = HAL_FORCE_READ_U32_REG_FIELD(hw->clk_conf, sclk_div_num); + *clk_sel = hw->clk_conf.sclk_sel; + *clk_active = hw->clk_conf.sclk_active; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_num, div_num); + hw->clk_conf.sclk_sel = clk_sel; + hw->clk_conf.sclk_active = clk_active; +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32s2/include/hal/i2c_ll.h b/components/hal/esp32s2/include/hal/i2c_ll.h index d45c4cfd05..9a648beeb6 100644 --- a/components/hal/esp32s2/include/hal/i2c_ll.h +++ b/components/hal/esp32s2/include/hal/i2c_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -134,6 +134,45 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d // Not supported on ESP32S2 } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + // Not supported on ESP32S2 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + // Not supported on ESP32S2 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + // Not supported on ESP32S2 +} + + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32s3/include/hal/i2c_ll.h b/components/hal/esp32s3/include/hal/i2c_ll.h index b747db0009..e4b758c3a9 100644 --- a/components/hal/esp32s3/include/hal/i2c_ll.h +++ b/components/hal/esp32s3/include/hal/i2c_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -171,6 +171,50 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_b, div_b); } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + /* Set div_a and div_b to 0, as it's not necessary to use them */ + *div_a = hw->clk_conf.sclk_div_a; + *div_b = hw->clk_conf.sclk_div_b; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + *div_num = HAL_FORCE_READ_U32_REG_FIELD(hw->clk_conf, sclk_div_num); + *clk_sel = hw->clk_conf.sclk_sel; + *clk_active = hw->clk_conf.sclk_active; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_num, div_num); + hw->clk_conf.sclk_sel = clk_sel; + hw->clk_conf.sclk_active = clk_active; +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/i2c_hal.c b/components/hal/i2c_hal.c index 6a44f13939..abf4b03db4 100644 --- a/components/hal/i2c_hal.c +++ b/components/hal/i2c_hal.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -59,6 +59,8 @@ void i2c_hal_deinit(i2c_hal_context_t *hal) hal->dev = NULL; } +#if !SOC_I2C_SUPPORT_HW_FSM_RST + void i2c_hal_get_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t *timing_config) { i2c_ll_get_scl_clk_timing(hal->dev, &timing_config->high_period, &timing_config->low_period, &timing_config->wait_high_period); @@ -66,6 +68,8 @@ void i2c_hal_get_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t * i2c_ll_get_stop_timing(hal->dev, &timing_config->stop_setup, &timing_config->stop_hold); i2c_ll_get_sda_timing(hal->dev, &timing_config->sda_sample, &timing_config->sda_hold); i2c_ll_get_tout(hal->dev, &timing_config->timeout); + i2c_ll_master_save_clock_configurations(hal->dev, &timing_config->clk_cfg.clk_div.integer, &timing_config->clk_cfg.clk_sel, &timing_config->clk_cfg.clk_active); + i2c_ll_master_get_fractional_divider(hal->dev, &timing_config->clk_cfg.clk_div.numerator, &timing_config->clk_cfg.clk_div.denominator); } void i2c_hal_set_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t *timing_config) @@ -75,4 +79,9 @@ void i2c_hal_set_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t * i2c_ll_master_set_stop_timing(hal->dev, timing_config->stop_setup, timing_config->stop_hold); i2c_ll_set_sda_timing(hal->dev, timing_config->sda_sample, timing_config->sda_hold); i2c_ll_set_tout(hal->dev, timing_config->timeout); + i2c_ll_master_restore_clock_configurations(hal->dev, timing_config->clk_cfg.clk_div.integer, timing_config->clk_cfg.clk_sel, timing_config->clk_cfg.clk_active); + i2c_ll_master_set_fractional_divider(hal->dev, timing_config->clk_cfg.clk_div.numerator, timing_config->clk_cfg.clk_div.denominator); + } + +#endif // !SOC_I2C_SUPPORT_HW_FSM_RST diff --git a/components/hal/include/hal/i2c_hal.h b/components/hal/include/hal/i2c_hal.h index 797bf6e8ab..7b0629ee85 100644 --- a/components/hal/include/hal/i2c_hal.h +++ b/components/hal/include/hal/i2c_hal.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -33,6 +33,15 @@ typedef struct { i2c_dev_t *dev; } i2c_hal_context_t; +/** + * @brief I2C hal clock configurations + */ +typedef struct { + uint8_t clk_sel; // clock select + uint8_t clk_active; // clock active + hal_utils_clk_div_t clk_div; // clock dividers +} i2c_hal_sclk_info_t; + /** * @brief Timing configuration structure. Used for I2C reset internally. */ @@ -47,6 +56,7 @@ typedef struct { int sda_sample; /*!< high_period time */ int sda_hold; /*!< sda hold time */ int timeout; /*!< timeout value */ + i2c_hal_sclk_info_t clk_cfg; /*!< clock configuration */ } i2c_hal_timing_config_t; #if SOC_I2C_SUPPORT_SLAVE @@ -136,6 +146,8 @@ void i2c_hal_deinit(i2c_hal_context_t *hal); */ void i2c_hal_master_trans_start(i2c_hal_context_t *hal); +#if !SOC_I2C_SUPPORT_HW_FSM_RST + /** * @brief Get timing configuration * @@ -152,6 +164,8 @@ void i2c_hal_get_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t * */ void i2c_hal_set_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t *timing_config); +#endif // !SOC_I2C_SUPPORT_HW_FSM_RST + #endif // #if SOC_I2C_SUPPORTED #ifdef __cplusplus diff --git a/components/hal/include/hal/i2c_types.h b/components/hal/include/hal/i2c_types.h index fba2ad0bad..e264b2aff2 100644 --- a/components/hal/include/hal/i2c_types.h +++ b/components/hal/include/hal/i2c_types.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -14,6 +14,7 @@ extern "C" { #include #include "soc/soc_caps.h" #include "soc/clk_tree_defs.h" +#include "hal/hal_utils.h" /** * @brief I2C port number, can be I2C_NUM_0 ~ (I2C_NUM_MAX-1).