From 5d31f9f44168fad7d09496669b995d8fce6a6260 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 12 May 2020 15:47:33 +0000 Subject: [PATCH 1/4] Ensure homekit_controller recieves zeroconf c# updates If an integration has a homekit config flow step homekit controller would not see updates for devices that were paired with it and would not rescan for changes. --- homeassistant/components/zeroconf/__init__.py | 10 ++++++++-- tests/components/zeroconf/test_init.py | 9 ++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/homeassistant/components/zeroconf/__init__.py b/homeassistant/components/zeroconf/__init__.py index 343897a44c09e5..514af01ec30368 100644 --- a/homeassistant/components/zeroconf/__init__.py +++ b/homeassistant/components/zeroconf/__init__.py @@ -178,8 +178,14 @@ def service_update(zeroconf, service_type, name, state_change): _LOGGER.debug("Discovered new device %s %s", name, info) # If we can handle it as a HomeKit discovery, we do that here. - if service_type == HOMEKIT_TYPE and handle_homekit(hass, info): - return + if service_type == HOMEKIT_TYPE: + handle_homekit(hass, info) + # Continue on here as homekit_controller + # still needs to get updates on devices + # so it can see when the 'c#' field is updated. + # + # If the device already has homekit pairings + # homekit_controller will ignore it. for domain in ZEROCONF[service_type]: hass.add_job( diff --git a/tests/components/zeroconf/test_init.py b/tests/components/zeroconf/test_init.py index 66a4a5bf44c05b..153d06f3b80621 100644 --- a/tests/components/zeroconf/test_init.py +++ b/tests/components/zeroconf/test_init.py @@ -123,7 +123,8 @@ async def test_homekit_match_partial_space(hass, mock_zeroconf): assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) assert len(mock_service_browser.mock_calls) == 1 - assert len(mock_config_flow.mock_calls) == 1 + # One call to `lifx`, one call to `homekit_controller` + assert len(mock_config_flow.mock_calls) == 2 assert mock_config_flow.mock_calls[0][1][0] == "lifx" @@ -142,7 +143,8 @@ async def test_homekit_match_partial_dash(hass, mock_zeroconf): assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) assert len(mock_service_browser.mock_calls) == 1 - assert len(mock_config_flow.mock_calls) == 1 + # One call to `rachio`, one call to `homekit_controller` + assert len(mock_config_flow.mock_calls) == 2 assert mock_config_flow.mock_calls[0][1][0] == "rachio" @@ -159,7 +161,8 @@ async def test_homekit_match_full(hass, mock_zeroconf): assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) assert len(mock_service_browser.mock_calls) == 1 - assert len(mock_config_flow.mock_calls) == 1 + # One call to `hue`, one call to `homekit_controller` + assert len(mock_config_flow.mock_calls) == 2 assert mock_config_flow.mock_calls[0][1][0] == "hue" From b508dfd0f64e8e1c54a1a22449df33b512f6f62f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 12 May 2020 17:35:39 +0000 Subject: [PATCH 2/4] Only send updates to homekit controller if the device is paired This avoids the device showing up a second time. --- homeassistant/components/zeroconf/__init__.py | 29 ++++++-- tests/components/zeroconf/test_init.py | 72 +++++++++++++++---- 2 files changed, 85 insertions(+), 16 deletions(-) diff --git a/homeassistant/components/zeroconf/__init__.py b/homeassistant/components/zeroconf/__init__.py index 514af01ec30368..39538fc36e0c22 100644 --- a/homeassistant/components/zeroconf/__init__.py +++ b/homeassistant/components/zeroconf/__init__.py @@ -43,6 +43,10 @@ CONF_DEFAULT_INTERFACE = "default_interface" DEFAULT_DEFAULT_INTERFACE = False +HOMEKIT_PROPERTIES = "properties" +HOMEKIT_PAIRED_STATUS_FLAG = "sf" +HOMEKIT_MODEL = "md" + CONFIG_SCHEMA = vol.Schema( { DOMAIN: vol.Schema( @@ -184,8 +188,25 @@ def service_update(zeroconf, service_type, name, state_change): # still needs to get updates on devices # so it can see when the 'c#' field is updated. # - # If the device already has homekit pairings - # homekit_controller will ignore it. + # We only send updates to homekit_controller + # if the device is already paired in order to avoid + # offering a second discovery for the same device + _LOGGER.debug("info: %s", info) + if ( + HOMEKIT_PROPERTIES in info + and HOMEKIT_PAIRED_STATUS_FLAG in info[HOMEKIT_PROPERTIES] + ): + _LOGGER.debug( + "status_flag: %s", + info[HOMEKIT_PROPERTIES][HOMEKIT_PAIRED_STATUS_FLAG], + ) + try: + if not int(info[HOMEKIT_PROPERTIES][HOMEKIT_PAIRED_STATUS_FLAG]): + return + except ValueError: + # HomeKit pairing status unknown + # likely bad homekit data + return for domain in ZEROCONF[service_type]: hass.add_job( @@ -209,10 +230,10 @@ def handle_homekit(hass, info) -> bool: Return if discovery was forwarded. """ model = None - props = info.get("properties", {}) + props = info.get(HOMEKIT_PROPERTIES, {}) for key in props: - if key.lower() == "md": + if key.lower() in HOMEKIT_MODEL: model = props[key] break diff --git a/tests/components/zeroconf/test_init.py b/tests/components/zeroconf/test_init.py index 153d06f3b80621..9d6d08d5b2751e 100644 --- a/tests/components/zeroconf/test_init.py +++ b/tests/components/zeroconf/test_init.py @@ -18,6 +18,9 @@ NON_ASCII_KEY: None, } +HOMEKIT_STATUS_UNPAIRED = b"0" +HOMEKIT_STATUS_PAIRED = b"1" + @pytest.fixture def mock_zeroconf(): @@ -45,8 +48,8 @@ def get_service_info_mock(service_type, name): ) -def get_homekit_info_mock(model): - """Return homekit info for get_service_info.""" +def get_homekit_info_mock(model, pairing_status): + """Return homekit info for get_service_info for an homekit device.""" def mock_homekit_info(service_type, name): return ServiceInfo( @@ -57,7 +60,7 @@ def mock_homekit_info(service_type, name): weight=0, priority=0, server="name.local.", - properties={b"md": model.encode()}, + properties={b"md": model.encode(), b"sf": pairing_status}, ) return mock_homekit_info @@ -119,12 +122,13 @@ async def test_homekit_match_partial_space(hass, mock_zeroconf): ) as mock_config_flow, patch.object( zeroconf, "HaServiceBrowser", side_effect=service_update_mock ) as mock_service_browser: - mock_zeroconf.get_service_info.side_effect = get_homekit_info_mock("LIFX bulb") + mock_zeroconf.get_service_info.side_effect = get_homekit_info_mock( + "LIFX bulb", HOMEKIT_STATUS_UNPAIRED + ) assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) assert len(mock_service_browser.mock_calls) == 1 - # One call to `lifx`, one call to `homekit_controller` - assert len(mock_config_flow.mock_calls) == 2 + assert len(mock_config_flow.mock_calls) == 1 assert mock_config_flow.mock_calls[0][1][0] == "lifx" @@ -138,13 +142,12 @@ async def test_homekit_match_partial_dash(hass, mock_zeroconf): zeroconf, "HaServiceBrowser", side_effect=service_update_mock ) as mock_service_browser: mock_zeroconf.get_service_info.side_effect = get_homekit_info_mock( - "Rachio-fa46ba" + "Rachio-fa46ba", HOMEKIT_STATUS_UNPAIRED ) assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) assert len(mock_service_browser.mock_calls) == 1 - # One call to `rachio`, one call to `homekit_controller` - assert len(mock_config_flow.mock_calls) == 2 + assert len(mock_config_flow.mock_calls) == 1 assert mock_config_flow.mock_calls[0][1][0] == "rachio" @@ -157,15 +160,60 @@ async def test_homekit_match_full(hass, mock_zeroconf): ) as mock_config_flow, patch.object( zeroconf, "HaServiceBrowser", side_effect=service_update_mock ) as mock_service_browser: - mock_zeroconf.get_service_info.side_effect = get_homekit_info_mock("BSB002") + mock_zeroconf.get_service_info.side_effect = get_homekit_info_mock( + "BSB002", HOMEKIT_STATUS_UNPAIRED + ) assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) + homekit_mock = get_homekit_info_mock("BSB002", HOMEKIT_STATUS_UNPAIRED) + info = homekit_mock("_hap._tcp.local.", "BSB002._hap._tcp.local.") + import pprint + + pprint.pprint(["homekit", info]) assert len(mock_service_browser.mock_calls) == 1 - # One call to `hue`, one call to `homekit_controller` - assert len(mock_config_flow.mock_calls) == 2 + assert len(mock_config_flow.mock_calls) == 1 assert mock_config_flow.mock_calls[0][1][0] == "hue" +async def test_homekit_already_paired(hass, mock_zeroconf): + """Test that an already paired device is sent to homekit_controller.""" + with patch.dict( + zc_gen.ZEROCONF, {zeroconf.HOMEKIT_TYPE: ["homekit_controller"]}, clear=True + ), patch.object( + hass.config_entries.flow, "async_init" + ) as mock_config_flow, patch.object( + zeroconf, "HaServiceBrowser", side_effect=service_update_mock + ) as mock_service_browser: + mock_zeroconf.get_service_info.side_effect = get_homekit_info_mock( + "tado", HOMEKIT_STATUS_PAIRED + ) + assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) + + assert len(mock_service_browser.mock_calls) == 1 + assert len(mock_config_flow.mock_calls) == 2 + assert mock_config_flow.mock_calls[0][1][0] == "tado" + assert mock_config_flow.mock_calls[1][1][0] == "homekit_controller" + + +async def test_homekit_invalid_paring_status(hass, mock_zeroconf): + """Test that missing paring data is not sent to homekit_controller.""" + with patch.dict( + zc_gen.ZEROCONF, {zeroconf.HOMEKIT_TYPE: ["homekit_controller"]}, clear=True + ), patch.object( + hass.config_entries.flow, "async_init" + ) as mock_config_flow, patch.object( + zeroconf, "HaServiceBrowser", side_effect=service_update_mock + ) as mock_service_browser: + mock_zeroconf.get_service_info.side_effect = get_homekit_info_mock( + "tado", b"invalid" + ) + assert await async_setup_component(hass, zeroconf.DOMAIN, {zeroconf.DOMAIN: {}}) + + assert len(mock_service_browser.mock_calls) == 1 + assert len(mock_config_flow.mock_calls) == 1 + assert mock_config_flow.mock_calls[0][1][0] == "tado" + + async def test_info_from_service_non_utf8(hass): """Test info_from_service handles non UTF-8 property keys and values correctly.""" service_type = "_test._tcp.local." From f788c11dd92e4e56108d50a8eb4d57c7e6d1f581 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 12 May 2020 17:37:53 +0000 Subject: [PATCH 3/4] remove debug --- homeassistant/components/zeroconf/__init__.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/homeassistant/components/zeroconf/__init__.py b/homeassistant/components/zeroconf/__init__.py index 39538fc36e0c22..10c0787f72bc7b 100644 --- a/homeassistant/components/zeroconf/__init__.py +++ b/homeassistant/components/zeroconf/__init__.py @@ -191,15 +191,10 @@ def service_update(zeroconf, service_type, name, state_change): # We only send updates to homekit_controller # if the device is already paired in order to avoid # offering a second discovery for the same device - _LOGGER.debug("info: %s", info) if ( HOMEKIT_PROPERTIES in info and HOMEKIT_PAIRED_STATUS_FLAG in info[HOMEKIT_PROPERTIES] ): - _LOGGER.debug( - "status_flag: %s", - info[HOMEKIT_PROPERTIES][HOMEKIT_PAIRED_STATUS_FLAG], - ) try: if not int(info[HOMEKIT_PROPERTIES][HOMEKIT_PAIRED_STATUS_FLAG]): return From db660eaa56075e369896deb6510e5627a3129a67 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 12 May 2020 17:38:45 +0000 Subject: [PATCH 4/4] fix refactor error --- homeassistant/components/zeroconf/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/components/zeroconf/__init__.py b/homeassistant/components/zeroconf/__init__.py index 10c0787f72bc7b..8376ed09e6c1c1 100644 --- a/homeassistant/components/zeroconf/__init__.py +++ b/homeassistant/components/zeroconf/__init__.py @@ -228,7 +228,7 @@ def handle_homekit(hass, info) -> bool: props = info.get(HOMEKIT_PROPERTIES, {}) for key in props: - if key.lower() in HOMEKIT_MODEL: + if key.lower() == HOMEKIT_MODEL: model = props[key] break