diff --git a/esphome/components/web_server/web_server.cpp b/esphome/components/web_server/web_server.cpp index f15448419d..faa2c728ca 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/scheduler.cpp b/esphome/core/scheduler.cpp index 5c01b4f3f4..e0d2b70102 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,16 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type item->callback = std::move(func); item->remove = false; + // Special handling for defer() (delay = 0, type = TIMEOUT) + 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; + } + + const auto now = this->millis_(); + // Type-specific setup if (type == SchedulerItem::INTERVAL) { item->interval = delay; @@ -209,6 +217,28 @@ optional HOT Scheduler::next_schedule_in() { return item->next_execution_ - now; } void HOT Scheduler::call() { + // 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, RP2040, 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 + while (!this->defer_queue_.empty()) { + std::unique_ptr item; + { + LockGuard guard{this->lock_}; + if (this->defer_queue_.empty()) // Double-check with lock held + break; + item = std::move(this->defer_queue_.front()); + this->defer_queue_.pop_front(); + } + // Skip if item was marked for removal or component failed + if (!this->should_skip_item_(item.get())) { + this->execute_item_(item.get()); + } + } + const auto now = this->millis_(); this->process_to_add(); @@ -294,13 +324,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 +388,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 +423,25 @@ 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 + 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; + + 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..e617eb99c2 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" @@ -145,6 +146,18 @@ class Scheduler { 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 +166,7 @@ class Scheduler { Mutex lock_; std::vector> items_; std::vector> to_add_; + std::deque> defer_queue_; // FIFO queue for defer() calls 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..29c1a2bf38 --- /dev/null +++ b/tests/integration/fixtures/defer_fifo_simple.yaml @@ -0,0 +1,93 @@ +esphome: + name: defer-fifo-simple + on_boot: + - lambda: |- + // Test 1: Test set_timeout with 0 delay (direct scheduler call) + static int set_timeout_order = 0; + static bool set_timeout_passed = true; + + ESP_LOGD("defer_test", "Test 1: 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 1 PASSED - set_timeout(0) maintains FIFO order"); + } else { + ESP_LOGE("defer_test", "✗ Test 1 FAILED - set_timeout(0) executed out of order"); + } + + // Start Test 2 after Test 1 completes + App.scheduler.set_timeout((Component*)nullptr, nullptr, 100, []() { + // Test 2: Test defer() method (component method) + static int defer_order = 0; + static bool defer_passed = true; + + ESP_LOGD("defer_test", "Test 2: 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) { + bool all_passed = set_timeout_passed && defer_passed; + if (defer_passed) { + ESP_LOGI("defer_test", "✓ Test 2 PASSED - defer() maintains FIFO order"); + if (all_passed) { + ESP_LOGI("defer_test", "✓ ALL TESTS PASSED - Both set_timeout(0) and defer() maintain FIFO order"); + } + } else { + ESP_LOGE("defer_test", "✗ Test 2 FAILED - defer() executed out of order"); + } + + // Publish test results + id(test_complete)->publish_state(true); + id(test_passed)->publish_state(all_passed); + } + }); + } + } + }; + + TestComponent test_component; + test_component.test_defer(); + + ESP_LOGD("defer_test", "Deferred 10 items using defer(), waiting for execution..."); + }); + } + }); + } + + ESP_LOGD("defer_test", "Deferred 10 items using set_timeout(0), waiting for execution..."); + +host: + +logger: + level: DEBUG + +api: + +binary_sensor: + - platform: template + name: "Test Complete" + id: test_complete + - platform: template + name: "Test Passed" + id: test_passed diff --git a/tests/integration/test_defer_fifo_simple.py b/tests/integration/test_defer_fifo_simple.py new file mode 100644 index 0000000000..95a14e64b7 --- /dev/null +++ b/tests/integration/test_defer_fifo_simple.py @@ -0,0 +1,78 @@ +"""Simple test that defer() maintains FIFO order.""" + +import asyncio + +from aioesphomeapi import BinarySensorInfo, BinarySensorState, EntityState +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 to get the keys + entity_info, _ = await asyncio.wait_for( + client.list_entities_services(), timeout=5.0 + ) + + # Find our test entities + test_complete_entity: BinarySensorInfo | None = None + test_passed_entity: BinarySensorInfo | None = None + + for entity in entity_info: + if isinstance(entity, BinarySensorInfo): + if entity.object_id == "test_complete": + test_complete_entity = entity + elif entity.object_id == "test_passed": + test_passed_entity = entity + + assert test_complete_entity is not None, "test_complete sensor not found" + assert test_passed_entity is not None, "test_passed sensor not found" + + # Get the event loop + loop = asyncio.get_running_loop() + + # Subscribe to state changes + states: dict[int, EntityState] = {} + test_complete_future: asyncio.Future[BinarySensorState] = loop.create_future() + test_passed_future: asyncio.Future[BinarySensorState] = loop.create_future() + + def on_state(state: EntityState) -> None: + states[state.key] = state + # Check if this is our test_complete binary sensor + if isinstance(state, BinarySensorState): + if state.key == test_complete_entity.key: + if state.state and not test_complete_future.done(): + test_complete_future.set_result(state) + elif state.key == test_passed_entity.key: + if not test_passed_future.done(): + test_passed_future.set_result(state) + + client.subscribe_states(on_state) + + # Wait for test completion with timeout + try: + await asyncio.wait_for(test_complete_future, timeout=10.0) + test_passed_state = await asyncio.wait_for(test_passed_future, timeout=1.0) + except asyncio.TimeoutError: + pytest.fail( + f"Test did not complete within 10 seconds. " + f"Received states: {list(states.values())}" + ) + + # Verify the test passed + assert test_passed_state.state is True, ( + "FIFO test failed - items executed out of order" + )