Fix for issue 1861

[sverleysen]

Commenting out this line of code should fix this issue.
#1861

[manup]

Thanks for the PR, does the ct mode then still work for the IKEA CWS color light?

[sverleysen]

I don't know, I don't have these lights. This change I have done is a change of @Flurkmark. He is using this change for a while now without issues.

[lbschenkel]

I do have these lights, and I had introduced that code you're commenting it out. Commenting it out breaks the ct->xy transformation, which makes ct not work for the CWS lights.

I'm not quite sure if I understand what is the original issue, maybe I can recommend an alternative fix and help testing it (I have both WS and CWS lights).

[sverleysen]

Initially a CT light bulb is correctly processed as a CT bulb, but when changing the color temperature using the REST API (what happens when you use Home Assistant), Deconz reports the same light bulb as a XY bulb.
Which results in not be able to get the current color temperature in Home Assistant. When changing the color_temp using the Deconz UI, the light is again a CT bulb and the color_temp attribute is available again in Home Assistant

[sverleysen]

[lbschenkel]

Thank you. I also use Home Assistant. I will try to replicate and get back, and hopefully propose some alternative.

[sverleysen]

Thank you. I also use Home Assistant. I will try to replicate and get back, and hopefully propose some alternative.

Any success in replicating the issue?

[lbschenkel]

Hi, sorry for the delay. With the coronavirus and some other personal stuff going on, I couldn't take a look at it. But now I have.

I could replicate your video: When I turn on the light on with the physical switch, I do see the color_temp attribute. But then if I change the color temperature of the light (unicast), either via HASS or Phoscon, the attribute disappears. If I change it via the group (multicast), then it reappears.

The reason for why this happens is because IKEA lights have a firmware bug in which they don't remember the color temperature in scenes, unless the colour is set via xy [1][2][3]. That's why the use of xy is forced for all IKEA lights, but only on unicast commands.

On top of that, I introduced [4] to use the same mechanism to allow using ct commands on lights lacking the ct capability but supporting colour (like the IKEA CWS lights), by translating ct into xy.

Commenting out the line you proposed in this PR would cancel the first behaviour I mentioned above, which would be a pretty annoying regression: when saving scenes, IKEA lights won't remember their light colour (unless they recently fixed this bug, but I saw no evidence of that).

Now, I totally understand this is frustrating and causes this side effect. I think this could be fixed by making deCONZ pretend it sent the original ct command and update the REST API status accordingly, or since it supports emulating ct anyway for lights that lack this capability, do the opposite translation and report a color_temp for all lights.

I'll need to dig into the source code again and check what's feasible. @manup, @ebaauw: any other suggestions?

[1] 4a43cb9
[2] #629
[3] #53
[4] #719

[ebaauw]

I'm afraid, you don't want to ask me in this case; I'm no fan of this functionality.

See my post on the API v2 (#1688) about a "raw" vs a "cooked" API:

I like the idea of a layered architecture, where the "raw" low-level v2 API would be RESTifying the ZCL commands and attributes (cf. the level of the GUI interface and the CLI plugin), on top of which which the high-level v2 API would provide the "cooked" home automation features (caching values, rules, ...). We might even consider developing these layers as two separate repositories. On the long run, the high-level API v2 might be used with implementations of the low-level API v2, e.g. using the RaspBee/ConBee serial interface to the radio device, or using other hardware (e.g. XBee). And the Hue-compatible v1 API might eventually be based on the same low-level v2 API.

I think the current API is more "raw" than "cooked", and should stick to exposing the lights as they are, warts and all. Imho, introducing "cooked" features like mapping ct to xy in the current code is doing more harm than good: it causes too much confusion and is hardly maintainable, lacking a proper capability model.

[lbschenkel]

I'm afraid, you don't want to ask me in this case; I'm no fan of this functionality.

See my post on the API v2 (#1688) about a "raw" vs a "cooked" API:

I have read it back then and I actually agree with you. However, the current situation is what it is, and the 2nd cooked layer does not currently exist.

The mapping from ct to xy was introduced by @manup to work around the IKEA firmware bug: if that is disabled, scenes will simply stop working for any IKEA white spectrum lights (in the sense of not saving properly) [*]. This will be a regression big enough to be unacceptable to many users, myself included. What else can we do in the current set of circumstances?

If we can't find a way for the REST API to keep the firmware bug workaround, the only other thing I can think of is that we add a way to enable/disable "quirks" from the command line, so the set of users who don't care about scenes can disable (let's say) quirk.ikea.ct_to_xy.

[*] the other circumstance, when the light does not support ct but does xy, is actually not kicking in nor it is a problem in this particular situation

[ebaauw]

What else can we do in the current set of circumstances?

I appreciate the dilemma. I’m sorry, I don’t have an answer. If memory serves, there was a similar problem for OSRAM colour temperature lights, but they don’t accept xy commands. I think they worked correctly when storing a scene from the current light state, but not when creating a scene while specifying the state (I need to brush up on the Scenes cluster commands).

[lbschenkel]

What else can we do in the current set of circumstances?

I appreciate the dilemma. I’m sorry, I don’t have an answer. If memory serves, there was a similar problem for OSRAM colour temperature lights, but they don’t accept xy commands. I think they worked correctly when storing a scene from the current light state, but not when creating a scene while specifying the state (I need to brush up on the Scenes cluster commands).

Interesting. Unless I'm misremembering it, the IKEA lights have the opposite bug in which they don't correctly store a scene from the current light state (which is the way you do it via deCONZ), unless the current light state was set via xy.

If the lights work correctly when you use "create scene", then it's possible that the workaround could be implemented for the scene creation step instead. This could have the additional benefit of working properly even when you set the light state via group commands, contrary to what happens today.

[ebaauw]

I replaced the IKEA and OSRAM bulbs on my production network with Hue bulbs (because of the routing problems), but I still have them lying around, somewhere. I’ll see if I can do some tests over the weekend.

which is the way you do it via deCONZ

I think deCONZ also supports the other command, when you update the scene state. One of the hidden features in the API:

deconz-rest-plugin/rest_groups.cpp

Line 93 in ca72c46

// PUT, PATCH /api/<apikey>/groups/<group_id>/scenes/<scene_id>/lights/<light_id>/state

[lbschenkel]

Off-topic P.S.: This reminds me of an old idea that I have which is to introduce a device database to deCONZ.

This device database will have entries mapping from a set of rules (MAC prefix, vendor, capabilities, firmware version, etc.) to a specific device entry. A device entry will have general information such as the device name, manufacturer, but most importantly: a set of quirks that apply.

Then in every case in the code in which it has a special case or alternative behaviour guarded by if(macPrefix == ...) or if(manufacturer == VENDOR_IKEA) could be substituted by something like if(dev.quirks.force_ct_to_xy). I feel that this would immensely improve the maintainability of the codebase, plus minimize or eliminate the "whitelisting dance" that is currently done for new devices.

Because at the beginning deCONZ might not have enumerated enough data to have an ideal mapping, every time new information is discovered that results in the device entry changing (let's say: from "IKEA light" to "IKEA GU10 WS light (firmware 3.x)" then the current best entry is saved to the DB.

Device entries can be listed in order of most specific to most generic, and the first matched entry wins. So we could have a separate entry for lights with newer firmware that has less/different quirks than the same light with an old firmware. To make it manageable, we could also allow entries to specify a parent entry, so they inherit all the parent's quirks unless the device forcefully disables the quirk (example: "IKEA GU10 WS light" can inherit "IKEA WS light" which inherits from "IKEA light").

[ebaauw]

Indeed, this is what I mean by capability model. Preferably, I would be a JSON file, so new devices can be supported without code changes. I have something like that in Homebridge Hue, but very rudimentary and still in code. You can specify default quirks per manufacturer, overriding them per model, if needed.
https://github.com/ebaauw/homebridge-hue/blob/b8a7aaf25f2a5ef98199399c9c5f15db656bf0ee/lib/HueLight.js#L31

[lbschenkel]

Indeed, this is what I mean by capability model. Preferably, I would be a JSON file [...]

Agreed. If the DB is in a external file, it also immensely lowers the barriers for contributors. Code changes will be required only to implement entirely new quirks, but not to enable/disable existing quirks for certain devices.

[ebaauw]

I’ll see if I can do some tests over the weekend.

This turned out to be more challenging than I imaged. Summary:

• The ZCL spec doesn't cater for Color Temperature in a scene state; different lights handle this very differently;
• The Store Scene command works consistenly across all Color temperature lights I've tested (Philips Hue, OSRAM Lightfy, IKEA Trådfri, icasa Filament);
• Not all lights support specifying the Color Temperature in the scene state through Add Scene;
• Reporting the scene state varies greatly per model. Updating the light states from the cached scene state on scene recall is very challenging. Doing a "fast poll" after recalling a scenes seems really necessary;
• Reporting the scene state from the REST API plugin is hugely inconsistent;
• The REST API plugin implementation of PUT /groups/#/scenes/#/lights/#/state, resulting in an Enhanced Add Scene command, is buggy. I haven't encountered any obvious bugs in the light firmware (other than different lights handling Color Temperature in scenes differently). EDIT None of the lights I tested seem to implement the standard in full (see next post).

Details:

The REST API plugin is hugely inconsistent for scenes:

• When doing a GET of /groups or /groups/#, the scenes are retuned as an array of objects, with an id key to indicate the scene ID;
• When doing a GET of /groups/#/scenes, the return value is an object, with the scene IDs as on of the keys. In each object, the lights are returned as an array of IDs;
• When doing a GET of /groups/#/scenes/#, the lights are returned as an array of objects, with id indicating the light (resource) ID. The state attributes are in the object; no separate state object. x and y are separate keys, instead of a combined xy array;
• A GET of /groups/#/scenes/#/lights results in a "resource not available" error;
• Updating a scene is done through a PUT of /groups/#/scenes/#/lights/#/state.

The Store Scene command sets the scene state to the current light state. The Add Scene and Enhanced Add Scene commands contain the scene state in their payload. The View Scene and Enhanced View Scene commands return the scene state, but the GUI doesn't display this, probably because the state is encoded in a kinda odd way.
It scene state doesn't list the attribute IDs nor types. It only lists the values (in "scene order" defined by the ZCL standard) and the total length per cluster. For On/Off, the length is 1 and the values is that of the OnOff attribute. For Level Control, the length is also 1 and the values is that of the Current Level attribute. Color Control is more complex. The defined order seems to be: Current X (two bytes), Current Y (two bytes), Enhanced Hue (two byes), Saturation (1 byte), Color Loop Active (1 byte), Color Loop Direction (1 byte), and Color Loop Time (2 byes), for a total of 11 bytes. Note that Current Color Temperature is missing here. According to the standard, it's OK to leave out trailing attributes.
On Store Scene, all supported attributes from all clusters seem to be copied to the scene state; on Add Scene, only the attributes specified in the payload. So, using Add Scene, it is possible to create a scene that doesn't change brightness. Not through the REST API, nor through the deCONZ GUI, though.

Different Color temperature lights seem to handle color temperature in scenes very differently. I've been testing the following lights:

Manufacturer	Type	Firmware	Color Capabilities	XY reflects CT
Philips	LTW001	1.50.2_r30933	0x0010	Yes
OSRAM	Classic B40 TW - LIGHTIFY	V1.05.10	0x0010	No
IKEA	TRADFRI bulb E27 WS�opal 980lm	2.3.007	0x0010	No
icasa	ICZB-FC	2.4.2	0x0010	Yes

Setting Color Temperature from the deCONZ GUI, following by a Store Scene:

• The Philips light updates Current X and Current Y to reflect the Color Temperature. It stores these in the scene state. The scene state only contains Current X and Current Y;
• The OSRAM light reports fixed values for Current X and Current Y. It stores and recalls Color Temperature in the scene state alright, but it doesn't report it. It reports the full 11 bytes for Color Control in the scene state, with fixed values;
• The IKEA light is very similar to the OSRAM, except that it reports only Color X in the scene state, with a fixed value;
• The icasa updates Current X and Current Y to reflect the Color Temperature. The scene state contains Current X, Current Y, and Enhanced Hue. The first two are fixed at 0, the latter holds the Color Temperature value.

Issuing Add Scene from the GUI is also handled differently. deCONZ issues a payload without any state attributes. This is valid, if I read the ZCL spec correctly, but WireShark thinks it's a malformed packet. The lights cannot seem to agree:

• The Philips light returns a Default Response with status Malformed Command;
• The OSRAM light returns an Add Scene Response with status Insufficient Space;
• The IKEA light returns a Default Response with status Malformed Command;
• The icasa light accepts the command, and returns an empty scene state on the next View Scene.

Issuing a PUT to /groups/#/scenes/#/lights/#/state results in an Add Scene command, with all values specified. The REST API plugin seems to fill in the values missing from the body from its cached scene state and/or default values - not from the current light state. I think this is a bug: the plugin should send only the values corresponding to the attributes in the body.

• The Philips light accepts the Current X and Current Y values in the scene state. On recalling the scene, it still reports a Color Mode of ct;
• The OSRAM light accepts the Current X and Current Y values in the scene state, and reports them back in the next View Scene command. However, on recall, Color Temperature is set to 153, irrespective of the values in the scene state;
• For the IKEA and icasa lights, the REST API plugin sends a malformed package: the Color Control length is set to 11, but the payload only contains Current X and Current Y. This appears to be a bug in the REST API plugin, rather than in the Trådfri firmware?! As far as I can tell, the difference with the other lights is that these lights don't support the Hue, Saturation, Enhanced Hue, and Color Loop attributes. EDIT See next post.

[lbschenkel]

Great job. I just skimmed what you read because I'm extremely busy today (and will be on this weekend), but I'll come back to this and let's see if how I can contribute in improving the status quo.

[ebaauw]

Some more reading in the ZCL spec (section 5.2.2.5):

Since there is a direct relation between ColorTemperatureMireds and XY, color temperature, if supported, is stored as XY in the scenes table.

Neither OSRAM, nor IKEA, nor icasa adhere to this. Kinda odd that they're still certified.

Attributes in the scene table that are not supported by the device (according to the ColorCapabilities attribute) SHALL be present but ignored.

I'm not sure what "present but ignored" means, but this could be read as not even the Philips Hue light adheres to the standard (since they return only Current X and Current Y in the scene state).

[ebaauw]

For the IKEA and icasa lights, the REST API plugin sends a malformed package: the Color Control length is set to 11, but the payload only contains Current X and Current Y. This appears to be a bug in the REST API plugin, rather than in the Trådfri firmware?!

Looking at the code, this could only happen if the colormode in the scene's light state hasn't yet been set. Looking at the database, cm is "none" for the IKEA and icasa. The payload isn't Current X and Current Y, but Colorloop Active, Colorloop Direction, and Colorloop Time. Also, the scene name is "Scene %u". I'm not sure which command I used to create the scene initially (too much testing), but I think this might have happened when the REST API plugin read the scene from the light, instead of creating it itself (e.g. when creating the scene from the GUI).

[ebaauw]

Patched the code, so it'll no longer send the malformed payload.

Back to testing the IKEA. Indeed, setting ct thru the API results in a Move to Color command, instead of a Move to Color Temperature. The light seems happy about this; the Current X and Current Y attributes reflect the color, but the Color Temperature attribute isn't updated. This will confuse the hell out of API clients, homebridge-hue included.

Doing a PUT to the scene state now succeeds; the light returns only Current X and Current Y in the View Scene. However, when recalling the scene, these values seem to be igored, and the light reverts to the colour temperature from the last Store Scene. So the light seems to update its internal colour temperature on Move to Color, and stores it in the scene, without exposing this over ZigBee.

For the icasa, the REST API sends Move to Color Temperature allright, when setting ct. I patched addTaskAddScene() to map ct in the scene state to hue, for this light, and modifyScene() to copy ct from the body to the LightState and set cm in the LightsState accordingly, for all lights.

This seems to work, I can now PUT /groups/#/scenes/#/lights/#/state with a body containing ct to store colour temperature in a scene on the iCasa. Setting ct in the scene state now also works for the LTW010, modulo some rounding / conversion errors in the ct -> xy translation. As this is a Color temperature light, the colormode remains ct on scene recall. For the LCT015 Enhanced color light, storing the scene works similar, but recalling it sets colormode to xy. I cannot tell without mutilating the light and removing the diffuser, but I suspect it drives the RGB channels instead of the CW/WW channels. I need to double-check if/how the Hue bridge handles this. EDIT This also happens on the Hue bridge.

When doing a PUT to /groups/#/scenes/#/store, the REST API plugin issues a (group broadcast) Add Scene, with an empty scene state, followed by a Store Scene. The Add Scene is, of course, ignored by most lights. I double-checked by sending a groupcast Add Scene from the GUI, followed by a unicast View Scene: indeed, no scene was created.

[manup]

The scene handling is really confusing in the specification. I remember when implementing it for the FLS-PP lp RGBW ballast around 2013, it wasn't clear how to treat color temperature properly since it wasn't technically part of the scene. The add scene commands only carry xy values but no hint if it relates to color or color temperature.

I can imagine that in the next API we could provide an option to recall scenes via unicast by just sending the related commands to set on/off, brightness and color based on the scene values stored in the database. Should work ok in smaller groups but scales badly for > 10'ish device groups, on the other hand it would solve the broadly broken firmware scene issues.

[ebaauw]

Not a fan of "recalling" scenes through unicasts: this will be a lousy user experience, as the lights won't react simultaneously. If you want to go this route, make sure it's exposed differently in the API from recalling a ZigBee scene.

I think the safe bet would be to stay away from Add Scene and to rely solely on (unicast) Store Scene commands to create and maintain scene light states. Most lights seem to support that alright. The only real limitation is that the 5-channel enhanced colour lights will drive the RGB channels (colormode xy) instead of the WW/CW channels (colormode ct). There might be some odd lights that don't handle Store Scene correctly: I think we should document this and advice people not to buy these.

[ebaauw]

One of my Homebridge Hue users has an issue that HomeKit doesn't reflect the colour temperature of their IKEA TW bulb correctly, see ebaauw/homebridge-hue#699.

Did some more testing/sniffing on my Trådfri TW bulb (which seems to be a different model)

When sending a Move to color temperature command, the light updates the Color temperature attribute to the actual setting (as opposed to the requested), but not Current x and Current y. On a subsequent Store scene, only Current x is reported in the scene, with the current (now bogus) value of Current x. However, when recalling the scene, the colour temperature is updated and Color temeperature shows the correct actual value.

When sending a Move to color command, the light updates the Current x and Current y attributes to the actual settings, but not Color temperature. On a subsequent Store scene, both are reported in the scene, with the current values. Because Color temperature isn't updated, the REST API plugin reverts to the old value of state.ct when the lights is next polled, causing API clients to show the wrong colour temperature.

I don't have any other IKEA TW lights, but it would seem that their recent firmwares have solved the issue for which the unholy hack has been introduced.

deconz-rest-plugin/zcl_tasks.cpp

Lines 392 to 394 in 1caed49

	// IKEA lights need to use "xy" because move to color temperature is broken and
	// won't update x,y values resulting in broken scenes.
	useXy = useXy || task.lightNode->manufacturerCode() == VENDOR_IKEA;

Even without the firmware fix, I think this hack does more harm than good, as the REST API reports the wrong colour temperature.

[ebaauw]

After removing the cursed line, the REST API plugin behaves properly. A PUT of /lights/xx/state or /groups/xx/action with {"ct": n} works. A PUT of /groups/xx/scenes/xx/store and a subsequence PUT of /groups/xx/scenes/xx/recall store and recall the colour temperature. It's just not reported when GETting /groups/xx/scenes/xx/lights.

[lbschenkel]

If recent firmwares can now save/recall ct in scenes properly, then I agree that this workaround is not warranted anymore. I just urge that we don't break the ability to save scenes for IKEA WS lights, otherwise this will be a huge feature regression.

[lbschenkel]

@ebaauw: can you provide me the binary with the hack removed, so I don't need to compile by myself? I have 4 different models of IKEA lights, WS and CWS, I can test what happens with those.

[ebaauw]

@lbschenkel What server do you run Homebridge on? What OS?

[lbschenkel]

@lbschenkel What server do you run Homebridge on? What OS?

Sorry, should have been more specific. I didn't mean Homebridge: I meant deCONZ. I would like to replicate your tests with the workaround disabled on all my lights, to confirm that latest firmware can save/recall scenes set via ct.

I'm running Debian "buster" x86-64.

[ebaauw]

I didn't mean Homebridge: I meant deCONZ.

My bad. Doing too many things in parallel.

I'm running Debian "buster" x86-64.

Sorry, I don't run that. I can only compile for Raspbian Stretch or Raspbian Buster on Raspberry Pi (armv7l).

[ebaauw]

Included in a4c712b