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

@@ -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<void()> &&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<SchedulerItem>();
   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<SchedulerItem>();
   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<uint32_t> 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,39 +85,43 @@ 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) {
+  if (now - last_print > 2000) {
-  //  std::vector<SchedulerItem *> old_items = this->items_;
+    last_print = now;
-  //  ESP_LOGVV(TAG, "Items: count=%u, now=%u", this->items_.size(), now);
+    std::vector<std::unique_ptr<SchedulerItem>> old_items;
-  //  while (!this->empty_()) {
+    ESP_LOGVV(TAG, "Items: count=%u, now=%u", this->items_.size(), now);
-  //    auto *item = this->items_[0];
+    while (!this->empty_()) {
-  //    const char *type = item->type == SchedulerItem::INTERVAL ? "interval" : "timeout";
+      auto item = std::move(this->items_[0]);
-  //    ESP_LOGVV(TAG, "  %s '%s' interval=%u last_execution=%u (%u) next=%u",
+      const char *type = item->type == SchedulerItem::INTERVAL ? "interval" : "timeout";
-  //             type, item->name.c_str(), item->interval, item->last_execution, item->last_execution_major,
+      ESP_LOGVV(TAG, "  %s '%s' interval=%u last_execution=%u (%u) next=%u (%u)", type, item->name.c_str(),
-  //             item->last_execution + item->interval);
+                item->interval, item->last_execution, item->last_execution_major, item->next_execution(),
-  //    this->pop_raw_();
+                item->next_execution_major());
-  //  }
+
-  //  ESP_LOGVV(TAG, "\n");
+      this->pop_raw_();
-  //  this->items_ = old_items;
+      old_items.push_back(std::move(item));
-  //}
+    }
+    ESP_LOGVV(TAG, "\n");
+    this->items_ = std::move(old_items);
+  }
+#endif  // ESPHOME_DEBUG_SCHEDULER
 
   while (!this->empty_()) {
+    // use scoping to indicate visibility of `item` variable
+    {
       // Don't copy-by value yet
-    auto *item = this->items_[0];
+      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;
+      uint8_t major = item->next_execution_major();
-    if (item->last_execution > now)
+      if (this->millis_major_ - major > 1)
-      major++;
-    if (major != this->millis_major_)
         break;
 
       // Don't run on failed components
       if (item->component != nullptr && item->component->is_failed()) {
         this->pop_raw_();
-      delete item;
         continue;
       }
 
@@ -128,6 +135,11 @@ void ICACHE_RAM_ATTR HOT Scheduler::call() {
       //  - timeouts/intervals get added, potentially invalidating vector pointers
       //  - timeouts/intervals get cancelled
       item->f();
+    }
+
+    {
+      // 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.
@@ -135,7 +147,6 @@ void ICACHE_RAM_ATTR HOT Scheduler::call() {
 
       if (item->remove) {
         // We were removed/cancelled in the function call, stop
-      delete item;
         continue;
       }
 
@@ -147,33 +158,30 @@ void ICACHE_RAM_ATTR HOT Scheduler::call() {
           if (item->last_execution < before)
             item->last_execution_major++;
         }
-      this->push_(item);
+        this->push_(std::move(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<Scheduler::SchedulerItem> 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<SchedulerItem> &a,
+                                       const std::unique_ptr<SchedulerItem> &b) {
   // min-heap
   // return true if *a* will happen after *b*
-  uint32_t a_next_exec = a->last_execution + a->timeout;
+  uint32_t a_next_exec = a->next_execution();
-  uint8_t a_next_exec_major = a->last_execution_major;
+  uint8_t a_next_exec_major = a->next_execution_major();
-  if (a_next_exec < a->last_execution)
+  uint32_t b_next_exec = b->next_execution();
-    a_next_exec_major++;
+  uint8_t b_next_exec_major = b->next_execution_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++;
 
   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;

esphome/core/scheduler.h

@@ -2,6 +2,7 @@
 
 #include "esphome/core/component.h"
 #include <vector>
+#include <memory>
 
 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<SchedulerItem> &a, const std::unique_ptr<SchedulerItem> &b);
   };
 
   uint32_t millis_();
   void cleanup_();
   void pop_raw_();
-  void push_(SchedulerItem *item);
+  void push_(std::unique_ptr<SchedulerItem> item);
   bool cancel_item_(Component *component, const std::string &name, SchedulerItem::Type type);
   bool empty_() {
     this->cleanup_();
     return this->items_.empty();
   }
 
-  std::vector<SchedulerItem *> items_;
+  std::vector<std::unique_ptr<SchedulerItem>> items_;
-  std::vector<SchedulerItem *> to_add_;
+  std::vector<std::unique_ptr<SchedulerItem>> to_add_;
   uint32_t last_millis_{0};
   uint8_t millis_major_{0};
 };