Skip to content
This repository has been archived by the owner. It is now read-only.

[Tradfri] Support for new RGB bulbs #4251 #4271

Merged

Conversation

@hreichert
Copy link
Contributor

@hreichert hreichert commented Sep 15, 2017

Issue: #4251

Add support for the new TRADFRI full color lamps.

  • New ThingType 0210 / Extended Color Light with additional channel color
  • Extended TradfriLightHandler
    • Handle color setting and retrieving
    • Changed color temperature calculation
  • New class TradfriColor for colorspace conversions
  • Extended TradfriDiscoveryService for automatic discovery of color lights
  • Tests for the above
  • Documentation in README.md

I think it's ready for review.

@hreichert hreichert force-pushed the tradfri-color-support-4251 branch 2 times, most recently from 38fd7f7 to 74f1c1b Sep 15, 2017
Add support for the new TRADFRI full color lamps.

Signed-off-by: Holger Reichert <mail@h0lger.de>
@hreichert hreichert force-pushed the tradfri-color-support-4251 branch from 74f1c1b to 6bc333e Sep 16, 2017
@hreichert hreichert changed the title [WIP] [Tradfri] Support for new RGB bulbs #4251 [Tradfri] Support for new RGB bulbs #4251 Sep 16, 2017
Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Many thanks, this looks pretty good!
All that I would like to ask you for is to remove the brightness channel from the new thing type.

<description>A dimmable light that supports full colors and color temperature settings.</description>

<channels>
<channel id="brightness" typeId="brightness"/>
Copy link
Contributor

@kaikreuzer kaikreuzer Sep 18, 2017

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.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

@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?

Copy link
Contributor

@htreu htreu Sep 18, 2017

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.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

Ah well I think you're right.
Will verify this and update my PR accordingly.

Copy link
Contributor

@sjsf sjsf Sep 18, 2017

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.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

I tried this, but got unexpected results.
My steps:

  • Breakpoints on TradfriLightHandler.handleCommand and ColorItem.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.setState kicks 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">
Copy link
Contributor

@cweitkamp cweitkamp Sep 18, 2017

In case of a color lamp with adjustable color temperature and brightness the corresponding ZigBee ID should be 210 instead of 200.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

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.

Copy link
Contributor

@kaikreuzer kaikreuzer Sep 18, 2017

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 ☺️

Copy link
Contributor

@cweitkamp cweitkamp Sep 18, 2017

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.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

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.

Copy link
Contributor

@cweitkamp cweitkamp Sep 18, 2017

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.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

Yes, 200... All these numbers!
I changed it now to 210 in the binding and the documentation. PR update ongoing with the other changes.

Copy link
Contributor

@kaikreuzer kaikreuzer Sep 18, 2017

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?

Copy link
Contributor

@kaikreuzer kaikreuzer Sep 18, 2017

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.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

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>
@hreichert
Copy link
Contributor Author

@hreichert hreichert commented Sep 18, 2017

Updated with following changes:

  • Renamed ThingType 200 to 210
  • Removed "brightness" channel from ThingType 210
  • Improved color calculations
  • Documentation


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 |
Copy link
Contributor

@kaikreuzer kaikreuzer Sep 18, 2017

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...

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

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.

Copy link
Contributor

@kaikreuzer kaikreuzer Sep 18, 2017

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.

Copy link
Contributor Author

@hreichert hreichert Sep 18, 2017

I vote for option 2 - new PR when the enhancements are done.

Copy link
Contributor

@kaikreuzer kaikreuzer Sep 19, 2017

ok!

@kaikreuzer kaikreuzer merged commit aac8815 into eclipse-archived:master Sep 19, 2017
2 checks passed
@openhab-bot
Copy link
Contributor

@openhab-bot openhab-bot commented Sep 19, 2017

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

@hreichert hreichert deleted the tradfri-color-support-4251 branch Sep 19, 2017
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants