Only poll HomeKit connection once for all entities on a single bridge/pairing (#25249)

* Stub for polling from a central location

* Allow connection to know the entity objects attached to it

* Move polling logic to connection

* Don't poll if no characteristics selected

* Loosen coupling between entity and HKDevice

* Disable track_time_interval when removing entry

* Revert self.entities changes

* Use @callback for async_state_changed

* Split out unload and remove and add a test

* Test that entity is gone and fix docstring

homeassistant/components/homekit_controller/__init__.py

@@ -1,6 +1,7 @@
 """Support for Homekit device discovery."""
 import logging
 
+from homeassistant.core import callback
 from homeassistant.helpers.entity import Entity
 from homeassistant.exceptions import ConfigEntryNotReady
 from homeassistant.helpers import device_registry as dr
@@ -27,7 +28,6 @@ class HomeKitEntity(Entity):
 
     def __init__(self, accessory, devinfo):
         """Initialise a generic HomeKit device."""
-        self._available = True
         self._accessory = accessory
         self._aid = devinfo['aid']
         self._iid = devinfo['iid']
@@ -35,6 +35,39 @@ class HomeKitEntity(Entity):
         self._chars = {}
         self.setup()
 
+        self._signals = []
+
+    async def async_added_to_hass(self):
+        """Entity added to hass."""
+        self._signals.append(
+            self.hass.helpers.dispatcher.async_dispatcher_connect(
+                self._accessory.signal_state_updated,
+                self.async_state_changed,
+            )
+        )
+
+        self._accessory.add_pollable_characteristics(
+            self.pollable_characteristics,
+        )
+
+    async def async_will_remove_from_hass(self):
+        """Prepare to be removed from hass."""
+        self._accessory.remove_pollable_characteristics(
+            self._aid,
+        )
+
+        for signal_remove in self._signals:
+            signal_remove()
+        self._signals.clear()
+
+    @property
+    def should_poll(self) -> bool:
+        """Return False.
+
+        Data update is triggered from HKDevice.
+        """
+        return False
+
     def setup(self):
         """Configure an entity baed on its HomeKit characterstics metadata."""
         # pylint: disable=import-error
@@ -47,7 +80,7 @@ class HomeKitEntity(Entity):
             get_uuid(c) for c in self.get_characteristic_types()
         ]
 
-        self._chars_to_poll = []
+        self.pollable_characteristics = []
         self._chars = {}
         self._char_names = {}
 
@@ -75,7 +108,7 @@ class HomeKitEntity(Entity):
         from homekit.model.characteristics import CharacteristicsTypes
 
         # Build up a list of (aid, iid) tuples to poll on update()
-        self._chars_to_poll.append((self._aid, char['iid']))
+        self.pollable_characteristics.append((self._aid, char['iid']))
 
         # Build a map of ctype -> iid
         short_name = CharacteristicsTypes.get_short(char['type'])
@@ -91,30 +124,11 @@ class HomeKitEntity(Entity):
         # pylint: disable=not-callable
         setup_fn(char)
 
-    async def async_update(self):
+    @callback
-        """Obtain a HomeKit device's state."""
+    def async_state_changed(self):
-        # pylint: disable=import-error
+        """Collect new data from bridge and update the entity state in hass."""
-        from homekit.exceptions import (
+        accessory_state = self._accessory.current_state.get(self._aid, {})
-            AccessoryDisconnectedError, AccessoryNotFoundError,
+        for iid, result in accessory_state.items():
-            EncryptionError)
-
-        try:
-            new_values_dict = await self._accessory.get_characteristics(
-                self._chars_to_poll
-            )
-        except AccessoryNotFoundError:
-            # Not only did the connection fail, but also the accessory is not
-            # visible on the network.
-            self._available = False
-            return
-        except (AccessoryDisconnectedError, EncryptionError):
-            # Temporary connection failure. Device is still available but our
-            # connection was dropped.
-            return
-
-        self._available = True
-
-        for (_, iid), result in new_values_dict.items():
             if 'value' not in result:
                 continue
             # Callback to update the entity with this characteristic value
@@ -125,6 +139,8 @@ class HomeKitEntity(Entity):
             # pylint: disable=not-callable
             update_fn(result['value'])
 
+        self.async_write_ha_state()
+
     @property
     def unique_id(self):
         """Return the ID of this device."""
@@ -139,7 +155,7 @@ class HomeKitEntity(Entity):
     @property
     def available(self) -> bool:
         """Return True if entity is available."""
-        return self._available
+        return self._accessory.available
 
     @property
     def device_info(self):
@@ -211,6 +227,17 @@ async def async_setup(hass, config):
     return True
 
 
+async def async_unload_entry(hass, entry):
+    """Disconnect from HomeKit devices before unloading entry."""
+    hkid = entry.data['AccessoryPairingID']
+
+    if hkid in hass.data[KNOWN_DEVICES]:
+        connection = hass.data[KNOWN_DEVICES][hkid]
+        await connection.async_unload()
+
+    return True
+
+
 async def async_remove_entry(hass, entry):
     """Cleanup caches before removing config entry."""
     hkid = entry.data['AccessoryPairingID']

homeassistant/components/homekit_controller/connection.py

@@ -1,10 +1,14 @@
 """Helpers for managing a pairing with a HomeKit accessory or bridge."""
 import asyncio
+import datetime
 import logging
 
-from .const import HOMEKIT_ACCESSORY_DISPATCH, ENTITY_MAP
+from homeassistant.helpers.event import async_track_time_interval
+
+from .const import DOMAIN, HOMEKIT_ACCESSORY_DISPATCH, ENTITY_MAP
 
 
+DEFAULT_SCAN_INTERVAL = datetime.timedelta(seconds=60)
 RETRY_INTERVAL = 60  # seconds
 
 _LOGGER = logging.getLogger(__name__)
@@ -81,11 +85,54 @@ class HKDevice():
         # allow one entity to use pairing at once.
         self.pairing_lock = asyncio.Lock()
 
+        self.available = True
+
+        self.signal_state_updated = '_'.join((
+            DOMAIN,
+            self.unique_id,
+            'state_updated',
+        ))
+
+        # Current values of all characteristics homekit_controller is tracking.
+        # Key is a (accessory_id, characteristic_id) tuple.
+        self.current_state = {}
+
+        self.pollable_characteristics = []
+
+        # If this is set polling is active and can be disabled by calling
+        # this method.
+        self._polling_interval_remover = None
+
+    def add_pollable_characteristics(self, characteristics):
+        """Add (aid, iid) pairs that we need to poll."""
+        self.pollable_characteristics.extend(characteristics)
+
+    def remove_pollable_characteristics(self, accessory_id):
+        """Remove all pollable characteristics by accessory id."""
+        self.pollable_characteristics = [
+            char for char in self.pollable_characteristics
+            if char[0] != accessory_id
+        ]
+
+    def async_set_unavailable(self):
+        """Mark state of all entities on this connection as unavailable."""
+        self.available = False
+        self.hass.helpers.dispatcher.async_dispatcher_send(
+            self.signal_state_updated,
+        )
+
     async def async_setup(self):
         """Prepare to use a paired HomeKit device in homeassistant."""
         cache = self.hass.data[ENTITY_MAP].get_map(self.unique_id)
         if not cache:
-            return await self.async_refresh_entity_map(self.config_num)
+            if await self.async_refresh_entity_map(self.config_num):
+                self._polling_interval_remover = async_track_time_interval(
+                    self.hass,
+                    self.async_update,
+                    DEFAULT_SCAN_INTERVAL
+                )
+                return True
+            return False
 
         self.accessories = cache['accessories']
         self.config_num = cache['config_num']
@@ -98,8 +145,34 @@ class HKDevice():
 
         self.add_entities()
 
+        await self.async_update()
+
+        self._polling_interval_remover = async_track_time_interval(
+            self.hass,
+            self.async_update,
+            DEFAULT_SCAN_INTERVAL
+        )
+
         return True
 
+    async def async_unload(self):
+        """Stop interacting with device and prepare for removal from hass."""
+        if self._polling_interval_remover:
+            self._polling_interval_remover()
+
+        unloads = []
+        for platform in self.platforms:
+            unloads.append(
+                self.hass.config_entries.async_forward_entry_unload(
+                    self.config_entry,
+                    platform
+                )
+            )
+
+        results = await asyncio.gather(*unloads)
+
+        return False not in results
+
     async def async_refresh_entity_map(self, config_num):
         """Handle setup of a HomeKit accessory."""
         # pylint: disable=import-error
@@ -132,6 +205,8 @@ class HKDevice():
         # Register and add new entities that are available
         self.add_entities()
 
+        await self.async_update()
+
         return True
 
     def add_listener(self, add_entities_cb):
@@ -184,6 +259,47 @@ class HKDevice():
                 )
                 self.platforms.add(platform)
 
+    async def async_update(self, now=None):
+        """Poll state of all entities attached to this bridge/accessory."""
+        # pylint: disable=import-error
+        from homekit.exceptions import (
+            AccessoryDisconnectedError, AccessoryNotFoundError,
+            EncryptionError)
+
+        if not self.pollable_characteristics:
+            _LOGGER.debug(
+                "HomeKit connection not polling any characteristics."
+            )
+            return
+
+        _LOGGER.debug("Starting HomeKit controller update")
+
+        try:
+            new_values_dict = await self.get_characteristics(
+                self.pollable_characteristics,
+            )
+        except AccessoryNotFoundError:
+            # Not only did the connection fail, but also the accessory is not
+            # visible on the network.
+            self.async_set_unavailable()
+            return
+        except (AccessoryDisconnectedError, EncryptionError):
+            # Temporary connection failure. Device is still available but our
+            # connection was dropped.
+            return
+
+        self.available = True
+
+        for (aid, cid), value in new_values_dict.items():
+            accessory = self.current_state.setdefault(aid, {})
+            accessory[cid] = value
+
+        self.hass.helpers.dispatcher.async_dispatcher_send(
+            self.signal_state_updated,
+        )
+
+        _LOGGER.debug("Finished HomeKit controller update")
+
     async def get_characteristics(self, *args, **kwargs):
         """Read latest state from homekit accessory."""
         async with self.pairing_lock:

tests/components/homekit_controller/common.py

@@ -14,10 +14,12 @@ from homeassistant import config_entries
 from homeassistant.components.homekit_controller.const import (
     CONTROLLER, DOMAIN, HOMEKIT_ACCESSORY_DISPATCH)
 from homeassistant.components.homekit_controller import (
-    async_setup_entry, config_flow)
+    config_flow)
 from homeassistant.setup import async_setup_component
 import homeassistant.util.dt as dt_util
-from tests.common import async_fire_time_changed, load_fixture
+from tests.common import (
+    async_fire_time_changed, load_fixture, MockConfigEntry
+)
 
 
 class FakePairing:
@@ -97,12 +99,13 @@ class FakeController:
 class Helper:
     """Helper methods for interacting with HomeKit fakes."""
 
-    def __init__(self, hass, entity_id, pairing, accessory):
+    def __init__(self, hass, entity_id, pairing, accessory, config_entry):
         """Create a helper for a given accessory/entity."""
         self.hass = hass
         self.entity_id = entity_id
         self.pairing = pairing
         self.accessory = accessory
+        self.config_entry = config_entry
 
         self.characteristics = {}
         for service in self.accessory.services:
@@ -113,9 +116,7 @@ class Helper:
 
     async def poll_and_get_state(self):
         """Trigger a time based poll and return the current entity state."""
-        next_update = dt_util.utcnow() + timedelta(seconds=60)
+        await time_changed(self.hass, 60)
-        async_fire_time_changed(self.hass, next_update)
-        await self.hass.async_block_till_done()
 
         state = self.hass.states.get(self.entity_id)
         assert state is not None
@@ -161,6 +162,13 @@ class FakeService(AbstractService):
         return char
 
 
+async def time_changed(hass, seconds):
+    """Trigger time changed."""
+    next_update = dt_util.utcnow() + timedelta(seconds)
+    async_fire_time_changed(hass, next_update)
+    await hass.async_block_till_done()
+
+
 async def setup_accessories_from_file(hass, path):
     """Load an collection of accessory defs from JSON data."""
     accessories_fixture = await hass.async_add_executor_job(
@@ -239,18 +247,24 @@ async def setup_test_accessories(hass, accessories):
         'AccessoryPairingID': discovery_info['properties']['id'],
     })
 
-    config_entry = config_entries.ConfigEntry(
+    config_entry = MockConfigEntry(
-        1, 'homekit_controller', 'TestData', pairing.pairing_data,
+        version=1,
-        'test', config_entries.CONN_CLASS_LOCAL_PUSH
+        domain='homekit_controller',
+        entry_id='TestData',
+        data=pairing.pairing_data,
+        title='test',
+        connection_class=config_entries.CONN_CLASS_LOCAL_PUSH
     )
 
+    config_entry.add_to_hass(hass)
+
     pairing_cls_loc = 'homekit.controller.ip_implementation.IpPairing'
     with mock.patch(pairing_cls_loc) as pairing_cls:
         pairing_cls.return_value = pairing
-        await async_setup_entry(hass, config_entry)
+        await config_entry.async_setup(hass)
         await hass.async_block_till_done()
 
-    return pairing
+    return config_entry, pairing
 
 
 async def device_config_changed(hass, accessories):
@@ -305,6 +319,8 @@ async def setup_test_component(hass, services, capitalize=False, suffix=None):
     accessory = Accessory('TestDevice', 'example.com', 'Test', '0001', '0.1')
     accessory.services.extend(services)
 
-    pairing = await setup_test_accessories(hass, [accessory])
+    config_entry, pairing = await setup_test_accessories(hass, [accessory])
     entity = 'testdevice' if suffix is None else 'testdevice_{}'.format(suffix)
-    return Helper(hass, '.'.join((domain, entity)), pairing, accessory)
+    return Helper(
+        hass, '.'.join((domain, entity)), pairing, accessory, config_entry
+    )

tests/components/homekit_controller/specific_devices/test_aqara_gateway.py

@@ -14,7 +14,7 @@ async def test_aqara_gateway_setup(hass):
     """Test that a Aqara Gateway can be correctly setup in HA."""
     accessories = await setup_accessories_from_file(
         hass, 'aqara_gateway.json')
-    pairing = await setup_test_accessories(hass, accessories)
+    config_entry, pairing = await setup_test_accessories(hass, accessories)
 
     entity_registry = await hass.helpers.entity_registry.async_get_registry()
 
@@ -24,7 +24,9 @@ async def test_aqara_gateway_setup(hass):
     assert alarm.unique_id == 'homekit-0000000123456789-66304'
 
     alarm_helper = Helper(
-        hass, 'alarm_control_panel.aqara_hub_1563', pairing, accessories[0])
+        hass, 'alarm_control_panel.aqara_hub_1563', pairing, accessories[0],
+        config_entry
+    )
     alarm_state = await alarm_helper.poll_and_get_state()
     assert alarm_state.attributes['friendly_name'] == 'Aqara Hub-1563'
 
@@ -33,7 +35,7 @@ async def test_aqara_gateway_setup(hass):
     assert light.unique_id == 'homekit-0000000123456789-65792'
 
     light_helper = Helper(
-        hass, 'light.aqara_hub_1563', pairing, accessories[0])
+        hass, 'light.aqara_hub_1563', pairing, accessories[0], config_entry)
     light_state = await light_helper.poll_and_get_state()
     assert light_state.attributes['friendly_name'] == 'Aqara Hub-1563'
     assert light_state.attributes['supported_features'] == (

tests/components/homekit_controller/specific_devices/test_ecobee3.py

@@ -7,30 +7,31 @@ https://github.com/home-assistant/home-assistant/issues/15336
 from unittest import mock
 
 from homekit import AccessoryDisconnectedError
-import pytest
 
-from homeassistant.exceptions import ConfigEntryNotReady
+from homeassistant.config_entries import ENTRY_STATE_SETUP_RETRY
 from homeassistant.components.climate.const import (
     SUPPORT_TARGET_TEMPERATURE, SUPPORT_TARGET_HUMIDITY)
 
 
 from tests.components.homekit_controller.common import (
     FakePairing, device_config_changed, setup_accessories_from_file,
-    setup_test_accessories, Helper
+    setup_test_accessories, Helper, time_changed
 )
 
 
 async def test_ecobee3_setup(hass):
     """Test that a Ecbobee 3 can be correctly setup in HA."""
     accessories = await setup_accessories_from_file(hass, 'ecobee3.json')
-    pairing = await setup_test_accessories(hass, accessories)
+    config_entry, pairing = await setup_test_accessories(hass, accessories)
 
     entity_registry = await hass.helpers.entity_registry.async_get_registry()
 
     climate = entity_registry.async_get('climate.homew')
     assert climate.unique_id == 'homekit-123456789012-16'
 
-    climate_helper = Helper(hass, 'climate.homew', pairing, accessories[0])
+    climate_helper = Helper(
+        hass, 'climate.homew', pairing, accessories[0], config_entry
+    )
     climate_state = await climate_helper.poll_and_get_state()
     assert climate_state.attributes['friendly_name'] == 'HomeW'
     assert climate_state.attributes['supported_features'] == (
@@ -53,7 +54,7 @@ async def test_ecobee3_setup(hass):
     assert occ1.unique_id == 'homekit-AB1C-56'
 
     occ1_helper = Helper(
-        hass, 'binary_sensor.kitchen', pairing, accessories[0])
+        hass, 'binary_sensor.kitchen', pairing, accessories[0], config_entry)
     occ1_state = await occ1_helper.poll_and_get_state()
     assert occ1_state.attributes['friendly_name'] == 'Kitchen'
 
@@ -131,8 +132,8 @@ async def test_ecobee3_setup_connection_failure(hass):
 
         # If there is no cached entity map and the accessory connection is
         # failing then we have to fail the config entry setup.
-        with pytest.raises(ConfigEntryNotReady):
+        config_entry, pairing = await setup_test_accessories(hass, accessories)
-            await setup_test_accessories(hass, accessories)
+        assert config_entry.state == ENTRY_STATE_SETUP_RETRY
 
     climate = entity_registry.async_get('climate.homew')
     assert climate is None
@@ -140,7 +141,16 @@ async def test_ecobee3_setup_connection_failure(hass):
     # When accessory raises ConfigEntryNoteReady HA will retry - lets make
     # sure there is no cruft causing conflicts left behind by now doing
     # a successful setup.
-    await setup_test_accessories(hass, accessories)
+
+    # We just advance time by 5 minutes so that the retry happens, rather
+    # than manually invoking async_setup_entry - this means we need to
+    # make sure the IpPairing mock is in place or we'll try to connect to
+    # a real device. Normally this mocking is done by the helper in
+    # setup_test_accessories.
+    pairing_cls_loc = 'homekit.controller.ip_implementation.IpPairing'
+    with mock.patch(pairing_cls_loc) as pairing_cls:
+        pairing_cls.return_value = pairing
+        await time_changed(hass, 5 * 60)
 
     climate = entity_registry.async_get('climate.homew')
     assert climate.unique_id == 'homekit-123456789012-16'

tests/components/homekit_controller/specific_devices/test_koogeek_ls1.py

@@ -19,7 +19,7 @@ LIGHT_ON = ('lightbulb', 'on')
 async def test_koogeek_ls1_setup(hass):
     """Test that a Koogeek LS1 can be correctly setup in HA."""
     accessories = await setup_accessories_from_file(hass, 'koogeek_ls1.json')
-    pairing = await setup_test_accessories(hass, accessories)
+    config_entry, pairing = await setup_test_accessories(hass, accessories)
 
     entity_registry = await hass.helpers.entity_registry.async_get_registry()
 
@@ -27,7 +27,13 @@ async def test_koogeek_ls1_setup(hass):
     entry = entity_registry.async_get('light.koogeek_ls1_20833f')
     assert entry.unique_id == 'homekit-AAAA011111111111-7'
 
-    helper = Helper(hass, 'light.koogeek_ls1_20833f', pairing, accessories[0])
+    helper = Helper(
+        hass,
+        'light.koogeek_ls1_20833f',
+        pairing,
+        accessories[0],
+        config_entry
+    )
     state = await helper.poll_and_get_state()
 
     # Assert that the friendly name is detected correctly
@@ -58,9 +64,11 @@ async def test_recover_from_failure(hass, utcnow, failure_cls):
     See https://github.com/home-assistant/home-assistant/issues/18949
     """
     accessories = await setup_accessories_from_file(hass, 'koogeek_ls1.json')
-    pairing = await setup_test_accessories(hass, accessories)
+    config_entry, pairing = await setup_test_accessories(hass, accessories)
 
-    helper = Helper(hass, 'light.koogeek_ls1_20833f', pairing, accessories[0])
+    helper = Helper(
+        hass, 'light.koogeek_ls1_20833f', pairing, accessories[0], config_entry
+    )
 
     # Set light state on fake device to off
     helper.characteristics[LIGHT_ON].set_value(False)
@@ -80,7 +88,8 @@ async def test_recover_from_failure(hass, utcnow, failure_cls):
         state = await helper.poll_and_get_state()
         assert state.state == 'off'
 
-        get_char.assert_called_with([(1, 8), (1, 9), (1, 10), (1, 11)])
+        chars = get_char.call_args[0][0]
+        assert set(chars) == {(1, 8), (1, 9), (1, 10), (1, 11)}
 
     # Test that entity changes state when network error goes away
     next_update += timedelta(seconds=60)

tests/components/homekit_controller/specific_devices/test_lennox_e30.py

@@ -14,14 +14,16 @@ from tests.components.homekit_controller.common import (
 async def test_lennox_e30_setup(hass):
     """Test that a Lennox E30 can be correctly setup in HA."""
     accessories = await setup_accessories_from_file(hass, 'lennox_e30.json')
-    pairing = await setup_test_accessories(hass, accessories)
+    config_entry, pairing = await setup_test_accessories(hass, accessories)
 
     entity_registry = await hass.helpers.entity_registry.async_get_registry()
 
     climate = entity_registry.async_get('climate.lennox')
     assert climate.unique_id == 'homekit-XXXXXXXX-100'
 
-    climate_helper = Helper(hass, 'climate.lennox', pairing, accessories[0])
+    climate_helper = Helper(
+        hass, 'climate.lennox', pairing, accessories[0], config_entry
+    )
     climate_state = await climate_helper.poll_and_get_state()
     assert climate_state.attributes['friendly_name'] == 'Lennox'
     assert climate_state.attributes['supported_features'] == (

tests/components/homekit_controller/test_light.py

@@ -1,4 +1,6 @@
 """Basic checks for HomeKitSwitch."""
+from homeassistant.components.homekit_controller.const import KNOWN_DEVICES
+
 from tests.components.homekit_controller.common import (
     FakeService, setup_test_component)
 
@@ -152,3 +154,23 @@ async def test_light_becomes_unavailable_but_recovers(hass, utcnow):
     assert state.state == 'on'
     assert state.attributes['brightness'] == 255
     assert state.attributes['color_temp'] == 400
+
+
+async def test_light_unloaded(hass, utcnow):
+    """Test entity and HKDevice are correctly unloaded."""
+    bulb = create_lightbulb_service_with_color_temp()
+    helper = await setup_test_component(hass, [bulb])
+
+    # Initial state is that the light is off
+    state = await helper.poll_and_get_state()
+    assert state.state == 'off'
+
+    unload_result = await helper.config_entry.async_unload(hass)
+    assert unload_result is True
+
+    # Make sure entity is unloaded
+    assert hass.states.get(helper.entity_id) is None
+
+    # Make sure HKDevice is no longer set to poll this accessory
+    conn = hass.data[KNOWN_DEVICES]['00:00:00:00:00:00']
+    assert not conn.pollable_characteristics