mirror/esphome
1
0
Fork 0
mirror of https://github.com/esphome/esphome.git synced 2026-02-28 18:04:19 -07:00
Code Issues Packages Projects Releases Wiki Activity

[uart] Remove redundant mutex, fix rx_event_task race, optimize event queue

Browse Source 
Remove the FreeRTOS mutex (lock_) that wrapped all ESP-IDF UART driver
calls. A codebase audit confirmed all UART wrapper methods (read_array,
write_array, peek_byte, available, flush) are called exclusively from
the cooperative main loop. The lock was protecting single-threaded code
from itself and added 4 unnecessary semaphore operations per read_array
call on the hot path.

Fix a race condition in rx_event_task_func where uart_flush_input() was
called from a separate FreeRTOS task without taking the lock, racing
with read_array() on the main loop. This could destroy ring buffer data
mid-read, causing data loss. The flush was also unnecessary since the
ESP-IDF driver self-heals the buffer-full condition: uart_read_bytes()
internally calls uart_check_buf_full() which moves stashed FIFO bytes
back into the ring buffer and re-enables RX interrupts.

Move the event queue allocation behind USE_UART_WAKE_LOOP_ON_RX. When
the feature is disabled, uart_driver_install is called with queue_size=0
and uart_queue=NULL (the documented API for skipping the event queue).
This saves ~320 bytes of RAM per UART instance and reduces ISR overhead
since the driver skips xQueueSendFromISR when no queue exists.

Add thread safety documentation to the class and rx_event_task.
This commit is contained in:
J. Nick Koston
2026-02-12 08:19:42 -06:00
parent e12ed08487
commit 6d1fa9fa72
2 changed files with 31 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

39
esphome/components/uart/uart_component_esp_idf.cpp
View File

@@ -90,7 +90,6 @@ void IDFUARTComponent::setup() {
return;
}
this->uart_num_ = static_cast<uart_port_t>(next_uart_num++);
this->lock_ = xSemaphoreCreateMutex();
#if (SOC_UART_LP_NUM >= 1)
size_t fifo_len = ((this->uart_num_ < SOC_UART_HP_NUM) ? SOC_UART_FIFO_LEN : SOC_LP_UART_FIFO_LEN);
@@ -102,11 +101,7 @@ void IDFUARTComponent::setup() {
this->rx_buffer_size_ = fifo_len * 2;
}
xSemaphoreTake(this->lock_, portMAX_DELAY);
this->load_settings(false);
xSemaphoreGive(this->lock_);
}
void IDFUARTComponent::load_settings(bool dump_config) {
@@ -126,13 +121,20 @@ void IDFUARTComponent::load_settings(bool dump_config) {
return;
}
}
#ifdef USE_UART_WAKE_LOOP_ON_RX
constexpr int event_queue_size = 20;
QueueHandle_t *event_queue_ptr = &this->uart_event_queue_;
#else
constexpr int event_queue_size = 0;
QueueHandle_t *event_queue_ptr = nullptr;
#endif
err = uart_driver_install(this->uart_num_, // UART number
this->rx_buffer_size_, // RX ring buffer size
0, // TX ring buffer size. If zero, driver will not use a TX buffer and TX function will
// block task until all data has been sent out
20, // event queue size/depth
&this->uart_event_queue_, // event queue
0 // Flags used to allocate the interrupt
0, // TX ring buffer size. If zero, driver will not use a TX buffer and TX function will
// block task until all data has been sent out
event_queue_size, // event queue size/depth
event_queue_ptr, // event queue
0 // Flags used to allocate the interrupt
);
if (err != ESP_OK) {
ESP_LOGW(TAG, "uart_driver_install failed: %s", esp_err_to_name(err));
@@ -282,9 +284,7 @@ void IDFUARTComponent::set_rx_timeout(size_t rx_timeout) {
}
void IDFUARTComponent::write_array(const uint8_t *data, size_t len) {
xSemaphoreTake(this->lock_, portMAX_DELAY);
int32_t write_len = uart_write_bytes(this->uart_num_, data, len);
xSemaphoreGive(this->lock_);
if (write_len != (int32_t) len) {
ESP_LOGW(TAG, "uart_write_bytes failed: %d != %zu", write_len, len);
this->mark_failed();
@@ -299,7 +299,6 @@ void IDFUARTComponent::write_array(const uint8_t *data, size_t len) {
bool IDFUARTComponent::peek_byte(uint8_t *data) {
if (!this->check_read_timeout_())
return false;
xSemaphoreTake(this->lock_, portMAX_DELAY);
if (this->has_peek_) {
*data = this->peek_byte_;
} else {
@@ -311,7 +310,6 @@ bool IDFUARTComponent::peek_byte(uint8_t *data) {
this->peek_byte_ = *data;
}
}
xSemaphoreGive(this->lock_);
return true;
}
@@ -320,7 +318,6 @@ bool IDFUARTComponent::read_array(uint8_t *data, size_t len) {
int32_t read_len = 0;
if (!this->check_read_timeout_(len))
return false;
xSemaphoreTake(this->lock_, portMAX_DELAY);
if (this->has_peek_) {
length_to_read--;
*data = this->peek_byte_;
@@ -329,7 +326,6 @@ bool IDFUARTComponent::read_array(uint8_t *data, size_t len) {
}
if (length_to_read > 0)
read_len = uart_read_bytes(this->uart_num_, data, length_to_read, 20 / portTICK_PERIOD_MS);
xSemaphoreGive(this->lock_);
#ifdef USE_UART_DEBUGGER
for (size_t i = 0; i < len; i++) {
this->debug_callback_.call(UART_DIRECTION_RX, data[i]);
@@ -342,9 +338,7 @@ size_t IDFUARTComponent::available() {
size_t available = 0;
esp_err_t err;
xSemaphoreTake(this->lock_, portMAX_DELAY);
err = uart_get_buffered_data_len(this->uart_num_, &available);
xSemaphoreGive(this->lock_);
if (err != ESP_OK) {
ESP_LOGW(TAG, "uart_get_buffered_data_len failed: %s", esp_err_to_name(err));
@@ -358,9 +352,7 @@ size_t IDFUARTComponent::available() {
void IDFUARTComponent::flush() {
ESP_LOGVV(TAG, " Flushing");
xSemaphoreTake(this->lock_, portMAX_DELAY);
uart_wait_tx_done(this->uart_num_, portMAX_DELAY);
xSemaphoreGive(this->lock_);
}
void IDFUARTComponent::check_logger_conflict() {}
@@ -405,8 +397,11 @@ void IDFUARTComponent::rx_event_task_func(void *param) {
case UART_FIFO_OVF:
case UART_BUFFER_FULL:
ESP_LOGW(TAG, "FIFO overflow or ring buffer full - clearing");
uart_flush_input(self->uart_num_);
// Don't call uart_flush_input() here - it races with read_array() on the main loop
// and can destroy data mid-read. The driver self-heals: uart_read_bytes() internally
// calls uart_check_buf_full() which moves stashed FIFO bytes into the ring buffer
// and re-enables RX interrupts once space is freed by normal reads.
ESP_LOGW(TAG, "FIFO overflow or ring buffer full");
#if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE)
App.wake_loop_threadsafe();
#endif

17
esphome/components/uart/uart_component_esp_idf.h
View File

@@ -8,6 +8,13 @@
namespace esphome::uart {
/// ESP-IDF UART driver wrapper.
///
/// Thread safety: All public methods (read_array, write_array, peek_byte, available, flush,
/// load_settings) must only be called from the main loop. The ESP-IDF UART driver API does not
/// guarantee thread safety, and ESPHome's peek byte state (has_peek_/peek_byte_) is not
/// synchronized. The rx_event_task (when enabled) must not call any of these methods — it
/// communicates with the main loop exclusively via App.wake_loop_threadsafe().
class IDFUARTComponent : public UARTComponent, public Component {
public:
void setup() override;
@@ -26,7 +33,9 @@ class IDFUARTComponent : public UARTComponent, public Component {
void flush() override;
uint8_t get_hw_serial_number() { return this->uart_num_; }
#ifdef USE_UART_WAKE_LOOP_ON_RX
QueueHandle_t *get_uart_event_queue() { return &this->uart_event_queue_; }
#endif
/**
* Load the UART with the current settings.
@@ -46,18 +55,20 @@ class IDFUARTComponent : public UARTComponent, public Component {
protected:
void check_logger_conflict() override;
uart_port_t uart_num_;
QueueHandle_t uart_event_queue_;
uart_config_t get_config_();
SemaphoreHandle_t lock_;
bool has_peek_{false};
uint8_t peek_byte_;
#ifdef USE_UART_WAKE_LOOP_ON_RX
// RX notification support
// RX notification support — runs on a separate FreeRTOS task.
// IMPORTANT: rx_event_task_func must NOT call any UART wrapper methods (read_array,
// write_array, etc.) or touch has_peek_/peek_byte_. It must only read from the
// event queue and call App.wake_loop_threadsafe().
void start_rx_event_task_();
static void rx_event_task_func(void *param);
QueueHandle_t uart_event_queue_;
TaskHandle_t rx_event_task_handle_{nullptr};
#endif // USE_UART_WAKE_LOOP_ON_RX
};