diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index ca5689bdf6..ef791d462c 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->get_client_combined_info().c_str()); - } else if (err == APIError::CONNECTION_CLOSED) { - ESP_LOGW(TAG, "%s: Connection closed", this->get_client_combined_info().c_str()); - } else { - ESP_LOGW(TAG, "%s: Reading failed: %s errno=%d", this->get_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->get_client_combined_info().c_str()); + } else if (err == APIError::CONNECTION_CLOSED) { + ESP_LOGW(TAG, "%s: Connection closed", this->get_client_combined_info().c_str()); + } else { + ESP_LOGW(TAG, "%s: Reading failed: %s errno=%d", this->get_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) { 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; } diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 28c5494e74..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(); @@ -85,7 +85,8 @@ 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())) { + // Note: No need to check server_ for null as the component is marked failed in setup() if server_ creation fails + if (this->client_ != nullptr || this->server_->ready()) { this->handle_(); } } @@ -107,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); + this->client_ = this->server_->accept((struct sockaddr *) &source_addr, &addr_len); + if (this->client_ == nullptr) + return; } - if (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; } diff --git a/esphome/components/ethernet/ethernet_component.cpp b/esphome/components/ethernet/ethernet_component.cpp index 0a6ba6470e..984a94b078 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_soon_any_context(); break; case ETHERNET_EVENT_STOP: event_name = "ETH stopped"; global_eth_component->started_ = false; 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"; @@ -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_soon_any_context(); // Enable loop when connection state changes break; default: return; @@ -425,8 +431,10 @@ void EthernetComponent::got_ip_event_handler(void *arg, esp_event_base_t event_b 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->enable_loop_soon_any_context(); // Enable loop when connection state changes #else global_eth_component->connected_ = true; + global_eth_component->enable_loop_soon_any_context(); // Enable loop when connection state changes #endif /* USE_NETWORK_IPV6 */ } @@ -439,8 +447,10 @@ void EthernetComponent::got_ip6_event_handler(void *arg, esp_event_base_t event_ #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->enable_loop_soon_any_context(); // Enable loop when connection state changes #else 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 */ @@ -452,6 +462,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 +632,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; 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 { diff --git a/esphome/core/component.cpp b/esphome/core/component.cpp index ef30591546..8f7886ce16 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 4d0b594542..ea7804e1d4 100644 --- a/esphome/core/component.h +++ b/esphome/core/component.h @@ -172,15 +172,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. * @@ -191,7 +191,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; @@ -364,7 +364,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"