Use the shared zeroconf instance for homekit_controller #37691 < 8000 /h1>

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bdraco · 2020-07-09T17:48:01Z

Proposed change

Should fix the cpu issue reported here
https://community.home-assistant.io/t/python3-high-cpu-usage/160012/26?u=bdraco

aiohomekit 0.2.45 supports using the shared zeroconf instance and significantly reduces the time needed to initialize configuration of new homekit controller devices.

Requires Jc2k/aiohomekit#6
Requires Jc2k/aiohomekit#7
Requires Jc2k/aiohomekit#8

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

probot-home-assistant · 2020-07-09T17:48:25Z

Hey there @Jc2k, mind taking a look at this pull request as its been labeled with an integration (homekit_controller) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

Jc2k · 2020-07-09T18:04:49Z

Is this just because of the number of networks zeroconf discovery is happening on or is there something else going on here to make it burn cpu?

bdraco · 2020-07-09T18:09:59Z

Is this just because of the number of networks zeroconf discovery is happening on or is there something else going on here to make it burn cpu?

If you have a lot of mdns traffic, each zeroconf instance is quite expensive. If you end up with two instances in the same python process they can amplify that.

Jc2k · 2020-07-09T19:35:39Z

0.2.43 released with the required change in.

bdraco · 2020-07-09T21:53:11Z

@Jc2k Your approach in Jc2k/aiohomekit#7 (comment) is better. I'll rework this to use that approach unless you want to run with it. Alternatively, feel free to close this or push directly to my remote.

Jc2k · 2020-07-09T22:23:47Z

If you’ve got the time to do it I’d appreciate it. I’m done for the day now but will cut a new tag first thing in the morning, then I probably won’t have any time till the evening again (on BST).

bdraco · 2020-07-09T22:26:32Z

If you’ve got the time to do it I’d appreciate it. I’m done for the day now but will cut a new tag first thing in the morning, then I probably won’t have any time till the evening again (on BST).

Happy to do it. Please ping me when the new tag is cut. I'll have some time tomorrow afternoon to work on it (HST) unless your morning is early enough that I'm still up.

balloob · 2020-07-10T06:39:12Z

homeassistant/components/homekit_controller/manifest.json

@@ -3,7 +3,8 @@
  "name": "HomeKit Controller",
  "config_flow": true,
  "documentation": "https://www.home-assistant.io/integrations/homekit_controller",
-  "requirements": ["aiohomekit[IP]==0.2.41"],
+  "requirements": ["aiohomekit[IP]==0.2.43"],


I think that you can drop [IP].

From our installation logs:

Requirement already satisfied: aiohomekit[IP]==0.2.41 in ./venv/lib/python3.7/site-packages (from -r requirements_all.txt (line 188)) (0.2.41) WARNING: aiohomekit 0.2.41 does not provide the extra 'ip'

👍 - it is originally supposed to have an [IP] feature flag, but it is getting all the deps Home Assistant needs without it and until add aio ble support to it we don't need the flag.

Jc2k · 2020-07-10T07:25:51Z

@bdraco 0.2.44 is out with the changes we discussed. Thanks again for your help with this!

bdraco · 2020-07-10T14:53:44Z

Thanks. Will make adjustments later today.

bdraco · 2020-07-10T15:58:45Z

I can't get homekit controller to discover my iDevices Switch.

bdraco · 2020-07-10T16:02:58Z

I think I broke something in zeroconf.

Edit: fixed here #37725

Also I think there is a timeout in aiohomekit that isn't working.

Digging in with testing

bdraco · 2020-07-10T16:03:56Z

2020-07-10 16:03:15 ERROR (MainThread) [aiohttp.server] Error handling request
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/aiohttp/web_protocol.py", line 418, in start
    resp = await task
  File "/usr/local/lib/python3.7/site-packages/aiohttp/web_app.py", line 458, in _handle
    resp = await handler(request)
  File "/usr/local/lib/python3.7/site-packages/aiohttp/web_middlewares.py", line 119, in impl
    return await handler(request)
  File "/usr/src/homeassistant/homeassistant/components/http/real_ip.py", line 39, in real_ip_middleware
    return await handler(request)
  File "/usr/src/homeassistant/homeassistant/components/http/ban.py", line 73, in ban_middleware
    return await handler(request)
  File "/usr/src/homeassistant/homeassistant/components/http/auth.py", line 127, in auth_middleware
    return await handler(request)
  File "/usr/src/homeassistant/homeassistant/components/http/view.py", line 129, in handle
    result = await result
  File "/usr/src/homeassistant/homeassistant/components/config/config_entries.py", line 136, in get
    return await super().get(request, flow_id)
  File "/usr/src/homeassistant/homeassistant/helpers/data_entry_flow.py", line 92, in get
    result = await self._flow_mgr.async_configure(flow_id)
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 153, in async_configure
    result = await self._async_handle_step(flow, cur_step["step_id"], user_input)
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 201, in _async_handle_step
    result: Dict = await getattr(flow, method)(user_input)
  File "/usr/src/homeassistant/homeassistant/components/homekit_controller/config_flow.py", line 275, in async_step_pair
    discovery = await self.controller.find_ip_by_device_id(self.hkid)
  File "/usr/local/lib/python3.7/site-packages/aiohomekit/controller/controller.py", line 92, in find_ip_by_device_id
    max_seconds=max_seconds, zeroconf_instance=self._zeroconf_instance
TypeError: discover_ip() got an unexpected keyword argument 'zeroconf_instance'

EDIT: Fixed here Jc2k/aiohomekit#8

bdraco · 2020-07-10T17:37:31Z

Will need to bump the version one more time and then it should be good to go

bdraco · 2020-07-10T18:08:10Z

Oh, the package isn't published yet.

Jc2k · 2020-07-10T18:10:46Z

Flaky test whilst I was cooking tea! It should auto publish in a few minutes.

Jc2k · 2020-07-10T18:23:59Z

It's there! https://pypi.org/project/aiohomekit/

bdraco · 2020-07-10T19:43:36Z

All looks good.

@Jc2k
Unrelated (or I missed it) It looks like we currently don't handle the case when an accessory changes ip address but we do handle the c# changing case. We could probably send the ip/port back to aiohomekit in the same block....

Jc2k · 2020-07-10T21:42:57Z

Yep the plan was to do something like that to keep the port and ip up to date in the config entry. Then the call to discover the ip and port in HomeKitConnection probably wouldn’t be needed. Or at least maybe it could only be invoked on retries. Would happily take a PR for it, otherwise it’s on my todo list. I want to try and polish of my programmable stateless switch branch before I start anything else though.

On the topic of zeroconf, I’d also like to expire discoveries that go offline as well. I don’t know if the zeroconf module has anything that could help with that?

Also if we could get the raw data when a zeroconf record is processed then we could pass the raw struct to aiohomekit instead of the device id. That would save us having to do a aiohomekit discovery at all in the step_zeroconf case.

Jc2k

LGTM

bdraco · 2020-07-10T22:18:48Z

Thanks for validating I was reading things correctly.

Right now rediscovering is a feature since there could be significant time from when the device is discovered and when the button is clicked.

zeroconf does generate listener events when an mdns entry is removed/expired so it should be possible to capture those in the future.

Added your notes to my list of things to look into (its a long list now though!).

…#37691)

homeassistant added the cla-signed label Jul 9, 2020

probot-home-assistant bot added integration: homekit_controller small-pr PRs with less than 30 lines. labels Jul 9, 2020

probot-home-assistant bot assigned Jc2k Jul 9, 2020

bdraco requested a review from Jc2k July 9, 2020 17:48

bdraco added the waiting-for-upstream We're waiting for a change upstream label Jul 9, 2020

balloob approved these changes Jul 10, 2020

View reviewed changes

balloob reviewed Jul 10, 2020

View reviewed changes

bdraco added 3 commits July 10, 2020 15:28

Use the shared zeroconf instance for homekit_controller

92d3664

bump version

a42cac2

Update for upstream changes. Thank you @Jc2k !

90e41d1

bdraco force-pushed the homekit_controller_zeroconf_load branch from 44c063b to 90e41d1 Compare July 10, 2020 15:28

naming

85e61ac

bdraco marked this pull request as draft July 10, 2020 17:37

bump version

cf6735b

bdraco removed the waiting-for-upstream We're waiting for a change upstream label Jul 10, 2020

bdraco marked this pull request as ready for review July 10, 2020 18:00

bdraco added the waiting-for-upstream We're waiting for a change upstream label Jul 10, 2020

bdraco removed the waiting-for-upstream We're waiting for a change upstream label Jul 10, 2020

empty commit to reset CI as re-run is not working

763ef74

Jc2k approved these changes Jul 10, 2020

View reviewed changes

bdraco merged commit 5293986 into home-assistant:dev Jul 10, 2020

bdraco mentioned this pull request Aug 8, 2020

Homekit controller can still get stuck in _find_data_for_device_id / not using shared zeroconf #38668

Closed

Jc2k mentioned this pull request Aug 10, 2020

homekit_controller: 'nobuffer' failure for zeroconf when there are lots of interfaces #36733

Closed

hahn-th pushed a commit to hahn-th/core that referenced this pull request May 2, 2025

Add removal instructions to minecraft_server.markdown (home-assistant…

abbd14c

…#37691)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use the shared zeroconf instance for homekit_controller #37691

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Use the shared zeroconf instance for homekit_controller #37691

Uh oh!

Conversation

Uh oh!

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Example entry for `configuration.yaml`: