diff --git a/custom_components/localthings/climate.py b/custom_components/localthings/climate.py index 34b1bc3..79d38af 100644 --- a/custom_components/localthings/climate.py +++ b/custom_components/localthings/climate.py @@ -13,9 +13,10 @@ snapshot via `coordinator.resource(href)` -- the same cross-resource read that Writes go through `coordinator.async_send_command(bound, (kind, value))`: the CLIMATE capability's `write_fn` maps each `(kind, value)` payload to the right -`(path_segs, body)`, and `async_send_command` POSTs to those path_segs (the -bound href is only used for logging), so one descriptor drives writes to power, -mode, temperature and wind resources. +`(path_segs, body)`, and `async_send_command` POSTs to those path_segs and +applies the optimistic value/settle guard to that same href -- not the bound +`/mode/vs/0` href -- so one descriptor drives writes to, and gets fresh state +back for, power, mode, temperature and wind resources alike. """ from __future__ import annotations diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 83b2d4b..71f1dd4 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -547,6 +547,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): return path_segs, body = result + # The write's actual target, not necessarily bound_entity.href. Most + # descriptors write to the same resource they're bound to, but a + # composite entity -- the AC's ClimateDesc, bound to /mode/vs/0 -- + # drives writes to several sibling resources via path_segs + # (/power/0, /temperature/desired/0, /wind/strength/vs/0, ...) that + # write_fn picks per payload (see airconditioner._climate_write). + # Applying the optimistic value and settle guard below to + # bound_entity.href instead of this target protected the wrong + # resource: /mode/vs/0 got the (nonsensical, wrong-shaped) optimistic + # merge while the resource the climate entity actually displays from + # (e.g. /power/0) never got one, so HA kept showing the pre-write + # state until the next real read of that resource -- the 20-60s lag + # in issues #17/#53, which survived the earlier optimistic-apply fix + # (issue #27) because that fix applied to the wrong href too. + write_href = '/' + '/'.join(path_segs) + # Apply the write optimistically before starting the settle guard, # not after -- mark_write_pending gates every source (poll, sweep, # observe) through the same apply(), itself included, so flipping @@ -555,19 +571,19 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # onto, the settle window was just delaying the real device # confirmation for a few seconds on every write, which read exactly # like the write being silently reverted (issue #27). - self._observe.apply(href, body, source='optimistic') - self._observe.mark_write_pending(href) + self._observe.apply(write_href, body, source='optimistic') + self._observe.mark_write_pending(write_href) def _do_put(): sess = self._session if sess is None: raise RuntimeError("no session") code, _ = sess.post(path_segs, cbor2.dumps(body), timeout=8.0) - self._log.info("PUT %s → code %#04x", href, code) + self._log.info("PUT %s → code %#04x", write_href, code) try: await self.hass.async_add_executor_job(_do_put) except Exception as e: - self._log.error("command failed for %s: %s", href, e) + self._log.error("command failed for %s: %s", write_href, e) else: await self.async_request_refresh() diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index 4692e9b..ce4772d 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -643,7 +643,13 @@ async def test_write_marks_href_pending_before_post( hass: HomeAssistant, mock_entry, mock_coordinator_observe_session ) -> None: """async_send_command must call mark_write_pending before POSTing so a - slow-to-settle device can't immediately revert the optimistic write.""" + slow-to-settle device can't immediately revert the optimistic write -- + and it must guard the write's actual target href (write_fn's own + path_segs), not bound_entity.href. Those differ for a composite entity + like the AC's ClimateDesc (bound to /mode/vs/0, but writing power/temp/ + fan/swing/preset to their own sibling hrefs) -- guarding the bound href + protected the wrong resource and left the one the entity actually + displays from unprotected (issues #17/#53).""" from custom_components.localthings.registry.discovery import BoundEntity from custom_components.localthings.registry.entities import NumberDesc @@ -662,17 +668,23 @@ async def test_write_marks_href_pending_before_post( fake.post = lambda *a, **k: (0x44, b'') await coordinator.async_send_command(bound, 5) - assert coordinator._observe._settle_until.get('/test/vs/0') is not None + assert coordinator._observe._settle_until.get('/some/path') is not None + assert coordinator._observe._settle_until.get('/test/vs/0') is None async def test_send_command_applies_write_optimistically_before_settling( - hass: HomeAssistant, mock_entry, mock_coordinator_observe_session + hass: HomeAssistant, mock_entry, mock_coordinator_observe_session, ) -> None: - """The cache must reflect a write immediately, and stay put against a - stale echo for the rest of the settle window -- otherwise - mark_write_pending has nothing to protect and just delays the real - device confirmation instead, which reads to a user as the write being - silently reverted (issue #27).""" + """The cache must reflect a write immediately at the write's actual + target href (write_fn's path_segs) -- not bound_entity.href, which + differs for a composite entity like the AC's ClimateDesc -- and stay put + against a stale echo for the rest of the settle window there. + + Applying the optimistic value to bound_entity.href instead is exactly + how issues #17/#53 survived the original optimistic-apply fix (issue + #27): the bound href got a harmless (if nonsensical) optimistic merge, + while the resource the entity actually reads from for its displayed + state never got one and stayed stale until the next real read of it.""" from custom_components.localthings.registry.discovery import BoundEntity from custom_components.localthings.registry.entities import NumberDesc @@ -691,16 +703,50 @@ async def test_send_command_applies_write_optimistically_before_settling( fake.post = lambda *a, **k: (0x44, b'') await coordinator.async_send_command(bound, 5) - # The optimistic value is visible right away, without waiting for a - # device response. - assert coordinator._cache.get('/test/vs/0') == {'value': 5} + # The optimistic value is visible right away at the write's real + # target, not the bound href. + assert coordinator._cache.get('/some/path') == {'value': 5} + assert coordinator._cache.get('/test/vs/0') != {'value': 5} # A stale update racing in behind the write (e.g. a poll/notify that # was already in flight before the PUT) must not clobber it while the # settle window is open. - applied = coordinator._observe.apply('/test/vs/0', {'value': 0}, source='observe') + applied = coordinator._observe.apply('/some/path', {'value': 0}, source='observe') assert applied is False - assert coordinator._cache.get('/test/vs/0') == {'value': 5} + assert coordinator._cache.get('/some/path') == {'value': 5} + + +async def test_climate_power_write_applies_to_its_own_href_not_bound_href( + hass: HomeAssistant, mock_entry, mock_coordinator_observe_session +) -> None: + """Regression for issues #17/#53, using the real AC capability. The + composite climate entity binds /mode/vs/0 (airconditioner.CLIMATE.href), + but a power command's write_fn (airconditioner._climate_write) targets + the sibling /power/0 -- the href climate.py's `_is_on()` actually reads. + The optimistic value and settle guard must land there, not on the bound + /mode/vs/0, or the entity never sees the write and shows stale state + until the next unrelated read of /power/0.""" + from custom_components.localthings.registry.capabilities import airconditioner + from custom_components.localthings.registry.discovery import BoundEntity + + fake = mock_coordinator_observe_session + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] + + bound = BoundEntity( + href=airconditioner.CLIMATE.href, + capability=coordinator.bound[0].capability, + desc=airconditioner.CLIMATE.entities[0], + ) + + with patch.object(fake, 'subscribe'): + fake.post = lambda *a, **k: (0x44, b'') + await coordinator.async_send_command(bound, ('power', True)) + + assert coordinator._cache.get('/power/0') == {'value': True} + assert coordinator._cache.get(airconditioner.CLIMATE.href) != {'value': True} + assert coordinator._observe._settle_until.get('/power/0') is not None class TestRemoteControlEnabled: @@ -803,7 +849,7 @@ async def test_send_command_allowed_when_remote_control_enabled( fake.post = lambda *a, **k: (0x44, b'') await coordinator.async_send_command(bound, 5) - assert coordinator._observe._settle_until.get('/test/vs/0') is not None + assert coordinator._cache.get('/some/path') == {'value': 5} async def test_send_command_remote_control_check_precedes_validate_fn(