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

Improve Xioami Aqara zeroconf discovery handling #37469

Merged
merged 30 commits into from
Aug 6, 2020

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Jul 4, 2020

Breaking change

Proposed change

Improve the handeling of a zeroconf discovered xiaomi aqara gateway.
This illiminates the need for a network search of the gateway, because the required information is already known.

fix issue with not beeing able to bind to interface if using some operating systems.

ensure listener loop can exit succesfully by setting a timeout and catching that timeout. (I experianced issues with homeassistant not beeing able to close or remove the xiaomi_aqara integration because it hangs forever on data, (ip_add, _) = self._mcastsocket.recvfrom(self.SOCKET_BUFSIZE) (there was no timeout) which prevented the listener loop to exit.

Allow users to mannualy specify an IP an Mac address if both the SSDP and auto-discovery fail.

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:

Config flow

Additional information

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:

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
Copy link

Hey there @Danielhiversen, @syssi, mind taking a look at this pull request as its been labeled with an integration (xiaomi_aqara) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@MartinHjelmare MartinHjelmare changed the title Xioami Aqara: Improve zeroconf discovery handeling Improve Xioami Aqara zeroconf discovery handling Jul 5, 2020
@dshokouhi dshokouhi added the waiting-for-upstream We're waiting for a change upstream label Jul 5, 2020
@starkillerOG
Copy link
Contributor Author

@Danielhiversen You just released PyXiaomiGateway on github and can see it under github releases.
However PyPi still shows 0.12.4 as the latest release and does not show 0.13.0....
Can you fix that?

@Danielhiversen
Copy link
Member

The travis tests failed, so it was not pushed to pypi https://travis-ci.org/github/Danielhiversen/PyXiaomiGateway

@meichthys
Copy link
Contributor

FYI - the build is now passing and pypi is up to date with PyXiaomiGateway v0.13.2

@starkillerOG
Copy link
Contributor Author

@frenck could you add this to the 0.113.3 milestone?
A lot of people are waiting for this fix to be able to use the xiaomi_aqara integration again.

@starkillerOG
Copy link
Contributor Author

Could someone merge this?

homeassistant/components/xiaomi_aqara/translations/ca.json Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_aqara/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_aqara/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_aqara/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_aqara/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_aqara/config_flow.py Outdated Show resolved Hide resolved
starkillerOG and others added 6 commits August 5, 2020 12:13
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@starkillerOG
Copy link
Contributor Author

@MartinHjelmare thanks for the revieuw!
I proccesed all feedback, only the translations I am not sure about.

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare the script.translations develop has been run, can you now approve?

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare Looks like the test failure is unrelated to this PR.
Something with the template binary sensor in this file: tests/components/template/test_binary_sensor.py

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good!

@syssi syssi added this to the 0.114.0 milestone Aug 6, 2020
@frenck
Copy link
Member

frenck commented Aug 6, 2020

@syssi, you've tagged it for 0.114.0, but this is an improvement, not a fix, right?

@starkillerOG
Copy link
Contributor Author

@frenck it is both, it makes some improvements but mainly it fixes an issue that prevents lots of users to setup the integration.
See issues #37306 and #37661 and Danielhiversen/PyXiaomiGateway#179

@frenck
Copy link
Member

frenck commented Aug 6, 2020

Ok, in that case, I'm a bit in the middle. The general policy is that we do not allow for new features/improvements after cutting the beta, only fixes.

@MartinHjelmare What do you think?

@@ -4,9 +4,11 @@
"step": {
"user": {
"title": "Xiaomi Aqara Gateway",
"description": "Connect to your Xiaomi Aqara Gateway",
"description": "Connect to your Xiaomi Aqara Gateway, if the IP and mac addresses are left empty, auto-discovery is used",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having this step, we should do discovery first, and then show a drop down of found entries with "Manual" being one of the options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint, that indeed sounds like a nice improvement.
There already is a drop down list in case multiple gateways are discovered using discovery so schould not be to hard to implement this.

I will not have time in the near future to do this, but I will keep it in mind to do it sometime in the future.

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to put this in the beta as we're 1 day in. Should be enough time to get things fixed.

@balloob balloob merged commit aaad986 into home-assistant:dev Aug 6, 2020
balloob pushed a commit that referenced this pull request Aug 6, 2020
Co-authored-by: Franck Nijhof <git@frenck.dev>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@starkillerOG starkillerOG mentioned this pull request Aug 7, 2020
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple Xiaomi Gateways via Integration page not working
9 participants