Input needed: Refactor logic setting light state

[ebaauw]

I've finally found the time to refactor the setting of the state of a /lights resource, see #1111 (comment). Next to the functional changes, I'm also adding parameter checking.

The logic for handling on and bri is described in the issue mentioned above. The logic for handling the other attributes is less clear. I run into some design decisions for these, @manup, everybody, please provide your input.

1. The Hue API clearly states: xy trumps ct trumps hue and sat. It ignores the over-trumped attributes without issuing an error, but clients can tell because they aren't reported in the response. Not sure how effect is supposed to relate to these. I want to do the following: for "none", clear the colour loop before handling the colour attributes; for "colorloop", set the colour loop after handling the colour attributes. And return an error for the colour attributes while colour loop is active (just as for bri and colour attributes when the light is off).
2. While not documented, I think the Hue API also acts like: bri trumps bri_inc and ct trumps ct_inc. Note that the deCONZ API doesn't expose xy_inc, hue_inc, nor sat_inc. While they might be great for rule actions for the magic cube, I haven't seen any use for these;
3. There is no ResourceItem for effect. Looking at the code, effect is exposed only when xy is. On the other hand, setting the colour loop changes colormode to hs?
4. I'm not sure I fully understand the code handling hue and sat. It would seem the plugin tries to calculate and update the xy values from the hue and sat values for the benefit of taskToLocalData(). Why isn't there similar logic for updating ct. And for updating xy and hue / sat when setting ct? And for updating ct and hue / sat when updating xy?
   I think API clients should take notice of colormode and only process the corresponding attributes (homebridge-hue does).
   I would really like to defer this setting of other colour attributes to the light, although not all non-Hue lights update xy, ct, or hue / sat from other colour modes;
5. I'm missing an addTaskSetEnhancedHueAndSaturation() in zcl_tasks.cpp. Is that intentional or just for historic reasons? Preferably, I would have a single addTask...() to set any combination of hue/enhanced hue and sat, depending on the light capabilities;
6. What's the policy for exposing attributes that aren't supported by the light? E.g. exposing ct for Color lights, or hue and sat for lights that don't support these for Alexa's benefit. And for using a different colour mode than specified in the API call to set the colour (e.g. setting hue / sat when xy was specified, or setting xy when ct was specified). How to handle colormode in these cases: I think it should report the actual light mode?
   Personally I feel this is a poor decision, leading only to confusion. If a light doesn't support hue and sat, it doesn't work with Alexa, sorry. If a light doesn't support ct, do the ct to xy conversion in your API client.
7. Currently a number of non-light devices are exposed as /lights resources: _Window covering device, Warning device, Fan, Range Extender, and Configuration Tool (did I miss any?). For Window covering device, on, bri, and sat are abused for Open, Percentage Lift and Percentage Tilt from the Window covering cluster. For Warning device, alert is abused for Warning from the IAS WD cluster. The only Fan currently (in progress of being) is the Hampton Bay fan module (Fan Module (Hampton Bay) #932), which exposes the Fan Control cluster on the same endpoint as the On/Off and Level Control clusters (to expose the light). The fan is exposed though an added speed attribute. I'm still not sure whether the Fan Control cluster should better be exposed as a separate (for now) /lights resource. The Trådfri repeater supports Identify, and correctly maps that to alert.
   Most of these devices support Groups and Scenes, but the API cannot handle these. In preparation of this, I'd like to change the attributes for Window covering device to open, lift, and tilt (and get rid of the 0..254 to 0..100 translations) and for Warning device to warning. As this would be a breaking change, how do we want to handle that. Expose old and new next to each other for a while?
   It might be prudent to add the cluster to the uniqueid of these non-light /lights resources, but that might also be a breaking change. It would be needed to expose the Hamption Bay as two resources.

[ebaauw]

3. There is no ResourceItem for effect. Looking at the code, effect is exposed only when xy is. On the other hand, setting the colour loop changes colormode to hs?

Because of this, effect isn't reported in the web socket notifications.

The (Hue) light reports colormode hs when setting the colormode through the GUI, so that explains the setting.

[ebaauw]

Cross-referencing #973.

[ebaauw]

Next iteration: the next commits add state.open, state.lift, and state.tilt to Window Covering devices:

• state.lift is best understood as "percentage closed". It reflects Current Position Lift Percentage from 0% (fully open) to 100% (fully closed). Note that some devices (like Xiaomi) reverse the values; the REST API plugin handles this, so state.lift works the same for all devices;
• state.open is derived from state.lift: it is true while state.lift < 100%.
• state.tilt reflects Current Position Tilt Percentage, from 0% to 100%.
• Setting state.lift to a value between 0 and 100 results in a Goto Lift Percentage command.
• Setting state.lift to "stop" results in a Stop command.
• Setting state.open to true results in an Up/Open command. Setting it to false will issue a Down/Close command.
• Setting state.tiltto a value between 0 and 100 results in a Goto Tilt Percentage command.

$ ph get /lights/327
{
  "etag": "1450b426c7a26529ac316db62e01b8a5",
  "hascolor": false,
  "lastseen": null,
  "manufacturername": "LUMI",
  "modelid": "lumi.curtain",
  "name": "Bedroom Curtains",
  "state": {
    "alert": "none",
    "bri": 254,
    "lift": 100,
    "on": true,
    "open": false,
    "reachable": true
  },
  "swversion": "04-13-2017",
  "type": "Window covering device",
  "uniqueid": "00:15:8d:00:02:83:4c:6c-01"
}

Note that not all Window Covering devices support all these commands. Notably the Xiaomi Curtain Controller B1 (that uses the Analog Output cluster), only supports (the equivalent) of Goto Lift Percentage.

The state.on, state.bri, and state.sat attributes are still supported, and work as before. However, when setting these, the REST API responds with the new attributes:

$ ph put -v /lights/327/state '{"on": false}'
{
  "open": true
}
$ ph put -v /lights/327/state '{"bri": 254}'
{
  "lift": 100
}

Both old attributes are included in the websocket notifications (note: I'm running with websocketnotifyall set to false):

Apr 05 23:13:25 pi2 dc_eventlog[860]: /lights/327/state: {"alert":null,"bri":182,"lift":72,"on":true,"open":true,"reachable":true}
Apr 05 23:13:30 pi2 dc_eventlog[860]: /lights/327/state: {"bri":0,"lift":0,"on":false}
Apr 05 23:14:15 pi2 dc_eventlog[860]: /lights/327/state: {"bri":33,"lift":13,"on":true}
Apr 05 23:14:20 pi2 dc_eventlog[860]: /lights/327/state: {"bri":254,"lift":100,"open":false}

[gmitch64]

Just to throw something in here...

The fan is exposed though an added speed attribute. I'm still not sure whether the Fan Control cluster should better be exposed as a separate (for now) /lights resource.

When looking physically at the device (and talking about the Hamton Bay one you've just fixed - thank you), you will ALWAYS have the fan. There may be an attached light unit, or there may no - it's optional whether you install it or not. Now, I don't know if the the remote can actually tell you if the light kit has been installed or not.

I'd think in this case, the "whole fan", should be presented up in a similar method to the multi sensors - with a sub device for the fan, and a sub device for the lights - like a multi sensor has a device for temp, a device for pressure, etc.

G

[ebaauw]

When looking physically at the device

I appreciate that you do that, but it's irrelevant for how a device is exposed through the REST API. The resource structure follows what the device looks like from a Zigbee perspective: the exposed endpoints and clusters. See my explanation in #932 (comment).

[ebaauw]

OK, PR #2553 caused some more effects that I had anticipated.

• Specifying invalid values, that were previously silently ignored, or coerced to valid values, now result in error messages (e.g. setting ct outside the ctmin to ctmax range, see deCONZ v2.05.76 breaks some bulbs integration with Home Assistant #2798; or setting colormode, see Gledopto ct colormode stopped working #2840);
• Specifying inappropriate commands, that accidentally worked, now result in error messages and no longer work (e.g. ct for a Color light, that doesn't support Color temperature, see deCONZ v2.05.76 breaks some bulbs integration with Home Assistant #2798).

I also found some other weird things:

• Allowing both integer and string ("stop") values to state.lift is highly inconsistent, see IKEA smart blinds FYRTUR and KADRILJ #1121 (comment).
• Some lights incorrectly report ctmax as 0, see No colour temperature (ct) possible within Innr RB 285 C #2853;
• According to the ZCL spec, the maximum value for Color Temperature, Color Temperature MIn and Color Temperature Max is 65279 (0xFEFF), same as for Current X and Current Y, see ctmax attribute at 65535 for IKEA lights? #2856.

For my next attempt, I propose the following:

• Only report an error for violations of the data type (e.g. setting setting ct to true, or setting ct to an integer value < 0 or > 0xFFFF). Coerce the value if of the correct type, but invalid (7 is a Hue light; 10 a Trådfri):

  $ ph put -v /lights/7/state '{"on": true, "ct": true}'
  ph put: fatal: invalid value, true, for parameter, ct.
  $ ph put -v /lights/7/state '{"on": true, "ct": -1}'
  ph put: fatal: invalid value, -1, for parameter, ct
  $ ph put -v /lights/7/state '{"on": true, "ct": 0}'
  {
   "on": true,
   "ct": 153
  }
  $ ph put -v /lights/7/state '{"on": true, "ct": 10000}'
  {
   "on": true,
   "ct": 500
  }
  $ ph put -v /lights/10/state '{"on": true, "ct": 0}'
  {
   "on": true,
   "ct": 250
  }
  $ ph put -v /lights/10/state '{"on": true, "ct": 10000}'
  {
   "on": true,
   "ct": 454
  }
  $ ph put -v /lights/7/state '{"on": true, "xy": [2,1]}'
  ph put: fatal: invalid value, [2,1], for parameter, xy
  $ ph put -v /lights/7/state '{"on": true, "xy": [1,1]}'
  {
   "on": true,
   "xy": [
     0.9961,
     0.9961
   ]
  }
  

• Return HTTP status 200 on API errors (just like the Hue bridge). This means that invalid keys or values in the body are ignored, but valid ones are still executed (with an experimental version of ph to handle this):

  $ ph put -v /lights/7/state '{"on": false, "ct": 1000, "xy":[1,2]}'
  {
    "warnings": [
      "invalid value, [1,2], for parameter, xy"
    ],
    "ct": 500,
    "on": false
  }
  $ ph put -v /lights/7/state '{"on": false, "ct": 1000, "xy":[1,2], "key": "invalid"}'
  {
    "warnings": [
      "parameter, key, not available",
      "invalid value, [1,2], for parameter, xy",
      "parameter, ct, is not modifiable. Device is set to off."
    ],
    "on": false
  }
  

• Add a sanity check that ctmax > ctmin. Change default ctmax to 65279.
• For window covering devices, use {"stop": true} instead of {"tilt": "stop"}.

[Smanar]

For window covering, I m agree, and for retro-compatibiliy there is still {"bri_inc": 0}

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.