From 5142ff372bb389c3df9d73830930db325272037c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 2 Dec 2025 10:01:54 -0600 Subject: [PATCH] [light] Use listener pattern for state callbacks with lazy allocation (#12166) --- esphome/components/light/automation.h | 62 ++++++----- esphome/components/light/light_call.cpp | 6 +- esphome/components/light/light_state.cpp | 26 +++-- esphome/components/light/light_state.h | 58 +++++++--- esphome/components/mqtt/mqtt_light.cpp | 7 +- esphome/components/mqtt/mqtt_light.h | 5 +- .../fixtures/light_automations.yaml | 26 +++++ tests/integration/test_light_automations.py | 101 ++++++++++++++++++ 8 files changed, 236 insertions(+), 55 deletions(-) create mode 100644 tests/integration/fixtures/light_automations.yaml create mode 100644 tests/integration/test_light_automations.py diff --git a/esphome/components/light/automation.h b/esphome/components/light/automation.h index 9893c15e0..c90d71c5d 100644 --- a/esphome/components/light/automation.h +++ b/esphome/components/light/automation.h @@ -120,46 +120,54 @@ template class LightIsOffCondition : public Condition { LightState *state_; }; -class LightTurnOnTrigger : public Trigger<> { +class LightTurnOnTrigger : public Trigger<>, public LightRemoteValuesListener { public: - LightTurnOnTrigger(LightState *a_light) { - a_light->add_new_remote_values_callback([this, a_light]() { - // using the remote value because of transitions we need to trigger as early as possible - auto is_on = a_light->remote_values.is_on(); - // only trigger when going from off to on - auto should_trigger = is_on && !this->last_on_; - // Set new state immediately so that trigger() doesn't devolve - // into infinite loop - this->last_on_ = is_on; - if (should_trigger) { - this->trigger(); - } - }); + explicit LightTurnOnTrigger(LightState *a_light) : light_(a_light) { + a_light->add_remote_values_listener(this); this->last_on_ = a_light->current_values.is_on(); } + void on_light_remote_values_update() override { + // using the remote value because of transitions we need to trigger as early as possible + auto is_on = this->light_->remote_values.is_on(); + // only trigger when going from off to on + auto should_trigger = is_on && !this->last_on_; + // Set new state immediately so that trigger() doesn't devolve + // into infinite loop + this->last_on_ = is_on; + if (should_trigger) { + this->trigger(); + } + } + protected: + LightState *light_; bool last_on_; }; -class LightTurnOffTrigger : public Trigger<> { +class LightTurnOffTrigger : public Trigger<>, public LightTargetStateReachedListener { public: - LightTurnOffTrigger(LightState *a_light) { - a_light->add_new_target_state_reached_callback([this, a_light]() { - auto is_on = a_light->current_values.is_on(); - // only trigger when going from on to off - if (!is_on) { - this->trigger(); - } - }); + explicit LightTurnOffTrigger(LightState *a_light) : light_(a_light) { + a_light->add_target_state_reached_listener(this); } + + void on_light_target_state_reached() override { + auto is_on = this->light_->current_values.is_on(); + // only trigger when going from on to off + if (!is_on) { + this->trigger(); + } + } + + protected: + LightState *light_; }; -class LightStateTrigger : public Trigger<> { +class LightStateTrigger : public Trigger<>, public LightRemoteValuesListener { public: - LightStateTrigger(LightState *a_light) { - a_light->add_new_remote_values_callback([this]() { this->trigger(); }); - } + explicit LightStateTrigger(LightState *a_light) { a_light->add_remote_values_listener(this); } + + void on_light_remote_values_update() override { this->trigger(); } }; // This is slightly ugly, but we can't log in headers, and can't make this a static method on AddressableSet diff --git a/esphome/components/light/light_call.cpp b/esphome/components/light/light_call.cpp index f523b4451..dca586173 100644 --- a/esphome/components/light/light_call.cpp +++ b/esphome/components/light/light_call.cpp @@ -174,8 +174,10 @@ void LightCall::perform() { this->parent_->set_immediately_(v, publish); } - if (!this->has_transition_()) { - this->parent_->target_state_reached_callback_.call(); + if (!this->has_transition_() && this->parent_->target_state_reached_listeners_) { + for (auto *listener : *this->parent_->target_state_reached_listeners_) { + listener->on_light_target_state_reached(); + } } if (publish) { this->parent_->publish_state(); diff --git a/esphome/components/light/light_state.cpp b/esphome/components/light/light_state.cpp index 9cde9077d..af619a426 100644 --- a/esphome/components/light/light_state.cpp +++ b/esphome/components/light/light_state.cpp @@ -127,7 +127,11 @@ void LightState::loop() { this->transformer_->stop(); this->is_transformer_active_ = false; this->transformer_ = nullptr; - this->target_state_reached_callback_.call(); + if (this->target_state_reached_listeners_) { + for (auto *listener : *this->target_state_reached_listeners_) { + listener->on_light_target_state_reached(); + } + } // Disable loop if idle (no transformer and no effect) this->disable_loop_if_idle_(); @@ -146,7 +150,11 @@ void LightState::loop() { float LightState::get_setup_priority() const { return setup_priority::HARDWARE - 1.0f; } void LightState::publish_state() { - this->remote_values_callback_.call(); + if (this->remote_values_listeners_) { + for (auto *listener : *this->remote_values_listeners_) { + listener->on_light_remote_values_update(); + } + } #if defined(USE_LIGHT) && defined(USE_CONTROLLER_REGISTRY) ControllerRegistry::notify_light_update(this); #endif @@ -171,11 +179,17 @@ StringRef LightState::get_effect_name_ref() { return EFFECT_NONE_REF; } -void LightState::add_new_remote_values_callback(std::function &&send_callback) { - this->remote_values_callback_.add(std::move(send_callback)); +void LightState::add_remote_values_listener(LightRemoteValuesListener *listener) { + if (!this->remote_values_listeners_) { + this->remote_values_listeners_ = make_unique>(); + } + this->remote_values_listeners_->push_back(listener); } -void LightState::add_new_target_state_reached_callback(std::function &&send_callback) { - this->target_state_reached_callback_.add(std::move(send_callback)); +void LightState::add_target_state_reached_listener(LightTargetStateReachedListener *listener) { + if (!this->target_state_reached_listeners_) { + this->target_state_reached_listeners_ = make_unique>(); + } + this->target_state_reached_listeners_->push_back(listener); } void LightState::set_default_transition_length(uint32_t default_transition_length) { diff --git a/esphome/components/light/light_state.h b/esphome/components/light/light_state.h index ad8922b46..7ea72306f 100644 --- a/esphome/components/light/light_state.h +++ b/esphome/components/light/light_state.h @@ -18,6 +18,29 @@ namespace esphome::light { class LightOutput; +class LightState; + +/** Listener interface for light remote value changes. + * + * Components can implement this interface to receive notifications + * when the light's remote values change (state, brightness, color, etc.) + * without the overhead of std::function callbacks. + */ +class LightRemoteValuesListener { + public: + virtual void on_light_remote_values_update() = 0; +}; + +/** Listener interface for light target state reached. + * + * Components can implement this interface to receive notifications + * when the light finishes a transition and reaches its target state + * without the overhead of std::function callbacks. + */ +class LightTargetStateReachedListener { + public: + virtual void on_light_target_state_reached() = 0; +}; enum LightRestoreMode : uint8_t { LIGHT_RESTORE_DEFAULT_OFF, @@ -121,21 +144,17 @@ class LightState : public EntityBase, public Component { /// Return the name of the current effect as StringRef (for API usage) StringRef get_effect_name_ref(); - /** - * This lets front-end components subscribe to light change events. This callback is called once - * when the remote color values are changed. - * - * @param send_callback The callback. + /** Add a listener for remote values changes. + * Listener is notified when the light's remote values change (state, brightness, color, etc.) + * Lazily allocates the listener vector on first registration. */ - void add_new_remote_values_callback(std::function &&send_callback); + void add_remote_values_listener(LightRemoteValuesListener *listener); - /** - * The callback is called once the state of current_values and remote_values are equal (when the - * transition is finished). - * - * @param send_callback + /** Add a listener for target state reached. + * Listener is notified when the light finishes a transition and reaches its target state. + * Lazily allocates the listener vector on first registration. */ - void add_new_target_state_reached_callback(std::function &&send_callback); + void add_target_state_reached_listener(LightTargetStateReachedListener *listener); /// Set the default transition length, i.e. the transition length when no transition is provided. void set_default_transition_length(uint32_t default_transition_length); @@ -279,19 +298,24 @@ class LightState : public EntityBase, public Component { // for effects, true if a transformer (transition) is active. bool is_transformer_active_ = false; - /** Callback to call when new values for the frontend are available. + /** Listeners for remote values changes. * * "Remote values" are light color values that are reported to the frontend and have a lower * publish frequency than the "real" color values. For example, during transitions the current * color value may change continuously, but the remote values will be reported as the target values * starting with the beginning of the transition. + * + * Lazily allocated - only created when a listener is actually registered. */ - CallbackManager remote_values_callback_{}; + std::unique_ptr> remote_values_listeners_; - /** Callback to call when the state of current_values and remote_values are equal - * This should be called once the state of current_values changed and equals the state of remote_values + /** Listeners for target state reached. + * Notified when the state of current_values and remote_values are equal + * (when the transition is finished). + * + * Lazily allocated - only created when a listener is actually registered. */ - CallbackManager target_state_reached_callback_{}; + std::unique_ptr> target_state_reached_listeners_; /// Initial state of the light. optional initial_state_{}; diff --git a/esphome/components/mqtt/mqtt_light.cpp b/esphome/components/mqtt/mqtt_light.cpp index 883b67ffc..fe911bfba 100644 --- a/esphome/components/mqtt/mqtt_light.cpp +++ b/esphome/components/mqtt/mqtt_light.cpp @@ -25,8 +25,11 @@ void MQTTJSONLightComponent::setup() { call.perform(); }); - auto f = std::bind(&MQTTJSONLightComponent::publish_state_, this); - this->state_->add_new_remote_values_callback([this, f]() { this->defer("send", f); }); + this->state_->add_remote_values_listener(this); +} + +void MQTTJSONLightComponent::on_light_remote_values_update() { + this->defer("send", [this]() { this->publish_state_(); }); } MQTTJSONLightComponent::MQTTJSONLightComponent(LightState *state) : state_(state) {} diff --git a/esphome/components/mqtt/mqtt_light.h b/esphome/components/mqtt/mqtt_light.h index 3d1e770d4..a105f3d7b 100644 --- a/esphome/components/mqtt/mqtt_light.h +++ b/esphome/components/mqtt/mqtt_light.h @@ -11,7 +11,7 @@ namespace esphome { namespace mqtt { -class MQTTJSONLightComponent : public mqtt::MQTTComponent { +class MQTTJSONLightComponent : public mqtt::MQTTComponent, public light::LightRemoteValuesListener { public: explicit MQTTJSONLightComponent(light::LightState *state); @@ -25,6 +25,9 @@ class MQTTJSONLightComponent : public mqtt::MQTTComponent { bool send_initial_state() override; + // LightRemoteValuesListener interface + void on_light_remote_values_update() override; + protected: std::string component_type() const override; const EntityBase *get_entity() const override; diff --git a/tests/integration/fixtures/light_automations.yaml b/tests/integration/fixtures/light_automations.yaml new file mode 100644 index 000000000..b5b88d95e --- /dev/null +++ b/tests/integration/fixtures/light_automations.yaml @@ -0,0 +1,26 @@ +esphome: + name: light-automations-test + +host: +api: # Port will be automatically injected +logger: + level: DEBUG + +output: + - platform: template + id: test_output + type: binary + write_action: + - lambda: "" + +light: + - platform: binary + id: test_light + name: "Test Light" + output: test_output + on_turn_on: + - logger.log: "TRIGGER: on_turn_on fired" + on_turn_off: + - logger.log: "TRIGGER: on_turn_off fired" + on_state: + - logger.log: "TRIGGER: on_state fired" diff --git a/tests/integration/test_light_automations.py b/tests/integration/test_light_automations.py new file mode 100644 index 000000000..9ff334548 --- /dev/null +++ b/tests/integration/test_light_automations.py @@ -0,0 +1,101 @@ +"""Integration test for light automation triggers. + +Tests that on_turn_on, on_turn_off, and on_state triggers work correctly +with the listener interface pattern. +""" + +import asyncio + +import pytest + +from .types import APIClientConnectedFactory, RunCompiledFunction + + +@pytest.mark.asyncio +async def test_light_automations( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test light on_turn_on, on_turn_off, and on_state triggers.""" + loop = asyncio.get_running_loop() + + # Futures for log line detection + on_turn_on_future: asyncio.Future[bool] = loop.create_future() + on_turn_off_future: asyncio.Future[bool] = loop.create_future() + on_state_count = 0 + counting_enabled = False + on_state_futures: list[asyncio.Future[bool]] = [] + + def create_on_state_future() -> asyncio.Future[bool]: + """Create a new future for on_state trigger.""" + future: asyncio.Future[bool] = loop.create_future() + on_state_futures.append(future) + return future + + def check_output(line: str) -> None: + """Check log output for trigger messages.""" + nonlocal on_state_count + if "TRIGGER: on_turn_on fired" in line: + if not on_turn_on_future.done(): + on_turn_on_future.set_result(True) + elif "TRIGGER: on_turn_off fired" in line: + if not on_turn_off_future.done(): + on_turn_off_future.set_result(True) + elif "TRIGGER: on_state fired" in line: + # Only count on_state after we start testing + if counting_enabled: + on_state_count += 1 + # Complete any pending on_state futures + for future in on_state_futures: + if not future.done(): + future.set_result(True) + break + + async with ( + run_compiled(yaml_config, line_callback=check_output), + api_client_connected() as client, + ): + # Get entities + entities = await client.list_entities_services() + light = next(e for e in entities[0] if e.object_id == "test_light") + + # Start counting on_state events now + counting_enabled = True + + # Test 1: Turn light on - should trigger on_turn_on and on_state + on_state_future_1 = create_on_state_future() + client.light_command(key=light.key, state=True) + + # Wait for on_turn_on trigger + try: + await asyncio.wait_for(on_turn_on_future, timeout=5.0) + except TimeoutError: + pytest.fail("on_turn_on trigger did not fire") + + # Wait for on_state trigger + try: + await asyncio.wait_for(on_state_future_1, timeout=5.0) + except TimeoutError: + pytest.fail("on_state trigger did not fire after turn on") + + # Test 2: Turn light off - should trigger on_turn_off and on_state + on_state_future_2 = create_on_state_future() + client.light_command(key=light.key, state=False) + + # Wait for on_turn_off trigger + try: + await asyncio.wait_for(on_turn_off_future, timeout=5.0) + except TimeoutError: + pytest.fail("on_turn_off trigger did not fire") + + # Wait for on_state trigger + try: + await asyncio.wait_for(on_state_future_2, timeout=5.0) + except TimeoutError: + pytest.fail("on_state trigger did not fire after turn off") + + # Verify on_state fired exactly twice (once for on, once for off) + assert on_state_count == 2, ( + f"on_state should have triggered exactly twice, got {on_state_count}" + )