openhab-core icon indicating copy to clipboard operation
openhab-core copied to clipboard

allow vetoing ItemCommandEvents

Open ccutrer opened this issue 3 years ago • 7 comments

Prevents ItemUpdater from forwarding a vetoed command to a group, and CommunicationManager from forwarding a vetoed command to a linked channel or AutoUpdateManager.

This encompasses several components to enable it:

  • Adding a veto method to ItemCommandEvent
  • Having CommunicationManager and ItemUpdater ignore vetoed command events.
  • Ensuring CommunicationManager (and thus AutoUpdateManager) and ItemUpdater have a lower priority to process the event after rules have had a chance to veto the event.
    • This involves adding a priority concept to EventHandler/ EventSubscriber; it defaults to a priority of 0
  • Adding a way to make rules run synchronously, so that their veto can be processed before CommunicationManager et. al. without race conditions
    • This includes extending rule builder and DTO
  • Adding a VetoCommandAction module for use from rules.

Note that the event must be vetoed by another EventSubscriber with a higher priority than ItemUpdater or AutoUpdateManager. The intercepting subscriber must veto it synchronously. This is safe since EventHandler has a single thread for processing events to subscribers. That said, any rule that opts in to synchronous processing in order to be able to veto commands must not use any blocking actions, since they're now run in the EventHandler thread.

As of yet there is limited UI support (the VetoCommandAction is set to EXPERT visibility, which means the rule must already have it -- or configure it in the Code view of a rule -- in order to be editable). And AFAICT zero possibility to set the rule as synchronous, in order for it to work. That leaves script-created rules that can access the setSynchronous method (my expected use case), and possibly editing rule configuration files directly. I think this is fine, since this is a potentially dangerous feature. You do NOT want to block on the event handler thread. A future direction that we may take is having module types declare themselves synchronous-safe, and only list those modules when you set a rule to synchronous. It's envisaged that rules that veto commands will be very simple - essentially just checking the state of some other item(s). No web requests, no execs, no timers, etc.

ccutrer avatar Oct 05 '22 16:10 ccutrer

Can you explain what the use-case for this would be?

J-N-K avatar Oct 05 '22 16:10 J-N-K

I have several use cases:

  • motorized shades are not allowed to close if the window they cover is open.
  • a water slide over a pool is not allowed to turn on unless the pool cover is open
  • outdoor flood lights are not allowed to turn on if it's sufficiently bright outside
  • prevent a door from locking remotely when the door is open

and a few more similar cases. There are multiple ways to solve these scenarios without being able to veto a command, but they all have drawbacks:

  • You can introduce a proxy item, and write a rule to forward the command to the actual item only if the conditions are met for it to be allowed, and a rule to transfer the state back to the proxy item when it changes. This introduces maintenance overhead by having to create essentially duplicate items, an extra rule, not to mention confusion of which item is which when working on the system (like when you're building the semantic model, should I tag the proxy or the real item?).
  • You can ensure that the only way to send a command to an item is via a rule (like my slide, which has no physical control besides inside the pool panel that we never open - I use a switch with no-load). This is similar to the proxy item scenario, just using some other "real" item as the proxy. But it does depend on the other "real" item being online, otherwise it can be broken for control via app/voice.
  • You can just let the command through, and send an additional command to offset the unwanted action. This might be required if there is local control anyway, but typically causes a stuttering effect, which can be unacceptable for some scenarios (I absolutely do not want water pouring onto my pool cover in any amount, whereas it's just fine if a floodlight is on for a couple seconds).

As an example, I've already converted my shades rules to use veto. It has vastly simplified my multiple other rules that interact with shades, because they don't have to worry about handling this at all. I had not created proxy items (the "best" alternate solution) yet, since I have sooo many shades (30+), and they're all a little different configuration, so the config isn't autogenerated. It's in ruby, but here it is:

received_command gShades.members, synchronous: true do |event|
  logger.info "checking #{event.item.name} before closing it"
  item = event.item
  next if event.up?
  next if event.refresh?
  next if item.tags.include?("AlwaysClose")

  shade_base_name = item.name[0..-20] # trim off "Shade_Rollershutter"
  contact_item = items["#{shade_base_name}Window_Contact"]
  contact_item ||= items["#{shade_base_name}Door_Contact"]

  next unless contact_item.open?

  if item.tags.include?("DoubleHung")
    target = event.down? ? PercentType::HUNDRED : event.command

   # double hung windows are allowed to close halfway even if the
   # window is open
    if target > 50
      event.veto
      item.ensure << 50
    end
  else
    event.veto
  end
end

ccutrer avatar Oct 05 '22 19:10 ccutrer

If you use a rule anyway, why not use a condition on the rule? In general I would say that this should be done as close to hardware as possible(e.g. my KNX shutter output has an additional input for that which is linked to my window open sensor). If that is not possible this needs a rule anyway (like you already do) and then I believe a condition on the rule would be the way to go. I have the feeling that vetoing commands adds very much complexity compared to the simplifications it provides.

J-N-K avatar Oct 08 '22 13:10 J-N-K

I agree with J-N-K. This adds very much complexity for little benefit. What happens if there are more than one item that listen to the command and you would want the first item to receive the command and the second one to ignore it. Or the other way round?

Proxy items are the way to go and while they are tedious and maybe even sometimes annoying to create they at least make it very transparent what is happening, e.g. when looking at the logs. Also they have to be done only for the command channel of the thing, so it's little overhead when mapping a thing with value channel to the items. And if you stick to a proper naming scheme it's clear which item does what.

spacemanspiff2007 avatar Oct 09 '22 03:10 spacemanspiff2007

I completely agree, this seems really complex, not just from the code perspective but from an end user perspective. It's not easy to explain and I see lots of confusion.

However, it does raise an idea. Could at least part of this be implemented as a Profile? I'm not a huge fan of profiles in general but this seems like something that might fit. But that's only if:

  • profiles can return nothing instead of some state, or somehow throw away the event from the Thing (I'm pretty sure this is possible or else some of the already existing profiles wouldn't work)
  • profiles are allowed to access and use the states of other Items beyond the Item it's linked to.

As a profile it should centralize the behavior making it less confusing and less complex. Though it might not address enough of the above scenarios to be useful.

This could also be implemented as a Rule Template. See https://community.openhab.org/t/debounce/128048 for a similar example. Once implemented as a template, you'd still need the Proxy Item and maybe one additional Group, but users can instantiate rules to do these checks meaning the end users only need to create Items and maybe add tags or metadata. That could be helpful if someone has lots of these types of vetoable Items.

rkoshak avatar Oct 10 '22 16:10 rkoshak

Yes, a VetoProfile or GateProfile would be fine. However, it only affects the interface thing/item and does not prevent rules from issuing commands. But that is IMO something that should be solved by conditions.

J-N-K avatar Oct 10 '22 16:10 J-N-K

Yes, a VetoProfile or GateProfile would be fine. However, it only affects the interface thing/item and does not prevent rules from issuing commands. But that is IMO something that should be solved by conditions.

I completely agree, a profile would not solve all the cases and I also agree that for the rest rule conditions, dynamically enabling/disabling of rules, or having a rule template that handles the proxying of the command would be the way to go for the rest.

rkoshak avatar Oct 10 '22 16:10 rkoshak

Since it seems that the majority agrees that this should not be implemented, I'm closing here. I still think that a profile that acts like a gate for commands (or even updates) would be a nice addition.

J-N-K avatar Oct 15 '22 07:10 J-N-K

:nod:. While I personally feel like this feature is worth the complication (it's not like OpenHAB is a simple chunk of code in the first place!), I understand the view (especially the forcing synchronous part I don't like), and knew it was a long shot for acceptance when I started working on it. I'll probably look to see if I can use profiles to accomplish what I need sometime soon. Thanks for the comments!

ccutrer avatar Oct 15 '22 15:10 ccutrer