[deconz] Adjust color-temperature handling, Extend bulb support #7900

HahnBenjamin · 2020-06-12T01:55:28Z

Add scaling of upper\lower level for ct values
Add IKEA CWS bulb support

Signed-off-by: Benjamin Hahn [email protected]

TravisBuddy · 2020-06-12T02:01:58Z

Travis tests were successful

Hey @HahnBenjamin,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

TravisBuddy · 2020-06-12T02:18:07Z

Travis tests were successful

Hey @HahnBenjamin,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

cpmeister · 2020-06-12T04:50:03Z

...ding.deconz/src/main/java/org/openhab/binding/deconz/internal/handler/LightThingHandler.java

+    private final int CT_MAX;
+    private final int CT_MIN;


Please make these variable names camelcase.

cpmeister · 2020-06-12T04:52:36Z

...econz/src/main/java/org/openhab/binding/deconz/internal/discovery/ThingDiscoveryService.java

+        properties.put(Thing.PROPERTY_VENDOR, light.manufacturername);
+        properties.put(Thing.PROPERTY_MODEL_ID, light.modelid);
+
+        if (light.ctmax != null)


The standard openhab coding format requires that all if statements should have brackets.

cpmeister · 2020-06-12T04:53:29Z

...econz/src/main/java/org/openhab/binding/deconz/internal/discovery/ThingDiscoveryService.java

+                .withProperty("id", lightID)
+                .withProperty(UNIQUE_ID, light.uniqueid)


Why not put these in the properties map to begin with? It would make things consistent.

cpmeister · 2020-06-12T04:55:08Z

...econz/src/main/java/org/openhab/binding/deconz/internal/discovery/ThingDiscoveryService.java

@@ -129,6 +138,9 @@ private void addLight(String lightID, LightMessage light) {
            case COLOR_DIMMABLE_LIGHT:
                thingTypeUID = THING_TYPE_COLOR_LIGHT;
                break;
+            case COLOR_LIGHT:
+                 thingTypeUID = (light.ctmin != null) ?THING_TYPE_EXTENDED_COLOR_LIGHT :THING_TYPE_COLOR_LIGHT;


please apply formatting here

J-N-K · 2020-06-12T06:09:54Z

Please do not merge this one. I have no time this morning but this will break functionality for some users. I‘ll comment later today.

HahnBenjamin · 2020-06-12T08:38:47Z

Please do not merge this one. I have no time this morning but this will break functionality for some users. I‘ll comment later today.

I assume I must make the new properties known to the existing things. Any pointer or reference on how to do this correcly would be much appreciated

J-N-K · 2020-06-12T15:58:27Z

That would be easy. You can check in initialize if all needed properties are present (or after the initial state was received) and then call a method like updateProperties to set the properties.

Unfortunately the situation is more complex. Your implementations is similar to what I originally implemented. I later deleted that code and set fixed limits for the color temperature.

But there is either a bug in the deconz software or in some Ikea bulbs, where minct is reported as 0 and maxct as 65535. However, if you send a vaöue outside of the defined range (like I have implemented it now), the colormode of the bulb switches to xy and no further ct commands are possible. The only way to recover is to use the deconz gui and do some "read/set property" calls (not sure what exactly un-bricked the bulb).

So you would have to at least check if the reported values for ctmin and ctmax are within the allowed range and use the default range if they are not. Another problem is that for different bulbs a value of say 50 does not represent the same color. IMO the better solution would be to use a custom state description provider and set the min/max values according to the reported (or assumed) range.

I do not understand the change for the extended color light. It should work withput that. Can you show what is reported by the light (response of REST API call for all lights, you can delete all information that does not belong to the light).

TravisBuddy · 2020-06-17T12:55:51Z

Travis tests were successful

Hey @HahnBenjamin,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

J-N-K

Did you see my comment regarding the wrong reporting of ctmax and ctmin? This needs to be taken care of. My suggestion would be: if the reported values are outside of 153-500, set them to 153 or 500 respectively. This should always be ok, since deconz does not accept values outside of that range.

Another idea: If we scale it properly, we could report the color temperature value in K. The conversion can be found here: https://en.wikipedia.org/wiki/Mired

HahnBenjamin · 2020-06-17T16:42:48Z

@J-N-K I was actually looking more deeply into the Ikea CWS light issue and figured out that it actually does not support setting ct value. The ct value is converted into xy-values in deconz as a workaround. I will try to adjust the add-on code accordingly. I will further need to dig a bit deeper into the openhab architecture to find out the internals. I had tested the mired conversion already locally but did not know how to incorporate this. The "custom state description provider" is something i will look for.

For my setup there are only correct ct ranges reported, but incorrect reporting should be considered.
The ct values accepted are actually defined by the ctmin and ctmax values itself.

The adjustments in the extended color light are taking into account the colormode the bulb is currently operating in. During the update the color channel was changing to a value different to what was set. i assume only the state variables of the current colormode are valid for certain bulbs.

J-N-K · 2020-06-17T17:14:57Z

...econz/src/main/java/org/openhab/binding/deconz/internal/discovery/ThingDiscoveryService.java

+        properties.put(Thing.PROPERTY_MODEL_ID, light.modelid);
+
+        if (light.ctmax != null && light.ctmin != null) {
+            properties.put(PROPERTY_CT_MAX, Integer.toString(light.ctmax));


define private static final int CT_MAX = 500; in this class

Suggested change

properties.put(PROPERTY_CT_MAX, Integer.toString(light.ctmax));

int ctmax = light.ctmax > 500 ? 500 : light.ctmax;

properties.put(PROPERTY_CT_MAX, Integer.toString(ctmax));

and the same for min

J-N-K · 2020-06-17T17:21:54Z

...ding.deconz/src/main/java/org/openhab/binding/deconz/internal/handler/LightThingHandler.java

-    private int unscaleColorTemperature(double ct) {
-        return (int) (ct / 100.0 * (500 - 153) + 153);
+    private int unscaleColorTemperature(final double ct) {
+        return (int) (ct / 100.0 * (ct_max - ct_min) + ct_min);


Suggested change

return (int) (ct / 100.0 * (ct_max - ct_min) + ct_min);

int ctMired =(int) (100000.0 / ct);

if (ctMired > ctmax) {

return ctmax;

} else if (ctMired < ctmin) {

return ctmin;

} else {

return ctMired;

}

and the conversion in the other direction below. We can add teh dynamic state description later. You haver to adjust the documentation accordingly (color temperature is not 0->100 but xxx K -> xxx K. If you change that today, we can merge it before the 2.5.6 release and it is not a breaking change since 2.5.5 did not contain light support for deconz.

TravisBuddy · 2020-06-17T17:30:21Z

Travis tests were successful

Hey @HahnBenjamin,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Benajmin Hahn <[email protected]>

See ZigBee Cluster Library Specification Page 5-5 - 327 Signed-off-by: Benajmin Hahn <[email protected]>

Signed-off-by: Benajmin Hahn <[email protected]>

TravisBuddy · 2020-06-18T00:21:17Z

Travis tests were successful

Hey @HahnBenjamin,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

HahnBenjamin · 2020-06-18T10:26:11Z

@J-N-K i tested this version with my local setup and it worked well for various bulbs of different vendors

J-N-K

Otherwise LGTM. Many thanks for your fast Action.

bundles/org.openhab.binding.deconz/README.md

...enhab.binding.deconz/src/main/java/org/openhab/binding/deconz/internal/BindingConstants.java

...ding.deconz/src/main/java/org/openhab/binding/deconz/internal/handler/LightThingHandler.java

bundles/org.openhab.binding.deconz/src/main/resources/ESH-INF/thing/light-thing-types.xml

J-N-K · 2020-06-18T12:30:04Z

@cpmeister Are your concerns already adressed by the last changes?

cpmeister

Apart from @J-N-K requested changes, LGTM

Signed-off-by: Benajmin Hahn <[email protected]>

J-N-K · 2020-06-18T20:36:45Z

Many thanks. Do you want to investigate the state description provider or should I do that?

HahnBenjamin · 2020-06-18T22:00:03Z

Can I find information about it in openHAB Developer Guide?

J-N-K · 2020-06-19T17:46:57Z

Basically you need to implement a DynamicStateDescriptionProvider (like https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.onewire/src/main/java/org/openhab/binding/onewire/internal/OwDynamicStateDescriptionProvider.java), add that as reference to the handler factory, pass it to the thing handler and create a StateDescription (like

openhab-addons/bundles/org.openhab.binding.onewire/src/main/java/org/openhab/binding/onewire/internal/device/AbstractDigitalOwDevice.java

Lines 82 to 87 in 020da77

    
           if (dynamicStateDescriptionProvider != null) { 
        
               StateDescription stateDescription = StateDescriptionFragmentBuilder.create() 
        
                       .withReadOnly(ioConfig.get(i).isInput()).build().toStateDescription(); 
        
               if (stateDescription != null) { 
        
                   dynamicStateDescriptionProvider.setDescription(ioConfig.get(i).getChannelUID(), 
        
                           stateDescription);

). Make sure to remove it on disposal of the handler.

…hab#7900) * Add property for ct colormode upper and lower limit Signed-off-by: Benajmin Hahn <[email protected]>

…hab#7900) * Add property for ct colormode upper and lower limit Signed-off-by: Benajmin Hahn <[email protected]> Signed-off-by: CSchlipp <[email protected]>

…hab#7900) * Add property for ct colormode upper and lower limit Signed-off-by: Benajmin Hahn <[email protected]>

…hab#7900) * Add property for ct colormode upper and lower limit Signed-off-by: Benajmin Hahn <[email protected]> Signed-off-by: Daan Meijer <[email protected]>

…hab#7900) * Add property for ct colormode upper and lower limit Signed-off-by: Benajmin Hahn <[email protected]>

HahnBenjamin requested a review from davidgraeff as a code owner June 12, 2020 01:55

HahnBenjamin force-pushed the deconz branch from 46c4f34 to ae8a067 Compare June 12, 2020 01:56

cpmeister requested changes Jun 12, 2020

View reviewed changes

cpmeister added the enhancement An enhancement or new feature for an existing add-on label Jun 16, 2020

J-N-K requested changes Jun 17, 2020

View reviewed changes

HahnBenjamin closed this Jun 17, 2020

HahnBenjamin reopened this Jun 17, 2020

J-N-K requested changes Jun 17, 2020

View reviewed changes

HahnBenjamin force-pushed the deconz branch from 72c30a6 to 329baeb Compare June 17, 2020 23:46

HahnBenjamin added 12 commits June 18, 2020 02:06

Add property for ct colormode upper and lower limit

9565894

Signed-off-by: Benajmin Hahn <[email protected]>

Expose generic bulb information as properties

6ce0cc8

Signed-off-by: Benajmin Hahn <[email protected]>

Adjust scaling of color temp range to bulb limits

937d3ad

Signed-off-by: Benajmin Hahn <[email protected]>

Adjust indention

dd93435

Signed-off-by: Benajmin Hahn <[email protected]>

Add support for IKEA CWS bulb

14bcfef

Signed-off-by: Benajmin Hahn <[email protected]>

Apply codestyle

0879f4d

Signed-off-by: Benajmin Hahn <[email protected]>

Consolidate property handling

0d8942c

Signed-off-by: Benajmin Hahn <[email protected]>

Apply correct type for "Color lights"

54e063b

Signed-off-by: Benajmin Hahn <[email protected]>

Define CT Constants according to ZCL Spec

f4b62c0

See ZigBee Cluster Library Specification Page 5-5 - 327 Signed-off-by: Benajmin Hahn <[email protected]>

Fix property handling

393905b

Signed-off-by: Benajmin Hahn <[email protected]>

Use Kelvin for color temperature channel

c869c41

Signed-off-by: Benajmin Hahn <[email protected]>

Adjust thing type limits

902ddc3

Signed-off-by: Benajmin Hahn <[email protected]>

HahnBenjamin added 3 commits June 18, 2020 02:08

Fix tests for CT value in kelvin

e64256c

Signed-off-by: Benajmin Hahn <[email protected]>

Apply spotless fixes

7797697

Signed-off-by: Benajmin Hahn <[email protected]>

Adjust documentation

935e304

Signed-off-by: Benajmin Hahn <[email protected]>

HahnBenjamin force-pushed the deconz branch from 329baeb to 935e304 Compare June 18, 2020 00:18

HahnBenjamin requested review from cpmeister and J-N-K June 18, 2020 10:26

J-N-K requested changes Jun 18, 2020

View reviewed changes

cpmeister approved these changes Jun 18, 2020

View reviewed changes

HahnBenjamin added 2 commits June 18, 2020 21:05

Update Readme

1a29543

Signed-off-by: Benajmin Hahn <[email protected]>

Remove apache.commons dependency

774f750

Signed-off-by: Benajmin Hahn <[email protected]>

J-N-K approved these changes Jun 18, 2020

View reviewed changes

J-N-K merged commit 86ad836 into openhab:2.5.x Jun 18, 2020

HahnBenjamin deleted the deconz branch June 18, 2020 22:00

cpmeister added this to the 2.5.6 milestone Jun 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[deconz] Adjust color-temperature handling, Extend bulb support #7900

[deconz] Adjust color-temperature handling, Extend bulb support #7900

HahnBenjamin commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

cpmeister Jun 12, 2020

cpmeister Jun 12, 2020

cpmeister Jun 12, 2020

cpmeister Jun 12, 2020

J-N-K commented Jun 12, 2020

HahnBenjamin commented Jun 12, 2020

J-N-K commented Jun 12, 2020

TravisBuddy commented Jun 17, 2020

J-N-K left a comment •

edited

Loading

HahnBenjamin commented Jun 17, 2020

J-N-K Jun 17, 2020

J-N-K Jun 17, 2020

TravisBuddy commented Jun 17, 2020

TravisBuddy commented Jun 18, 2020

HahnBenjamin commented Jun 18, 2020

J-N-K left a comment

J-N-K commented Jun 18, 2020

cpmeister left a comment

J-N-K commented Jun 18, 2020

HahnBenjamin commented Jun 18, 2020

J-N-K commented Jun 19, 2020 •

edited

Loading

		.withProperty("id", lightID)
		.withProperty(UNIQUE_ID, light.uniqueid)

	properties.put(PROPERTY_CT_MAX, Integer.toString(light.ctmax));
	int ctmax = light.ctmax > 500 ? 500 : light.ctmax;
	properties.put(PROPERTY_CT_MAX, Integer.toString(ctmax));

-        return (int) (ct / 100.0 * (ct_max - ct_min) + ct_min);
+        int ctMired =(int) (100000.0 / ct);
+        if (ctMired > ctmax) {
+            return ctmax;
+        } else if (ctMired < ctmin) {
+            return ctmin;
+        } else {
+            return ctMired;
+        }

[deconz] Adjust color-temperature handling, Extend bulb support #7900

[deconz] Adjust color-temperature handling, Extend bulb support #7900

Conversation

HahnBenjamin commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

Travis tests were successful

TravisBuddy commented Jun 12, 2020

Travis tests were successful

cpmeister Jun 12, 2020

Choose a reason for hiding this comment

cpmeister Jun 12, 2020

Choose a reason for hiding this comment

cpmeister Jun 12, 2020

Choose a reason for hiding this comment

cpmeister Jun 12, 2020

Choose a reason for hiding this comment

J-N-K commented Jun 12, 2020

HahnBenjamin commented Jun 12, 2020

J-N-K commented Jun 12, 2020

TravisBuddy commented Jun 17, 2020

Travis tests were successful

J-N-K left a comment • edited Loading

Choose a reason for hiding this comment

HahnBenjamin commented Jun 17, 2020

J-N-K Jun 17, 2020

Choose a reason for hiding this comment

J-N-K Jun 17, 2020

Choose a reason for hiding this comment

TravisBuddy commented Jun 17, 2020

Travis tests were successful

TravisBuddy commented Jun 18, 2020

Travis tests were successful

HahnBenjamin commented Jun 18, 2020

J-N-K left a comment

Choose a reason for hiding this comment

J-N-K commented Jun 18, 2020

cpmeister left a comment

Choose a reason for hiding this comment

J-N-K commented Jun 18, 2020

HahnBenjamin commented Jun 18, 2020

J-N-K commented Jun 19, 2020 • edited Loading

J-N-K left a comment •

edited

Loading

J-N-K commented Jun 19, 2020 •

edited

Loading