Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

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

Merged
merged 2 commits into from Sep 19, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bc333e
[Tradfri] Support for new RGB bulbs #4251
hreichert Sep 15, 2017
2979e20
[Tradfri] Support for new RGB bulbs #4251
hreichert Sep 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 19 additions & 1 deletion ...est/java/org/eclipse/smarthome/binding/tradfri/discovery/TradfriDiscoveryServiceTest.java
View file
Open in desktop
Expand Up @@ -81,9 +81,10 @@ public void cleanUp() {

@Test
public void correctSupportedTypes() {
assertThat(discovery.getSupportedThingTypes().size(), is(2));
assertThat(discovery.getSupportedThingTypes().size(), is(3));
assertTrue(discovery.getSupportedThingTypes().contains(TradfriBindingConstants.THING_TYPE_DIMMABLE_LIGHT));
assertTrue(discovery.getSupportedThingTypes().contains(TradfriBindingConstants.THING_TYPE_COLOR_TEMP_LIGHT));
assertTrue(discovery.getSupportedThingTypes().contains(TradfriBindingConstants.THING_TYPE_COLOR_LIGHT));
}

@Test
Expand All @@ -101,4 +102,21 @@ public void validDiscoveryResult() {
assertThat(discoveryResult.getProperties().get(DeviceConfig.ID), is(65537));
assertThat(discoveryResult.getRepresentationProperty(), is(DeviceConfig.ID));
}

@Test
public void validDiscoveryResultColorLightCWS() {
String json = "{\"9001\":\"TRADFRI bulb E27 CWS opal 600lm\",\"9002\":1505151864,\"9020\":1505433527,\"9003\":65550,\"9019\":1,\"9054\":0,\"5750\":2,\"3\":{\"0\":\"IKEA of Sweden\",\"1\":\"TRADFRI bulb E27 CWS opal 600lm\",\"2\":\"\",\"3\":\"1.3.002\",\"6\":1},\"3311\":[{\"5850\":1,\"5708\":0,\"5851\":254,\"5707\":0,\"5709\":33137,\"5710\":27211,\"5711\":0,\"5706\":\"efd275\",\"9003\":0}]}";
JsonObject data = new JsonParser().parse(json).getAsJsonObject();

discovery.onUpdate("65550", data);

assertNotNull(discoveryResult);
assertThat(discoveryResult.getFlag(), is(DiscoveryResultFlag.NEW));
assertThat(discoveryResult.getThingUID(), is(new ThingUID("tradfri:0200:1:65550")));
assertThat(discoveryResult.getThingTypeUID(), is(TradfriBindingConstants.THING_TYPE_COLOR_LIGHT));
assertThat(discoveryResult.getBridgeUID(), is(new ThingUID("tradfri:gateway:1")));
assertThat(discoveryResult.getProperties().get(DeviceConfig.ID), is(65550));
assertThat(discoveryResult.getRepresentationProperty(), is(DeviceConfig.ID));
}

}
139 changes: 139 additions & 0 deletions ...i.test/src/test/java/org/eclipse/smarthome/binding/tradfri/internal/TradfriColorTest.java
View file
Open in desktop
@@ -0,0 +1,139 @@
/**
* Copyright (c) 2014-2017 by the respective copyright holders.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*/
package org.eclipse.smarthome.binding.tradfri.internal;

import static org.junit.Assert.*;

import org.eclipse.smarthome.core.library.types.HSBType;
import org.eclipse.smarthome.core.library.types.PercentType;
import org.junit.Test;

/**
* Tests for {@link TradfriColor}.
*
* @author Holger Reichert - Initial contribution
*/
public class TradfriColorTest {

@Test
public void testFromCieKnownGood1() {
TradfriColor color = TradfriColor.fromCie(29577, 12294, 254);
assertNotNull(color);
assertEquals(255, (int) color.rgbR);
assertEquals(21, (int) color.rgbG);
assertEquals(207, (int) color.rgbB);
assertEquals(29577, (int) color.xyX);
assertEquals(12294, (int) color.xyY);
assertEquals(254, (int) color.brightness);
}

@Test
public void testFromCieKnownGood2() {
TradfriColor color = TradfriColor.fromCie(19983, 37417, 84);
assertNotNull(color);
assertEquals(110, (int) color.rgbR);
assertEquals(174, (int) color.rgbG);
assertEquals(58, (int) color.rgbB);
assertEquals(19983, (int) color.xyX);
assertEquals(37417, (int) color.xyY);
assertEquals(84, (int) color.brightness);
}

@Test
public void testFromHSBTypeKnownGood1() {
TradfriColor color = TradfriColor.fromHSBType(HSBType.RED);
assertNotNull(color);
assertEquals(254, (int) color.rgbR);
assertEquals(0, (int) color.rgbG);
assertEquals(0, (int) color.rgbB);
assertEquals(45914, (int) color.xyX);
assertEquals(19615, (int) color.xyY);
assertEquals(254, (int) color.brightness);
}

@Test
public void testConversionReverse() {
// convert from HSBType
TradfriColor color = TradfriColor.fromHSBType(HSBType.GREEN);
assertNotNull(color);
assertEquals(0, (int) color.rgbR);
assertEquals(254, (int) color.rgbG); // 254 instead of 255 - only approximated calculation
assertEquals(0, (int) color.rgbB);
assertEquals(11299, (int) color.xyX);
assertEquals(48941, (int) color.xyY);
assertEquals(254, (int) color.brightness);
// convert the result again based on the XY values
TradfriColor reverse = TradfriColor.fromCie(color.xyX, color.xyY, color.brightness);
assertNotNull(reverse);
assertEquals(0, (int) reverse.rgbR);
assertEquals(255, (int) reverse.rgbG); // 255 instead of 254 - only approximated calculation
assertEquals(0, (int) reverse.rgbB);
assertEquals(11299, (int) reverse.xyX);
assertEquals(48941, (int) reverse.xyY);
assertEquals(254, (int) reverse.brightness);
}

@Test
public void testFromColorTemperatureMinMiddleMax() {
// coldest color temperature -> preset 1
TradfriColor colorMin = TradfriColor.fromColorTemperature(PercentType.ZERO);
assertNotNull(colorMin);
assertEquals(24933, (int) colorMin.xyX);
assertEquals(24691, (int) colorMin.xyY);
// middle color temperature -> preset 2
TradfriColor colorMiddle = TradfriColor.fromColorTemperature(new PercentType(50));
assertNotNull(colorMiddle);
assertEquals(30138, (int) colorMiddle.xyX);
assertEquals(26909, (int) colorMiddle.xyY);
// warmest color temperature -> preset 3
TradfriColor colorMax = TradfriColor.fromColorTemperature(PercentType.HUNDRED);
assertNotNull(colorMax);
assertEquals(33137, (int) colorMax.xyX);
assertEquals(27211, (int) colorMax.xyY);
}

@Test
public void testFromColorTemperatureInbetween() {
// 30 percent must be between preset 1 and 2
TradfriColor color2 = TradfriColor.fromColorTemperature(new PercentType(30));
assertNotNull(color2);
assertEquals(28056, (int) color2.xyX);
assertEquals(26022, (int) color2.xyY);
// 70 percent must be between preset 2 and 3
TradfriColor color3 = TradfriColor.fromColorTemperature(new PercentType(70));
assertNotNull(color3);
assertEquals(31338, (int) color3.xyX);
assertEquals(27030, (int) color3.xyY);
}

@Test
public void testCalculateColorTemperature() {
// preset 1 -> coldest -> 0 percent
PercentType preset1 = TradfriColor.calculateColorTemperature(24933, 24691);
assertEquals(0, preset1.intValue());
// preset 2 -> middle -> 50 percent
PercentType preset2 = TradfriColor.calculateColorTemperature(30138, 26909);
assertEquals(50, preset2.intValue());
// preset 3 -> warmest -> 100 percent
PercentType preset3 = TradfriColor.calculateColorTemperature(33137, 27211);
assertEquals(100, preset3.intValue());
// preset 3 -> warmest -> 100 percent
PercentType colder = TradfriColor.calculateColorTemperature(22222, 23333);
assertEquals(0, colder.intValue());
// preset 3 -> warmest -> 100 percent
PercentType temp3 = TradfriColor.calculateColorTemperature(34000, 34000);
assertEquals(100, temp3.intValue());
// mixed case 1
PercentType mixed1 = TradfriColor.calculateColorTemperature(0, 1000000);
assertEquals(0, mixed1.intValue());
// mixed case 1
PercentType mixed2 = TradfriColor.calculateColorTemperature(1000000, 0);
assertEquals(100, mixed2.intValue());
}

}
6 changes: 6 additions & 0 deletions extensions/binding/org.eclipse.smarthome.binding.tradfri/ESH-INF/i18n/tradfri_de.properties
View file
Open in desktop
Expand Up @@ -9,6 +9,8 @@ thing-type.tradfri.0100.label = Dimmbare Lampe (wei
thing-type.tradfri.0100.description = Dimmbare Lampe mit fester Farbtemperatur.
thing-type.tradfri.0220.label = Farbtemperatur Lampe (wei�)
thing-type.tradfri.0220.description = Dimmbare Lampe mit einstellbarer Farbtemperatur.
thing-type.tradfri.0200.label = Farbspektrum Lampe
thing-type.tradfri.0200.description = Dimmbare Lampe mit einstellbarer Farbe und Farbtemperatur.

# thing type configuration
thing-type.config.tradfri.gateway.host.label = IP-Adresse
Expand All @@ -21,9 +23,13 @@ thing-type.config.tradfri.0100.id.label = ID der Lampe
thing-type.config.tradfri.0100.id.description = ID zur Identifikation der Lampe.
thing-type.config.tradfri.0220.id.label = ID der Lampe
thing-type.config.tradfri.0220.id.description = ID zur Identifikation der Lampe.
thing-type.config.tradfri.0200.id.label = ID der Lampe
thing-type.config.tradfri.0200.id.description = ID zur Identifikation der Lampe.

# channel types
channel-type.tradfri.brightness.label = Helligkeit
channel-type.tradfri.brightness.description = Erm�glicht die Steuerung der Helligkeit. Erm�glicht ebenfalls die Lampe ein- und auszuschalten.
channel-type.tradfri.color_temperature.label = Farbtemperatur
channel-type.tradfri.color_temperature.description = Erm�glicht die Steuerung der Farbtemperatur. Von Tageslichtwei� (0) bis Warmwei� (100).
channel-type.tradfri.color.label = Farbe
channel-type.tradfri.color.description = Erm�glicht die Steuerung der Farbe.
29 changes: 29 additions & 0 deletions extensions/binding/org.eclipse.smarthome.binding.tradfri/ESH-INF/thing/thing-types.xml
View file
Open in desktop
Expand Up @@ -71,6 +71,28 @@
</config-description>
</thing-type>

<thing-type id="0200">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<supported-bridge-type-refs>
<bridge-type-ref id="gateway"/>
</supported-bridge-type-refs>

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

<channel id="color_temperature" typeId="color_temperature"/>
<channel id="color" typeId="color"/>
</channels>

<config-description>
<parameter name="id" type="integer" required="true">
<label>ID</label>
<description>The identifier of the light on the gateway</description>
</parameter>
</config-description>
</thing-type>

<!-- note that this isn't yet supported by the code as we do not receive any data from the gateway for it -->
<thing-type id="0820" listed="false">
<supported-bridge-type-refs>
Expand Down Expand Up @@ -108,4 +130,11 @@
<category>ColorLight</category>
</channel-type>

<channel-type id="color">
<item-type>Color</item-type>
<label>Color</label>
<description>Control the color of the light.</description>
<category>ColorLight</category>
</channel-type>

</thing:thing-descriptions>
15 changes: 11 additions & 4 deletions extensions/binding/org.eclipse.smarthome.binding.tradfri/README.md
View file
Open in desktop
Expand Up @@ -12,14 +12,15 @@ The thing type ids are defined according to the lighting devices defined for Zig
|--------------------------|------------------|------------|
| Dimmable Light | 0x0100 | 0100 |
| Colour Temperature Light | 0x0220 | 0220 |

| Colour Light | 0x0200 | 0200 |

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 |
| 0200 | X | X | X | X |

## Thing Configuration

Expand All @@ -29,12 +30,15 @@ The devices require only a single (integer) parameter, which is their instance i

## Channels

All devices support the `brightness` channel, while the white spectrum bulbs additionally also support the `color_temperature` channel (refer to the matrix above).
All devices support the `brightness` channel.
The white spectrum bulbs additionally also support the `color_temperature` channel. Full color bulbs additionally also support the `color` channel.
Refer to the matrix above.

| Channel Type ID | Item Type | Description |
|-------------------|-----------|---------------------------------------------|
| brightness | Dimmer | The brightness of the bulb in percent |
| color_temperature | Dimmer | color temperature from 0%=cold to 100%=warm |
| color | Color | full color |

## Full Example

Expand All @@ -43,14 +47,16 @@ demo.things:
```
Bridge tradfri:gateway:mygateway [ host="192.168.0.177", code="EHPW5rIJKyXFgjH3" ] {
0100 myDimmableBulb [ id=65537 ]
0220 myColorTempBulb [ id=65538 ]
0220 myColorTempBulb [ id=65538 ]
0200 myColorBulb [ id=65539 ]
}
```

demo.items:

```
Dimmer Light { channel="tradfri:0100:mygateway:myDimmableBulb:brightness" }
Dimmer Light { channel="tradfri:0100:mygateway:myDimmableBulb:brightness" }
Color ColorLight { channel="tradfri:0200:mygateway:myColorBulb:color" }
```

demo.sitemap:
Expand All @@ -60,6 +66,7 @@ sitemap demo label="Main Menu"
{
Frame {
Slider item=Light label="Brightness [%.1f %%]"
Colorpicker item=ColorLight
}
}
```
7 changes: 5 additions & 2 deletions ....tradfri/src/main/java/org/eclipse/smarthome/binding/tradfri/TradfriBindingConstants.java
View file
Open in desktop
Expand Up @@ -29,17 +29,20 @@ public class TradfriBindingConstants {

public final static ThingTypeUID THING_TYPE_DIMMABLE_LIGHT = new ThingTypeUID(BINDING_ID, "0100");
public final static ThingTypeUID THING_TYPE_COLOR_TEMP_LIGHT = new ThingTypeUID(BINDING_ID, "0220");
public final static ThingTypeUID THING_TYPE_COLOR_LIGHT = new ThingTypeUID(BINDING_ID, "0200");
public final static ThingTypeUID THING_TYPE_DIMMER = new ThingTypeUID(BINDING_ID, "0820");

public static final Set<ThingTypeUID> SUPPORTED_LIGHT_TYPES_UIDS = Collections.unmodifiableSet(
Stream.of(THING_TYPE_DIMMABLE_LIGHT, THING_TYPE_COLOR_TEMP_LIGHT).collect(Collectors.toSet()));
public static final Set<ThingTypeUID> SUPPORTED_LIGHT_TYPES_UIDS = Collections
.unmodifiableSet(Stream.of(THING_TYPE_DIMMABLE_LIGHT, THING_TYPE_COLOR_TEMP_LIGHT, THING_TYPE_COLOR_LIGHT)
.collect(Collectors.toSet()));

// Not yet used - included for future support
public static final Set<ThingTypeUID> SUPPORTED_CONTROLLER_TYPES_UIDS = Collections.singleton(THING_TYPE_DIMMER);

// List of all Channel IDs
public static final String CHANNEL_BRIGHTNESS = "brightness";
public static final String CHANNEL_COLOR_TEMPERATURE = "color_temperature";
public static final String CHANNEL_COLOR = "color";

// IPSO Objects
public static final String DEVICES = "15001";
Expand Down