-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Split update cover method #106
Conversation
If OK, I will also move the code from |
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 @tetienne, looks good. A few minor comments.
return self._position == 0 | ||
|
||
if getattr(self, "_tilt_position", None) is not None: | ||
if self._tilt_position is not None: |
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.
To be checked, but IMO the is_closed is only related to the position not the tilt position.
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 own a device (BioclimaticPergola) which only has slats that could be tilted (or closed). In that case only tilt_position
exists.
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.
OK I understand. In home assistant, it will ease its usage indeed. In the UI, there is no button to close or open the tilt? And their is no visual indication if your pergola is opened or closed without this trick?
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.
There is a button to open and close the tilt, see open_cover_tilt
and close_cover_tilt
. Based on SUPPORTED_FEATURES the required buttons are shown in HA.
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.
Toggling SUPPORTED_FEATURES based on the available commands, this is very dynamic and supports multiple device types without any hardcoded list.
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.
How does it look like without this fix? Sorry for my question, but often due to little hack, it's hard to understand the code later.
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 is not really a hack, it first does the check if it is closed based on the state. If the state is not available, it has a look at the current position. If the current position is unknown, it falls back to the tilt position.
I've updated the PR according to your remarks. |
To ease the maintenance and future improvement split the huge update method in several smaller parts.