From 60d949bf7b57ffbc045697a993b6935382ed7844 Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Sun, 26 Oct 2025 08:21:06 +1000 Subject: [PATCH 1/7] WIP --- esphome/components/usb_host/usb_host.h | 10 ++-- .../components/usb_host/usb_host_client.cpp | 54 +++++++++---------- esphome/components/usb_uart/usb_uart.cpp | 14 ++--- 3 files changed, 39 insertions(+), 39 deletions(-) diff --git a/esphome/components/usb_host/usb_host.h b/esphome/components/usb_host/usb_host.h index 43b24a54a5..cfc92bc637 100644 --- a/esphome/components/usb_host/usb_host.h +++ b/esphome/components/usb_host/usb_host.h @@ -55,7 +55,7 @@ static const uint8_t USB_DIR_IN = 1 << 7; static const uint8_t USB_DIR_OUT = 0; static const size_t SETUP_PACKET_SIZE = 8; -static const size_t MAX_REQUESTS = USB_HOST_MAX_REQUESTS; // maximum number of outstanding requests possible. +static constexpr size_t MAX_REQUESTS = USB_HOST_MAX_REQUESTS; // maximum number of outstanding requests possible. static_assert(MAX_REQUESTS >= 1 && MAX_REQUESTS <= 32, "MAX_REQUESTS must be between 1 and 32"); // Select appropriate bitmask type for tracking allocation of TransferRequest slots. @@ -133,9 +133,8 @@ class USBClient : public Component { float get_setup_priority() const override { return setup_priority::IO; } void on_opened(uint8_t addr); void on_removed(usb_device_handle_t handle); - void control_transfer_callback(const usb_transfer_t *xfer) const; - void transfer_in(uint8_t ep_address, const transfer_cb_t &callback, uint16_t length); - void transfer_out(uint8_t ep_address, const transfer_cb_t &callback, const uint8_t *data, uint16_t length); + bool transfer_in(uint8_t ep_address, const transfer_cb_t &callback, uint16_t length); + bool transfer_out(uint8_t ep_address, const transfer_cb_t &callback, const uint8_t *data, uint16_t length); void dump_config() override; void release_trq(TransferRequest *trq); bool control_transfer(uint8_t type, uint8_t request, uint16_t value, uint16_t index, const transfer_cb_t &callback, @@ -147,7 +146,6 @@ class USBClient : public Component { EventPool event_pool; protected: - bool register_(); TransferRequest *get_trq_(); // Lock-free allocation using atomic bitmask (multi-consumer safe) virtual void disconnect(); virtual void on_connected() {} @@ -158,7 +156,7 @@ class USBClient : public Component { // USB task management static void usb_task_fn(void *arg); - void usb_task_loop(); + [[noreturn]] void usb_task_loop() const; TaskHandle_t usb_task_handle_{nullptr}; diff --git a/esphome/components/usb_host/usb_host_client.cpp b/esphome/components/usb_host/usb_host_client.cpp index 2139ed869a..cc0c932503 100644 --- a/esphome/components/usb_host/usb_host_client.cpp +++ b/esphome/components/usb_host/usb_host_client.cpp @@ -188,9 +188,9 @@ void USBClient::setup() { } // Pre-allocate USB transfer buffers for all slots at startup // This avoids any dynamic allocation during runtime - for (size_t i = 0; i < MAX_REQUESTS; i++) { - usb_host_transfer_alloc(64, 0, &this->requests_[i].transfer); - this->requests_[i].client = this; // Set once, never changes + for (auto &request : this->requests_) { + usb_host_transfer_alloc(64, 0, &request.transfer); + request.client = this; // Set once, never changes } // Create and start USB task @@ -210,8 +210,7 @@ void USBClient::usb_task_fn(void *arg) { auto *client = static_cast(arg); client->usb_task_loop(); } - -void USBClient::usb_task_loop() { +void USBClient::usb_task_loop() const { while (true) { usb_host_client_handle_events(this->handle_, portMAX_DELAY); } @@ -334,22 +333,23 @@ static void control_callback(const usb_transfer_t *xfer) { // This multi-threaded access is intentional for performance - USB task can // immediately restart transfers without waiting for main loop scheduling. TransferRequest *USBClient::get_trq_() { - trq_bitmask_t mask = this->trq_in_use_.load(std::memory_order_relaxed); + trq_bitmask_t mask = this->trq_in_use_.load(std::memory_order_acquire); // Find first available slot (bit = 0) and try to claim it atomically // We use a while loop to allow retrying the same slot after CAS failure - size_t i = 0; - while (i != MAX_REQUESTS) { - if (mask & (static_cast(1) << i)) { - // Slot is in use, move to next slot - i++; - continue; + for (;;) { + if (mask == (1 << MAX_REQUESTS) - 1) { + ESP_LOGE(TAG, "All %zu transfer slots in use", MAX_REQUESTS); + return nullptr; } + // find the least significant zero bit + trq_bitmask_t lsb = ~mask & (mask + 1); // Slot i appears available, try to claim it atomically - trq_bitmask_t desired = mask | (static_cast(1) << i); // Set bit i to mark as in-use + trq_bitmask_t desired = mask | lsb; - if (this->trq_in_use_.compare_exchange_weak(mask, desired, std::memory_order_acquire, std::memory_order_relaxed)) { + if (this->trq_in_use_.compare_exchange_weak(mask, desired)) { + auto i = __builtin_ctz(lsb); // count trailing zeroes // Successfully claimed slot i - prepare the TransferRequest auto *trq = &this->requests_[i]; trq->transfer->context = trq; @@ -358,13 +358,9 @@ TransferRequest *USBClient::get_trq_() { } // CAS failed - another thread modified the bitmask // mask was already updated by compare_exchange_weak with the current value - // No need to reload - the CAS already did that for us - i = 0; } - - ESP_LOGE(TAG, "All %zu transfer slots in use", MAX_REQUESTS); - return nullptr; } + void USBClient::disconnect() { this->on_disconnected(); auto err = usb_host_device_close(this->handle_, this->device_handle_); @@ -446,11 +442,11 @@ static void transfer_callback(usb_transfer_t *xfer) { * * @throws None. */ -void USBClient::transfer_in(uint8_t ep_address, const transfer_cb_t &callback, uint16_t length) { +bool USBClient::transfer_in(uint8_t ep_address, const transfer_cb_t &callback, uint16_t length) { auto *trq = this->get_trq_(); if (trq == nullptr) { ESP_LOGE(TAG, "Too many requests queued"); - return; + return false; } trq->callback = callback; trq->transfer->callback = transfer_callback; @@ -460,7 +456,9 @@ void USBClient::transfer_in(uint8_t ep_address, const transfer_cb_t &callback, u if (err != ESP_OK) { ESP_LOGE(TAG, "Failed to submit transfer, address=%x, length=%d, err=%x", ep_address, length, err); this->release_trq(trq); + return false; } + return true; } /** @@ -476,11 +474,11 @@ void USBClient::transfer_in(uint8_t ep_address, const transfer_cb_t &callback, u * * @throws None. */ -void USBClient::transfer_out(uint8_t ep_address, const transfer_cb_t &callback, const uint8_t *data, uint16_t length) { +bool USBClient::transfer_out(uint8_t ep_address, const transfer_cb_t &callback, const uint8_t *data, uint16_t length) { auto *trq = this->get_trq_(); if (trq == nullptr) { ESP_LOGE(TAG, "Too many requests queued"); - return; + return false; } trq->callback = callback; trq->transfer->callback = transfer_callback; @@ -491,7 +489,9 @@ void USBClient::transfer_out(uint8_t ep_address, const transfer_cb_t &callback, if (err != ESP_OK) { ESP_LOGE(TAG, "Failed to submit transfer, address=%x, length=%d, err=%x", ep_address, length, err); this->release_trq(trq); + return false; } + return true; } void USBClient::dump_config() { ESP_LOGCONFIG(TAG, @@ -505,7 +505,7 @@ void USBClient::dump_config() { // - Main loop: When transfer submission fails // // THREAD SAFETY: Lock-free using atomic AND to clear bit -// Thread-safe atomic operation allows multi-threaded deallocation +// Thread-safe atomic operation allows multithreaded deallocation void USBClient::release_trq(TransferRequest *trq) { if (trq == nullptr) return; @@ -517,10 +517,10 @@ void USBClient::release_trq(TransferRequest *trq) { return; } - // Atomically clear bit i to mark slot as available + // Atomically clear the bit to mark slot as available // fetch_and with inverted bitmask clears the bit atomically - trq_bitmask_t bit = static_cast(1) << index; - this->trq_in_use_.fetch_and(static_cast(~bit), std::memory_order_release); + trq_bitmask_t mask = ~(static_cast(1) << index); + this->trq_in_use_.fetch_and(mask, std::memory_order_release); } } // namespace usb_host diff --git a/esphome/components/usb_uart/usb_uart.cpp b/esphome/components/usb_uart/usb_uart.cpp index 29003e071e..86a47e6f56 100644 --- a/esphome/components/usb_uart/usb_uart.cpp +++ b/esphome/components/usb_uart/usb_uart.cpp @@ -231,7 +231,7 @@ void USBUartComponent::start_input(USBUartChannel *channel) { auto callback = [this, channel](const usb_host::TransferStatus &status) { ESP_LOGV(TAG, "Transfer result: length: %u; status %X", status.data_len, status.error_code); if (!status.success) { - ESP_LOGE(TAG, "Control transfer failed, status=%s", esp_err_to_name(status.error_code)); + ESP_LOGE(TAG, "Input transfer failed, status=%s", esp_err_to_name(status.error_code)); // On failure, don't restart - let next read_array() trigger it channel->input_started_.store(false); return; @@ -264,7 +264,9 @@ void USBUartComponent::start_input(USBUartChannel *channel) { this->start_input(channel); }; channel->input_started_.store(true); - this->transfer_in(ep->bEndpointAddress, callback, ep->wMaxPacketSize); + if (!this->transfer_in(ep->bEndpointAddress, callback, ep->wMaxPacketSize)) { + channel->input_started_.store(false); + } } void USBUartComponent::start_output(USBUartChannel *channel) { @@ -328,7 +330,7 @@ void USBUartTypeCdcAcm::on_connected() { channel->cdc_dev_ = cdc_devs[i++]; fix_mps(channel->cdc_dev_.in_ep); fix_mps(channel->cdc_dev_.out_ep); - channel->initialised_.store(true); + channel->initialised_ = true; auto err = usb_host_interface_claim(this->handle_, this->device_handle_, channel->cdc_dev_.bulk_interface_number, 0); if (err != ESP_OK) { @@ -357,11 +359,11 @@ void USBUartTypeCdcAcm::on_disconnected() { usb_host_endpoint_flush(this->device_handle_, channel->cdc_dev_.notify_ep->bEndpointAddress); } usb_host_interface_release(this->handle_, this->device_handle_, channel->cdc_dev_.bulk_interface_number); - channel->initialised_.store(false); - channel->input_started_.store(false); - channel->output_started_.store(false); + channel->input_started_.store(true); + channel->output_started_.store(true); channel->input_buffer_.clear(); channel->output_buffer_.clear(); + channel->initialised_.store(false); } USBClient::on_disconnected(); } From 5d170da76275e45e5014e7faa3a077c32e513d61 Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Sun, 26 Oct 2025 09:45:49 +1000 Subject: [PATCH 2/7] Add instrumentation --- esphome/components/usb_host/usb_host.h | 1 + 1 file changed, 1 insertion(+) diff --git a/esphome/components/usb_host/usb_host.h b/esphome/components/usb_host/usb_host.h index cfc92bc637..5e9866f381 100644 --- a/esphome/components/usb_host/usb_host.h +++ b/esphome/components/usb_host/usb_host.h @@ -137,6 +137,7 @@ class USBClient : public Component { bool transfer_out(uint8_t ep_address, const transfer_cb_t &callback, const uint8_t *data, uint16_t length); void dump_config() override; void release_trq(TransferRequest *trq); + trq_bitmask_t get_trq_in_use() const { return trq_in_use_; } bool control_transfer(uint8_t type, uint8_t request, uint16_t value, uint16_t index, const transfer_cb_t &callback, const std::vector &data = {}); From 28ee05b1a36b2895d79ce7a7df4e0b42042ab1b8 Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Sun, 26 Oct 2025 09:51:15 +1000 Subject: [PATCH 3/7] Revert incorrect change --- esphome/components/usb_uart/usb_uart.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/components/usb_uart/usb_uart.cpp b/esphome/components/usb_uart/usb_uart.cpp index 86a47e6f56..60c4720e96 100644 --- a/esphome/components/usb_uart/usb_uart.cpp +++ b/esphome/components/usb_uart/usb_uart.cpp @@ -330,7 +330,7 @@ void USBUartTypeCdcAcm::on_connected() { channel->cdc_dev_ = cdc_devs[i++]; fix_mps(channel->cdc_dev_.in_ep); fix_mps(channel->cdc_dev_.out_ep); - channel->initialised_ = true; + channel->initialised_.store(true); auto err = usb_host_interface_claim(this->handle_, this->device_handle_, channel->cdc_dev_.bulk_interface_number, 0); if (err != ESP_OK) { From c3606a9229e492c4349a5efdf1af1c32927bc56f Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Sun, 26 Oct 2025 10:05:44 +1000 Subject: [PATCH 4/7] Fix race condition in start_input --- esphome/components/usb_uart/usb_uart.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/esphome/components/usb_uart/usb_uart.cpp b/esphome/components/usb_uart/usb_uart.cpp index 60c4720e96..86d4f4078e 100644 --- a/esphome/components/usb_uart/usb_uart.cpp +++ b/esphome/components/usb_uart/usb_uart.cpp @@ -214,7 +214,7 @@ void USBUartComponent::dump_config() { } } void USBUartComponent::start_input(USBUartChannel *channel) { - if (!channel->initialised_.load() || channel->input_started_.load()) + if (!channel->initialised_.load()) return; // THREAD CONTEXT: Called from both USB task and main loop threads // - USB task: Immediate restart after successful transfer for continuous data flow @@ -226,6 +226,12 @@ void USBUartComponent::start_input(USBUartChannel *channel) { // // The underlying transfer_in() uses lock-free atomic allocation from the // TransferRequest pool, making this multi-threaded access safe + + // if already started, don't restart. A spurious failure in compare_exchange_weak + // is not a problem, as it will be retried on the next read_array() + auto started = false; + if (!channel->input_started_.compare_exchange_weak(started, true)) + return; const auto *ep = channel->cdc_dev_.in_ep; // CALLBACK CONTEXT: This lambda is executed in USB task via transfer_callback auto callback = [this, channel](const usb_host::TransferStatus &status) { @@ -263,7 +269,6 @@ void USBUartComponent::start_input(USBUartChannel *channel) { channel->input_started_.store(false); this->start_input(channel); }; - channel->input_started_.store(true); if (!this->transfer_in(ep->bEndpointAddress, callback, ep->wMaxPacketSize)) { channel->input_started_.store(false); } From 733001bf651aaa01ca931bbc414e849588988104 Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Tue, 28 Oct 2025 08:32:24 +1000 Subject: [PATCH 5/7] Fix warning about shift overflow --- esphome/components/usb_host/usb_host.h | 1 + esphome/components/usb_host/usb_host_client.cpp | 2 +- tests/components/usb_uart/common.yaml | 3 +++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/esphome/components/usb_host/usb_host.h b/esphome/components/usb_host/usb_host.h index 5e9866f381..31bdde2df8 100644 --- a/esphome/components/usb_host/usb_host.h +++ b/esphome/components/usb_host/usb_host.h @@ -65,6 +65,7 @@ static_assert(MAX_REQUESTS >= 1 && MAX_REQUESTS <= 32, "MAX_REQUESTS must be bet // This is tied to the static_assert above, which enforces MAX_REQUESTS is between 1 and 32. // If MAX_REQUESTS is increased above 32, this logic and the static_assert must be updated. using trq_bitmask_t = std::conditional<(MAX_REQUESTS <= 16), uint16_t, uint32_t>::type; +static constexpr trq_bitmask_t ALL_REQUESTS_IN_USE = MAX_REQUESTS == 32 ? ~0 : (1 << MAX_REQUESTS) - 1; static constexpr size_t USB_EVENT_QUEUE_SIZE = 32; // Size of event queue between USB task and main loop static constexpr size_t USB_TASK_STACK_SIZE = 4096; // Stack size for USB task (same as ESP-IDF USB examples) diff --git a/esphome/components/usb_host/usb_host_client.cpp b/esphome/components/usb_host/usb_host_client.cpp index cc0c932503..2456b0c742 100644 --- a/esphome/components/usb_host/usb_host_client.cpp +++ b/esphome/components/usb_host/usb_host_client.cpp @@ -338,7 +338,7 @@ TransferRequest *USBClient::get_trq_() { // Find first available slot (bit = 0) and try to claim it atomically // We use a while loop to allow retrying the same slot after CAS failure for (;;) { - if (mask == (1 << MAX_REQUESTS) - 1) { + if (mask == ALL_REQUESTS_IN_USE) { ESP_LOGE(TAG, "All %zu transfer slots in use", MAX_REQUESTS); return nullptr; } diff --git a/tests/components/usb_uart/common.yaml b/tests/components/usb_uart/common.yaml index 46ad6291f9..474c3f5c8d 100644 --- a/tests/components/usb_uart/common.yaml +++ b/tests/components/usb_uart/common.yaml @@ -1,3 +1,6 @@ +usb_host: + max_transfer_requests: 32 + usb_uart: - id: uart_0 type: cdc_acm From d6c23ac0563acfba946fadde1a0183bd23bd0aa1 Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Thu, 30 Oct 2025 05:38:16 +1000 Subject: [PATCH 6/7] Add clarifying comment --- esphome/components/usb_uart/usb_uart.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/esphome/components/usb_uart/usb_uart.cpp b/esphome/components/usb_uart/usb_uart.cpp index 86d4f4078e..c24fffb11d 100644 --- a/esphome/components/usb_uart/usb_uart.cpp +++ b/esphome/components/usb_uart/usb_uart.cpp @@ -364,6 +364,7 @@ void USBUartTypeCdcAcm::on_disconnected() { usb_host_endpoint_flush(this->device_handle_, channel->cdc_dev_.notify_ep->bEndpointAddress); } usb_host_interface_release(this->handle_, this->device_handle_, channel->cdc_dev_.bulk_interface_number); + // Reset the input and output started flags to their initial state to avoid the possibility of spurious restarts channel->input_started_.store(true); channel->output_started_.store(true); channel->input_buffer_.clear(); From a21057a744cbdbc8318afd9de1f7c59b97c18de3 Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Thu, 30 Oct 2025 06:04:33 +1000 Subject: [PATCH 7/7] Relax memory order to acquire --- esphome/components/usb_host/usb_host_client.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/components/usb_host/usb_host_client.cpp b/esphome/components/usb_host/usb_host_client.cpp index 2456b0c742..dc216a209d 100644 --- a/esphome/components/usb_host/usb_host_client.cpp +++ b/esphome/components/usb_host/usb_host_client.cpp @@ -348,7 +348,7 @@ TransferRequest *USBClient::get_trq_() { // Slot i appears available, try to claim it atomically trq_bitmask_t desired = mask | lsb; - if (this->trq_in_use_.compare_exchange_weak(mask, desired)) { + if (this->trq_in_use_.compare_exchange_weak(mask, desired, std::memory_order::acquire)) { auto i = __builtin_ctz(lsb); // count trailing zeroes // Successfully claimed slot i - prepare the TransferRequest auto *trq = &this->requests_[i];