-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
Extend knx brightness with rgb brightness if brightness addresses are not supported #31496
Conversation
Hello @FredericMa, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
Hey there @Julius2342, mind taking a look at this pull request as its been labeled with a integration ( |
Sorry for the mess! First PR here... I'll clean this up this evening. |
…d but brightness is not supported.
Codecov Report
@@ Coverage Diff @@
## dev #31496 +/- ##
==========================================
+ Coverage 94.62% 94.65% +0.02%
==========================================
Files 750 756 +6
Lines 54314 54808 +494
==========================================
+ Hits 51396 51877 +481
- Misses 2918 2931 +13
Continue to review full report at Codecov.
|
I need some advise on resolving the linting error below: I do a null check before getting the brightness so it will never crash... |
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. |
It means you are trying to access an value as a list, while it isn't a list. |
Thanks for your reply. |
Aah yes, but that has been fixed some time ago. Please rebase your PR onto the latest |
/AzurePipelines run |
Commenter does not have sufficient privileges for PR 31496 in repo home-assistant/core |
I already deleted the remote branch (stupid me) but it you can also trigger the build manually via comments but I don't have enough privileges to do so (see above). |
I'm happy to retrigger a run, but the PR needs to be rebased really... |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
Got it, I create PR #33152 for this. |
Proposed change
In the documentation of the KNX light is an example to configure a RGB light. This also includes the group addresses for the brightness. My LED controllers only have color addresses (DPT 232.600) and no brightness addresses. So in this case I only configure the color addresses. The consequence is that there is no correct feedback on the set brightness on the color DPT. ALso, if I change the color, the brightness does a fallback to the default brightness (255):
https://github.com/home-assistant/home-assistant/blob/bea7aae8cd87aaef58359383d8c0ac0c34ef6abd/homeassistant/components/knx/light.py#L293-L294
The change proposed provides feedback on the brightness of the selected color and if only the color is changed, it gets the current brightness and doesn't fall back to the default brightness anymore. So 2 fixes for the price of one. 😉
Type of change
Example entry for
configuration.yaml
:Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale: