From 8ba22183b962db2664112c772c3b5fbd88f0a7ee Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 19 Jun 2025 03:30:41 +0200 Subject: [PATCH] Add enable_loop_soon_any_context() for thread and ISR-safe loop enabling (#9127) --- esphome/core/application.cpp | 94 ++++++++++++++++--- esphome/core/application.h | 3 + esphome/core/component.cpp | 23 ++++- esphome/core/component.h | 31 +++++- .../loop_test_component/__init__.py | 18 ++++ .../loop_test_isr_component.cpp | 80 ++++++++++++++++ .../loop_test_isr_component.h | 32 +++++++ .../fixtures/loop_disable_enable.yaml | 5 + tests/integration/test_loop_disable_enable.py | 59 +++++++++++- 9 files changed, 325 insertions(+), 20 deletions(-) create mode 100644 tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.cpp create mode 100644 tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.h diff --git a/esphome/core/application.cpp b/esphome/core/application.cpp index 58df49f0f2..49c1e5fd61 100644 --- a/esphome/core/application.cpp +++ b/esphome/core/application.cpp @@ -97,6 +97,20 @@ void Application::loop() { // Feed WDT with time this->feed_wdt(last_op_end_time); + // Process any pending enable_loop requests from ISRs + // This must be done before marking in_loop_ = true to avoid race conditions + if (this->has_pending_enable_loop_requests_) { + // Clear flag BEFORE processing to avoid race condition + // If ISR sets it during processing, we'll catch it next loop iteration + // This is safe because: + // 1. Each component has its own pending_enable_loop_ flag that we check + // 2. If we can't process a component (wrong state), enable_pending_loops_() + // will set this flag back to true + // 3. Any new ISR requests during processing will set the flag again + this->has_pending_enable_loop_requests_ = false; + this->enable_pending_loops_(); + } + // Mark that we're in the loop for safe reentrant modifications this->in_loop_ = true; @@ -286,24 +300,82 @@ void Application::disable_component_loop_(Component *component) { } } +void Application::activate_looping_component_(uint16_t index) { + // Helper to move component from inactive to active section + if (index != this->looping_components_active_end_) { + std::swap(this->looping_components_[index], this->looping_components_[this->looping_components_active_end_]); + } + this->looping_components_active_end_++; +} + void Application::enable_component_loop_(Component *component) { - // This method must be reentrant - components can re-enable themselves during their own loop() call - // Single pass through all components to find and move if needed - // With typical 10-30 components, O(n) is faster than maintaining a map + // This method is only called when component state is LOOP_DONE, so we know + // the component must be in the inactive section (if it exists in looping_components_) + // Only search the inactive portion for better performance + // With typical 0-5 inactive components, O(k) is much faster than O(n) const uint16_t size = this->looping_components_.size(); - for (uint16_t i = 0; i < size; i++) { + for (uint16_t i = this->looping_components_active_end_; i < size; i++) { if (this->looping_components_[i] == component) { - if (i < this->looping_components_active_end_) { - return; // Already active - } // Found in inactive section - move to active - if (i != this->looping_components_active_end_) { - std::swap(this->looping_components_[i], this->looping_components_[this->looping_components_active_end_]); - } - this->looping_components_active_end_++; + this->activate_looping_component_(i); return; } } + // Component not found in looping_components_ - this is normal for components + // that don't have loop() or were not included in the partitioned vector +} + +void Application::enable_pending_loops_() { + // Process components that requested enable_loop from ISR context + // Only iterate through inactive looping_components_ (typically 0-5) instead of all components + // + // Race condition handling: + // 1. We check if component is already in LOOP state first - if so, just clear the flag + // This handles reentrancy where enable_loop() was called between ISR and processing + // 2. We only clear pending_enable_loop_ after checking state, preventing lost requests + // 3. If any components aren't in LOOP_DONE state, we set has_pending_enable_loop_requests_ + // back to true to ensure we check again next iteration + // 4. ISRs can safely set flags at any time - worst case is we process them next iteration + // 5. The global flag (has_pending_enable_loop_requests_) is cleared before this method, + // so any ISR that fires during processing will be caught in the next loop + const uint16_t size = this->looping_components_.size(); + bool has_pending = false; + + for (uint16_t i = this->looping_components_active_end_; i < size; i++) { + Component *component = this->looping_components_[i]; + if (!component->pending_enable_loop_) { + continue; // Skip components without pending requests + } + + // Check current state + uint8_t state = component->component_state_ & COMPONENT_STATE_MASK; + + // If already in LOOP state, nothing to do - clear flag and continue + if (state == COMPONENT_STATE_LOOP) { + component->pending_enable_loop_ = false; + continue; + } + + // If not in LOOP_DONE state, can't enable yet - keep flag set + if (state != COMPONENT_STATE_LOOP_DONE) { + has_pending = true; // Keep tracking this component + continue; // Keep the flag set - try again next iteration + } + + // Clear the pending flag and enable the loop + component->pending_enable_loop_ = false; + ESP_LOGD(TAG, "%s loop enabled from ISR", component->get_component_source()); + component->component_state_ &= ~COMPONENT_STATE_MASK; + component->component_state_ |= COMPONENT_STATE_LOOP; + + // Move to active section + this->activate_looping_component_(i); + } + + // If we couldn't process some requests, ensure we check again next iteration + if (has_pending) { + this->has_pending_enable_loop_requests_ = true; + } } #ifdef USE_SOCKET_SELECT_SUPPORT diff --git a/esphome/core/application.h b/esphome/core/application.h index ea298638d2..93d5a78958 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -577,6 +577,8 @@ class Application { // to ensure component state is properly updated along with the loop partition void disable_component_loop_(Component *component); void enable_component_loop_(Component *component); + void enable_pending_loops_(); + void activate_looping_component_(uint16_t index); void feed_wdt_arch_(); @@ -682,6 +684,7 @@ class Application { uint32_t loop_interval_{16}; size_t dump_config_at_{SIZE_MAX}; uint8_t app_state_{0}; + volatile bool has_pending_enable_loop_requests_{false}; Component *current_component_{nullptr}; uint32_t loop_component_start_time_{0}; diff --git a/esphome/core/component.cpp b/esphome/core/component.cpp index 3117f49ac1..625a7b2125 100644 --- a/esphome/core/component.cpp +++ b/esphome/core/component.cpp @@ -148,10 +148,12 @@ void Component::mark_failed() { App.disable_component_loop_(this); } void Component::disable_loop() { - ESP_LOGD(TAG, "%s loop disabled", this->get_component_source()); - this->component_state_ &= ~COMPONENT_STATE_MASK; - this->component_state_ |= COMPONENT_STATE_LOOP_DONE; - App.disable_component_loop_(this); + if ((this->component_state_ & COMPONENT_STATE_MASK) != COMPONENT_STATE_LOOP_DONE) { + ESP_LOGD(TAG, "%s loop disabled", this->get_component_source()); + this->component_state_ &= ~COMPONENT_STATE_MASK; + this->component_state_ |= COMPONENT_STATE_LOOP_DONE; + App.disable_component_loop_(this); + } } void Component::enable_loop() { if ((this->component_state_ & COMPONENT_STATE_MASK) == COMPONENT_STATE_LOOP_DONE) { @@ -161,6 +163,19 @@ void Component::enable_loop() { App.enable_component_loop_(this); } } +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/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; +} void Component::reset_to_construction_state() { if ((this->component_state_ & COMPONENT_STATE_MASK) == COMPONENT_STATE_FAILED) { ESP_LOGI(TAG, "Component %s is being reset to construction state", this->get_component_source()); diff --git a/esphome/core/component.h b/esphome/core/component.h index a37d64086a..7f2bdd8414 100644 --- a/esphome/core/component.h +++ b/esphome/core/component.h @@ -171,6 +171,27 @@ class Component { */ void enable_loop(); + /** 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, 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_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. + * + * Use disable_loop() from the main thread only. + * + * If you need to disable the loop from ISR, carefully implement + * it in the component itself, with an ISR safe approach, and call + * disable_loop() in its next ::loop() iteration. Implementations + * will need to carefully consider all possible race conditions. + */ + void enable_loop_soon_any_context(); + bool is_failed() const; bool is_ready() const; @@ -331,16 +352,18 @@ class Component { /// Cancel a defer callback using the specified name, name must not be empty. bool cancel_defer(const std::string &name); // NOLINT + // Ordered for optimal packing on 32-bit systems + float setup_priority_override_{NAN}; + const char *component_source_{nullptr}; + const char *error_message_{nullptr}; + uint16_t warn_if_blocking_over_{WARN_IF_BLOCKING_OVER_MS}; ///< Warn if blocked for this many ms (max 65.5s) /// State of this component - each bit has a purpose: /// Bits 0-1: Component state (0x00=CONSTRUCTION, 0x01=SETUP, 0x02=LOOP, 0x03=FAILED) /// Bit 2: STATUS_LED_WARNING /// Bit 3: STATUS_LED_ERROR /// Bits 4-7: Unused - reserved for future expansion (50% of the bits are free) uint8_t component_state_{0x00}; - float setup_priority_override_{NAN}; - const char *component_source_{nullptr}; - uint16_t warn_if_blocking_over_{WARN_IF_BLOCKING_OVER_MS}; ///< Warn if blocked for this many ms (max 65.5s) - const char *error_message_{nullptr}; + 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/__init__.py b/tests/integration/fixtures/external_components/loop_test_component/__init__.py index c5eda67d1e..b66d4598f4 100644 --- a/tests/integration/fixtures/external_components/loop_test_component/__init__.py +++ b/tests/integration/fixtures/external_components/loop_test_component/__init__.py @@ -7,9 +7,13 @@ CODEOWNERS = ["@esphome/tests"] loop_test_component_ns = cg.esphome_ns.namespace("loop_test_component") LoopTestComponent = loop_test_component_ns.class_("LoopTestComponent", cg.Component) +LoopTestISRComponent = loop_test_component_ns.class_( + "LoopTestISRComponent", cg.Component +) CONF_DISABLE_AFTER = "disable_after" CONF_TEST_REDUNDANT_OPERATIONS = "test_redundant_operations" +CONF_ISR_COMPONENTS = "isr_components" COMPONENT_CONFIG_SCHEMA = cv.Schema( { @@ -20,10 +24,18 @@ COMPONENT_CONFIG_SCHEMA = cv.Schema( } ) +ISR_COMPONENT_CONFIG_SCHEMA = cv.Schema( + { + cv.GenerateID(): cv.declare_id(LoopTestISRComponent), + cv.Required(CONF_NAME): cv.string, + } +) + CONFIG_SCHEMA = cv.Schema( { cv.GenerateID(): cv.declare_id(LoopTestComponent), cv.Required(CONF_COMPONENTS): cv.ensure_list(COMPONENT_CONFIG_SCHEMA), + cv.Optional(CONF_ISR_COMPONENTS): cv.ensure_list(ISR_COMPONENT_CONFIG_SCHEMA), } ).extend(cv.COMPONENT_SCHEMA) @@ -76,3 +88,9 @@ async def to_code(config): comp_config[CONF_TEST_REDUNDANT_OPERATIONS] ) ) + + # Create ISR test components + for isr_config in config.get(CONF_ISR_COMPONENTS, []): + var = cg.new_Pvariable(isr_config[CONF_ID]) + await cg.register_component(var, isr_config) + cg.add(var.set_name(isr_config[CONF_NAME])) 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 new file mode 100644 index 0000000000..30afec0422 --- /dev/null +++ b/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.cpp @@ -0,0 +1,80 @@ +#include "loop_test_isr_component.h" +#include "esphome/core/hal.h" +#include "esphome/core/application.h" + +namespace esphome { +namespace loop_test_component { + +static const char *const ISR_TAG = "loop_test_isr_component"; + +void LoopTestISRComponent::setup() { + ESP_LOGI(ISR_TAG, "[%s] ISR component setup called", this->name_.c_str()); + this->last_check_time_ = millis(); +} + +void LoopTestISRComponent::loop() { + this->loop_count_++; + ESP_LOGI(ISR_TAG, "[%s] ISR component loop count: %d", this->name_.c_str(), this->loop_count_); + + // Disable after 5 loops + if (this->loop_count_ == 5) { + ESP_LOGI(ISR_TAG, "[%s] Disabling after 5 loops", this->name_.c_str()); + this->disable_loop(); + this->last_disable_time_ = millis(); + // Simulate ISR after disabling + this->set_timeout("simulate_isr_1", 50, [this]() { + ESP_LOGI(ISR_TAG, "[%s] Simulating ISR enable", this->name_.c_str()); + this->simulate_isr_enable(); + // Test reentrancy - call enable_loop() directly after ISR + // This simulates another thread calling enable_loop while processing ISR enables + this->set_timeout("test_reentrant", 10, [this]() { + ESP_LOGI(ISR_TAG, "[%s] Testing reentrancy - calling enable_loop() directly", this->name_.c_str()); + this->enable_loop(); + }); + }); + } + + // If we get here after being disabled, it means ISR re-enabled us + if (this->loop_count_ > 5 && this->loop_count_ < 10) { + ESP_LOGI(ISR_TAG, "[%s] Running after ISR re-enable! ISR was called %d times", this->name_.c_str(), + this->isr_call_count_); + } + + // Disable again after 10 loops to test multiple ISR enables + if (this->loop_count_ == 10) { + ESP_LOGI(ISR_TAG, "[%s] Disabling again after 10 loops", this->name_.c_str()); + this->disable_loop(); + this->last_disable_time_ = millis(); + + // Test pure ISR enable without any main loop enable + this->set_timeout("simulate_isr_2", 50, [this]() { + ESP_LOGI(ISR_TAG, "[%s] Testing pure ISR enable (no main loop enable)", this->name_.c_str()); + this->simulate_isr_enable(); + // DO NOT call enable_loop() - test that ISR alone works + }); + } + + // Log when we're running after second ISR enable + if (this->loop_count_ > 10) { + ESP_LOGI(ISR_TAG, "[%s] Running after pure ISR re-enable! ISR was called %d times total", this->name_.c_str(), + this->isr_call_count_); + } +} + +void IRAM_ATTR LoopTestISRComponent::simulate_isr_enable() { + // This simulates what would happen in a real ISR + // In a real scenario, this would be called from an actual interrupt handler + + this->isr_call_count_++; + + // 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 +} + +} // namespace loop_test_component +} // namespace esphome 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 new file mode 100644 index 0000000000..20e11b5ecd --- /dev/null +++ b/tests/integration/fixtures/external_components/loop_test_component/loop_test_isr_component.h @@ -0,0 +1,32 @@ +#pragma once + +#include "esphome/core/component.h" +#include "esphome/core/log.h" +#include "esphome/core/hal.h" + +namespace esphome { +namespace loop_test_component { + +class LoopTestISRComponent : public Component { + public: + void set_name(const std::string &name) { this->name_ = name; } + + void setup() override; + void loop() override; + + // Simulates an ISR calling enable_loop_soon_any_context + void simulate_isr_enable(); + + float get_setup_priority() const override { return setup_priority::DATA; } + + protected: + std::string name_; + int loop_count_{0}; + uint32_t last_disable_time_{0}; + uint32_t last_check_time_{0}; + bool isr_enable_pending_{false}; + int isr_call_count_{0}; +}; + +} // namespace loop_test_component +} // namespace esphome diff --git a/tests/integration/fixtures/loop_disable_enable.yaml b/tests/integration/fixtures/loop_disable_enable.yaml index 17010f7c34..f19d7f60ca 100644 --- a/tests/integration/fixtures/loop_disable_enable.yaml +++ b/tests/integration/fixtures/loop_disable_enable.yaml @@ -35,6 +35,11 @@ loop_test_component: test_redundant_operations: true disable_after: 10 + # ISR test component that uses enable_loop_soon_any_context + isr_components: + - id: isr_test + name: "isr_test" + # Interval to re-enable the self_disable_10 component after some time interval: - interval: 0.5s diff --git a/tests/integration/test_loop_disable_enable.py b/tests/integration/test_loop_disable_enable.py index 84301c25d8..d5f868aa93 100644 --- a/tests/integration/test_loop_disable_enable.py +++ b/tests/integration/test_loop_disable_enable.py @@ -41,17 +41,25 @@ async def test_loop_disable_enable( redundant_disable_tested = asyncio.Event() # Event fired when self_disable_10 component is re-enabled and runs again (count > 10) self_disable_10_re_enabled = asyncio.Event() + # Events for ISR component testing + isr_component_disabled = asyncio.Event() + isr_component_re_enabled = asyncio.Event() + isr_component_pure_re_enabled = asyncio.Event() # Track loop counts for components self_disable_10_counts: list[int] = [] normal_component_counts: list[int] = [] + isr_component_counts: list[int] = [] def on_log_line(line: str) -> None: """Process each log line from the process output.""" # Strip ANSI color codes clean_line = re.sub(r"\x1b\[[0-9;]*m", "", line) - if "loop_test_component" not in clean_line: + if ( + "loop_test_component" not in clean_line + and "loop_test_isr_component" not in clean_line + ): return log_messages.append(clean_line) @@ -92,6 +100,18 @@ async def test_loop_disable_enable( ): redundant_disable_tested.set() + # ISR component events + elif "[isr_test]" in clean_line: + if "ISR component loop count:" in clean_line: + count = int(clean_line.split("ISR component loop count: ")[1]) + isr_component_counts.append(count) + elif "Disabling after 5 loops" in clean_line: + isr_component_disabled.set() + elif "Running after ISR re-enable!" in clean_line: + isr_component_re_enabled.set() + elif "Running after pure ISR re-enable!" in clean_line: + isr_component_pure_re_enabled.set() + # Write, compile and run the ESPHome device with log callback async with ( run_compiled(yaml_config, line_callback=on_log_line), @@ -148,3 +168,40 @@ async def test_loop_disable_enable( assert later_self_disable_counts, ( "self_disable_10 was re-enabled but did not run additional times" ) + + # Test ISR component functionality + # Wait for ISR component to disable itself after 5 loops + try: + await asyncio.wait_for(isr_component_disabled.wait(), timeout=3.0) + except asyncio.TimeoutError: + pytest.fail("ISR component did not disable itself within 3 seconds") + + # Verify it ran exactly 5 times before disabling + first_run_counts = [c for c in isr_component_counts if c <= 5] + assert len(first_run_counts) == 5, ( + f"Expected 5 loops before disable, got {first_run_counts}" + ) + + # Wait for component to be re-enabled by periodic ISR simulation and run again + try: + await asyncio.wait_for(isr_component_re_enabled.wait(), timeout=2.0) + except asyncio.TimeoutError: + pytest.fail("ISR component was not re-enabled after ISR call") + + # Verify it's running again after ISR enable + count_after_isr = len(isr_component_counts) + assert count_after_isr > 5, ( + f"Component didn't run after ISR enable: got {count_after_isr} counts total" + ) + + # Wait for pure ISR enable (no main loop enable) to work + try: + await asyncio.wait_for(isr_component_pure_re_enabled.wait(), timeout=2.0) + except asyncio.TimeoutError: + pytest.fail("ISR component was not re-enabled by pure ISR call") + + # Verify it ran after pure ISR enable + final_count = len(isr_component_counts) + assert final_count > 10, ( + f"Component didn't run after pure ISR enable: got {final_count} counts total" + )