From ade4b62aec9cf35494a9ed1585483982ae8a7234 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 15 Nov 2022 11:41:55 -0600 Subject: [PATCH] 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 | 59 ++-- .../components/homekit/accessories.py | 2 +- .../components/homekit/diagnostics.py | 11 +- .../components/homekit/type_triggers.py | 22 +- tests/components/homekit/test_diagnostics.py | 322 +++++++++++++++++- tests/components/homekit/test_init.py | 64 +++- 6 files changed, 449 insertions(+), 31 deletions(-) diff --git a/homeassistant/components/homekit/__init__.py b/homeassistant/components/homekit/__init__.py index ca73c7dc24267..1129e8c3f66c7 100644 --- a/homeassistant/components/homekit/__init__.py +++ b/homeassistant/components/homekit/__init__.py @@ -23,6 +23,9 @@ 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,28 +909,46 @@ async def _async_create_bridge_accessory( 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 = [] - 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", + await self._async_add_trigger_accessories() + 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, - device_id, + trigger, + ex, ) - 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 + 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.""" diff --git a/homeassistant/components/homekit/accessories.py b/homeassistant/components/homekit/accessories.py index 75b122dfb94e8..eef43892677d1 100644 --- a/homeassistant/components/homekit/accessories.py +++ b/homeassistant/components/homekit/accessories.py @@ -657,7 +657,7 @@ def get_iid_for_obj(self, obj: Characteristic | Service) -> int: """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 ) diff --git a/homeassistant/components/homekit/diagnostics.py b/homeassistant/components/homekit/diagnostics.py index 1d0bfb92fcc44..f27171e6eae8b 100644 --- a/homeassistant/components/homekit/diagnostics.py +++ b/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 diff --git a/homeassistant/components/homekit/type_triggers.py b/homeassistant/components/homekit/type_triggers.py index b9b2ad6ce8f44..b239d67877c7d 100644 --- a/homeassistant/components/homekit/type_triggers.py +++ b/homeassistant/components/homekit/type_triggers.py @@ -7,9 +7,11 @@ 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 @@ SERV_SERVICE_LABEL, SERV_STATELESS_PROGRAMMABLE_SWITCH, ) +from .util import cleanup_name_for_homekit _LOGGER = logging.getLogger(__name__) @@ -39,13 +42,22 @@ def __init__( 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"] - subtype = trigger.get("subtype") + type_: str = trigger["type"] + subtype: str | None = trigger.get("subtype") unique_id = f'{type_}-{subtype or ""}' - trigger_name = ( - f"{type_.title()} {subtype.title()}" if subtype else type_.title() - ) + if (entity_id := trigger.get("entity_id")) and ( + 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], diff --git a/tests/components/homekit/test_diagnostics.py b/tests/components/homekit/test_diagnostics.py index d114c462e2f2c..30fe5f2d8fcec 100644 --- a/tests/components/homekit/test_diagnostics.py +++ b/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() diff --git a/tests/components/homekit/test_init.py b/tests/components/homekit/test_init.py index 17933616fc4fd..f91855b31b513 100644 --- a/tests/components/homekit/test_init.py +++ b/tests/components/homekit/test_init.py @@ -7,9 +7,17 @@ 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 + )