Fix colortemp conversion for osramlightify#6516
Conversation
Copied from the LIFX fix in 75df4be.
|
@amelchio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @olimpiurob, @tfriedel and @fabaff to be potential reviewers. |
| o_temp = self._light.temp() | ||
| self._temperature = int(TEMP_MIN_HASS + (TEMP_MAX_HASS - TEMP_MIN_HASS) | ||
| * (o_temp - TEMP_MIN) / (TEMP_MAX - TEMP_MIN)) | ||
| self._temperature = color_temperature_kelvin_to_mired(self._light.temp()) |
There was a problem hiding this comment.
line too long (81 > 79 characters)
| self._rgb = (0, 0, 0) | ||
| self._name = "" | ||
| self._temperature = TEMP_MIN | ||
| self._temperature = 2000 |
There was a problem hiding this comment.
While we're doing cleanup here, can you just change self._brightness, self._rgb, self._name, self._temperature, and self._state assignments in __init__ to None? These will get overwritten when self.update() is called, so it's safer/cleaner to initialize them to None since it shouldn't matter if everything is working. It's bad practice to preload fake data in the constructor.
Also if you're feeling extra ambitious you can drop the self.update() call in __init__, and change add_devices_callback(new_lights) to add_devices_callback(new_lights, True) to migrate to the new style and remove IO from the constructor.
@armills: While we're doing cleanup here, can you just change self._brightness, self._rgb, self._name, self._temperature, and self._state assignments in __init__ to None? These will get overwritten when self.update() is called, so it's safer/cleaner to initialize them to None since it shouldn't matter if everything is working.
|
Sure. I did not like putting the 2000 constant in there in the first place. However, I do not have the hardware to test my modifications so I am not too ambitious. |
|
Ah, that's fair enough. I think it's pretty clear at least from a visual inspection that all of those attributes get unconditionally assigned in |
emlove
left a comment
emlove
left a comment
There was a problem hiding this comment.
Looks good, but it wouldn't hurt to get a sanity check from someone with the hardware.
|
The #6498 issue has a success report on my initial commit. |
|
Ah, didn't see that context. I'm just going to give this some time to let other devs take a look before we merge, since it was just opened. |
|
When this is merged, I have a follow up PR ready, to add the lightify group feature to HA, similar to how it is handled with hue. |
|
Thanks 🐬 |
* Fix colortemp conversion for osramlightify Copied from the LIFX fix in 75df4be. * Fix style * Updates from review @armills: While we're doing cleanup here, can you just change self._brightness, self._rgb, self._name, self._temperature, and self._state assignments in __init__ to None? These will get overwritten when self.update() is called, so it's safer/cleaner to initialize them to None since it shouldn't matter if everything is working.
|
cherry-picked for 0.40 |
ghost
added
integration: osramlightify
and removed
platform: light.osramlightify
labels
7 participants
Description:
Related issue (if applicable): fixes #6498
Checklist:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully.