[Tradfri] Support for new RGB bulbs #4251 #4271
Conversation
hreichert
force-pushed
the
tradfri-color-support-4251
branch
2 times, most recently
from
38fd7f7
to
74f1c1b
Compare
Add support for the new TRADFRI full color lamps. Signed-off-by: Holger Reichert <mail@h0lger.de>
hreichert
force-pushed
the
tradfri-color-support-4251
branch
from
74f1c1b
to
6bc333e
Compare
hreichert
changed the title
| <description>A dimmable light that supports full colors and color temperature settings.</description> | ||
|
|
||
| <channels> | ||
| <channel id="brightness" typeId="brightness"/> |
There was a problem hiding this comment.
The color channel already accepts all the same (PercentType) commands as the brightness channel, so a thing that has the color channel, should not have the brightness channel as well as this is redundant.
See e.g. also the hue + LIFX bindings where it is the same.
There was a problem hiding this comment.
@kaikreuzer
Ok, I can remove the channel. But I think that we loose functionality:
Imagine a group gWohnzimmer_Brightness for the living room.
In this group we have
- Something_Wohnzimmer_Brightness (something something)
- Tradfri_Wohnzimmer1_Brightness (dimmable bulb)
- Tradfri_Wohnzimmer2_Brightness (dimmable color temperature bulb)
- Tradfri_Wohnzimmer3_Brightness (full color bulb)
Further we have a Slider item=gWohnzimmer_Brightness in the sitemap.
Slide the slider and see how all lamps, also the full color bulb, change their brightness.
Without the brightness channel, one would have to use some rules, I think?
Or do I have a knot in my brain?
There was a problem hiding this comment.
Iirc when sending only the brightness value (0 - 100) to a color item it will be interpreted as brightness only. So in your example all bulbs should change their brightness.
There was a problem hiding this comment.
Ah well I think you're right.
Will verify this and update my PR accordingly.
There was a problem hiding this comment.
You remember correctly! The ColorItem takes care of this: If it gets updated with a percent type, it keeps everything else from the previous state and only adjusts the brightness.
There was a problem hiding this comment.
I tried this, but got unexpected results.
My steps:
- Breakpoints on
TradfriLightHandler.handleCommandandColorItem.setState - Item: tradfri_0200_gwa0cc2b6d64bb_65551_color
- Set a color with BasicUI -> handleCommand receives HSBType -> good
- Send "100" to the same item/channel with BasicUI Slider -> handleCommand receives PercentType -> unexpected?
- AFTER this, the breakpoint on
ColorItem.setStatekicks in
My current code has already a special handling for PercentType on the color channel, so the functionality is OK. But the ColorItem.setState doesn't help here, I think.
What's your opinion about this?
Looking at the HueLightHandler in comparison, there's also special handling for PercentType:
case CHANNEL_COLOR:
if (command instanceof HSBType) {
[...]
} else if (command instanceof PercentType) {
[...]
} else if (command instanceof OnOffType) {
[...]
} else if (command instanceof IncreaseDecreaseType) {
[...]
}
break;
| @@ -71,6 +71,28 @@ | |||
| </config-description> | |||
| </thing-type> | |||
|
|
|||
| <thing-type id="0200"> | |||
There was a problem hiding this comment.
In case of a color lamp with adjustable color temperature and brightness the corresponding ZigBee ID should be 210 instead of 200.
There was a problem hiding this comment.
The real device ID of this lamps is 200 (0x0200), please see #4251 (confirmed on hardware).
Do we take the real device ID, or the device ID that we imagine that is correct?
The whole thingType naming based on technical device IDs, that the IKEA gateway do not even expose, is anyway a bit.. hmm
But that's anoter issue.
There was a problem hiding this comment.
When creating it this way I was hoping that IKEA would stay close to the specification, which would be the best way to figure out the capabilities of the device. This is what we ended up with in the hue binding after many 3rd party devices were supported by it. I'd guess that sooner or later IKEA will come to the same conclusion and our approach will prove to be the right choice
There was a problem hiding this comment.
Sry didn't look into #4251. You already complained your choice. And this is another break with the ZigBee protocol caused by IKEA developers.
To be compliant I would suggest to use the same thing-type definition as we use in the Hue and other bindings. In this case it should be the device ID of what we think the bulb is capable of -> 210. If not, we should remove the color_temperature channel as well (where later one is IMHO the worse way).
btw. I didn't mentioned it before: You did a great job so far.
There was a problem hiding this comment.
My personal feeling is also that 210 is the right choice, and not 200. But as the real device ID was 210, I renamed it so stick with the previous naming scheme.
I'm happy to re-rename it to 210, cause this better reflects the capabilities of the Thing.
P.S: After re-reading my previous comment, it sounded a bit offensive. That was not intentional.
There was a problem hiding this comment.
But as the real device ID was 210, ...
You mean 200, I guess. ;-) Okay, we give it a try.
... it sounded a bit offensive.
Oh no, everything was alright. I am fine with it.
There was a problem hiding this comment.
Yes, 200... All these numbers!
I changed it now to 210 in the binding and the documentation. PR update ongoing with the other changes.
There was a problem hiding this comment.
Hm, my comment above actually meant that we SHOULD go for the real Zigbee type, because sooner or later other devices will appear with the 210 type - and maybe they then need a different logic to be supported (e.g. hardware instead of software color temp setting). So I was actually fine with the "200" choice.
If not, we should remove the color_temperature channel as well
@cweitkamp The other option would be that we think about doing "software calculation" on those (hue) as well - do you think that would be possible?
There was a problem hiding this comment.
But I don't have a too strong opinion on 200 vs. 210 - if you both now agreed that 210 is the better choice, so be it. We can still revisit this once we see more devices that bring us more clarity on how the gateway handles them.
There was a problem hiding this comment.
Well, @cweitkamp has also good arguments.
I'm absolutely against removing the color_temperature channel from the new thing. See my discussion above; but replace "brightness" with "color temperature". And this can not be handled with the color channel, at least not without rules or something.
* Renamed ThingType 200 to 210 * Removed "brightness" channel from ThingType 210 * Improved color calculations * Documentation Signed-off-by: Holger Reichert <mail@h0lger.de>
|
Updated with following changes:
|
|
|
||
| The following matrix lists the capabilities (channels) for each of the supported lighting device types: | ||
|
|
||
| | Thing type | On/Off | Brightness | Color | Color Temperature | | ||
| |-------------|:------:|:----------:|:-----:|:-----------------:| | ||
| | 0100 | X | X | | | | ||
| | 0220 | X | X | | X | | ||
| | 0210 | X | | X | X | |
There was a problem hiding this comment.
I am noticing that this table was already incorrect - neither 0100 nor 0220 nor 0210 should have an X for On/Off... - so we could actually remove the whole column...
There was a problem hiding this comment.
Very good point, as there is no "onoff" channel. Should I open a new issue for this? I want to extend the documentation anyway with more examples.
There was a problem hiding this comment.
You could simply remove the column as part of this PR - or we first merge and you can come up with a new PR that enhances the documentation in general.
There was a problem hiding this comment.
I vote for option 2 - new PR when the enhancements are done.
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/ikea-tradfri-gateway/26135/118 |
Issue: #4251
Add support for the new TRADFRI full color lamps.
I think it's ready for review.