-
Notifications
You must be signed in to change notification settings - Fork 7.8k
feat(zigbee): Add Temperature Dimmable Light support #12031
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
base: master
Are you sure you want to change the base?
Conversation
👋 Hello Hannes-Beckmann, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
P-R-O-C-H-Y
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Hannes-Beckmann, thank you for your addition to the Zigbee Library.
I have tested using a HA (ZHA) and a C6 with RGB light and it works as a charm.
I have requested a few changes to make the API as close to the other Light endpoints.
...ries/Zigbee/examples/Zigbee_Temperature_Dimmable_Light/Zigbee_Temperature_Dimmable_Light.ino
Outdated
Show resolved
Hide resolved
...ries/Zigbee/examples/Zigbee_Temperature_Dimmable_Light/Zigbee_Temperature_Dimmable_Light.ino
Outdated
Show resolved
Hide resolved
Test Results 76 files 76 suites 18m 15s ⏱️ Results for commit 19a539d. ♻️ This comment has been updated with latest results. |
|
@Hannes-Beckmann What do tou think about extending the ColorDimmableLight with the Temperature color mode, as the light can support both, X/Y or Hue/SAT + Temp? #11654 |
|
Hi, good to hear that the example works as intended. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hi @Hannes-Beckmann, If that are 2 endpoints, then it will how up as 2 lights. Not a single light with both options. |
|
I think we might have misunderstood each other earlier. If temperature support is added directly into ColorDimmableLight, Home Assistant will likely always show a temperature slider, even for users who only want to control an RGB LED strip without any white LEDs. Because of that, I think different classes to match different hardware configurations would be needed. Something like:
Of course, users could emulate missing white channels through some calculations on RGB lights, but that shouldn't affect how these endpoint classes are designed. If a ColorTemperatureDimmableLight is what you are proposing, I would offer to write a PR for that, since I am working on a project that could use this, but this will take some time. |
|
The zippy is actually reading the color_capabilities attribute which should be used later on to show the proper controls - https://github.com/zigpy/zha/blob/7ca9e13db07bd7dff9b0017de3ef58cf993b9430/zha/zigbee/cluster_handlers/lighting.py#L54-L155 I mean that in the ColorDimambleLight, there will be a new method (or set of methods) to specify the color modes or add them 1 by one. I will do some tests and reach back to you. I think that having all those Light classes is just multiplication of something, that can be easily contained in 1 class. |
|
Ahh okay, if that is the case, enabling features would be a much cleaner alternative than duplicating everything. |
Description of Change
This pull request introduces a new Zigbee Temperature Dimmable Light endpoint.
Test Scenarios
I have tested this Pull Request with pioarduino with the esp32-c6-devkitm-1 board on an ESP32-C6-MINI-1U using the ZHA-Integration in Home Assistant.
The included example is not tested since I do not have access to a board with a built-in RGB-LED.
Note that the minimum and maximum temperature is not respected by Home Assistant. Out of range values are clamped.