diff --git a/custom_components/localthings/__init__.py b/custom_components/localthings/__init__.py index f9f3dd2..72bbb3b 100644 --- a/custom_components/localthings/__init__.py +++ b/custom_components/localthings/__init__.py @@ -13,32 +13,45 @@ from homeassistant.helpers import entity_registry as er from .const import CONF_HOST, CONF_PORT, CONF_SERIAL, DOMAIN, PLATFORMS from .coordinator import LocalThingsCoordinator +from .registry.identity import resolve_serial _LOGGER = logging.getLogger(__name__) -def _serial_from_unique_id(entry: ConfigEntry) -> str | None: +def _serial_from_unique_id(entry: ConfigEntry) -> str: """The device identity a pre-v2 entry was created with. The config flow has always keyed the entry's unique_id on the serial the probe read (`localthings_`), so that string is the identity the entry's registry entries were minted from -- there is no need to reach the - device to recover it. + device to recover it. Anything we can't recover one from resolves to the + host, which is what the coordinator seeded such an entry with anyway. - One wrinkle: for a board reporting a placeholder serial (issues #83/#189) - the two sides used to disagree. The config flow fell back to `host:port` - while the coordinator fell back to `host`, so the entry and its own - devices/entities were keyed differently. Collapse that to the - coordinator's form, which is the one the registry actually holds. + The recovered string goes back through resolve_serial rather than being + taken at face value, because the unique_id records what the flow believed + at the time it ran, not what the registry holds now. Entries created + before the placeholder rules landed (issues #83/#189) were keyed on the + placeholder itself -- `localthings_Nothing(SVC)`, `localthings_FFFF...` -- + while the coordinator has since been resolving those same boards to the + host. Re-keying the registry onto the placeholder to match the unique_id + would reintroduce the collision those issues are about: two units of that + family report the *same* placeholder, so they'd share entity unique_ids + again. + + A later wrinkle, same root cause: for a stretch the two sides disagreed on + which fallback to use, the flow writing `host:port` while the coordinator + wrote `host`. Collapse that to the coordinator's form too -- the registry + is what has to keep working. """ + host = entry.data[CONF_HOST] prefix = f"{DOMAIN}_" unique_id = entry.unique_id or "" if not unique_id.startswith(prefix): - return None + return host serial = unique_id[len(prefix) :] - if serial == f"{entry.data[CONF_HOST]}:{entry.data.get(CONF_PORT)}": - return entry.data[CONF_HOST] - return serial or None + if serial == f"{host}:{entry.data.get(CONF_PORT)}": + return host + return resolve_serial(serial, host) @callback @@ -92,6 +105,16 @@ def _repair_placeholder_keys(hass: HomeAssistant, entry: ConfigEntry, serial: st fresh = {(DOMAIN, f"{serial}{ident[1][len(host) :]}") for ident in stale} existing = dev_reg.async_get_device(identifiers=fresh) if existing is not None and existing.id != device.id: + # Removing a device takes its entities with it. Anything still + # attached here came through the pass above re-keyed rather than + # removed -- i.e. it's the surviving copy, not a duplicate -- so + # move it onto the device it now belongs to first. Otherwise the + # rewrite that was supposed to preserve an entity_id, name and + # area destroys them a few lines later. + for entity in er.async_entries_for_device( + ent_reg, device.id, include_disabled_entities=True + ): + ent_reg.async_update_entity(entity.entity_id, device_id=existing.id) _LOGGER.debug("removing orphaned device %s", device.id) dev_reg.async_remove_device(device.id) else: @@ -113,9 +136,7 @@ async def async_migrate_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: return False if entry.version == 1: - serial = ( - entry.data.get(CONF_SERIAL) or _serial_from_unique_id(entry) or entry.data[CONF_HOST] - ) + serial = entry.data.get(CONF_SERIAL) or _serial_from_unique_id(entry) hass.config_entries.async_update_entry( entry, data={**entry.data, CONF_SERIAL: serial}, diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 14fd6d1..62ffe0a 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -600,7 +600,7 @@ def _read_device(sess, host: str, port: int) -> dict: from .registry.batch import parse_device0_batch from .registry.by_type import resolve as resolve_registry - from .registry.identity import read_identity, resolve_serial + from .registry.identity import read_identity, resolve_model, resolve_serial identity = read_identity(sess, None) @@ -617,14 +617,14 @@ def _read_device(sess, host: str, port: int) -> dict: resources = parse_device0_batch(body) if isinstance(body, list) else {} info = resources.get("/information/vs/0", {}) - model_num = info.get("x.com.samsung.da.modelNum", "") registry = resolve_registry(resources, device_types=identity.device_types) return { "port": port, + # Resolved through the same helpers _run_discovery uses, so the device + # the coordinator registers up front is the one discovery would have + # produced -- no rename, and no re-key, once the first poll lands. "serial": resolve_serial(info.get("x.com.samsung.da.serialNum"), host), - # Same derivation _run_discovery uses, so the device the coordinator - # registers up front is the one discovery would have produced. - "model": model_num.split("|", 1)[0] if model_num else identity.model, + "model": resolve_model(info.get("x.com.samsung.da.modelNum", ""), identity), "manufacturer": identity.manufacturer or "Samsung", "device_type_name": registry.name if registry is not None else None, "device_type_recognized": registry is not None, diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 51c36d8..c343232 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -53,6 +53,7 @@ from .registry.identity import ( DeviceIdentity, device_display_name, read_identity, + resolve_model, resolve_serial, ) from .registry.subdevices import ( @@ -777,7 +778,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self.device_serial = serial ident = self._identity - model = model_num.split("|", 1)[0] if model_num else (ident.model if ident else "") + model = resolve_model(model_num, ident) name = device_display_name(device_type_name, model) mfr = (ident.manufacturer if ident else "") or "Samsung" diff --git a/custom_components/localthings/registry/identity.py b/custom_components/localthings/registry/identity.py index 0cbdea2..68478bc 100644 --- a/custom_components/localthings/registry/identity.py +++ b/custom_components/localthings/registry/identity.py @@ -62,6 +62,24 @@ def resolve_serial(raw_serial: str | None, host: str) -> str: return s +def resolve_model(model_num: str, identity: DeviceIdentity | None) -> str: + """The model string to name and register a device under. + + `model_num` is /information/vs/0's x.com.samsung.da.modelNum, which many + boards report as `|` -- only the part before the pipe is the + model a user would recognize. A board that reports no modelNum at all + falls back to /oic/p's mnmo, which read_identity already parsed. + + Shared with resolve_serial's motivation: the config flow resolves this + once and persists it on the entry, and the coordinator recomputes it after + the first poll. Two copies of the split rule would let those two disagree, + and a device that renames itself on the first poll is the visible symptom. + """ + if model_num: + return model_num.split("|", 1)[0] + return identity.model if identity else "" + + def device_display_name(device_type_name: str | None, model: str) -> str: """The HA device name for a resolved device type + model. diff --git a/tests/localthings/test_migration.py b/tests/localthings/test_migration.py index 68d6d53..e8997bd 100644 --- a/tests/localthings/test_migration.py +++ b/tests/localthings/test_migration.py @@ -58,6 +58,93 @@ async def test_migration_collapses_the_host_port_unique_id( assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" +async def test_migration_resolves_a_placeholder_serial_unique_id( + hass: HomeAssistant, mock_coordinator_session +) -> None: + """An entry created before the placeholder rules landed was keyed on the + placeholder itself (issues #83/#189), while the coordinator has been + resolving those boards to the host ever since. The unique_id records what + the flow believed then, not what the registry holds -- taking it at face + value would re-key working devices back onto a string every unit of the + family reports, which is the collision those issues are about.""" + entry = _legacy_entry(hass, f"{DOMAIN}_Nothing(SVC)") + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_HOST)}, + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_SERIAL] == MOCK_HOST + assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" + unchanged = dev_reg.async_get(device.id) + assert unchanged is not None + assert unchanged.identifiers == {(DOMAIN, MOCK_HOST)} + + +async def test_migration_resolves_an_all_hex_placeholder_unique_id( + hass: HomeAssistant, mock_coordinator_session +) -> None: + """The issue #189 flash-unset sentinel, same reasoning.""" + entry = _legacy_entry(hass, f"{DOMAIN}_FFFFFFFFFFFFFFF") + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_SERIAL] == MOCK_HOST + + +async def test_migration_keeps_a_survivor_off_a_removed_duplicate_device( + hass: HomeAssistant, +) -> None: + """Removing a device takes its entities with it (entity_registry's + async_device_modified), so an entity that came through the pass above + re-keyed rather than removed has to move to the surviving device first -- + otherwise the rewrite that exists to preserve an entity_id, name and area + destroys all three a few lines later. + + Migration is called directly here: what the repair leaves behind is the + contract, and going through async_setup would let the platform re-adding + its entities hide a row that had in fact been deleted.""" + from custom_components.localthings import async_migrate_entry + + entry = _legacy_entry(hass, f"{DOMAIN}_{MOCK_SERIAL}") + dev_reg = dr.async_get(hass) + ent_reg = er.async_get(hass) + + real_device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_SERIAL)}, + ) + orphan_device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_HOST)}, + ) + # The serial-keyed device exists, but this entity's serial-keyed *key* is + # free -- e.g. the user deleted the visible duplicate by hand -- so the + # entity pass rewrites it instead of removing it. + survivor = ent_reg.async_get_or_create( + "sensor", + DOMAIN, + f"{DOMAIN}_{MOCK_HOST}_connection_mode", + config_entry=entry, + device_id=orphan_device.id, + suggested_object_id="kitchen_fridge_connection", + ) + + assert await async_migrate_entry(hass, entry) is True + await hass.async_block_till_done() + + assert dev_reg.async_get(orphan_device.id) is None + kept = ent_reg.async_get(survivor.entity_id) + assert kept is not None + assert kept.entity_id == "sensor.kitchen_fridge_connection" + assert kept.unique_id == f"{DOMAIN}_{MOCK_SERIAL}_connection_mode" + assert kept.device_id == real_device.id + + async def test_migration_rekeys_an_ip_keyed_device_and_entity( hass: HomeAssistant, mock_coordinator_session ) -> None: