Skip to content

Fix KNX rgb(w) color (again)#51060

Merged
marvin-w merged 11 commits intohome-assistant:devfrom
farmio:individual-light-fix
Nov 15, 2021
Merged

Fix KNX rgb(w) color (again)#51060
marvin-w merged 11 commits intohome-assistant:devfrom
farmio:individual-light-fix

Conversation

@farmio
Copy link
Copy Markdown
Member

@farmio farmio commented May 25, 2021

Proposed change

Calculate master brightness from color and scale up color.

As HA treats RGB as 4-channel Light (red, green, blue, brightness) we have to fake the KNX 3-channel RGB color to have brightness not applied twice. Same for RGBW.

At least I think this is how it should work. I don't have any colored lights so I can't test this live. Would be nice if anyone could test this with an actual actuator.

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)

@github-actions
Copy link
Copy Markdown

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Jun 25, 2021
@farmio
Copy link
Copy Markdown
Member Author

farmio commented Jun 25, 2021

@spacegaier I think you have tested this. Shall we use it or close it?

@github-actions github-actions bot removed the stale label Jun 25, 2021
@EricReiche
Copy link
Copy Markdown

Hi, how to test this?
I replaced the /usr/src/homeassistant/homeassistant/components/knx/light.py in my docker container and restarted it. Should that do the trick?
If yes, it didn't work.

@farmio
Copy link
Copy Markdown
Member Author

farmio commented Sep 2, 2021

Docker makes this more complicated. If you have mounted it as volume it may do the trick. Else it will not persist these changes after a container restart (which you need to load it). You can inspect the code in the running container from a shell or just add some print("### hello") and see if it appears in the logs.
Easier way is to install a local test environment and just check out the branch / PR via GitHub Desktop 😉

If it is loaded at least one of the rgb colors should always be 255 in developer tools -> states. At least that was the intention.

@farmio farmio marked this pull request as ready for review October 29, 2021 19:42
@farmio
Copy link
Copy Markdown
Member Author

farmio commented Oct 29, 2021

I did not find anyone to test this on a live system, but I still think it should be the way to go.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Oct 29, 2021

Considering how many branches there are here, it would be good to have some tests for this so it doesn't cause regressions for other light types.

@farmio
Copy link
Copy Markdown
Member Author

farmio commented Oct 29, 2021

I'm just working on writing tests for the knx light module... will take some time - so many possible configurations 🤪

@farmio
Copy link
Copy Markdown
Member Author

farmio commented Nov 1, 2021

Done. I'll rebase this when #58912 is merged.

@farmio farmio requested a review from marvin-w as a code owner November 5, 2021 08:20
Copy link
Copy Markdown
Contributor

@marvin-w marvin-w left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Nov 14, 2021
@marvin-w marvin-w merged commit ca3e672 into home-assistant:dev Nov 15, 2021
@epenet
Copy link
Copy Markdown
Contributor

epenet commented Nov 15, 2021

@epenet
Copy link
Copy Markdown
Contributor

epenet commented Nov 15, 2021

Actually it's more than flaky: it fails everytime on my machine:

FAILED tests/components/knx/test_light.py::test_light_rgbw_individual[pyloop] - AssertionError: assert 'on' == 'off'

Results (3.90s):
      44 passed
       1 failed
         - tests/components/knx/test_light.py:620 test_light_rgbw_individual[pyloop]

@marvin-w
Copy link
Copy Markdown
Contributor

I guess this is a side effect from the library bump. On it.

@marvin-w
Copy link
Copy Markdown
Contributor

#59748

@farmio farmio deleted the individual-light-fix branch November 15, 2021 21:28
natekspencer pushed a commit to natekspencer/home-assistant-core that referenced this pull request Nov 16, 2021
* calculate brightness from color; scale color

* fix merge

* fix sending color only for brightness independent rgb color

* fix tests for rgb and rgbw color

* use public match_max_scale
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix by-code-owner cla-signed integration: knx smash Indicator this PR is close to finish for merging or closing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants