diff --git a/esphome/components/homeassistant/light/homeassistant_light.cpp b/esphome/components/homeassistant/light/homeassistant_light.cpp index 6ebe0cadbfb..21823bb9097 100644 --- a/esphome/components/homeassistant/light/homeassistant_light.cpp +++ b/esphome/components/homeassistant/light/homeassistant_light.cpp @@ -131,9 +131,7 @@ void HomeassistantLight::setup() { ESP_LOGW(TAG, "'%s': Can't parse '%s' as state!", this->entity_id_, value.c_str()); return; } - const bool state = on_off == PARSE_ON; - this->on_report_(this->imported_.state != state); - this->imported_.state = state; + this->import_(this->imported_.state, optional(on_off == PARSE_ON)); }); server->subscribe_home_assistant_state(this->entity_id_, "color_mode", [this](StringRef value) { if (is_unset(value)) { @@ -144,19 +142,16 @@ void HomeassistantLight::setup() { ESP_LOGE(TAG, "'%s': Can't parse 'color_mode' value '%s' as color mode!", this->entity_id_, value.c_str()); return; } - this->on_report_(this->imported_.color_mode != *mode); - this->imported_.color_mode = *mode; + this->import_(this->imported_.color_mode, *mode); }); server->subscribe_home_assistant_state(this->entity_id_, "brightness", [this](StringRef value) { if (auto brightness = this->parse_integer_("brightness", value, 255)) { - this->on_report_(this->imported_.brightness != *brightness); - this->imported_.brightness = *brightness; + this->import_(this->imported_.brightness, static_cast(*brightness)); } }); server->subscribe_home_assistant_state(this->entity_id_, "color_temp_kelvin", [this](StringRef value) { if (auto kelvin = this->parse_integer_("color_temp_kelvin", value, UINT16_MAX)) { - this->on_report_(this->imported_.color_temp_kelvin != *kelvin); - this->imported_.color_temp_kelvin = *kelvin; + this->import_(this->imported_.color_temp_kelvin, *kelvin); } }); server->subscribe_home_assistant_state(this->entity_id_, "rgb_color", [this](StringRef value) { @@ -202,11 +197,19 @@ optional HomeassistantLight::parse_integer_(const char *name, StringRe void HomeassistantLight::on_report_(bool changed) { // A report that changed an attribute answers the change that armed the echo timeout; a repeated // value (or a trait) does not, so a refused change still gets its warning - if (changed && this->echo_pending_) { - this->echo_pending_ = false; - this->cancel_timeout(ECHO_TIMEOUT_ID); + if (changed) { + if (this->echo_pending_) { + this->echo_pending_ = false; + this->cancel_timeout(ECHO_TIMEOUT_ID); + } + // Noted as the report arrives, not when the deferred apply runs: a call read from the same loop + // pass before this report is older than it, one read after it is newer and must not pass for + // the transition the report is held for + this->report_in_transition_ = this->state_->is_transitioning(); + if (this->report_in_transition_) { + this->held_hash_ = state_hash(this->state_->remote_values); + } } - this->report_changed_ |= changed; this->schedule_apply_(); } @@ -255,9 +258,8 @@ void HomeassistantLight::apply_imported_() { if (!this->state_->set_external_state(values)) { // A report that changed something during a transition or flash is applied once it ends, in // place of the values it ends at; a trait or a repeated report has nothing newer to say - if (this->state_->is_transitioning() && this->report_changed_) { + if (this->report_in_transition_) { this->report_held_ = true; - this->held_hash_ = state_hash(this->state_->remote_values); return; } // A local change still waits for LightState::loop() to send it. Home Assistant reports again @@ -266,7 +268,7 @@ void HomeassistantLight::apply_imported_() { ESP_LOGD(TAG, "'%s': Report dropped, a local change is on its way", this->entity_id_); return; } - this->report_changed_ = false; + this->report_in_transition_ = false; ESP_LOGV(TAG, "'%s': Took over the state reported by Home Assistant", this->entity_id_); } @@ -289,15 +291,17 @@ void HomeassistantLight::update_state(LightState *state) { } this->initial_seen_ = true; } - // A call without a transition replaces whatever ran, so a report held for it is older than the call + // A call without a transition replaces whatever ran, so a report held for it, or still waiting to + // be applied, is older than the call if (!state->is_transitioning()) { this->report_held_ = false; + this->report_in_transition_ = false; } } bool HomeassistantLight::is_imported_(const Attributes &outgoing) const { const Attributes &imported = this->imported_; - if (!imported.state.has_value() || !outgoing.state.has_value() || *imported.state != *outgoing.state) { + if (!outgoing.state.has_value() || imported.state != outgoing.state) { return false; } if (!*outgoing.state) { @@ -346,7 +350,7 @@ void HomeassistantLight::write_state(LightState *state) { const bool transitioned = this->transitioned_; this->report_held_ = false; this->transitioned_ = false; - this->report_changed_ = false; // this change is newer than any report so far, held or dropped + this->report_in_transition_ = false; // this change is newer than any report so far, held or dropped // Home Assistant changed meanwhile; its report is newer than what the transition ends at or // what the flash goes back to if (held) { @@ -430,10 +434,10 @@ void HomeassistantLight::write_state(LightState *state) { entity_id.key = ProgmemStringRef::from_lit(ENTITY_ID_KEY); entity_id.value = ProgmemStringRef(StringRef(this->entity_id_)); - auto add_number = [&](ProgmemStringRef key, auto &buf, unsigned number) { + auto add_number = [&](ProgmemStringRef key, char *buf, size_t size, unsigned number) { auto &entry = resp.data.emplace_back(); entry.key = key; - entry.value = ProgmemStringRef(StringRef(buf, buf_append_printf(buf, sizeof(buf), 0, "%u", number))); + entry.value = ProgmemStringRef(StringRef(buf, buf_append_printf(buf, size, 0, "%u", number))); }; if (!*outgoing.state) { resp.service = ProgmemStringRef::from_lit(SERVICE_TURN_OFF); @@ -443,7 +447,7 @@ void HomeassistantLight::write_state(LightState *state) { // The level goes under "white" in that mode, which also selects the mode add_number(outgoing.color_mode == ColorMode::WHITE ? ProgmemStringRef::from_lit(WHITE_KEY) : ProgmemStringRef::from_lit(BRIGHTNESS_KEY), - level_buf, outgoing.brightness); + level_buf, sizeof(level_buf), outgoing.brightness); } // Home Assistant converts the color to the light's own channels, which loses the white balance // of a cold/warm white light, so the color goes only when it or the mode changed (or may have, @@ -451,7 +455,8 @@ void HomeassistantLight::write_state(LightState *state) { const Attributes &imported = this->imported_; const bool new_color = this->echo_pending_ || outgoing.color_mode != imported.color_mode; if (outgoing.color_temp_kelvin != 0 && (new_color || outgoing.color_temp_kelvin != imported.color_temp_kelvin)) { - add_number(ProgmemStringRef::from_lit(COLOR_TEMP_KELVIN_KEY), color_temp_buf, outgoing.color_temp_kelvin); + add_number(ProgmemStringRef::from_lit(COLOR_TEMP_KELVIN_KEY), color_temp_buf, sizeof(color_temp_buf), + outgoing.color_temp_kelvin); } if (outgoing.has_rgb && (new_color || !imported.has_rgb || outgoing.rgb != imported.rgb)) { // A list, which only a template renders as one diff --git a/esphome/components/homeassistant/light/homeassistant_light.h b/esphome/components/homeassistant/light/homeassistant_light.h index a7f316a22f8..a90a39f2c0a 100644 --- a/esphome/components/homeassistant/light/homeassistant_light.h +++ b/esphome/components/homeassistant/light/homeassistant_light.h @@ -4,6 +4,7 @@ #include "esphome/components/light/light_output.h" #include "esphome/core/component.h" +#include "esphome/core/helpers.h" #include "esphome/core/string_ref.h" namespace esphome::homeassistant { @@ -42,6 +43,11 @@ class HomeassistantLight final : public light::LightOutput, public Component { optional parse_integer_(const char *name, StringRef value, uint16_t max); /// A state or attribute report: answers a pending echo when it changed something, and schedules the apply void on_report_(bool changed); + /// Takes over a reported attribute, reporting whether it changed + template void import_(T &field, const T &value) { + this->on_report_(field != value); + field = value; + } /// A state at the resolution Home Assistant gets, to see a transition retargeted by a newer call static uint32_t state_hash(const light::LightColorValues &values); /// Schedules apply_imported_() once for the attributes of one update @@ -56,15 +62,15 @@ class HomeassistantLight final : public light::LightOutput, public Component { light::LightState *state_{nullptr}; light::LightTraits traits_{}; Attributes imported_{}; - uint32_t held_hash_{0}; // state_hash() of what the transition a report was held for ends at bool apply_scheduled_ : 1 {false}; - bool initial_write_ : 1 {true}; // the light core's own first write is not a local change - bool initial_seen_ : 1 {false}; // the light core's setup call has set the initial state - bool echo_pending_ : 1 {false}; // a sent change waits for Home Assistant to report it back - bool report_held_ : 1 {false}; // a report arrived during a transition or flash; applied when it ends - bool transitioned_ : 1 {false}; // a transition or flash ran since the last write sent - bool report_changed_ : 1 {false}; // the reports since the last apply changed an attribute + bool initial_write_ : 1 {true}; // the light core's own first write is not a local change + bool initial_seen_ : 1 {false}; // the light core's setup call has set the initial state + bool echo_pending_ : 1 {false}; // a sent change waits for Home Assistant to report it back + bool report_held_ : 1 {false}; // a report arrived during a transition or flash; applied when it ends + bool transitioned_ : 1 {false}; // a transition or flash ran since the last write sent + bool report_in_transition_ : 1 {false}; // the last changed report, not yet applied, arrived during a transition uint8_t report_timeout_s_{10}; + uint32_t held_hash_{0}; // state_hash() of what the transition a report was held for ends at }; } // namespace esphome::homeassistant diff --git a/tests/integration/test_api_homeassistant_light.py b/tests/integration/test_api_homeassistant_light.py index 2cb9b4d6d05..93eefb8dbb4 100644 --- a/tests/integration/test_api_homeassistant_light.py +++ b/tests/integration/test_api_homeassistant_light.py @@ -12,12 +12,16 @@ from aioesphomeapi import ( EntityState, HomeassistantServiceCall, LightColorCapability, + LightInfo, LightState, UserService, ) +from aioesphomeapi.api_pb2 import HomeAssistantStateResponse, LightCommandRequest +from google.protobuf import message import pytest from .log_utils import LineWaiter +from .state_utils import require_entity from .types import APIClientConnectedFactory, RunCompiledFunction ENTITY = "light.kitchen" @@ -43,6 +47,19 @@ RGB = ( ) +def command_msg(key: int, **fields: float | bool) -> LightCommandRequest: + """A light command with has_ set for each field given.""" + return LightCommandRequest( + key=key, **fields, **{f"has_{name}": True for name in fields} + ) + + +def report_msg(attribute: str, value: str) -> HomeAssistantStateResponse: + return HomeAssistantStateResponse( + entity_id=ENTITY, attribute=attribute, state=value + ) + + @dataclass class FakeHomeAssistant: """The light entity as the device sees it over the API, and what the device sends back.""" @@ -57,6 +74,19 @@ class FakeHomeAssistant: async def next_call(self) -> HomeassistantServiceCall: return await asyncio.wait_for(self.calls.get(), timeout=5.0) + async def expect_call( + self, service: str = "light.turn_on", **data: str + ) -> HomeassistantServiceCall: + """The next action sent; its data is checked in full when given.""" + call = await self.next_call() + assert call.service == service + if data: + assert call.data == {"entity_id": ENTITY, **data} + return call + + def command(self, **kwargs: float | bool | tuple[float, float, float]) -> None: + self.client.light_command(self.key, **kwargs) + async def next_state(self, matches: Callable[[LightState], bool]) -> LightState: async with asyncio.timeout(5.0): while not matches(state := await self.states.get()): @@ -66,6 +96,14 @@ class FakeHomeAssistant: def report(self, attribute: str | None, value: str) -> None: self.client.send_home_assistant_state(ENTITY, attribute, value) + async def report_off(self) -> None: + self.report(None, "off") + await self.next_state(lambda s: not s.state) + + def send_together(self, *messages: message.Message) -> None: + """One write, so the device reads all of them in one loop pass.""" + self.client._get_connection().send_messages(messages) + @asynccontextmanager async def light_entity( @@ -96,7 +134,7 @@ async def light_entity( api_client_connected() as client, ): entities, services = await client.list_entities_services() - key = next(e.key for e in entities if e.name == "HA Light") + key = require_entity(entities, "ha_light", LightInfo).key client.subscribe_service_calls(calls.put_nowait) client.subscribe_states(on_state) client.subscribe_home_assistant_states(on_state_sub) @@ -163,15 +201,11 @@ async def test_api_homeassistant_light( # Local changes go to Home Assistant with its attribute names and scales; the color # temperature it reported is not repeated - ha.client.light_command(ha.key, brightness=0.5) - call = await ha.next_call() - assert call.service == "light.turn_on" - assert call.data == {"entity_id": ENTITY, "brightness": "128"} + ha.command(brightness=0.5) + call = await ha.expect_call(brightness="128") assert not call.data_template - ha.client.light_command(ha.key, rgb=(0.0, 0.0, 1.0)) - call = await ha.next_call() - assert call.service == "light.turn_on" - assert call.data == {"entity_id": ENTITY, "brightness": "128"} + ha.command(rgb=(0.0, 0.0, 1.0)) + call = await ha.expect_call(brightness="128") assert call.data_template == {"rgb_color": "[0,0,255]"} # Home Assistant echoes the result; that is not sent back. Returning to the state it @@ -182,25 +216,24 @@ async def test_api_homeassistant_light( await ha.waiter.wait_for( "turned off", timeout=5.0 ) # a reported state runs the triggers too - ha.client.light_command(ha.key, state=True) - assert (await ha.next_call()).service == "light.turn_on" - ha.client.light_command(ha.key, state=False) - assert (await ha.next_call()).service == "light.turn_off" + ha.command(state=True) + await ha.expect_call() + ha.command(state=False) + await ha.expect_call("light.turn_off") # Without a report of the change within report_timeout, the light goes back to the reported state ha.report(None, "on") await ha.next_state(lambda s: s.state) - ha.client.light_command(ha.key, state=False) - assert (await ha.next_call()).service == "light.turn_off" + ha.command(state=False) + await ha.expect_call("light.turn_off") await ha.next_state(lambda s: s.state) await ha.waiter.wait_for("did not report the change back", timeout=5.0) - ha.report(None, "off") - await ha.next_state(lambda s: not s.state) + await ha.report_off() # The fallback also drops the levels of the unsent change: the light keeps the brightness # Home Assistant reported last, which a later turn_on without a brightness would send - ha.client.light_command(ha.key, state=True, brightness=0.4) - call = await ha.next_call() + ha.command(state=True, brightness=0.4) + call = await ha.expect_call() assert call.data["brightness"] == "102" state = await ha.next_state(lambda s: not s.state) assert state.brightness == pytest.approx(200 / 255) @@ -208,19 +241,18 @@ async def test_api_homeassistant_light( # An automation changing the light while a report is applied is a local change too await ha.client.execute_service(set_force_off, {"value": True}) ha.report(None, "on") - assert (await ha.next_call()).service == "light.turn_off" + await ha.expect_call("light.turn_off") await ha.client.execute_service(set_force_off, {"value": False}) - ha.report(None, "off") - await ha.next_state(lambda s: not s.state) + await ha.report_off() # A change that leaves Home Assistant as it is gets no report, so none is sent - ha.client.light_command(ha.key, state=False) + ha.command(state=False) await ha.waiter.wait_for("already has this state", timeout=5.0) # A trait that arrives while a change waits for its report is not that report, and does # not bring the old state back early either - ha.client.light_command(ha.key, state=True) - assert (await ha.next_call()).service == "light.turn_on" + ha.command(state=True) + await ha.expect_call() await ha.next_state(lambda s: s.state) ha.report("max_color_temp_kelvin", "6500") await ha.next_state(lambda s: not s.state) @@ -230,7 +262,7 @@ async def test_api_homeassistant_light( ) # A report that arrives during a flash is applied when the flash ends instead of being undone - ha.client.light_command(ha.key, state=True, flash_length=1.0) + ha.command(state=True, flash_length=1.0) ha.report(None, "on") ha.report("brightness", "77") state = await ha.next_state(lambda s: s.brightness == pytest.approx(77 / 255)) @@ -239,84 +271,96 @@ async def test_api_homeassistant_light( # A trait or a repeated report during a local transition is nothing new, so the change the # transition ends at is still sent - ha.client.light_command(ha.key, state=False, transition_length=1.0) + ha.command(state=False, transition_length=1.0) ha.report("max_color_temp_kelvin", "6500") ha.report(None, "on") - assert (await ha.next_call()).service == "light.turn_off" - ha.report(None, "off") - await ha.next_state(lambda s: not s.state) + await ha.expect_call("light.turn_off") + await ha.report_off() # A newer local command wins over a report held during a transition - ha.client.light_command(ha.key, state=True, transition_length=1.0) + ha.command(state=True, transition_length=1.0) ha.report("brightness", "50") await asyncio.sleep(0.1) # held before the command arrives - ha.client.light_command(ha.key, state=True, brightness=1.0) - call = await ha.next_call() - assert call.service == "light.turn_on" + ha.command(state=True, brightness=1.0) + call = await ha.expect_call() assert call.data["brightness"] == "255" ha.report(None, "on") ha.report("brightness", "255") await ha.next_state(lambda s: s.state and s.brightness == 1.0) # Also when it changes only the color, at the level the transition was heading for - ha.client.light_command(ha.key, brightness=0.5, transition_length=1.0) + ha.command(brightness=0.5, transition_length=1.0) ha.report("brightness", "50") await asyncio.sleep(0.1) - ha.client.light_command(ha.key, color_temperature=400.0) - call = await ha.next_call() - assert call.service == "light.turn_on" - assert call.data == { - "entity_id": ENTITY, - "brightness": "128", - "color_temp_kelvin": "2500", - } + ha.command(color_temperature=400.0) + await ha.expect_call(brightness="128", color_temp_kelvin="2500") ha.report("brightness", "128") ha.report("color_temp_kelvin", "2500") await ha.next_state(lambda s: s.color_temperature == 400.0) # Or when it is a transition itself, replacing the one the report was held for - ha.client.light_command(ha.key, brightness=1.0, transition_length=1.0) + ha.command(brightness=1.0, transition_length=1.0) ha.report("brightness", "50") await asyncio.sleep(0.1) - ha.client.light_command(ha.key, color_temperature=250.0, transition_length=1.0) - call = await ha.next_call() - assert call.service == "light.turn_on" - assert call.data == { - "entity_id": ENTITY, - "brightness": "255", - "color_temp_kelvin": "4000", - } + ha.command(color_temperature=250.0, transition_length=1.0) + await ha.expect_call(brightness="255", color_temp_kelvin="4000") ha.report("brightness", "255") ha.report("color_temp_kelvin", "4000") await ha.next_state(lambda s: s.color_temperature == 250.0) # Or a transition back to where the one the report was held for started - ha.client.light_command(ha.key, brightness=0.5, transition_length=1.0) + ha.command(brightness=0.5, transition_length=1.0) ha.report("brightness", "50") await asyncio.sleep(0.1) - ha.client.light_command(ha.key, brightness=1.0, transition_length=1.0) - call = await ha.next_call() - assert call.service == "light.turn_on" + ha.command(brightness=1.0, transition_length=1.0) + call = await ha.expect_call() assert call.data["brightness"] == "255" ha.report("brightness", "255") await ha.next_state(lambda s: s.brightness == 1.0) # And when it goes back to the state the transition started from - ha.client.light_command(ha.key, state=False) - assert (await ha.next_call()).service == "light.turn_off" - ha.report(None, "off") - await ha.next_state(lambda s: not s.state) - ha.client.light_command(ha.key, state=True, transition_length=1.0) + ha.command(state=False) + await ha.expect_call("light.turn_off") + await ha.report_off() + ha.command(state=True, transition_length=1.0) ha.report(None, "on") ha.report("brightness", "60") await asyncio.sleep(0.1) - ha.client.light_command(ha.key, state=False) - assert (await ha.next_call()).service == "light.turn_off" - ha.report(None, "off") - await ha.next_state(lambda s: not s.state) + ha.command(state=False) + await ha.expect_call("light.turn_off") + await ha.report_off() + + # Also when the report and the call that replaces the transition are read in the same loop + # pass: the report is applied after the call, but it is older + ha.send_together( + command_msg(ha.key, state=True, brightness=0.5, transition_length=1000), + report_msg("brightness", "50"), + command_msg(ha.key, brightness=1.0, transition_length=1000), + ) + call = await ha.expect_call() + assert call.data["brightness"] == "255" + ha.report(None, "on") + ha.report("brightness", "255") + await ha.next_state(lambda s: s.state and s.brightness == 1.0) + + # And a report read after that call in the same pass is newer than it: the hold follows the + # last changed report, even when a trait opened the batch, so it is applied at the end + ha.send_together( + report_msg("max_color_temp_kelvin", "6500"), + command_msg(ha.key, brightness=0.5, transition_length=1000), + report_msg("brightness", "50"), + command_msg(ha.key, brightness=0.8, transition_length=1000), + report_msg("brightness", "77"), + ) + state = await ha.next_state(lambda s: s.brightness == pytest.approx(77 / 255)) + assert state.state + assert ha.calls.empty() + ha.command(state=False) + await ha.expect_call("light.turn_off") + await ha.report_off() # A flash ends where it began, which Home Assistant has, so nothing is sent and the user is told - ha.client.light_command(ha.key, state=True, flash_length=0.5) + ha.command(state=True, flash_length=0.5) await ha.waiter.wait_for("nothing to send (a flash never is)", timeout=5.0) assert ha.calls.empty() @@ -347,45 +391,35 @@ async def test_api_homeassistant_light_rgbww( # A level change sends the level alone; Home Assistant derived the color it reports from # the white balance and would derive a different balance from the color sent back - ha.client.light_command(ha.key, brightness=0.5) - call = await ha.next_call() - assert call.service == "light.turn_on" - assert call.data == {"entity_id": ENTITY, "brightness": "128"} + ha.command(brightness=0.5) + call = await ha.expect_call(brightness="128") assert not call.data_template ha.report("brightness", "128") await ha.next_state(lambda s: s.brightness == pytest.approx(128 / 255)) # A new color goes along - ha.client.light_command(ha.key, rgb=(0.0, 1.0, 0.0)) - call = await ha.next_call() - assert call.data == {"entity_id": ENTITY, "brightness": "128"} + ha.command(rgb=(0.0, 1.0, 0.0)) + call = await ha.expect_call(brightness="128") assert call.data_template == {"rgb_color": "[0,255,0]"} ha.report("rgb_color", "(0, 255, 0)") await ha.next_state(lambda s: s.green == 1.0 and s.red == 0.0) # Switching to color temperature sends it, and the level alone once there - ha.client.light_command(ha.key, color_temperature=250.0) - call = await ha.next_call() - assert call.data == { - "entity_id": ENTITY, - "brightness": "128", - "color_temp_kelvin": "4000", - } + ha.command(color_temperature=250.0) + call = await ha.expect_call(brightness="128", color_temp_kelvin="4000") assert not call.data_template ha.report("color_mode", "color_temp") ha.report("color_temp_kelvin", "4000") state = await ha.next_state(lambda s: s.color_mode == COLOR_TEMPERATURE) assert state.color_temperature == 250.0 - ha.client.light_command(ha.key, brightness=1.0) - call = await ha.next_call() - assert call.data == {"entity_id": ENTITY, "brightness": "255"} + ha.command(brightness=1.0) + await ha.expect_call(brightness="255") ha.report("brightness", "255") await ha.next_state(lambda s: s.brightness == 1.0) # Back to the color Home Assistant still reports: the mode changed, so the color is sent - ha.client.light_command(ha.key, rgb=(0.0, 1.0, 0.0)) - call = await ha.next_call() - assert call.data == {"entity_id": ENTITY, "brightness": "255"} + ha.command(rgb=(0.0, 1.0, 0.0)) + call = await ha.expect_call(brightness="255") assert call.data_template == {"rgb_color": "[0,255,0]"} ha.report("color_mode", "rgbww") await ha.next_state(lambda s: s.color_mode == RGB)