From 560251ab2af52c2d52755d2c3effda2d1951636b Mon Sep 17 00:00:00 2001 From: Otto Winter Date: Thu, 31 Oct 2019 20:25:16 +0100 Subject: [PATCH] Scheduler fixes (#813) * Scheduler fixes Fixes https://github.com/esphome/issues/issues/789, fixes https://github.com/esphome/issues/issues/788 Also changes to use unique_ptr - this should be much safer than the raw pointers form before (though the scoping rules might cause some issues, but looking closely I didn't find anything) * Disable debugging * Format --- esphome/core/scheduler.cpp | 179 ++++++++++++++++++++----------------- esphome/core/scheduler.h | 18 +++- 2 files changed, 112 insertions(+), 85 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 88054147f8..cc4331b38e 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -9,6 +9,9 @@ static const char *TAG = "scheduler"; static const uint32_t SCHEDULER_DONT_RUN = 4294967295UL; +// Uncomment to debug scheduler +// #define ESPHOME_DEBUG_SCHEDULER + void HOT Scheduler::set_timeout(Component *component, const std::string &name, uint32_t timeout, std::function &&func) { const uint32_t now = this->millis_(); @@ -21,7 +24,7 @@ void HOT Scheduler::set_timeout(Component *component, const std::string &name, u ESP_LOGVV(TAG, "set_timeout(name='%s', timeout=%u)", name.c_str(), timeout); - auto *item = new SchedulerItem(); + auto item = make_unique(); item->component = component; item->name = name; item->type = SchedulerItem::TIMEOUT; @@ -30,7 +33,7 @@ void HOT Scheduler::set_timeout(Component *component, const std::string &name, u item->last_execution_major = this->millis_major_; item->f = std::move(func); item->remove = false; - this->push_(item); + this->push_(std::move(item)); } bool HOT Scheduler::cancel_timeout(Component *component, const std::string &name) { return this->cancel_item_(component, name, SchedulerItem::TIMEOUT); @@ -52,7 +55,7 @@ void HOT Scheduler::set_interval(Component *component, const std::string &name, ESP_LOGVV(TAG, "set_interval(name='%s', interval=%u, offset=%u)", name.c_str(), interval, offset); - auto *item = new SchedulerItem(); + auto item = make_unique(); item->component = component; item->name = name; item->type = SchedulerItem::INTERVAL; @@ -63,7 +66,7 @@ void HOT Scheduler::set_interval(Component *component, const std::string &name, item->last_execution_major--; item->f = std::move(func); item->remove = false; - this->push_(item); + this->push_(std::move(item)); } bool HOT Scheduler::cancel_interval(Component *component, const std::string &name) { return this->cancel_item_(component, name, SchedulerItem::INTERVAL); @@ -71,7 +74,7 @@ bool HOT Scheduler::cancel_interval(Component *component, const std::string &nam optional HOT Scheduler::next_schedule_in() { if (this->empty_()) return {}; - auto *item = this->items_[0]; + auto &item = this->items_[0]; const uint32_t now = this->millis_(); uint32_t next_time = item->last_execution + item->interval; if (next_time < now) @@ -82,98 +85,103 @@ void ICACHE_RAM_ATTR HOT Scheduler::call() { const uint32_t now = this->millis_(); this->process_to_add(); - // Uncomment for debugging the scheduler: +#ifdef ESPHOME_DEBUG_SCHEDULER + static uint32_t last_print = 0; - // if (random_uint32() % 400 == 0) { - // std::vector old_items = this->items_; - // ESP_LOGVV(TAG, "Items: count=%u, now=%u", this->items_.size(), now); - // while (!this->empty_()) { - // auto *item = this->items_[0]; - // const char *type = item->type == SchedulerItem::INTERVAL ? "interval" : "timeout"; - // ESP_LOGVV(TAG, " %s '%s' interval=%u last_execution=%u (%u) next=%u", - // type, item->name.c_str(), item->interval, item->last_execution, item->last_execution_major, - // item->last_execution + item->interval); - // this->pop_raw_(); - // } - // ESP_LOGVV(TAG, "\n"); - // this->items_ = old_items; - //} + if (now - last_print > 2000) { + last_print = now; + std::vector> old_items; + ESP_LOGVV(TAG, "Items: count=%u, now=%u", this->items_.size(), now); + while (!this->empty_()) { + auto item = std::move(this->items_[0]); + const char *type = item->type == SchedulerItem::INTERVAL ? "interval" : "timeout"; + ESP_LOGVV(TAG, " %s '%s' interval=%u last_execution=%u (%u) next=%u (%u)", type, item->name.c_str(), + item->interval, item->last_execution, item->last_execution_major, item->next_execution(), + item->next_execution_major()); + + this->pop_raw_(); + old_items.push_back(std::move(item)); + } + ESP_LOGVV(TAG, "\n"); + this->items_ = std::move(old_items); + } +#endif // ESPHOME_DEBUG_SCHEDULER while (!this->empty_()) { - // Don't copy-by value yet - auto *item = this->items_[0]; - if ((now - item->last_execution) < item->interval) - // Not reached timeout yet, done for this call - break; - uint8_t major = item->last_execution_major; - if (item->last_execution > now) - major++; - if (major != this->millis_major_) - break; + // use scoping to indicate visibility of `item` variable + { + // Don't copy-by value yet + auto &item = this->items_[0]; + if ((now - item->last_execution) < item->interval) + // Not reached timeout yet, done for this call + break; + uint8_t major = item->next_execution_major(); + if (this->millis_major_ - major > 1) + break; - // Don't run on failed components - if (item->component != nullptr && item->component->is_failed()) { - this->pop_raw_(); - delete item; - continue; - } + // Don't run on failed components + if (item->component != nullptr && item->component->is_failed()) { + this->pop_raw_(); + continue; + } #ifdef ESPHOME_LOG_HAS_VERY_VERBOSE - const char *type = item->type == SchedulerItem::INTERVAL ? "interval" : "timeout"; - ESP_LOGVV(TAG, "Running %s '%s' with interval=%u last_execution=%u (now=%u)", type, item->name.c_str(), - item->interval, item->last_execution, now); + const char *type = item->type == SchedulerItem::INTERVAL ? "interval" : "timeout"; + ESP_LOGVV(TAG, "Running %s '%s' with interval=%u last_execution=%u (now=%u)", type, item->name.c_str(), + item->interval, item->last_execution, now); #endif - // Warning: During f(), a lot of stuff can happen, including: - // - timeouts/intervals get added, potentially invalidating vector pointers - // - timeouts/intervals get cancelled - item->f(); - - // Only pop after function call, this ensures we were reachable - // during the function call and know if we were cancelled. - this->pop_raw_(); - - if (item->remove) { - // We were removed/cancelled in the function call, stop - delete item; - continue; + // Warning: During f(), a lot of stuff can happen, including: + // - timeouts/intervals get added, potentially invalidating vector pointers + // - timeouts/intervals get cancelled + item->f(); } - if (item->type == SchedulerItem::INTERVAL) { - if (item->interval != 0) { - const uint32_t before = item->last_execution; - const uint32_t amount = (now - item->last_execution) / item->interval; - item->last_execution += amount * item->interval; - if (item->last_execution < before) - item->last_execution_major++; + { + // 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_(); + + if (item->remove) { + // We were removed/cancelled in the function call, stop + continue; + } + + if (item->type == SchedulerItem::INTERVAL) { + if (item->interval != 0) { + const uint32_t before = item->last_execution; + const uint32_t amount = (now - item->last_execution) / item->interval; + item->last_execution += amount * item->interval; + if (item->last_execution < before) + item->last_execution_major++; + } + this->push_(std::move(item)); } - this->push_(item); - } else { - delete item; } } this->process_to_add(); } void HOT Scheduler::process_to_add() { - for (auto *it : this->to_add_) { + for (auto &it : this->to_add_) { if (it->remove) { - delete it; continue; } - this->items_.push_back(it); + this->items_.push_back(std::move(it)); std::push_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); } this->to_add_.clear(); } void HOT Scheduler::cleanup_() { while (!this->items_.empty()) { - auto item = this->items_[0]; + auto &item = this->items_[0]; if (!item->remove) return; - delete item; this->pop_raw_(); } } @@ -181,15 +189,15 @@ void HOT Scheduler::pop_raw_() { std::pop_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); this->items_.pop_back(); } -void HOT Scheduler::push_(Scheduler::SchedulerItem *item) { this->to_add_.push_back(item); } +void HOT Scheduler::push_(std::unique_ptr item) { this->to_add_.push_back(std::move(item)); } bool HOT Scheduler::cancel_item_(Component *component, const std::string &name, Scheduler::SchedulerItem::Type type) { bool ret = false; - for (auto *it : this->items_) + for (auto &it : this->items_) if (it->component == component && it->name == name && it->type == type) { it->remove = true; ret = true; } - for (auto *it : this->to_add_) + for (auto &it : this->to_add_) if (it->component == component && it->name == name && it->type == type) { it->remove = true; ret = true; @@ -206,21 +214,30 @@ uint32_t Scheduler::millis_() { return now; } -bool HOT Scheduler::SchedulerItem::cmp(Scheduler::SchedulerItem *a, Scheduler::SchedulerItem *b) { +bool HOT Scheduler::SchedulerItem::cmp(const std::unique_ptr &a, + const std::unique_ptr &b) { // min-heap // return true if *a* will happen after *b* - uint32_t a_next_exec = a->last_execution + a->timeout; - uint8_t a_next_exec_major = a->last_execution_major; - if (a_next_exec < a->last_execution) - a_next_exec_major++; - - uint32_t b_next_exec = b->last_execution + b->timeout; - uint8_t b_next_exec_major = b->last_execution_major; - if (b_next_exec < b->last_execution) - b_next_exec_major++; + uint32_t a_next_exec = a->next_execution(); + uint8_t a_next_exec_major = a->next_execution_major(); + uint32_t b_next_exec = b->next_execution(); + uint8_t b_next_exec_major = b->next_execution_major(); if (a_next_exec_major != b_next_exec_major) { - return a_next_exec_major > b_next_exec_major; + // The "major" calculation is quite complicated. + // Basically, we need to check if the major value lies in the future or + // + + // Here are some cases to think about: + // Format: a_major,b_major -> expected result (a-b, b-a) + // a=255,b=0 -> false (255, 1) + // a=0,b=1 -> false (255, 1) + // a=1,b=0 -> true (1, 255) + // a=0,b=255 -> true (1, 255) + + uint8_t diff1 = a_next_exec_major - b_next_exec_major; + uint8_t diff2 = b_next_exec_major - a_next_exec_major; + return diff1 < diff2; } return a_next_exec > b_next_exec; diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index 1faadfabd0..5688058a1e 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -2,6 +2,7 @@ #include "esphome/core/component.h" #include +#include namespace esphome { @@ -34,21 +35,30 @@ class Scheduler { bool remove; uint8_t last_execution_major; - static bool cmp(SchedulerItem *a, SchedulerItem *b); + inline uint32_t next_execution() { return this->last_execution + this->timeout; } + inline uint8_t next_execution_major() { + uint32_t next_exec = this->next_execution(); + uint8_t next_exec_major = this->last_execution_major; + if (next_exec < this->last_execution) + next_exec_major++; + return next_exec_major; + } + + static bool cmp(const std::unique_ptr &a, const std::unique_ptr &b); }; uint32_t millis_(); void cleanup_(); void pop_raw_(); - void push_(SchedulerItem *item); + void push_(std::unique_ptr item); bool cancel_item_(Component *component, const std::string &name, SchedulerItem::Type type); bool empty_() { this->cleanup_(); return this->items_.empty(); } - std::vector items_; - std::vector to_add_; + std::vector> items_; + std::vector> to_add_; uint32_t last_millis_{0}; uint8_t millis_major_{0}; };