diff --git a/custom_components/blebox_advanced/__init__.py b/custom_components/blebox_advanced/__init__.py index 6c7f6d4..0693801 100644 --- a/custom_components/blebox_advanced/__init__.py +++ b/custom_components/blebox_advanced/__init__.py @@ -99,6 +99,13 @@ async def async_setup_entry( hass, entry, state=snapshot.actions if snapshot else None ) + if snapshot is None: + # Nothing has ever been observed on this device - it is not answering + # now and it has never been remembered - so every polled platform below + # creates nothing, and with no entity listening the coordinator would + # stop polling and never notice the device coming back. + entry.async_on_unload(coordinator.async_keep_polling_without_entities()) + await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) entry.async_on_unload(entry.add_update_listener(_async_options_updated)) return True diff --git a/custom_components/blebox_advanced/api.py b/custom_components/blebox_advanced/api.py index f098a61..119426a 100644 --- a/custom_components/blebox_advanced/api.py +++ b/custom_components/blebox_advanced/api.py @@ -321,7 +321,7 @@ def _handle( ATTR_BLEBOX_ID: data.blebox_id, **hints, } - device_id = _async_ha_device_id(hass, data) + device_id = _async_ha_device_id(hass, entry_id, data) if device_id is not None: event_data[CONF_DEVICE_ID] = device_id hass.bus.async_fire(HA_EVENT, event_data) @@ -363,20 +363,56 @@ def _parse_state_hints(query: Mapping[str, str]) -> dict[str, Any]: @callback -def _async_ha_device_id(hass: HomeAssistant, data: BleBoxEventsData) -> str | None: +def _async_ha_device_id( + hass: HomeAssistant, entry_id: str, data: BleBoxEventsData +) -> str | None: """Resolve (and cache) the device registry id backing this entry. Looked up lazily because the device entry may not exist yet the first time a callback arrives, and its id never changes once it does. + + Scoped to this config entry rather than looked up by identifier alone, and + that scoping is the whole point. We claim ``(blebox, )`` - the + *official* integration's domain, deliberately, so our entities land on the + same device as its relay and power entities. That makes a bare + ``async_get_device(identifiers=...)`` ambiguous the moment the official + integration is configured for the same device, which README documents as a + supported setup and which is the normal state mid-migration: the registry + narrows several matches by identifier-domain priority and keeps only rows + whose config entry domain equals the looked-up identifier's domain, so the + official row wins outright and ours is filtered out before the + "prefer the calling integration's own row" fallback is ever reached. + Device triggers are only ever offered on *our* row (see + :mod:`.device_trigger`, which skips config entries of any other domain), so + publishing the official row's id here meant the automation editor stored one + id and the bus carried another, and every device trigger built in the UI + silently never fired. The event entities kept working throughout because + they run off the dispatcher rather than the bus, which is what made it + quiet. + + ``async_entries_for_config_entry`` rather than the newer + ``async_get_device_by_identifier``: identifiers are unique within a config + entry so the two mean the same thing here, but the module-level helper has + been in Home Assistant for years, and the receiver is the one module that + should not acquire a version floor of its own. """ - if data.ha_device_id is not None: - return data.ha_device_id - device = dr.async_get(hass).async_get_device( - identifiers={(BLEBOX_DOMAIN, data.blebox_id)} - ) - if device is not None: - data.ha_device_id = device.id - return data.ha_device_id + registry = dr.async_get(hass) + if (cached := data.ha_device_id) is not None: + # Only trust the cache while it still names a row this entry owns. A + # user can delete the device from the UI, and an id cached by a build + # from before the scoping fix names the official integration's row - + # neither may be allowed to outlive its truth for the rest of a session. + device = registry.async_get(cached) + if device is not None and entry_id in device.config_entries: + return cached + data.ha_device_id = None + + identifier = (BLEBOX_DOMAIN, data.blebox_id) + for device in dr.async_entries_for_config_entry(registry, entry_id): + if identifier in device.identifiers: + data.ha_device_id = device.id + return device.id + return None @callback diff --git a/custom_components/blebox_advanced/blebox_actions.py b/custom_components/blebox_advanced/blebox_actions.py index 30905a2..a1fe661 100644 --- a/custom_components/blebox_advanced/blebox_actions.py +++ b/custom_components/blebox_advanced/blebox_actions.py @@ -368,12 +368,32 @@ def supports_http_action(self, trigger_type: int) -> bool: return ACTION_HTTP_GET in allowed if allowed else not self.field_preferences def free_slots(self) -> list[dict[str, Any]]: - """Unconfigured slots, in slot order.""" + """Unconfigured slots, in slot order. + + These three accessors partition the slot array: a slot is free, or ours, + or somebody else's, and never two of those at once. Provisioning adds + the pools up to decide whether a run fits, so an overlap between any two + of them would overcount the device's real capacity. + """ return [a for a in self.actions if not is_configured(a)] def owned_actions(self) -> list[dict[str, Any]]: - """Slots created by this integration, in slot order.""" - return [a for a in self.actions if is_owned(a)] + """Slots holding a live callback of this integration, in slot order. + + ``is_configured`` is required as well as the ownership marker, and it is + not redundant with it: trigger type and action type are separate fields, + so a slot can carry our URL while its trigger reads as unconfigured. + Firmware that honours the ``triggerType: 0`` half of a clear and keeps + the ``actionType``/``param`` half leaves exactly that behind. + + Such a slot used to appear in this list *and* in :meth:`free_slots`, + which let one physical slot be counted twice towards capacity and be + handed out twice in one run: the second callback silently destroyed the + first, and a slot taken from the free list while still sitting in the + reclaimable list was wiped by the clearing pass that followed. A slot + with no trigger never fires, so it is simply free. + """ + return [a for a in self.actions if is_configured(a) and is_owned(a)] def foreign_actions(self) -> list[dict[str, Any]]: """Return configured slots owned by someone else; these are never touched.""" @@ -942,6 +962,19 @@ async def _async_sync_locked(self, desired: list[DesiredAction]) -> SyncResult: writes.append(build_action_payload(stale, template, **_clear_overrides())) result.cleared.append(_slot_id(stale)) + # The whole plan is checked before the first request leaves, because a + # run that wrote two things into one slot would break the promise this + # method exists to keep in the quietest possible way: the second write + # destroys the first, and `SyncResult` still reports both. The pools are + # disjoint by construction, so this can only fire if that ever stops + # being true - in which case refusing the run leaves the device exactly + # as it was, which is the outcome design rule 2 asks for. + planned = [_slot_id(payload) for payload in writes] + if len(set(planned)) != len(planned): + raise BleBoxActionApiError( + f"Refusing to write the same action slot twice in one run: {planned}" + ) + for payload in writes: await self.async_save_action(payload) @@ -956,10 +989,19 @@ async def async_remove_owned_actions(self) -> list[int]: async with self._action_lock: state = await self.async_get_actions_state() template = _field_template(state.actions) + # Every slot carrying our marker, rather than `owned_actions()`: + # removal has to erase our callback URL even from a slot whose + # trigger the firmware has already zeroed, or the entry goes away + # and the callback token stays readable in the wBox app. + # # Slot ids are resolved before the first write, so a device that # describes a slot without one fails before anything is touched # rather than half way through the clearing. - targets = [(_slot_id(action), action) for action in state.owned_actions()] + targets = [ + (_slot_id(action), action) + for action in state.actions + if is_owned(action) + ] for _, action in targets: await self.async_save_action( build_action_payload(action, template, **_clear_overrides()) diff --git a/custom_components/blebox_advanced/coordinator.py b/custom_components/blebox_advanced/coordinator.py index 6d11fbc..02f449c 100644 --- a/custom_components/blebox_advanced/coordinator.py +++ b/custom_components/blebox_advanced/coordinator.py @@ -23,7 +23,7 @@ from typing import Any from homeassistant.config_entries import ConfigEntry -from homeassistant.core import HomeAssistant, callback +from homeassistant.core import CALLBACK_TYPE, HomeAssistant, callback from homeassistant.helpers import issue_registry as ir from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed @@ -191,6 +191,19 @@ def _measured_values(state: dict[str, Any]) -> tuple[str, ...]: return tuple(sorted(types)) +def _identity(info: DeviceInfo | None) -> tuple[str, ...] | None: + """Return the identity fields a device page shows, or None if it never said.""" + if info is None: + return None + return ( + info.device_type, + info.product, + info.firmware_version, + info.hardware_version, + info.api_level, + ) + + def _timed_relays(state: dict[str, Any]) -> tuple[int, ...]: """Return the relays reporting a countdown, one sensor each.""" relays = state.get("relays") @@ -228,7 +241,13 @@ def capability_signature(snapshot: DeviceSnapshot) -> tuple[Any, ...]: _measured_values(state), _timed_relays(state), snapshot.uptime_s is not None, - snapshot.info is not None, + # The identity is compared by value, not merely by presence. It decides + # no entity, so it is not a capability, but it is what a device page + # shows; remembering only "an identity existed" left an offline start + # seeding the firmware version from before the last update, which is + # exactly the field someone checks to confirm an update worked. These + # move about as often as the firmware does, so they cost no writes. + _identity(snapshot.info), ) @@ -492,6 +511,59 @@ def async_request_full_refresh(self) -> None: """Ask for settings and actions on the next refresh, not just state.""" self._force_full = True + @callback + def async_keep_polling_without_entities(self) -> CALLBACK_TYPE: + """Poll on with nothing listening, and reload once the device answers. + + A device that has never answered has nothing remembered, so every polled + platform creates nothing and no ``CoordinatorEntity`` is ever added. + ``DataUpdateCoordinator`` arms its interval only while something is + listening, so without this the entry stays inert for good: it never + polls, so automatic callbacks are never healed and the entities never + arrive however long the device has been back, until somebody reloads the + entry by hand. Setup deliberately succeeds while the device is down, so + Home Assistant will not retry it either. + + Registering a listener is deliberately the whole mechanism. A timer of + our own would poll a device that *does* have entities a second time on + every interval; a listener shares the single interval the coordinator + already keeps, and it is only ever registered when there are no entities + to keep it. + + Polling alone would still not produce the entities, because platform + setup has already run and nothing runs it again - so the first answer + reloads the entry. Setup refreshes before it forwards the platforms, so + by then they see a live snapshot and create everything the device turned + out to have. Stopping first makes this a one-shot: the reloaded entry + either has entities of its own or lands right back here. + + Returns the callback that stops it, for the caller to hand to the config + entry so that an unload takes it down with everything else. + """ + remove: CALLBACK_TYPE | None = None + + @callback + def _async_stop() -> None: + """Stop watching, at most once: removing a listener twice raises.""" + nonlocal remove + if remove is None: + return + unsubscribe, remove = remove, None + unsubscribe() + + @callback + def _async_answered() -> None: + """Reload the entry as soon as there is something to build from.""" + # Listeners are told about a *failed* refresh too, which is what + # keeps entities unavailable, so the snapshot is what to check. + if self.data is None: + return + _async_stop() + self.hass.config_entries.async_schedule_reload(self.config_entry.entry_id) + + remove = self.async_add_listener(_async_answered) + return _async_stop + @property def settings(self) -> dict[str, Any]: """The device's settings, preferring a value just written from here. @@ -575,20 +647,31 @@ async def _async_update_data(self) -> DeviceSnapshot: # Best-effort: a device that answers its identity but not these still # delivers events, so a failure here must not fail the whole update. - settings: dict[str, Any] = {} - settings_read = True + # + # What a failure must not do either is publish an empty payload as live + # state. The update still succeeds, so every entity stays *available* + # and simply reports the wrong thing: the cloud tunnel, the backlight + # and the access point all read as off, the overload threshold and the + # restart select go unknown, and the access point blanks its SSID. Home + # Assistant records each of those as a genuine state change, and the + # relay-only polls in between carry it forward until the next slow + # cycle, so an automation watching the cloud tunnel fires on nothing at + # all. The payload the device last gave is still the best description of + # it, so that is what a failed read carries forward - only ever on + # failure, so a device that really did answer with an empty object is + # still believed. + settings: dict[str, Any] try: settings = await self.manager.async_get_settings() except BleBoxError as err: - settings_read = False + settings = previous.settings _LOGGER.debug("Settings unavailable on %s: %s", info.name, err) - network: dict[str, Any] = {} - network_read = True + network: dict[str, Any] try: network = await self.manager.async_get_network() except BleBoxError as err: - network_read = False + network = previous.network _LOGGER.debug("Network state unavailable on %s: %s", info.name, err) uptime = await self.manager.async_get_uptime() @@ -612,13 +695,18 @@ async def _async_update_data(self) -> DeviceSnapshot: health=health, ) - # A fetch that failed is not evidence that a capability went away: the - # best-effort reads above report an empty object either way, and - # remembering that would leave the next offline start without those - # entities. Uptime is best-effort inside the API layer itself, so a - # device that has reported one before has to keep reporting it to count. - uptime_read = uptime is not None or previous.uptime_s is None - if settings_read and network_read and uptime_read: + # A fetch that failed is not evidence that a capability went away, but + # it no longer has to block this either: the best-effort reads above + # carry the last payload forward, so the snapshot describes the same + # capabilities the last successful read did. Demanding that all of them + # succeed instead meant firmware without ``/api/device/network`` never + # had its shape remembered at all, and a device that has never been + # remembered comes up with no entities at all when it is unreachable at + # startup. Uptime is the one read still worth guarding: it is + # best-effort inside the API layer itself and answers ``None`` either + # way, so a device that has reported one before has to keep reporting it + # to count as having answered. + if uptime is not None or previous.uptime_s is None: self._async_remember_capabilities(snapshot) return snapshot diff --git a/custom_components/blebox_advanced/entity.py b/custom_components/blebox_advanced/entity.py index 96f094c..a454b30 100644 --- a/custom_components/blebox_advanced/entity.py +++ b/custom_components/blebox_advanced/entity.py @@ -12,9 +12,11 @@ from contextlib import AbstractContextManager, contextmanager from typing import Any +from homeassistant.core import callback from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers import device_registry as dr from homeassistant.helpers.device_registry import DeviceInfo +from homeassistant.helpers.typing import UNDEFINED from homeassistant.helpers.update_coordinator import CoordinatorEntity from .blebox_actions import BleBoxError, InsufficientSlotsError @@ -97,6 +99,42 @@ def deep_merge(base: dict[str, Any], patch: dict[str, Any]) -> dict[str, Any]: return merged +def device_identity( + entry: BleBoxEventsConfigEntry, data: BleBoxEventsData +) -> dict[str, str | None]: + """Return the model, firmware and hardware version the device reports now. + + Keyed to match both :class:`DeviceInfo` and + ``device_registry.async_update_device``, because the device page has to be + told the same three values in two different ways: once when an entity is + added, and again whenever a poll finds them changed. + + The coordinator re-reads identity on every slow cycle, so its snapshot is + the live answer and wins. ``entry.data`` is only what the config flow wrote + at setup and nothing ever updates it, which is why it is the fallback rather + than the source: it is all a device that has never answered has, and the + offline-start path depends on those values still producing valid device + info. + + ``device_type`` and not ``product`` on purpose. The config flow stored the + device type as the model, so reading the marketing name here instead would + silently rename the model on every existing device page. + """ + snapshot = data.coordinator.data + info = snapshot.info if snapshot else None + return { + "model": (info.device_type if info else "") + or entry.data.get(CONF_MODEL) + or None, + "sw_version": (info.firmware_version if info else "") + or entry.data.get(CONF_SW_VERSION) + or None, + "hw_version": (info.hardware_version if info else "") + or entry.data.get(CONF_HW_VERSION) + or None, + } + + def build_device_info( entry: BleBoxEventsConfigEntry, data: BleBoxEventsData ) -> DeviceInfo: @@ -110,13 +148,14 @@ def build_device_info( is advertised too, so the link survives if the official integration ever changes how it builds identifiers. """ + identity = device_identity(entry, data) device_info = DeviceInfo( identifiers={(BLEBOX_DOMAIN, data.blebox_id)}, manufacturer=MANUFACTURER, name=entry.title, - model=entry.data.get(CONF_MODEL) or None, - sw_version=entry.data.get(CONF_SW_VERSION) or None, - hw_version=entry.data.get(CONF_HW_VERSION) or None, + model=identity["model"], + sw_version=identity["sw_version"], + hw_version=identity["hw_version"], configuration_url=data.manager.base_url, ) if (mac := mac_connection(data.blebox_id)) is not None: @@ -152,6 +191,49 @@ def __init__( if placeholders: self._attr_translation_placeholders = placeholders self._attr_device_info = build_device_info(entry, data) + # What the device page was last told, so an unchanged identity costs a + # dict comparison rather than a registry lookup on every poll. + self._identity = device_identity(entry, data) + + @callback + def _handle_coordinator_update(self) -> None: + """Write the new state, taking any identity change to the device page.""" + self._async_apply_device_identity() + super()._handle_coordinator_update() + + @callback + def _async_apply_device_identity(self) -> None: + """Push a changed firmware, hardware or model version into the registry. + + Home Assistant reads ``device_info`` once, when the entity is added, so + without this the device page keeps showing whatever the device reported + at setup for as long as the entry stays loaded - and firmware version is + exactly the field a user checks to confirm an update worked, on an + integration that can start that update itself. Sourcing the values live + is not enough on its own: nothing re-reads them until a reload. + + The registry row is addressed by id rather than looked up by identifier. + Our identifier is the *official* integration's (see + :func:`build_device_info`), and looking one up by identifier finds that + integration's row instead of ours when both are configured for the same + device. + """ + identity = device_identity(self._entry, self._data) + # Nothing new to say, or no device page to say it to. The identity is + # recorded as applied only once it really has been, so an entity that + # somehow has no device row yet tries again on the next poll. + if identity == self._identity or self.device_entry is None: + return + self._identity = identity + # UNDEFINED rather than None for anything unreported, so firmware that + # stops sending ``hv`` leaves the hardware version the page already + # shows alone instead of blanking it. + dr.async_get(self.hass).async_update_device( + self.device_entry.id, + model=identity["model"] or UNDEFINED, + sw_version=identity["sw_version"] or UNDEFINED, + hw_version=identity["hw_version"] or UNDEFINED, + ) @property def settings(self) -> dict[str, Any]: diff --git a/custom_components/blebox_advanced/event.py b/custom_components/blebox_advanced/event.py index af067f7..0a8fbaa 100644 --- a/custom_components/blebox_advanced/event.py +++ b/custom_components/blebox_advanced/event.py @@ -72,7 +72,35 @@ def _async_enable_selected_inputs(hass: HomeAssistant, data: BleBoxEventsData) - registry_entry is not None and registry_entry.disabled_by is er.RegistryEntryDisabler.INTEGRATION ): - registry.async_update_entity(entity_id, disabled_by=None) + _async_enable_quietly(registry, entity_id) + + +@callback +def _async_enable_quietly(registry: er.EntityRegistry, entity_id: str) -> None: + """Clear our own disable without making Home Assistant reload the entry. + + Enabling a registry entry normally schedules a reload of its config entry + thirty seconds later, which is how an entity switched on in the UI comes + into existence. Here that reload is pure waste: this runs inside platform + setup and the entity is added, enabled, a few lines further down, so all the + reload does is tear down every entity of the entry and re-run provisioning + to arrive at what it already had. Ticking events for a new input in the + options therefore reloaded the entry twice, the second time for no visible + reason at all. + + Home Assistant skips that scheduling for exactly one transition: an entity + coming back from ``CONFIG_ENTRY``, because enabling a config entry reloads + it anyway. Handing our own disable over to ``CONFIG_ENTRY`` first therefore + reaches the same end state quietly - the hand-over is ignored too, since the + entity is still disabled at that point, and only an entity that ends up + *enabled* is worth a reload. Both writes are synchronous with nothing + awaited between them, so no other code can observe the entry in between and + the registry only ever persists the final value. + """ + registry.async_update_entity( + entity_id, disabled_by=er.RegistryEntryDisabler.CONFIG_ENTRY + ) + registry.async_update_entity(entity_id, disabled_by=None) async def async_setup_entry( diff --git a/tests/test_advanced.py b/tests/test_advanced.py index 11f1a3d..e3e0732 100644 --- a/tests/test_advanced.py +++ b/tests/test_advanced.py @@ -22,6 +22,7 @@ from homeassistant.helpers import issue_registry as ir from homeassistant.helpers.dispatcher import async_dispatcher_send from homeassistant.helpers.network import NoURLAvailableError +from homeassistant.setup import async_setup_component from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.blebox_advanced import device_trigger @@ -1717,6 +1718,186 @@ async def test_triggers_are_only_listed_for_devices_that_are_ours( assert await device_trigger.async_get_triggers(hass, "no-such-device") == [] +# --- the device row a press names ------------------------------------------- +# +# The event has to name the same registry row the triggers were offered on, or +# every device automation built in the editor stores one id while the bus +# carries another and nothing ever fires. The event entities keep working +# either way - they run off the dispatcher, not the bus - so only a test that +# looks at the id itself can see this. + + +def _official_row(hass: HomeAssistant) -> dr.DeviceEntry: + """Register the official BleBox integration's row for the same device. + + Both integrations claim ``(blebox, )`` on purpose, which is what + puts our entities on the official device's page. Each config entry still + gets its own registry row, so from here on a lookup by that identifier + alone is ambiguous - and Home Assistant resolves it in the official + integration's favour, its domain being the one in the identifier. README + documents running both side by side, and it is the normal state during a + migration. + """ + official_entry = MockConfigEntry(domain=BLEBOX_DOMAIN, unique_id=BLEBOX_ID) + official_entry.add_to_hass(hass) + return dr.async_get(hass).async_get_or_create( + config_entry_id=official_entry.entry_id, + identifiers={(BLEBOX_DOMAIN, BLEBOX_ID)}, + connections={(dr.CONNECTION_NETWORK_MAC, "ae:0b:fb:f9:27:ba")}, + manufacturer="BleBox", + name="Simon GO Switch", + model="switchBox", + ) + + +def _our_row(hass: HomeAssistant) -> dr.DeviceEntry: + """Return the registry row this integration's own entities live on. + + Read back through one of our entities rather than by identifier, so the + test does not resolve the row the same ambiguous way the bug did. + """ + entity = er.async_get(hass).async_get(_eid(hass, "event", "input_0")) + row = dr.async_get(hass).async_get(entity.device_id) + assert row is not None + return row + + +async def test_a_press_names_the_row_its_triggers_were_offered_on( + hass: HomeAssistant, hass_client_no_auth +) -> None: + """With the official integration also configured, the event names our row. + + Triggers are only ever offered on the row carrying one of *our* config + entries, so this is the id the automation editor stores. Firing the event + against the official integration's row instead breaks every one of them at + once, silently. + """ + official = _official_row(hass) + await _setup(hass, _entry()) + ours = _our_row(hass) + + assert ours.id != official.id + assert len(await device_trigger.async_get_triggers(hass, ours.id)) == 8 + assert await device_trigger.async_get_triggers(hass, official.id) == [] + + fired: list[Any] = [] + hass.bus.async_listen(HA_EVENT, lambda event: fired.append(event)) + + client = await hass_client_no_auth() + assert (await client.get(f"/api/{DOMAIN}/{TOKEN}/0/short_press")).status == 200 + await hass.async_block_till_done() + + assert len(fired) == 1 + assert fired[0].data["device_id"] == ours.id + + +async def test_a_device_trigger_still_fires_beside_the_official_integration( + hass: HomeAssistant, hass_client_no_auth +) -> None: + """The whole chain, with both integrations set up for the one device. + + Editor-shaped automation on the row the editor would have offered, then a + real callback over HTTP. This is the acceptance criterion the id-level + assertions above only stand in for. + """ + _official_row(hass) + await _setup(hass, _entry()) + ours = _our_row(hass) + + assert await async_setup_component( + hass, + "automation", + { + "automation": { + "triggers": { + "trigger": "device", + "domain": DOMAIN, + "device_id": ours.id, + "type": "long_press", + "subtype": "1", + }, + "actions": {"event": "blebox_advanced_test_fired"}, + } + }, + ) + await hass.async_block_till_done() + + fired: list[Any] = [] + hass.bus.async_listen( + "blebox_advanced_test_fired", lambda event: fired.append(event) + ) + + client = await hass_client_no_auth() + assert (await client.get(f"/api/{DOMAIN}/{TOKEN}/0/long_press")).status == 200 + await hass.async_block_till_done() + + assert len(fired) == 1 + + +async def test_a_press_names_our_row_when_we_are_the_only_integration( + hass: HomeAssistant, hass_client_no_auth +) -> None: + """Scoping the lookup to our entry leaves the ordinary setup untouched. + + The row is picked by the BleBox identifier it carries, not by being the + only row the entry happens to own, so a second row registered against the + same entry cannot be mistaken for the device. + """ + entry = _entry() + entry.add_to_hass(hass) + decoy = dr.async_get(hass).async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, f"{BLEBOX_ID}_not_the_device")}, + name="Something else this entry owns", + ) + await _setup(hass, entry) + ours = _our_row(hass) + + assert ours.id != decoy.id + assert len(await device_trigger.async_get_triggers(hass, ours.id)) == 8 + + fired: list[Any] = [] + hass.bus.async_listen(HA_EVENT, lambda event: fired.append(event)) + + client = await hass_client_no_auth() + assert (await client.get(f"/api/{DOMAIN}/{TOKEN}/0/short_press")).status == 200 + await hass.async_block_till_done() + + assert len(fired) == 1 + assert fired[0].data["device_id"] == ours.id + + +async def test_a_cached_row_id_that_is_not_ours_is_discarded( + hass: HomeAssistant, hass_client_no_auth +) -> None: + """The cached id corrects itself rather than outliving its truth. + + The id is cached because the row may not exist the first time a callback + arrives and never changes once it does. Two things can still falsify it: a + value written by a build from before the lookup was scoped, which names the + official integration's row, and a row deleted from the device page. Neither + may be allowed to stand for the rest of the session, so a cached id is + trusted only while it still names a row this entry owns. + """ + official = _official_row(hass) + entry = _entry() + await _setup(hass, entry) + ours = _our_row(hass) + + entry.runtime_data.ha_device_id = official.id + + fired: list[Any] = [] + hass.bus.async_listen(HA_EVENT, lambda event: fired.append(event)) + + client = await hass_client_no_auth() + assert (await client.get(f"/api/{DOMAIN}/{TOKEN}/0/short_press")).status == 200 + await hass.async_block_till_done() + + assert len(fired) == 1 + assert fired[0].data["device_id"] == ours.id + assert entry.runtime_data.ha_device_id == ours.id + + # --- more of the callback endpoint ------------------------------------------- diff --git a/tests/test_blebox_actions.py b/tests/test_blebox_actions.py index 428baa7..449813c 100644 --- a/tests/test_blebox_actions.py +++ b/tests/test_blebox_actions.py @@ -120,6 +120,20 @@ def owned_slot( ) +def half_cleared_owned_slot(slot_id: int, input_id: int) -> dict[str, Any]: + """Build a slot of ours that the device reports as having no trigger. + + Trigger type and action type are separate fields, so firmware that honours + the ``triggerType: 0`` half of a clear and keeps the ``actionType``/``param`` + half leaves this behind: a slot the device considers empty which still + carries our callback URL. It is the one shape that used to satisfy both + "free" and "ours". + """ + slot = owned_slot(slot_id, input_id, TRIGGER_SHORT_CLICK, "short_press") + slot["triggerType"] = TRIGGER_UNCONFIGURED + return slot + + def make_state(actions: list[dict[str, Any]], total: int = 8) -> ActionsState: """Build an ActionsState padded out to `total` fixed slots.""" slots = list(actions) @@ -768,6 +782,123 @@ async def test_a_recycled_slot_is_repurposed_in_one_write() -> None: assert result.slots_free == 0 +# --- Slots that read as both free and ours ---------------------------------- + + +async def test_a_half_cleared_slot_is_spent_once_not_filled_and_wiped() -> None: + """A slot that reads as both empty and ours is used once and left alone. + + Regression: `free_slots()` selected on trigger type while `owned_actions()` + selected on action type plus our URL marker, and neither excluded the other. + A half-cleared slot of ours therefore sat in both lists, so the run took it + from the free list, wrote the callback into it, and then cleared the very + same slot on the way out because it was still queued as stale. The device + was left with an empty slot while `SyncResult` reported a creation, and the + user's button silently never fired. + """ + manager = RecordingManager(make_state([half_cleared_owned_slot(0, 0)], total=1)) + + url = owned_url(0, "long_press") + result = await manager.async_sync_http_actions( + [DesiredAction(0, TRIGGER_LONG_CLICK, url, "HA IN1 long_press")] + ) + + assert [write["id"] for write in manager.writes] == [0] + assert manager.writes[0]["param"] == url + assert manager.writes[0]["triggerType"] == TRIGGER_LONG_CLICK + assert result.created == [0] + assert result.cleared == [] + + +async def test_a_half_cleared_slot_is_counted_once_towards_capacity() -> None: + """One physical slot counts once, and a run that needs two still refuses. + + Regression: counted as free *and* as reclaimable, a single half-cleared slot + of ours made a two-callback run look like it fitted on a device with one + usable slot. Both callbacks were then planned into that slot and the second + write destroyed the first, which is exactly the "either fits entirely or + changes nothing" promise the capacity check exists to keep. + """ + theirs = configured_slot(1, 1, TRIGGER_SHORT_CLICK, 1, "", "user action") + manager = RecordingManager( + make_state([half_cleared_owned_slot(0, 0), theirs], total=2) + ) + + desired = [ + DesiredAction( + 0, TRIGGER_LONG_CLICK, owned_url(0, "long_press"), "HA IN1 long_press" + ), + DesiredAction( + 1, TRIGGER_LONG_CLICK, owned_url(1, "long_press"), "HA IN2 long_press" + ), + ] + with pytest.raises(InsufficientSlotsError) as err: + await manager.async_sync_http_actions(desired) + + assert manager.writes == [] + assert err.value.needed == 2 + # The half-cleared slot is usable, once. The user's slot never is. + assert err.value.available == 1 + assert err.value.total == 2 + + +async def test_a_plan_that_double_books_a_slot_writes_nothing() -> None: + """Two writes planned into one slot abort the run before the first request. + + The slot pools are disjoint by construction, so this drives the check with a + state whose pools deliberately overlap the way they used to. What it pins is + the failure mode: a run that would write a slot twice must leave the device + exactly as it was rather than create a callback and wipe it moments later. + """ + + class OverlappingState(ActionsState): + """Slot pools that overlap, as they did before they were made disjoint.""" + + def free_slots(self) -> list[dict[str, Any]]: + """Offer every slot as free, including the ones we already own.""" + return list(self.actions) + + base = make_state([owned_slot(0, 0, TRIGGER_SHORT_CLICK, "short_press")], total=1) + manager = RecordingManager( + OverlappingState(base.actions, base.items_limit, base.field_preferences) + ) + + with pytest.raises(BleBoxActionApiError) as err: + await manager.async_sync_http_actions( + [ + DesiredAction( + 0, + TRIGGER_LONG_CLICK, + owned_url(0, "long_press"), + "HA IN1 long_press", + ) + ] + ) + + assert manager.writes == [] + assert "twice" in str(err.value) + + +async def test_removal_erases_our_url_from_a_half_cleared_slot() -> None: + """Deleting the integration takes our URL out of a dormant slot as well. + + A half-cleared slot no longer counts as one of ours for provisioning, since + a slot with no trigger never fires and is simply free. Removal is the one + place that still has to visit it: leaving it be would keep the callback + token readable in the wBox app after the entry it belonged to was gone. + """ + theirs = configured_slot(1, 1, TRIGGER_LONG_CLICK, 1, "", "user action") + manager = RecordingManager( + make_state([half_cleared_owned_slot(0, 0), theirs], total=2) + ) + + cleared = await manager.async_remove_owned_actions() + + assert cleared == [0] + assert [write["id"] for write in manager.writes] == [0] + assert manager.writes[0]["param"] == "" + + # --- Malformed device payloads ---------------------------------------------- diff --git a/tests/test_integration.py b/tests/test_integration.py index e44a3e4..3d1e51a 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -8,19 +8,24 @@ from __future__ import annotations +import dataclasses from contextlib import ExitStack +from datetime import timedelta from typing import Any from unittest.mock import patch import pytest from homeassistant.components.device_automation import DeviceAutomationType -from homeassistant.const import CONF_HOST, CONF_PORT, STATE_UNAVAILABLE +from homeassistant.config_entries import RELOAD_AFTER_UPDATE_DELAY +from homeassistant.const import CONF_HOST, CONF_PORT, STATE_ON, STATE_UNAVAILABLE from homeassistant.core import HomeAssistant from homeassistant.helpers import device_registry as dr from homeassistant.helpers import entity_registry as er from homeassistant.helpers import issue_registry as ir +from homeassistant.util import dt as dt_util from pytest_homeassistant_custom_component.common import ( MockConfigEntry, + async_fire_time_changed, async_get_device_automations, ) @@ -50,6 +55,8 @@ HA_EVENT, MODE_AUTOMATIC, MODE_MANUAL, + RESTART_STATE_RESTORE, + SCAN_INTERVAL_SECONDS, ) BLEBOX_ID = "ae0bfbf927ba" @@ -728,6 +735,12 @@ async def test_a_half_answered_poll_does_not_forget_the_shape( answers its identity but times out on one of those reports exactly what a device that genuinely has none of it reports. Remembering that would leave the next offline start missing entities the device really does have. + + Settings and network defend themselves by carrying the last payload + forward, so what is remembered is still what the device last said about + itself. Uptime cannot do that - it only ever counts up, so a carried-forward + one would be a lie - and it is therefore still what decides whether a poll + is worth remembering at all. """ entry = _entry() await _setup(hass, entry) @@ -748,6 +761,16 @@ async def test_a_half_answered_poll_does_not_forget_the_shape( assert entry.data[CONF_DEVICE_CACHE] == remembered assert entry.data[CONF_DEVICE_CACHE]["settings"]["relays"] + # A device that has reported an uptime and now will not is the one case + # that still has to hold the shape back, rather than remember a device + # without an uptime sensor. + with _device_reads(uptime=None): + coordinator.async_request_full_refresh() + await coordinator.async_refresh() + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_CACHE] == remembered + async def test_a_changed_shape_is_remembered_without_reloading( hass: HomeAssistant, @@ -777,6 +800,32 @@ async def test_a_changed_shape_is_remembered_without_reloading( assert entry.runtime_data.coordinator is coordinator +async def test_new_firmware_is_remembered_for_the_next_offline_start( + hass: HomeAssistant, +) -> None: + """A firmware update reaches the remembered identity, not just the live one. + + The signature recorded only whether an identity existed, so a firmware + version change never rewrote the cache. Starting up unreachable then seeded + the device page from before the update, which is the one field someone + checks to confirm an update worked. + """ + entry = _entry() + await _setup(hass, entry) + assert entry.data[CONF_DEVICE_CACHE]["info"]["fv"] == DEVICE.firmware_version + + updated = dataclasses.replace(DEVICE, firmware_version="0.1600") + with ( + _device_reads(), + patch(f"{MANAGER}.async_get_device_info", return_value=updated), + ): + entry.runtime_data.coordinator.async_request_full_refresh() + await entry.runtime_data.coordinator.async_refresh() + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_CACHE]["info"]["fv"] == "0.1600" + + async def test_removing_the_entry_clears_its_repair_issues( hass: HomeAssistant, ) -> None: @@ -836,3 +885,267 @@ async def test_diagnostics_redact_the_token(hass: HomeAssistant) -> None: assert TOKEN not in repr(diagnostics) assert diagnostics["inputs"]["detected"] == [0, 1] assert len(diagnostics["callback_mappings"]) == 3 + + +# --- a best-effort read that failed is not a device that answered emptily ---- + + +async def test_a_failed_settings_read_keeps_the_settings_it_had( + hass: HomeAssistant, +) -> None: + """A settings read that times out leaves the settings-backed entities alone. + + Regression: settings are a best-effort read, so a failure was swallowed and + the empty payload left behind went into the snapshot as though the device + had answered with it. The poll still counted as a success, so nothing went + unavailable and the entities simply inverted: Home Assistant recorded that + as a genuine state change, so the switch the README singles out as the + security relevant one reported itself turned off and any automation watching + the cloud tunnel fired for nothing. + """ + entry = _entry() + await _setup(hass, entry) + coordinator = entry.runtime_data.coordinator + + tunnel = _registered(hass, "switch", "cloud_tunnel") + backlight = _registered(hass, "light", "buttons_backlight") + overload = _registered(hass, "number", "overload_threshold") + restart = _registered(hass, "select", "state_after_restart") + assert hass.states.get(tunnel).state == STATE_ON + + with ( + _device_reads(), + patch( + f"{MANAGER}.async_get_settings", + side_effect=BleBoxConnectionError("timed out"), + ), + ): + coordinator.async_request_full_refresh() + await coordinator.async_refresh() + await hass.async_block_till_done() + + # Still believed, because the device did answer everything else - and still + # reporting what it last said about itself rather than the absence of it. + assert coordinator.last_update_success + assert hass.states.get(tunnel).state == STATE_ON + assert hass.states.get(backlight).state == STATE_ON + # The colour matters as much as the state here: with the payload blanked, + # turning the backlight on wrote the default colour over the user's own. + assert hass.states.get(backlight).attributes["rgb_color"] == (255, 255, 255) + assert hass.states.get(overload).state == "0.0" + assert hass.states.get(restart).state == RESTART_STATE_RESTORE + + # Every relay-only poll until the next slow cycle carries the same snapshot + # forward, which is what stretched a single failed read to a whole minute. + with _device_reads(): + await coordinator.async_refresh() + await hass.async_block_till_done() + assert hass.states.get(tunnel).state == STATE_ON + + # A device that really does answer with an empty object is still believed: + # carrying values forward must not outlive the failure it covers for. + with _device_reads(), patch(f"{MANAGER}.async_get_settings", return_value={}): + coordinator.async_request_full_refresh() + await coordinator.async_refresh() + await hass.async_block_till_done() + assert hass.states.get(tunnel).state == "off" + + +async def test_a_failed_network_read_keeps_the_access_point_it_had( + hass: HomeAssistant, +) -> None: + """The same for the network read, which the access point switch is built on. + + A blanked network payload turned the access point switch off and emptied the + SSID it publishes, so the device looked as though it had stopped + broadcasting - the opposite of the state the entity exists to warn about. + """ + entry = _entry() + await _setup(hass, entry) + coordinator = entry.runtime_data.coordinator + + access_point = _registered(hass, "switch", "access_point") + assert hass.states.get(access_point).state == STATE_ON + + with ( + _device_reads(), + patch( + f"{MANAGER}.async_get_network", + side_effect=BleBoxConnectionError("timed out"), + ), + ): + coordinator.async_request_full_refresh() + await coordinator.async_refresh() + await hass.async_block_till_done() + + assert coordinator.last_update_success + state = hass.states.get(access_point) + assert state.state == STATE_ON + assert state.attributes["ssid"] == NETWORK["apSSID"] + + +async def test_a_device_without_a_network_endpoint_is_still_remembered( + hass: HomeAssistant, +) -> None: + """Firmware without ``/api/device/network`` still has its shape remembered. + + The remembered shape is the only thing that gives a device its entities back + after a restart while it is unreachable. It used to be written only when + settings, network *and* uptime had all answered on the same poll, so a + device whose firmware simply does not have that endpoint was never + remembered at all and fell into the offline-start trap on every restart it + was unlucky with. + """ + entry = _entry() + entry.add_to_hass(hass) + with ( + _device_reads(), + patch( + f"{MANAGER}.async_get_network", + side_effect=BleBoxConnectionError("no such endpoint"), + ), + patch(f"{MANAGER}.async_save_action"), + ): + assert await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + cache = entry.data.get(CONF_DEVICE_CACHE) + assert cache, "a device that answered everything it has was not remembered" + assert cache["settings"]["relays"] + # Nothing was invented for the endpoint it does not have, so the access + # point switch is absent here exactly as it is on a live poll. + assert cache["network"] == {} + assert _registered(hass, "switch", "access_point") is None + + # What all of that is for: the entities come back on an offline restart. + persisted = dict(entry.data) + with patch(f"{MANAGER}.async_remove_owned_actions", return_value=[]): + await hass.config_entries.async_remove(entry.entry_id) + await hass.async_block_till_done() + + restarted = MockConfigEntry( + domain=DOMAIN, + title=entry.title, + unique_id=BLEBOX_ID, + data=persisted, + options=dict(entry.options), + ) + restarted.add_to_hass(hass) + with _unreachable(): + assert await hass.config_entries.async_setup(restarted.entry_id) + await hass.async_block_till_done() + + assert _registered(hass, "switch", "relay") is not None + assert _registered(hass, "switch", "cloud_tunnel") is not None + + # Unloaded inside the patch: a poll would otherwise reach a real socket. + await hass.config_entries.async_unload(restarted.entry_id) + await hass.async_block_till_done() + + +async def test_a_device_that_has_never_answered_comes_back_by_itself( + hass: HomeAssistant, +) -> None: + """An entry set up while the device was down still recovers on its own. + + Regression: setup deliberately succeeds when the device is unreachable, so + Home Assistant never retries it, and with nothing remembered every polled + platform creates nothing. `DataUpdateCoordinator` arms its interval only + while something is listening to it, so the entry stopped polling altogether: + the device could come back and nothing would notice, no automatic callback + would ever be healed, and only a manual reload got the entry out of it. + """ + entry = _entry() + entry.add_to_hass(hass) + with _unreachable(): + assert await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + assert _registered(hass, "switch", "relay") is None + # Kept across the reload below, which replaces the runtime data. + coordinator = entry.runtime_data.coordinator + + # A poll that failed is announced just as a successful one is, and must + # not be taken for the device answering: there is still nothing to build + # anything from, so nothing should happen but another poll. + async_fire_time_changed( + hass, dt_util.utcnow() + timedelta(seconds=SCAN_INTERVAL_SECONDS + 1) + ) + await hass.async_block_till_done() + assert not coordinator.last_update_success + assert _registered(hass, "switch", "relay") is None + + # The announcement a poll makes when it fails after a good one, which is + # how entities go unavailable. It carries no snapshot either. + coordinator.async_update_listeners() + await hass.async_block_till_done() + assert _registered(hass, "switch", "relay") is None + + with _device_reads(), patch(f"{MANAGER}.async_save_action"): + async_fire_time_changed( + hass, dt_util.utcnow() + timedelta(seconds=2 * SCAN_INTERVAL_SECONDS + 2) + ) + await hass.async_block_till_done() + + # It polled at all, which is what an entry with no entities stopped + # doing, and then acted on what the poll finally told it. + assert coordinator.last_update_success + relay = _registered(hass, "switch", "relay") + assert relay is not None, "the device answered and nothing was created" + assert hass.states.get(relay).state == STATE_ON + # The pushed entities were there all along and survive the recovery. + assert hass.states.get(_entity_id(hass, 0)) is not None + + await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() + + +async def test_enabling_events_for_an_input_reloads_the_entry_once( + hass: HomeAssistant, +) -> None: + """Ticking events for an unused input reloads the entry once, not twice. + + An input with nothing selected is registered disabled, and + ``entity_registry_enabled_default`` is read only at that first registration, + so platform setup has to clear the disable by hand for the option to mean + anything. Doing that the obvious way makes Home Assistant schedule a reload + of the config entry thirty seconds later, which tears down every entity and + re-runs provisioning to arrive at exactly what the reload the user's own + change already triggered had just finished building. + """ + entry = _entry(**{CONF_ENABLED_EVENTS: {"0": ["short_press"], "1": []}}) + await _setup(hass, entry) + registry = er.async_get(hass) + spare = _entity_id(hass, 1) + assert registry.async_get(spare).disabled_by is er.RegistryEntryDisabler.INTEGRATION + + with ( + _device_reads(), + patch(f"{MANAGER}.async_save_action"), + patch.object( + hass.config_entries, + "async_reload", + wraps=hass.config_entries.async_reload, + ) as reload, + ): + hass.config_entries.async_update_entry( + entry, + options={ + **entry.options, + CONF_ENABLED_EVENTS: {"0": ["short_press"], "1": ["long_press"]}, + }, + ) + await hass.async_block_till_done() + + # The option took effect on the one reload the change itself asked for. + assert reload.call_count == 1 + assert registry.async_get(spare).disabled_by is None + assert hass.states.get(spare) is not None + + async_fire_time_changed( + hass, dt_util.utcnow() + timedelta(seconds=RELOAD_AFTER_UPDATE_DELAY + 1) + ) + await hass.async_block_till_done() + assert reload.call_count == 1, "the entry was reloaded a second time" + + await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() diff --git a/tests/test_settings_entities.py b/tests/test_settings_entities.py index 1082715..a54d881 100644 --- a/tests/test_settings_entities.py +++ b/tests/test_settings_entities.py @@ -8,12 +8,14 @@ import time from contextlib import ExitStack +from dataclasses import replace from typing import Any from unittest.mock import patch import pytest from homeassistant.core import HomeAssistant from homeassistant.exceptions import HomeAssistantError, ServiceValidationError +from homeassistant.helpers import device_registry as dr from homeassistant.helpers import entity_registry as er from pytest_homeassistant_custom_component.common import MockConfigEntry @@ -36,6 +38,7 @@ UPTIME_S, _actions_state, _entry, + _unreachable, ) @@ -677,3 +680,70 @@ async def test_device_settings_win_again_after_the_settle_window( await hass.async_block_till_done() assert hass.states.get(entity_id).state == "on" + + +# --- device identity -------------------------------------------------------- + + +async def test_a_firmware_update_reaches_the_device_page(hass: HomeAssistant) -> None: + """A new firmware version shows on the device page without a reload. + + Regression: model, firmware and hardware version were read out of + `entry.data`, which the config flow writes once and nothing ever updates. + The coordinator re-read the identity on every slow cycle and the update + entity showed the new version, but the device page kept the old one + indefinitely - and that page is exactly where a user looks to confirm a + firmware update, which this integration can start itself, actually worked. + """ + entry = _entry() + await _setup_with(hass, entry) + + registry = dr.async_get(hass) + device = next(iter(dr.async_entries_for_config_entry(registry, entry.entry_id))) + assert device.sw_version == "0.1502" + + coordinator = entry.runtime_data.coordinator + with ( + _reads(), + patch( + f"{MANAGER}.async_get_device_info", + return_value=replace(DEVICE, firmware_version="0.1600"), + ), + ): + # Forced, because identity is only re-read on the slow cycle. + coordinator.async_request_full_refresh() + await coordinator.async_refresh() + await hass.async_block_till_done() + + assert registry.async_get(device.id).sw_version == "0.1600" + firmware = hass.states.get(_eid(hass, "update", "firmware")) + # The device page and the update entity next to it must not disagree. + assert firmware.attributes["installed_version"] == "0.1600" + + +async def test_a_device_that_never_answered_still_has_a_device_page( + hass: HomeAssistant, +) -> None: + """The versions the config flow stored carry an entry that starts offline. + + Live identity is preferred over `entry.data`, but a device that has never + answered has no live identity to offer: the fallback is all it has, and it + has to keep producing device info Home Assistant accepts, or an offline + start would leave the pushed event entities with no device page at all. + """ + entry = _entry() + entry.add_to_hass(hass) + with _unreachable(): + assert await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + registry = dr.async_get(hass) + device = next(iter(dr.async_entries_for_config_entry(registry, entry.entry_id))) + assert device.model == "switchBox" + assert device.sw_version == "0.1502" + assert device.hw_version == "s_KS.swB.1.5.T.p55ST-0.3" + + # Unloaded inside the patch: the retry timer would otherwise fire + # against a real socket during teardown. + await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done()