Allow homekit_controller to handle device registry entries for devices with poor serial numbers (#64749)

homeassistant/components/homekit_controller/connection.py

@@ -25,6 +25,8 @@ from .const import (
     ENTITY_MAP,
     HOMEKIT_ACCESSORY_DISPATCH,
     IDENTIFIER_ACCESSORY_ID,
+    IDENTIFIER_LEGACY_ACCESSORY_ID,
+    IDENTIFIER_LEGACY_SERIAL_NUMBER,
     IDENTIFIER_SERIAL_NUMBER,
 )
 from .device_trigger import async_fire_triggers, async_setup_triggers_for_entry
@@ -141,6 +143,9 @@ class HKDevice:
         self._polling_lock_warned = False
         self._poll_failures = 0
 
+        # This is set to True if we can't rely on serial numbers to be unique
+        self.unreliable_serial_numbers = False
+
         self.watchable_characteristics = []
 
         self.pairing.dispatcher_connect(self.process_new_events)
@@ -207,24 +212,16 @@ class HKDevice:
             service_type=ServicesTypes.ACCESSORY_INFORMATION,
         )
 
-        serial_number = info.value(CharacteristicsTypes.SERIAL_NUMBER)
+        identifiers = {
+            (
+                IDENTIFIER_ACCESSORY_ID,
+                f"{self.unique_id}:aid:{accessory.aid}",
+            )
+        }
 
-        if valid_serial_number(serial_number):
+        if not self.unreliable_serial_numbers:
-            identifiers = {(DOMAIN, IDENTIFIER_SERIAL_NUMBER, serial_number)}
+            serial_number = info.value(CharacteristicsTypes.SERIAL_NUMBER)
-        else:
+            identifiers.add((IDENTIFIER_SERIAL_NUMBER, serial_number))
-            # Some accessories do not have a serial number
-            identifiers = {
-                (
-                    DOMAIN,
-                    IDENTIFIER_ACCESSORY_ID,
-                    f"{self.unique_id}_{accessory.aid}",
-                )
-            }
-
-        if accessory.aid == 1:
-            # Accessory 1 is the root device (sometimes the only device, sometimes a bridge)
-            # Link the root device to the pairing id for the connection.
-            identifiers.add((DOMAIN, IDENTIFIER_ACCESSORY_ID, self.unique_id))
 
         device_info = DeviceInfo(
             identifiers=identifiers,
@@ -240,13 +237,84 @@ class HKDevice:
             # It *doesn't* have a via_device, as it is the device we are connecting to
             # Every other accessory should use it as its via device.
             device_info[ATTR_VIA_DEVICE] = (
-                DOMAIN,
+                IDENTIFIER_ACCESSORY_ID,
-                IDENTIFIER_SERIAL_NUMBER,
+                f"{self.unique_id}:aid:1",
-                self.connection_info["serial-number"],
             )
 
         return device_info
 
+    @callback
+    def async_migrate_devices(self):
+        """Migrate legacy device entries from 3-tuples to 2-tuples."""
+        _LOGGER.debug(
+            "Migrating device registry entries for pairing %s", self.unique_id
+        )
+
+        device_registry = dr.async_get(self.hass)
+
+        for accessory in self.entity_map.accessories:
+            info = accessory.services.first(
+                service_type=ServicesTypes.ACCESSORY_INFORMATION,
+            )
+
+            identifiers = {
+                (
+                    DOMAIN,
+                    IDENTIFIER_LEGACY_ACCESSORY_ID,
+                    f"{self.unique_id}_{accessory.aid}",
+                ),
+            }
+
+            if accessory.aid == 1:
+                identifiers.add(
+                    (DOMAIN, IDENTIFIER_LEGACY_ACCESSORY_ID, self.unique_id)
+                )
+
+            serial_number = info.value(CharacteristicsTypes.SERIAL_NUMBER)
+            if valid_serial_number(serial_number):
+                identifiers.add(
+                    (DOMAIN, IDENTIFIER_LEGACY_SERIAL_NUMBER, serial_number)
+                )
+
+            device = device_registry.async_get_device(identifiers=identifiers)
+            if not device:
+                continue
+
+            if self.config_entry.entry_id not in device.config_entries:
+                _LOGGER.info(
+                    "Found candidate device for %s:aid:%s, but owned by a different config entry, skipping",
+                    self.unique_id,
+                    accessory.aid,
+                )
+                continue
+
+            _LOGGER.info(
+                "Migrating device identifiers for %s:aid:%s",
+                self.unique_id,
+                accessory.aid,
+            )
+
+            new_identifiers = {
+                (
+                    IDENTIFIER_ACCESSORY_ID,
+                    f"{self.unique_id}:aid:{accessory.aid}",
+                )
+            }
+
+            if not self.unreliable_serial_numbers:
+                serial_number = info.value(CharacteristicsTypes.SERIAL_NUMBER)
+                new_identifiers.add((IDENTIFIER_SERIAL_NUMBER, serial_number))
+            else:
+                _LOGGER.debug(
+                    "Not migrating serial number identifier for %s:aid:%s (it is wrong, not unique or unreliable)",
+                    self.unique_id,
+                    accessory.aid,
+                )
+
+            device_registry.async_update_device(
+                device.id, new_identifiers=new_identifiers
+            )
+
     @callback
     def async_create_devices(self):
         """
@@ -265,8 +333,8 @@ class HKDevice:
         for accessory in sorted(
             self.entity_map.accessories, key=lambda accessory: accessory.aid
         ):
-
             device_info = self.device_info_for_accessory(accessory)
+
             device = device_registry.async_get_or_create(
                 config_entry_id=self.config_entry.entry_id,
                 **device_info,
@@ -276,6 +344,46 @@ class HKDevice:
 
         self.devices = devices
 
+    @callback
+    def async_detect_workarounds(self):
+        """Detect any workarounds that are needed for this pairing."""
+        unreliable_serial_numbers = False
+
+        devices = set()
+
+        for accessory in self.entity_map.accessories:
+            info = accessory.services.first(
+                service_type=ServicesTypes.ACCESSORY_INFORMATION,
+            )
+
+            serial_number = info.value(CharacteristicsTypes.SERIAL_NUMBER)
+
+            if not valid_serial_number(serial_number):
+                _LOGGER.debug(
+                    "Serial number %r is not valid, it cannot be used as a unique identifier",
+                    serial_number,
+                )
+                unreliable_serial_numbers = True
+
+            elif serial_number in devices:
+                _LOGGER.debug(
+                    "Serial number %r is duplicated within this pairing, it cannot be used as a unique identifier",
+                    serial_number,
+                )
+                unreliable_serial_numbers = True
+
+            elif serial_number == info.value(CharacteristicsTypes.HARDWARE_REVISION):
+                # This is a known bug with some devices (e.g. RYSE SmartShades)
+                _LOGGER.debug(
+                    "Serial number %r is actually the hardware revision, it cannot be used as a unique identifier",
+                    serial_number,
+                )
+                unreliable_serial_numbers = True
+
+            devices.add(serial_number)
+
+        self.unreliable_serial_numbers = unreliable_serial_numbers
+
     async def async_process_entity_map(self):
         """
         Process the entity map and load any platforms or entities that need adding.
@@ -289,6 +397,11 @@ class HKDevice:
 
         self.pairing.pairing_data["accessories"] = self.accessories
 
+        self.async_detect_workarounds()
+
+        # Migrate to new device ids
+        self.async_migrate_devices()
+
         await self.async_load_platforms()
 
         self.async_create_devices()

homeassistant/components/homekit_controller/const.py

@@ -11,9 +11,10 @@ TRIGGERS = f"{DOMAIN}-triggers"
 HOMEKIT_DIR = ".homekit"
 PAIRING_FILE = "pairing.json"
 
-IDENTIFIER_SERIAL_NUMBER = "serial-number"
+IDENTIFIER_SERIAL_NUMBER = "homekit_controller:serial-number"
-IDENTIFIER_ACCESSORY_ID = "accessory-id"
+IDENTIFIER_ACCESSORY_ID = "homekit_controller:accessory-id"
-
+IDENTIFIER_LEGACY_SERIAL_NUMBER = "serial-number"
+IDENTIFIER_LEGACY_ACCESSORY_ID = "accessory-id"
 
 # Mapping from Homekit type to component.
 HOMEKIT_ACCESSORY_DISPATCH = {

tests/components/homekit_controller/common.py

@@ -41,7 +41,7 @@ logger = logging.getLogger(__name__)
 
 
 # Root device in test harness always has an accessory id of this
-HUB_TEST_ACCESSORY_ID: Final[str] = "00:00:00:00:00:00"
+HUB_TEST_ACCESSORY_ID: Final[str] = "00:00:00:00:00:00:aid:1"
 
 
 @dataclass
@@ -263,8 +263,8 @@ async def assert_devices_and_entities_created(
 
         device = device_registry.async_get_device(
             {
-                (DOMAIN, IDENTIFIER_SERIAL_NUMBER, expected.serial_number),
+                (IDENTIFIER_SERIAL_NUMBER, expected.serial_number),
-                (DOMAIN, IDENTIFIER_ACCESSORY_ID, expected.unique_id),
+                (IDENTIFIER_ACCESSORY_ID, expected.unique_id),
             }
         )
 
@@ -282,7 +282,7 @@ async def assert_devices_and_entities_created(
         serial_number_set = False
         accessory_id_set = False
 
-        for _, key, value in device.identifiers:
+        for key, value in device.identifiers:
             if key == IDENTIFIER_SERIAL_NUMBER:
                 assert value == expected.serial_number
                 serial_number_set = True

tests/components/homekit_controller/specific_devices/test_anker_eufycam.py

@@ -33,6 +33,7 @@ async def test_eufycam_setup(hass):
                     sw_version="1.6.7",
                     hw_version="1.0.0",
                     serial_number="A0000A000000000D",
+                    unique_id="00:00:00:00:00:00:aid:4",
                     devices=[],
                     entities=[
                         EntityTestInfo(

tests/components/homekit_controller/specific_devices/test_ecobee3.py

@@ -54,6 +54,7 @@ async def test_ecobee3_setup(hass):
                     sw_version="1.0.0",
                     hw_version="",
                     serial_number="AB1C",
+                    unique_id="00:00:00:00:00:00:aid:2",
                     devices=[],
                     entities=[
                         EntityTestInfo(
@@ -71,6 +72,7 @@ async def test_ecobee3_setup(hass):
                     sw_version="1.0.0",
                     hw_version="",
                     serial_number="AB2C",
+                    unique_id="00:00:00:00:00:00:aid:3",
                     devices=[],
                     entities=[
                         EntityTestInfo(
@@ -88,6 +90,7 @@ async def test_ecobee3_setup(hass):
                     sw_version="1.0.0",
                     hw_version="",
                     serial_number="AB3C",
+                    unique_id="00:00:00:00:00:00:aid:4",
                     devices=[],
                     entities=[
                         EntityTestInfo(
@@ -139,7 +142,7 @@ async def test_ecobee3_setup_from_cache(hass, hass_storage):
         "version": 1,
         "data": {
             "pairings": {
-                "00:00:00:00:00:00": {
+                HUB_TEST_ACCESSORY_ID: {
                     "config_num": 1,
                     "accessories": [
                         a.to_accessory_and_service_list() for a in accessories

tests/components/homekit_controller/specific_devices/test_haa_fan.py

@@ -38,6 +38,7 @@ async def test_haa_fan_setup(hass):
                     sw_version="5.0.18",
                     hw_version="",
                     serial_number="C718B3-2",
+                    unique_id="00:00:00:00:00:00:aid:2",
                     devices=[],
                     entities=[
                         EntityTestInfo(

tests/components/homekit_controller/specific_devices/test_homeassistant_bridge.py

@@ -41,6 +41,7 @@ async def test_homeassistant_bridge_fan_setup(hass):
                     sw_version="0.104.0.dev0",
                     hw_version="",
                     serial_number="fan.living_room_fan",
+                    unique_id="00:00:00:00:00:00:aid:1256851357",
                     devices=[],
                     entities=[
                         EntityTestInfo(

tests/components/homekit_controller/specific_devices/test_hue_bridge.py

@@ -36,6 +36,7 @@ async def test_hue_bridge_setup(hass):
                     sw_version="45.1.17846",
                     hw_version="",
                     serial_number="6623462389072572",
+                    unique_id="00:00:00:00:00:00:aid:6623462389072572",
                     devices=[],
                     entities=[
                         EntityTestInfo(

tests/components/homekit_controller/specific_devices/test_ryse_smart_bridge.py

@@ -33,11 +33,9 @@ async def test_ryse_smart_bridge_setup(hass):
             manufacturer="RYSE Inc.",
             sw_version="1.3.0",
             hw_version="0101.3521.0436",
-            # This is an actual bug in the device..
-            serial_number="0101.3521.0436",
             devices=[
                 DeviceTestInfo(
-                    unique_id="00:00:00:00:00:00_2",
+                    unique_id="00:00:00:00:00:00:aid:2",
                     name="Master Bath South",
                     model="RYSE Shade",
                     manufacturer="RYSE Inc.",
@@ -63,7 +61,7 @@ async def test_ryse_smart_bridge_setup(hass):
                     ],
                 ),
                 DeviceTestInfo(
-                    unique_id="00:00:00:00:00:00_3",
+                    unique_id="00:00:00:00:00:00:aid:3",
                     name="RYSE SmartShade",
                     model="RYSE Shade",
                     manufacturer="RYSE Inc.",
@@ -110,11 +108,9 @@ async def test_ryse_smart_bridge_four_shades_setup(hass):
             manufacturer="RYSE Inc.",
             sw_version="1.3.0",
             hw_version="0401.3521.0679",
-            # This is an actual bug in the device..
-            serial_number="0401.3521.0679",
             devices=[
                 DeviceTestInfo(
-                    unique_id="00:00:00:00:00:00_2",
+                    unique_id="00:00:00:00:00:00:aid:2",
                     name="LR Left",
                     model="RYSE Shade",
                     manufacturer="RYSE Inc.",
@@ -140,7 +136,7 @@ async def test_ryse_smart_bridge_four_shades_setup(hass):
                     ],
                 ),
                 DeviceTestInfo(
-                    unique_id="00:00:00:00:00:00_3",
+                    unique_id="00:00:00:00:00:00:aid:3",
                     name="LR Right",
                     model="RYSE Shade",
                     manufacturer="RYSE Inc.",
@@ -166,7 +162,7 @@ async def test_ryse_smart_bridge_four_shades_setup(hass):
                     ],
                 ),
                 DeviceTestInfo(
-                    unique_id="00:00:00:00:00:00_4",
+                    unique_id="00:00:00:00:00:00:aid:4",
                     name="BR Left",
                     model="RYSE Shade",
                     manufacturer="RYSE Inc.",
@@ -192,7 +188,7 @@ async def test_ryse_smart_bridge_four_shades_setup(hass):
                     ],
                 ),
                 DeviceTestInfo(
-                    unique_id="00:00:00:00:00:00_5",
+                    unique_id="00:00:00:00:00:00:aid:5",
                     name="RZSS",
                     model="RYSE Shade",
                     manufacturer="RYSE Inc.",

tests/components/homekit_controller/specific_devices/test_velux_gateway.py

@@ -48,6 +48,7 @@ async def test_velux_cover_setup(hass):
                     sw_version="48",
                     hw_version="",
                     serial_number="1111111a114a111a",
+                    unique_id="00:00:00:00:00:00:aid:3",
                     devices=[],
                     entities=[
                         EntityTestInfo(
@@ -68,6 +69,7 @@ async def test_velux_cover_setup(hass):
                     sw_version="16",
                     hw_version="",
                     serial_number="a11b111",
+                    unique_id="00:00:00:00:00:00:aid:2",
                     devices=[],
                     entities=[
                         EntityTestInfo(

tests/components/homekit_controller/test_connection.py

@@ -0,0 +1,164 @@
+"""Tests for HKDevice."""
+
+import dataclasses
+
+import pytest
+
+from homeassistant.components.homekit_controller.const import (
+    DOMAIN,
+    IDENTIFIER_ACCESSORY_ID,
+    IDENTIFIER_LEGACY_ACCESSORY_ID,
+    IDENTIFIER_LEGACY_SERIAL_NUMBER,
+    IDENTIFIER_SERIAL_NUMBER,
+)
+from homeassistant.core import HomeAssistant
+from homeassistant.helpers import device_registry as dr
+
+from tests.common import MockConfigEntry
+from tests.components.homekit_controller.common import (
+    setup_accessories_from_file,
+    setup_platform,
+    setup_test_accessories,
+)
+
+
+@dataclasses.dataclass
+class DeviceMigrationTest:
+    """Holds the expected state before and after testing a device identifier migration."""
+
+    fixture: str
+    manufacturer: str
+    before: set[tuple[str, str, str]]
+    after: set[tuple[str, str]]
+
+
+DEVICE_MIGRATION_TESTS = [
+    # 0401.3521.0679 was incorrectly treated as a serial number, it should be stripped out during migration
+    DeviceMigrationTest(
+        fixture="ryse_smart_bridge_four_shades.json",
+        manufacturer="RYSE Inc.",
+        before={
+            (DOMAIN, IDENTIFIER_LEGACY_ACCESSORY_ID, "00:00:00:00:00:00"),
+            (DOMAIN, IDENTIFIER_LEGACY_SERIAL_NUMBER, "0401.3521.0679"),
+        },
+        after={(IDENTIFIER_ACCESSORY_ID, "00:00:00:00:00:00:aid:1")},
+    ),
+    # This shade has a serial of 1.0.0, which we should already ignore. Make sure it gets migrated to a 2-tuple
+    DeviceMigrationTest(
+        fixture="ryse_smart_bridge_four_shades.json",
+        manufacturer="RYSE Inc.",
+        before={
+            (DOMAIN, IDENTIFIER_LEGACY_ACCESSORY_ID, "00:00:00:00:00:00_3"),
+        },
+        after={(IDENTIFIER_ACCESSORY_ID, "00:00:00:00:00:00:aid:3")},
+    ),
+    # Test migrating a Hue bridge - it has a valid serial number and has an accessory id
+    DeviceMigrationTest(
+        fixture="hue_bridge.json",
+        manufacturer="Philips Lighting",
+        before={
+            (DOMAIN, IDENTIFIER_LEGACY_ACCESSORY_ID, "00:00:00:00:00:00"),
+            (DOMAIN, IDENTIFIER_LEGACY_SERIAL_NUMBER, "123456"),
+        },
+        after={
+            (IDENTIFIER_ACCESSORY_ID, "00:00:00:00:00:00:aid:1"),
+            (IDENTIFIER_SERIAL_NUMBER, "123456"),
+        },
+    ),
+    # Test migrating a Hue remote - it has a valid serial number
+    # Originally as a non-hub non-broken device it wouldn't have had an accessory id
+    DeviceMigrationTest(
+        fixture="hue_bridge.json",
+        manufacturer="Philips",
+        before={
+            (DOMAIN, IDENTIFIER_LEGACY_SERIAL_NUMBER, "6623462389072572"),
+        },
+        after={
+            (IDENTIFIER_ACCESSORY_ID, "00:00:00:00:00:00:aid:6623462389072572"),
+            (IDENTIFIER_SERIAL_NUMBER, "6623462389072572"),
+        },
+    ),
+    # Test migrating a Koogeek LS1. This is just for completeness (testing hub and hub-less devices)
+    DeviceMigrationTest(
+        fixture="koogeek_ls1.json",
+        manufacturer="Koogeek",
+        before={
+            (DOMAIN, IDENTIFIER_LEGACY_ACCESSORY_ID, "00:00:00:00:00:00"),
+            (DOMAIN, IDENTIFIER_LEGACY_SERIAL_NUMBER, "AAAA011111111111"),
+        },
+        after={
+            (IDENTIFIER_ACCESSORY_ID, "00:00:00:00:00:00:aid:1"),
+            (IDENTIFIER_SERIAL_NUMBER, "AAAA011111111111"),
+        },
+    ),
+]
+
+
+@pytest.mark.parametrize("variant", DEVICE_MIGRATION_TESTS)
+async def test_migrate_device_id_no_serial_skip_if_other_owner(
+    hass: HomeAssistant, variant: DeviceMigrationTest
+):
+    """
+    Don't migrate unrelated devices.
+
+    Create a device registry entry that needs migrate, but belongs to a different
+    config entry. It should be ignored.
+    """
+    device_registry = dr.async_get(hass)
+
+    bridge = device_registry.async_get_or_create(
+        config_entry_id="XX",
+        identifiers=variant.before,
+        manufacturer="RYSE Inc.",
+        model="RYSE SmartBridge",
+        name="Wiring Closet",
+        sw_version="1.3.0",
+        hw_version="0101.2136.0344",
+    )
+
+    accessories = await setup_accessories_from_file(hass, variant.fixture)
+    await setup_test_accessories(hass, accessories)
+
+    bridge = device_registry.async_get(bridge.id)
+
+    assert bridge.identifiers == variant.before
+    assert bridge.config_entries == {"XX"}
+
+
+@pytest.mark.parametrize("variant", DEVICE_MIGRATION_TESTS)
+async def test_migrate_device_id_no_serial(
+    hass: HomeAssistant, variant: DeviceMigrationTest
+):
+    """Test that a Ryse smart bridge with four shades can be migrated correctly in HA."""
+    device_registry = dr.async_get(hass)
+
+    accessories = await setup_accessories_from_file(hass, variant.fixture)
+
+    fake_controller = await setup_platform(hass)
+    await fake_controller.add_paired_device(accessories, "00:00:00:00:00:00")
+    config_entry = MockConfigEntry(
+        version=1,
+        domain="homekit_controller",
+        entry_id="TestData",
+        data={"AccessoryPairingID": "00:00:00:00:00:00"},
+        title="test",
+    )
+    config_entry.add_to_hass(hass)
+
+    device = device_registry.async_get_or_create(
+        config_entry_id=config_entry.entry_id,
+        identifiers=variant.before,
+        manufacturer="Dummy Manufacturer",
+        model="Dummy Model",
+        name="Dummy Name",
+        sw_version="99999999991",
+        hw_version="99999999999",
+    )
+
+    await hass.config_entries.async_setup(config_entry.entry_id)
+    await hass.async_block_till_done()
+
+    device = device_registry.async_get(device.id)
+
+    assert device.identifiers == variant.after
+    assert device.manufacturer == variant.manufacturer