[RFC] thing actions

[t-8ch]

This implements support for thing actions, allowing rules to attach arbitrary parameters to rules.
Currently only the transitiontime on color channels is supported. Adding support for more converters is easily doable.
It allows rules to either supply a Map<String, Object> or use a typesafe builder to construct parameters.

Open points:

• Should a converter ignore parameters it does not understand or error out?
• is a Map<String, Object> the best way of representing a collection of parameters or would a datastructure validating all types be better.

(Untested, as I currently don't have access to a zigbee dongle.)

Signed-off-by: Thomas Weißschuh [email protected]

[t-8ch]

Another open point:
Should the transition time be configured with the internal X times "100ms" form or rather with a java.time.Duration?

[t-8ch]

@cdjackson @hsudbrock @puzzle-star What do you think?

Snippet I used to test:
(I am not sure why the typecast is needed there)

import org.openhab.binding.zigbee.action.ZigBeeThingActions

rule "test"
when
	Channel "zigbee:device:c9bb146b:00178801032372ed:00178801032372ED_11_color" triggered
then
	val zigbeeActions = getActions("zigbee", "zigbee:device:c9bb146b:00178801032372ed") as ZigBeeThingActions

	zigbeeActions.sendCommand("00178801032372ED_11_color", HSBType.GREEN, newHashMap("transitionTime" -> 500))
end

[t-8ch]

Sorry for being annoying, but did you already find the time to look at this @cdjackson?

[cdjackson]

This is where OH gets messy - we now have two concepts for changing configuration - configuration parameters, and now actions. How do we deal with this?

Personally, I would prefer to see configuration parameters configurable via rules rather than adding a new concept - I know you didn't add this concept, but we somehow have to deal with this. Either we make the system only use actions, or we make it use configuration, or we somehow need to combine the two (which seems messy).

Since transition time is already using configuration parameters, I think it's best not to add another concept, but the issue then is it can't be changed via rules unless we can add the ability to change parameters by rules in OH. Given that this problem comes up quite often, I think that's what should happen - but I don't know if it would be accepted.

[t-8ch]

I (mostly) agree. However only doing this via configuration parameters would be a bit messy if a parameter really only should apply to a single state change.

What do you think about having a generic mechanism to provide channel configuration (overrides) when executing thing actions?

1. This would allow us to bridge the time until channel configurations can be changed from rules
2. Provides a nice API for users that really only need a different configuration per-action
3. Keeps things fairly consistent by reusing the names of the settings for both channel configuration parameters and per-action overrides

[cdjackson]

Fair point. I’m not sure I completely understand your suggestion - I think you’re suggesting a kind of abstraction from actions to parameters? It sounds reasonable if you want to propose something?

…

On 19 Apr 2019, at 11:51, Thomas Weißschuh ***@***.***> wrote: I (mostly) agree. However only doing this via configuration parameters would be a bit messy if a parameter really only should apply to a single state change. What do you think about having a generic mechanism to provide channel configuration (overrides) when executing thing actions? This would allow us to bridge the time until channel configurations can be changed from rules Provides a nice API for users that really only need a different configuration per-action Keeps things fairly consistent by reusing the names of the settings for both channel configuration parameters and per-action overrides — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#373 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH6IQ5SOST57FY33CU2WP3PRGP4LANCNFSM4GSTAPMA>.

[t-8ch]

It would mostly look like the current PR but instead of having an explicit enumeration of valid parameters it would be possible to pass any configuration parameter of the channel itself.

[cdjackson]

Ok, I’m fine with that - I think it sounds like a good compromise and avoids duplication.

…

On 19 Apr 2019, at 12:24, Thomas Weißschuh ***@***.***> wrote: It would mostly look like the current PR but instead of having an explicit enumeration of valid parameters it would be possible to pass any configuration parameter of the channel itself. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#373 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH6IQZV4RUIKCEYMR7L5LLPRGTWTANCNFSM4GSTAPMA>.

[t-8ch]

I'd like to work again over the holidays.

We talked about having a way of overriding any configuration for single command.

This would be racy for configurations that are persisted in the ZigBee device itself.

Instead I think it should only be possible to override parameters that are sent directly inside the ZigBee command. (Like the transitionTime)

[cdjackson]

Instead I think it should only be possible to override parameters that are sent directly inside the ZigBee command. (Like the transitionTime).

Again, these are configured as configuration parameters. IMHO it should be possible to configure parameters in rules, and I thought that I saw something recently that indicated that was now (or may soon be) possible.

This is an issue that keeps coming up, and we keep adding bodges to allow people to configure parameters in rules - I think we should just allow parameters to be configured in rules.......

[kaikreuzer]

I think we should just allow parameters to be configured in rules

This only makes sense to me, if we manage to finally implement eclipse-archived/smarthome#3484. Accessing thing handler configurations from rules is imho not really desirable.

But what @t-8ch asks for sounds indeed like a perfect match for thing actions. Any action/command that is parametrizable "directly inside the ZigBee command" can nicely be called from within rules and without the need to deal with any thing configurations or channels at all.

[cdjackson]

I’m not sure I agree that it doesn’t make sense to allow rule to configure the handler - there are times when (for example) you might need to change polling rates, and I’m sure there are plenty of other such conditions. But let’s leave that for another day. Additionally, the configuration we are talking about here is in fact in the handler - if we simply allowed configuration of things through rules, this would all be solved, as would a load of other issues that people keep raising. I thought that I saw something recently that allowed this? Was I mistaken? For OH3, I would really like to see the actions dropped in favour of a much more refined command “system”. Some time back I proposed using attributes to provide secondary data to commands - eg OnOff command = On, speed = x. The current system is IMHO quite messy.

…

On 24 Dec 2019, at 11:53, Kai Kreuzer ***@***.***> wrote: I think we should just allow parameters to be configured in rules This only makes sense to me, if we manage to finally implement eclipse-archived/smarthome#3484 <eclipse-archived/smarthome#3484>. Accessing thing handler configurations from rules is imho not really desirable. But what @t-8ch <https://github.com/t-8ch> asks for sounds indeed like a perfect match for thing actions. Any action/command that is parametrizable "directly inside the ZigBee command" can nicely be called from within rules and without the need to deal with any thing configurations or channels at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#373?email_source=notifications&email_token=AAH6IQ4HZ6C6O4O5CWKYMWDQ2HZZ5A5CNFSM4GSTAPMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHTC5LQ#issuecomment-568733358>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH6IQ33VA27AWM7QS5BGQLQ2HZZ5ANCNFSM4GSTAPMA>.

[cdjackson]

See -: openhab/openhab-core#1004 openhab/openhab-core#583 These two PRs proposed adding the ability to configure things via rules, but it is rejected each time. This is really a recurring discussion unfortunately - it’s very common with Zwave that people want to configure with rules as well :(

…

On 24 Dec 2019, at 11:53, Kai Kreuzer ***@***.***> wrote: I think we should just allow parameters to be configured in rules This only makes sense to me, if we manage to finally implement eclipse-archived/smarthome#3484 <eclipse-archived/smarthome#3484>. Accessing thing handler configurations from rules is imho not really desirable. But what @t-8ch <https://github.com/t-8ch> asks for sounds indeed like a perfect match for thing actions. Any action/command that is parametrizable "directly inside the ZigBee command" can nicely be called from within rules and without the need to deal with any thing configurations or channels at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#373?email_source=notifications&email_token=AAH6IQ4HZ6C6O4O5CWKYMWDQ2HZZ5A5CNFSM4GSTAPMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHTC5LQ#issuecomment-568733358>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH6IQ33VA27AWM7QS5BGQLQ2HZZ5ANCNFSM4GSTAPMA>.

[t-8ch]

Even if the configuration could be changed from rules it will be impossible to change it only for one single command because commands are executed asynchronously via the commandScheduler.

(The exact usecase is to have a rule-triggered "sunrise" with a very long transitionTime. During the long transition it should still be possible to instantly control the light with the normal control mechanisms)

[cdjackson]

True - this is why I proposed the ability to set attributes on commands, so that secondary information can be attributed to a single command.

…

On 26 Dec 2019, at 14:13, Thomas Weißschuh ***@***.***> wrote: Even if the configuration could be changed from rules it will be impossible to change it only for one single command because commands are executed asynchronously via the commandScheduler. (The exact usecase is to have a rule-triggered "sunrise" with a very long transitionTime. During the long transition it should still be possible to instantly control the light with the normal control mechanisms) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#373?email_source=notifications&email_token=AAH6IQ5VC5WSKMEXFNU6QZ3Q2S32BA5CNFSM4GSTAPMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHVUAMI#issuecomment-569065521>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH6IQ2KSILSRNIK2B75VJTQ2S32BANCNFSM4GSTAPMA>.

[t-8ch]

Yeah, I also tried to push this, but this was fairly clearly rejected by OH itself.

IMHO by implementing parameters via thing actions now it should be fairly easy to later implement attributes if they are implemented in OH.

[cdjackson]

Ok, let’s proceed on that basis. Please can you provide any PR against the 2.5.x branch and not master. I want to maintain comparability with 2.5 until there is clear direction for OH3.0.

…

On 26 Dec 2019, at 14:21, Thomas Weißschuh ***@***.***> wrote: Yeah, I also tried to push this, but this was fairly clearly rejected by OH itself. IMHO implementing parameters via thing actions now it should be fairly easy to later implement attributes if they are implemented in OH. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#373?email_source=notifications&email_token=AAH6IQ2SVWM7SSTJHWAB333Q2S4XPA5CNFSM4GSTAPMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHVUJCI#issuecomment-569066633>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH6IQZDGRWSXXM6OXYYQHDQ2S4XPANCNFSM4GSTAPMA>.

[t-8ch]

Thanks!

The PR is now based on the 2.5.x branch.

The following questions are open IMO:

• Is a Map<String, Object> sufficient or should there be a dedicated class that enforces correct parameters and their types?
• Should parameters which are not used by a command be silently ignored or should they throw errors.
• The docs want both non-static and static action methods. For new and old rules. Should compat with old rules be preserved?
  (Personally I don't know how the old ones look)

[cdjackson]

Yeah, I also tried to push this, but this was fairly clearly rejected by OH itself.

I created an issue to discuss this point. openhab/openhab-core#1298

Is a Map<String, Object> sufficient or should there be a dedicated class that enforces correct parameters and their types?

I'd be tempted to use a custom class.

Should parameters which are not used by a command be silently ignored or should they throw errors.

I guess they can be ignored, but I would probably log an INFO or WARN so the user knows there's an issue (if they check the logs ;) ).

Should compat with old rules be preserved?

Personally, I'd be happy to ignore the old rules at this stage.

[t-8ch]

Updated the PR to include the latest feedback.

I am not sure in which java package to put the new functionality, though.

Currently the transition time is passed as an integer which signifies a multiple of 100ms, as ZSS handles it internally.
Personally I would prefer to have it as a java.time.Duration, wdyt?

[cdjackson]

I think Duration is probably fine.

Looking through this, it still feels fundamentally wrong (don't worry - I'm not going to get into a big discussion here about that ;) ). We need to document this and I must admit I've not looked at actions much in the past but it seems that we now have two ways to do the same think.

If I want to turn a switch off, I use the standard send command to OFF, but if I want to turn off the same switch with a map of parameters, then I use the action. Can you provide some concrete examples of this (here for starters, but ultimately we should put it in the readme).

[t-8ch]

We need to document this and I must admit I've not looked at actions much in the past but it seems that we now have two ways to do the same think.

Yes, this has to be documented.

If I want to turn a switch off, I use the standard send command to OFF, but if I want to turn off the same switch with a map of parameters, then I use the action.

This is correct. The actions also support commands without parameters, so in cases where parameters are not always used, users can still stick to the actions for everything for consistency.