Skip to content
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.

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

Homekit controller component to use zeroconf discovery #24042

Merged

Conversation

Projects
None yet
5 participants
@Kane610
Copy link
Contributor

commented May 22, 2019

Description:

Related issue (if applicable): fixes home-assistant/architecture#198 (comment)
home-assistant/home-assistant.io#9506

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@Kane610 Kane610 requested a review from Jc2k as a code owner May 22, 2019

@Kane610 Kane610 changed the title Homekit controller component to user zeroconf discovery Homekit controller component to use zeroconf discovery May 22, 2019

@codeowners-notifier

This comment has been minimized.

Copy link

commented May 22, 2019

Hey there @Jc2k, mind taking a look at this pull request as its been labeled with a integration (homekit_controller) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Nice one! Happy with the changes to hk_controller. Will try and test resetting a HK device and doing the discovery / pairing dance with this in place tonight to make sure everything is still happy.

Is this a breaking change?

  • Users will have to enable zeroconf in their configs?
  • Users will have to remove homekit from either ignore or enable in their discovery config?

For hk_controller its a bit more annoying because discovery triggers entities showing up at all. So it won't just break the initial setup but peoples existing working setups. (This will actually be fixed in 0.94 by us moving to config entries, but discovery still has to happen at least once after upgrading to 0.94 to create a proper config entry).

I didn't think of this to ask on the original PR but:

  • Should the discovery component have a manifest dependencies on zeroconf? So all discovery users get grandfathered in to zeroconf, at least temporarily.
  • Should zeroconf respect discovery.ignore? Or have its own similar setting?
@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Nice one! Happy with the changes to hk_controller. Will try and test resetting a HK device and doing the discovery / pairing dance with this in place tonight to make sure everything is still happy.

Is this a breaking change?

  • Users will have to enable zeroconf in their configs?

I actually think it is enabled as a part of the default configuration now. I did a fix with tests for that at least.

  • Users will have to remove homekit from either ignore or enable in their discovery config?

Good point, that is a breaking change

For hk_controller its a bit more annoying because discovery triggers entities showing up at all. So it won't just break the initial setup but peoples existing working setups. (This will actually be fixed in 0.94 by us moving to config entries, but discovery still has to happen at least once after upgrading to 0.94 to create a proper config entry).

I didn't think of this to ask on the original PR but:

  • Should the discovery component have a manifest dependencies on zeroconf? So all discovery users get grandfathered in to zeroconf, at least temporarily.

Wouldn't that make issues with other protocols not zeroconf?

  • Should zeroconf respect discovery.ignore? Or have its own similar setting?

Good question, right now there is no such thing. But it doesn't notify either so it won't be intrusive right?

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Not HomeKit but a counter example - I know I turn discovery of Apple TVs off on my dev instance because it was turning my TV in another room on. I found that more annoying than the notify thing.

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Not HomeKit but a counter example - I know I turn discovery of Apple TVs off on my dev instance because it was turning my TV in another room on. I found that more annoying than the notify thing.

Apple TV integration is not config entry, thus not moved to zeroconf

@balloob

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Can we update the discovery integration to still accept 'homekit' as a valid ignore value? That way it's not a breaking change.

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

In the case of homekit_controller, it still needs to be an acceptable value for enable as homekit_controller used to be opt in (see #23625).

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Did some testing with a mock homekit accessory. All working, but there is one gotcha. If i turn the accessory on and off I get duplicate active flows under "Discovered":

Screenshot 2019-05-22 at 20 34 47

Is this a bug in the homekit_controller bit or in the new zeroconf stuff? I think the value of properties might change on restart but the name is still the same / ip / mac / port / etc is still the same.

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

A new added signal? That could generate a second one I guess. Do we need some way to check if a discovery exist that is equal to the new discovered one?

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I will try to see what happens exactly after ive done some chores, but yes I think we need to check if a discovery exists already. I think the foo._hap._tcp.local. name is unique on a network, maybe we can de-duplicate on that? Or on the IP and port?

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@Jc2k going to sleep, will be gone for the weekend.

So name should be the unique identifier? It is unique in zeroconf I believe so that would work, problem is that sometimes it happens that it registers a second one ending with a 'name'-2

@Kane610 Kane610 force-pushed the Kane610:zeroconf-discovery-homekit_controller branch from 9ee5d9d to 325b22a May 24, 2019

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@balloob I will fix discovery for axis, esphome, HomeKit and trådfri so we don't introduce a breaking change by discovery configuration. Will fix this on Sunday

@balloob

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@Jc2k I would expect it to be up to HomeKit config lfow to figure out that a flow is already in progress. This is how I do it for Hue: https://github.com/home-assistant/home-assistant/pull/24090/files#diff-e2b998653dcc963eb01149d36ba03d15R151

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

OK cool - i've gone and done something like that based on the homekit accessory id here:

Jc2k@40b2384

This solves the dupe problem for homekit devices. I added tests to keep config flow at 100%. Can either cherry pick it into this PR or I can open a fresh one after this is merged. With this commit and changes to avoid this refactor being a breaking change I am a happy bunny.

@Jc2k

Jc2k approved these changes May 25, 2019

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

Should we change that failing test to just assert that "some" flows were loaded (i.e. > 0) so its less brittle?

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Just create a new PR to improve homekit 👍🏻

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

In light of #24115, what's the plan here? Are we still proceeding with this PR? If so i'll leave Jc2k/home-assistant@40b2384 until this is merged. If we are aborting on migrating for now I can rebase my commit against dev and get a PR open.

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

@Jc2k we are proceeding, but we need to revert to the sync library

@balloob

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Yeah, we are moving forward with the zeroconf integration doing scanning. It's a lot more flexible than the old discovery approach.

@balloob balloob merged commit 42ee8ee into home-assistant:dev May 29, 2019

8 of 12 checks passed

build Workflow: build
Details
ci/circleci: test 3.5.5 Your tests failed on CircleCI
Details
ci/circleci: test 3.6 Your tests failed on CircleCI
Details
ci/circleci: test 3.7 Your tests failed on CircleCI
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
home-assistant Build #20190524.36 succeeded
Details

@Jc2k Jc2k referenced this pull request May 29, 2019

Merged

Fix discovering the same HomeKit device multiple times #24178

4 of 4 tasks complete

@Kane610 Kane610 deleted the Kane610:zeroconf-discovery-homekit_controller branch May 29, 2019

@balloob balloob referenced this pull request Jun 4, 2019

Merged

0.94.0 #24305

@dasb00ter

This comment has been minimized.

Copy link

commented Jun 29, 2019

Would like to get rid of the notification that pops up now. I have not enabled zeroconfig and I'm seeing similar. I don't own a single Apple product. How can I turn this off. I have manually configured my nanoleaf lights ages ago.

Here is the notification:
HomeKitt Accessory: Nanoleaf Light Panels redacted
HomeKit Accessory: Nanoleaf Light Panels redacted
HomeKit Accessory: Nanoleaf Light Panels redacted
HomeKit Accessory: Nanoleaf Light Panels redacted

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

This isn’t a homekit issue, the other ticket you commented on is the right place for this issue.

HomeKit is just a protocol, you don’t need an Apple device to have HomeKit capable devices. So it’s not a bug that we detect compatible products. It’s a bug that you can’t ignore them satisfactorily, though. Which is what the other ticket is about.

@dasb00ter

This comment has been minimized.

Copy link

commented Jun 29, 2019

Isnt it a zeroconfig issue? I didnt have a problem before this pull request are you stating its not related?

@Jc2k

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

This PR let’s homekit use zeroconf, but that zeroconf doesn’t support the same enable/disable switches as the discovery module (nor have an alternate ignore feature) is a separate thing.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jun 29, 2019

@balloob

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

PRs are not the right place to discuss these things. Please discuss in issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.