From 4a0a4624ff6989590e3ca65e3b6b10dd22600824 Mon Sep 17 00:00:00 2001 From: Jc2k Date: Sun, 23 Jan 2022 23:00:05 +0000 Subject: [PATCH] Allow homekit_controller to handle device registry entries for devices with poor serial numbers (#64749) --- .../homekit_controller/connection.py | 155 ++++++++++++++--- .../components/homekit_controller/const.py | 7 +- tests/components/homekit_controller/common.py | 8 +- .../specific_devices/test_anker_eufycam.py | 1 + .../specific_devices/test_ecobee3.py | 5 +- .../specific_devices/test_haa_fan.py | 1 + .../test_homeassistant_bridge.py | 1 + .../specific_devices/test_hue_bridge.py | 1 + .../test_ryse_smart_bridge.py | 16 +- .../specific_devices/test_velux_gateway.py | 2 + .../homekit_controller/test_connection.py | 164 ++++++++++++++++++ 11 files changed, 322 insertions(+), 39 deletions(-) create mode 100644 tests/components/homekit_controller/test_connection.py diff --git a/homeassistant/components/homekit_controller/connection.py b/homeassistant/components/homekit_controller/connection.py index 6e5a93bc62f..1e011eaf15d 100644 --- a/homeassistant/components/homekit_controller/connection.py +++ b/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): - identifiers = {(DOMAIN, IDENTIFIER_SERIAL_NUMBER, serial_number)} - else: - # 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)) + if not self.unreliable_serial_numbers: + serial_number = info.value(CharacteristicsTypes.SERIAL_NUMBER) + identifiers.add((IDENTIFIER_SERIAL_NUMBER, serial_number)) 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_SERIAL_NUMBER, - self.connection_info["serial-number"], + IDENTIFIER_ACCESSORY_ID, + f"{self.unique_id}:aid:1", ) 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() diff --git a/homeassistant/components/homekit_controller/const.py b/homeassistant/components/homekit_controller/const.py index bf2fd7ce1aa..ff5c0a6f20a 100644 --- a/homeassistant/components/homekit_controller/const.py +++ b/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_ACCESSORY_ID = "accessory-id" - +IDENTIFIER_SERIAL_NUMBER = "homekit_controller:serial-number" +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 = { diff --git a/tests/components/homekit_controller/common.py b/tests/components/homekit_controller/common.py index bdd8987c031..ef77d5bc652 100644 --- a/tests/components/homekit_controller/common.py +++ b/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), - (DOMAIN, IDENTIFIER_ACCESSORY_ID, expected.unique_id), + (IDENTIFIER_SERIAL_NUMBER, expected.serial_number), + (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 diff --git a/tests/components/homekit_controller/specific_devices/test_anker_eufycam.py b/tests/components/homekit_controller/specific_devices/test_anker_eufycam.py index 6fd52b68064..82348054df9 100644 --- a/tests/components/homekit_controller/specific_devices/test_anker_eufycam.py +++ b/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( diff --git a/tests/components/homekit_controller/specific_devices/test_ecobee3.py b/tests/components/homekit_controller/specific_devices/test_ecobee3.py index bd272459bdb..d2c8a793798 100644 --- a/tests/components/homekit_controller/specific_devices/test_ecobee3.py +++ b/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 diff --git a/tests/components/homekit_controller/specific_devices/test_haa_fan.py b/tests/components/homekit_controller/specific_devices/test_haa_fan.py index cdaee412565..e51bec194ed 100644 --- a/tests/components/homekit_controller/specific_devices/test_haa_fan.py +++ b/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( diff --git a/tests/components/homekit_controller/specific_devices/test_homeassistant_bridge.py b/tests/components/homekit_controller/specific_devices/test_homeassistant_bridge.py index af1a639b2e1..33e53e9413c 100644 --- a/tests/components/homekit_controller/specific_devices/test_homeassistant_bridge.py +++ b/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( diff --git a/tests/components/homekit_controller/specific_devices/test_hue_bridge.py b/tests/components/homekit_controller/specific_devices/test_hue_bridge.py index 9c58bc8a8a4..76d09629064 100644 --- a/tests/components/homekit_controller/specific_devices/test_hue_bridge.py +++ b/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( diff --git a/tests/components/homekit_controller/specific_devices/test_ryse_smart_bridge.py b/tests/components/homekit_controller/specific_devices/test_ryse_smart_bridge.py index b714f460d5e..51ebbfdc345 100644 --- a/tests/components/homekit_controller/specific_devices/test_ryse_smart_bridge.py +++ b/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.", diff --git a/tests/components/homekit_controller/specific_devices/test_velux_gateway.py b/tests/components/homekit_controller/specific_devices/test_velux_gateway.py index e2db1a7b425..d8d73709c49 100644 --- a/tests/components/homekit_controller/specific_devices/test_velux_gateway.py +++ b/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( diff --git a/tests/components/homekit_controller/test_connection.py b/tests/components/homekit_controller/test_connection.py new file mode 100644 index 00000000000..0099900b585 --- /dev/null +++ b/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