From 0946f285113a52e85d66932fbdd8fb0e43300e1a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Jun 2025 19:08:18 +0200 Subject: [PATCH 01/12] avoid string copy in scheduler for const strings --- esphome/core/scheduler.cpp | 29 +++++++++++++--- esphome/core/scheduler.h | 71 +++++++++++++++++++++++++++++++++++--- 2 files changed, 91 insertions(+), 9 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 8144435163..fbf68522aa 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -22,8 +22,17 @@ static const uint32_t MAX_LOGICALLY_DELETED_ITEMS = 10; // 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. +void HOT Scheduler::set_timeout(Component *component, const char *name, uint32_t timeout, std::function func) { + return this->set_timeout_(component, name, timeout, func, false); +} + void HOT Scheduler::set_timeout(Component *component, const std::string &name, uint32_t timeout, std::function func) { + return this->set_timeout_(component, name, timeout, func, true); +} + +void HOT Scheduler::set_timeout_(Component *component, const std::string &name, uint32_t timeout, + std::function func, bool make_copy) { const auto now = this->millis_(); if (!name.empty()) @@ -34,7 +43,7 @@ void HOT Scheduler::set_timeout(Component *component, const std::string &name, u auto item = make_unique(); item->component = component; - item->name = name; + item->set_name(name.c_str(), make_copy); item->type = SchedulerItem::TIMEOUT; item->next_execution_ = now + timeout; item->callback = std::move(func); @@ -49,6 +58,14 @@ bool HOT Scheduler::cancel_timeout(Component *component, const std::string &name } void HOT Scheduler::set_interval(Component *component, const std::string &name, uint32_t interval, std::function func) { + this->set_interval_(component, name, interval, func, true); +} +void HOT Scheduler::set_interval(Component *component, const char *name, uint32_t interval, + std::function func) { + this->set_interval_(component, name, interval, func, false); +} +void HOT Scheduler::set_interval_(Component *component, const std::string &name, uint32_t interval, + std::function func, bool make_copy) { const auto now = this->millis_(); if (!name.empty()) @@ -64,7 +81,7 @@ void HOT Scheduler::set_interval(Component *component, const std::string &name, auto item = make_unique(); item->component = component; - item->name = name; + item->set_name(name.c_str(), make_copy); item->type = SchedulerItem::INTERVAL; item->interval = interval; item->next_execution_ = now + offset; @@ -85,7 +102,7 @@ struct RetryArgs { uint8_t retry_countdown; uint32_t current_interval; Component *component; - std::string name; + std::string name; // Keep as std::string since retry uses it dynamically float backoff_increase_factor; Scheduler *scheduler; }; @@ -303,14 +320,16 @@ bool HOT Scheduler::cancel_item_(Component *component, const std::string &name, LockGuard guard{this->lock_}; bool ret = false; for (auto &it : this->items_) { - if (it->component == component && it->name == name && it->type == type && !it->remove) { + const char *item_name = it->get_name(); + if (it->component == component && item_name != nullptr && name == item_name && it->type == type && !it->remove) { to_remove_++; it->remove = true; ret = true; } } for (auto &it : this->to_add_) { - if (it->component == component && it->name == name && it->type == type) { + const char *item_name = it->get_name(); + if (it->component == component && item_name != nullptr && name == item_name && it->type == type) { it->remove = true; ret = true; } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 1284bcd4a7..80452d6628 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -12,11 +12,19 @@ class Component; class Scheduler { public: + // Public API - accepts std::string for backward compatibility void set_timeout(Component *component, const std::string &name, uint32_t timeout, std::function func); + void set_timeout(Component *component, const char *name, uint32_t timeout, std::function func); + void set_timeout_(Component *component, const std::string &name, uint32_t timeout, std::function func, + bool make_copy); + bool cancel_timeout(Component *component, const std::string &name); void set_interval(Component *component, const std::string &name, uint32_t interval, std::function func); - bool cancel_interval(Component *component, const std::string &name); + void set_interval(Component *component, const char *name, uint32_t interval, std::function func); + void set_interval_(Component *component, const std::string &name, uint32_t interval, std::function func, + bool make_copy); + bool cancel_interval(Component *component, const std::string &name); void set_retry(Component *component, const std::string &name, uint32_t initial_wait_time, uint8_t max_attempts, std::function func, float backoff_increase_factor = 1.0f); bool cancel_retry(Component *component, const std::string &name); @@ -36,10 +44,65 @@ class Scheduler { // with a 16-bit rollover counter to create a 64-bit time that won't roll over for // billions of years. This ensures correct scheduling even when devices run for months. uint64_t next_execution_; - std::string name; + + // Optimized name storage using tagged union + union { + const char *static_name; // For string literals (no allocation) + char *dynamic_name; // For allocated strings + } name_; + std::function callback; - enum Type : uint8_t { TIMEOUT, INTERVAL } type; - bool remove; + + // Bit-packed fields to minimize padding + enum Type : uint8_t { TIMEOUT, INTERVAL } type : 1; + bool remove : 1; + bool owns_name : 1; // True if name_.dynamic_name needs to be freed + // 5 bits padding + + // Constructor + SchedulerItem() + : component(nullptr), + interval(0), + next_execution_(0), + callback(nullptr), + type(TIMEOUT), + remove(false), + owns_name(false) { + name_.static_name = nullptr; + } + + // Destructor to clean up dynamic names + ~SchedulerItem() { + if (owns_name && name_.dynamic_name) { + delete[] name_.dynamic_name; + } + } + + // Helper to get the name regardless of storage type + const char *get_name() const { return owns_name ? name_.dynamic_name : name_.static_name; } + + // Helper to set name with proper ownership + void set_name(const char *name, bool make_copy = false) { + // Clean up old dynamic name if any + if (owns_name && name_.dynamic_name) { + delete[] name_.dynamic_name; + } + + if (name == nullptr || name[0] == '\0') { + name_.static_name = nullptr; + owns_name = false; + } else if (make_copy) { + // Make a copy for dynamic strings + size_t len = strlen(name); + name_.dynamic_name = new char[len + 1]; + strcpy(name_.dynamic_name, name); + owns_name = true; + } else { + // Use static string directly + name_.static_name = name; + owns_name = false; + } + } static bool cmp(const std::unique_ptr &a, const std::unique_ptr &b); const char *get_type_str() { From 825b1113b6e690cf93c3bd16046f751dfa9dbef2 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Jun 2025 23:17:41 +0200 Subject: [PATCH 02/12] tweak --- esphome/core/scheduler.cpp | 122 +++++++++++++++++++++++++++---------- esphome/core/scheduler.h | 12 ++-- 2 files changed, 98 insertions(+), 36 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index fbf68522aa..a701147d32 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -22,20 +22,21 @@ static const uint32_t MAX_LOGICALLY_DELETED_ITEMS = 10; // 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. -void HOT Scheduler::set_timeout(Component *component, const char *name, uint32_t timeout, std::function func) { - return this->set_timeout_(component, name, timeout, func, false); -} - -void HOT Scheduler::set_timeout(Component *component, const std::string &name, uint32_t timeout, - std::function func) { - return this->set_timeout_(component, name, timeout, func, true); -} - -void HOT Scheduler::set_timeout_(Component *component, const std::string &name, uint32_t timeout, - std::function func, bool make_copy) { +// Template implementation for set_timeout +template +void HOT Scheduler::set_timeout_impl_(Component *component, const NameType &name, uint32_t timeout, + std::function func, bool make_copy) { const auto now = this->millis_(); - if (!name.empty()) + // Handle empty name check based on type + bool is_empty = false; + if constexpr (std::is_same_v) { + is_empty = name.empty(); + } else { + is_empty = (name == nullptr || name[0] == '\0'); + } + + if (!is_empty) this->cancel_timeout(component, name); if (timeout == SCHEDULER_DONT_RUN) @@ -43,32 +44,62 @@ void HOT Scheduler::set_timeout_(Component *component, const std::string &name, auto item = make_unique(); item->component = component; - item->set_name(name.c_str(), make_copy); + + // Set name based on type + if constexpr (std::is_same_v) { + item->set_name(name.c_str(), make_copy); + } else { + item->set_name(name, make_copy); + } + item->type = SchedulerItem::TIMEOUT; item->next_execution_ = now + timeout; item->callback = std::move(func); item->remove = false; #ifdef ESPHOME_DEBUG_SCHEDULER - ESP_LOGD(TAG, "set_timeout(name='%s/%s', timeout=%" PRIu32 ")", item->get_source(), name.c_str(), timeout); + const char *name_str = nullptr; + if constexpr (std::is_same_v) { + name_str = name.c_str(); + } else { + name_str = name; + } + ESP_LOGD(TAG, "set_timeout(name='%s/%s', timeout=%" PRIu32 ")", item->get_source(), name_str, timeout); #endif this->push_(std::move(item)); } + +// Explicit instantiations +template void Scheduler::set_timeout_impl_(Component *, const std::string &, uint32_t, + std::function, bool); +template void Scheduler::set_timeout_impl_(Component *, const char *const &, uint32_t, + std::function, bool); + +void HOT Scheduler::set_timeout(Component *component, const char *name, uint32_t timeout, std::function func) { + return this->set_timeout_impl_(component, name, timeout, std::move(func), false); +} + +void HOT Scheduler::set_timeout(Component *component, const std::string &name, uint32_t timeout, + std::function func) { + return this->set_timeout_impl_(component, name, timeout, std::move(func), true); +} bool HOT Scheduler::cancel_timeout(Component *component, const std::string &name) { return this->cancel_item_(component, name, SchedulerItem::TIMEOUT); } -void HOT Scheduler::set_interval(Component *component, const std::string &name, uint32_t interval, - std::function func) { - this->set_interval_(component, name, interval, func, true); -} -void HOT Scheduler::set_interval(Component *component, const char *name, uint32_t interval, - std::function func) { - this->set_interval_(component, name, interval, func, false); -} -void HOT Scheduler::set_interval_(Component *component, const std::string &name, uint32_t interval, - std::function func, bool make_copy) { +// Template implementation for set_interval +template +void HOT Scheduler::set_interval_impl_(Component *component, const NameType &name, uint32_t interval, + std::function func, bool make_copy) { const auto now = this->millis_(); - if (!name.empty()) + // Handle empty name check based on type + bool is_empty = false; + if constexpr (std::is_same_v) { + is_empty = name.empty(); + } else { + is_empty = (name == nullptr || name[0] == '\0'); + } + + if (!is_empty) this->cancel_interval(component, name); if (interval == SCHEDULER_DONT_RUN) @@ -81,18 +112,46 @@ void HOT Scheduler::set_interval_(Component *component, const std::string &name, auto item = make_unique(); item->component = component; - item->set_name(name.c_str(), make_copy); + + // Set name based on type + if constexpr (std::is_same_v) { + item->set_name(name.c_str(), make_copy); + } else { + item->set_name(name, make_copy); + } + item->type = SchedulerItem::INTERVAL; item->interval = interval; item->next_execution_ = now + offset; item->callback = std::move(func); item->remove = false; #ifdef ESPHOME_DEBUG_SCHEDULER - ESP_LOGD(TAG, "set_interval(name='%s/%s', interval=%" PRIu32 ", offset=%" PRIu32 ")", item->get_source(), - name.c_str(), interval, offset); + const char *name_str = nullptr; + if constexpr (std::is_same_v) { + name_str = name.c_str(); + } else { + name_str = name; + } + ESP_LOGD(TAG, "set_interval(name='%s/%s', interval=%" PRIu32 ", offset=%" PRIu32 ")", item->get_source(), name_str, + interval, offset); #endif this->push_(std::move(item)); } + +// Explicit instantiations +template void Scheduler::set_interval_impl_(Component *, const std::string &, uint32_t, + std::function, bool); +template void Scheduler::set_interval_impl_(Component *, const char *const &, uint32_t, + std::function, bool); + +void HOT Scheduler::set_interval(Component *component, const std::string &name, uint32_t interval, + std::function func) { + return this->set_interval_impl_(component, name, interval, std::move(func), true); +} +void HOT Scheduler::set_interval(Component *component, const char *name, uint32_t interval, + std::function func) { + return this->set_interval_impl_(component, name, interval, std::move(func), false); +} bool HOT Scheduler::cancel_interval(Component *component, const std::string &name) { return this->cancel_item_(component, name, SchedulerItem::INTERVAL); } @@ -180,8 +239,8 @@ void HOT Scheduler::call() { this->lock_.unlock(); ESP_LOGD(TAG, " %s '%s/%s' interval=%" PRIu32 " next_execution in %" PRIu64 "ms at %" PRIu64, - item->get_type_str(), item->get_source(), item->name.c_str(), item->interval, - item->next_execution_ - now, item->next_execution_); + item->get_type_str(), item->get_source(), item->get_name(), item->interval, item->next_execution_ - now, + item->next_execution_); old_items.push_back(std::move(item)); } @@ -238,8 +297,7 @@ void HOT Scheduler::call() { #ifdef ESPHOME_DEBUG_SCHEDULER ESP_LOGV(TAG, "Running %s '%s/%s' with interval=%" PRIu32 " next_execution=%" PRIu64 " (now=%" PRIu64 ")", - item->get_type_str(), item->get_source(), item->name.c_str(), item->interval, item->next_execution_, - now); + item->get_type_str(), item->get_source(), item->get_name(), item->interval, item->next_execution_, now); #endif // Warning: During callback(), a lot of stuff can happen, including: diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 80452d6628..ca437e690c 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -15,14 +15,10 @@ class Scheduler { // Public API - accepts std::string for backward compatibility void set_timeout(Component *component, const std::string &name, uint32_t timeout, std::function func); void set_timeout(Component *component, const char *name, uint32_t timeout, std::function func); - void set_timeout_(Component *component, const std::string &name, uint32_t timeout, std::function func, - bool make_copy); bool cancel_timeout(Component *component, const std::string &name); void set_interval(Component *component, const std::string &name, uint32_t interval, std::function func); void set_interval(Component *component, const char *name, uint32_t interval, std::function func); - void set_interval_(Component *component, const std::string &name, uint32_t interval, std::function func, - bool make_copy); bool cancel_interval(Component *component, const std::string &name); void set_retry(Component *component, const std::string &name, uint32_t initial_wait_time, uint8_t max_attempts, @@ -36,6 +32,14 @@ class Scheduler { void process_to_add(); protected: + // Template helper to handle both const char* and std::string efficiently + template + void set_timeout_impl_(Component *component, const NameType &name, uint32_t timeout, std::function func, + bool make_copy); + template + void set_interval_impl_(Component *component, const NameType &name, uint32_t interval, std::function func, + bool make_copy); + struct SchedulerItem { // Ordered by size to minimize padding Component *component; From f058107c0562f236aabb0bea9547f5e388249804 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Jun 2025 23:33:54 +0200 Subject: [PATCH 03/12] tweak --- esphome/core/component.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/core/component.cpp b/esphome/core/component.cpp index 625a7b2125..f86a90d607 100644 --- a/esphome/core/component.cpp +++ b/esphome/core/component.cpp @@ -189,7 +189,7 @@ bool Component::is_in_loop_state() const { return (this->component_state_ & COMPONENT_STATE_MASK) == COMPONENT_STATE_LOOP; } void Component::defer(std::function &&f) { // NOLINT - App.scheduler.set_timeout(this, "", 0, std::move(f)); + App.scheduler.set_timeout(this, static_cast(nullptr), 0, std::move(f)); } bool Component::cancel_defer(const std::string &name) { // NOLINT return App.scheduler.cancel_timeout(this, name); From 956959fc32edb07f156b724ed05870621e6711d6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 09:23:16 -0500 Subject: [PATCH 04/12] safety --- esphome/core/component.cpp | 8 ++++++ esphome/core/component.h | 32 +++++++++++++++++++++++ esphome/core/scheduler.cpp | 52 ++++++++++++++++++++++++++++++++++++-- esphome/core/scheduler.h | 23 +++++++++++++++++ 4 files changed, 113 insertions(+), 2 deletions(-) diff --git a/esphome/core/component.cpp b/esphome/core/component.cpp index f86a90d607..a1645219b1 100644 --- a/esphome/core/component.cpp +++ b/esphome/core/component.cpp @@ -60,6 +60,10 @@ void Component::set_interval(const std::string &name, uint32_t interval, std::fu App.scheduler.set_interval(this, name, interval, std::move(f)); } +void Component::set_interval(const char *name, uint32_t interval, std::function &&f) { // NOLINT + App.scheduler.set_interval(this, name, interval, std::move(f)); +} + bool Component::cancel_interval(const std::string &name) { // NOLINT return App.scheduler.cancel_interval(this, name); } @@ -77,6 +81,10 @@ void Component::set_timeout(const std::string &name, uint32_t timeout, std::func App.scheduler.set_timeout(this, name, timeout, std::move(f)); } +void Component::set_timeout(const char *name, uint32_t timeout, std::function &&f) { // NOLINT + App.scheduler.set_timeout(this, name, timeout, std::move(f)); +} + bool Component::cancel_timeout(const std::string &name) { // NOLINT return App.scheduler.cancel_timeout(this, name); } diff --git a/esphome/core/component.h b/esphome/core/component.h index 7f2bdd8414..900db27e29 100644 --- a/esphome/core/component.h +++ b/esphome/core/component.h @@ -260,6 +260,22 @@ class Component { */ void set_interval(const std::string &name, uint32_t interval, std::function &&f); // NOLINT + /** Set an interval function with a const char* name. + * + * IMPORTANT: The provided name pointer must remain valid for the lifetime of the scheduler item. + * This means the name should be: + * - A string literal (e.g., "update") + * - A static const char* variable + * - A pointer with lifetime >= the scheduled task + * + * For dynamic strings, use the std::string overload instead. + * + * @param name The identifier for this interval function (must have static lifetime) + * @param interval The interval in ms + * @param f The function to call + */ + void set_interval(const char *name, uint32_t interval, std::function &&f); // NOLINT + void set_interval(uint32_t interval, std::function &&f); // NOLINT /** Cancel an interval function. @@ -328,6 +344,22 @@ class Component { */ void set_timeout(const std::string &name, uint32_t timeout, std::function &&f); // NOLINT + /** Set a timeout function with a const char* name. + * + * IMPORTANT: The provided name pointer must remain valid for the lifetime of the scheduler item. + * This means the name should be: + * - A string literal (e.g., "init") + * - A static const char* variable + * - A pointer with lifetime >= the timeout duration + * + * For dynamic strings, use the std::string overload instead. + * + * @param name The identifier for this timeout function (must have static lifetime) + * @param timeout The timeout in ms + * @param f The function to call + */ + void set_timeout(const char *name, uint32_t timeout, std::function &&f); // NOLINT + void set_timeout(uint32_t timeout, std::function &&f); // NOLINT /** Cancel a timeout function. diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index a701147d32..30c4cb8137 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -17,6 +17,41 @@ static const uint32_t MAX_LOGICALLY_DELETED_ITEMS = 10; // Uncomment to debug scheduler // #define ESPHOME_DEBUG_SCHEDULER +#ifdef ESPHOME_DEBUG_SCHEDULER +// Helper to validate that a pointer looks like it's in static memory +static void validate_static_string(const char *name) { + if (name == nullptr) + return; + + // This is a heuristic check - stack and heap pointers are typically + // much higher in memory than static data + uintptr_t addr = reinterpret_cast(name); + + // Create a stack variable to compare against + int stack_var; + uintptr_t stack_addr = reinterpret_cast(&stack_var); + + // If the string pointer is near our stack variable, it's likely on the stack + // Using 8KB range as ESP32 main task stack is typically 8192 bytes + if (addr > (stack_addr - 0x2000) && addr < (stack_addr + 0x2000)) { + ESP_LOGW(TAG, + "WARNING: Scheduler name '%s' at %p appears to be on the stack - this is unsafe!\n" + " Stack reference at %p", + name, name, &stack_var); + } + + // Also check if it might be on the heap by seeing if it's in a very different range + // This is platform-specific but generally heap is allocated far from static memory + static const char *static_str = "test"; + uintptr_t static_addr = reinterpret_cast(static_str); + + // If the address is very far from known static memory, it might be heap + if (addr > static_addr + 0x100000 || (static_addr > 0x100000 && addr < static_addr - 0x100000)) { + ESP_LOGW(TAG, "WARNING: Scheduler name '%s' at %p might be on heap (static ref at %p)", name, name, static_str); + } +} +#endif + // A note on locking: the `lock_` lock protects the `items_` and `to_add_` containers. It must be taken when writing to // them (i.e. when adding/removing items, but not when changing items). As items are only deleted from the loop task, // iterating over them from the loop task is fine; but iterating from any other context requires the lock to be held to @@ -50,6 +85,12 @@ void HOT Scheduler::set_timeout_impl_(Component *component, const NameType &name item->set_name(name.c_str(), make_copy); } else { item->set_name(name, make_copy); +#ifdef ESPHOME_DEBUG_SCHEDULER + // Validate static strings in debug mode + if (!make_copy && name != nullptr) { + validate_static_string(name); + } +#endif } item->type = SchedulerItem::TIMEOUT; @@ -118,6 +159,12 @@ void HOT Scheduler::set_interval_impl_(Component *component, const NameType &nam item->set_name(name.c_str(), make_copy); } else { item->set_name(name, make_copy); +#ifdef ESPHOME_DEBUG_SCHEDULER + // Validate static strings in debug mode + if (!make_copy && name != nullptr) { + validate_static_string(name); + } +#endif } item->type = SchedulerItem::INTERVAL; @@ -238,9 +285,10 @@ void HOT Scheduler::call() { this->pop_raw_(); this->lock_.unlock(); + const char *name = item->get_name(); ESP_LOGD(TAG, " %s '%s/%s' interval=%" PRIu32 " next_execution in %" PRIu64 "ms at %" PRIu64, - item->get_type_str(), item->get_source(), item->get_name(), item->interval, item->next_execution_ - now, - item->next_execution_); + item->get_type_str(), item->get_source(), name ? name : "(null)", item->interval, + item->next_execution_ - now, item->next_execution_); old_items.push_back(std::move(item)); } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index ca437e690c..73940d7fff 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -14,10 +14,33 @@ class Scheduler { public: // Public API - accepts std::string for backward compatibility void set_timeout(Component *component, const std::string &name, uint32_t timeout, std::function func); + + /** Set a timeout with a const char* name. + * + * IMPORTANT: The provided name pointer must remain valid for the lifetime of the scheduler item. + * This means the name should be: + * - A string literal (e.g., "update") + * - A static const char* variable + * - A pointer with lifetime >= the scheduled task + * + * For dynamic strings, use the std::string overload instead. + */ void set_timeout(Component *component, const char *name, uint32_t timeout, std::function func); bool cancel_timeout(Component *component, const std::string &name); + void set_interval(Component *component, const std::string &name, uint32_t interval, std::function func); + + /** Set an interval with a const char* name. + * + * IMPORTANT: The provided name pointer must remain valid for the lifetime of the scheduler item. + * This means the name should be: + * - A string literal (e.g., "update") + * - A static const char* variable + * - A pointer with lifetime >= the scheduled task + * + * For dynamic strings, use the std::string overload instead. + */ void set_interval(Component *component, const char *name, uint32_t interval, std::function func); bool cancel_interval(Component *component, const std::string &name); From e6334b0716591eec25db66fa4d21e8bd350a4913 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 09:41:12 -0500 Subject: [PATCH 05/12] dry --- esphome/core/scheduler.cpp | 157 ++++++++++++------------------------- esphome/core/scheduler.h | 10 +-- 2 files changed, 54 insertions(+), 113 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 30c4cb8137..9035214faf 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -57,147 +57,92 @@ 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. -// Template implementation for set_timeout -template -void HOT Scheduler::set_timeout_impl_(Component *component, const NameType &name, uint32_t timeout, - std::function func, bool make_copy) { +// 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) { const auto now = this->millis_(); - // Handle empty name check based on type - bool is_empty = false; - if constexpr (std::is_same_v) { - is_empty = name.empty(); + // Get the name as const char* + const char *name_cstr = nullptr; + const std::string *name_str = nullptr; + + if (is_static_string) { + name_cstr = static_cast(name_ptr); } else { - is_empty = (name == nullptr || name[0] == '\0'); + name_str = static_cast(name_ptr); + name_cstr = name_str->c_str(); } - if (!is_empty) - this->cancel_timeout(component, name); + // Check if name is empty + bool is_empty = (name_cstr == nullptr || name_cstr[0] == '\0'); - if (timeout == SCHEDULER_DONT_RUN) + if (!is_empty) { + if (type == SchedulerItem::TIMEOUT) { + this->cancel_timeout(component, name_cstr); + } else { + this->cancel_interval(component, name_cstr); + } + } + + if (delay == SCHEDULER_DONT_RUN) return; + // For intervals, calculate offset + uint32_t offset = 0; + if (type == SchedulerItem::INTERVAL && delay != 0) { + offset = (random_uint32() % delay) / 2; + } + auto item = make_unique(); item->component = component; - // Set name based on type - if constexpr (std::is_same_v) { - item->set_name(name.c_str(), make_copy); - } else { - item->set_name(name, make_copy); -#ifdef ESPHOME_DEBUG_SCHEDULER - // Validate static strings in debug mode - if (!make_copy && name != nullptr) { - validate_static_string(name); - } -#endif - } + // Set name with appropriate copy flag + item->set_name(name_cstr, !is_static_string); - item->type = SchedulerItem::TIMEOUT; - item->next_execution_ = now + timeout; +#ifdef ESPHOME_DEBUG_SCHEDULER + // Validate static strings in debug mode + if (is_static_string && name_cstr != nullptr) { + validate_static_string(name_cstr); + } +#endif + + item->type = type; + item->interval = (type == SchedulerItem::INTERVAL) ? delay : 0; + item->next_execution_ = now + ((type == SchedulerItem::TIMEOUT) ? delay : offset); item->callback = std::move(func); item->remove = false; + #ifdef ESPHOME_DEBUG_SCHEDULER - const char *name_str = nullptr; - if constexpr (std::is_same_v) { - name_str = name.c_str(); + const char *type_str = (type == SchedulerItem::TIMEOUT) ? "timeout" : "interval"; + if (type == SchedulerItem::TIMEOUT) { + ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ")", type_str, item->get_source(), name_cstr, type_str, delay); } else { - name_str = name; + ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ", offset=%" PRIu32 ")", type_str, item->get_source(), name_cstr, + type_str, delay, offset); } - ESP_LOGD(TAG, "set_timeout(name='%s/%s', timeout=%" PRIu32 ")", item->get_source(), name_str, timeout); #endif this->push_(std::move(item)); } -// Explicit instantiations -template void Scheduler::set_timeout_impl_(Component *, const std::string &, uint32_t, - std::function, bool); -template void Scheduler::set_timeout_impl_(Component *, const char *const &, uint32_t, - std::function, bool); - void HOT Scheduler::set_timeout(Component *component, const char *name, uint32_t timeout, std::function func) { - return this->set_timeout_impl_(component, name, timeout, std::move(func), false); + this->set_timer_common_(component, SchedulerItem::TIMEOUT, true, name, timeout, std::move(func)); } void HOT Scheduler::set_timeout(Component *component, const std::string &name, uint32_t timeout, std::function func) { - return this->set_timeout_impl_(component, name, timeout, std::move(func), true); + 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); } -// Template implementation for set_interval -template -void HOT Scheduler::set_interval_impl_(Component *component, const NameType &name, uint32_t interval, - std::function func, bool make_copy) { - const auto now = this->millis_(); - - // Handle empty name check based on type - bool is_empty = false; - if constexpr (std::is_same_v) { - is_empty = name.empty(); - } else { - is_empty = (name == nullptr || name[0] == '\0'); - } - - if (!is_empty) - this->cancel_interval(component, name); - - if (interval == SCHEDULER_DONT_RUN) - return; - - // only put offset in lower half - uint32_t offset = 0; - if (interval != 0) - offset = (random_uint32() % interval) / 2; - - auto item = make_unique(); - item->component = component; - - // Set name based on type - if constexpr (std::is_same_v) { - item->set_name(name.c_str(), make_copy); - } else { - item->set_name(name, make_copy); -#ifdef ESPHOME_DEBUG_SCHEDULER - // Validate static strings in debug mode - if (!make_copy && name != nullptr) { - validate_static_string(name); - } -#endif - } - - item->type = SchedulerItem::INTERVAL; - item->interval = interval; - item->next_execution_ = now + offset; - item->callback = std::move(func); - item->remove = false; -#ifdef ESPHOME_DEBUG_SCHEDULER - const char *name_str = nullptr; - if constexpr (std::is_same_v) { - name_str = name.c_str(); - } else { - name_str = name; - } - ESP_LOGD(TAG, "set_interval(name='%s/%s', interval=%" PRIu32 ", offset=%" PRIu32 ")", item->get_source(), name_str, - interval, offset); -#endif - this->push_(std::move(item)); -} - -// Explicit instantiations -template void Scheduler::set_interval_impl_(Component *, const std::string &, uint32_t, - std::function, bool); -template void Scheduler::set_interval_impl_(Component *, const char *const &, uint32_t, - std::function, bool); - void HOT Scheduler::set_interval(Component *component, const std::string &name, uint32_t interval, std::function func) { - return this->set_interval_impl_(component, name, interval, std::move(func), true); + this->set_timer_common_(component, SchedulerItem::INTERVAL, false, &name, interval, std::move(func)); } + void HOT Scheduler::set_interval(Component *component, const char *name, uint32_t interval, std::function func) { - return this->set_interval_impl_(component, name, interval, std::move(func), false); + 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); diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 73940d7fff..7fc2e42b99 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -55,13 +55,9 @@ class Scheduler { void process_to_add(); protected: - // Template helper to handle both const char* and std::string efficiently - template - void set_timeout_impl_(Component *component, const NameType &name, uint32_t timeout, std::function func, - bool make_copy); - template - void set_interval_impl_(Component *component, const NameType &name, uint32_t interval, std::function func, - bool make_copy); + // Common implementation for both timeout and interval + void set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, + uint32_t delay, std::function func); struct SchedulerItem { // Ordered by size to minimize padding From a15b9f5d3b5ecb3e631ab699b6b41bb43b3475c4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 09:45:59 -0500 Subject: [PATCH 06/12] dry --- esphome/core/scheduler.cpp | 60 +++++++++++++++----------------------- esphome/core/scheduler.h | 8 ++--- 2 files changed, 28 insertions(+), 40 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 9035214faf..c8d7f877b9 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -60,67 +60,55 @@ static void validate_static_string(const char *name) { // 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) { - const auto now = this->millis_(); - // Get the name as const char* - const char *name_cstr = nullptr; - const std::string *name_str = nullptr; + const char *name_cstr = + is_static_string ? static_cast(name_ptr) : static_cast(name_ptr)->c_str(); - if (is_static_string) { - name_cstr = static_cast(name_ptr); - } else { - name_str = static_cast(name_ptr); - name_cstr = name_str->c_str(); - } - - // Check if name is empty - bool is_empty = (name_cstr == nullptr || name_cstr[0] == '\0'); - - if (!is_empty) { - if (type == SchedulerItem::TIMEOUT) { - this->cancel_timeout(component, name_cstr); - } else { - this->cancel_interval(component, name_cstr); - } + // 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) return; - // For intervals, calculate offset - uint32_t offset = 0; - if (type == SchedulerItem::INTERVAL && delay != 0) { - offset = (random_uint32() % delay) / 2; - } + const auto now = this->millis_(); + // Create and populate the scheduler item auto item = make_unique(); item->component = component; - - // Set name with appropriate copy flag item->set_name(name_cstr, !is_static_string); + item->type = type; + item->callback = std::move(func); + item->remove = false; + + // Type-specific setup + if (type == SchedulerItem::INTERVAL) { + item->interval = delay; + // Calculate random offset (0 to interval/2) + uint32_t offset = (delay != 0) ? (random_uint32() % delay) / 2 : 0; + item->next_execution_ = now + offset; + } else { + item->interval = 0; + item->next_execution_ = now + delay; + } #ifdef ESPHOME_DEBUG_SCHEDULER // Validate static strings in debug mode if (is_static_string && name_cstr != nullptr) { validate_static_string(name_cstr); } -#endif - item->type = type; - item->interval = (type == SchedulerItem::INTERVAL) ? delay : 0; - item->next_execution_ = now + ((type == SchedulerItem::TIMEOUT) ? delay : offset); - item->callback = std::move(func); - item->remove = false; - -#ifdef ESPHOME_DEBUG_SCHEDULER + // Debug logging const char *type_str = (type == SchedulerItem::TIMEOUT) ? "timeout" : "interval"; if (type == SchedulerItem::TIMEOUT) { ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ")", type_str, item->get_source(), name_cstr, type_str, delay); } else { ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ", offset=%" PRIu32 ")", type_str, item->get_source(), name_cstr, - type_str, delay, offset); + type_str, delay, static_cast(item->next_execution_ - now)); } #endif + this->push_(std::move(item)); } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 7fc2e42b99..fa808df2e7 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -55,10 +55,6 @@ class Scheduler { void process_to_add(); protected: - // Common implementation for both timeout and interval - void set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, - uint32_t delay, std::function func); - struct SchedulerItem { // Ordered by size to minimize padding Component *component; @@ -143,6 +139,10 @@ class Scheduler { } }; + // Common implementation for both timeout and interval + void set_timer_common_(Component *component, SchedulerItem::Type type, bool is_static_string, const void *name_ptr, + uint32_t delay, std::function func); + uint64_t millis_(); void cleanup_(); void pop_raw_(); From 0a3bbb8554fdc4583e49b0324f9d9a36a6667e97 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 09:48:26 -0500 Subject: [PATCH 07/12] dry --- esphome/core/scheduler.h | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index fa808df2e7..3c23aace62 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -80,13 +80,7 @@ class Scheduler { // Constructor SchedulerItem() - : component(nullptr), - interval(0), - next_execution_(0), - callback(nullptr), - type(TIMEOUT), - remove(false), - owns_name(false) { + : component(nullptr), interval(0), next_execution_(0), type(TIMEOUT), remove(false), owns_name(false) { name_.static_name = nullptr; } @@ -105,11 +99,11 @@ class Scheduler { // Clean up old dynamic name if any if (owns_name && name_.dynamic_name) { delete[] name_.dynamic_name; + owns_name = false; } - if (name == nullptr || name[0] == '\0') { + if (!name || !name[0]) { name_.static_name = nullptr; - owns_name = false; } else if (make_copy) { // Make a copy for dynamic strings size_t len = strlen(name); @@ -119,24 +113,12 @@ class Scheduler { } else { // Use static string directly name_.static_name = name; - owns_name = false; } } static bool cmp(const std::unique_ptr &a, const std::unique_ptr &b); - const char *get_type_str() { - switch (this->type) { - case SchedulerItem::INTERVAL: - return "interval"; - case SchedulerItem::TIMEOUT: - return "timeout"; - default: - return ""; - } - } - const char *get_source() { - return this->component != nullptr ? this->component->get_component_source() : "unknown"; - } + const char *get_type_str() const { return (type == TIMEOUT) ? "timeout" : "interval"; } + const char *get_source() const { return component ? component->get_component_source() : "unknown"; } }; // Common implementation for both timeout and interval From df3469efbad837da510aca81164e8f2b58cfc46b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 09:48:58 -0500 Subject: [PATCH 08/12] dry --- 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 3c23aace62..0d1dc45d52 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -86,11 +86,19 @@ class Scheduler { // Destructor to clean up dynamic names ~SchedulerItem() { - if (owns_name && name_.dynamic_name) { + if (owns_name) { delete[] name_.dynamic_name; } } + // Delete copy operations to prevent accidental copies + SchedulerItem(const SchedulerItem &) = delete; + SchedulerItem &operator=(const SchedulerItem &) = delete; + + // Default move operations + SchedulerItem(SchedulerItem &&) = default; + SchedulerItem &operator=(SchedulerItem &&) = default; + // Helper to get the name regardless of storage type const char *get_name() const { return owns_name ? name_.dynamic_name : name_.static_name; } From a9ace366ebb53a795a54d8c4b20c10f197f10331 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 09:50:27 -0500 Subject: [PATCH 09/12] dry --- esphome/core/scheduler.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 0d1dc45d52..4b0dc77c14 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -75,18 +75,18 @@ class Scheduler { // Bit-packed fields to minimize padding enum Type : uint8_t { TIMEOUT, INTERVAL } type : 1; bool remove : 1; - bool owns_name : 1; // True if name_.dynamic_name needs to be freed + bool name_is_dynamic : 1; // True if name was dynamically allocated (needs delete[]) // 5 bits padding // Constructor SchedulerItem() - : component(nullptr), interval(0), next_execution_(0), type(TIMEOUT), remove(false), owns_name(false) { + : component(nullptr), interval(0), next_execution_(0), type(TIMEOUT), remove(false), name_is_dynamic(false) { name_.static_name = nullptr; } // Destructor to clean up dynamic names ~SchedulerItem() { - if (owns_name) { + if (name_is_dynamic) { delete[] name_.dynamic_name; } } @@ -100,14 +100,14 @@ class Scheduler { SchedulerItem &operator=(SchedulerItem &&) = default; // Helper to get the name regardless of storage type - const char *get_name() const { return owns_name ? name_.dynamic_name : name_.static_name; } + const char *get_name() const { return name_is_dynamic ? name_.dynamic_name : name_.static_name; } // Helper to set name with proper ownership void set_name(const char *name, bool make_copy = false) { // Clean up old dynamic name if any - if (owns_name && name_.dynamic_name) { + if (name_is_dynamic && name_.dynamic_name) { delete[] name_.dynamic_name; - owns_name = false; + name_is_dynamic = false; } if (!name || !name[0]) { @@ -117,7 +117,7 @@ class Scheduler { size_t len = strlen(name); name_.dynamic_name = new char[len + 1]; strcpy(name_.dynamic_name, name); - owns_name = true; + name_is_dynamic = true; } else { // Use static string directly name_.static_name = name; From 67a20e212d7a950379c899881c14ea6a058bcaed Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 09:59:50 -0500 Subject: [PATCH 10/12] safe --- esphome/core/scheduler.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index c8d7f877b9..25df4bf50c 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -102,10 +102,11 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // Debug logging const char *type_str = (type == SchedulerItem::TIMEOUT) ? "timeout" : "interval"; if (type == SchedulerItem::TIMEOUT) { - ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ")", type_str, item->get_source(), name_cstr, type_str, delay); + ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ")", type_str, item->get_source(), + name_cstr ? name_cstr : "(null)", type_str, delay); } else { - ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ", offset=%" PRIu32 ")", type_str, item->get_source(), name_cstr, - type_str, delay, static_cast(item->next_execution_ - now)); + ESP_LOGD(TAG, "set_%s(name='%s/%s', %s=%" PRIu32 ", offset=%" PRIu32 ")", type_str, item->get_source(), + name_cstr ? name_cstr : "(null)", type_str, delay, static_cast(item->next_execution_ - now)); } #endif @@ -277,8 +278,10 @@ void HOT Scheduler::call() { 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 ")", - item->get_type_str(), item->get_source(), item->get_name(), item->interval, item->next_execution_, now); + item->get_type_str(), item->get_source(), item_name ? item_name : "(null)", item->interval, + item->next_execution_, now); #endif // Warning: During callback(), a lot of stuff can happen, including: From 2946bc9d72358ced7945e414a999ead20d2fe500 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 10:10:43 -0500 Subject: [PATCH 11/12] cover --- .../fixtures/scheduler_string_test.yaml | 156 +++++++++++++++++ .../integration/test_scheduler_string_test.py | 163 ++++++++++++++++++ 2 files changed, 319 insertions(+) create mode 100644 tests/integration/fixtures/scheduler_string_test.yaml create mode 100644 tests/integration/test_scheduler_string_test.py diff --git a/tests/integration/fixtures/scheduler_string_test.yaml b/tests/integration/fixtures/scheduler_string_test.yaml new file mode 100644 index 0000000000..1c0e22ecec --- /dev/null +++ b/tests/integration/fixtures/scheduler_string_test.yaml @@ -0,0 +1,156 @@ +esphome: + name: scheduler-string-test + on_boot: + priority: -100 + then: + - logger.log: "Starting scheduler string tests" + platformio_options: + build_flags: + - "-DESPHOME_DEBUG_SCHEDULER" # Enable scheduler debug logging + +host: +api: +logger: + level: VERBOSE + +globals: + - id: timeout_counter + type: int + initial_value: '0' + - id: interval_counter + type: int + initial_value: '0' + - id: dynamic_counter + type: int + initial_value: '0' + - id: static_tests_done + type: bool + initial_value: 'false' + - id: dynamic_tests_done + type: bool + initial_value: 'false' + - id: results_reported + type: bool + initial_value: 'false' + +script: + - id: test_static_strings + then: + - logger.log: "Testing static string timeouts and intervals" + - lambda: |- + auto *component1 = id(test_sensor1); + // Test 1: Static string literals with set_timeout + App.scheduler.set_timeout(component1, "static_timeout_1", 100, []() { + ESP_LOGI("test", "Static timeout 1 fired"); + id(timeout_counter) += 1; + }); + + // Test 2: Static const char* with set_timeout + static const char* TIMEOUT_NAME = "static_timeout_2"; + App.scheduler.set_timeout(component1, TIMEOUT_NAME, 200, []() { + ESP_LOGI("test", "Static timeout 2 fired"); + id(timeout_counter) += 1; + }); + + // Test 3: Static string literal with set_interval + App.scheduler.set_interval(component1, "static_interval_1", 500, []() { + ESP_LOGI("test", "Static interval 1 fired, count: %d", id(interval_counter)); + id(interval_counter) += 1; + if (id(interval_counter) >= 3) { + App.scheduler.cancel_interval(id(test_sensor1), "static_interval_1"); + ESP_LOGI("test", "Cancelled static interval 1"); + } + }); + + // Test 4: Empty string (should be handled safely) + App.scheduler.set_timeout(component1, "", 300, []() { + ESP_LOGI("test", "Empty string timeout fired"); + }); + + - id: test_dynamic_strings + then: + - logger.log: "Testing dynamic string timeouts and intervals" + - lambda: |- + auto *component2 = id(test_sensor2); + + // Test 5: Dynamic string with set_timeout (std::string) + std::string dynamic_name = "dynamic_timeout_" + std::to_string(id(dynamic_counter)++); + App.scheduler.set_timeout(component2, dynamic_name, 150, []() { + ESP_LOGI("test", "Dynamic timeout fired"); + id(timeout_counter) += 1; + }); + + // Test 6: Dynamic string with set_interval + std::string interval_name = "dynamic_interval_" + std::to_string(id(dynamic_counter)++); + App.scheduler.set_interval(component2, interval_name, 600, [interval_name]() { + ESP_LOGI("test", "Dynamic interval fired: %s", interval_name.c_str()); + id(interval_counter) += 1; + if (id(interval_counter) >= 6) { + App.scheduler.cancel_interval(id(test_sensor2), interval_name); + ESP_LOGI("test", "Cancelled dynamic interval"); + } + }); + + // Test 7: Cancel with different string object but same content + std::string cancel_name = "cancel_test"; + App.scheduler.set_timeout(component2, cancel_name, 5000, []() { + ESP_LOGI("test", "This should be cancelled"); + }); + + // Cancel using a different string object + std::string cancel_name_2 = "cancel_test"; + App.scheduler.cancel_timeout(component2, cancel_name_2); + ESP_LOGI("test", "Cancelled timeout using different string object"); + + - id: report_results + then: + - lambda: |- + ESP_LOGI("test", "Final results - Timeouts: %d, Intervals: %d", + id(timeout_counter), id(interval_counter)); + +sensor: + - platform: template + name: Test Sensor 1 + id: test_sensor1 + lambda: return 1.0; + update_interval: never + + - platform: template + name: Test Sensor 2 + id: test_sensor2 + lambda: return 2.0; + update_interval: never + +interval: + # Run static string tests after boot - using script to run once + - interval: 0.5s + then: + - if: + condition: + lambda: 'return id(static_tests_done) == false;' + then: + - lambda: 'id(static_tests_done) = true;' + - script.execute: test_static_strings + - logger.log: "Started static string tests" + + # Run dynamic string tests after static tests + - interval: 1s + then: + - if: + condition: + lambda: 'return id(static_tests_done) && !id(dynamic_tests_done);' + then: + - lambda: 'id(dynamic_tests_done) = true;' + - delay: 1s + - script.execute: test_dynamic_strings + + # Report results after all tests + - interval: 1s + then: + - if: + condition: + lambda: 'return id(dynamic_tests_done) && !id(results_reported);' + then: + - lambda: 'id(results_reported) = true;' + - delay: 3s + - script.execute: report_results diff --git a/tests/integration/test_scheduler_string_test.py b/tests/integration/test_scheduler_string_test.py new file mode 100644 index 0000000000..54b78d697b --- /dev/null +++ b/tests/integration/test_scheduler_string_test.py @@ -0,0 +1,163 @@ +"""Test scheduler string optimization with static and dynamic strings.""" + +import asyncio +import re + +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_scheduler_string_test( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that scheduler handles both static and dynamic strings correctly.""" + # Track counts + timeout_count = 0 + interval_count = 0 + + # Events for each test completion + static_timeout_1_fired = asyncio.Event() + static_timeout_2_fired = asyncio.Event() + static_interval_fired = asyncio.Event() + static_interval_cancelled = asyncio.Event() + empty_string_timeout_fired = asyncio.Event() + dynamic_timeout_fired = asyncio.Event() + dynamic_interval_fired = asyncio.Event() + cancel_test_done = asyncio.Event() + final_results_logged = asyncio.Event() + + # Track interval counts + static_interval_count = 0 + dynamic_interval_count = 0 + + def on_log_line(line: str) -> None: + nonlocal \ + timeout_count, \ + interval_count, \ + static_interval_count, \ + dynamic_interval_count + + # Strip ANSI color codes + clean_line = re.sub(r"\x1b\[[0-9;]*m", "", line) + + # Check for static timeout completions + if "Static timeout 1 fired" in clean_line: + static_timeout_1_fired.set() + timeout_count += 1 + + elif "Static timeout 2 fired" in clean_line: + static_timeout_2_fired.set() + timeout_count += 1 + + # Check for static interval + elif "Static interval 1 fired" in clean_line: + match = re.search(r"count: (\d+)", clean_line) + if match: + static_interval_count = int(match.group(1)) + static_interval_fired.set() + + elif "Cancelled static interval 1" in clean_line: + static_interval_cancelled.set() + + # Check for empty string timeout + elif "Empty string timeout fired" in clean_line: + empty_string_timeout_fired.set() + + # Check for dynamic string tests + elif "Dynamic timeout fired" in clean_line: + dynamic_timeout_fired.set() + timeout_count += 1 + + elif "Dynamic interval fired" in clean_line: + dynamic_interval_count += 1 + dynamic_interval_fired.set() + + # Check for cancel test + elif "Cancelled timeout using different string object" in clean_line: + cancel_test_done.set() + + # Check for final results + elif "Final results" in clean_line: + match = re.search(r"Timeouts: (\d+), Intervals: (\d+)", clean_line) + if match: + timeout_count = int(match.group(1)) + interval_count = int(match.group(2)) + final_results_logged.set() + + 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-string-test" + + # Wait for static string tests + try: + await asyncio.wait_for(static_timeout_1_fired.wait(), timeout=3.0) + except asyncio.TimeoutError: + pytest.fail("Static timeout 1 did not fire within 3 seconds") + + try: + await asyncio.wait_for(static_timeout_2_fired.wait(), timeout=3.0) + except asyncio.TimeoutError: + pytest.fail("Static timeout 2 did not fire within 3 seconds") + + try: + await asyncio.wait_for(static_interval_fired.wait(), timeout=3.0) + except asyncio.TimeoutError: + pytest.fail("Static interval did not fire within 3 seconds") + + try: + await asyncio.wait_for(static_interval_cancelled.wait(), timeout=3.0) + except asyncio.TimeoutError: + pytest.fail("Static interval was not cancelled within 3 seconds") + + # Verify static interval ran at least 3 times + assert static_interval_count >= 2, ( + f"Expected static interval to run at least 3 times, got {static_interval_count + 1}" + ) + + # Wait for dynamic string tests + try: + await asyncio.wait_for(dynamic_timeout_fired.wait(), timeout=5.0) + except asyncio.TimeoutError: + pytest.fail("Dynamic timeout did not fire within 5 seconds") + + try: + await asyncio.wait_for(dynamic_interval_fired.wait(), timeout=5.0) + except asyncio.TimeoutError: + pytest.fail("Dynamic interval did not fire within 5 seconds") + + # Wait for cancel test + try: + await asyncio.wait_for(cancel_test_done.wait(), timeout=5.0) + except asyncio.TimeoutError: + pytest.fail("Cancel test did not complete within 5 seconds") + + # Wait for final results + try: + await asyncio.wait_for(final_results_logged.wait(), timeout=10.0) + except asyncio.TimeoutError: + pytest.fail("Final results were not logged within 10 seconds") + + # Verify results + assert timeout_count >= 3, f"Expected at least 3 timeouts, got {timeout_count}" + assert interval_count >= 3, ( + f"Expected at least 3 interval fires, got {interval_count}" + ) + + # Empty string timeout DOES fire (scheduler accepts empty names) + assert empty_string_timeout_fired.is_set(), "Empty string timeout should fire" + + # Log final status + print("\nScheduler string test completed successfully:") + print(f" Timeouts fired: {timeout_count}") + print(f" Intervals fired: {interval_count}") + print(f" Static interval count: {static_interval_count + 1}") + print(f" Dynamic interval count: {dynamic_interval_count}") From 53b9c8d5bbe29571c3dd987be8fbd4436fda0aff Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Jun 2025 10:15:05 -0500 Subject: [PATCH 12/12] cleanup --- esphome/core/scheduler.cpp | 2 +- .../fixtures/scheduler_string_test.yaml | 24 ++++++------ .../integration/test_scheduler_string_test.py | 39 ++++++++----------- 3 files changed, 29 insertions(+), 36 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 25df4bf50c..67fb87f58d 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -211,7 +211,7 @@ void HOT Scheduler::call() { if (now - last_print > 2000) { last_print = now; std::vector> old_items; - ESP_LOGD(TAG, "Items: count=%u, now=%" PRIu64 " (%u, %" PRIu32 ")", this->items_.size(), now, this->millis_major_, + ESP_LOGD(TAG, "Items: count=%zu, now=%" PRIu64 " (%u, %" PRIu32 ")", this->items_.size(), now, this->millis_major_, this->last_millis_); while (!this->empty_()) { this->lock_.lock(); diff --git a/tests/integration/fixtures/scheduler_string_test.yaml b/tests/integration/fixtures/scheduler_string_test.yaml index 1c0e22ecec..ed10441ccc 100644 --- a/tests/integration/fixtures/scheduler_string_test.yaml +++ b/tests/integration/fixtures/scheduler_string_test.yaml @@ -40,20 +40,20 @@ script: - lambda: |- auto *component1 = id(test_sensor1); // Test 1: Static string literals with set_timeout - App.scheduler.set_timeout(component1, "static_timeout_1", 100, []() { + App.scheduler.set_timeout(component1, "static_timeout_1", 50, []() { ESP_LOGI("test", "Static timeout 1 fired"); id(timeout_counter) += 1; }); // Test 2: Static const char* with set_timeout static const char* TIMEOUT_NAME = "static_timeout_2"; - App.scheduler.set_timeout(component1, TIMEOUT_NAME, 200, []() { + App.scheduler.set_timeout(component1, TIMEOUT_NAME, 100, []() { ESP_LOGI("test", "Static timeout 2 fired"); id(timeout_counter) += 1; }); // Test 3: Static string literal with set_interval - App.scheduler.set_interval(component1, "static_interval_1", 500, []() { + App.scheduler.set_interval(component1, "static_interval_1", 200, []() { ESP_LOGI("test", "Static interval 1 fired, count: %d", id(interval_counter)); id(interval_counter) += 1; if (id(interval_counter) >= 3) { @@ -63,7 +63,7 @@ script: }); // Test 4: Empty string (should be handled safely) - App.scheduler.set_timeout(component1, "", 300, []() { + App.scheduler.set_timeout(component1, "", 150, []() { ESP_LOGI("test", "Empty string timeout fired"); }); @@ -75,14 +75,14 @@ script: // Test 5: Dynamic string with set_timeout (std::string) std::string dynamic_name = "dynamic_timeout_" + std::to_string(id(dynamic_counter)++); - App.scheduler.set_timeout(component2, dynamic_name, 150, []() { + App.scheduler.set_timeout(component2, dynamic_name, 100, []() { ESP_LOGI("test", "Dynamic timeout fired"); id(timeout_counter) += 1; }); // Test 6: Dynamic string with set_interval std::string interval_name = "dynamic_interval_" + std::to_string(id(dynamic_counter)++); - App.scheduler.set_interval(component2, interval_name, 600, [interval_name]() { + App.scheduler.set_interval(component2, interval_name, 250, [interval_name]() { ESP_LOGI("test", "Dynamic interval fired: %s", interval_name.c_str()); id(interval_counter) += 1; if (id(interval_counter) >= 6) { @@ -93,7 +93,7 @@ script: // Test 7: Cancel with different string object but same content std::string cancel_name = "cancel_test"; - App.scheduler.set_timeout(component2, cancel_name, 5000, []() { + App.scheduler.set_timeout(component2, cancel_name, 2000, []() { ESP_LOGI("test", "This should be cancelled"); }); @@ -123,7 +123,7 @@ sensor: interval: # Run static string tests after boot - using script to run once - - interval: 0.5s + - interval: 0.1s then: - if: condition: @@ -134,23 +134,23 @@ interval: - logger.log: "Started static string tests" # Run dynamic string tests after static tests - - interval: 1s + - interval: 0.2s then: - if: condition: lambda: 'return id(static_tests_done) && !id(dynamic_tests_done);' then: - lambda: 'id(dynamic_tests_done) = true;' - - delay: 1s + - delay: 0.2s - script.execute: test_dynamic_strings # Report results after all tests - - interval: 1s + - interval: 0.2s then: - if: condition: lambda: 'return id(dynamic_tests_done) && !id(results_reported);' then: - lambda: 'id(results_reported) = true;' - - delay: 3s + - delay: 1s - script.execute: report_results diff --git a/tests/integration/test_scheduler_string_test.py b/tests/integration/test_scheduler_string_test.py index 54b78d697b..2953278367 100644 --- a/tests/integration/test_scheduler_string_test.py +++ b/tests/integration/test_scheduler_string_test.py @@ -99,24 +99,24 @@ async def test_scheduler_string_test( # Wait for static string tests try: - await asyncio.wait_for(static_timeout_1_fired.wait(), timeout=3.0) + await asyncio.wait_for(static_timeout_1_fired.wait(), timeout=0.5) except asyncio.TimeoutError: - pytest.fail("Static timeout 1 did not fire within 3 seconds") + pytest.fail("Static timeout 1 did not fire within 0.5 seconds") try: - await asyncio.wait_for(static_timeout_2_fired.wait(), timeout=3.0) + await asyncio.wait_for(static_timeout_2_fired.wait(), timeout=0.5) except asyncio.TimeoutError: - pytest.fail("Static timeout 2 did not fire within 3 seconds") + pytest.fail("Static timeout 2 did not fire within 0.5 seconds") try: - await asyncio.wait_for(static_interval_fired.wait(), timeout=3.0) + await asyncio.wait_for(static_interval_fired.wait(), timeout=1.0) except asyncio.TimeoutError: - pytest.fail("Static interval did not fire within 3 seconds") + pytest.fail("Static interval did not fire within 1 seconds") try: - await asyncio.wait_for(static_interval_cancelled.wait(), timeout=3.0) + await asyncio.wait_for(static_interval_cancelled.wait(), timeout=2.0) except asyncio.TimeoutError: - pytest.fail("Static interval was not cancelled within 3 seconds") + pytest.fail("Static interval was not cancelled within 2 seconds") # Verify static interval ran at least 3 times assert static_interval_count >= 2, ( @@ -125,26 +125,26 @@ async def test_scheduler_string_test( # Wait for dynamic string tests try: - await asyncio.wait_for(dynamic_timeout_fired.wait(), timeout=5.0) + await asyncio.wait_for(dynamic_timeout_fired.wait(), timeout=1.0) except asyncio.TimeoutError: - pytest.fail("Dynamic timeout did not fire within 5 seconds") + pytest.fail("Dynamic timeout did not fire within 1 seconds") try: - await asyncio.wait_for(dynamic_interval_fired.wait(), timeout=5.0) + await asyncio.wait_for(dynamic_interval_fired.wait(), timeout=1.5) except asyncio.TimeoutError: - pytest.fail("Dynamic interval did not fire within 5 seconds") + pytest.fail("Dynamic interval did not fire within 1.5 seconds") # Wait for cancel test try: - await asyncio.wait_for(cancel_test_done.wait(), timeout=5.0) + await asyncio.wait_for(cancel_test_done.wait(), timeout=1.0) except asyncio.TimeoutError: - pytest.fail("Cancel test did not complete within 5 seconds") + pytest.fail("Cancel test did not complete within 1 seconds") # Wait for final results try: - await asyncio.wait_for(final_results_logged.wait(), timeout=10.0) + await asyncio.wait_for(final_results_logged.wait(), timeout=4.0) except asyncio.TimeoutError: - pytest.fail("Final results were not logged within 10 seconds") + pytest.fail("Final results were not logged within 4 seconds") # Verify results assert timeout_count >= 3, f"Expected at least 3 timeouts, got {timeout_count}" @@ -154,10 +154,3 @@ async def test_scheduler_string_test( # Empty string timeout DOES fire (scheduler accepts empty names) assert empty_string_timeout_fired.is_set(), "Empty string timeout should fire" - - # Log final status - print("\nScheduler string test completed successfully:") - print(f" Timeouts fired: {timeout_count}") - print(f" Intervals fired: {interval_count}") - print(f" Static interval count: {static_interval_count + 1}") - print(f" Dynamic interval count: {dynamic_interval_count}")