Fix instability with HomeKit trigger accessories (#80703)

fixes https://github.com/home-assistant/core/issues/78774
fixes https://github.com/home-assistant/core/issues/81685

homeassistant/components/homekit/__init__.py

@@ -23,6 +23,9 @@ from homeassistant.components.binary_sensor import (
     BinarySensorDeviceClass,
 )
 from homeassistant.components.camera import DOMAIN as CAMERA_DOMAIN
+from homeassistant.components.device_automation.trigger import (
+    async_validate_trigger_config,
+)
 from homeassistant.components.http import HomeAssistantView
 from homeassistant.components.humidifier import DOMAIN as HUMIDIFIER_DOMAIN
 from homeassistant.components.network import MDNS_TARGET_IP
@@ -906,29 +909,47 @@ class HomeKit:
         self.bridge = HomeBridge(self.hass, self.driver, self._name)
         for state in entity_states:
             self.add_bridge_accessory(state)
-        dev_reg = device_registry.async_get(self.hass)
         if self._devices:
-            valid_device_ids = []
+            await self._async_add_trigger_accessories()
-            for device_id in self._devices:
-                if not dev_reg.async_get(device_id):
-                    _LOGGER.warning(
-                        "HomeKit %s cannot add device %s because it is missing from the device registry",
-                        self._name,
-                        device_id,
-                    )
-                else:
-                    valid_device_ids.append(device_id)
-            for device_id, device_triggers in (
-                await device_automation.async_get_device_automations(
-                    self.hass,
-                    device_automation.DeviceAutomationType.TRIGGER,
-                    valid_device_ids,
-                )
-            ).items():
-                if device := dev_reg.async_get(device_id):
-                    self.add_bridge_triggers_accessory(device, device_triggers)
         return self.bridge
 
+    async def _async_add_trigger_accessories(self) -> None:
+        """Add devices with triggers to the bridge."""
+        dev_reg = device_registry.async_get(self.hass)
+        valid_device_ids = []
+        for device_id in self._devices:
+            if not dev_reg.async_get(device_id):
+                _LOGGER.warning(
+                    "HomeKit %s cannot add device %s because it is missing from the device registry",
+                    self._name,
+                    device_id,
+                )
+            else:
+                valid_device_ids.append(device_id)
+        for device_id, device_triggers in (
+            await device_automation.async_get_device_automations(
+                self.hass,
+                device_automation.DeviceAutomationType.TRIGGER,
+                valid_device_ids,
+            )
+        ).items():
+            device = dev_reg.async_get(device_id)
+            assert device is not None
+            valid_device_triggers: list[dict[str, Any]] = []
+            for trigger in device_triggers:
+                try:
+                    await async_validate_trigger_config(self.hass, trigger)
+                except vol.Invalid as ex:
+                    _LOGGER.debug(
+                        "%s: cannot add unsupported trigger %s because it requires additional inputs which are not supported by HomeKit: %s",
+                        self._name,
+                        trigger,
+                        ex,
+                    )
+                    continue
+                valid_device_triggers.append(trigger)
+            self.add_bridge_triggers_accessory(device, valid_device_triggers)
+
     async def _async_create_accessories(self) -> bool:
         """Create the accessories."""
         assert self.driver is not None

homeassistant/components/homekit/accessories.py

@@ -657,7 +657,7 @@ class HomeIIDManager(IIDManager):  # type: ignore[misc]
         """Get IID for object."""
         aid = obj.broker.aid
         if isinstance(obj, Characteristic):
-            service = obj.service
+            service: Service = obj.service
             iid = self._iid_storage.get_or_allocate_iid(
                 aid, service.type_id, service.unique_id, obj.type_id, obj.unique_id
             )

homeassistant/components/homekit/diagnostics.py

@@ -67,13 +67,16 @@ def _get_accessory_diagnostics(
     hass: HomeAssistant, accessory: HomeAccessory
 ) -> dict[str, Any]:
     """Return diagnostics for an accessory."""
-    return {
+    entity_state = None
+    if accessory.entity_id:
+        entity_state = hass.states.get(accessory.entity_id)
+    data = {
         "aid": accessory.aid,
         "config": accessory.config,
         "category": accessory.category,
         "name": accessory.display_name,
         "entity_id": accessory.entity_id,
-        "entity_state": async_redact_data(
-            hass.states.get(accessory.entity_id), TO_REDACT
-        ),
     }
+    if entity_state:
+        data["entity_state"] = async_redact_data(entity_state, TO_REDACT)
+    return data

homeassistant/components/homekit/type_triggers.py

@@ -7,9 +7,11 @@ from typing import Any
 from pyhap.const import CATEGORY_SENSOR
 
 from homeassistant.core import CALLBACK_TYPE, Context
+from homeassistant.helpers import entity_registry
 from homeassistant.helpers.trigger import async_initialize_triggers
 
 from .accessories import TYPES, HomeAccessory
+from .aidmanager import get_system_unique_id
 from .const import (
     CHAR_NAME,
     CHAR_PROGRAMMABLE_SWITCH_EVENT,
@@ -18,6 +20,7 @@ from .const import (
     SERV_SERVICE_LABEL,
     SERV_STATELESS_PROGRAMMABLE_SWITCH,
 )
+from .util import cleanup_name_for_homekit
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -39,13 +42,22 @@ class DeviceTriggerAccessory(HomeAccessory):
         self._remove_triggers: CALLBACK_TYPE | None = None
         self.triggers = []
         assert device_triggers is not None
+        ent_reg = entity_registry.async_get(self.hass)
         for idx, trigger in enumerate(device_triggers):
-            type_ = trigger["type"]
+            type_: str = trigger["type"]
-            subtype = trigger.get("subtype")
+            subtype: str | None = trigger.get("subtype")
             unique_id = f'{type_}-{subtype or ""}'
-            trigger_name = (
+            if (entity_id := trigger.get("entity_id")) and (
-                f"{type_.title()} {subtype.title()}" if subtype else type_.title()
+                entry := ent_reg.async_get(entity_id)
-            )
+            ):
+                unique_id += f"-entity_unique_id:{get_system_unique_id(entry)}"
+            trigger_name_parts = []
+            if entity_id and (state := self.hass.states.get(entity_id)):
+                trigger_name_parts.append(state.name)
+            trigger_name_parts.append(type_.replace("_", " ").title())
+            if subtype:
+                trigger_name_parts.append(subtype.replace("_", " ").title())
+            trigger_name = cleanup_name_for_homekit(" ".join(trigger_name_parts))
             serv_stateless_switch = self.add_preload_service(
                 SERV_STATELESS_PROGRAMMABLE_SWITCH,
                 [CHAR_NAME, CHAR_SERVICE_LABEL_INDEX],

tests/components/homekit/test_diagnostics.py

@@ -1,12 +1,14 @@
 """Test homekit diagnostics."""
-from unittest.mock import ANY, patch
+from unittest.mock import ANY, MagicMock, patch
 
 from homeassistant.components.homekit.const import (
+    CONF_DEVICES,
     CONF_HOMEKIT_MODE,
     DOMAIN,
     HOMEKIT_MODE_ACCESSORY,
 )
 from homeassistant.const import CONF_NAME, CONF_PORT, EVENT_HOMEASSISTANT_STARTED
+from homeassistant.setup import async_setup_component
 
 from .util import async_init_integration
 
@@ -290,3 +292,321 @@ async def test_config_entry_accessory(
     ), patch("homeassistant.components.homekit.async_port_is_available"):
         assert await hass.config_entries.async_unload(entry.entry_id)
         await hass.async_block_till_done()
+
+
+async def test_config_entry_with_trigger_accessory(
+    hass,
+    hass_client,
+    hk_driver,
+    mock_async_zeroconf,
+    events,
+    demo_cleanup,
+    device_reg,
+    entity_reg,
+):
+    """Test generating diagnostics for a bridge config entry with a trigger accessory."""
+    assert await async_setup_component(hass, "demo", {"demo": {}})
+    hk_driver.publish = MagicMock()
+
+    demo_config_entry = MockConfigEntry(domain="domain")
+    demo_config_entry.add_to_hass(hass)
+    assert await async_setup_component(hass, "demo", {"demo": {}})
+    await hass.async_block_till_done()
+
+    entry = entity_reg.async_get("light.ceiling_lights")
+    assert entry is not None
+    device_id = entry.device_id
+
+    entry = MockConfigEntry(
+        domain=DOMAIN,
+        data={
+            CONF_NAME: "mock_name",
+            CONF_PORT: 12345,
+            CONF_DEVICES: [device_id],
+            "filter": {
+                "exclude_domains": [],
+                "exclude_entities": [],
+                "include_domains": [],
+                "include_entities": ["light.none"],
+            },
+        },
+    )
+    entry.add_to_hass(hass)
+    assert await hass.config_entries.async_setup(entry.entry_id)
+    hass.bus.async_fire(EVENT_HOMEASSISTANT_STARTED)
+    await hass.async_block_till_done()
+    diag = await get_diagnostics_for_config_entry(hass, hass_client, entry)
+    diag.pop("iid_storage")
+    diag.pop("bridge")
+    assert diag == {
+        "accessories": [
+            {
+                "aid": 1,
+                "services": [
+                    {
+                        "characteristics": [
+                            {"format": "bool", "iid": 2, "perms": ["pw"], "type": "14"},
+                            {
+                                "format": "string",
+                                "iid": 3,
+                                "perms": ["pr"],
+                                "type": "20",
+                                "value": "Home Assistant",
+                            },
+                            {
+                                "format": "string",
+                                "iid": 4,
+                                "perms": ["pr"],
+                                "type": "21",
+                                "value": "Bridge",
+                            },
+                            {
+                                "format": "string",
+                                "iid": 5,
+                                "perms": ["pr"],
+                                "type": "23",
+                                "value": "mock_name",
+                            },
+                            {
+                                "format": "string",
+                                "iid": 6,
+                                "perms": ["pr"],
+                                "type": "30",
+                                "value": "homekit.bridge",
+                            },
+                            {
+                                "format": "string",
+                                "iid": 7,
+                                "perms": ["pr"],
+                                "type": "52",
+                                "value": "2022.12.0",
+                            },
+                        ],
+                        "iid": 1,
+                        "type": "3E",
+                    },
+                    {
+                        "characteristics": [
+                            {
+                                "format": "string",
+                                "iid": 9,
+                                "perms": ["pr", "ev"],
+                                "type": "37",
+                                "value": "01.01.00",
+                            }
+                        ],
+                        "iid": 8,
+                        "type": "A2",
+                    },
+                ],
+            },
+            {
+                "aid": ANY,
+                "services": [
+                    {
+                        "characteristics": [
+                            {"format": "bool", "iid": 2, "perms": ["pw"], "type": "14"},
+                            {
+                                "format": "string",
+                                "iid": 3,
+                                "perms": ["pr"],
+                                "type": "20",
+                                "value": "Demo",
+                            },
+                            {
+                                "format": "string",
+                                "iid": 4,
+                                "perms": ["pr"],
+                                "type": "21",
+                                "value": "Home Assistant",
+                            },
+                            {
+                                "format": "string",
+                                "iid": 5,
+                                "perms": ["pr"],
+                                "type": "23",
+                                "value": "Ceiling Lights",
+                            },
+                            {
+                                "format": "string",
+                                "iid": 6,
+                                "perms": ["pr"],
+                                "type": "30",
+                                "value": ANY,
+                            },
+                            {
+                                "format": "string",
+                                "iid": 7,
+                                "perms": ["pr"],
+                                "type": "52",
+                                "value": ANY,
+                            },
+                        ],
+                        "iid": 1,
+                        "type": "3E",
+                    },
+                    {
+                        "characteristics": [
+                            {
+                                "format": "uint8",
+                                "iid": 9,
+                                "perms": ["pr", "ev"],
+                                "type": "73",
+                                "valid-values": [0],
+                                "value": None,
+                            },
+                            {
+                                "format": "string",
+                                "iid": 10,
+                                "perms": ["pr"],
+                                "type": "23",
+                                "value": "Ceiling Lights " "Changed States",
+                            },
+                            {
+                                "format": "uint8",
+                                "iid": 11,
+                                "maxValue": 255,
+                                "minStep": 1,
+                                "minValue": 1,
+                                "perms": ["pr"],
+                                "type": "CB",
+                                "value": 1,
+                            },
+                        ],
+                        "iid": 8,
+                        "linked": [12],
+                        "type": "89",
+                    },
+                    {
+                        "characteristics": [
+                            {
+                                "format": "uint8",
+                                "iid": 13,
+                                "perms": ["pr"],
+                                "type": "CD",
+                                "valid-values": [0, 1],
+                                "value": 1,
+                            }
+                        ],
+                        "iid": 12,
+                        "type": "CC",
+                    },
+                    {
+                        "characteristics": [
+                            {
+                                "format": "uint8",
+                                "iid": 15,
+                                "perms": ["pr", "ev"],
+                                "type": "73",
+                                "valid-values": [0],
+                                "value": None,
+                            },
+                            {
+                                "format": "string",
+                                "iid": 16,
+                                "perms": ["pr"],
+                                "type": "23",
+                                "value": "Ceiling Lights " "Turned Off",
+                            },
+                            {
+                                "format": "uint8",
+                                "iid": 17,
+                                "maxValue": 255,
+                                "minStep": 1,
+                                "minValue": 1,
+                                "perms": ["pr"],
+                                "type": "CB",
+                                "value": 2,
+                            },
+                        ],
+                        "iid": 14,
+                        "linked": [18],
+                        "type": "89",
+                    },
+                    {
+                        "characteristics": [
+                            {
+                                "format": "uint8",
+                                "iid": 19,
+                                "perms": ["pr"],
+                                "type": "CD",
+                                "valid-values": [0, 1],
+                                "value": 1,
+                            }
+                        ],
+                        "iid": 18,
+                        "type": "CC",
+                    },
+                    {
+                        "characteristics": [
+                            {
+                                "format": "uint8",
+                                "iid": 21,
+                                "perms": ["pr", "ev"],
+                                "type": "73",
+                                "valid-values": [0],
+                                "value": None,
+                            },
+                            {
+                                "format": "string",
+                                "iid": 22,
+                                "perms": ["pr"],
+                                "type": "23",
+                                "value": "Ceiling Lights " "Turned On",
+                            },
+                            {
+                                "format": "uint8",
+                                "iid": 23,
+                                "maxValue": 255,
+                                "minStep": 1,
+                                "minValue": 1,
+                                "perms": ["pr"],
+                                "type": "CB",
+                                "value": 3,
+                            },
+                        ],
+                        "iid": 20,
+                        "linked": [24],
+                        "type": "89",
+                    },
+                    {
+                        "characteristics": [
+                            {
+                                "format": "uint8",
+                                "iid": 25,
+                                "perms": ["pr"],
+                                "type": "CD",
+                                "valid-values": [0, 1],
+                                "value": 1,
+                            }
+                        ],
+                        "iid": 24,
+                        "type": "CC",
+                    },
+                ],
+            },
+        ],
+        "client_properties": {},
+        "config-entry": {
+            "data": {"name": "mock_name", "port": 12345},
+            "options": {
+                "devices": [device_id],
+                "filter": {
+                    "exclude_domains": [],
+                    "exclude_entities": [],
+                    "include_domains": [],
+                    "include_entities": ["light.none"],
+                },
+            },
+            "title": "Mock Title",
+            "version": 1,
+        },
+        "config_version": 2,
+        "pairing_id": ANY,
+        "status": 1,
+    }
+    with patch("pyhap.accessory_driver.AccessoryDriver.async_start"), patch(
+        "homeassistant.components.homekit.HomeKit.async_stop"
+    ), patch("homeassistant.components.homekit.async_port_is_available"):
+        assert await hass.config_entries.async_unload(entry.entry_id)
+        await hass.async_block_till_done()

tests/components/homekit/test_init.py

@@ -7,9 +7,17 @@ from homeassistant.components.homekit.const import (
     DOMAIN as DOMAIN_HOMEKIT,
     EVENT_HOMEKIT_CHANGED,
 )
-from homeassistant.const import ATTR_ENTITY_ID, ATTR_SERVICE
+from homeassistant.config_entries import SOURCE_ZEROCONF, ConfigEntryState
+from homeassistant.const import (
+    ATTR_ENTITY_ID,
+    ATTR_SERVICE,
+    EVENT_HOMEASSISTANT_STARTED,
+)
 from homeassistant.setup import async_setup_component
 
+from .util import PATH_HOMEKIT
+
+from tests.common import MockConfigEntry
 from tests.components.logbook.common import MockRow, mock_humanify
 
 
@@ -52,3 +60,57 @@ async def test_humanify_homekit_changed_event(hass, hk_driver, mock_get_source_i
     assert event2["domain"] == DOMAIN_HOMEKIT
     assert event2["message"] == "send command set_cover_position to 75 for Window"
     assert event2["entity_id"] == "cover.window"
+
+
+async def test_bridge_with_triggers(
+    hass, hk_driver, mock_async_zeroconf, entity_reg, caplog
+):
+    """Test we can setup a bridge with triggers and we ignore numeric states.
+
+    Since numeric states are not supported by HomeKit as they require
+    an above or below additional configuration which we have no way
+    to input, we ignore them.
+    """
+    assert await async_setup_component(hass, "demo", {"demo": {}})
+    await hass.async_block_till_done()
+
+    entry = entity_reg.async_get("cover.living_room_window")
+    assert entry is not None
+    device_id = entry.device_id
+
+    entry = MockConfigEntry(
+        domain=DOMAIN_HOMEKIT,
+        source=SOURCE_ZEROCONF,
+        data={
+            "name": "HASS Bridge",
+            "port": 12345,
+        },
+        options={
+            "filter": {
+                "exclude_domains": [],
+                "exclude_entities": [],
+                "include_domains": [],
+                "include_entities": ["cover.living_room_window"],
+            },
+            "exclude_accessory_mode": True,
+            "mode": "bridge",
+            "devices": [device_id],
+        },
+    )
+    entry.add_to_hass(hass)
+
+    with patch(
+        "homeassistant.components.network.async_get_source_ip", return_value="1.2.3.4"
+    ), patch(f"{PATH_HOMEKIT}.async_port_is_available", return_value=True):
+        assert await hass.config_entries.async_setup(entry.entry_id)
+        await hass.async_block_till_done()
+        hass.bus.async_fire(EVENT_HOMEASSISTANT_STARTED)
+        await hass.async_block_till_done()
+
+        assert entry.state == ConfigEntryState.LOADED
+        await hass.config_entries.async_unload(entry.entry_id)
+        await hass.async_block_till_done()
+
+    assert (
+        "requires additional inputs which are not supported by HomeKit" in caplog.text
+    )