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

Support knx tunable white and color temperature lights #19699

Merged
merged 13 commits into from Feb 8, 2019

Conversation

Projects
None yet
8 participants
@marvin-w
Copy link
Contributor

marvin-w commented Jan 1, 2019

Description:

This PR intends to add support for tunable white and color temperature for knx lights. It does bump the version of the xknx library to 0.9.4.

Closes #18266

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7979

cc @farmio

Release notes

  • Support for tunable white and color temperature for KNX lights.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.
@Julius2342

This comment has been minimized.

Copy link
Contributor

Julius2342 commented Jan 1, 2019

This PR also incorporates all changes from #18266 (the never ending story).

( @oliverblaha )

@marvin-w marvin-w changed the title Support for tunable white and color temperature for knx lights. [WIP] Support for tunable white and color temperature for knx lights. Jan 6, 2019

@marvin-w marvin-w changed the title [WIP] Support for tunable white and color temperature for knx lights. Support for tunable white and color temperature for knx lights. Jan 15, 2019

@marvin-w

This comment has been minimized.

Copy link
Contributor Author

marvin-w commented Jan 15, 2019

@farmio Can you please test the latest changes and give appropriate feedback? I don't own those kind of devices. My lights still work as before 👯‍♂️

@farmio

This comment has been minimized.

Copy link

farmio commented Jan 15, 2019

works for me ;)
thank you for your help and the PR!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 27, 2019

Is the plan that we merge this instead of #18266?

@marvin-w

This comment has been minimized.

Copy link
Contributor Author

marvin-w commented Jan 27, 2019

@MartinHjelmare yes thats the plan

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 28, 2019

@amelchio you reviewed #18266. Can you have a look at this?

@MartinHjelmare MartinHjelmare changed the title Support for tunable white and color temperature for knx lights. Support knx tunable white and color temperature lights Jan 28, 2019

@amelchio amelchio self-assigned this Jan 28, 2019

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Jan 28, 2019

Okay, will do. Maybe not today, maybe not tomorrow, but soon.

@amelchio
Copy link
Member

amelchio left a comment

Looks good, I have just some small comments.

It also looks complicated but that seems inherent.

if self.device.supports_color_temperature:
kelvin = self.device.current_color_temperature
return color_util.color_temperature_kelvin_to_mired(kelvin) \
if kelvin is not None else DEFAULT_COLOR_TEMPERATURE

This comment has been minimized.

@amelchio

amelchio Feb 1, 2019

Member

Why DEFAULT_COLOR_TEMPERATURE and not None?

This comment has been minimized.

@farmio

farmio Feb 1, 2019

Tbh I just copied the brightness property here. I'll remove DEFAULT_COLOR_TEMPERATURE and test it. After all the KNX actuator should have a default on its own.

I have made some real world tests -> It works just fine with None instead of DEFAULT_COLOR_TEMPERATURE . @marvin-w could you please edit line 196 and 201 accordingly? The constant DEFAULT_COLOR_TEMPERATURE in line 38 isn't used anymore then and can also be removed.

if self.device.supports_tunable_white:
relative_ct = self.device.current_tunable_white
return self.min_mireds + (((255 - relative_ct) / 255) *
(self.max_mireds - self.min_mireds)) \

This comment has been minimized.

@amelchio

amelchio Feb 1, 2019

Member

The Kelvin/mireds relationship is reciprocal, not linear. Since most bulbs work in Kelvin, I expect that this calculation should be done in Kelvin and then converted to mireds(?)

This comment has been minimized.

@farmio

farmio Feb 1, 2019

As Mireds is the go-to unit for HA it seemed unnecessary to calculate this in a foreign unit. Especially since all Mireds (or Kelvin if we changed it) values in this calculation are constants.

This comment has been minimized.

@amelchio

amelchio Feb 1, 2019

Member

It matters. For your default ranges:

  • 2700-6000 Kelvin, 50% is 4350 Kelvin
  • 370-166 mireds, 50% is 268 mireds

However, 4350 Kelvin is 230 mireds.

This comment has been minimized.

@farmio

farmio Feb 2, 2019

🤔 I have implemented this locally now, but I think it's a little counterintuitive. If I set the color temp. to 50% (manually form ETS or any other KNX device) HA would visualize this like in the screenshot. I would assume the slider was centered at 50% ct.
On the other side the sliders from similar relative and absolute ct light are in sync now.

A user thinking in percent (using relative) may have no idea about Mireds/Kelvin relationship and why this matters for his/her lamp.
bildschirmfoto 2019-02-02 um 16 21 14

This comment has been minimized.

@amelchio

amelchio Feb 3, 2019

Member

The slider is indeed not intuitive because it is in mireds while we usually think in Kelvin.

This comment has been minimized.

@farmio

farmio Feb 6, 2019

So shall we leave it as it is for lights that represent their color temperature in percent (outside of the HA world)?
Or shall we go with the correct values but lesser intuitive sliders?

This comment has been minimized.

@amelchio

amelchio Feb 6, 2019

Member

Do it right or you will get error reports that Home Assistant sets the wrong temperature (I have been there, several times).

Fixing the slider is an unrelated issue that could improve the UI for all lights.

Show resolved Hide resolved homeassistant/components/light/knx.py Outdated
if kelvin > self._max_kelvin:
kelvin = self._max_kelvin
elif kelvin < self._min_kelvin:
kelvin = self._min_kelvin

This comment has been minimized.

@amelchio

amelchio Feb 1, 2019

Member

This seems to be the only place that self._min_kelvin and self._max_kelvin is used. You could just use self._min_mireds and remove the Kelvin attributes.

This comment has been minimized.

@farmio

farmio Feb 1, 2019

I used your proposed approach in a previous version. It came to rounding errors when reaching _min_mireds.
If I remember correctly:

  • User set min_kelvin to 2550 -> lowest possible value from HA-plugin to xknx after conversion and comparison in Mireds and conversion to Kelvin was 2551.
  • User set max_kelvin to 6200 -> highest value from HA-plugin to xknx after conversion and comparison in Mireds and conversion to Kelvin was 6211 which exceeds the user setting.

I would like to leave the user defined temperature boundaries in Kelvin because it is the same Unit thats used in the KNX actuator parameterisation.

This comment has been minimized.

@amelchio

amelchio Feb 3, 2019

Member

That's understandable. In fact, it's probably self._min_mireds and self._max_mireds that should be avoided and then convert values going in/out of the platform. But I am also fine with leaving it as-is for now.

This comment has been minimized.

@farmio

farmio Feb 3, 2019

We can't remove self.min_mireds and self.max_mireds propertys because they are needed to override the default properties of the Light class. We could remove the self._min_mireds attributes and put its calculation color_util.color_temperature_kelvin_to_mired(self._max_kelvin)
in the self.min_mireds property.

elif self.device.supports_tunable_white and \
update_color_temp:
relative_ct = 255 - int(255 * (mireds - self.min_mireds) /
(self.max_mireds - self.min_mireds))

This comment has been minimized.

@amelchio

amelchio Feb 1, 2019

Member

The same comment about reciprocal relationship applies here.

@amelchio amelchio removed their assignment Feb 1, 2019

@farmio

This comment has been minimized.

Copy link

farmio commented Feb 1, 2019

@amelchio Thank you for your review!
I second that Kelvin is the unit that is used in KNX and xknx (and most bulbs probably), but it seemed to me that Mireds is the only unit for color temperature in Home Assistant. So I tried to use Mireds in the HA-plugin as a main unit and Kelvin in the xknx device module.
Additionally I tried to minimize calls to color_util.color_temperature_kelvin_to_mired() and its counterpart - mostly for brevity and clarity.

farmio and others added some commits Feb 4, 2019

return None when ct value is unknown
- return None instead of default value when ct is unknown
- remove DEFAULT_COLOR_TEMPERATURE
Merge pull request #1 from farmio/feature/knx-added-support-for-tunab…
…le-white-and-color-temperature-for-lights

review change requests
@homeassistant

This comment has been minimized.

Copy link

homeassistant commented Feb 4, 2019

Hi @farmio,

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!

@homeassistant homeassistant added cla-needed and removed cla-needed labels Feb 4, 2019

farmio added some commits Feb 6, 2019

use Kelvin as base for relative color temperature
use Kelvin as base for relative color temperature instead of Mireds

@farmio farmio referenced this pull request Feb 6, 2019

Merged

review change requests #2

Update request from oliverblaha
Co-Authored-By: marvin-w <marvin@fam-wichmann.de>
mireds = kwargs.get(ATTR_COLOR_TEMP, self.color_temp)

# to make sure that the following calculations always have valid values available,
# fall back to default values for any parameter that has neither been set

This comment has been minimized.

@houndci-bot

houndci-bot Feb 6, 2019

line too long (81 > 79 characters)

hs_color = kwargs.get(ATTR_HS_COLOR, self.hs_color)
mireds = kwargs.get(ATTR_COLOR_TEMP, self.color_temp)

# to make sure that the following calculations always have valid values available,

This comment has been minimized.

@houndci-bot

houndci-bot Feb 6, 2019

line too long (90 > 79 characters)

farmio and others added some commits Feb 6, 2019

Merge branch 'feature/knx-added-support-for-tunable-white-and-color-t…
…emperature-for-lights' into feature/knx-added-support-for-tunable-white-and-color-temperature-for-lights
Merge pull request #2 from farmio/feature/knx-added-support-for-tunab…
…le-white-and-color-temperature-for-lights

review change requests
@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Feb 7, 2019

This seems to be shaping up nicely but I lost track ... are we ready for a second review?

Please note the conflict, probably because the file has moved to components/knx/light.py in dev.

@marvin-w

This comment has been minimized.

Copy link
Contributor Author

marvin-w commented Feb 7, 2019

@amelchio I fixed the conflict - you can give it a second shot. Thanks! :)

@amelchio
Copy link
Member

amelchio left a comment

Looks good! Just one style issue I noticed about accessing config keys.

Show resolved Hide resolved homeassistant/components/knx/light.py Outdated
Show resolved Hide resolved homeassistant/components/knx/light.py Outdated
@amelchio
Copy link
Member

amelchio left a comment

Awesome 🎉

farmio and others added some commits Feb 7, 2019

Update homeassistant/components/knx/light.py
Co-Authored-By: marvin-w <marvin@fam-wichmann.de>
Update homeassistant/components/knx/light.py
Co-Authored-By: marvin-w <marvin@fam-wichmann.de>

@amelchio amelchio merged commit 1e95719 into home-assistant:dev Feb 8, 2019

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 93.356%
Details

@wafflebot wafflebot bot removed the in progress label Feb 8, 2019

@farmio

This comment has been minimized.

Copy link

farmio commented Feb 8, 2019

Hooray!! Thank you Marvin and Amelchio 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment