diff --git a/esphome/components/script/script.h b/esphome/components/script/script.h index 97b27af498d..f00ad7f4afa 100644 --- a/esphome/components/script/script.h +++ b/esphome/components/script/script.h @@ -306,7 +306,7 @@ template class ScriptWaitAction final : public Actionparam_queue_.empty()) { auto ¶ms = this->param_queue_.front(); - this->play_next_tuple_(params, std::make_index_sequence{}); + std::apply([this](auto &...args) { this->play_next_(args...); }, params); this->param_queue_.pop_front(); } else { // Queue is now empty - disable loop until next play_complex @@ -323,12 +323,8 @@ template class ScriptWaitAction final : public Action void play_next_tuple_(const std::tuple &tuple, std::index_sequence /*unused*/) { - this->play_next_(std::get(tuple)...); - } - C *script_; - std::list> param_queue_; + std::list...>> param_queue_; }; } // namespace esphome::script diff --git a/esphome/core/automation.h b/esphome/core/automation.h index d3f1fe59e59..35c2a17c247 100644 --- a/esphome/core/automation.h +++ b/esphome/core/automation.h @@ -8,6 +8,7 @@ #include "esphome/core/string_ref.h" #include #include +#include #include #include @@ -494,6 +495,13 @@ template class TemplatableValue { }; }; +/// How deferring actions store a trigger argument: a copy, since a referenced object may be gone once the trigger +/// returns, unless the type cannot be copied (e.g. an entity, which outlives the wait). A copyable `Base &` would be +/// stored sliced, so entity-like argument types must not be copyable. +template +using stored_trigger_arg_t = + std::conditional_t>, std::remove_cvref_t, T>; + /** Base class for all automation conditions. * * @tparam Ts The template parameters to pass when executing. diff --git a/esphome/core/base_automation.h b/esphome/core/base_automation.h index 8762cbe7110..69def222da2 100644 --- a/esphome/core/base_automation.h +++ b/esphome/core/base_automation.h @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -500,7 +501,7 @@ template class WaitUntilAction : public Action, public Co // Store for later processing auto now = millis(); auto timeout = this->timeout_value_.optional_value(x...); - this->var_queue_.emplace_back(now, timeout, std::make_tuple(x...)); + this->var_queue_.emplace_back(now, timeout, x...); // Do immediate check with fresh timestamp - don't call loop() synchronously! // Let the event loop call it to avoid reentrancy issues @@ -532,7 +533,13 @@ template class WaitUntilAction : public Action, public Co } protected: - using QueueItem = std::tuple, std::tuple>; + struct QueueItem { + QueueItem(uint32_t start, optional timeout, const Ts &...x) + : start(start), timeout(timeout), args(x...) {} + uint32_t start; + optional timeout; + std::tuple...> args; + }; // Helper: Process queue, triggering completed items and removing them // Returns true if queue still has pending items @@ -546,14 +553,14 @@ template class WaitUntilAction : public Action, public Co std::list pending; while (!queue.empty()) { auto it = queue.begin(); - auto start = std::get(*it); - auto timeout = std::get>(*it); + auto start = it->start; + auto timeout = it->timeout; // Check if timeout has expired auto expired = timeout && (now - start) >= *timeout; // Keep waiting if not expired and condition not met - if (!expired && !this->condition_->check_tuple(std::get>(*it))) { + if (!expired && !std::apply([this](auto &...args) { return this->condition_->check(args...); }, it->args)) { pending.splice(pending.end(), queue, it); continue; } @@ -564,7 +571,7 @@ template class WaitUntilAction : public Action, public Co std::list completed; completed.splice(completed.begin(), queue, it); uint8_t generation = this->stop_generation_; - this->play_next_tuple_(std::get>(completed.front())); + std::apply([this](auto &...args) { this->play_next_(args...); }, completed.front().args); if (generation != this->stop_generation_) { // stop() ran inside the chain - the items still held locally were cancelled pending.clear(); @@ -581,7 +588,7 @@ template class WaitUntilAction : public Action, public Co Condition *condition_; std::list var_queue_{}; - // Bumped by stop() so process_queue_() can detect a stop from inside play_next_tuple_() + // Bumped by stop() so process_queue_() can detect a stop from inside play_next_() uint8_t stop_generation_{0}; }; diff --git a/tests/components/core/test_wait_until_action.cpp b/tests/components/core/test_wait_until_action.cpp new file mode 100644 index 00000000000..56cfa10d513 --- /dev/null +++ b/tests/components/core/test_wait_until_action.cpp @@ -0,0 +1,109 @@ +#include + +#include +#include + +#include "esphome/core/base_automation.h" + +namespace esphome::testing { + +class FlagCondition : public Condition { + public: + bool check(const std::string & /*value*/) override { return this->ready; } + bool ready{false}; +}; + +class RecordAction : public Action { + public: + void play(const std::string &value) override { this->seen = value; } + std::string seen; +}; + +// A trigger argument passed as `const T &` must be copied while waiting: the +// referenced object is gone once the trigger has returned. +TEST(WaitUntilActionTest, KeepsCopyOfReferenceArguments) { + FlagCondition condition; + WaitUntilAction wait(&condition); + RecordAction record; + ActionList actions; + actions.add_actions({&wait, &record}); + + { + auto value = std::make_unique("argument longer than the small string buffer"); + actions.play(*value); + } + EXPECT_TRUE(record.seen.empty()); + + condition.ready = true; + wait.loop(); + EXPECT_EQ(record.seen, "argument longer than the small string buffer"); +} + +class Counter { + public: + int value{0}; +}; + +class CounterCondition : public Condition { + public: + bool check(Counter & /*counter*/) override { return this->ready; } + bool ready{false}; +}; + +class IncrementAction : public Action { + public: + void play(Counter &counter) override { this->seen = ++counter.value; } + int seen{0}; +}; + +// A non-const reference argument used to fail to compile +TEST(WaitUntilActionTest, AcceptsMutableReferenceArguments) { + CounterCondition condition; + WaitUntilAction wait(&condition); + IncrementAction increment; + ActionList actions; + actions.add_actions({&wait, &increment}); + + Counter counter; + actions.play(counter); + condition.ready = true; + wait.loop(); + EXPECT_EQ(increment.seen, 1); + // Like delay, the actions after the wait work on a copy + EXPECT_EQ(counter.value, 0); +} + +class Entity { + public: + Entity() = default; + Entity(const Entity &) = delete; + int value{0}; +}; + +class EntityCondition : public Condition { + public: + bool check(Entity & /*entity*/) override { return this->ready; } + bool ready{false}; +}; + +class EntityIncrementAction : public Action { + public: + void play(Entity &entity) override { entity.value++; } +}; + +// A type that cannot be copied, such as an entity, is kept by reference +TEST(WaitUntilActionTest, KeepsReferenceToNonCopyableArguments) { + EntityCondition condition; + WaitUntilAction wait(&condition); + EntityIncrementAction increment; + ActionList actions; + actions.add_actions({&wait, &increment}); + + Entity entity; + actions.play(entity); + condition.ready = true; + wait.loop(); + EXPECT_EQ(entity.value, 1); +} + +} // namespace esphome::testing diff --git a/tests/components/script/test_script_wait_action.cpp b/tests/components/script/test_script_wait_action.cpp new file mode 100644 index 00000000000..17f553996ea --- /dev/null +++ b/tests/components/script/test_script_wait_action.cpp @@ -0,0 +1,96 @@ +#include + +#include +#include + +#include "esphome/components/script/script.h" + +namespace esphome::script::testing { + +class FakeScript { + public: + bool is_running() const { return this->running; } + bool running{true}; +}; + +class RecordAction : public Action { + public: + void play(const std::string &value) override { this->seen = value; } + std::string seen; +}; + +// A trigger argument passed as `const T &` must be copied while waiting: the +// referenced object is gone once the trigger has returned. +TEST(ScriptWaitActionTest, KeepsCopyOfReferenceArguments) { + FakeScript script; + ScriptWaitAction wait(&script); + RecordAction record; + ActionList actions; + actions.add_actions({&wait, &record}); + + { + auto value = std::make_unique("argument longer than the small string buffer"); + actions.play(*value); + } + EXPECT_TRUE(record.seen.empty()); + + script.running = false; + wait.loop(); + EXPECT_EQ(record.seen, "argument longer than the small string buffer"); +} + +class Counter { + public: + int value{0}; +}; + +class IncrementAction : public Action { + public: + void play(Counter &counter) override { this->seen = ++counter.value; } + int seen{0}; +}; + +TEST(ScriptWaitActionTest, KeepsCopyOfMutableReferenceArguments) { + FakeScript script; + ScriptWaitAction wait(&script); + IncrementAction increment; + ActionList actions; + actions.add_actions({&wait, &increment}); + + Counter counter; + actions.play(counter); + script.running = false; + wait.loop(); + EXPECT_EQ(increment.seen, 1); + // Like delay, the actions after the wait work on a copy + EXPECT_EQ(counter.value, 0); +} + +class Entity { + public: + Entity() = default; + Entity(const Entity &) = delete; + int value{0}; +}; + +class EntityIncrementAction : public Action { + public: + void play(Entity &entity) override { entity.value++; } +}; + +// A type that cannot be copied, such as an entity, is kept by reference +TEST(ScriptWaitActionTest, KeepsReferenceToNonCopyableArguments) { + FakeScript script; + ScriptWaitAction wait(&script); + EntityIncrementAction increment; + ActionList actions; + actions.add_actions({&wait, &increment}); + + Entity entity; + actions.play(entity); + script.running = false; + wait.loop(); + EXPECT_EQ(entity.value, 1); +} + +} // namespace esphome::script::testing