Skip to content

Add Xiaomi Miio vacuum config flow#46669

Merged
MartinHjelmare merged 18 commits intohome-assistant:devfrom
starkillerOG:Miio_vacuum_config_flow
Feb 22, 2021
Merged

Add Xiaomi Miio vacuum config flow#46669
MartinHjelmare merged 18 commits intohome-assistant:devfrom
starkillerOG:Miio_vacuum_config_flow

Conversation

@starkillerOG
Copy link
Copy Markdown
Contributor

@starkillerOG starkillerOG commented Feb 16, 2021

Breaking change

The Xiaomi Miio Vacuum platform now uses Config Flow, the old yaml config is imported to a Config Entry, please remove the yaml config once you see the Loading Xiaomi Miio Vacuum via platform setup is deprecated. Please remove it from your configuration. warning in the log.

Proposed change

Add config flow for the Xiaomi Miio Vacuum platform

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

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

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

@probot-home-assistant
Copy link
Copy Markdown

This pull request needs to be manually signed off by @home-assistant/core before it can get merged.
(message by ReviewEnforcer)

@rytilahti
Copy link
Copy Markdown
Member

I haven't yet tested nor yet looked at the code, but when the manual configuration gets dropped, there needs to be a way to specify the model information if the info query fails. python-miio will currently raise a specific exception if this happens (see rytilahti/python-miio#685) and I know personally that gen1 vacuums w/o cloud connection do have this problem, but there may be other devices that behave similarly.

Another use case for this is to allow supporting devices that are not yet added to the list of supported devices, but which still share (and can function) as they resemble other devices.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@rytilahti I will implement an aditional dorp down to select the model if the initial info call fails.
So a user would first only get the Host and Token field, if they then get an error an aditional model drop down will apear.

The only problem left is then that we also need the mac-adress if the info call fails....
We could use someting like getmac https://pypi.org/project/getmac/ as a fallback to retrieve the mac_adress.

@MartinHjelmare MartinHjelmare changed the title Miio Vacuum Config Flow Add Xiaomi Miio vacuum config flow Feb 17, 2021
Comment thread tests/components/xiaomi_miio/test_vacuum.py Outdated
Comment thread tests/components/xiaomi_miio/test_vacuum.py Outdated
Comment thread homeassistant/components/xiaomi_miio/config_flow.py Outdated
@starkillerOG
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare I think this is ready to be merged

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare could you revieuw again/approve?

Comment thread homeassistant/components/xiaomi_miio/vacuum.py Outdated
Comment thread homeassistant/components/xiaomi_miio/vacuum.py Outdated
@starkillerOG
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare any more suggestions?

@rytilahti
Copy link
Copy Markdown
Member

I don't currently have time to read through & give it a whirl, but having the optional model in the dialog looks good to me (and more importantly, allows myself to keep using the integration with my vacuum ;-)!

If you wish, I can do some testing (and do a brief code review) at some point next week.

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

Looks good!

Let me know when we should merge.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

starkillerOG commented Feb 21, 2021

@rytilahti I just tested this code myself and at least the config flow works.
I can setup a "vacuum" through manually selecting the model, it adds the entity just fine (but of course since I don't have a vacuum, it shows unavailable and will not work (I used a fake IP and token, after getting the first connection error, it does add it if you presist).

I am pretty confident that it will work.
If this is merged I can continue with the rest of the platforms (such as #46866) withouth having to deal with merge conflicts all the time.

@rytilahti I think this can be merged now, if you experiance any problems during testing I can fix that in a small bugfix PR, but as I sad, I am 95% sure it will just work, Do you agree?

@rytilahti
Copy link
Copy Markdown
Member

Sounds good to me @starkillerOG, I trust you (and @MartinHjelmare) on this matter! I'll mark it down as a personal todo to convert my vacuum away from the yaml configuration, and we will simply prepare a fix if necessary.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare this can be merged now

@TheZoker
Copy link
Copy Markdown
Contributor

I just tested the new config flow with my Roborock S5 Max and it works without any issue (I set it up by inserting the IP and the token)👍
I just noticed, that the default device id is xiaomi_device and the device name is Xiaomi Device. Wouldn't a name like Xiaomi Vacuum be better?

@starkillerOG
Copy link
Copy Markdown
Contributor Author

@TheZoker thanks for testing!
The name is general because the config flow is also used for switches, air purifiers air humidifiers etc etc.
In order to have very general code without duplication the name was chosen to be constant. But since the device now has a unique_id you can just change the name and entity_id in the frontend.

@MartinHjelmare MartinHjelmare merged commit 338c07a into home-assistant:dev Feb 22, 2021
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 23, 2021
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.

6 participants