-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Fix ZHA wrong cover states #99646
Fix ZHA wrong cover states #99646
Conversation
It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Hey there @dmulcahey, @Adminiuga, @puddly, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested myself, but I believe that this will not cover all cases.
new_pos = kwargs[ATTR_POSITION] | ||
res = await self._cover_cluster_handler.go_to_lift_percentage(100 - new_pos) | ||
self._requested_position = 100 - kwargs[ATTR_POSITION] | ||
res = await self._cover_cluster_handler.go_to_lift_percentage( | ||
self._requested_position | ||
) | ||
if res[1] is not Status.SUCCESS: | ||
raise HomeAssistantError(f"Failed to set cover position: {res[1]}") | ||
self.async_update_state( | ||
STATE_CLOSING if new_pos < self._current_position else STATE_OPENING | ||
STATE_CLOSING | ||
if self._requested_position < self._current_position | ||
else STATE_OPENING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that something is wrong here.
- The old
new_pos
gets 0-100 values. - The new
_requested_position
gets 100-0 values (100 - new_pos
)
Then in the async_update_state
the condition has changed this way:
- old way:
new_pos < self._current_position
(ie: 80 < 50) - new way:
_requested_position < self._current_position
(ie: if new_pos=80, _requested_position=20; 20 < 50)
I don't see how this can work after change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I got confused between _current_position that takes 0 to 100 and go_to_lift_percentage which is the opposite.
I updated this so _requested_position is set in the same way as _current_position and can be compared.
elif ( | ||
self._requested_position | ||
and self._current_position == self._requested_position | ||
): | ||
self._state = STATE_OPEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that some covers are reporting discrete values. In these devices, when you require (ie) a set_position=70 the cover moves to position but it reports (ie) a current_position=69.
If you raise a little more, maybe you get a current_position=75, but you can get all the 0-100 values.
For these devices this approach would not fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this approach won't be good for the devices you mentioned, though, isn't it the point of the quirks to fix the issues related to specific products ? ZHA should only consider the generic case where the blind is sending back the requested position. I don't really see the point of having all the blinds in the wrong state just to avoid some being in the wrong state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far the devices I have seen can't move that exact (typically not a stepper motor) They stop in a range of +/- 1 or 2 for the position, depending on whether they are moving up or down. So you could easily improve it by comparing/allowing the requested position in a small range.
BUT: I don't get the logic here at all! Why should a requested_position=2 (almost fully closed) considered as STATE_OPEN ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 0
is closed and anything else is considered open.
This is adding extra functionality to the existing cover logic, where previously it just retained the same state as before if the position was anything but completely open or completely closed, which I think is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m67hoff Ideally, we would like to have a state Partly_Open which would be anything between Close and Open, but that states doesn't exist. That's why, even opened at 2%, a blind is still Open and not Close.
Will this PR also fix the problem that the cover state only reports "opening" or "closing" when triggered by HA commands, but not when triggered by the device itself (e.g. by pressing the hardwired buttons on the device)? |
I'm not sure. It all depends on what your device is sending back to HA for its position. |
I don't know the technical details, but the cover position is correctly updated in HA. So the device sends the positions. But HA doesn't recognize that position values that are going down means that the cover is closing. |
For reference also check out this draft PR for an alternative solution where tomasbedrich is asking for feedback -> #105467 |
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. |
@GuillaumeB-GitHub any updates on this? |
3bafaac
to
993c4ed
Compare
Hi, I rebased but I think that MR is going to be useless when the MR #108238 is pushed to production. Looking at the code, I think the changes in the code will fixed that bug. Looking at the below function, we can see that the state is set to CLOSE only if the position is 100, otherwise it's OPEN, which is what we want. It's tagged to the version 2024.3.0b3, so I'm guessing this is going to be released soon. If that version fixes the problem, I'll close that MR. |
Duplicate of #102072 which has been merged on October 24th, 2023. This one should be closed. |
Yeah, not a duplicate of that. Cover states are something completely different than cover tilt. |
I can confirm that the latest update fixed the issue reported in that MR (ie: blind states staying at opening or closing if not set to fully opened or fully closed) I'm closing this MR. |
Proposed change
Bug Fix : #98933
Currently, the function async_set_position in the class ZhaCover is not able to manage the case when the cover reaches the desired position, causing the state to stay as Opening or Closing. Any final position that is not 0 should set the state to Open.
This fix saves the requested position so ZHA can know when the requested position has been reached and change the state to Open. Note that we need to clear that requested position when the set positon feature is not used to avoid conflicts when using the Open/Close/Stop features.
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: