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

Add Xiaomi Aqara Config Flow #35595

Merged
merged 41 commits into from
Jun 22, 2020
Merged

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented May 13, 2020

Breaking change

YAML configuration of the xiaomi_aqara component has been deprecated, please remove the xiaomi_aqara platform from your configuration.yaml file.

The xiaomi_aqara component now uses the new Config Flow.
Xiaomi Aqara Gateways schould be discovered automatically and show up under "Configuration"-> "Integrations", from there hit configure and go through the steps.
If your Xiaomi Aqara Gateway does not show up automatically, go to "Configuration"-> "Integrations"-> press the "+" icon -> search for "xiaomi_aqara" and enter the setup.

If no key is provided during setup, only the binary_sensor and sensor platforms will be available. A key is required to activate the other platforms.

Proposed change

  • Implement Config Flow
  • Implement the zeroconf discovery using Config Flow
  • Set device_ids and add device information
  • Removed the depricated discovery implementation.

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

@starkillerOG
Copy link
Contributor Author

starkillerOG commented May 13, 2020

To Do:

  • add config flow tests
  • add documentation
  • update .coveragerc
  • add async_unload_entry

@starkillerOG
Copy link
Contributor Author

@Danielhiversen I am not sure if the XiaomiGateway class of the pyxiaomigateway library does I/O?
If so, how can I schedule the initialization of the class in the executor pool?
The normal hass.async_add_executor_job will give me an error that XiaomiGateway is not callable since it is a class and not a function...

@MartinHjelmare MartinHjelmare changed the title Xiaomi Aqara: Add Config Flow Add Xiaomi Aqara Config Flow May 14, 2020
@MartinHjelmare
Copy link
Member

gateway_instance = await hass.async_add_executor_job(
    gateway_class, param1, param2
)

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare this lint test failure is totally unrelated:
tests/components/arcam_fmj/test_device_trigger.py:52:26: F541 f-string is missing placeholders
Think this is something wrong in the dev branch or something.

Could you restart the tests?

@MartinHjelmare
Copy link
Member

Please rebase on latest dev branch to let the build pass.

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare I have fully tested this.
I think it is now ready to be merged.
Only thing left is the unique_id (mac or sid), see the discussion about pros/cons...

@starkillerOG
Copy link
Contributor Author

I think the failed tests are unrelated to this PR, could you confirm @MartinHjelmare

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare any more feedback?

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare can you now approve?

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare tested the code again, and everything seems to work.
I think this is ready for merging.

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!

@MartinHjelmare
Copy link
Member

Please update the breaking change paragraph and mention that if no key is set only the binary_sensor and sensor platforms will be loaded. A key is required to activate the other platforms.

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare I added it to the breaking change

@MartinHjelmare MartinHjelmare merged commit 1f9721b into home-assistant:dev Jun 22, 2020
@lock lock bot locked and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants