8000 Restore status attribute for xiaomi_vacuum by tamasv · Pull Request #16366 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Restore status attribute for xiaomi_vacuum #16366

8000
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

Merged
merged 2 commits into from
Sep 8, 2018

Conversation

tamasv
Copy link
Contributor
@tamasv tamasv commented Sep 2, 2018

…cuum

Description:

Pre #15643 xiaomi_vacuum component provided specific states ( like "Going to target", "Zoned cleanup" ).
As a side effect of the mentioned PR, device state got simplified, having the same general string "Cleaning" for many specific actions ( like zoned cleanup, spot cleanup and going to target).
This breaks existing automation, which relies on a specific state of the device and limits the toolset for doing a complex script.
I think trying to standardize the vacuum states is a good idea, but I strongly advise against making such a change without depr. warning.

This PR contains two things related to the vacuum component:

  • Added some common states, which could be used by other vacuums, like STATE_SPOT_CLEANING
  • Exposed xiaomi_vacuum raw state through state and state_code attr

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

  • New dependencies have been added to the REQUIREMENTS variable (example).

@tyjtyj
Copy link
Contributor
tyjtyj commented Sep 5, 2018

This break my automation differentiate between spot cleaning and full cleaning

https://community.home-assistant.io/t/xiaomi-vacuum-spot-cleaning-state/66998

@@ -68,6 +68,11 @@
STATE_PAUSED = STATE_PAUSED
STATE_RETURNING = 'returning'
STATE_ERROR = 'error'
STATE_REMOTE = 'remote controlled'
Copy link
Member

Choose a reason for hiding this comment

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

Changes to the entity model need to be discussed and cleared in an issue in our architecture repo.

https://developers.home-assistant.io/docs/en/entity_index.html#changing-the-entity-model

This is the current one about vaccum:
home-assistant/architecture#29

But that one is more or less ready already.

@tamasv
Copy link
Contributor Author
tamasv commented Sep 5, 2018

@MartinHjelmare the original PR #15643 doesn't even mention that "status" attr. will be removed. If you look at the commit messages or the PR description itself, you have like 0 chance to figure out what will happen.
I also find it very interesting, that after someone tried to complain there, the PR just got locked basically limiting any discussion about the current situation.
The mentioed architecture ticket home-assistant/architecture#29 is about providing "general" states for every vacuum component, but again, it should not remove already existing "status" attribute or limit/hide any information which is exposed by the vacuum.
So should I create a new arch. ticket just to get back functionally, which existed for a long time?

@MartinHjelmare
Copy link
Member

Start by commenting in the existing arch issue. If someone thinks a new issue should be opened, they can say so.

We don't discuss in closed PRs.

@rytilahti
Copy link
Member

I'm not sure whether it makes sense to add more "generic states" (maybe it does), but I'll just say that IMO the 'raw status' from the device should be somehow accessible to the user.

@tamasv
Copy link
Contributor Author
tamasv commented Sep 6, 2018

@rytilahti as there is a call for spot cleaning in the generic vacuum class, I think having a STATE_SPOT_CLEANING is a must. Also, having a "STATE_UPDATING" could come in handy because you really don't want to start a cleaning during an update process.

However, I agree that these changes should go through the arch. ticket, so I'll revert every STATE_* modifications and only restore the original "status" call for the xiaomi_vacuum. This will ensure, that we won't break any real-world automation and gives us enough time to do a proper migration to STATE_*.

@tamasv tamasv changed the title Added new states and exposed state/state code received from xiaomi va… Fix, restore status attribute for xiaomi_vacuum Sep 8, 2018
@tamasv
Copy link
Contributor Author
tamasv commented Sep 8, 2018

@MartinHjelmare removed all the changes new STATE_* stuff and restored the "status" attribute for xiaomi_vacuum.
According to the home-assistant/architecture#29 , removing already existing "status" attribute is not the goal, nor the #15643 mentions this explicitly.
@rytilahti agreed, that we should make the "raw" state accessible.

This gives us some time to discuss STATE_* stuff in home-assistant/architecture#29

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.

< 8000 div class="comment-body markdown-body js-comment-body soft-wrap css-overflow-wrap-anywhere user-select-contain d-block">

Looks good!

@MartinHjelmare MartinHjelmare changed the title Fix, restore status attribute for xiaomi_vacuum Restore status attribute for xiaomi_vacuum Sep 8, 2018
@MartinHjelmare MartinHjelmare merged commit 9314338 into home-assistant:dev Sep 8, 2018
@ghost ghost removed the in progress label Sep 8, 2018
@cnrd
Copy link
Contributor
cnrd commented Sep 8, 2018

I just want to say thanks, I was planning to add the status back when I had time, as I can see that a lot of people were having problems with the changes. :-)
The goal of the STATE_* stuff is to have a common baseline for all vacuums so we need to make sure that any STATE_* is shared across most vacuums. (This should be discussed in the Arch issue however).

@balloob balloob mentioned this pull request Sep 17, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
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
0