From 6bb32c2e619f6dd8f43054daad18ea6612a1be12 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 10:12:14 -0500 Subject: [PATCH 01/33] tweaks --- .../rapid_cancellation_component.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp b/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp index 210576e613..dcc9367390 100644 --- a/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp +++ b/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp @@ -38,7 +38,7 @@ void SchedulerRapidCancellationComponent::run_rapid_cancellation_test() { std::string name = ss.str(); // All threads schedule timeouts - this will implicitly cancel existing ones - this->set_timeout(name, 100, [this, name]() { + this->set_timeout(name, 150, [this, name]() { this->total_executed_.fetch_add(1); ESP_LOGI(TAG, "Executed callback '%s'", name.c_str()); }); From a71030c4de2548acdbb60306d77e260a134d853e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 10:40:19 -0500 Subject: [PATCH 02/33] fix race --- esphome/core/scheduler.cpp | 12 ++++++------ esphome/core/scheduler.h | 5 ++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 5c01b4f3f4..525525dbc3 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -65,13 +65,13 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type const char *name_cstr = is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); - // Cancel existing timer if name is not empty - if (name_cstr != nullptr && name_cstr[0] != '\0') { - this->cancel_item_(component, name_cstr, type); - } - - if (delay == SCHEDULER_DONT_RUN) + if (delay == SCHEDULER_DONT_RUN) { + // Cancel existing timer if name is not empty + if (name_cstr != nullptr && name_cstr[0] != '\0') { + this->cancel_item_(component, name_cstr, type); + } return; + } const auto now = this->millis_(); diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index a64968932e..b3c69068b5 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -2,6 +2,7 @@ #include #include +#include #include "esphome/core/component.h" #include "esphome/core/helpers.h" @@ -135,10 +136,12 @@ class Scheduler { void set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, uint32_t delay, std::function func); + // Helper to cancel items by name - must be called with lock held + bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); + uint64_t millis_(); void cleanup_(); void pop_raw_(); - void push_(std::unique_ptr item); // Common implementation for cancel operations bool cancel_item_common_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); From b00adbddceca26ddd1c68bec621ba37b02faa231 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 10:40:44 -0500 Subject: [PATCH 03/33] fix race --- esphome/core/scheduler.cpp | 82 +++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 37 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 525525dbc3..2d54077dc3 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -58,6 +58,31 @@ static void validate_static_string(const char *name) { // iterating over them from the loop task is fine; but iterating from any other context requires the lock to be held to // avoid the main thread modifying the list while it is being accessed. +// Helper to cancel items by name - must be called with lock held +bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type) { + bool ret = false; + + for (auto &it : this->items_) { + const char *item_name = it->get_name(); + if (it->component == component && item_name != nullptr && strcmp(name, item_name) == 0 && it->type == type && + !it->remove) { + this->to_remove_++; + it->remove = true; + ret = true; + } + } + for (auto &it : this->to_add_) { + const char *item_name = it->get_name(); + if (it->component == component && item_name != nullptr && strcmp(name, item_name) == 0 && it->type == type && + !it->remove) { + it->remove = true; + ret = true; + } + } + + return ret; +} + // Common implementation for both timeout and interval void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, uint32_t delay, std::function func) { @@ -66,7 +91,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); if (delay == SCHEDULER_DONT_RUN) { - // Cancel existing timer if name is not empty + // Still need to cancel existing timer if name is not empty if (name_cstr != nullptr && name_cstr[0] != '\0') { this->cancel_item_(component, name_cstr, type); } @@ -111,7 +136,16 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type } #endif - this->push_(std::move(item)); + { + LockGuard guard{this->lock_}; + // If name is provided, do atomic cancel-and-add + if (name_cstr != nullptr && name_cstr[0] != '\0') { + // Cancel existing items + this->cancel_item_locked_(component, name_cstr, type); + } + // Add new item directly to to_add_ (not using push_ to avoid double-locking) + this->to_add_.push_back(std::move(item)); + } } void HOT Scheduler::set_timeout(Component *component, const char *name, uint32_t timeout, std::function func) { @@ -242,10 +276,10 @@ void HOT Scheduler::call() { } #endif // ESPHOME_DEBUG_SCHEDULER - auto to_remove_was = to_remove_; + auto to_remove_was = this->to_remove_; auto items_was = this->items_.size(); // If we have too many items to remove - if (to_remove_ > MAX_LOGICALLY_DELETED_ITEMS) { + if (this->to_remove_ > MAX_LOGICALLY_DELETED_ITEMS) { std::vector> valid_items; while (!this->empty_()) { LockGuard guard{this->lock_}; @@ -260,10 +294,10 @@ void HOT Scheduler::call() { } // The following should not happen unless I'm missing something - if (to_remove_ != 0) { + if (this->to_remove_ != 0) { ESP_LOGW(TAG, "to_remove_ was %" PRIu32 " now: %" PRIu32 " items where %zu now %zu. Please report this", to_remove_was, to_remove_, items_was, items_.size()); - to_remove_ = 0; + this->to_remove_ = 0; } } @@ -304,26 +338,23 @@ void HOT Scheduler::call() { } { - this->lock_.lock(); + LockGuard guard{this->lock_}; // new scope, item from before might have been moved in the vector auto item = std::move(this->items_[0]); - // Only pop after function call, this ensures we were reachable // during the function call and know if we were cancelled. this->pop_raw_(); - this->lock_.unlock(); - if (item->remove) { // We were removed/cancelled in the function call, stop - to_remove_--; + this->to_remove_--; continue; } if (item->type == SchedulerItem::INTERVAL) { item->next_execution_ = now + item->interval; - this->push_(std::move(item)); + this->to_add_.push_back(std::move(item)); } } } @@ -348,7 +379,7 @@ void HOT Scheduler::cleanup_() { if (!item->remove) return; - to_remove_--; + this->to_remove_--; { LockGuard guard{this->lock_}; @@ -360,10 +391,6 @@ void HOT Scheduler::pop_raw_() { std::pop_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); this->items_.pop_back(); } -void HOT Scheduler::push_(std::unique_ptr item) { - LockGuard guard{this->lock_}; - this->to_add_.push_back(std::move(item)); -} // Common implementation for cancel operations bool HOT Scheduler::cancel_item_common_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type) { @@ -377,26 +404,7 @@ bool HOT Scheduler::cancel_item_common_(Component *component, bool is_static_str // obtain lock because this function iterates and can be called from non-loop task context LockGuard guard{this->lock_}; - bool ret = false; - - for (auto &it : this->items_) { - const char *item_name = it->get_name(); - if (it->component == component && item_name != nullptr && strcmp(name_cstr, item_name) == 0 && it->type == type && - !it->remove) { - to_remove_++; - it->remove = true; - ret = true; - } - } - for (auto &it : this->to_add_) { - const char *item_name = it->get_name(); - if (it->component == component && item_name != nullptr && strcmp(name_cstr, item_name) == 0 && it->type == type) { - it->remove = true; - ret = true; - } - } - - return ret; + return this->cancel_item_locked_(component, name_cstr, type); } bool HOT Scheduler::cancel_item_(Component *component, const std::string &name, Scheduler::SchedulerItem::Type type) { From 9bfa942cf286acc3ecf6b92f883d9ca6b8400f89 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 10:58:15 -0500 Subject: [PATCH 04/33] merge --- esphome/core/helpers.cpp | 9 ++++++++- esphome/core/helpers.h | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/esphome/core/helpers.cpp b/esphome/core/helpers.cpp index b4923c7af0..7d9b86fccd 100644 --- a/esphome/core/helpers.cpp +++ b/esphome/core/helpers.cpp @@ -645,7 +645,7 @@ void hsv_to_rgb(int hue, float saturation, float value, float &red, float &green } // System APIs -#if defined(USE_ESP8266) || defined(USE_RP2040) || defined(USE_HOST) +#if defined(USE_ESP8266) || defined(USE_RP2040) // ESP8266 doesn't have mutexes, but that shouldn't be an issue as it's single-core and non-preemptive OS. Mutex::Mutex() {} Mutex::~Mutex() {} @@ -658,6 +658,13 @@ Mutex::~Mutex() {} void Mutex::lock() { xSemaphoreTake(this->handle_, portMAX_DELAY); } bool Mutex::try_lock() { return xSemaphoreTake(this->handle_, 0) == pdTRUE; } void Mutex::unlock() { xSemaphoreGive(this->handle_); } +#elif defined(USE_HOST) +// Host platform uses std::mutex for proper thread synchronization +Mutex::Mutex() { handle_ = new std::mutex(); } +Mutex::~Mutex() { delete static_cast(handle_); } +void Mutex::lock() { static_cast(handle_)->lock(); } +bool Mutex::try_lock() { return static_cast(handle_)->try_lock(); } +void Mutex::unlock() { static_cast(handle_)->unlock(); } #endif #if defined(USE_ESP8266) diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 362f3d1fa4..d92cf07702 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -32,6 +32,10 @@ #include #endif +#ifdef USE_HOST +#include +#endif + #define HOT __attribute__((hot)) #define ESPDEPRECATED(msg, when) __attribute__((deprecated(msg))) #define ESPHOME_ALWAYS_INLINE __attribute__((always_inline)) From 2a15f35e9d88637e9c2fd3cb22d8cc20e9f2ae7b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 11:04:04 -0500 Subject: [PATCH 05/33] cleanup --- esphome/core/scheduler.cpp | 22 +++++++--------------- esphome/core/scheduler.h | 5 +---- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 2d54077dc3..2b89d45bc9 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -93,7 +93,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == SCHEDULER_DONT_RUN) { // Still need to cancel existing timer if name is not empty if (name_cstr != nullptr && name_cstr[0] != '\0') { - this->cancel_item_(component, name_cstr, type); + this->cancel_item_(component, is_static_string, name_ptr, type); } return; } @@ -157,10 +157,10 @@ void HOT Scheduler::set_timeout(Component *component, const std::string &name, u this->set_timer_common_(component, SchedulerItem::TIMEOUT, false, &name, timeout, std::move(func)); } bool HOT Scheduler::cancel_timeout(Component *component, const std::string &name) { - return this->cancel_item_(component, name, SchedulerItem::TIMEOUT); + return this->cancel_item_(component, false, &name, SchedulerItem::TIMEOUT); } bool HOT Scheduler::cancel_timeout(Component *component, const char *name) { - return this->cancel_item_(component, name, SchedulerItem::TIMEOUT); + return this->cancel_item_(component, true, name, SchedulerItem::TIMEOUT); } void HOT Scheduler::set_interval(Component *component, const std::string &name, uint32_t interval, std::function func) { @@ -172,10 +172,10 @@ void HOT Scheduler::set_interval(Component *component, const char *name, uint32_ this->set_timer_common_(component, SchedulerItem::INTERVAL, true, name, interval, std::move(func)); } bool HOT Scheduler::cancel_interval(Component *component, const std::string &name) { - return this->cancel_item_(component, name, SchedulerItem::INTERVAL); + return this->cancel_item_(component, false, &name, SchedulerItem::INTERVAL); } bool HOT Scheduler::cancel_interval(Component *component, const char *name) { - return this->cancel_item_(component, name, SchedulerItem::INTERVAL); + return this->cancel_item_(component, true, name, SchedulerItem::INTERVAL); } struct RetryArgs { @@ -392,8 +392,8 @@ void HOT Scheduler::pop_raw_() { this->items_.pop_back(); } // Common implementation for cancel operations -bool HOT Scheduler::cancel_item_common_(Component *component, bool is_static_string, const void *name_ptr, - SchedulerItem::Type type) { +bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, const void *name_ptr, + SchedulerItem::Type type) { // Get the name as const char* const char *name_cstr = is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); @@ -407,14 +407,6 @@ bool HOT Scheduler::cancel_item_common_(Component *component, bool is_static_str return this->cancel_item_locked_(component, name_cstr, type); } -bool HOT Scheduler::cancel_item_(Component *component, const std::string &name, Scheduler::SchedulerItem::Type type) { - return this->cancel_item_common_(component, false, &name, type); -} - -bool HOT Scheduler::cancel_item_(Component *component, const char *name, SchedulerItem::Type type) { - return this->cancel_item_common_(component, true, name, type); -} - uint64_t Scheduler::millis_() { // Get the current 32-bit millis value const uint32_t now = millis(); diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index b3c69068b5..6eceec151b 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -143,10 +143,7 @@ class Scheduler { void cleanup_(); void pop_raw_(); // Common implementation for cancel operations - bool cancel_item_common_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); - - bool cancel_item_(Component *component, const std::string &name, SchedulerItem::Type type); - bool cancel_item_(Component *component, const char *name, SchedulerItem::Type type); + bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); bool empty_() { this->cleanup_(); From 8e8ef8378028a3b4d57a0a262c3543d0b0fe1e0c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 11:05:18 -0500 Subject: [PATCH 06/33] cleanup --- esphome/core/scheduler.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 2b89d45bc9..14c9768b3c 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -143,7 +143,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // Cancel existing items this->cancel_item_locked_(component, name_cstr, type); } - // Add new item directly to to_add_ (not using push_ to avoid double-locking) + // Add new item directly to to_add_ + // since we have the lock held this->to_add_.push_back(std::move(item)); } } @@ -354,6 +355,8 @@ void HOT Scheduler::call() { if (item->type == SchedulerItem::INTERVAL) { item->next_execution_ = now + item->interval; + // Add new item directly to to_add_ + // since we have the lock held this->to_add_.push_back(std::move(item)); } } From 8da322fe9efe96e0128af82ae312eddc6df64071 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Sun, 6 Jul 2025 14:04:43 -0400 Subject: [PATCH 07/33] [sx127x] Improve error handling (#9351) --- esphome/components/sx127x/sx127x.cpp | 24 +++++++++++++----------- esphome/components/sx127x/sx127x.h | 4 +++- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/esphome/components/sx127x/sx127x.cpp b/esphome/components/sx127x/sx127x.cpp index e41efe098c..7f62ee2bd3 100644 --- a/esphome/components/sx127x/sx127x.cpp +++ b/esphome/components/sx127x/sx127x.cpp @@ -252,15 +252,17 @@ size_t SX127x::get_max_packet_size() { } } -void SX127x::transmit_packet(const std::vector &packet) { +SX127xError SX127x::transmit_packet(const std::vector &packet) { if (this->payload_length_ > 0 && this->payload_length_ != packet.size()) { ESP_LOGE(TAG, "Packet size does not match config"); - return; + return SX127xError::INVALID_PARAMS; } if (packet.empty() || packet.size() > this->get_max_packet_size()) { ESP_LOGE(TAG, "Packet size out of range"); - return; + return SX127xError::INVALID_PARAMS; } + + SX127xError ret = SX127xError::NONE; if (this->modulation_ == MOD_LORA) { this->set_mode_standby(); if (this->payload_length_ == 0) { @@ -278,11 +280,13 @@ void SX127x::transmit_packet(const std::vector &packet) { this->write_fifo_(packet); this->set_mode_tx(); } + // wait until transmit completes, typically the delay will be less than 100 ms uint32_t start = millis(); while (!this->dio0_pin_->digital_read()) { if (millis() - start > 4000) { ESP_LOGE(TAG, "Transmit packet failure"); + ret = SX127xError::TIMEOUT; break; } } @@ -291,6 +295,7 @@ void SX127x::transmit_packet(const std::vector &packet) { } else { this->set_mode_sleep(); } + return ret; } void SX127x::call_listeners_(const std::vector &packet, float rssi, float snr) { @@ -335,13 +340,7 @@ void SX127x::loop() { } void SX127x::run_image_cal() { - uint32_t start = millis(); - uint8_t mode = this->read_register_(REG_OP_MODE); - if ((mode & MODE_MASK) != MODE_STDBY) { - ESP_LOGE(TAG, "Need to be in standby for image cal"); - return; - } - if (mode & MOD_LORA) { + if (this->modulation_ == MOD_LORA) { this->set_mode_(MOD_FSK, MODE_SLEEP); this->set_mode_(MOD_FSK, MODE_STDBY); } @@ -350,13 +349,15 @@ void SX127x::run_image_cal() { } else { this->write_register_(REG_IMAGE_CAL, IMAGE_CAL_START); } + uint32_t start = millis(); while (this->read_register_(REG_IMAGE_CAL) & IMAGE_CAL_RUNNING) { if (millis() - start > 20) { ESP_LOGE(TAG, "Image cal failure"); + this->mark_failed(); break; } } - if (mode & MOD_LORA) { + if (this->modulation_ == MOD_LORA) { this->set_mode_(this->modulation_, MODE_SLEEP); this->set_mode_(this->modulation_, MODE_STDBY); } @@ -375,6 +376,7 @@ void SX127x::set_mode_(uint8_t modulation, uint8_t mode) { } if (millis() - start > 20) { ESP_LOGE(TAG, "Set mode failure"); + this->mark_failed(); break; } } diff --git a/esphome/components/sx127x/sx127x.h b/esphome/components/sx127x/sx127x.h index fe9f60e860..4cc7c9b6d3 100644 --- a/esphome/components/sx127x/sx127x.h +++ b/esphome/components/sx127x/sx127x.h @@ -34,6 +34,8 @@ enum SX127xBw : uint8_t { SX127X_BW_500_0, }; +enum class SX127xError { NONE = 0, TIMEOUT, INVALID_PARAMS }; + class SX127xListener { public: virtual void on_packet(const std::vector &packet, float rssi, float snr) = 0; @@ -79,7 +81,7 @@ class SX127x : public Component, void set_sync_value(const std::vector &sync_value) { this->sync_value_ = sync_value; } void run_image_cal(); void configure(); - void transmit_packet(const std::vector &packet); + SX127xError transmit_packet(const std::vector &packet); void register_listener(SX127xListener *listener) { this->listeners_.push_back(listener); } Trigger, float, float> *get_packet_trigger() const { return this->packet_trigger_; }; From 10a03ad538f8abbf33b959c398d4dbddd614fd53 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 15:37:09 -0500 Subject: [PATCH 08/33] tidy --- .../rapid_cancellation_component.cpp | 2 +- .../simultaneous_callbacks_component.cpp | 2 +- .../string_name_stress_component.cpp | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp b/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp index dcc9367390..cd4e019882 100644 --- a/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp +++ b/tests/integration/fixtures/external_components/scheduler_rapid_cancellation_component/rapid_cancellation_component.cpp @@ -29,7 +29,7 @@ void SchedulerRapidCancellationComponent::run_rapid_cancellation_test() { threads.reserve(NUM_THREADS); for (int thread_id = 0; thread_id < NUM_THREADS; thread_id++) { - threads.emplace_back([this, thread_id]() { + threads.emplace_back([this]() { for (int i = 0; i < OPERATIONS_PER_THREAD; i++) { // Use modulo to ensure multiple threads use the same names int name_index = i % NUM_NAMES; diff --git a/tests/integration/fixtures/external_components/scheduler_simultaneous_callbacks_component/simultaneous_callbacks_component.cpp b/tests/integration/fixtures/external_components/scheduler_simultaneous_callbacks_component/simultaneous_callbacks_component.cpp index e8cef41bd0..b4c2b8c6c2 100644 --- a/tests/integration/fixtures/external_components/scheduler_simultaneous_callbacks_component/simultaneous_callbacks_component.cpp +++ b/tests/integration/fixtures/external_components/scheduler_simultaneous_callbacks_component/simultaneous_callbacks_component.cpp @@ -48,7 +48,7 @@ void SchedulerSimultaneousCallbacksComponent::run_simultaneous_callbacks_test() std::string name = ss.str(); // Schedule callback for exactly DELAY_MS from now - this->set_timeout(name, DELAY_MS, [this, thread_id, i, name]() { + this->set_timeout(name, DELAY_MS, [this, name]() { // Increment concurrent counter atomically int current = this->callbacks_at_once_.fetch_add(1) + 1; diff --git a/tests/integration/fixtures/external_components/scheduler_string_name_stress_component/string_name_stress_component.cpp b/tests/integration/fixtures/external_components/scheduler_string_name_stress_component/string_name_stress_component.cpp index e20745b7cc..9071e573bb 100644 --- a/tests/integration/fixtures/external_components/scheduler_string_name_stress_component/string_name_stress_component.cpp +++ b/tests/integration/fixtures/external_components/scheduler_string_name_stress_component/string_name_stress_component.cpp @@ -59,19 +59,19 @@ void SchedulerStringNameStressComponent::run_string_name_stress_test() { // Also test nested scheduling from callbacks if (j % 10 == 0) { // Every 10th callback schedules another callback - this->set_timeout(dynamic_name, delay, [component, i, j, callback_id]() { + this->set_timeout(dynamic_name, delay, [component, callback_id]() { component->executed_callbacks_.fetch_add(1); ESP_LOGV(TAG, "Executed string-named callback %d (nested scheduler)", callback_id); // Schedule another timeout from within this callback with a new dynamic name std::string nested_name = "nested_from_" + std::to_string(callback_id); - component->set_timeout(nested_name, 1, [component, callback_id]() { + component->set_timeout(nested_name, 1, [callback_id]() { ESP_LOGV(TAG, "Executed nested string-named callback from %d", callback_id); }); }); } else { // Regular callback - this->set_timeout(dynamic_name, delay, [component, i, j, callback_id]() { + this->set_timeout(dynamic_name, delay, [component, callback_id]() { component->executed_callbacks_.fetch_add(1); ESP_LOGV(TAG, "Executed string-named callback %d", callback_id); }); From b6fade7339f941e9d08caa14188bc2928259cf80 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 17:01:51 -0500 Subject: [PATCH 09/33] Fix defer() thread safety issues on multi-core platforms (#9317) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- esphome/components/web_server/web_server.cpp | 86 +++-------- esphome/components/web_server/web_server.h | 12 -- esphome/core/helpers.cpp | 9 +- esphome/core/helpers.h | 4 + esphome/core/scheduler.cpp | 103 ++++++++++--- esphome/core/scheduler.h | 21 +++ .../fixtures/defer_fifo_simple.yaml | 109 ++++++++++++++ tests/integration/fixtures/defer_stress.yaml | 38 +++++ .../defer_stress_component/__init__.py | 19 +++ .../defer_stress_component.cpp | 75 ++++++++++ .../defer_stress_component.h | 20 +++ tests/integration/test_defer_fifo_simple.py | 117 +++++++++++++++ tests/integration/test_defer_stress.py | 137 ++++++++++++++++++ 13 files changed, 654 insertions(+), 96 deletions(-) create mode 100644 tests/integration/fixtures/defer_fifo_simple.yaml create mode 100644 tests/integration/fixtures/defer_stress.yaml create mode 100644 tests/integration/fixtures/external_components/defer_stress_component/__init__.py create mode 100644 tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.cpp create mode 100644 tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.h create mode 100644 tests/integration/test_defer_fifo_simple.py create mode 100644 tests/integration/test_defer_stress.py diff --git a/esphome/components/web_server/web_server.cpp b/esphome/components/web_server/web_server.cpp index 77c20b956b..20ff1a7c29 100644 --- a/esphome/components/web_server/web_server.cpp +++ b/esphome/components/web_server/web_server.cpp @@ -255,11 +255,7 @@ void DeferredUpdateEventSourceList::on_client_disconnect_(DeferredUpdateEventSou } #endif -WebServer::WebServer(web_server_base::WebServerBase *base) : base_(base) { -#ifdef USE_ESP32 - to_schedule_lock_ = xSemaphoreCreateMutex(); -#endif -} +WebServer::WebServer(web_server_base::WebServerBase *base) : base_(base) {} #ifdef USE_WEBSERVER_CSS_INCLUDE void WebServer::set_css_include(const char *css_include) { this->css_include_ = css_include; } @@ -308,30 +304,7 @@ void WebServer::setup() { // getting a lot of events this->set_interval(10000, [this]() { this->events_.try_send_nodefer("", "ping", millis(), 30000); }); } -void WebServer::loop() { -#ifdef USE_ESP32 - // Check atomic flag first to avoid taking semaphore when queue is empty - if (this->to_schedule_has_items_.load(std::memory_order_relaxed) && xSemaphoreTake(this->to_schedule_lock_, 0L)) { - std::function fn; - if (!to_schedule_.empty()) { - // scheduler execute things out of order which may lead to incorrect state - // this->defer(std::move(to_schedule_.front())); - // let's execute it directly from the loop - fn = std::move(to_schedule_.front()); - to_schedule_.pop_front(); - if (to_schedule_.empty()) { - this->to_schedule_has_items_.store(false, std::memory_order_relaxed); - } - } - xSemaphoreGive(this->to_schedule_lock_); - if (fn) { - fn(); - } - } -#endif - - this->events_.loop(); -} +void WebServer::loop() { this->events_.loop(); } void WebServer::dump_config() { ESP_LOGCONFIG(TAG, "Web Server:\n" @@ -526,13 +499,13 @@ void WebServer::handle_switch_request(AsyncWebServerRequest *request, const UrlM std::string data = this->switch_json(obj, obj->state, detail); request->send(200, "application/json", data.c_str()); } else if (match.method_equals("toggle")) { - this->schedule_([obj]() { obj->toggle(); }); + this->defer([obj]() { obj->toggle(); }); request->send(200); } else if (match.method_equals("turn_on")) { - this->schedule_([obj]() { obj->turn_on(); }); + this->defer([obj]() { obj->turn_on(); }); request->send(200); } else if (match.method_equals("turn_off")) { - this->schedule_([obj]() { obj->turn_off(); }); + this->defer([obj]() { obj->turn_off(); }); request->send(200); } else { request->send(404); @@ -568,7 +541,7 @@ void WebServer::handle_button_request(AsyncWebServerRequest *request, const UrlM std::string data = this->button_json(obj, detail); request->send(200, "application/json", data.c_str()); } else if (match.method_equals("press")) { - this->schedule_([obj]() { obj->press(); }); + this->defer([obj]() { obj->press(); }); request->send(200); return; } else { @@ -648,7 +621,7 @@ void WebServer::handle_fan_request(AsyncWebServerRequest *request, const UrlMatc std::string data = this->fan_json(obj, detail); request->send(200, "application/json", data.c_str()); } else if (match.method_equals("toggle")) { - this->schedule_([obj]() { obj->toggle().perform(); }); + this->defer([obj]() { obj->toggle().perform(); }); request->send(200); } else if (match.method_equals("turn_on") || match.method_equals("turn_off")) { auto call = match.method_equals("turn_on") ? obj->turn_on() : obj->turn_off(); @@ -680,7 +653,7 @@ void WebServer::handle_fan_request(AsyncWebServerRequest *request, const UrlMatc return; } } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); } else { request->send(404); @@ -729,7 +702,7 @@ void WebServer::handle_light_request(AsyncWebServerRequest *request, const UrlMa std::string data = this->light_json(obj, detail); request->send(200, "application/json", data.c_str()); } else if (match.method_equals("toggle")) { - this->schedule_([obj]() { obj->toggle().perform(); }); + this->defer([obj]() { obj->toggle().perform(); }); request->send(200); } else if (match.method_equals("turn_on")) { auto call = obj->turn_on(); @@ -786,7 +759,7 @@ void WebServer::handle_light_request(AsyncWebServerRequest *request, const UrlMa call.set_effect(effect); } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); } else if (match.method_equals("turn_off")) { auto call = obj->turn_off(); @@ -796,7 +769,7 @@ void WebServer::handle_light_request(AsyncWebServerRequest *request, const UrlMa call.set_transition_length(*transition * 1000); } } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); } else { request->send(404); @@ -881,7 +854,7 @@ void WebServer::handle_cover_request(AsyncWebServerRequest *request, const UrlMa } } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -939,7 +912,7 @@ void WebServer::handle_number_request(AsyncWebServerRequest *request, const UrlM call.set_value(*value); } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1014,7 +987,7 @@ void WebServer::handle_date_request(AsyncWebServerRequest *request, const UrlMat call.set_date(value); } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1073,7 +1046,7 @@ void WebServer::handle_time_request(AsyncWebServerRequest *request, const UrlMat call.set_time(value); } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1131,7 +1104,7 @@ void WebServer::handle_datetime_request(AsyncWebServerRequest *request, const Ur call.set_datetime(value); } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1248,7 +1221,7 @@ void WebServer::handle_select_request(AsyncWebServerRequest *request, const UrlM call.set_option(option.c_str()); // NOLINT } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1335,7 +1308,7 @@ void WebServer::handle_climate_request(AsyncWebServerRequest *request, const Url call.set_target_temperature(*target_temperature); } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1452,13 +1425,13 @@ void WebServer::handle_lock_request(AsyncWebServerRequest *request, const UrlMat std::string data = this->lock_json(obj, obj->state, detail); request->send(200, "application/json", data.c_str()); } else if (match.method_equals("lock")) { - this->schedule_([obj]() { obj->lock(); }); + this->defer([obj]() { obj->lock(); }); request->send(200); } else if (match.method_equals("unlock")) { - this->schedule_([obj]() { obj->unlock(); }); + this->defer([obj]() { obj->unlock(); }); request->send(200); } else if (match.method_equals("open")) { - this->schedule_([obj]() { obj->open(); }); + this->defer([obj]() { obj->open(); }); request->send(200); } else { request->send(404); @@ -1529,7 +1502,7 @@ void WebServer::handle_valve_request(AsyncWebServerRequest *request, const UrlMa } } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1594,7 +1567,7 @@ void WebServer::handle_alarm_control_panel_request(AsyncWebServerRequest *reques return; } - this->schedule_([call]() mutable { call.perform(); }); + this->defer([call]() mutable { call.perform(); }); request->send(200); return; } @@ -1695,7 +1668,7 @@ void WebServer::handle_update_request(AsyncWebServerRequest *request, const UrlM return; } - this->schedule_([obj]() mutable { obj->perform(); }); + this->defer([obj]() mutable { obj->perform(); }); request->send(200); return; } @@ -2072,17 +2045,6 @@ void WebServer::add_sorting_group(uint64_t group_id, const std::string &group_na } #endif -void WebServer::schedule_(std::function &&f) { -#ifdef USE_ESP32 - xSemaphoreTake(this->to_schedule_lock_, portMAX_DELAY); - to_schedule_.push_back(std::move(f)); - this->to_schedule_has_items_.store(true, std::memory_order_relaxed); - xSemaphoreGive(this->to_schedule_lock_); -#else - this->defer(std::move(f)); -#endif -} - } // namespace web_server } // namespace esphome #endif diff --git a/esphome/components/web_server/web_server.h b/esphome/components/web_server/web_server.h index 82b31ab656..0c15881d1e 100644 --- a/esphome/components/web_server/web_server.h +++ b/esphome/components/web_server/web_server.h @@ -14,12 +14,6 @@ #include #include #include -#ifdef USE_ESP32 -#include -#include -#include -#include -#endif #if USE_WEBSERVER_VERSION >= 2 extern const uint8_t ESPHOME_WEBSERVER_INDEX_HTML[] PROGMEM; @@ -504,7 +498,6 @@ class WebServer : public Controller, public Component, public AsyncWebHandler { protected: void add_sorting_info_(JsonObject &root, EntityBase *entity); - void schedule_(std::function &&f); web_server_base::WebServerBase *base_; #ifdef USE_ARDUINO DeferredUpdateEventSourceList events_; @@ -524,11 +517,6 @@ class WebServer : public Controller, public Component, public AsyncWebHandler { const char *js_include_{nullptr}; #endif bool expose_log_{true}; -#ifdef USE_ESP32 - std::deque> to_schedule_; - SemaphoreHandle_t to_schedule_lock_; - std::atomic to_schedule_has_items_{false}; -#endif }; } // namespace web_server diff --git a/esphome/core/helpers.cpp b/esphome/core/helpers.cpp index b4923c7af0..7d9b86fccd 100644 --- a/esphome/core/helpers.cpp +++ b/esphome/core/helpers.cpp @@ -645,7 +645,7 @@ void hsv_to_rgb(int hue, float saturation, float value, float &red, float &green } // System APIs -#if defined(USE_ESP8266) || defined(USE_RP2040) || defined(USE_HOST) +#if defined(USE_ESP8266) || defined(USE_RP2040) // ESP8266 doesn't have mutexes, but that shouldn't be an issue as it's single-core and non-preemptive OS. Mutex::Mutex() {} Mutex::~Mutex() {} @@ -658,6 +658,13 @@ Mutex::~Mutex() {} void Mutex::lock() { xSemaphoreTake(this->handle_, portMAX_DELAY); } bool Mutex::try_lock() { return xSemaphoreTake(this->handle_, 0) == pdTRUE; } void Mutex::unlock() { xSemaphoreGive(this->handle_); } +#elif defined(USE_HOST) +// Host platform uses std::mutex for proper thread synchronization +Mutex::Mutex() { handle_ = new std::mutex(); } +Mutex::~Mutex() { delete static_cast(handle_); } +void Mutex::lock() { static_cast(handle_)->lock(); } +bool Mutex::try_lock() { return static_cast(handle_)->try_lock(); } +void Mutex::unlock() { static_cast(handle_)->unlock(); } #endif #if defined(USE_ESP8266) diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 362f3d1fa4..d92cf07702 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -32,6 +32,10 @@ #include #endif +#ifdef USE_HOST +#include +#endif + #define HOT __attribute__((hot)) #define ESPDEPRECATED(msg, when) __attribute__((deprecated(msg))) #define ESPHOME_ALWAYS_INLINE __attribute__((always_inline)) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 5c01b4f3f4..515f6fd355 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -73,8 +73,6 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == SCHEDULER_DONT_RUN) return; - const auto now = this->millis_(); - // Create and populate the scheduler item auto item = make_unique(); item->component = component; @@ -83,6 +81,19 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type item->callback = std::move(func); item->remove = false; +#if !defined(USE_ESP8266) && !defined(USE_RP2040) + // Special handling for defer() (delay = 0, type = TIMEOUT) + // ESP8266 and RP2040 are excluded because they don't need thread-safe defer handling + if (delay == 0 && type == SchedulerItem::TIMEOUT) { + // Put in defer queue for guaranteed FIFO execution + LockGuard guard{this->lock_}; + this->defer_queue_.push_back(std::move(item)); + return; + } +#endif + + const auto now = this->millis_(); + // Type-specific setup if (type == SchedulerItem::INTERVAL) { item->interval = delay; @@ -209,6 +220,35 @@ optional HOT Scheduler::next_schedule_in() { return item->next_execution_ - now; } void HOT Scheduler::call() { +#if !defined(USE_ESP8266) && !defined(USE_RP2040) + // Process defer queue first to guarantee FIFO execution order for deferred items. + // Previously, defer() used the heap which gave undefined order for equal timestamps, + // causing race conditions on multi-core systems (ESP32, BK7200). + // With the defer queue: + // - Deferred items (delay=0) go directly to defer_queue_ in set_timer_common_ + // - Items execute in exact order they were deferred (FIFO guarantee) + // - No deferred items exist in to_add_, so processing order doesn't affect correctness + // ESP8266 and RP2040 don't use this queue - they fall back to the heap-based approach + // (ESP8266: single-core, RP2040: empty mutex implementation). + while (!this->defer_queue_.empty()) { + // The outer check is done without a lock for performance. If the queue + // appears non-empty, we lock and process an item. We don't need to check + // empty() again inside the lock because only this thread can remove items. + std::unique_ptr item; + { + LockGuard lock(this->lock_); + item = std::move(this->defer_queue_.front()); + this->defer_queue_.pop_front(); + } + + // Execute callback without holding lock to prevent deadlocks + // if the callback tries to call defer() again + if (!this->should_skip_item_(item.get())) { + this->execute_item_(item.get()); + } + } +#endif + const auto now = this->millis_(); this->process_to_add(); @@ -282,8 +322,6 @@ void HOT Scheduler::call() { this->pop_raw_(); continue; } - App.set_current_component(item->component); - #ifdef ESPHOME_DEBUG_SCHEDULER const char *item_name = item->get_name(); ESP_LOGV(TAG, "Running %s '%s/%s' with interval=%" PRIu32 " next_execution=%" PRIu64 " (now=%" PRIu64 ")", @@ -294,13 +332,7 @@ void HOT Scheduler::call() { // Warning: During callback(), a lot of stuff can happen, including: // - timeouts/intervals get added, potentially invalidating vector pointers // - timeouts/intervals get cancelled - { - uint32_t now_ms = millis(); - WarnIfComponentBlockingGuard guard{item->component, now_ms}; - item->callback(); - // Call finish to ensure blocking time is properly calculated and reported - guard.finish(); - } + this->execute_item_(item.get()); } { @@ -364,6 +396,26 @@ void HOT Scheduler::push_(std::unique_ptr item) { LockGuard guard{this->lock_}; this->to_add_.push_back(std::move(item)); } +// Helper function to check if item matches criteria for cancellation +bool HOT Scheduler::matches_item_(const std::unique_ptr &item, Component *component, + const char *name_cstr, SchedulerItem::Type type) { + if (item->component != component || item->type != type || item->remove) { + return false; + } + const char *item_name = item->get_name(); + return item_name != nullptr && strcmp(name_cstr, item_name) == 0; +} + +// Helper to execute a scheduler item +void HOT Scheduler::execute_item_(SchedulerItem *item) { + App.set_current_component(item->component); + + uint32_t now_ms = millis(); + WarnIfComponentBlockingGuard guard{item->component, now_ms}; + item->callback(); + guard.finish(); +} + // Common implementation for cancel operations bool HOT Scheduler::cancel_item_common_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type) { @@ -379,19 +431,28 @@ bool HOT Scheduler::cancel_item_common_(Component *component, bool is_static_str LockGuard guard{this->lock_}; bool ret = false; - for (auto &it : this->items_) { - const char *item_name = it->get_name(); - if (it->component == component && item_name != nullptr && strcmp(name_cstr, item_name) == 0 && it->type == type && - !it->remove) { - to_remove_++; - it->remove = true; + // Check all containers for matching items +#if !defined(USE_ESP8266) && !defined(USE_RP2040) + // Only check defer_queue_ on platforms that have it + for (auto &item : this->defer_queue_) { + if (this->matches_item_(item, component, name_cstr, type)) { + item->remove = true; ret = true; } } - for (auto &it : this->to_add_) { - const char *item_name = it->get_name(); - if (it->component == component && item_name != nullptr && strcmp(name_cstr, item_name) == 0 && it->type == type) { - it->remove = true; +#endif + + for (auto &item : this->items_) { + if (this->matches_item_(item, component, name_cstr, type)) { + item->remove = true; + ret = true; + this->to_remove_++; // Only track removals for heap items + } + } + + for (auto &item : this->to_add_) { + if (this->matches_item_(item, component, name_cstr, type)) { + item->remove = true; ret = true; } } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index a64968932e..bf5e63cccf 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -2,6 +2,7 @@ #include #include +#include #include "esphome/core/component.h" #include "esphome/core/helpers.h" @@ -142,9 +143,22 @@ class Scheduler { // Common implementation for cancel operations bool cancel_item_common_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); + private: bool cancel_item_(Component *component, const std::string &name, SchedulerItem::Type type); bool cancel_item_(Component *component, const char *name, SchedulerItem::Type type); + // Helper functions for cancel operations + bool matches_item_(const std::unique_ptr &item, Component *component, const char *name_cstr, + SchedulerItem::Type type); + + // Helper to execute a scheduler item + void execute_item_(SchedulerItem *item); + + // Helper to check if item should be skipped + bool should_skip_item_(const SchedulerItem *item) const { + return item->remove || (item->component != nullptr && item->component->is_failed()); + } + bool empty_() { this->cleanup_(); return this->items_.empty(); @@ -153,6 +167,13 @@ class Scheduler { Mutex lock_; std::vector> items_; std::vector> to_add_; +#if !defined(USE_ESP8266) && !defined(USE_RP2040) + // ESP8266 and RP2040 don't need the defer queue because: + // ESP8266: Single-core with no preemptive multitasking + // RP2040: Currently has empty mutex implementation in ESPHome + // Both platforms save 40 bytes of RAM by excluding this + std::deque> defer_queue_; // FIFO queue for defer() calls +#endif uint32_t last_millis_{0}; uint16_t millis_major_{0}; uint32_t to_remove_{0}; diff --git a/tests/integration/fixtures/defer_fifo_simple.yaml b/tests/integration/fixtures/defer_fifo_simple.yaml new file mode 100644 index 0000000000..db24ebf601 --- /dev/null +++ b/tests/integration/fixtures/defer_fifo_simple.yaml @@ -0,0 +1,109 @@ +esphome: + name: defer-fifo-simple + +host: + +logger: + level: DEBUG + +api: + services: + - service: test_set_timeout + then: + - lambda: |- + // Test set_timeout with 0 delay (direct scheduler call) + static int set_timeout_order = 0; + static bool set_timeout_passed = true; + + // Reset for this test + set_timeout_order = 0; + set_timeout_passed = true; + + ESP_LOGD("defer_test", "Testing set_timeout(0) for FIFO order..."); + for (int i = 0; i < 10; i++) { + int expected = i; + App.scheduler.set_timeout((Component*)nullptr, nullptr, 0, [expected]() { + ESP_LOGD("defer_test", "set_timeout(0) item %d executed, order %d", expected, set_timeout_order); + if (set_timeout_order != expected) { + ESP_LOGE("defer_test", "FIFO violation in set_timeout: expected %d but got execution order %d", expected, set_timeout_order); + set_timeout_passed = false; + } + set_timeout_order++; + + if (set_timeout_order == 10) { + if (set_timeout_passed) { + ESP_LOGI("defer_test", "✓ Test PASSED - set_timeout(0) maintains FIFO order"); + id(test_result)->trigger("passed"); + } else { + ESP_LOGE("defer_test", "✗ Test FAILED - set_timeout(0) executed out of order"); + id(test_result)->trigger("failed"); + } + id(test_complete)->trigger("test_finished"); + } + }); + } + + ESP_LOGD("defer_test", "Deferred 10 items using set_timeout(0), waiting for execution..."); + + - service: test_defer + then: + - lambda: |- + // Test defer() method (component method) + static int defer_order = 0; + static bool defer_passed = true; + + // Reset for this test + defer_order = 0; + defer_passed = true; + + ESP_LOGD("defer_test", "Testing defer() for FIFO order..."); + + // Create a test component class that exposes defer() + class TestComponent : public Component { + public: + void test_defer() { + for (int i = 0; i < 10; i++) { + int expected = i; + this->defer([expected]() { + ESP_LOGD("defer_test", "defer() item %d executed, order %d", expected, defer_order); + if (defer_order != expected) { + ESP_LOGE("defer_test", "FIFO violation in defer: expected %d but got execution order %d", expected, defer_order); + defer_passed = false; + } + defer_order++; + + if (defer_order == 10) { + if (defer_passed) { + ESP_LOGI("defer_test", "✓ Test PASSED - defer() maintains FIFO order"); + id(test_result)->trigger("passed"); + } else { + ESP_LOGE("defer_test", "✗ Test FAILED - defer() executed out of order"); + id(test_result)->trigger("failed"); + } + id(test_complete)->trigger("test_finished"); + } + }); + } + } + }; + + // Use a static instance so it doesn't go out of scope + static TestComponent test_component; + test_component.test_defer(); + + ESP_LOGD("defer_test", "Deferred 10 items using defer(), waiting for execution..."); + +event: + - platform: template + name: "Test Complete" + id: test_complete + device_class: button + event_types: + - "test_finished" + - platform: template + name: "Test Result" + id: test_result + device_class: button + event_types: + - "passed" + - "failed" diff --git a/tests/integration/fixtures/defer_stress.yaml b/tests/integration/fixtures/defer_stress.yaml new file mode 100644 index 0000000000..6df475229b --- /dev/null +++ b/tests/integration/fixtures/defer_stress.yaml @@ -0,0 +1,38 @@ +esphome: + name: defer-stress-test + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + components: [defer_stress_component] + +host: + +logger: + level: VERBOSE + +defer_stress_component: + id: defer_stress + +api: + services: + - service: run_stress_test + then: + - lambda: |- + id(defer_stress)->run_multi_thread_test(); + +event: + - platform: template + name: "Test Complete" + id: test_complete + device_class: button + event_types: + - "test_finished" + - platform: template + name: "Test Result" + id: test_result + device_class: button + event_types: + - "passed" + - "failed" diff --git a/tests/integration/fixtures/external_components/defer_stress_component/__init__.py b/tests/integration/fixtures/external_components/defer_stress_component/__init__.py new file mode 100644 index 0000000000..177e595f51 --- /dev/null +++ b/tests/integration/fixtures/external_components/defer_stress_component/__init__.py @@ -0,0 +1,19 @@ +import esphome.codegen as cg +import esphome.config_validation as cv +from esphome.const import CONF_ID + +defer_stress_component_ns = cg.esphome_ns.namespace("defer_stress_component") +DeferStressComponent = defer_stress_component_ns.class_( + "DeferStressComponent", cg.Component +) + +CONFIG_SCHEMA = cv.Schema( + { + cv.GenerateID(): cv.declare_id(DeferStressComponent), + } +).extend(cv.COMPONENT_SCHEMA) + + +async def to_code(config): + var = cg.new_Pvariable(config[CONF_ID]) + await cg.register_component(var, config) diff --git a/tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.cpp b/tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.cpp new file mode 100644 index 0000000000..21ca45947e --- /dev/null +++ b/tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.cpp @@ -0,0 +1,75 @@ +#include "defer_stress_component.h" +#include "esphome/core/log.h" +#include +#include +#include +#include + +namespace esphome { +namespace defer_stress_component { + +static const char *const TAG = "defer_stress"; + +void DeferStressComponent::setup() { ESP_LOGCONFIG(TAG, "DeferStressComponent setup"); } + +void DeferStressComponent::run_multi_thread_test() { + // Use member variables instead of static to avoid issues + this->total_defers_ = 0; + this->executed_defers_ = 0; + static constexpr int NUM_THREADS = 10; + static constexpr int DEFERS_PER_THREAD = 100; + + ESP_LOGI(TAG, "Starting defer stress test - multi-threaded concurrent defers"); + + // Ensure we're starting clean + ESP_LOGI(TAG, "Initial counters: total=%d, executed=%d", this->total_defers_.load(), this->executed_defers_.load()); + + // Track start time + auto start_time = std::chrono::steady_clock::now(); + + // Create threads + std::vector threads; + + ESP_LOGI(TAG, "Creating %d threads, each will defer %d callbacks", NUM_THREADS, DEFERS_PER_THREAD); + + threads.reserve(NUM_THREADS); + for (int i = 0; i < NUM_THREADS; i++) { + threads.emplace_back([this, i]() { + ESP_LOGV(TAG, "Thread %d starting", i); + // Each thread directly calls defer() without any locking + for (int j = 0; j < DEFERS_PER_THREAD; j++) { + int defer_id = this->total_defers_.fetch_add(1); + ESP_LOGV(TAG, "Thread %d calling defer for request %d", i, defer_id); + + // Capture this pointer safely for the lambda + auto *component = this; + + // Directly call defer() from this thread - no locking! + this->defer([component, i, j, defer_id]() { + component->executed_defers_.fetch_add(1); + ESP_LOGV(TAG, "Executed defer %d (thread %d, index %d)", defer_id, i, j); + }); + + ESP_LOGV(TAG, "Thread %d called defer for request %d successfully", i, defer_id); + + // Small random delay to increase contention + if (j % 10 == 0) { + std::this_thread::sleep_for(std::chrono::microseconds(100)); + } + } + ESP_LOGV(TAG, "Thread %d finished", i); + }); + } + + // Wait for all threads to complete + for (auto &t : threads) { + t.join(); + } + + auto end_time = std::chrono::steady_clock::now(); + auto thread_time = std::chrono::duration_cast(end_time - start_time).count(); + ESP_LOGI(TAG, "All threads finished in %lldms. Created %d defer requests", thread_time, this->total_defers_.load()); +} + +} // namespace defer_stress_component +} // namespace esphome diff --git a/tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.h b/tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.h new file mode 100644 index 0000000000..59b7565726 --- /dev/null +++ b/tests/integration/fixtures/external_components/defer_stress_component/defer_stress_component.h @@ -0,0 +1,20 @@ +#pragma once + +#include "esphome/core/component.h" +#include + +namespace esphome { +namespace defer_stress_component { + +class DeferStressComponent : public Component { + public: + void setup() override; + void run_multi_thread_test(); + + private: + std::atomic total_defers_{0}; + std::atomic executed_defers_{0}; +}; + +} // namespace defer_stress_component +} // namespace esphome diff --git a/tests/integration/test_defer_fifo_simple.py b/tests/integration/test_defer_fifo_simple.py new file mode 100644 index 0000000000..5a62a45786 --- /dev/null +++ b/tests/integration/test_defer_fifo_simple.py @@ -0,0 +1,117 @@ +"""Simple test that defer() maintains FIFO order.""" + +import asyncio + +from aioesphomeapi import EntityState, Event, EventInfo, UserService +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_defer_fifo_simple( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that defer() maintains FIFO order with a simple test.""" + + async with run_compiled(yaml_config), api_client_connected() as client: + # Verify we can connect + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "defer-fifo-simple" + + # List entities and services + entity_info, services = await asyncio.wait_for( + client.list_entities_services(), timeout=5.0 + ) + + # Find our test entities + test_complete_entity: EventInfo | None = None + test_result_entity: EventInfo | None = None + + for entity in entity_info: + if isinstance(entity, EventInfo): + if entity.object_id == "test_complete": + test_complete_entity = entity + elif entity.object_id == "test_result": + test_result_entity = entity + + assert test_complete_entity is not None, "test_complete event not found" + assert test_result_entity is not None, "test_result event not found" + + # Find our test services + test_set_timeout_service: UserService | None = None + test_defer_service: UserService | None = None + for service in services: + if service.name == "test_set_timeout": + test_set_timeout_service = service + elif service.name == "test_defer": + test_defer_service = service + + assert test_set_timeout_service is not None, ( + "test_set_timeout service not found" + ) + assert test_defer_service is not None, "test_defer service not found" + + # Get the event loop + loop = asyncio.get_running_loop() + + # Subscribe to states + # (events are delivered as EventStates through subscribe_states) + test_complete_future: asyncio.Future[bool] = loop.create_future() + test_result_future: asyncio.Future[bool] = loop.create_future() + + def on_state(state: EntityState) -> None: + if not isinstance(state, Event): + return + + if ( + state.key == test_complete_entity.key + and state.event_type == "test_finished" + and not test_complete_future.done() + ): + test_complete_future.set_result(True) + return + + if state.key == test_result_entity.key and not test_result_future.done(): + if state.event_type == "passed": + test_result_future.set_result(True) + elif state.event_type == "failed": + test_result_future.set_result(False) + + client.subscribe_states(on_state) + + # Test 1: Test set_timeout(0) + client.execute_service(test_set_timeout_service, {}) + + # Wait for first test completion + try: + await asyncio.wait_for(test_complete_future, timeout=5.0) + test1_passed = await asyncio.wait_for(test_result_future, timeout=1.0) + except asyncio.TimeoutError: + pytest.fail("Test set_timeout(0) did not complete within 5 seconds") + + assert test1_passed is True, ( + "set_timeout(0) FIFO test failed - items executed out of order" + ) + + # Reset futures for second test + test_complete_future = loop.create_future() + test_result_future = loop.create_future() + + # Test 2: Test defer() + client.execute_service(test_defer_service, {}) + + # Wait for second test completion + try: + await asyncio.wait_for(test_complete_future, timeout=5.0) + test2_passed = await asyncio.wait_for(test_result_future, timeout=1.0) + except asyncio.TimeoutError: + pytest.fail("Test defer() did not complete within 5 seconds") + + # Verify the test passed + assert test2_passed is True, ( + "defer() FIFO test failed - items executed out of order" + ) diff --git a/tests/integration/test_defer_stress.py b/tests/integration/test_defer_stress.py new file mode 100644 index 0000000000..f63ec8d25f --- /dev/null +++ b/tests/integration/test_defer_stress.py @@ -0,0 +1,137 @@ +"""Stress test for defer() thread safety with multiple threads.""" + +import asyncio +from pathlib import Path +import re + +from aioesphomeapi import UserService +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_defer_stress( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that defer() doesn't crash when called rapidly from multiple threads.""" + + # Get the absolute path to the external components directory + external_components_path = str( + Path(__file__).parent / "fixtures" / "external_components" + ) + + # Replace the placeholder in the YAML config with the actual path + yaml_config = yaml_config.replace( + "EXTERNAL_COMPONENT_PATH", external_components_path + ) + + # Create a future to signal test completion + loop = asyncio.get_event_loop() + test_complete_future: asyncio.Future[None] = loop.create_future() + + # Track executed defers and their order + executed_defers: set[int] = set() + thread_executions: dict[ + int, list[int] + ] = {} # thread_id -> list of indices in execution order + fifo_violations: list[str] = [] + + def on_log_line(line: str) -> None: + # Track all executed defers with thread and index info + match = re.search(r"Executed defer (\d+) \(thread (\d+), index (\d+)\)", line) + if not match: + return + + defer_id = int(match.group(1)) + thread_id = int(match.group(2)) + index = int(match.group(3)) + + executed_defers.add(defer_id) + + # Track execution order per thread + if thread_id not in thread_executions: + thread_executions[thread_id] = [] + + # Check FIFO ordering within thread + if thread_executions[thread_id] and thread_executions[thread_id][-1] >= index: + fifo_violations.append( + f"Thread {thread_id}: index {index} executed after " + f"{thread_executions[thread_id][-1]}" + ) + + thread_executions[thread_id].append(index) + + # Check if we've executed all 1000 defers (0-999) + if len(executed_defers) == 1000 and not test_complete_future.done(): + test_complete_future.set_result(None) + + async with ( + run_compiled(yaml_config, line_callback=on_log_line), + api_client_connected() as client, + ): + # Verify we can connect + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "defer-stress-test" + + # List entities and services + entity_info, services = await asyncio.wait_for( + client.list_entities_services(), timeout=5.0 + ) + + # Find our test service + run_stress_test_service: UserService | None = None + for service in services: + if service.name == "run_stress_test": + run_stress_test_service = service + break + + assert run_stress_test_service is not None, "run_stress_test service not found" + + # Call the run_stress_test service to start the test + client.execute_service(run_stress_test_service, {}) + + # Wait for all defers to execute (should be quick) + try: + await asyncio.wait_for(test_complete_future, timeout=5.0) + except asyncio.TimeoutError: + # Report how many we got + pytest.fail( + f"Stress test timed out. Only {len(executed_defers)} of " + f"1000 defers executed. Missing IDs: " + f"{sorted(set(range(1000)) - executed_defers)[:10]}..." + ) + + # Verify all defers executed + assert len(executed_defers) == 1000, ( + f"Expected 1000 defers, got {len(executed_defers)}" + ) + + # Verify we have all IDs from 0-999 + expected_ids = set(range(1000)) + missing_ids = expected_ids - executed_defers + assert not missing_ids, f"Missing defer IDs: {sorted(missing_ids)}" + + # Verify FIFO ordering was maintained within each thread + assert not fifo_violations, "FIFO ordering violations detected:\n" + "\n".join( + fifo_violations[:10] + ) + + # Verify each thread executed all its defers in order + for thread_id, indices in thread_executions.items(): + assert len(indices) == 100, ( + f"Thread {thread_id} executed {len(indices)} defers, expected 100" + ) + # Indices should be 0-99 in ascending order + assert indices == list(range(100)), ( + f"Thread {thread_id} executed indices out of order: {indices[:10]}..." + ) + + # If we got here without crashing and with proper ordering, the test passed + assert True, ( + "Test completed successfully - all 1000 defers executed with " + "FIFO ordering preserved" + ) From 3ca956cd6aea96f619a52d8b9211d69628ed3201 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 17:27:32 -0500 Subject: [PATCH 10/33] fix merge error --- esphome/core/scheduler.cpp | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index bcdeba291f..907a12d60e 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -58,31 +58,6 @@ static void validate_static_string(const char *name) { // iterating over them from the loop task is fine; but iterating from any other context requires the lock to be held to // avoid the main thread modifying the list while it is being accessed. -// Helper to cancel items by name - must be called with lock held -bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type) { - bool ret = false; - - for (auto &it : this->items_) { - const char *item_name = it->get_name(); - if (it->component == component && item_name != nullptr && strcmp(name, item_name) == 0 && it->type == type && - !it->remove) { - this->to_remove_++; - it->remove = true; - ret = true; - } - } - for (auto &it : this->to_add_) { - const char *item_name = it->get_name(); - if (it->component == component && item_name != nullptr && strcmp(name, item_name) == 0 && it->type == type && - !it->remove) { - it->remove = true; - ret = true; - } - } - - return ret; -} - // Common implementation for both timeout and interval void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, uint32_t delay, std::function func) { From 4c1b8c8b96575cc0ef6c8dc35862ff567f739d9b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 17:33:50 -0500 Subject: [PATCH 11/33] preen --- esphome/core/scheduler.cpp | 55 +++++++++++++++++--------------------- esphome/core/scheduler.h | 9 ++++--- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 907a12d60e..cda43f5552 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -401,15 +401,6 @@ void HOT Scheduler::pop_raw_() { std::pop_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); this->items_.pop_back(); } -// Helper function to check if item matches criteria for cancellation -bool HOT Scheduler::matches_item_(const std::unique_ptr &item, Component *component, - const char *name_cstr, SchedulerItem::Type type) { - if (item->component != component || item->type != type || item->remove) { - return false; - } - const char *item_name = item->get_name(); - return item_name != nullptr && strcmp(name_cstr, item_name) == 0; -} // Helper to execute a scheduler item void HOT Scheduler::execute_item_(SchedulerItem *item) { @@ -437,37 +428,41 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co return this->cancel_item_locked_(component, name_cstr, type); } +// Helper to mark items for cancellation and return count +template +size_t HOT Scheduler::mark_items_for_removal_(Container &items, Component *component, const char *name_cstr, + SchedulerItem::Type type) { + size_t cancelled_count = 0; + for (auto &item : items) { + if (item->component != component || item->type != type || item->remove) { + continue; + } + const char *item_name = item->get_name(); + if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + item->remove = true; + cancelled_count++; + } + } + return cancelled_count; +} + // Helper to cancel items by name - must be called with lock held bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { - bool ret = false; + size_t total_cancelled = 0; // Check all containers for matching items #if !defined(USE_ESP8266) && !defined(USE_RP2040) // Only check defer_queue_ on platforms that have it - for (auto &item : this->defer_queue_) { - if (this->matches_item_(item, component, name_cstr, type)) { - item->remove = true; - ret = true; - } - } + total_cancelled += this->mark_items_for_removal_(this->defer_queue_, component, name_cstr, type); #endif - for (auto &item : this->items_) { - if (this->matches_item_(item, component, name_cstr, type)) { - item->remove = true; - ret = true; - this->to_remove_++; // Only track removals for heap items - } - } + size_t items_cancelled = this->mark_items_for_removal_(this->items_, component, name_cstr, type); + this->to_remove_ += items_cancelled; // Only track removals for heap items + total_cancelled += items_cancelled; - for (auto &item : this->to_add_) { - if (this->matches_item_(item, component, name_cstr, type)) { - item->remove = true; - ret = true; - } - } + total_cancelled += this->mark_items_for_removal_(this->to_add_, component, name_cstr, type); - return ret; + return total_cancelled > 0; } uint64_t Scheduler::millis_() { diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 64ea4cf652..1f64d4c985 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -140,6 +140,11 @@ class Scheduler { // Helper to cancel items by name - must be called with lock held bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); + // Helper to mark items for cancellation and return count + template + size_t mark_items_for_removal_(Container &items, Component *component, const char *name_cstr, + SchedulerItem::Type type); + uint64_t millis_(); void cleanup_(); void pop_raw_(); @@ -147,10 +152,6 @@ class Scheduler { bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); private: - // Helper functions for cancel operations - bool matches_item_(const std::unique_ptr &item, Component *component, const char *name_cstr, - SchedulerItem::Type type); - // Helper to execute a scheduler item void execute_item_(SchedulerItem *item); From 3ffdd1d45140c5c511ffdef1decb159dd57c197b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 17:42:57 -0500 Subject: [PATCH 12/33] preen --- esphome/core/scheduler.cpp | 83 +++++++++++++++++++++++++++++++++----- esphome/core/scheduler.h | 16 ++++++-- 2 files changed, 85 insertions(+), 14 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index cda43f5552..473c465631 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -81,6 +81,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type item->callback = std::move(func); item->remove = false; + const auto now = this->millis_(); + #if !defined(USE_ESP8266) && !defined(USE_RP2040) // Special handling for defer() (delay = 0, type = TIMEOUT) // ESP8266 and RP2040 are excluded because they don't need thread-safe defer handling @@ -92,8 +94,6 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type } #endif - const auto now = this->millis_(); - // Type-specific setup if (type == SchedulerItem::INTERVAL) { item->interval = delay; @@ -428,10 +428,42 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co return this->cancel_item_locked_(component, name_cstr, type); } -// Helper to mark items for cancellation and return count +// Cancel heap items (items_ and to_add_) +bool HOT Scheduler::cancel_heap_item_(Component *component, bool is_static_string, const void *name_ptr, + SchedulerItem::Type type) { + // Get the name as const char* + const char *name_cstr = + is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); + + // Handle null or empty names + if (name_cstr == nullptr) + return false; + + // obtain lock because this function iterates and can be called from non-loop task context + LockGuard guard{this->lock_}; + return this->cancel_heap_items_locked_(component, name_cstr, type) > 0; +} + +// Cancel deferred items (defer_queue_) +bool HOT Scheduler::cancel_deferred_item_(Component *component, bool is_static_string, const void *name_ptr, + SchedulerItem::Type type) { + // Get the name as const char* + const char *name_cstr = + is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); + + // Handle null or empty names + if (name_cstr == nullptr) + return false; + + // obtain lock because this function iterates and can be called from non-loop task context + LockGuard guard{this->lock_}; + return this->cancel_deferred_items_locked_(this->defer_queue_, component, name_cstr, type) > 0; +} + +// Helper to mark deferred/to_add items for cancellation (no to_remove_ tracking needed) template -size_t HOT Scheduler::mark_items_for_removal_(Container &items, Component *component, const char *name_cstr, - SchedulerItem::Type type) { +size_t HOT Scheduler::cancel_deferred_items_locked_(Container &items, Component *component, const char *name_cstr, + SchedulerItem::Type type) { size_t cancelled_count = 0; for (auto &item : items) { if (item->component != component || item->type != type || item->remove) { @@ -446,6 +478,39 @@ size_t HOT Scheduler::mark_items_for_removal_(Container &items, Component *compo return cancelled_count; } +// Helper to mark heap items for cancellation and update to_remove_ count +size_t HOT Scheduler::cancel_heap_items_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { + size_t cancelled_count = 0; + + // Cancel items in the main heap + for (auto &item : this->items_) { + if (item->component != component || item->type != type || item->remove) { + continue; + } + const char *item_name = item->get_name(); + if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + item->remove = true; + cancelled_count++; + this->to_remove_++; // Track removals for heap items + } + } + + // Cancel items in to_add_ + for (auto &item : this->to_add_) { + if (item->component != component || item->type != type || item->remove) { + continue; + } + const char *item_name = item->get_name(); + if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + item->remove = true; + cancelled_count++; + // Don't track removals for to_add_ items + } + } + + return cancelled_count; +} + // Helper to cancel items by name - must be called with lock held bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { size_t total_cancelled = 0; @@ -453,14 +518,10 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Check all containers for matching items #if !defined(USE_ESP8266) && !defined(USE_RP2040) // Only check defer_queue_ on platforms that have it - total_cancelled += this->mark_items_for_removal_(this->defer_queue_, component, name_cstr, type); + total_cancelled += this->cancel_deferred_items_locked_(this->defer_queue_, component, name_cstr, type); #endif - size_t items_cancelled = this->mark_items_for_removal_(this->items_, component, name_cstr, type); - this->to_remove_ += items_cancelled; // Only track removals for heap items - total_cancelled += items_cancelled; - - total_cancelled += this->mark_items_for_removal_(this->to_add_, component, name_cstr, type); + total_cancelled += this->cancel_heap_items_locked_(component, name_cstr, type); return total_cancelled > 0; } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 1f64d4c985..239e59895f 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -140,10 +140,13 @@ class Scheduler { // Helper to cancel items by name - must be called with lock held bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); - // Helper to mark items for cancellation and return count + // Helper to mark deferred/to_add items for cancellation (no to_remove_ tracking needed) template - size_t mark_items_for_removal_(Container &items, Component *component, const char *name_cstr, - SchedulerItem::Type type); + size_t cancel_deferred_items_locked_(Container &items, Component *component, const char *name_cstr, + SchedulerItem::Type type); + + // Helper to mark heap items for cancellation and update to_remove_ count + size_t cancel_heap_items_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); uint64_t millis_(); void cleanup_(); @@ -151,6 +154,13 @@ class Scheduler { // Common implementation for cancel operations bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); + // Cancel heap items (items_ and to_add_) + bool cancel_heap_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); + + // Cancel deferred items (defer_queue_) + bool cancel_deferred_item_(Component *component, bool is_static_string, const void *name_ptr, + SchedulerItem::Type type); + private: // Helper to execute a scheduler item void execute_item_(SchedulerItem *item); From 758e5b89bb52a47ba09f0ad987cd7e6390ca37e5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 17:53:56 -0500 Subject: [PATCH 13/33] preen --- esphome/core/scheduler.cpp | 54 ++++++++++++++++++++++++++------------ esphome/core/scheduler.h | 8 +++--- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 473c465631..8b984aac34 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -68,7 +68,12 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == SCHEDULER_DONT_RUN) { // Still need to cancel existing timer if name is not empty if (name_cstr != nullptr && name_cstr[0] != '\0') { - this->cancel_item_(component, is_static_string, name_ptr, type); + LockGuard guard{this->lock_}; + if (delay == 0 && type == SchedulerItem::TIMEOUT) { + this->cancel_deferred_item_locked_(component, is_static_string, name_ptr, type); + } else { + this->cancel_heap_item_locked_(component, is_static_string, name_ptr, type); + } } return; } @@ -127,7 +132,16 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // If name is provided, do atomic cancel-and-add if (name_cstr != nullptr && name_cstr[0] != '\0') { // Cancel existing items - this->cancel_item_locked_(component, name_cstr, type); + if (delay == 0 && type == SchedulerItem::TIMEOUT) { + // For defer (delay=0), only cancel from defer queue + this->cancel_deferred_item_locked_(component, name_cstr, type); + } else if (type == SchedulerItem::TIMEOUT) { + // For regular timeouts, check all containers since we don't know where it might be + this->cancel_item_locked_(component, name_cstr, type); + } else { + // For intervals, only check heap items + this->cancel_heap_item_locked_(component, name_cstr, type); + } } // Add new item directly to to_add_ // since we have the lock held @@ -441,7 +455,7 @@ bool HOT Scheduler::cancel_heap_item_(Component *component, bool is_static_strin // obtain lock because this function iterates and can be called from non-loop task context LockGuard guard{this->lock_}; - return this->cancel_heap_items_locked_(component, name_cstr, type) > 0; + return this->cancel_heap_item_locked_(component, name_cstr, type) > 0; } // Cancel deferred items (defer_queue_) @@ -457,15 +471,15 @@ bool HOT Scheduler::cancel_deferred_item_(Component *component, bool is_static_s // obtain lock because this function iterates and can be called from non-loop task context LockGuard guard{this->lock_}; - return this->cancel_deferred_items_locked_(this->defer_queue_, component, name_cstr, type) > 0; + return this->cancel_deferred_item_locked_(component, name_cstr, type) > 0; } -// Helper to mark deferred/to_add items for cancellation (no to_remove_ tracking needed) -template -size_t HOT Scheduler::cancel_deferred_items_locked_(Container &items, Component *component, const char *name_cstr, - SchedulerItem::Type type) { +// Helper to mark deferred items for cancellation (no to_remove_ tracking needed) +size_t HOT Scheduler::cancel_deferred_item_locked_(Component *component, const char *name_cstr, + SchedulerItem::Type type) { size_t cancelled_count = 0; - for (auto &item : items) { +#if !defined(USE_ESP8266) && !defined(USE_RP2040) + for (auto &item : this->defer_queue_) { if (item->component != component || item->type != type || item->remove) { continue; } @@ -475,11 +489,15 @@ size_t HOT Scheduler::cancel_deferred_items_locked_(Container &items, Component cancelled_count++; } } +#else + // On platforms without defer queue, defer items go to the heap + cancelled_count = this->cancel_heap_item_locked_(component, name_cstr, type); +#endif return cancelled_count; } // Helper to mark heap items for cancellation and update to_remove_ count -size_t HOT Scheduler::cancel_heap_items_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { +size_t HOT Scheduler::cancel_heap_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { size_t cancelled_count = 0; // Cancel items in the main heap @@ -512,16 +530,18 @@ size_t HOT Scheduler::cancel_heap_items_locked_(Component *component, const char } // Helper to cancel items by name - must be called with lock held -bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { +bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type, + uint32_t delay) { size_t total_cancelled = 0; // Check all containers for matching items -#if !defined(USE_ESP8266) && !defined(USE_RP2040) - // Only check defer_queue_ on platforms that have it - total_cancelled += this->cancel_deferred_items_locked_(this->defer_queue_, component, name_cstr, type); -#endif - - total_cancelled += this->cancel_heap_items_locked_(component, name_cstr, type); + if (delay == 0 && type == SchedulerItem::TIMEOUT) { + // Cancel deferred items only + total_cancelled += this->cancel_deferred_item_locked_(component, name_cstr, type); + } else { + // Cancel heap items (items_ and to_add_) + total_cancelled += this->cancel_heap_item_locked_(component, name_cstr, type); + } return total_cancelled > 0; } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 239e59895f..489f9186ab 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -140,13 +140,11 @@ class Scheduler { // Helper to cancel items by name - must be called with lock held bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); - // Helper to mark deferred/to_add items for cancellation (no to_remove_ tracking needed) - template - size_t cancel_deferred_items_locked_(Container &items, Component *component, const char *name_cstr, - SchedulerItem::Type type); + // Helper to mark deferred items for cancellation (no to_remove_ tracking needed) + size_t cancel_deferred_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); // Helper to mark heap items for cancellation and update to_remove_ count - size_t cancel_heap_items_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); + size_t cancel_heap_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); uint64_t millis_(); void cleanup_(); From e355ce04f7e7a2f35122187bbba2110d49b3bed6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:01:21 -0500 Subject: [PATCH 14/33] preen --- esphome/core/scheduler.cpp | 71 +++++--------------------------------- esphome/core/scheduler.h | 2 ++ 2 files changed, 11 insertions(+), 62 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 8b984aac34..7a0d33ee1b 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -68,12 +68,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == SCHEDULER_DONT_RUN) { // Still need to cancel existing timer if name is not empty if (name_cstr != nullptr && name_cstr[0] != '\0') { - LockGuard guard{this->lock_}; - if (delay == 0 && type == SchedulerItem::TIMEOUT) { - this->cancel_deferred_item_locked_(component, is_static_string, name_ptr, type); - } else { - this->cancel_heap_item_locked_(component, is_static_string, name_ptr, type); - } + this->cancel_item_(component, is_static_string, name_ptr, type); } return; } @@ -132,16 +127,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // If name is provided, do atomic cancel-and-add if (name_cstr != nullptr && name_cstr[0] != '\0') { // Cancel existing items - if (delay == 0 && type == SchedulerItem::TIMEOUT) { - // For defer (delay=0), only cancel from defer queue - this->cancel_deferred_item_locked_(component, name_cstr, type); - } else if (type == SchedulerItem::TIMEOUT) { - // For regular timeouts, check all containers since we don't know where it might be - this->cancel_item_locked_(component, name_cstr, type); - } else { - // For intervals, only check heap items - this->cancel_heap_item_locked_(component, name_cstr, type); - } + this->cancel_item_locked_(component, name_cstr, type); } // Add new item directly to to_add_ // since we have the lock held @@ -442,43 +428,11 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co return this->cancel_item_locked_(component, name_cstr, type); } -// Cancel heap items (items_ and to_add_) -bool HOT Scheduler::cancel_heap_item_(Component *component, bool is_static_string, const void *name_ptr, - SchedulerItem::Type type) { - // Get the name as const char* - const char *name_cstr = - is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); - - // Handle null or empty names - if (name_cstr == nullptr) - return false; - - // obtain lock because this function iterates and can be called from non-loop task context - LockGuard guard{this->lock_}; - return this->cancel_heap_item_locked_(component, name_cstr, type) > 0; -} - -// Cancel deferred items (defer_queue_) -bool HOT Scheduler::cancel_deferred_item_(Component *component, bool is_static_string, const void *name_ptr, - SchedulerItem::Type type) { - // Get the name as const char* - const char *name_cstr = - is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); - - // Handle null or empty names - if (name_cstr == nullptr) - return false; - - // obtain lock because this function iterates and can be called from non-loop task context - LockGuard guard{this->lock_}; - return this->cancel_deferred_item_locked_(component, name_cstr, type) > 0; -} - +#if !defined(USE_ESP8266) && !defined(USE_RP2040) // Helper to mark deferred items for cancellation (no to_remove_ tracking needed) size_t HOT Scheduler::cancel_deferred_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { size_t cancelled_count = 0; -#if !defined(USE_ESP8266) && !defined(USE_RP2040) for (auto &item : this->defer_queue_) { if (item->component != component || item->type != type || item->remove) { continue; @@ -489,12 +443,9 @@ size_t HOT Scheduler::cancel_deferred_item_locked_(Component *component, const c cancelled_count++; } } -#else - // On platforms without defer queue, defer items go to the heap - cancelled_count = this->cancel_heap_item_locked_(component, name_cstr, type); -#endif return cancelled_count; } +#endif // Helper to mark heap items for cancellation and update to_remove_ count size_t HOT Scheduler::cancel_heap_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { @@ -530,18 +481,14 @@ size_t HOT Scheduler::cancel_heap_item_locked_(Component *component, const char } // Helper to cancel items by name - must be called with lock held -bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type, - uint32_t delay) { +bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { size_t total_cancelled = 0; // Check all containers for matching items - if (delay == 0 && type == SchedulerItem::TIMEOUT) { - // Cancel deferred items only - total_cancelled += this->cancel_deferred_item_locked_(component, name_cstr, type); - } else { - // Cancel heap items (items_ and to_add_) - total_cancelled += this->cancel_heap_item_locked_(component, name_cstr, type); - } +#if !defined(USE_ESP8266) && !defined(USE_RP2040) + total_cancelled += this->cancel_deferred_item_locked_(component, name_cstr, type); +#endif + total_cancelled += this->cancel_heap_item_locked_(component, name_cstr, type); return total_cancelled > 0; } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 489f9186ab..844dfc600f 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -140,8 +140,10 @@ class Scheduler { // Helper to cancel items by name - must be called with lock held bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); +#if !defined(USE_ESP8266) && !defined(USE_RP2040) // Helper to mark deferred items for cancellation (no to_remove_ tracking needed) size_t cancel_deferred_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); +#endif // Helper to mark heap items for cancellation and update to_remove_ count size_t cancel_heap_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); From 48957aee8bb243c8e09bb2a6de6454345a2211a8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:03:53 -0500 Subject: [PATCH 15/33] preen --- esphome/core/scheduler.cpp | 3 ++- esphome/core/scheduler.h | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 7a0d33ee1b..e8e21cd38d 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -89,6 +89,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == 0 && type == SchedulerItem::TIMEOUT) { // Put in defer queue for guaranteed FIFO execution LockGuard guard{this->lock_}; + this->cancel_deferred_item_locked_(component, is_static_string, name_ptr, type); this->defer_queue_.push_back(std::move(item)); return; } @@ -434,7 +435,7 @@ size_t HOT Scheduler::cancel_deferred_item_locked_(Component *component, const c SchedulerItem::Type type) { size_t cancelled_count = 0; for (auto &item : this->defer_queue_) { - if (item->component != component || item->type != type || item->remove) { + if (item->component != component || item->remove) { continue; } const char *item_name = item->get_name(); diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 844dfc600f..06a0543881 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -157,9 +157,11 @@ class Scheduler { // Cancel heap items (items_ and to_add_) bool cancel_heap_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); +#if !defined(USE_ESP8266) && !defined(USE_RP2040) // Cancel deferred items (defer_queue_) bool cancel_deferred_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); +#endif private: // Helper to execute a scheduler item From 4900f7c7ca41ce99503432a0f3e80b93ca459bbc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:07:34 -0500 Subject: [PATCH 16/33] preen --- esphome/core/scheduler.cpp | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index e8e21cd38d..773b5775d2 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -68,7 +68,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == SCHEDULER_DONT_RUN) { // Still need to cancel existing timer if name is not empty if (name_cstr != nullptr && name_cstr[0] != '\0') { - this->cancel_item_(component, is_static_string, name_ptr, type); + LockGuard guard{this->lock_}; + this->cancel_item_locked_(component, name_cstr, type); } return; } @@ -89,7 +90,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == 0 && type == SchedulerItem::TIMEOUT) { // Put in defer queue for guaranteed FIFO execution LockGuard guard{this->lock_}; - this->cancel_deferred_item_locked_(component, is_static_string, name_ptr, type); + this->cancel_item_locked_(component, is_static_string, name_ptr, type); this->defer_queue_.push_back(std::move(item)); return; } @@ -429,25 +430,6 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co return this->cancel_item_locked_(component, name_cstr, type); } -#if !defined(USE_ESP8266) && !defined(USE_RP2040) -// Helper to mark deferred items for cancellation (no to_remove_ tracking needed) -size_t HOT Scheduler::cancel_deferred_item_locked_(Component *component, const char *name_cstr, - SchedulerItem::Type type) { - size_t cancelled_count = 0; - for (auto &item : this->defer_queue_) { - if (item->component != component || item->remove) { - continue; - } - const char *item_name = item->get_name(); - if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { - item->remove = true; - cancelled_count++; - } - } - return cancelled_count; -} -#endif - // Helper to mark heap items for cancellation and update to_remove_ count size_t HOT Scheduler::cancel_heap_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { size_t cancelled_count = 0; @@ -487,7 +469,17 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Check all containers for matching items #if !defined(USE_ESP8266) && !defined(USE_RP2040) - total_cancelled += this->cancel_deferred_item_locked_(component, name_cstr, type); + // Cancel items in defer queue + for (auto &item : this->defer_queue_) { + if (item->component != component || item->type != type || item->remove) { + continue; + } + const char *item_name = item->get_name(); + if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + item->remove = true; + total_cancelled++; + } + } #endif total_cancelled += this->cancel_heap_item_locked_(component, name_cstr, type); From 939d01dd99d56101b671fe88d0423f86337af207 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:08:50 -0500 Subject: [PATCH 17/33] preen --- esphome/core/scheduler.cpp | 60 +++++++++++++++++--------------------- esphome/core/scheduler.h | 8 ----- 2 files changed, 26 insertions(+), 42 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 773b5775d2..1a38b6a83e 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -430,39 +430,6 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co return this->cancel_item_locked_(component, name_cstr, type); } -// Helper to mark heap items for cancellation and update to_remove_ count -size_t HOT Scheduler::cancel_heap_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { - size_t cancelled_count = 0; - - // Cancel items in the main heap - for (auto &item : this->items_) { - if (item->component != component || item->type != type || item->remove) { - continue; - } - const char *item_name = item->get_name(); - if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { - item->remove = true; - cancelled_count++; - this->to_remove_++; // Track removals for heap items - } - } - - // Cancel items in to_add_ - for (auto &item : this->to_add_) { - if (item->component != component || item->type != type || item->remove) { - continue; - } - const char *item_name = item->get_name(); - if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { - item->remove = true; - cancelled_count++; - // Don't track removals for to_add_ items - } - } - - return cancelled_count; -} - // Helper to cancel items by name - must be called with lock held bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { size_t total_cancelled = 0; @@ -481,7 +448,32 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c } } #endif - total_cancelled += this->cancel_heap_item_locked_(component, name_cstr, type); + + // Cancel items in the main heap + for (auto &item : this->items_) { + if (item->component != component || item->type != type || item->remove) { + continue; + } + const char *item_name = item->get_name(); + if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + item->remove = true; + total_cancelled++; + this->to_remove_++; // Track removals for heap items + } + } + + // Cancel items in to_add_ + for (auto &item : this->to_add_) { + if (item->component != component || item->type != type || item->remove) { + continue; + } + const char *item_name = item->get_name(); + if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + item->remove = true; + total_cancelled++; + // Don't track removals for to_add_ items + } + } return total_cancelled > 0; } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 06a0543881..13e36601e3 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -140,14 +140,6 @@ class Scheduler { // Helper to cancel items by name - must be called with lock held bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); -#if !defined(USE_ESP8266) && !defined(USE_RP2040) - // Helper to mark deferred items for cancellation (no to_remove_ tracking needed) - size_t cancel_deferred_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); -#endif - - // Helper to mark heap items for cancellation and update to_remove_ count - size_t cancel_heap_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type); - uint64_t millis_(); void cleanup_(); void pop_raw_(); From 52d3dba89c57c17c082039d4e43605bf5d2465f7 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:11:04 -0500 Subject: [PATCH 18/33] adjust --- esphome/core/scheduler.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 13e36601e3..8dde00cff5 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -149,12 +149,6 @@ class Scheduler { // Cancel heap items (items_ and to_add_) bool cancel_heap_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); -#if !defined(USE_ESP8266) && !defined(USE_RP2040) - // Cancel deferred items (defer_queue_) - bool cancel_deferred_item_(Component *component, bool is_static_string, const void *name_ptr, - SchedulerItem::Type type); -#endif - private: // Helper to execute a scheduler item void execute_item_(SchedulerItem *item); From 462b44ee23d2946f41e7f494c1c5e7bcae243df6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:15:11 -0500 Subject: [PATCH 19/33] adjust --- esphome/core/scheduler.h | 3 - .../fixtures/defer_fifo_simple.yaml | 109 -------------- tests/integration/fixtures/defer_stress.yaml | 38 ----- tests/integration/test_defer_fifo_simple.py | 117 --------------- tests/integration/test_defer_stress.py | 137 ------------------ 5 files changed, 404 deletions(-) delete mode 100644 tests/integration/fixtures/defer_fifo_simple.yaml delete mode 100644 tests/integration/fixtures/defer_stress.yaml delete mode 100644 tests/integration/test_defer_fifo_simple.py delete mode 100644 tests/integration/test_defer_stress.py diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 8dde00cff5..27d95f5c05 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -146,9 +146,6 @@ class Scheduler { // Common implementation for cancel operations bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); - // Cancel heap items (items_ and to_add_) - bool cancel_heap_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); - private: // Helper to execute a scheduler item void execute_item_(SchedulerItem *item); diff --git a/tests/integration/fixtures/defer_fifo_simple.yaml b/tests/integration/fixtures/defer_fifo_simple.yaml deleted file mode 100644 index db24ebf601..0000000000 --- a/tests/integration/fixtures/defer_fifo_simple.yaml +++ /dev/null @@ -1,109 +0,0 @@ -esphome: - name: defer-fifo-simple - -host: - -logger: - level: DEBUG - -api: - services: - - service: test_set_timeout - then: - - lambda: |- - // Test set_timeout with 0 delay (direct scheduler call) - static int set_timeout_order = 0; - static bool set_timeout_passed = true; - - // Reset for this test - set_timeout_order = 0; - set_timeout_passed = true; - - ESP_LOGD("defer_test", "Testing set_timeout(0) for FIFO order..."); - for (int i = 0; i < 10; i++) { - int expected = i; - App.scheduler.set_timeout((Component*)nullptr, nullptr, 0, [expected]() { - ESP_LOGD("defer_test", "set_timeout(0) item %d executed, order %d", expected, set_timeout_order); - if (set_timeout_order != expected) { - ESP_LOGE("defer_test", "FIFO violation in set_timeout: expected %d but got execution order %d", expected, set_timeout_order); - set_timeout_passed = false; - } - set_timeout_order++; - - if (set_timeout_order == 10) { - if (set_timeout_passed) { - ESP_LOGI("defer_test", "✓ Test PASSED - set_timeout(0) maintains FIFO order"); - id(test_result)->trigger("passed"); - } else { - ESP_LOGE("defer_test", "✗ Test FAILED - set_timeout(0) executed out of order"); - id(test_result)->trigger("failed"); - } - id(test_complete)->trigger("test_finished"); - } - }); - } - - ESP_LOGD("defer_test", "Deferred 10 items using set_timeout(0), waiting for execution..."); - - - service: test_defer - then: - - lambda: |- - // Test defer() method (component method) - static int defer_order = 0; - static bool defer_passed = true; - - // Reset for this test - defer_order = 0; - defer_passed = true; - - ESP_LOGD("defer_test", "Testing defer() for FIFO order..."); - - // Create a test component class that exposes defer() - class TestComponent : public Component { - public: - void test_defer() { - for (int i = 0; i < 10; i++) { - int expected = i; - this->defer([expected]() { - ESP_LOGD("defer_test", "defer() item %d executed, order %d", expected, defer_order); - if (defer_order != expected) { - ESP_LOGE("defer_test", "FIFO violation in defer: expected %d but got execution order %d", expected, defer_order); - defer_passed = false; - } - defer_order++; - - if (defer_order == 10) { - if (defer_passed) { - ESP_LOGI("defer_test", "✓ Test PASSED - defer() maintains FIFO order"); - id(test_result)->trigger("passed"); - } else { - ESP_LOGE("defer_test", "✗ Test FAILED - defer() executed out of order"); - id(test_result)->trigger("failed"); - } - id(test_complete)->trigger("test_finished"); - } - }); - } - } - }; - - // Use a static instance so it doesn't go out of scope - static TestComponent test_component; - test_component.test_defer(); - - ESP_LOGD("defer_test", "Deferred 10 items using defer(), waiting for execution..."); - -event: - - platform: template - name: "Test Complete" - id: test_complete - device_class: button - event_types: - - "test_finished" - - platform: template - name: "Test Result" - id: test_result - device_class: button - event_types: - - "passed" - - "failed" diff --git a/tests/integration/fixtures/defer_stress.yaml b/tests/integration/fixtures/defer_stress.yaml deleted file mode 100644 index 6df475229b..0000000000 --- a/tests/integration/fixtures/defer_stress.yaml +++ /dev/null @@ -1,38 +0,0 @@ -esphome: - name: defer-stress-test - -external_components: - - source: - type: local - path: EXTERNAL_COMPONENT_PATH - components: [defer_stress_component] - -host: - -logger: - level: VERBOSE - -defer_stress_component: - id: defer_stress - -api: - services: - - service: run_stress_test - then: - - lambda: |- - id(defer_stress)->run_multi_thread_test(); - -event: - - platform: template - name: "Test Complete" - id: test_complete - device_class: button - event_types: - - "test_finished" - - platform: template - name: "Test Result" - id: test_result - device_class: button - event_types: - - "passed" - - "failed" diff --git a/tests/integration/test_defer_fifo_simple.py b/tests/integration/test_defer_fifo_simple.py deleted file mode 100644 index 5a62a45786..0000000000 --- a/tests/integration/test_defer_fifo_simple.py +++ /dev/null @@ -1,117 +0,0 @@ -"""Simple test that defer() maintains FIFO order.""" - -import asyncio - -from aioesphomeapi import EntityState, Event, EventInfo, UserService -import pytest - -from .types import APIClientConnectedFactory, RunCompiledFunction - - -@pytest.mark.asyncio -async def test_defer_fifo_simple( - yaml_config: str, - run_compiled: RunCompiledFunction, - api_client_connected: APIClientConnectedFactory, -) -> None: - """Test that defer() maintains FIFO order with a simple test.""" - - async with run_compiled(yaml_config), api_client_connected() as client: - # Verify we can connect - device_info = await client.device_info() - assert device_info is not None - assert device_info.name == "defer-fifo-simple" - - # List entities and services - entity_info, services = await asyncio.wait_for( - client.list_entities_services(), timeout=5.0 - ) - - # Find our test entities - test_complete_entity: EventInfo | None = None - test_result_entity: EventInfo | None = None - - for entity in entity_info: - if isinstance(entity, EventInfo): - if entity.object_id == "test_complete": - test_complete_entity = entity - elif entity.object_id == "test_result": - test_result_entity = entity - - assert test_complete_entity is not None, "test_complete event not found" - assert test_result_entity is not None, "test_result event not found" - - # Find our test services - test_set_timeout_service: UserService | None = None - test_defer_service: UserService | None = None - for service in services: - if service.name == "test_set_timeout": - test_set_timeout_service = service - elif service.name == "test_defer": - test_defer_service = service - - assert test_set_timeout_service is not None, ( - "test_set_timeout service not found" - ) - assert test_defer_service is not None, "test_defer service not found" - - # Get the event loop - loop = asyncio.get_running_loop() - - # Subscribe to states - # (events are delivered as EventStates through subscribe_states) - test_complete_future: asyncio.Future[bool] = loop.create_future() - test_result_future: asyncio.Future[bool] = loop.create_future() - - def on_state(state: EntityState) -> None: - if not isinstance(state, Event): - return - - if ( - state.key == test_complete_entity.key - and state.event_type == "test_finished" - and not test_complete_future.done() - ): - test_complete_future.set_result(True) - return - - if state.key == test_result_entity.key and not test_result_future.done(): - if state.event_type == "passed": - test_result_future.set_result(True) - elif state.event_type == "failed": - test_result_future.set_result(False) - - client.subscribe_states(on_state) - - # Test 1: Test set_timeout(0) - client.execute_service(test_set_timeout_service, {}) - - # Wait for first test completion - try: - await asyncio.wait_for(test_complete_future, timeout=5.0) - test1_passed = await asyncio.wait_for(test_result_future, timeout=1.0) - except asyncio.TimeoutError: - pytest.fail("Test set_timeout(0) did not complete within 5 seconds") - - assert test1_passed is True, ( - "set_timeout(0) FIFO test failed - items executed out of order" - ) - - # Reset futures for second test - test_complete_future = loop.create_future() - test_result_future = loop.create_future() - - # Test 2: Test defer() - client.execute_service(test_defer_service, {}) - - # Wait for second test completion - try: - await asyncio.wait_for(test_complete_future, timeout=5.0) - test2_passed = await asyncio.wait_for(test_result_future, timeout=1.0) - except asyncio.TimeoutError: - pytest.fail("Test defer() did not complete within 5 seconds") - - # Verify the test passed - assert test2_passed is True, ( - "defer() FIFO test failed - items executed out of order" - ) diff --git a/tests/integration/test_defer_stress.py b/tests/integration/test_defer_stress.py deleted file mode 100644 index f63ec8d25f..0000000000 --- a/tests/integration/test_defer_stress.py +++ /dev/null @@ -1,137 +0,0 @@ -"""Stress test for defer() thread safety with multiple threads.""" - -import asyncio -from pathlib import Path -import re - -from aioesphomeapi import UserService -import pytest - -from .types import APIClientConnectedFactory, RunCompiledFunction - - -@pytest.mark.asyncio -async def test_defer_stress( - yaml_config: str, - run_compiled: RunCompiledFunction, - api_client_connected: APIClientConnectedFactory, -) -> None: - """Test that defer() doesn't crash when called rapidly from multiple threads.""" - - # Get the absolute path to the external components directory - external_components_path = str( - Path(__file__).parent / "fixtures" / "external_components" - ) - - # Replace the placeholder in the YAML config with the actual path - yaml_config = yaml_config.replace( - "EXTERNAL_COMPONENT_PATH", external_components_path - ) - - # Create a future to signal test completion - loop = asyncio.get_event_loop() - test_complete_future: asyncio.Future[None] = loop.create_future() - - # Track executed defers and their order - executed_defers: set[int] = set() - thread_executions: dict[ - int, list[int] - ] = {} # thread_id -> list of indices in execution order - fifo_violations: list[str] = [] - - def on_log_line(line: str) -> None: - # Track all executed defers with thread and index info - match = re.search(r"Executed defer (\d+) \(thread (\d+), index (\d+)\)", line) - if not match: - return - - defer_id = int(match.group(1)) - thread_id = int(match.group(2)) - index = int(match.group(3)) - - executed_defers.add(defer_id) - - # Track execution order per thread - if thread_id not in thread_executions: - thread_executions[thread_id] = [] - - # Check FIFO ordering within thread - if thread_executions[thread_id] and thread_executions[thread_id][-1] >= index: - fifo_violations.append( - f"Thread {thread_id}: index {index} executed after " - f"{thread_executions[thread_id][-1]}" - ) - - thread_executions[thread_id].append(index) - - # Check if we've executed all 1000 defers (0-999) - if len(executed_defers) == 1000 and not test_complete_future.done(): - test_complete_future.set_result(None) - - async with ( - run_compiled(yaml_config, line_callback=on_log_line), - api_client_connected() as client, - ): - # Verify we can connect - device_info = await client.device_info() - assert device_info is not None - assert device_info.name == "defer-stress-test" - - # List entities and services - entity_info, services = await asyncio.wait_for( - client.list_entities_services(), timeout=5.0 - ) - - # Find our test service - run_stress_test_service: UserService | None = None - for service in services: - if service.name == "run_stress_test": - run_stress_test_service = service - break - - assert run_stress_test_service is not None, "run_stress_test service not found" - - # Call the run_stress_test service to start the test - client.execute_service(run_stress_test_service, {}) - - # Wait for all defers to execute (should be quick) - try: - await asyncio.wait_for(test_complete_future, timeout=5.0) - except asyncio.TimeoutError: - # Report how many we got - pytest.fail( - f"Stress test timed out. Only {len(executed_defers)} of " - f"1000 defers executed. Missing IDs: " - f"{sorted(set(range(1000)) - executed_defers)[:10]}..." - ) - - # Verify all defers executed - assert len(executed_defers) == 1000, ( - f"Expected 1000 defers, got {len(executed_defers)}" - ) - - # Verify we have all IDs from 0-999 - expected_ids = set(range(1000)) - missing_ids = expected_ids - executed_defers - assert not missing_ids, f"Missing defer IDs: {sorted(missing_ids)}" - - # Verify FIFO ordering was maintained within each thread - assert not fifo_violations, "FIFO ordering violations detected:\n" + "\n".join( - fifo_violations[:10] - ) - - # Verify each thread executed all its defers in order - for thread_id, indices in thread_executions.items(): - assert len(indices) == 100, ( - f"Thread {thread_id} executed {len(indices)} defers, expected 100" - ) - # Indices should be 0-99 in ascending order - assert indices == list(range(100)), ( - f"Thread {thread_id} executed indices out of order: {indices[:10]}..." - ) - - # If we got here without crashing and with proper ordering, the test passed - assert True, ( - "Test completed successfully - all 1000 defers executed with " - "FIFO ordering preserved" - ) From 339a3270f6a21cf524923c115ca278f39baaf8bd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:16:25 -0500 Subject: [PATCH 20/33] adjust --- .../fixtures/scheduler_defer_fifo_simple.yaml | 109 ++++++++++++++ .../fixtures/scheduler_defer_stress.yaml | 38 +++++ .../test_scheduler_defer_fifo_simple.py | 117 +++++++++++++++ .../test_scheduler_defer_stress.py | 137 ++++++++++++++++++ 4 files changed, 401 insertions(+) create mode 100644 tests/integration/fixtures/scheduler_defer_fifo_simple.yaml create mode 100644 tests/integration/fixtures/scheduler_defer_stress.yaml create mode 100644 tests/integration/test_scheduler_defer_fifo_simple.py create mode 100644 tests/integration/test_scheduler_defer_stress.py diff --git a/tests/integration/fixtures/scheduler_defer_fifo_simple.yaml b/tests/integration/fixtures/scheduler_defer_fifo_simple.yaml new file mode 100644 index 0000000000..7384082ac2 --- /dev/null +++ b/tests/integration/fixtures/scheduler_defer_fifo_simple.yaml @@ -0,0 +1,109 @@ +esphome: + name: scheduler-defer-fifo-simple + +host: + +logger: + level: DEBUG + +api: + services: + - service: test_set_timeout + then: + - lambda: |- + // Test set_timeout with 0 delay (direct scheduler call) + static int set_timeout_order = 0; + static bool set_timeout_passed = true; + + // Reset for this test + set_timeout_order = 0; + set_timeout_passed = true; + + ESP_LOGD("defer_test", "Testing set_timeout(0) for FIFO order..."); + for (int i = 0; i < 10; i++) { + int expected = i; + App.scheduler.set_timeout((Component*)nullptr, nullptr, 0, [expected]() { + ESP_LOGD("defer_test", "set_timeout(0) item %d executed, order %d", expected, set_timeout_order); + if (set_timeout_order != expected) { + ESP_LOGE("defer_test", "FIFO violation in set_timeout: expected %d but got execution order %d", expected, set_timeout_order); + set_timeout_passed = false; + } + set_timeout_order++; + + if (set_timeout_order == 10) { + if (set_timeout_passed) { + ESP_LOGI("defer_test", "✓ Test PASSED - set_timeout(0) maintains FIFO order"); + id(test_result)->trigger("passed"); + } else { + ESP_LOGE("defer_test", "✗ Test FAILED - set_timeout(0) executed out of order"); + id(test_result)->trigger("failed"); + } + id(test_complete)->trigger("test_finished"); + } + }); + } + + ESP_LOGD("defer_test", "Deferred 10 items using set_timeout(0), waiting for execution..."); + + - service: test_defer + then: + - lambda: |- + // Test defer() method (component method) + static int defer_order = 0; + static bool defer_passed = true; + + // Reset for this test + defer_order = 0; + defer_passed = true; + + ESP_LOGD("defer_test", "Testing defer() for FIFO order..."); + + // Create a test component class that exposes defer() + class TestComponent : public Component { + public: + void test_defer() { + for (int i = 0; i < 10; i++) { + int expected = i; + this->defer([expected]() { + ESP_LOGD("defer_test", "defer() item %d executed, order %d", expected, defer_order); + if (defer_order != expected) { + ESP_LOGE("defer_test", "FIFO violation in defer: expected %d but got execution order %d", expected, defer_order); + defer_passed = false; + } + defer_order++; + + if (defer_order == 10) { + if (defer_passed) { + ESP_LOGI("defer_test", "✓ Test PASSED - defer() maintains FIFO order"); + id(test_result)->trigger("passed"); + } else { + ESP_LOGE("defer_test", "✗ Test FAILED - defer() executed out of order"); + id(test_result)->trigger("failed"); + } + id(test_complete)->trigger("test_finished"); + } + }); + } + } + }; + + // Use a static instance so it doesn't go out of scope + static TestComponent test_component; + test_component.test_defer(); + + ESP_LOGD("defer_test", "Deferred 10 items using defer(), waiting for execution..."); + +event: + - platform: template + name: "Test Complete" + id: test_complete + device_class: button + event_types: + - "test_finished" + - platform: template + name: "Test Result" + id: test_result + device_class: button + event_types: + - "passed" + - "failed" diff --git a/tests/integration/fixtures/scheduler_defer_stress.yaml b/tests/integration/fixtures/scheduler_defer_stress.yaml new file mode 100644 index 0000000000..0d9c1d1405 --- /dev/null +++ b/tests/integration/fixtures/scheduler_defer_stress.yaml @@ -0,0 +1,38 @@ +esphome: + name: scheduler-defer-stress-test + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + components: [defer_stress_component] + +host: + +logger: + level: VERBOSE + +defer_stress_component: + id: defer_stress + +api: + services: + - service: run_stress_test + then: + - lambda: |- + id(defer_stress)->run_multi_thread_test(); + +event: + - platform: template + name: "Test Complete" + id: test_complete + device_class: button + event_types: + - "test_finished" + - platform: template + name: "Test Result" + id: test_result + device_class: button + event_types: + - "passed" + - "failed" diff --git a/tests/integration/test_scheduler_defer_fifo_simple.py b/tests/integration/test_scheduler_defer_fifo_simple.py new file mode 100644 index 0000000000..ce17afff33 --- /dev/null +++ b/tests/integration/test_scheduler_defer_fifo_simple.py @@ -0,0 +1,117 @@ +"""Simple test that defer() maintains FIFO order.""" + +import asyncio + +from aioesphomeapi import EntityState, Event, EventInfo, UserService +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_defer_fifo_simple( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that defer() maintains FIFO order with a simple test.""" + + async with run_compiled(yaml_config), api_client_connected() as client: + # Verify we can connect + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "scheduler-defer-fifo-simple" + + # List entities and services + entity_info, services = await asyncio.wait_for( + client.list_entities_services(), timeout=5.0 + ) + + # Find our test entities + test_complete_entity: EventInfo | None = None + test_result_entity: EventInfo | None = None + + for entity in entity_info: + if isinstance(entity, EventInfo): + if entity.object_id == "test_complete": + test_complete_entity = entity + elif entity.object_id == "test_result": + test_result_entity = entity + + assert test_complete_entity is not None, "test_complete event not found" + assert test_result_entity is not None, "test_result event not found" + + # Find our test services + test_set_timeout_service: UserService | None = None + test_defer_service: UserService | None = None + for service in services: + if service.name == "test_set_timeout": + test_set_timeout_service = service + elif service.name == "test_defer": + test_defer_service = service + + assert test_set_timeout_service is not None, ( + "test_set_timeout service not found" + ) + assert test_defer_service is not None, "test_defer service not found" + + # Get the event loop + loop = asyncio.get_running_loop() + + # Subscribe to states + # (events are delivered as EventStates through subscribe_states) + test_complete_future: asyncio.Future[bool] = loop.create_future() + test_result_future: asyncio.Future[bool] = loop.create_future() + + def on_state(state: EntityState) -> None: + if not isinstance(state, Event): + return + + if ( + state.key == test_complete_entity.key + and state.event_type == "test_finished" + and not test_complete_future.done() + ): + test_complete_future.set_result(True) + return + + if state.key == test_result_entity.key and not test_result_future.done(): + if state.event_type == "passed": + test_result_future.set_result(True) + elif state.event_type == "failed": + test_result_future.set_result(False) + + client.subscribe_states(on_state) + + # Test 1: Test set_timeout(0) + client.execute_service(test_set_timeout_service, {}) + + # Wait for first test completion + try: + await asyncio.wait_for(test_complete_future, timeout=5.0) + test1_passed = await asyncio.wait_for(test_result_future, timeout=1.0) + except asyncio.TimeoutError: + pytest.fail("Test set_timeout(0) did not complete within 5 seconds") + + assert test1_passed is True, ( + "set_timeout(0) FIFO test failed - items executed out of order" + ) + + # Reset futures for second test + test_complete_future = loop.create_future() + test_result_future = loop.create_future() + + # Test 2: Test defer() + client.execute_service(test_defer_service, {}) + + # Wait for second test completion + try: + await asyncio.wait_for(test_complete_future, timeout=5.0) + test2_passed = await asyncio.wait_for(test_result_future, timeout=1.0) + except asyncio.TimeoutError: + pytest.fail("Test defer() did not complete within 5 seconds") + + # Verify the test passed + assert test2_passed is True, ( + "defer() FIFO test failed - items executed out of order" + ) diff --git a/tests/integration/test_scheduler_defer_stress.py b/tests/integration/test_scheduler_defer_stress.py new file mode 100644 index 0000000000..844efb59f7 --- /dev/null +++ b/tests/integration/test_scheduler_defer_stress.py @@ -0,0 +1,137 @@ +"""Stress test for defer() thread safety with multiple threads.""" + +import asyncio +from pathlib import Path +import re + +from aioesphomeapi import UserService +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_defer_stress( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that defer() doesn't crash when called rapidly from multiple threads.""" + + # Get the absolute path to the external components directory + external_components_path = str( + Path(__file__).parent / "fixtures" / "external_components" + ) + + # Replace the placeholder in the YAML config with the actual path + yaml_config = yaml_config.replace( + "EXTERNAL_COMPONENT_PATH", external_components_path + ) + + # Create a future to signal test completion + loop = asyncio.get_event_loop() + test_complete_future: asyncio.Future[None] = loop.create_future() + + # Track executed defers and their order + executed_defers: set[int] = set() + thread_executions: dict[ + int, list[int] + ] = {} # thread_id -> list of indices in execution order + fifo_violations: list[str] = [] + + def on_log_line(line: str) -> None: + # Track all executed defers with thread and index info + match = re.search(r"Executed defer (\d+) \(thread (\d+), index (\d+)\)", line) + if not match: + return + + defer_id = int(match.group(1)) + thread_id = int(match.group(2)) + index = int(match.group(3)) + + executed_defers.add(defer_id) + + # Track execution order per thread + if thread_id not in thread_executions: + thread_executions[thread_id] = [] + + # Check FIFO ordering within thread + if thread_executions[thread_id] and thread_executions[thread_id][-1] >= index: + fifo_violations.append( + f"Thread {thread_id}: index {index} executed after " + f"{thread_executions[thread_id][-1]}" + ) + + thread_executions[thread_id].append(index) + + # Check if we've executed all 1000 defers (0-999) + if len(executed_defers) == 1000 and not test_complete_future.done(): + test_complete_future.set_result(None) + + async with ( + run_compiled(yaml_config, line_callback=on_log_line), + api_client_connected() as client, + ): + # Verify we can connect + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "scheduler-defer-stress-test" + + # List entities and services + entity_info, services = await asyncio.wait_for( + client.list_entities_services(), timeout=5.0 + ) + + # Find our test service + run_stress_test_service: UserService | None = None + for service in services: + if service.name == "run_stress_test": + run_stress_test_service = service + break + + assert run_stress_test_service is not None, "run_stress_test service not found" + + # Call the run_stress_test service to start the test + client.execute_service(run_stress_test_service, {}) + + # Wait for all defers to execute (should be quick) + try: + await asyncio.wait_for(test_complete_future, timeout=5.0) + except asyncio.TimeoutError: + # Report how many we got + pytest.fail( + f"Stress test timed out. Only {len(executed_defers)} of " + f"1000 defers executed. Missing IDs: " + f"{sorted(set(range(1000)) - executed_defers)[:10]}..." + ) + + # Verify all defers executed + assert len(executed_defers) == 1000, ( + f"Expected 1000 defers, got {len(executed_defers)}" + ) + + # Verify we have all IDs from 0-999 + expected_ids = set(range(1000)) + missing_ids = expected_ids - executed_defers + assert not missing_ids, f"Missing defer IDs: {sorted(missing_ids)}" + + # Verify FIFO ordering was maintained within each thread + assert not fifo_violations, "FIFO ordering violations detected:\n" + "\n".join( + fifo_violations[:10] + ) + + # Verify each thread executed all its defers in order + for thread_id, indices in thread_executions.items(): + assert len(indices) == 100, ( + f"Thread {thread_id} executed {len(indices)} defers, expected 100" + ) + # Indices should be 0-99 in ascending order + assert indices == list(range(100)), ( + f"Thread {thread_id} executed indices out of order: {indices[:10]}..." + ) + + # If we got here without crashing and with proper ordering, the test passed + assert True, ( + "Test completed successfully - all 1000 defers executed with " + "FIFO ordering preserved" + ) From e077e6cec74c81ea67ce4eda9e303fa789b0a200 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:17:16 -0500 Subject: [PATCH 21/33] adjust --- esphome/core/scheduler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 1a38b6a83e..6a9969e802 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -82,8 +82,6 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type item->callback = std::move(func); item->remove = false; - const auto now = this->millis_(); - #if !defined(USE_ESP8266) && !defined(USE_RP2040) // Special handling for defer() (delay = 0, type = TIMEOUT) // ESP8266 and RP2040 are excluded because they don't need thread-safe defer handling @@ -96,6 +94,8 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type } #endif + const auto now = this->millis_(); + // Type-specific setup if (type == SchedulerItem::INTERVAL) { item->interval = delay; From f21365775393037606c83334fd4b89b34f4c237f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:18:47 -0500 Subject: [PATCH 22/33] adjust --- esphome/core/scheduler.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 27d95f5c05..7d618f01c9 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -137,16 +137,16 @@ class Scheduler { void set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, uint32_t delay, std::function func); - // Helper to cancel items by name - must be called with lock held - bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); - uint64_t millis_(); void cleanup_(); void pop_raw_(); - // Common implementation for cancel operations - bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); private: + // Helper to cancel items by name - must be called with lock held + bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); + + // Common implementation for cancel operations + bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); // Helper to execute a scheduler item void execute_item_(SchedulerItem *item); From 82d68c87e2eb47db1c6734fcedfde0e1d7166492 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:24:00 -0500 Subject: [PATCH 23/33] adjust --- esphome/core/scheduler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 6a9969e802..63a3653e7c 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -88,7 +88,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == 0 && type == SchedulerItem::TIMEOUT) { // Put in defer queue for guaranteed FIFO execution LockGuard guard{this->lock_}; - this->cancel_item_locked_(component, is_static_string, name_ptr, type); + this->cancel_item_locked_(component, name_cstr, type); this->defer_queue_.push_back(std::move(item)); return; } From 2dc222aea61a4ef401960edaf31afc944fe63b9b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:26:29 -0500 Subject: [PATCH 24/33] tweak --- .../fixtures/scheduler_defer_cancel.yaml | 51 ++++++++++ .../test_scheduler_defer_cancel.py | 94 +++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 tests/integration/fixtures/scheduler_defer_cancel.yaml create mode 100644 tests/integration/test_scheduler_defer_cancel.py diff --git a/tests/integration/fixtures/scheduler_defer_cancel.yaml b/tests/integration/fixtures/scheduler_defer_cancel.yaml new file mode 100644 index 0000000000..9e3f927c33 --- /dev/null +++ b/tests/integration/fixtures/scheduler_defer_cancel.yaml @@ -0,0 +1,51 @@ +esphome: + name: scheduler-defer-cancel + +host: + +logger: + level: DEBUG + +api: + services: + - service: test_defer_cancel + then: + - lambda: |- + // Schedule 10 defers with the same name + // Only the last one should execute + for (int i = 1; i <= 10; i++) { + App.scheduler.set_timeout(nullptr, "test_defer", 0, [i]() { + ESP_LOGI("TEST", "Defer executed: %d", i); + // Fire event with the defer number + std::string event_type = "defer_executed_" + std::to_string(i); + id(test_result)->trigger(event_type); + }); + } + + // Schedule completion notification after all defers + App.scheduler.set_timeout(nullptr, "completion", 0, []() { + ESP_LOGI("TEST", "Test complete"); + id(test_complete)->trigger("test_finished"); + }); + +event: + - platform: template + id: test_result + name: "Test Result" + event_types: + - "defer_executed_1" + - "defer_executed_2" + - "defer_executed_3" + - "defer_executed_4" + - "defer_executed_5" + - "defer_executed_6" + - "defer_executed_7" + - "defer_executed_8" + - "defer_executed_9" + - "defer_executed_10" + + - platform: template + id: test_complete + name: "Test Complete" + event_types: + - "test_finished" diff --git a/tests/integration/test_scheduler_defer_cancel.py b/tests/integration/test_scheduler_defer_cancel.py new file mode 100644 index 0000000000..923cf946c4 --- /dev/null +++ b/tests/integration/test_scheduler_defer_cancel.py @@ -0,0 +1,94 @@ +"""Test that defer() with the same name cancels previous defers.""" + +import asyncio + +from aioesphomeapi import EntityState, Event, EventInfo, UserService +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_scheduler_defer_cancel( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that defer() with the same name cancels previous defers.""" + + async with run_compiled(yaml_config), api_client_connected() as client: + # Verify we can connect + device_info = await client.device_info() + assert device_info is not None + assert device_info.name == "scheduler-defer-cancel" + + # List entities and services + entity_info, services = await asyncio.wait_for( + client.list_entities_services(), timeout=5.0 + ) + + # Find our test entities + test_complete_entity: EventInfo | None = None + test_result_entity: EventInfo | None = None + + for entity in entity_info: + if isinstance(entity, EventInfo): + if entity.object_id == "test_complete": + test_complete_entity = entity + elif entity.object_id == "test_result": + test_result_entity = entity + + assert test_complete_entity is not None, "test_complete event not found" + assert test_result_entity is not None, "test_result event not found" + + # Find our test service + test_defer_cancel_service: UserService | None = None + for service in services: + if service.name == "test_defer_cancel": + test_defer_cancel_service = service + + assert test_defer_cancel_service is not None, ( + "test_defer_cancel service not found" + ) + + # Get the event loop + loop = asyncio.get_running_loop() + + # Subscribe to states + test_complete_future: asyncio.Future[bool] = loop.create_future() + test_result_future: asyncio.Future[int] = loop.create_future() + + def on_state(state: EntityState) -> None: + if not isinstance(state, Event): + return + + if ( + state.key == test_complete_entity.key + and state.event_type == "test_finished" + and not test_complete_future.done() + ): + test_complete_future.set_result(True) + return + + if state.key == test_result_entity.key and not test_result_future.done(): + # Event type should be "defer_executed_X" where X is the defer number + if state.event_type.startswith("defer_executed_"): + defer_num = int(state.event_type.split("_")[-1]) + test_result_future.set_result(defer_num) + + client.subscribe_states(on_state) + + # Execute the test + client.execute_service(test_defer_cancel_service, {}) + + # Wait for test completion + try: + await asyncio.wait_for(test_complete_future, timeout=10.0) + executed_defer = await asyncio.wait_for(test_result_future, timeout=1.0) + except asyncio.TimeoutError: + pytest.fail("Test did not complete within timeout") + + # Verify that only defer 10 was executed + assert executed_defer == 10, ( + f"Expected defer 10 to execute, got {executed_defer}" + ) From f395767766c4d7162e63f985dc6876ba15451a63 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:27:49 -0500 Subject: [PATCH 25/33] tweak --- esphome/core/scheduler.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 63a3653e7c..e63869200b 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -436,15 +436,17 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Check all containers for matching items #if !defined(USE_ESP8266) && !defined(USE_RP2040) - // Cancel items in defer queue - for (auto &item : this->defer_queue_) { - if (item->component != component || item->type != type || item->remove) { - continue; - } - const char *item_name = item->get_name(); - if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { - item->remove = true; - total_cancelled++; + // Only check defer queue for timeouts (intervals never go there) + if (type == SchedulerItem::TIMEOUT) { + for (auto &item : this->defer_queue_) { + if (item->component != component || item->remove) { + continue; + } + const char *item_name = item->get_name(); + if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + item->remove = true; + total_cancelled++; + } } } #endif From 2759f3828eed103265150c4e53a0286d8e94758e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:34:56 -0500 Subject: [PATCH 26/33] tweak --- esphome/core/scheduler.cpp | 14 +++++++++----- esphome/core/scheduler.h | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index e63869200b..1004c74083 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -69,7 +69,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // Still need to cancel existing timer if name is not empty if (name_cstr != nullptr && name_cstr[0] != '\0') { LockGuard guard{this->lock_}; - this->cancel_item_locked_(component, name_cstr, type); + this->cancel_item_locked_(component, name_cstr, type, delay == 0 && type == SchedulerItem::TIMEOUT); } return; } @@ -88,7 +88,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type if (delay == 0 && type == SchedulerItem::TIMEOUT) { // Put in defer queue for guaranteed FIFO execution LockGuard guard{this->lock_}; - this->cancel_item_locked_(component, name_cstr, type); + this->cancel_item_locked_(component, name_cstr, type, true); this->defer_queue_.push_back(std::move(item)); return; } @@ -129,7 +129,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // If name is provided, do atomic cancel-and-add if (name_cstr != nullptr && name_cstr[0] != '\0') { // Cancel existing items - this->cancel_item_locked_(component, name_cstr, type); + this->cancel_item_locked_(component, name_cstr, type, delay == 0 && type == SchedulerItem::TIMEOUT); } // Add new item directly to to_add_ // since we have the lock held @@ -427,11 +427,12 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co // obtain lock because this function iterates and can be called from non-loop task context LockGuard guard{this->lock_}; - return this->cancel_item_locked_(component, name_cstr, type); + return this->cancel_item_locked_(component, name_cstr, type, false); } // Helper to cancel items by name - must be called with lock held -bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type) { +bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type, + bool defer_only) { size_t total_cancelled = 0; // Check all containers for matching items @@ -448,6 +449,9 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c total_cancelled++; } } + if (defer_only) { + return total_cancelled > 0; + } } #endif diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 7d618f01c9..c154a29a91 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -143,7 +143,7 @@ class Scheduler { private: // Helper to cancel items by name - must be called with lock held - bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type); + bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type, bool defer_only); // Common implementation for cancel operations bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); From ba8f3d3f6392967128b9d67507bc67456fe2b7b8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:36:05 -0500 Subject: [PATCH 27/33] tweak --- esphome/core/scheduler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 1004c74083..8e756c6b50 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -129,7 +129,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // If name is provided, do atomic cancel-and-add if (name_cstr != nullptr && name_cstr[0] != '\0') { // Cancel existing items - this->cancel_item_locked_(component, name_cstr, type, delay == 0 && type == SchedulerItem::TIMEOUT); + this->cancel_item_locked_(component, name_cstr, type, false); } // Add new item directly to to_add_ // since we have the lock held From 0900fd3ceab25c47b366d6be6616b8d2deebc1f4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:42:47 -0500 Subject: [PATCH 28/33] tweak --- esphome/core/scheduler.cpp | 18 +++--------------- esphome/core/scheduler.h | 11 +++++++++++ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 8e756c6b50..fa0f6c00f6 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -440,11 +440,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Only check defer queue for timeouts (intervals never go there) if (type == SchedulerItem::TIMEOUT) { for (auto &item : this->defer_queue_) { - if (item->component != component || item->remove) { - continue; - } - const char *item_name = item->get_name(); - if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + if (this->matches_item_(item, component, name_cstr, type)) { item->remove = true; total_cancelled++; } @@ -457,11 +453,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Cancel items in the main heap for (auto &item : this->items_) { - if (item->component != component || item->type != type || item->remove) { - continue; - } - const char *item_name = item->get_name(); - if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + if (this->matches_item_(item, component, name_cstr, type)) { item->remove = true; total_cancelled++; this->to_remove_++; // Track removals for heap items @@ -470,11 +462,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Cancel items in to_add_ for (auto &item : this->to_add_) { - if (item->component != component || item->type != type || item->remove) { - continue; - } - const char *item_name = item->get_name(); - if (item_name != nullptr && strcmp(name_cstr, item_name) == 0) { + if (this->matches_item_(item, component, name_cstr, type)) { item->remove = true; total_cancelled++; // Don't track removals for to_add_ items diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index c154a29a91..1acf9c1d6b 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -147,6 +147,17 @@ class Scheduler { // Common implementation for cancel operations bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); + + // Helper function to check if item matches criteria for cancellation + bool HOT matches_item_(const std::unique_ptr &item, Component *component, const char *name_cstr, + SchedulerItem::Type type) { + if (item->component != component || item->type != type || item->remove) { + return false; + } + const char *item_name = item->get_name(); + return item_name != nullptr && strcmp(name_cstr, item_name) == 0; + } + // Helper to execute a scheduler item void execute_item_(SchedulerItem *item); From 033c469250993631209ef910baf999d429487f73 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:44:19 -0500 Subject: [PATCH 29/33] tweak --- esphome/core/scheduler.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 1acf9c1d6b..a9e2e62e5d 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -155,7 +155,15 @@ class Scheduler { return false; } const char *item_name = item->get_name(); - return item_name != nullptr && strcmp(name_cstr, item_name) == 0; + if (item_name == nullptr) { + return false; + } + // Fast path: if pointers are equal (common with string deduplication) + if (item_name == name_cstr) { + return true; + } + // Slow path: compare string contents + return strcmp(name_cstr, item_name) == 0; } // Helper to execute a scheduler item From c45901746b6c9f9caf8e4754c67ec25657e2cae0 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:46:48 -0500 Subject: [PATCH 30/33] tweak --- esphome/core/scheduler.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index a9e2e62e5d..9ff6336bd5 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -149,8 +149,8 @@ class Scheduler { bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); // Helper function to check if item matches criteria for cancellation - bool HOT matches_item_(const std::unique_ptr &item, Component *component, const char *name_cstr, - SchedulerItem::Type type) { + inline bool HOT matches_item_(const std::unique_ptr &item, Component *component, const char *name_cstr, + SchedulerItem::Type type) { if (item->component != component || item->type != type || item->remove) { return false; } From ad51e647af26574117af7c9bc7afdc712a1786ab Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:48:50 -0500 Subject: [PATCH 31/33] tweak --- esphome/core/scheduler.cpp | 6 ++---- esphome/core/scheduler.h | 5 +++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index fa0f6c00f6..be8f7db83f 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -62,8 +62,7 @@ static void validate_static_string(const char *name) { void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, uint32_t delay, std::function func) { // Get the name as const char* - const char *name_cstr = - is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); + const char *name_cstr = this->get_name_cstr_(is_static_string, name_ptr); if (delay == SCHEDULER_DONT_RUN) { // Still need to cancel existing timer if name is not empty @@ -418,8 +417,7 @@ void HOT Scheduler::execute_item_(SchedulerItem *item) { bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type) { // Get the name as const char* - const char *name_cstr = - is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); + const char *name_cstr = this->get_name_cstr_(is_static_string, name_ptr); // Handle null or empty names if (name_cstr == nullptr) diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 9ff6336bd5..f3f78d39af 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -145,6 +145,11 @@ class Scheduler { // Helper to cancel items by name - must be called with lock held bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type, bool defer_only); + // Helper to extract name as const char* from either static string or std::string + inline const char *get_name_cstr_(bool is_static_string, const void *name_ptr) { + return is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); + } + // Common implementation for cancel operations bool cancel_item_(Component *component, bool is_static_string, const void *name_ptr, SchedulerItem::Type type); From db84d8e8dc1aa216e235b25ab9e68c85ff452ce8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:49:41 -0500 Subject: [PATCH 32/33] tweak --- esphome/core/scheduler.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index be8f7db83f..0c4a4ff230 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -123,17 +123,15 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type } #endif - { - LockGuard guard{this->lock_}; - // If name is provided, do atomic cancel-and-add - if (name_cstr != nullptr && name_cstr[0] != '\0') { - // Cancel existing items - this->cancel_item_locked_(component, name_cstr, type, false); - } - // Add new item directly to to_add_ - // since we have the lock held - this->to_add_.push_back(std::move(item)); + LockGuard guard{this->lock_}; + // If name is provided, do atomic cancel-and-add + if (name_cstr != nullptr && name_cstr[0] != '\0') { + // Cancel existing items + this->cancel_item_locked_(component, name_cstr, type, false); } + // Add new item directly to to_add_ + // since we have the lock held + this->to_add_.push_back(std::move(item)); } void HOT Scheduler::set_timeout(Component *component, const char *name, uint32_t timeout, std::function func) { From add7bec7f214a4b0b5027855b71f63073f52f38d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 18:54:00 -0500 Subject: [PATCH 33/33] tweak --- tests/integration/test_scheduler_defer_fifo_simple.py | 2 +- tests/integration/test_scheduler_defer_stress.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_scheduler_defer_fifo_simple.py b/tests/integration/test_scheduler_defer_fifo_simple.py index ce17afff33..eb4058fedd 100644 --- a/tests/integration/test_scheduler_defer_fifo_simple.py +++ b/tests/integration/test_scheduler_defer_fifo_simple.py @@ -9,7 +9,7 @@ from .types import APIClientConnectedFactory, RunCompiledFunction @pytest.mark.asyncio -async def test_defer_fifo_simple( +async def test_scheduler_defer_fifo_simple( yaml_config: str, run_compiled: RunCompiledFunction, api_client_connected: APIClientConnectedFactory, diff --git a/tests/integration/test_scheduler_defer_stress.py b/tests/integration/test_scheduler_defer_stress.py index 844efb59f7..d546b7132f 100644 --- a/tests/integration/test_scheduler_defer_stress.py +++ b/tests/integration/test_scheduler_defer_stress.py @@ -11,7 +11,7 @@ from .types import APIClientConnectedFactory, RunCompiledFunction @pytest.mark.asyncio -async def test_defer_stress( +async def test_scheduler_defer_stress( yaml_config: str, run_compiled: RunCompiledFunction, api_client_connected: APIClientConnectedFactory,