-
-
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
Add support for "setRGB" command for light entity. #75
Conversation
Don't we need to pull this from the states as well? To show the current color. |
Indeed we do. Added in the last commit. |
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.
Nice work @vlebourl! Just a few comments + the debug strings could be removed. :)
custom_components/tahoma/light.py
Outdated
|
||
if ATTR_HS_COLOR in kwargs: | ||
rgb = color_util.color_hs_to_RGB(*kwargs[ATTR_HS_COLOR]) | ||
self._rgb = [int(float(c)) for c in rgb] |
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.
Can we already set self._rgb
here? What if the command fails, in that case the state is incorrect?
Or should we implement this behavior in all integrations, where we directly update the state, but change it back if the command fails. (which happens via the update() function)
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.
This is one still applies, but is now renamed to self._hs_color
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.
Agreed, self._hs_color
should be set in the update triggered after the turn_on, now that the command awaits for its completion.
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.
isn't that the same for _brightness
and _effect
though?
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.
You are right, let's keep it for now to have the same behavior across this integration.
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, let me push that and we can merge.
#Related Github issues: #75
fix issue #73