Skip to content

Add unique_id for Xiaomi Vacuums#34225

Closed
shbatm wants to merge 3 commits intohome-assistant:devfrom
shbatm:xiaomi_vacuum_uid
Closed

Add unique_id for Xiaomi Vacuums#34225
shbatm wants to merge 3 commits intohome-assistant:devfrom
shbatm:xiaomi_vacuum_uid

Conversation

@shbatm
Copy link
Copy Markdown
Contributor

@shbatm shbatm commented Apr 14, 2020

Proposed change

Add a unique_id property for Xiaomi Vacuums.

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

Additional information

Implement the unique_id property for Xiaomi Miio Vacuum devices so they can be renamed and manipulated in the UI better. Unique ID used is the same format as the other Miio device domains (Model-MAC Address).

  • This PR fixes or closes issue: (no issue created) Xiaomi/Miio Vacuums do not implement a unique_id and cannot have the entity_id or friendly_name customized via the UI.
  • This PR is related to issue: N/A
  • Link to documentation pull request: N/A

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. N/A
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt. N/A
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc. N/A

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@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.

@probot-home-assistant
Copy link
Copy Markdown

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

Comment thread homeassistant/components/xiaomi_miio/vacuum.py Outdated
Co-Authored-By: J. Nick Koston <nick@koston.org>
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread homeassistant/components/xiaomi_miio/vacuum.py Outdated
unique_id = None

try:
vacuum = Vacuum(host, token)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only wrap the line where an error could occur.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That would be the info call in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should be cleaned up in the other platforms as well when changes discussed in #32091 (comment) are implemented. They are all inconsistent.

Comment thread homeassistant/components/xiaomi_miio/vacuum.py Outdated
Copy link
Copy Markdown
Member

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

The integration needs refactoring before this can happen. The common code should not be spread around each platform file like it currently is, see #32091 (comment) (and the comments from Martin above) as well as the linked commentary on what should be done.

Now, the second point why this has not been added already is that this will break it for users who do not allow the vacuum to the internet (at least for gen1 users, I have no access to other devices), so there needs to be a solution for those users to configure the unique ID somehow.

@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented Apr 14, 2020

Now, the second point why this has not been added already is that this will break it for users who do not allow the vacuum to the internet (at least for gen1 users, I have no access to other devices), so there needs to be a solution for those users to configure the unique ID somehow.

Why would the info call need to be routed through the internet? I have my Roborock S5 Max blocked from external communications, but can call miIO.info and get a valid response. If retrieving the MAC address can't be relied upon, this should be handled in the python-miio library (just my opinion).

The integration needs refactoring before this can happen.

Bowing out of that. I'll leave this PR request open for now just in case Martin want's to approve the small-pr in the meantime. If not, feel free to close--this was needed for me so I could actually rename the devices in the UI.

@rytilahti
Copy link
Copy Markdown
Member

Why would the info call need to be routed through the internet? I have my Roborock S5 Max blocked from external communications, but can call miIO.info and get a valid response. If retrieving the MAC address can't be relied upon, this should be handled in the python-miio library (just my opinion).

It is not routed through the internet, gen1 vacuums just simply respond with an empty payload if they have no cloud connectivity. However, you are right, this could be fixed in python-miio by handing out a surrogate info when this happens. I'll make a todo to do just that, but no matter whether it's acceptable to merge this before refactoring the integration, it should not be done before a new python-miio version with that fix is released.

@azogue
Copy link
Copy Markdown
Member

azogue commented Apr 17, 2020

Just a question, or a note for the future, @rytilahti and @shbatm,

  • should the addition of the unique_id property be implemented with device_id + device_info properties, so in the next step, we can jump to the device registry too?

@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Apr 17, 2020

@azogue I'm not sure I'm following, could you elaborate? The proper way to my understanding would be to use the mac address obtained from device_info, but I'm unsure how this will go with the "subdevices" (i.e., sensors etc.). I'm looking into adding some a sort of pseudo mac based on the "deviceid" field available in the miio handshake (for that gen1 problem), but it's not a priority/I don't have much time to work on it at the moment.

@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented Apr 18, 2020

I think he's referring to the Home Assistant device_info which is used when you have a config flow (as opposed to YAML), so device information can be added into the HA Device Registry.

I think what he's hinting at is the setup function should save the full result of the miIO.info call and pass it to the device class init function, that way the info can be used for both unique_id and device_info (future) properties in HA without having to make a second call to the device.

https://developers.home-assistant.io/docs/device_registry_index/#device-properties

@azogue
Copy link
Copy Markdown
Member

azogue commented Apr 18, 2020

@azogue I'm not sure I'm following, could you elaborate? The proper way to my understanding would be to use the mac address obtained from device_info, but I'm unsure how this will go with the "subdevices" (i.e., sensors etc.).

As @shbatm pointed out much better than me, I was talking about HA concepts around the config_flow :)

About expanding a physical device like the vacuum into multiple entities in HA related to 1 device, the usual approach is to use the unique id as device_id and generate subentities with unique_id = device_id + postfix.
I'm not a fan of exploding state attributes into their own sensor entities, but for things like the battery level of each device, is the current approach (and makes that batt level visible in the devices table). Anyway, the "subentities explosion" can be made in a different stage.

I'm looking into adding some a sort of pseudo mac based on the "deviceid" field available in the miio handshake (for that gen1 problem), but it's not a priority/I don't have much time to work on it at the moment.

I think the current approach is to use the generated ConfigEntry uuid as device_id/unique_id if there is no way to extract something unique like the MAC, but in this case that could add an extra problem to identify an existent device (the config flow has to avoid multiple configs around the same device!). Another path to explore in those cases could be the access token? or as you say, something linked to the handshake data

I may be able to help you @rytilahti, I have a gen1 vacuum and time to spare :)
I'll try to understand what needs to be done in python-miio, and if I'm able, I'll do a PR there :)

@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Apr 18, 2020

@shbatm @azogue thanks for the clarifications! Yes, I think the whole info object should be passed (it contains also information like hw, firmware versions etc. - https://github.com/rytilahti/python-miio/blob/master/miio/device.py#L25)

Back to the problematic cases; as far as I know this info problem is only happening with non-cloud-connected gen1 vacuums, but there may be also other devices behaving similarly. That being said, I think the best way is to go around this limitation is to create a surrogate info objects when the information fetching fails. This will make handling of these devices transparent to homeassistant, which can trust that there will always be an info object available.

Ignoring rest of the data available in this object, the three potential "seeds" for the mac information are the following:

  • Use real mac address of the device obtained from the network stack. This has the downside for not working with devices that are not directly connected (i.e., need routing) to the homeassistant host.
  • The token. The downside is that this changes on updates and on factory resets.
  • If you run mirobo -d info on your vacuum (or any miio enabled device), you will see the header with a field dubbed device_id. I'm not completely sure if this is unique/different between devices (mine is 02f2a40d for vacuum, 02e60973 for zhimi powerstrip, 10097cf3 for a gosund plug, if you want to compare), but I think this would be the most suitable candidate to fall back to.

@azogue feel free to experiment on this, any help would be greatly appreciated :-)

On the entity explosion, I agree that it (and other refactorings, e.g., the setups should be done using the model info, python-miio needs to be extended to have a proper model<=>class mapping not unlike https://github.com/rytilahti/python-miio/blob/master/miio/discovery.py#L85) should be done in separate PRs. In the end we will want to have a configuration flow for asking the tokens for detected/added devices.

edit: In case the device id in the header is really unique, this could obviously be used instead of the mac address. I checked all devices I have (gen1 vacuum, 2 yeelight color bulbs, yeelight bedside lamp, gosund plug and zhimi smartstrip) where that was the case. Nevertheless, I think we want to have a surrogate info just to simplify the implementation of device type detection.

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 21, 2020

You can always make unique ID optional while figuring out unique ID for the non connected cases.

You can use the config entry ID for devices that don't have a unique ID themselves.

@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Apr 22, 2020

Okay, then I think the best approach is simply to modify python-miio to raise a new type of exception (DeviceInfoException or something similar) that can be catched separately from regular connection errors.

This integration can then be adapted to fallback to config entry id (or other means) while using the device information for devices which support it.

rytilahti added a commit to rytilahti/python-miio that referenced this pull request Apr 23, 2020
…bleException

Enables better control of error cases for downstream users:

* PayloadDecodeException gets raised if the payload cannot be decoded even after all quirks are tried
* DeviceInfoUnavailable gets raised by Device.info() if decoding the miIO.info payload fails

Related: home-assistant/core#34225
rytilahti added a commit to rytilahti/python-miio that referenced this pull request Apr 30, 2020
* Add two new exceptions: PayloadDecodeException and DeviceInfoUnavailableException

Enables better control of error cases for downstream users:

* PayloadDecodeException gets raised if the payload cannot be decoded even after all quirks are tried
* DeviceInfoUnavailable gets raised by Device.info() if decoding the miIO.info payload fails

Related: home-assistant/core#34225

* add tests
@rytilahti rytilahti mentioned this pull request May 16, 2020
@stale
Copy link
Copy Markdown

stale Bot commented Jun 3, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale Bot added the stale label Jun 3, 2020
@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented Jun 8, 2020

Requires upstream changes.

@stale stale Bot removed the stale label Jun 8, 2020
@shbatm shbatm marked this pull request as draft June 8, 2020 00:14
@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Jun 16, 2020

The latest python-miio release from two weeks ago (https://github.com/rytilahti/python-miio/releases/tag/0.5.1) adds two more specific exceptions that should allow this PR to proceed?

So basically wrapping the info call to catch DeviceInfoUnavailableException, and use other means for unique id when the info is not available :-)

@stale
Copy link
Copy Markdown

stale Bot commented Jul 18, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale Bot added the stale label Jul 18, 2020
@stale stale Bot closed this Jul 25, 2020
@axellebot
Copy link
Copy Markdown

This feature is still a needed feature, right ?

@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented Sep 20, 2020

Yes, I just don't have the resources to implement.

xvlady pushed a commit to xvlady/python-miio that referenced this pull request May 9, 2021
…hti#685)

* Add two new exceptions: PayloadDecodeException and DeviceInfoUnavailableException

Enables better control of error cases for downstream users:

* PayloadDecodeException gets raised if the payload cannot be decoded even after all quirks are tried
* DeviceInfoUnavailable gets raised by Device.info() if decoding the miIO.info payload fails

Related: home-assistant/core#34225

* add tests
@shbatm shbatm deleted the xiaomi_vacuum_uid branch December 21, 2022 00:38
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.

9 participants