From 797330d6ab411eda34a4639b625c64e0c5306f03 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 16 Jun 2025 17:28:04 +0200 Subject: [PATCH 01/11] Disable Ethernet loop polling when connected and stable --- esphome/components/ethernet/ethernet_component.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/esphome/components/ethernet/ethernet_component.cpp b/esphome/components/ethernet/ethernet_component.cpp index fe96973924..f2e465c144 100644 --- a/esphome/components/ethernet/ethernet_component.cpp +++ b/esphome/components/ethernet/ethernet_component.cpp @@ -274,6 +274,9 @@ void EthernetComponent::loop() { ESP_LOGW(TAG, "Connection lost; reconnecting"); this->state_ = EthernetComponentState::CONNECTING; this->start_connect_(); + } else { + // When connected and stable, disable the loop to save CPU cycles + this->disable_loop(); } break; } @@ -397,11 +400,13 @@ void EthernetComponent::eth_event_handler(void *arg, esp_event_base_t event_base case ETHERNET_EVENT_START: event_name = "ETH started"; global_eth_component->started_ = true; + global_eth_component->enable_loop(); break; case ETHERNET_EVENT_STOP: event_name = "ETH stopped"; global_eth_component->started_ = false; global_eth_component->connected_ = false; + global_eth_component->enable_loop(); break; case ETHERNET_EVENT_CONNECTED: event_name = "ETH connected"; @@ -409,6 +414,7 @@ void EthernetComponent::eth_event_handler(void *arg, esp_event_base_t event_base case ETHERNET_EVENT_DISCONNECTED: event_name = "ETH disconnected"; global_eth_component->connected_ = false; + global_eth_component->enable_loop(); break; default: return; @@ -452,6 +458,8 @@ void EthernetComponent::start_connect_() { #endif /* USE_NETWORK_IPV6 */ this->connect_begin_ = millis(); this->status_set_warning("waiting for IP configuration"); + // Enable loop during connection phase + this->enable_loop(); esp_err_t err; err = esp_netif_set_hostname(this->eth_netif_, App.get_name().c_str()); @@ -620,6 +628,7 @@ bool EthernetComponent::powerdown() { } this->connected_ = false; this->started_ = false; + // No need to enable_loop() here as this is only called during shutdown/reboot if (this->phy_->pwrctl(this->phy_, false) != ESP_OK) { ESP_LOGE(TAG, "Error powering down ethernet PHY"); return false; From 1179ab33f2f5dbf067e6876d334ed6e003c29d8c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 12:51:57 +0200 Subject: [PATCH 02/11] tweaks --- .../ethernet/ethernet_component.cpp | 27 ++++++++++++------- .../components/ethernet/ethernet_component.h | 2 ++ 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/esphome/components/ethernet/ethernet_component.cpp b/esphome/components/ethernet/ethernet_component.cpp index f2e465c144..8ae15250c4 100644 --- a/esphome/components/ethernet/ethernet_component.cpp +++ b/esphome/components/ethernet/ethernet_component.cpp @@ -405,16 +405,14 @@ void EthernetComponent::eth_event_handler(void *arg, esp_event_base_t event_base case ETHERNET_EVENT_STOP: event_name = "ETH stopped"; global_eth_component->started_ = false; - global_eth_component->connected_ = false; - global_eth_component->enable_loop(); + global_eth_component->set_connected_(false); // This will enable the loop break; case ETHERNET_EVENT_CONNECTED: event_name = "ETH connected"; break; case ETHERNET_EVENT_DISCONNECTED: event_name = "ETH disconnected"; - global_eth_component->connected_ = false; - global_eth_component->enable_loop(); + global_eth_component->set_connected_(false); // This will enable the loop break; default: return; @@ -430,9 +428,9 @@ void EthernetComponent::got_ip_event_handler(void *arg, esp_event_base_t event_b ESP_LOGV(TAG, "[Ethernet event] ETH Got IP " IPSTR, IP2STR(&ip_info->ip)); global_eth_component->got_ipv4_address_ = true; #if USE_NETWORK_IPV6 && (USE_NETWORK_MIN_IPV6_ADDR_COUNT > 0) - global_eth_component->connected_ = global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT; + global_eth_component->set_connected_(global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT); #else - global_eth_component->connected_ = true; + global_eth_component->set_connected_(true); #endif /* USE_NETWORK_IPV6 */ } @@ -443,10 +441,10 @@ void EthernetComponent::got_ip6_event_handler(void *arg, esp_event_base_t event_ ESP_LOGV(TAG, "[Ethernet event] ETH Got IPv6: " IPV6STR, IPV62STR(event->ip6_info.ip)); global_eth_component->ipv6_count_ += 1; #if (USE_NETWORK_MIN_IPV6_ADDR_COUNT > 0) - global_eth_component->connected_ = - global_eth_component->got_ipv4_address_ && (global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT); + global_eth_component->set_connected_(global_eth_component->got_ipv4_address_ && + (global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT)); #else - global_eth_component->connected_ = global_eth_component->got_ipv4_address_; + global_eth_component->set_connected_(global_eth_component->got_ipv4_address_); #endif } #endif /* USE_NETWORK_IPV6 */ @@ -523,6 +521,15 @@ void EthernetComponent::start_connect_() { bool EthernetComponent::is_connected() { return this->state_ == EthernetComponentState::CONNECTED; } +void EthernetComponent::set_connected_(bool connected) { + if (this->connected_ != connected) { + this->connected_ = connected; + // Always enable loop when connection state changes + // so the state machine can process the state change + this->enable_loop(); + } +} + void EthernetComponent::dump_connect_params_() { esp_netif_ip_info_t ip; esp_netif_get_ip_info(this->eth_netif_, &ip); @@ -626,7 +633,7 @@ bool EthernetComponent::powerdown() { ESP_LOGE(TAG, "Ethernet PHY not assigned"); return false; } - this->connected_ = false; + this->set_connected_(false); this->started_ = false; // No need to enable_loop() here as this is only called during shutdown/reboot if (this->phy_->pwrctl(this->phy_, false) != ESP_OK) { diff --git a/esphome/components/ethernet/ethernet_component.h b/esphome/components/ethernet/ethernet_component.h index 7a205d89f0..ebcd4ded81 100644 --- a/esphome/components/ethernet/ethernet_component.h +++ b/esphome/components/ethernet/ethernet_component.h @@ -104,6 +104,8 @@ class EthernetComponent : public Component { void ksz8081_set_clock_reference_(esp_eth_mac_t *mac); /// @brief Set arbitratry PHY registers from config. void write_phy_register_(esp_eth_mac_t *mac, PHYRegister register_data); + /// @brief Safely set connected state and ensure loop is enabled for state machine processing + void set_connected_(bool connected); std::string use_address_; #ifdef USE_ETHERNET_SPI From ec186e632470cfeef8e1d82468c3c01f1a340ae5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 14:17:45 +0200 Subject: [PATCH 03/11] rename --- esphome/core/component.cpp | 6 +++--- esphome/core/component.h | 12 ++++++------ .../loop_test_component/loop_test_isr_component.cpp | 8 ++++---- .../loop_test_component/loop_test_isr_component.h | 2 +- tests/integration/fixtures/loop_disable_enable.yaml | 2 +- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/esphome/core/component.cpp b/esphome/core/component.cpp index f5d36e1f14..625a7b2125 100644 --- a/esphome/core/component.cpp +++ b/esphome/core/component.cpp @@ -163,15 +163,15 @@ void Component::enable_loop() { App.enable_component_loop_(this); } } -void IRAM_ATTR HOT Component::enable_loop_soon_from_isr() { - // This method is ISR-safe because: +void IRAM_ATTR HOT Component::enable_loop_soon_any_context() { + // This method is thread and ISR-safe because: // 1. Only performs simple assignments to volatile variables (atomic on all platforms) // 2. No read-modify-write operations that could be interrupted // 3. No memory allocation, object construction, or function calls // 4. IRAM_ATTR ensures code is in IRAM, not flash (required for ISR execution) // 5. Components are never destroyed, so no use-after-free concerns // 6. App is guaranteed to be initialized before any ISR could fire - // 7. Multiple ISR calls are safe - just sets the same flags to true + // 7. Multiple ISR/thread calls are safe - just sets the same flags to true // 8. Race condition with main loop is handled by clearing flag before processing this->pending_enable_loop_ = true; App.has_pending_enable_loop_requests_ = true; diff --git a/esphome/core/component.h b/esphome/core/component.h index c17eaad389..7f2bdd8414 100644 --- a/esphome/core/component.h +++ b/esphome/core/component.h @@ -171,15 +171,15 @@ class Component { */ void enable_loop(); - /** ISR-safe version of enable_loop() that can be called from interrupt context. + /** Thread and ISR-safe version of enable_loop() that can be called from any context. * * This method defers the actual enable via enable_pending_loops_ to the main loop, - * making it safe to call from ISR handlers, timer callbacks, or other - * interrupt contexts. + * making it safe to call from ISR handlers, timer callbacks, other threads, + * or any interrupt context. * * @note The actual loop enabling will happen on the next main loop iteration. * @note Only one pending enable request is tracked per component. - * @note There is no disable_loop_soon_from_isr() on purpose - it would race + * @note There is no disable_loop_soon_any_context() on purpose - it would race * against enable calls and synchronization would get too complex * to provide a safe version that would work for each component. * @@ -190,7 +190,7 @@ class Component { * disable_loop() in its next ::loop() iteration. Implementations * will need to carefully consider all possible race conditions. */ - void enable_loop_soon_from_isr(); + void enable_loop_soon_any_context(); bool is_failed() const; @@ -363,7 +363,7 @@ class Component { /// Bit 3: STATUS_LED_ERROR /// Bits 4-7: Unused - reserved for future expansion (50% of the bits are free) uint8_t component_state_{0x00}; - volatile bool pending_enable_loop_{false}; ///< ISR-safe flag for enable_loop_soon_from_isr + volatile bool pending_enable_loop_{false}; ///< ISR-safe flag for enable_loop_soon_any_context }; /** This class simplifies creating components that periodically check a state. diff --git a/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.cpp b/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.cpp index 2b0ce15060..30afec0422 100644 --- a/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.cpp +++ b/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.cpp @@ -67,10 +67,10 @@ void IRAM_ATTR LoopTestISRComponent::simulate_isr_enable() { this->isr_call_count_++; - // Call enable_loop_soon_from_isr multiple times to test that it's safe - this->enable_loop_soon_from_isr(); - this->enable_loop_soon_from_isr(); // Test multiple calls - this->enable_loop_soon_from_isr(); // Should be idempotent + // Call enable_loop_soon_any_context multiple times to test that it's safe + this->enable_loop_soon_any_context(); + this->enable_loop_soon_any_context(); // Test multiple calls + this->enable_loop_soon_any_context(); // Should be idempotent // Note: In a real ISR, we cannot use ESP_LOG* macros as they're not ISR-safe // For testing, we'll track the call count and log it from the main loop diff --git a/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.h b/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.h index 511903a613..20e11b5ecd 100644 --- a/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.h +++ b/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.h @@ -14,7 +14,7 @@ class LoopTestISRComponent : public Component { void setup() override; void loop() override; - // Simulates an ISR calling enable_loop_soon_from_isr + // Simulates an ISR calling enable_loop_soon_any_context void simulate_isr_enable(); float get_setup_priority() const override { return setup_priority::DATA; } diff --git a/tests/integration/fixtures/loop_disable_enable.yaml b/tests/integration/fixtures/loop_disable_enable.yaml index 8c69fd6181..f19d7f60ca 100644 --- a/tests/integration/fixtures/loop_disable_enable.yaml +++ b/tests/integration/fixtures/loop_disable_enable.yaml @@ -35,7 +35,7 @@ loop_test_component: test_redundant_operations: true disable_after: 10 - # ISR test component that uses enable_loop_soon_from_isr + # ISR test component that uses enable_loop_soon_any_context isr_components: - id: isr_test name: "isr_test" From 610215ab60f1e38b2d6fc55138d85ff90a084d43 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 14:24:31 +0200 Subject: [PATCH 04/11] updates --- esphome/components/gpio/binary_sensor/gpio_binary_sensor.cpp | 2 +- esphome/components/gpio/binary_sensor/gpio_binary_sensor.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/esphome/components/gpio/binary_sensor/gpio_binary_sensor.cpp b/esphome/components/gpio/binary_sensor/gpio_binary_sensor.cpp index 8832ed02c3..4b8369cd59 100644 --- a/esphome/components/gpio/binary_sensor/gpio_binary_sensor.cpp +++ b/esphome/components/gpio/binary_sensor/gpio_binary_sensor.cpp @@ -14,7 +14,7 @@ void IRAM_ATTR GPIOBinarySensorStore::gpio_intr(GPIOBinarySensorStore *arg) { arg->changed_ = true; // Wake up the component from its disabled loop state if (arg->component_ != nullptr) { - arg->component_->enable_loop_soon_from_isr(); + arg->component_->enable_loop_soon_any_context(); } } } diff --git a/esphome/components/gpio/binary_sensor/gpio_binary_sensor.h b/esphome/components/gpio/binary_sensor/gpio_binary_sensor.h index e2802252d5..8cf52f540b 100644 --- a/esphome/components/gpio/binary_sensor/gpio_binary_sensor.h +++ b/esphome/components/gpio/binary_sensor/gpio_binary_sensor.h @@ -36,7 +36,7 @@ class GPIOBinarySensorStore { volatile bool state_{false}; volatile bool last_state_{false}; volatile bool changed_{false}; - Component *component_{nullptr}; // Pointer to the component for enable_loop_soon_from_isr() + Component *component_{nullptr}; // Pointer to the component for enable_loop_soon_any_context() }; class GPIOBinarySensor : public binary_sensor::BinarySensor, public Component { From bd50a7f1ab42b80a27a58a9c63dc787171b2eb52 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 14:33:58 +0200 Subject: [PATCH 05/11] cleanup --- .../ethernet/ethernet_component.cpp | 33 +++++++++---------- .../components/ethernet/ethernet_component.h | 2 -- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/esphome/components/ethernet/ethernet_component.cpp b/esphome/components/ethernet/ethernet_component.cpp index 8ae15250c4..47db61eea5 100644 --- a/esphome/components/ethernet/ethernet_component.cpp +++ b/esphome/components/ethernet/ethernet_component.cpp @@ -400,19 +400,21 @@ void EthernetComponent::eth_event_handler(void *arg, esp_event_base_t event_base case ETHERNET_EVENT_START: event_name = "ETH started"; global_eth_component->started_ = true; - global_eth_component->enable_loop(); + global_eth_component->enable_loop_soon_any_context(); break; case ETHERNET_EVENT_STOP: event_name = "ETH stopped"; global_eth_component->started_ = false; - global_eth_component->set_connected_(false); // This will enable the loop + global_eth_component->connected_ = false; + global_eth_component->enable_loop_soon_any_context(); // Enable loop when connection state changes break; case ETHERNET_EVENT_CONNECTED: event_name = "ETH connected"; break; case ETHERNET_EVENT_DISCONNECTED: event_name = "ETH disconnected"; - global_eth_component->set_connected_(false); // This will enable the loop + global_eth_component->connected_ = false; + global_eth_component->enable_loop_soon_any_context(); // Enable loop when connection state changes break; default: return; @@ -428,9 +430,11 @@ void EthernetComponent::got_ip_event_handler(void *arg, esp_event_base_t event_b ESP_LOGV(TAG, "[Ethernet event] ETH Got IP " IPSTR, IP2STR(&ip_info->ip)); global_eth_component->got_ipv4_address_ = true; #if USE_NETWORK_IPV6 && (USE_NETWORK_MIN_IPV6_ADDR_COUNT > 0) - global_eth_component->set_connected_(global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT); + global_eth_component->connected_ = global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT; + global_eth_component->enable_loop_soon_any_context(); // Enable loop when connection state changes #else - global_eth_component->set_connected_(true); + global_eth_component->connected_ = true; + global_eth_component->enable_loop_soon_any_context(); // Enable loop when connection state changes #endif /* USE_NETWORK_IPV6 */ } @@ -441,10 +445,12 @@ void EthernetComponent::got_ip6_event_handler(void *arg, esp_event_base_t event_ ESP_LOGV(TAG, "[Ethernet event] ETH Got IPv6: " IPV6STR, IPV62STR(event->ip6_info.ip)); global_eth_component->ipv6_count_ += 1; #if (USE_NETWORK_MIN_IPV6_ADDR_COUNT > 0) - global_eth_component->set_connected_(global_eth_component->got_ipv4_address_ && - (global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT)); + global_eth_component->connected_ = + global_eth_component->got_ipv4_address_ && (global_eth_component->ipv6_count_ >= USE_NETWORK_MIN_IPV6_ADDR_COUNT); + global_eth_component->enable_loop_soon_any_context(); // Enable loop when connection state changes #else - global_eth_component->set_connected_(global_eth_component->got_ipv4_address_); + global_eth_component->connected_ = global_eth_component->got_ipv4_address_; + global_eth_component->enable_loop_soon_any_context(); // Enable loop when connection state changes #endif } #endif /* USE_NETWORK_IPV6 */ @@ -521,15 +527,6 @@ void EthernetComponent::start_connect_() { bool EthernetComponent::is_connected() { return this->state_ == EthernetComponentState::CONNECTED; } -void EthernetComponent::set_connected_(bool connected) { - if (this->connected_ != connected) { - this->connected_ = connected; - // Always enable loop when connection state changes - // so the state machine can process the state change - this->enable_loop(); - } -} - void EthernetComponent::dump_connect_params_() { esp_netif_ip_info_t ip; esp_netif_get_ip_info(this->eth_netif_, &ip); @@ -633,7 +630,7 @@ bool EthernetComponent::powerdown() { ESP_LOGE(TAG, "Ethernet PHY not assigned"); return false; } - this->set_connected_(false); + this->connected_ = false; this->started_ = false; // No need to enable_loop() here as this is only called during shutdown/reboot if (this->phy_->pwrctl(this->phy_, false) != ESP_OK) { diff --git a/esphome/components/ethernet/ethernet_component.h b/esphome/components/ethernet/ethernet_component.h index ebcd4ded81..7a205d89f0 100644 --- a/esphome/components/ethernet/ethernet_component.h +++ b/esphome/components/ethernet/ethernet_component.h @@ -104,8 +104,6 @@ class EthernetComponent : public Component { void ksz8081_set_clock_reference_(esp_eth_mac_t *mac); /// @brief Set arbitratry PHY registers from config. void write_phy_register_(esp_eth_mac_t *mac, PHYRegister register_data); - /// @brief Safely set connected state and ensure loop is enabled for state machine processing - void set_connected_(bool connected); std::string use_address_; #ifdef USE_ETHERNET_SPI From 3f71c09b7b8743a14d6d99ff22ff2d7609a9e724 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 18:36:55 +0200 Subject: [PATCH 06/11] Fix slow noise handshake by reading multiple messages per loop --- esphome/components/api/api_connection.cpp | 60 +++++++++++++---------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index 3e2b7c0154..3034ffb678 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -28,6 +28,12 @@ namespace esphome { namespace api { +// Read a maximum of 5 messages per loop iteration to prevent starving other components. +// This is a balance between API responsiveness and allowing other components to run. +// Since each message could contain multiple protobuf messages when using packet batching, +// this limits the number of messages processed, not the number of TCP packets. +static constexpr uint8_t MAX_MESSAGES_PER_LOOP = 5; + static const char *const TAG = "api.connection"; static const int ESP32_CAMERA_STOP_STREAM = 5000; @@ -109,33 +115,38 @@ void APIConnection::loop() { return; } + const uint32_t now = App.get_loop_component_start_time(); // Check if socket has data ready before attempting to read if (this->helper_->is_socket_ready()) { - ReadPacketBuffer buffer; - err = this->helper_->read_packet(&buffer); - if (err == APIError::WOULD_BLOCK) { - // pass - } else if (err != APIError::OK) { - on_fatal_error(); - if (err == APIError::SOCKET_READ_FAILED && errno == ECONNRESET) { - ESP_LOGW(TAG, "%s: Connection reset", this->client_combined_info_.c_str()); - } else if (err == APIError::CONNECTION_CLOSED) { - ESP_LOGW(TAG, "%s: Connection closed", this->client_combined_info_.c_str()); - } else { - ESP_LOGW(TAG, "%s: Reading failed: %s errno=%d", this->client_combined_info_.c_str(), api_error_to_str(err), - errno); - } - return; - } else { - this->last_traffic_ = App.get_loop_component_start_time(); - // read a packet - if (buffer.data_len > 0) { - this->read_message(buffer.data_len, buffer.type, &buffer.container[buffer.data_offset]); - } else { - this->read_message(0, buffer.type, nullptr); - } - if (this->remove_) + // Read up to MAX_MESSAGES_PER_LOOP messages per loop to improve throughput + for (uint8_t message_count = 0; message_count < MAX_MESSAGES_PER_LOOP; message_count++) { + ReadPacketBuffer buffer; + err = this->helper_->read_packet(&buffer); + if (err == APIError::WOULD_BLOCK) { + // No more data available + break; + } else if (err != APIError::OK) { + on_fatal_error(); + if (err == APIError::SOCKET_READ_FAILED && errno == ECONNRESET) { + ESP_LOGW(TAG, "%s: Connection reset", this->client_combined_info_.c_str()); + } else if (err == APIError::CONNECTION_CLOSED) { + ESP_LOGW(TAG, "%s: Connection closed", this->client_combined_info_.c_str()); + } else { + ESP_LOGW(TAG, "%s: Reading failed: %s errno=%d", this->client_combined_info_.c_str(), api_error_to_str(err), + errno); + } return; + } else { + this->last_traffic_ = now; + // read a packet + if (buffer.data_len > 0) { + this->read_message(buffer.data_len, buffer.type, &buffer.container[buffer.data_offset]); + } else { + this->read_message(0, buffer.type, nullptr); + } + if (this->remove_) + return; + } } } @@ -152,7 +163,6 @@ void APIConnection::loop() { static uint8_t max_ping_retries = 60; static uint16_t ping_retry_interval = 1000; - const uint32_t now = App.get_loop_component_start_time(); if (this->sent_ping_) { // Disconnect if not responded within 2.5*keepalive if (now - this->last_traffic_ > (KEEPALIVE_TIMEOUT_MS * 5) / 2) { From a5a099336bca6586e3df749c902cfb7e0ed8e8f2 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 19:22:23 +0200 Subject: [PATCH 07/11] one more --- esphome/components/api/api_frame_helper.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/esphome/components/api/api_frame_helper.cpp b/esphome/components/api/api_frame_helper.cpp index e0eb94836d..ff660f439e 100644 --- a/esphome/components/api/api_frame_helper.cpp +++ b/esphome/components/api/api_frame_helper.cpp @@ -274,12 +274,21 @@ APIError APINoiseFrameHelper::init() { } /// Run through handshake messages (if in that phase) APIError APINoiseFrameHelper::loop() { - APIError err = state_action_(); - if (err != APIError::OK && err != APIError::WOULD_BLOCK) { - return err; + // During handshake phase, process as many actions as possible until we can't progress + // socket_->ready() stays true until next main loop, but state_action() will return + // WOULD_BLOCK when no more data is available to read + while (state_ != State::DATA && this->socket_->ready()) { + APIError err = state_action_(); + if (err != APIError::OK && err != APIError::WOULD_BLOCK) { + return err; + } + if (err == APIError::WOULD_BLOCK) { + break; + } } + if (!this->tx_buf_.empty()) { - err = try_send_tx_buf_(); + APIError err = try_send_tx_buf_(); if (err != APIError::OK && err != APIError::WOULD_BLOCK) { return err; } From 7dfdf965b72119baf99c53ec227fa23d774e156e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 21:26:32 +0200 Subject: [PATCH 08/11] remove safety check --- esphome/components/esphome/ota/ota_esphome.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 28c5494e74..30a379accd 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -85,7 +85,7 @@ void ESPHomeOTAComponent::dump_config() { void ESPHomeOTAComponent::loop() { // Skip handle_() call if no client connected and no incoming connections // This optimization reduces idle loop overhead when OTA is not active - if (client_ != nullptr || (server_ && server_->ready())) { + if (client_ != nullptr || server_->ready()) { this->handle_(); } } From 8002fe0dd5815156dfb88a413507dc45026d19cb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 21:27:30 +0200 Subject: [PATCH 09/11] remove safety check --- esphome/components/esphome/ota/ota_esphome.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 30a379accd..04b93bf0f9 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -85,6 +85,7 @@ void ESPHomeOTAComponent::dump_config() { void ESPHomeOTAComponent::loop() { // Skip handle_() call if no client connected and no incoming connections // This optimization reduces idle loop overhead when OTA is not active + // Note: No need to check server_ for null as the component is marked failed in setup() if server_ creation fails if (client_ != nullptr || server_->ready()) { this->handle_(); } From ca7ede8f96653226ee5ef0df275b0d4f53e320cf Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 18 Jun 2025 21:35:04 +0200 Subject: [PATCH 10/11] more cleanups --- esphome/components/esphome/ota/ota_esphome.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 04b93bf0f9..34ccb0b69f 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -113,9 +113,9 @@ void ESPHomeOTAComponent::handle_() { struct sockaddr_storage source_addr; socklen_t addr_len = sizeof(source_addr); client_ = server_->accept((struct sockaddr *) &source_addr, &addr_len); + if (client_ == nullptr) + return; } - if (client_ == nullptr) - return; int enable = 1; int err = client_->setsockopt(IPPROTO_TCP, TCP_NODELAY, &enable, sizeof(int)); From ca6ae746c1c53aa74732fe369fe2101d0f3ec196 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 19 Jun 2025 00:39:19 +0200 Subject: [PATCH 11/11] be explict --- .../components/esphome/ota/ota_esphome.cpp | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 34ccb0b69f..4cc82b9094 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -26,19 +26,19 @@ void ESPHomeOTAComponent::setup() { ota::register_ota_platform(this); #endif - server_ = socket::socket_ip_loop_monitored(SOCK_STREAM, 0); // monitored for incoming connections - if (server_ == nullptr) { + this->server_ = socket::socket_ip_loop_monitored(SOCK_STREAM, 0); // monitored for incoming connections + if (this->server_ == nullptr) { ESP_LOGW(TAG, "Could not create socket"); this->mark_failed(); return; } int enable = 1; - int err = server_->setsockopt(SOL_SOCKET, SO_REUSEADDR, &enable, sizeof(int)); + int err = this->server_->setsockopt(SOL_SOCKET, SO_REUSEADDR, &enable, sizeof(int)); if (err != 0) { ESP_LOGW(TAG, "Socket unable to set reuseaddr: errno %d", err); // we can still continue } - err = server_->setblocking(false); + err = this->server_->setblocking(false); if (err != 0) { ESP_LOGW(TAG, "Socket unable to set nonblocking mode: errno %d", err); this->mark_failed(); @@ -54,14 +54,14 @@ void ESPHomeOTAComponent::setup() { return; } - err = server_->bind((struct sockaddr *) &server, sizeof(server)); + err = this->server_->bind((struct sockaddr *) &server, sizeof(server)); if (err != 0) { ESP_LOGW(TAG, "Socket unable to bind: errno %d", errno); this->mark_failed(); return; } - err = server_->listen(4); + err = this->server_->listen(4); if (err != 0) { ESP_LOGW(TAG, "Socket unable to listen: errno %d", errno); this->mark_failed(); @@ -86,7 +86,7 @@ void ESPHomeOTAComponent::loop() { // Skip handle_() call if no client connected and no incoming connections // This optimization reduces idle loop overhead when OTA is not active // Note: No need to check server_ for null as the component is marked failed in setup() if server_ creation fails - if (client_ != nullptr || server_->ready()) { + if (this->client_ != nullptr || this->server_->ready()) { this->handle_(); } } @@ -108,21 +108,21 @@ void ESPHomeOTAComponent::handle_() { size_t size_acknowledged = 0; #endif - if (client_ == nullptr) { + if (this->client_ == nullptr) { // We already checked server_->ready() in loop(), so we can accept directly struct sockaddr_storage source_addr; socklen_t addr_len = sizeof(source_addr); - client_ = server_->accept((struct sockaddr *) &source_addr, &addr_len); - if (client_ == nullptr) + this->client_ = this->server_->accept((struct sockaddr *) &source_addr, &addr_len); + if (this->client_ == nullptr) return; } int enable = 1; - int err = client_->setsockopt(IPPROTO_TCP, TCP_NODELAY, &enable, sizeof(int)); + int err = this->client_->setsockopt(IPPROTO_TCP, TCP_NODELAY, &enable, sizeof(int)); if (err != 0) { ESP_LOGW(TAG, "Socket could not enable TCP nodelay, errno %d", errno); - client_->close(); - client_ = nullptr; + this->client_->close(); + this->client_ = nullptr; return; }