openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.86k stars 3.58k forks source link

[hue] Transition time configuration for API v2 #15323

Open jlaur opened 1 year ago

jlaur commented 1 year ago

13570 introduced a dynamics channel that replaces the fadetime parameter available for API v1.

They both provide the same functionality, i.e. setting the transition time for lights to gradually reach their new state over a period of time.

However, the implementations are very different.

Having it as a configuration parameter made it possible to always use a specific transition time for a specific device.

Exposing it as a channel makes it possible to dynamically configure this transition time before a command. The setting is not preserved, but is reset after 10 seconds to "no transition time", so it's no longer possible to always have a transition time of e.g. 400 ms for individual lights.

I'm not exactly sure how this channel is intended to be used. If used in rules, I suppose a thing action could be used instead. If there is a valid use-case for it, we should of course preserve it.

In either case I would propose to reintroduce a configuration parameter per device so that each device can have a default transition time. If we are going to preserve the dynamics channel, we could fallback to the configuration parameter value in case there is no value provided through the channel.

The default fadetime used to be 400 milliseconds, but seems to be 0 now, although I'm not 100% sure if Hue has its own default when omitted (which could then still be 400 milliseconds). We should also consider which default value to use.

Your Environment

jlaur commented 1 year ago

@andrewfg, @cweitkamp, @lolodomo, @kaikreuzer, @J-N-K - after finally migrating to API v2 yesterday I'm taking notes of issues, and this is one of them. I have marked it to be discussed. There might be hidden clues/existing discussions in #13570, but I don't remember, and it will be quite time and memory consuming to try to find it - sorry.

andrewfg commented 1 year ago

The dynamics channel is described in the documentation here. It can OPTIONALLY be applied to either device, room, or zone things. It has the effect of telling the Hue Bridge to execute the command transition over a delayed period of time.

There is also a rule action as described in the documentation here that hooks into the same command DTO field.

If there is a valid use-case for it, we should of course preserve it.

Of course there is a valid use case for the dynamics channel. It was discussed during the thread of the original PR and/or on the community forum. We should certainly not delete it.

Having it as a configuration parameter made it possible to always use a specific transition time for a specific device. We should also consider which default value to use.

I have no objection to adding a config param. However the following things should apply..

andrewfg commented 1 year ago

@jlaur BTW (I guess you already noticed it) but you could solve your LK Wiser case by setting a default config param for the transition time > 0 on that specific device. So your respective PR #15316 could be eliminated.

jlaur commented 1 year ago

@jlaur BTW (I guess you already noticed it) but you could solve your LK Wiser case by setting a default config param for the transition time > 0 on that specific device. So your respective PR #15316 could be eliminated.

That would most likely require users to find that it doesn't work - then read the documentation - and then manually configure transition time. I would prefer to have this auto-detected and working out of the box when this is actually possible with very little effort.

jlaur commented 1 year ago

If there is a valid use-case for it, we should of course preserve it.

Of course there is a valid use case for the dynamics channel. It was discussed during the thread of the original PR and/or on the community forum. We should certainly not delete it.

Can you provide a link or remind me?

Having it as a configuration parameter made it possible to always use a specific transition time for a specific device. We should also consider which default value to use.

I have no objection to adding a config param. However the following things should apply..

  • Do not call it "fadeTime" -- the dynamics can apply to any type of transition (e.g. a colour change) which may not necessarily involve fading (i.e. not only brightness changes).

I agree. Signify's own term "transition time" (IIRC) is probably not the worst term here?

  • By default the API v2 command DTO has a null entry for this JSON element. So the default value of the config param should also allow a 'null' value (e.g. say -1) meaning that the DTO field will not be set.

👍 However, perhaps we could also add an optional parameter on the bridge level to avoid having to configure a "default" transition time for all things. This would resemble the API v1 implementation where the fadetime parameter was optional and defaulted to 400 ms.

  • If the user sets a valid numeric value (either 0 or a positive integer) then the respective field could be added to the command DTO.
  • If there is both a valid config param value and a dynamics channel value > 0 then the latter shall apply.
  • Likewise if there is a valid config param value and a dynamic rule action is called then the latter shall apply.

Agreed. With my proposal above the full precedence rules would be:

andrewfg commented 1 year ago

Can you provide a link or remind me?

There were several discussions, but I think the main contributor was @seime starting here

Signify's own term "transition time" (IIRC) is probably not the worst term here?

Ok transitionTime (or maybe transitionTimeMilliSeconds)

add an optional parameter on the bridge level avoid having to configure a "default" transition time for all things

I see no point in having a parameter at bridge level. Remember that according to the API v2 specification the DTO field is optional and therefore the default value of the JSON field is null. Adding a bridge level parameter to override the API v2 null DTO default with a second non API v2 standard, but OH specific, 'default' is both confusing and unnecessary.

jlaur commented 1 year ago

Can you provide a link or remind me?

There were several discussions, but I think the main contributor was @seime starting here

Thanks, I now read through that, but failed to find any argument or reason for providing it as a channel.

I see no point in having a parameter at bridge level. Remember that according to the API v2 specification the DTO field is optional and therefore the default value of the JSON field is null. Adding a bridge level parameter to override the API v2 null DTO default with a second non API v2 standard, but OH specific, 'default' is both confusing and unnecessary.

The point was already written above:

to avoid having to configure a "default" transition time for all things. This would resemble the API v1 implementation where the fadetime parameter was optional and defaulted to 400 ms.

So if I would like 400 ms on all my lights, I could configure it on the bridge level, and I wouldn't need to configure each individual device thing. If I don't configure anything anywhere, it would still default to null in the DTO. As I already wrote:

Otherwise leave null in the DTO to omit/use Hue default.

andrewfg commented 1 year ago

would like 400 ms on all my lights

Two things..

  1. Why do you need it? PS check the performance of your lights with the null default..
  2. If you do need it then you can type "transitionTimeMilliSeconds=400" ONE time (or copy from here), and then paste it 39 times (or whatever) into your .things file.
andrewfg commented 1 year ago

failed to find any argument or reason for providing it as a channel.

@jlaur look -- I did not introduce that code out of personal whim or fashion; it was introduced as a result of specific requests and discussions either on the forum or on github; personally I am not willing to spend any more of my time trawling over old discussions; if you want to positively prove that the channel is NOT required then it is your job to do that work; and if you decide to delete the channel and cause upset to other users who contributed to those discussions, then it is a) your fault, and b) your responsibility later to fix it.

My only additional comments on this topic are 1) that this is an advanced channel (and so hidden from naive users), and 2) that I would argue to therefore keep it anyway for exactly the same analogous reasons why I personally fought so strongly to keep the 'xyzOnly' channels before.