Store HomeKit generated accessory id against unique_id where possible (#33109)

* HomeKit: Store generated aid against unique_id where possible

* Fix conflict

* Fix max accessories check

* homekit counts the bridge as an accessory

* Add coverage for aidmanager

* prepare for merge

Co-authored-by: J. Nick Koston <nick@koston.org>

homeassistant/components/homekit/__init__.py

@@ -1,7 +1,6 @@
 """Support for Apple HomeKit."""
 import ipaddress
 import logging
-from zlib import adler32
 
 import voluptuous as vol
 from zeroconf import InterfaceChoice
@@ -32,7 +31,9 @@ from homeassistant.helpers.entityfilter import FILTER_SCHEMA
 from homeassistant.util import get_local_ip
 from homeassistant.util.decorator import Registry
 
+from .aidmanager import AccessoryAidStorage
 from .const import (
+    AID_STORAGE,
     BRIDGE_NAME,
     CONF_ADVERTISE_IP,
     CONF_AUTO_START,
@@ -120,6 +121,9 @@ async def async_setup(hass, config):
     """Set up the HomeKit component."""
     _LOGGER.debug("Begin setup HomeKit")
 
+    aid_storage = hass.data[AID_STORAGE] = AccessoryAidStorage(hass)
+    await aid_storage.async_initialize()
+
     conf = config[DOMAIN]
     name = conf[CONF_NAME]
     port = conf[CONF_PORT]
@@ -277,14 +281,6 @@ def get_accessory(hass, driver, state, aid, config):
     return TYPES[a_type](hass, driver, name, state.entity_id, aid, config)
 
 
-def generate_aid(entity_id):
-    """Generate accessory aid with zlib adler32."""
-    aid = adler32(entity_id.encode("utf-8"))
-    if aid in (0, 1):
-        return None
-    return aid
-
-
 class HomeKit:
     """Class to handle all actions between HomeKit and Home Assistant."""
 
@@ -339,9 +335,10 @@ class HomeKit:
 
     def reset_accessories(self, entity_ids):
         """Reset the accessory to load the latest configuration."""
+        aid_storage = self.hass.data[AID_STORAGE]
         removed = []
         for entity_id in entity_ids:
-            aid = generate_aid(entity_id)
+            aid = aid_storage.get_or_allocate_aid_for_entity_id(entity_id)
             if aid not in self.bridge.accessories:
                 _LOGGER.warning(
                     "Could not reset accessory. entity_id not found %s", entity_id
@@ -359,7 +356,19 @@ class HomeKit:
         """Try adding accessory to bridge if configured beforehand."""
         if not state or not self._filter(state.entity_id):
             return
-        aid = generate_aid(state.entity_id)
+
+        # The bridge itself counts as an accessory
+        if len(self.bridge.accessories) + 1 >= MAX_DEVICES:
+            _LOGGER.warning(
+                "Cannot add %s as this would exceeded the %d device limit. Consider using the filter option.",
+                state.entity_id,
+                MAX_DEVICES,
+            )
+            return
+
+        aid = self.hass.data[AID_STORAGE].get_or_allocate_aid_for_entity_id(
+            state.entity_id
+        )
         conf = self._config.pop(state.entity_id, {})
         # If an accessory cannot be created or added due to an exception
         # of any kind (usually in pyhap) it should not prevent
@@ -400,17 +409,12 @@ class HomeKit:
 
         for state in self.hass.states.all():
             self.add_bridge_accessory(state)
+
         self.driver.add_accessory(self.bridge)
 
         if not self.driver.state.paired:
             show_setup_message(self.hass, self.driver.state.pincode)
 
-        if len(self.bridge.accessories) > MAX_DEVICES:
-            _LOGGER.warning(
-                "You have exceeded the device limit, which might "
-                "cause issues. Consider using the filter option."
-            )
-
         _LOGGER.debug("Driver start")
         self.hass.add_job(self.driver.start)
         self.status = STATUS_RUNNING

homeassistant/components/homekit/aidmanager.py

@@ -0,0 +1,142 @@
+"""
+Manage allocation of accessory ID's.
+
+HomeKit needs to allocate unique numbers to each accessory. These need to
+be stable between reboots and upgrades.
+
+Using a hash function to generate them means collisions. It also means you
+can't change the hash without causing breakages for HA users.
+
+This module generates and stores them in a HA storage.
+"""
+import logging
+import random
+from zlib import adler32
+
+from fnvhash import fnv1a_32
+
+from homeassistant.core import HomeAssistant, callback
+from homeassistant.helpers.entity_registry import RegistryEntry
+from homeassistant.helpers.storage import Store
+
+from .const import DOMAIN
+
+AID_MANAGER_STORAGE_KEY = f"{DOMAIN}.aids"
+AID_MANAGER_STORAGE_VERSION = 1
+AID_MANAGER_SAVE_DELAY = 2
+
+INVALID_AIDS = (0, 1)
+
+AID_MIN = 2
+AID_MAX = 18446744073709551615
+
+_LOGGER = logging.getLogger(__name__)
+
+
+def get_system_unique_id(entity: RegistryEntry):
+    """Determine the system wide unique_id for an entity."""
+    return f"{entity.platform}.{entity.domain}.{entity.unique_id}"
+
+
+def _generate_aids(unique_id: str, entity_id: str) -> int:
+    """Generate accessory aid."""
+
+    # Backward compatibility: Previously HA used to *only* do adler32 on the entity id.
+    # Not stable if entity ID changes
+    # Not robust against collisions
+    yield adler32(entity_id.encode("utf-8"))
+
+    # Use fnv1a_32 of the unique id as
+    # fnv1a_32 has less collisions than
+    # adler32
+    yield fnv1a_32(unique_id.encode("utf-8"))
+
+    # If called again resort to random allocations.
+    # Given the size of the range its unlikely we'll encounter duplicates
+    # But try a few times regardless
+    for _ in range(5):
+        yield random.randrange(AID_MIN, AID_MAX)
+
+
+class AccessoryAidStorage:
+    """
+    Holds a map of entity ID to HomeKit ID.
+
+    Will generate new ID's, ensure they are unique and store them to make sure they
+    persist over reboots.
+    """
+
+    def __init__(self, hass: HomeAssistant):
+        """Create a new entity map store."""
+        self.hass = hass
+        self.store = Store(hass, AID_MANAGER_STORAGE_VERSION, AID_MANAGER_STORAGE_KEY)
+        self.allocations = {}
+        self.allocated_aids = set()
+
+        self._entity_registry = None
+
+    async def async_initialize(self):
+        """Load the latest AID data."""
+        self._entity_registry = (
+            await self.hass.helpers.entity_registry.async_get_registry()
+        )
+
+        raw_storage = await self.store.async_load()
+        if not raw_storage:
+            # There is no data about aid allocations yet
+            return
+
+        self.allocations = raw_storage.get("unique_ids", {})
+        self.allocated_aids = set(self.allocations.values())
+
+    def get_or_allocate_aid_for_entity_id(self, entity_id: str):
+        """Generate a stable aid for an entity id."""
+        entity = self._entity_registry.async_get(entity_id)
+
+        if entity:
+            return self._get_or_allocate_aid(
+                get_system_unique_id(entity), entity.entity_id
+            )
+
+        _LOGGER.warning(
+            "Entity '%s' does not have a stable unique identifier so aid allocation will be unstable and may cause collisions",
+            entity_id,
+        )
+        return adler32(entity_id.encode("utf-8"))
+
+    def _get_or_allocate_aid(self, unique_id: str, entity_id: str):
+        """Allocate (and return) a new aid for an accessory."""
+        if unique_id in self.allocations:
+            return self.allocations[unique_id]
+
+        for aid in _generate_aids(unique_id, entity_id):
+            if aid in INVALID_AIDS:
+                continue
+            if aid not in self.allocated_aids:
+                self.allocations[unique_id] = aid
+                self.allocated_aids.add(aid)
+                self.async_schedule_save()
+                return aid
+
+        raise ValueError(
+            f"Unable to generate unique aid allocation for {entity_id} [{unique_id}]"
+        )
+
+    def delete_aid(self, unique_id: str):
+        """Delete an aid allocation."""
+        if unique_id not in self.allocations:
+            return
+
+        aid = self.allocations.pop(unique_id)
+        self.allocated_aids.discard(aid)
+        self.async_schedule_save()
+
+    @callback
+    def async_schedule_save(self):
+        """Schedule saving the entity map cache."""
+        self.store.async_delay_save(self._data_to_save, AID_MANAGER_SAVE_DELAY)
+
+    @callback
+    def _data_to_save(self):
+        """Return data of entity map to store in a file."""
+        return {"unique_ids": self.allocations}

homeassistant/components/homekit/const.py

@@ -5,6 +5,7 @@ DEVICE_PRECISION_LEEWAY = 6
 DOMAIN = "homekit"
 HOMEKIT_FILE = ".homekit.state"
 HOMEKIT_NOTIFY_ID = 4663548
+AID_STORAGE = "homekit-aid-allocations"
 
 
 # #### Attributes ####

homeassistant/components/homekit/manifest.json

@@ -2,6 +2,6 @@
   "domain": "homekit",
   "name": "HomeKit",
   "documentation": "https://www.home-assistant.io/integrations/homekit",
-  "requirements": ["HAP-python==2.8.2"],
+  "requirements": ["HAP-python==2.8.2","fnvhash==0.1.0"],
   "codeowners": ["@bdraco"]
 }

requirements_all.txt

@@ -561,6 +561,9 @@ fixerio==1.0.0a0
 # homeassistant.components.flux_led
 flux_led==0.22
 
+# homeassistant.components.homekit
+fnvhash==0.1.0
+
 # homeassistant.components.foobot
 foobot_async==0.3.1
 

requirements_test_all.txt

@@ -213,6 +213,9 @@ ephem==3.7.7.0
 # homeassistant.components.feedreader
 feedparser-homeassistant==5.2.2.dev1
 
+# homeassistant.components.homekit
+fnvhash==0.1.0
+
 # homeassistant.components.foobot
 foobot_async==0.3.1
 

tests/components/homekit/test_aidmanager.py

@@ -0,0 +1,120 @@
+"""Tests for the HomeKit AID manager."""
+from asynctest import patch
+import pytest
+
+from homeassistant.components.homekit.aidmanager import (
+    AccessoryAidStorage,
+    get_system_unique_id,
+)
+from homeassistant.helpers import device_registry
+
+from tests.common import MockConfigEntry, mock_device_registry, mock_registry
+
+
+@pytest.fixture
+def device_reg(hass):
+    """Return an empty, loaded, registry."""
+    return mock_device_registry(hass)
+
+
+@pytest.fixture
+def entity_reg(hass):
+    """Return an empty, loaded, registry."""
+    return mock_registry(hass)
+
+
+async def test_aid_generation(hass, device_reg, entity_reg):
+    """Test generating aids."""
+    config_entry = MockConfigEntry(domain="test", data={})
+    config_entry.add_to_hass(hass)
+    device_entry = device_reg.async_get_or_create(
+        config_entry_id=config_entry.entry_id,
+        connections={(device_registry.CONNECTION_NETWORK_MAC, "12:34:56:AB:CD:EF")},
+    )
+    light_ent = entity_reg.async_get_or_create(
+        "light", "device", "unique_id", device_id=device_entry.id
+    )
+    light_ent2 = entity_reg.async_get_or_create(
+        "light", "device", "other_unique_id", device_id=device_entry.id
+    )
+    remote_ent = entity_reg.async_get_or_create(
+        "remote", "device", "unique_id", device_id=device_entry.id
+    )
+    hass.states.async_set(light_ent.entity_id, "on")
+    hass.states.async_set(light_ent2.entity_id, "on")
+    hass.states.async_set(remote_ent.entity_id, "on")
+    hass.states.async_set("remote.has_no_unique_id", "on")
+
+    with patch(
+        "homeassistant.components.homekit.aidmanager.AccessoryAidStorage.async_schedule_save"
+    ):
+        aid_storage = AccessoryAidStorage(hass)
+    await aid_storage.async_initialize()
+
+    for _ in range(0, 2):
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id(light_ent.entity_id)
+            == 1692141785
+        )
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id(light_ent2.entity_id)
+            == 2732133210
+        )
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id(remote_ent.entity_id)
+            == 1867188557
+        )
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id("remote.has_no_unique_id")
+            == 1872038229
+        )
+
+    aid_storage.delete_aid(get_system_unique_id(light_ent))
+    aid_storage.delete_aid(get_system_unique_id(light_ent2))
+    aid_storage.delete_aid(get_system_unique_id(remote_ent))
+    aid_storage.delete_aid("non-existant-one")
+
+    for _ in range(0, 2):
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id(light_ent.entity_id)
+            == 1692141785
+        )
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id(light_ent2.entity_id)
+            == 2732133210
+        )
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id(remote_ent.entity_id)
+            == 1867188557
+        )
+        assert (
+            aid_storage.get_or_allocate_aid_for_entity_id("remote.has_no_unique_id")
+            == 1872038229
+        )
+
+
+async def test_aid_adler32_collision(hass, device_reg, entity_reg):
+    """Test generating aids."""
+    config_entry = MockConfigEntry(domain="test", data={})
+    config_entry.add_to_hass(hass)
+    device_entry = device_reg.async_get_or_create(
+        config_entry_id=config_entry.entry_id,
+        connections={(device_registry.CONNECTION_NETWORK_MAC, "12:34:56:AB:CD:EF")},
+    )
+
+    with patch(
+        "homeassistant.components.homekit.aidmanager.AccessoryAidStorage.async_schedule_save"
+    ):
+        aid_storage = AccessoryAidStorage(hass)
+    await aid_storage.async_initialize()
+
+    seen_aids = set()
+
+    for unique_id in range(0, 202):
+        ent = entity_reg.async_get_or_create(
+            "light", "device", unique_id, device_id=device_entry.id
+        )
+        hass.states.async_set(ent.entity_id, "on")
+        aid = aid_storage.get_or_allocate_aid_for_entity_id(ent.entity_id)
+        assert aid not in seen_aids
+        seen_aids.add(aid)

tests/components/homekit/test_homekit.py

@@ -12,10 +12,10 @@ from homeassistant.components.homekit import (
     STATUS_STOPPED,
     STATUS_WAIT,
     HomeKit,
-    generate_aid,
 )
 from homeassistant.components.homekit.accessories import HomeBridge
 from homeassistant.components.homekit.const import (
+    AID_STORAGE,
     BRIDGE_NAME,
     CONF_AUTO_START,
     CONF_SAFE_MODE,
@@ -51,17 +51,6 @@ def debounce_patcher():
     patcher.stop()
 
 
-def test_generate_aid():
-    """Test generate aid method."""
-    aid = generate_aid("demo.entity")
-    assert isinstance(aid, int)
-    assert aid >= 2 and aid <= 18446744073709551615
-
-    with patch(f"{PATH_HOMEKIT}.adler32") as mock_adler32:
-        mock_adler32.side_effect = [0, 1]
-        assert generate_aid("demo.entity") is None
-
-
 async def test_setup_min(hass):
     """Test async_setup with min config options."""
     with patch(f"{PATH_HOMEKIT}.HomeKit") as mock_homekit:
@@ -223,29 +212,31 @@ async def test_homekit_setup_safe_mode(hass, hk_driver):
     assert homekit.driver.safe_mode is True
 
 
-async def test_homekit_add_accessory():
+async def test_homekit_add_accessory(hass):
     """Add accessory if config exists and get_acc returns an accessory."""
-    homekit = HomeKit("hass", None, None, None, lambda entity_id: True, {}, None, None)
+    homekit = HomeKit(hass, None, None, None, lambda entity_id: True, {}, None, None)
     homekit.driver = "driver"
     homekit.bridge = mock_bridge = Mock()
+    homekit.bridge.accessories = range(10)
+
+    assert await setup.async_setup_component(hass, DOMAIN, {DOMAIN: {}})
 
     with patch(f"{PATH_HOMEKIT}.get_accessory") as mock_get_acc:
-
         mock_get_acc.side_effect = [None, "acc", None]
         homekit.add_bridge_accessory(State("light.demo", "on"))
-        mock_get_acc.assert_called_with("hass", "driver", ANY, 363398124, {})
+        mock_get_acc.assert_called_with(hass, "driver", ANY, 363398124, {})
         assert not mock_bridge.add_accessory.called
 
         homekit.add_bridge_accessory(State("demo.test", "on"))
-        mock_get_acc.assert_called_with("hass", "driver", ANY, 294192020, {})
+        mock_get_acc.assert_called_with(hass, "driver", ANY, 294192020, {})
         assert mock_bridge.add_accessory.called
 
         homekit.add_bridge_accessory(State("demo.test_2", "on"))
-        mock_get_acc.assert_called_with("hass", "driver", ANY, 429982757, {})
+        mock_get_acc.assert_called_with(hass, "driver", ANY, 429982757, {})
         mock_bridge.add_accessory.assert_called_with("acc")
 
 
-async def test_homekit_remove_accessory():
+async def test_homekit_remove_accessory(hass):
     """Remove accessory from bridge."""
     homekit = HomeKit("hass", None, None, None, lambda entity_id: True, {}, None, None)
     homekit.driver = "driver"
@@ -259,8 +250,12 @@ async def test_homekit_remove_accessory():
 
 async def test_homekit_entity_filter(hass):
     """Test the entity filter."""
+    assert await setup.async_setup_component(hass, DOMAIN, {DOMAIN: {}})
+
     entity_filter = generate_filter(["cover"], ["demo.test"], [], [])
     homekit = HomeKit(hass, None, None, None, entity_filter, {}, None, None)
+    homekit.bridge = Mock()
+    homekit.bridge.accessories = {}
 
     with patch(f"{PATH_HOMEKIT}.get_accessory") as mock_get_acc:
         mock_get_acc.return_value = None
@@ -314,6 +309,8 @@ async def test_homekit_start_with_a_broken_accessory(hass, hk_driver, debounce_p
     pin = b"123-45-678"
     entity_filter = generate_filter(["cover", "light"], ["demo.test"], [], [])
 
+    assert await setup.async_setup_component(hass, DOMAIN, {DOMAIN: {}})
+
     homekit = HomeKit(hass, None, None, None, entity_filter, {}, None, None)
     homekit.bridge = Mock()
     homekit.bridge.accessories = []
@@ -366,6 +363,7 @@ async def test_homekit_reset_accessories(hass):
     entity_id = "light.demo"
     homekit = HomeKit(hass, None, None, None, {}, {entity_id: {}}, None)
     homekit.bridge = Mock()
+    homekit.bridge.accessories = {}
 
     with patch(f"{PATH_HOMEKIT}.HomeKit", return_value=homekit), patch(
         f"{PATH_HOMEKIT}.HomeKit.setup"
@@ -375,7 +373,7 @@ async def test_homekit_reset_accessories(hass):
 
         assert await setup.async_setup_component(hass, DOMAIN, {DOMAIN: {}})
 
-        aid = generate_aid(entity_id)
+        aid = hass.data[AID_STORAGE].get_or_allocate_aid_for_entity_id(entity_id)
         homekit.bridge.accessories = {aid: "acc"}
         homekit.status = STATUS_RUNNING
 
@@ -394,11 +392,17 @@ async def test_homekit_reset_accessories(hass):
 
 async def test_homekit_too_many_accessories(hass, hk_driver):
     """Test adding too many accessories to HomeKit."""
-    homekit = HomeKit(hass, None, None, None, None, None, None)
+
+    entity_filter = generate_filter(["cover", "light"], ["demo.test"], [], [])
+
+    homekit = HomeKit(hass, None, None, None, entity_filter, {}, None, None)
     homekit.bridge = Mock()
-    homekit.bridge.accessories = range(MAX_DEVICES + 1)
+    # The bridge itself counts as an accessory
+    homekit.bridge.accessories = range(MAX_DEVICES)
     homekit.driver = hk_driver
 
+    hass.states.async_set("light.demo", "on")
+
     with patch("pyhap.accessory_driver.AccessoryDriver.start"), patch(
         "pyhap.accessory_driver.AccessoryDriver.add_accessory"
     ), patch("homeassistant.components.homekit._LOGGER.warning") as mock_warn: