Merge branch 'dev' into while-loop-in-place

The stack probe reads the frame address, which AddressSanitizer does not
move to a fake stack, and the test includes hal.h for delay().

esphome/core/base_automation.h

@@ -11,6 +11,7 @@
 
 #include <array>
 #include <list>
+#include <memory>
 #include <tuple>
 #include <type_traits>
 #include <vector>
@@ -360,6 +361,16 @@ template<bool HasElse, typename... Ts> class IfAction : public Action<Ts...> {
   [[no_unique_address]] std::conditional_t<HasElse, ActionList<Ts...>, NoElse> else_;
 };
 
+/// The while body running synchronously, on the stack of WhileAction::loop_(); its play() marks it ended.
+/// One for all loops: automations run on the main loop, one body at a time.
+namespace internal {
+struct WhileBody {
+  const void *owner;
+  bool ended;
+};
+inline WhileBody *active_while_body = nullptr;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
+}  // namespace internal
+
 template<typename... Ts> class WhileAction : public Action<Ts...> {
  public:
   WhileAction(Condition<Ts...> *condition) : condition_(condition) {}
@@ -391,14 +402,21 @@ template<typename... Ts> class WhileAction : public Action<Ts...> {
       this->play_next_(x...);
       return;
     }
-    this->then_.play(x...);
+    this->loop_(x...);
   }
 
   // Branch-end resume hook: loop again or continue
   void play(const Ts &...x) override {
-    // The condition may stop this automation
-    if (this->num_running_ > 0 && this->condition_->check(x...) && this->num_running_ > 0) {
-      this->then_.play(x...);
+    // A body that ended without waiting is looped by loop_(); mark it ended. A second end in one pass loops on
+    // its own.
+    internal::WhileBody *body = internal::active_while_body;
+    if (body != nullptr && !body->ended && body->owner == this &&
+        static_cast<Frame *>(body)->keys == Keys{std::addressof(x)...}) {
+      body->ended = true;
+      return;
+    }
+    if (this->continues_(x...)) {
+      this->loop_(x...);
     } else {
       this->play_next_(x...);
     }
@@ -407,6 +425,34 @@ template<typename... Ts> class WhileAction : public Action<Ts...> {
   void stop() override { this->then_.stop(); }
 
  protected:
+  // The argument storage, which tells a run from another run of this action
+  using Keys = std::array<const void *, sizeof...(Ts)>;
+  struct Frame : internal::WhileBody {
+    Keys keys;
+  };
+
+  bool continues_(const Ts &...x) {
+    // The condition may stop this automation
+    return this->num_running_ > 0 && this->condition_->check(x...) && this->num_running_ > 0;
+  }
+
+  // Plays the body again in place, not by recursion, while it ends without waiting
+  void loop_(const Ts &...x) {
+    Frame frame{{this, false}, {std::addressof(x)...}};
+    do {
+      // Saved and restored, as the body can start this action again
+      frame.ended = false;
+      internal::WhileBody *outer = internal::active_while_body;
+      internal::active_while_body = &frame;
+      this->then_.play(x...);
+      internal::active_while_body = outer;
+      // A body that waits resumes through play() later
+      if (!frame.ended)
+        return;
+    } while (this->continues_(x...));
+    this->play_next_(x...);
+  }
+
   Condition<Ts...> *condition_;
   ActionList<Ts...> then_;
 };

tests/components/core/test_action_branch_owner.cpp

@@ -1,8 +1,12 @@
 #include <gtest/gtest.h>
+#include <algorithm>
+#include <cstdint>
+#include <utility>
 #include <vector>
 
 #include "esphome/core/automation.h"
 #include "esphome/core/base_automation.h"
+#include "esphome/core/hal.h"
 
 namespace esphome::core::testing {
 
@@ -79,10 +83,10 @@ class StopOnCheckCondition : public Condition<> {
 };
 
 // Turns its condition off after a number of checks
-class CountdownCondition : public Condition<> {
+template<typename... Ts> class CountdownCondition : public Condition<Ts...> {
  public:
   explicit CountdownCondition(int passes) : passes_(passes) {}
-  bool check() override { return this->passes_-- > 0; }
+  bool check(const Ts &.../*x*/) override { return this->passes_-- > 0; }
 
  protected:
   int passes_;
@@ -177,7 +181,7 @@ TEST(ActionBranchOwner, PerItemIfAndWhileResumeAfterTheirBranch) {
   EXPECT_EQ(log, (Log{1, 2, 4, 3, 4}));
 
   log.clear();
-  CountdownCondition loop_cond(2);
+  CountdownCondition<> loop_cond(2);
   WhileAction<> loop(&loop_cond);
   DeferredAction wait(&log, 5);
   RecordAction after2(&log, 6);
@@ -341,7 +345,7 @@ TEST(ActionBranchOwner, EmptyBranchesFinishImmediately) {
 
 TEST(ActionBranchOwner, WhileRechecksEachPassAndExits) {
   Log log;
-  CountdownCondition cond(3);
+  CountdownCondition<> cond(3);
   WhileAction<> loop(&cond);
   RecordAction body(&log, 1);
   RecordAction after(&log, 2);
@@ -379,7 +383,7 @@ TEST(ActionBranchOwner, StopMidBranchClearsRunning) {
 
 TEST(ActionBranchOwner, NestedIfInsideWhileResumesEachOwner) {
   Log log;
-  CountdownCondition loop_cond(2);
+  CountdownCondition<> loop_cond(2);
   FlagCondition if_cond;
   WhileAction<> loop(&loop_cond);
   IfAction<false> inner(&if_cond);
@@ -400,4 +404,222 @@ TEST(ActionBranchOwner, NestedIfInsideWhileResumesEachOwner) {
   EXPECT_EQ(top.num_running(), 0);
 }
 
+// Records how deep the stack is on each pass. It works after the chain continues, as real actions do (a
+// destructor, a log), so the compiler cannot turn the recursion into a jump.
+class StackDepthAction : public Action<int> {
+ public:
+  void play_complex(const int &x) override {
+    this->num_running_++;
+    // The frame address, as AddressSanitizer may move locals to a fake stack
+    auto at = reinterpret_cast<uintptr_t>(__builtin_frame_address(0));
+    this->lowest = std::min(this->lowest, at);
+    this->highest = std::max(this->highest, at);
+    this->play_next_(x);
+    this->passes = this->passes + 1;
+  }
+  void play(const int & /*x*/) override {}
+  uintptr_t lowest{UINTPTR_MAX};
+  uintptr_t highest{0};
+  volatile int passes{0};
+};
+
+// Each pass of a body that never waits starts at the same depth, not deeper than the last
+TEST(ActionBranchOwner, WhileBodyThatNeverWaitsKeepsTheStackFlat) {
+  CountdownCondition<int> cond(1000);
+  WhileAction<int> loop(&cond);
+  StackDepthAction body;
+  loop.add_then({&body});
+  ActionList<int> list;
+  list.add_actions({&loop});
+
+  list.play(1);
+  EXPECT_EQ(body.passes, 1000);
+  EXPECT_LT(body.highest - body.lowest, 256U);
+  EXPECT_FALSE(list.is_running());
+}
+
+// Keeps the first argument by reference, like wait_until does for an entity, and copies the second
+class MixedHandoffAction : public Action<const int &, int> {
+ public:
+  void play_complex(const int &entity, const int &value) override {
+    this->num_running_++;
+    this->finish();
+    this->entity_ = &entity;
+    this->value_ = value;
+    this->waiting_ = true;
+  }
+  void play(const int & /*entity*/, const int & /*value*/) override {}
+  void finish() {
+    if (this->waiting_) {
+      this->waiting_ = false;
+      int released = this->value_;
+      this->play_next_(*this->entity_, released);
+    }
+  }
+
+ protected:
+  const int *entity_{nullptr};
+  int value_{0};
+  bool waiting_{false};
+};
+
+class OncePerSecondValueCondition : public Condition<const int &, int> {
+ public:
+  bool check(const int & /*entity*/, const int &x) override { return !std::exchange(this->seen_[x], true); }
+
+ protected:
+  bool seen_[4]{};
+};
+
+class RecordSecondAction : public Action<const int &, int> {
+ public:
+  explicit RecordSecondAction(Log *log) : log_(log) {}
+  void play(const int & /*entity*/, const int &x) override { this->log_->push_back(x); }
+
+ protected:
+  Log *log_;
+};
+
+// The end of another run's body inside this run's body must resume that run, not end this one. Runs on the same
+// entity share its storage; the copied argument still tells them apart.
+TEST(ActionBranchOwner, WhileTellsRunsApartBySharedAndCopiedArguments) {
+  OncePerSecondValueCondition cond;
+  WhileAction<const int &, int> loop(&cond);
+  MixedHandoffAction wait;
+  Log log;
+  RecordSecondAction after(&log);
+  loop.add_then({&wait});
+  ActionList<const int &, int> list;
+  list.add_actions({&loop, &after});
+
+  const int entity = 0;
+  list.play(entity, 1);
+  list.play(entity, 2);
+  EXPECT_EQ(log, (Log{1}));
+  wait.finish();
+  EXPECT_EQ(log, (Log{1, 2}));
+  EXPECT_FALSE(list.is_running());
+}
+
+class CountPassAction : public Action<> {
+ public:
+  void play() override { this->passes++; }
+  int passes{0};
+};
+
+// Without arguments a wait_until timeout of one run can end inside another run's pass. Runs without arguments
+// are alike: whichever frame takes that end loops once for it, so every pass and every run still continues once.
+TEST(ActionBranchOwner, WhileWithoutArgumentsKeepsRunsWhenAWaitTimesOutInside) {
+  CountdownCondition<> cond(3);
+  WhileAction<> loop(&cond);
+  CountPassAction pass;
+  FlagCondition never;
+  never.value = false;
+  WaitUntilAction<> wait(&never);
+  wait.set_timeout_value([]() -> uint32_t { return 10; });
+  Log log;
+  RecordAction after(&log, 1);
+  loop.add_then({&pass, &wait});
+  ActionList<> list;
+  list.add_actions({&loop, &after});
+
+  list.play();  // run A waits
+  delay(15);
+  list.play();  // run B's pass releases A, whose wait timed out, inside it
+  never.value = true;
+  wait.loop();  // the remaining waits end
+  EXPECT_EQ(pass.passes, 3);
+  EXPECT_EQ(log, (Log{1, 1}));
+  EXPECT_FALSE(list.is_running());
+}
+
+// Releases the run that waits here, then lets the current run go on at once: two ends in one pass
+class ReleaseAndPassAction : public Action<> {
+ public:
+  void play_complex() override {
+    this->num_running_++;
+    if (this->waiting_) {
+      this->waiting_ = false;
+      this->play_next_();
+    }
+    if (this->pass) {
+      this->play_next_();
+      return;
+    }
+    this->waiting_ = true;
+  }
+  void play() override {}
+  bool pass{false};
+
+ protected:
+  bool waiting_{false};
+};
+
+// Without arguments both ends of one pass mark the same body; the second must still continue its run
+TEST(ActionBranchOwner, WhileWithoutArgumentsContinuesBothEndsOfOnePass) {
+  CountdownCondition<> cond(2);
+  WhileAction<> loop(&cond);
+  ReleaseAndPassAction wait;
+  Log log;
+  RecordAction after(&log, 1);
+  loop.add_then({&wait});
+  ActionList<> list;
+  list.add_actions({&loop, &after});
+
+  list.play();  // run A waits
+  wait.pass = true;
+  list.play();  // run B's pass releases A and ends itself
+  EXPECT_EQ(log, (Log{1, 1}));
+  EXPECT_FALSE(list.is_running());
+}
+
+// True for `passes` checks, then false once, then again: an inner loop's condition for each outer pass
+class CycleCondition : public Condition<int> {
+ public:
+  explicit CycleCondition(int passes) : passes_(passes) {}
+  bool check(const int & /*x*/) override { return ++this->count_ % (this->passes_ + 1) != 0; }
+
+ protected:
+  int passes_;
+  int count_{0};
+};
+
+// A while inside a while: each restores the other's frame, and neither stack grows
+TEST(ActionBranchOwner, NestedWhileBodiesKeepTheStackFlat) {
+  CountdownCondition<int> outer_cond(3);
+  CycleCondition inner_cond(5);
+  WhileAction<int> outer(&outer_cond);
+  WhileAction<int> inner(&inner_cond);
+  StackDepthAction outer_body, inner_body;
+  inner.add_then({&inner_body});
+  outer.add_then({&outer_body, &inner});
+  ActionList<int> list;
+  list.add_actions({&outer});
+
+  list.play(1);
+  EXPECT_EQ(outer_body.passes, 3);
+  EXPECT_EQ(inner_body.passes, 15);
+  EXPECT_LT(outer_body.highest - outer_body.lowest, 256U);
+  EXPECT_LT(inner_body.highest - inner_body.lowest, 256U);
+  EXPECT_FALSE(list.is_running());
+}
+
+// The second end in one pass keeps looping while the condition holds
+TEST(ActionBranchOwner, WhileWithoutArgumentsLoopsOnFromTheSecondEndOfOnePass) {
+  CountdownCondition<> cond(4);
+  WhileAction<> loop(&cond);
+  ReleaseAndPassAction wait;
+  Log log;
+  RecordAction after(&log, 1);
+  loop.add_then({&wait});
+  ActionList<> list;
+  list.add_actions({&loop, &after});
+
+  list.play();  // run A waits
+  wait.pass = true;
+  list.play();  // run B's pass releases A and ends itself; one of them passes once more
+  EXPECT_EQ(log, (Log{1, 1}));
+  EXPECT_FALSE(list.is_running());
+}
+
 }  // namespace esphome::core::testing