Skip to content

Fix KNX light: turn on color light with only brightness#50979

Merged
emontnemery merged 5 commits intohome-assistant:devfrom
farmio:individual-light-fix
May 24, 2021
Merged

Fix KNX light: turn on color light with only brightness#50979
emontnemery merged 5 commits intohome-assistant:devfrom
farmio:individual-light-fix

Conversation

@farmio
Copy link
Copy Markdown
Member

@farmio farmio commented May 22, 2021

Proposed change

  • Fix assigning individual_color light red address to all channels.

  • Turn on color light with only brightness in kwargs. E.g., from turning on with brightness slider.
    When rgb(w)_color property returns calculate brightness from it.
    If color is not known default to white (3 channel for rgb; only white channel for rgbw).
    If master brightness address is set it will be used - no color sent.
    Previously a individual_color configuration would ignore a turn_on call with only brightness in kwargs.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @Julius2342, @marvin-w, mind taking a look at this pull request as its been labeled with an integration (knx) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@farmio
Copy link
Copy Markdown
Member Author

farmio commented May 22, 2021

@emontnemery I'd have some additional questions regarding color_mode:

  1. When turned on with color information, will there always be brightness too in kwargs? (rgb, rgbw, xy, hsv)
  2. when turned on with brightness slider, what is the expected behavior? White? All channels for rgbw, or just w?
  3. Why can there only brightness information in kwargs when COLOR_MODE_BRIGHTNESS must not be set?
  4. why should COLOR_MODE_BRIGHTNESS not be set in color_modes?

@farmio farmio marked this pull request as ready for review May 23, 2021 21:55
@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented May 24, 2021

@farmio I try to answer the questions:
Q1: When turned on with color information, will there always be brightness too in kwargs? (rgb, rgbw, xy, hsv)
A1: No, the brightness is not guaranteed to be in the kwargs. Likewise, if the light is turned on with only a brightness, there is no color information in the kwargs. In both cases the light entity is expected to remember its brightness or color.

Q2: when turned on with brightness slider, what is the expected behavior? White? All channels for rgbw, or just w?
A2: If the light is sent a turn on call with only brightness set, the light is expected to not change the color.

Q3: Why can there only brightness information in kwargs when COLOR_MODE_BRIGHTNESS must not be set?
why should COLOR_MODE_BRIGHTNESS not be set in color_modes?
A3: COLOR_MODE_BRIGHTNESS means the light's state is defined only by the brightness, that's not the case for a light which supports colors. For a light which supports colors, the light's state is defined by a color and a brightness.

With this in mind, is the PR ready for review?

@farmio
Copy link
Copy Markdown
Member Author

farmio commented May 24, 2021

Q1: When turned on with color information, will there always be brightness too in kwargs? (rgb, rgbw, xy, hsv)

A1: No, the brightness is guaranteed to be in the kwargs.

I guess this is a typo and you meant "not guaranteed"?

If the controller supports saving its last brightness/color it will not be overwritten.
If not (or it's the first turn-on) we default to 100% / white.
I guess saving it in the HA integration would just introduce problems with controllers that store this information themselves or have dedicated turn-on logic.

If this is ok, this PR would be ready for review.

@emontnemery
Copy link
Copy Markdown
Contributor

I guess this is a typo and you meant "not guaranteed"?

Yes, fixed above

If the controller supports saving its last brightness/color it will not be overwritten.
If not (or it's the first turn-on) we default to 100% / white.
I guess saving it in the HA integration would just introduce problems with controllers that store this information themselves or have dedicated turn-on logic.

Yes, that makes sense.

Copy link
Copy Markdown
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@emontnemery
Copy link
Copy Markdown
Contributor

Thanks, @farmio 👍

@emontnemery emontnemery merged commit 331cb3b into home-assistant:dev May 24, 2021
@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented May 24, 2021

Sorry, I missed one detail, why was this needed: c74f12a, that should be OK in Python 3.8 with annotations imported from __future__?

@farmio
Copy link
Copy Markdown
Member Author

farmio commented May 24, 2021

This is what I thought too. But apparently it doesn't hold for generic builtins in cast().
https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-generic-builtins

File "/homeassistant/components/knx/light.py", line 408, in async_turn_on
    await set_color(rgbw[:3], rgbw[3], brightness)
File "/homeassistant/components/knx/light.py", line 369, in set_color
    tuple[int, int, int],
TypeError: 'type' object is not subscriptable

I'd assumed that type argument to cast() is a no-op at runtime, but it seems to evaluate.

@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented May 24, 2021

That's odd, I also expected cast() to be a no-op.
Thanks for confirming though!

@farmio farmio deleted the individual-light-fix branch May 24, 2021 09:51
@github-actions github-actions Bot locked and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants