mirror of
https://github.com/esphome/esphome.git
synced 2026-10-12 06:14:17 +00:00
[core] Copy trigger arguments in wait_until and script.wait (#20536)
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
5d75828ceb
commit
7c0f63ea20
@@ -306,7 +306,7 @@ template<class C, typename... Ts> class ScriptWaitAction final : public Action<T
|
||||
// play_next_() can trigger more items to be queued
|
||||
if (!this->param_queue_.empty()) {
|
||||
auto ¶ms = this->param_queue_.front();
|
||||
this->play_next_tuple_(params, std::make_index_sequence<sizeof...(Ts)>{});
|
||||
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 C, typename... Ts> class ScriptWaitAction final : public Action<T
|
||||
}
|
||||
|
||||
protected:
|
||||
template<size_t... S> void play_next_tuple_(const std::tuple<Ts...> &tuple, std::index_sequence<S...> /*unused*/) {
|
||||
this->play_next_(std::get<S>(tuple)...);
|
||||
}
|
||||
|
||||
C *script_;
|
||||
std::list<std::tuple<Ts...>> param_queue_;
|
||||
std::list<std::tuple<stored_trigger_arg_t<Ts>...>> param_queue_;
|
||||
};
|
||||
|
||||
} // namespace esphome::script
|
||||
|
||||
@@ -8,6 +8,7 @@
|
||||
#include "esphome/core/string_ref.h"
|
||||
#include <concepts>
|
||||
#include <functional>
|
||||
#include <type_traits>
|
||||
#include <utility>
|
||||
#include <vector>
|
||||
|
||||
@@ -490,6 +491,13 @@ template<typename... X> class TemplatableValue<std::string, X...> {
|
||||
};
|
||||
};
|
||||
|
||||
/// 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<typename T>
|
||||
using stored_trigger_arg_t =
|
||||
std::conditional_t<std::is_copy_constructible_v<std::remove_cvref_t<T>>, std::remove_cvref_t<T>, T>;
|
||||
|
||||
/** Base class for all automation conditions.
|
||||
*
|
||||
* @tparam Ts The template parameters to pass when executing.
|
||||
|
||||
@@ -11,6 +11,7 @@
|
||||
|
||||
#include <array>
|
||||
#include <list>
|
||||
#include <tuple>
|
||||
#include <type_traits>
|
||||
#include <vector>
|
||||
|
||||
@@ -500,7 +501,7 @@ template<typename... Ts> class WaitUntilAction : public Action<Ts...>, 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<typename... Ts> class WaitUntilAction : public Action<Ts...>, public Co
|
||||
}
|
||||
|
||||
protected:
|
||||
using QueueItem = std::tuple<uint32_t, optional<uint32_t>, std::tuple<Ts...>>;
|
||||
struct QueueItem {
|
||||
QueueItem(uint32_t start, optional<uint32_t> timeout, const Ts &...x)
|
||||
: start(start), timeout(timeout), args(x...) {}
|
||||
uint32_t start;
|
||||
optional<uint32_t> timeout;
|
||||
std::tuple<stored_trigger_arg_t<Ts>...> args;
|
||||
};
|
||||
|
||||
// Helper: Process queue, triggering completed items and removing them
|
||||
// Returns true if queue still has pending items
|
||||
@@ -546,14 +553,14 @@ template<typename... Ts> class WaitUntilAction : public Action<Ts...>, public Co
|
||||
std::list<QueueItem> pending;
|
||||
while (!queue.empty()) {
|
||||
auto it = queue.begin();
|
||||
auto start = std::get<uint32_t>(*it);
|
||||
auto timeout = std::get<optional<uint32_t>>(*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<std::tuple<Ts...>>(*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<typename... Ts> class WaitUntilAction : public Action<Ts...>, public Co
|
||||
std::list<QueueItem> completed;
|
||||
completed.splice(completed.begin(), queue, it);
|
||||
uint8_t generation = this->stop_generation_;
|
||||
this->play_next_tuple_(std::get<std::tuple<Ts...>>(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<typename... Ts> class WaitUntilAction : public Action<Ts...>, public Co
|
||||
|
||||
Condition<Ts...> *condition_;
|
||||
std::list<QueueItem> 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};
|
||||
};
|
||||
|
||||
|
||||
@@ -0,0 +1,109 @@
|
||||
#include <gtest/gtest.h>
|
||||
|
||||
#include <memory>
|
||||
#include <string>
|
||||
|
||||
#include "esphome/core/base_automation.h"
|
||||
|
||||
namespace esphome::testing {
|
||||
|
||||
class FlagCondition : public Condition<const std::string &> {
|
||||
public:
|
||||
bool check(const std::string & /*value*/) override { return this->ready; }
|
||||
bool ready{false};
|
||||
};
|
||||
|
||||
class RecordAction : public Action<const std::string &> {
|
||||
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<const std::string &> wait(&condition);
|
||||
RecordAction record;
|
||||
ActionList<const std::string &> actions;
|
||||
actions.add_actions({&wait, &record});
|
||||
|
||||
{
|
||||
auto value = std::make_unique<std::string>("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<Counter &> {
|
||||
public:
|
||||
bool check(Counter & /*counter*/) override { return this->ready; }
|
||||
bool ready{false};
|
||||
};
|
||||
|
||||
class IncrementAction : public Action<Counter &> {
|
||||
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<Counter &> wait(&condition);
|
||||
IncrementAction increment;
|
||||
ActionList<Counter &> 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<Entity &> {
|
||||
public:
|
||||
bool check(Entity & /*entity*/) override { return this->ready; }
|
||||
bool ready{false};
|
||||
};
|
||||
|
||||
class EntityIncrementAction : public Action<Entity &> {
|
||||
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<Entity &> wait(&condition);
|
||||
EntityIncrementAction increment;
|
||||
ActionList<Entity &> actions;
|
||||
actions.add_actions({&wait, &increment});
|
||||
|
||||
Entity entity;
|
||||
actions.play(entity);
|
||||
condition.ready = true;
|
||||
wait.loop();
|
||||
EXPECT_EQ(entity.value, 1);
|
||||
}
|
||||
|
||||
} // namespace esphome::testing
|
||||
@@ -0,0 +1,96 @@
|
||||
#include <gtest/gtest.h>
|
||||
|
||||
#include <memory>
|
||||
#include <string>
|
||||
|
||||
#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<const std::string &> {
|
||||
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<FakeScript, const std::string &> wait(&script);
|
||||
RecordAction record;
|
||||
ActionList<const std::string &> actions;
|
||||
actions.add_actions({&wait, &record});
|
||||
|
||||
{
|
||||
auto value = std::make_unique<std::string>("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<Counter &> {
|
||||
public:
|
||||
void play(Counter &counter) override { this->seen = ++counter.value; }
|
||||
int seen{0};
|
||||
};
|
||||
|
||||
TEST(ScriptWaitActionTest, KeepsCopyOfMutableReferenceArguments) {
|
||||
FakeScript script;
|
||||
ScriptWaitAction<FakeScript, Counter &> wait(&script);
|
||||
IncrementAction increment;
|
||||
ActionList<Counter &> 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<Entity &> {
|
||||
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<FakeScript, Entity &> wait(&script);
|
||||
EntityIncrementAction increment;
|
||||
ActionList<Entity &> 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
|
||||
Reference in New Issue
Block a user