Skip to content
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

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

Merged
merged 2 commits into from Sep 19, 2017

Conversation

Projects
None yet
7 participants
@hreichert
Copy link
Contributor

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 hreichert:tradfri-color-support-4251 branch 2 times, most recently from 38fd7f7 to 74f1c1b Sep 15, 2017

[Tradfri] Support for new RGB bulbs #4251
Add support for the new TRADFRI full color lamps.

Signed-off-by: Holger Reichert <mail@h0lger.de>

@hreichert hreichert force-pushed the hreichert: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

@kaikreuzer
Copy link
Member

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"/>

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 18, 2017

Member

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.

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@htreu

htreu Sep 18, 2017

Contributor

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.

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 18, 2017

Contributor

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.

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@cweitkamp

cweitkamp Sep 18, 2017

Contributor

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

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 18, 2017

Member

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

This comment has been minimized.

Copy link
@cweitkamp

cweitkamp Sep 18, 2017

Contributor

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.

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@cweitkamp

cweitkamp Sep 18, 2017

Contributor

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.

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 18, 2017

Member

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?

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 18, 2017

Member

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.

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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.

[Tradfri] Support for new RGB bulbs #4251
* Renamed ThingType 200 to 210
* Removed "brightness" channel from ThingType 210
* Improved color calculations
* Documentation

Signed-off-by: Holger Reichert <mail@h0lger.de>
@hreichert

This comment has been minimized.

Copy link
Contributor Author

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 |

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 18, 2017

Member

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

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 18, 2017

Member

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.

This comment has been minimized.

Copy link
@hreichert

hreichert Sep 18, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@kaikreuzer

@kaikreuzer kaikreuzer merged commit aac8815 into eclipse:master Sep 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
@openhab-bot

This comment has been minimized.

Copy link
Contributor

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 hreichert:tradfri-color-support-4251 branch Sep 19, 2017

This was referenced Sep 19, 2017

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.