Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Allow bindings to deactivate auto-update for channels #595

Closed
kaikreuzer opened this issue Dec 3, 2015 · 15 comments · Fixed by #5011
Closed

Allow bindings to deactivate auto-update for channels #595

kaikreuzer opened this issue Dec 3, 2015 · 15 comments · Fixed by #5011

Comments

@kaikreuzer
Copy link
Contributor

migrated from Bugzilla #481178
status NEW severity normal in component Core for ---
Reported in version unspecified on platform PC
Assigned to: Project Inbox

On 2015-10-31 19:43:07 -0400, Kai Kreuzer wrote:

By default, the auto-update binding sets the status for a channel after seeing a command for it. The binding must be allowed to deactivate this feature for certain channels, so that it can have the state updates under its own control.

@maggu2810
Copy link
Contributor

maggu2810 commented Feb 27, 2017

By default, the auto-update binding sets the status for a channel after seeing a command for it. The binding must be allowed to deactivate this feature for certain channels, so that it can have the state updates under its own control.

I am currently looking into this...

Please correct me if I am wrong, but the auto-update post an update for the item after receiving a command for it.
This could currently be disabled by implementations of AutoUpdateBindingConfigProvider by using the item name.

There could be different ways to implement that feature, so, without going into deeper details of the special implementations I assume the principle is equal.
The ItemChannelLinkRegistry needs to be used to get the channel UIDs of the linked channels.
Then we could take the binding ID and decide if an auto-update should be done (how it is done e.g. by provided services, a map that is filled by some code, ... does not matter now).

But what is the desired behaviour of:

  • binding A provides thing A that contains channel channel_A
  • binding B provides thing B that contains channel channel_B
  • there is an item multiLinkedItem
  • channel_A is linked to multiLinkedItem
  • channel_B is linked to multiLinkedItem
  • binding A wants to enable the auto update
  • binding B wants to disable the auto update
  • multiLinkedItem received a command (e.g. SwitchType.ON)

At the moment, if there exists multiple AutoUpdateBindingConfigProvider for the same item the auto update is done if at least one enables the auto update.
Should we proceed the same way using binding IDs?

@kaikreuzer
Copy link
Contributor Author

Then we could take the binding ID and decide if an auto-update should be done

Hm, I think the binding ID is much too coarse grained. The idea of this issue was to achieve a similar feature as in OH1, where a binding can decide for each item (here now channel), whether it wants to disable the auto-update as it itself will take care of reliable status updates after commands. This feature very much depends on the specific channel, so having just all on/off isn't imho helpful.

Also note that the AutoUpdateBinding is a remainder of the otherwise already removed OH1 binding APIs - we will have to think about whether it makes sense to keep it in place (also with the AutoUpdateBindingConfigProvider interface). While this still allows the user to add an { autoupdate="false"} in an item file, there is no equivalent for any db defined items available. I therefore wonder if the ItemChannelLinks might not be a better place for such an information.

At the moment, if there exists multiple AutoUpdateBindingConfigProvider for the same item the auto update is done if at least one enables the auto update.
Should we proceed the same way using binding IDs?

To me, this is a bit unlogical (I cannot remember the reason why it had been built like that). Imho it would be better to give every binding a veto, so that auto-update if off, if a single binding turns it off as it wants to take care of the updates itself.

@maggu2810
Copy link
Contributor

maggu2810 commented Feb 27, 2017

so having just all on/off isn't imho helpful.

It is at least helpful for persons like me, that don't rely an the auto-update at all and so I want to disable it for every item that is linked to some channel of a thing of that bindings.

The idea of this issue was to achieve a similar feature as in OH1, where a binding can decide for each item (here now channel), whether it wants to disable the auto-update as it itself will take care of reliable status updates after commands. This feature very much depends on the specific channel, so having just all on/off isn't imho helpful.

Hm, if the binding author wants to use the auto-update for some channels and don't want to use it for others channel, why not simply put the updateState line into the handleCommand method of the respective channels.
Why should I try add a bunch of configuration as binding author?
As he already implements the handleCommand method he could skip updating states for every implementation or handle it differently in the code.

Also note that the AutoUpdateBinding is a remainder of the otherwise already removed OH1 binding APIs - we will have to think about whether it makes sense to keep it in place (also with the AutoUpdateBindingConfigProvider interface).

I don't think it is very useful for the normal use case.
Perhaps there should be one option to configure the behaviour for all the items that are not linked and so do not correspondent to any binding.

To me, this is a bit unlogical (I cannot remember the reason why it had been built like that). Imho it would be better to give every binding a veto, so that auto-update if off, if a single binding turns it off as it wants to take care of the updates itself.

Okay for me, too, if we want to break the current behaviour.

@maggu2810
Copy link
Contributor

IIRC AutoUpdateBindingConfigProvider is not inly a reminder but used by model.item

@kaikreuzer
Copy link
Contributor Author

It is at least helpful for persons like me, that don't rely an the auto-update at all

Well, there is a much simpler solution for you: Simply remove the autoupdate bundle from your setup :-)

why not simply put the updateState line into the handleCommand method of the respective channels.

Because the default behavior, especially if there is NO binding is that the state update is done. Removing this feature would force every binding to do it manually.

Why should I try add a bunch of configuration as binding author?

As we do not have a deactivation feature in place yet, but a large number of well-working bindings, I think this shows that deactivating the auto-update is rather the exception. And the required "bunch of configuration" could be a simple flag that we add to the channel xml, this can be pretty simply.

AutoUpdateBindingConfigProvider is not inly a reminder but used by model.item

Yes, it is used for parsing the {} parts of the item files. But this is part of "old" APIs - with ESH bindings, the only used value is the "channel" reference. I didn't say that we can easily remove the BindingConfigProvider interface right now, I just didn't want to raise its importance again.

@maggu2810
Copy link
Contributor

Simply remove the autoupdate bundle from your setup

This does not work as long as I want to use an unmodified model.item.
Sure, I can add a custom bundle that provides the AutoUpdateBindingConfigProvider interface in the same package but this isn't a better work around at all.

And as you have written above, there are bindings that rely on the auto-update, so, removing the auto-update bundle completely will not work, too.

@maggu2810
Copy link
Contributor

I updated the PR #3082

  • The new interface offers two function to add and remove a configuration to enable / disable auto-update for a whole binding (by Binding ID)
  • The new interface offers two function to add and remove a configuration to enable / disable auto-update for channels (by Channel UID)

@kaikreuzer
Copy link
Contributor Author

This does not work as long as I want to use an unmodified model.item.

Ah, I wasn't even aware of the AutoUpdateGenericBindingConfigProvider. It does not seem to have any dependency to the model stuff, so possibly this could be moved to the autoupdate bundle (and thus allow you to not include it)?

@maggu2810
Copy link
Contributor

The class implements the BindingConfigReader interface that is part of model.item

@sjsf
Copy link
Contributor

sjsf commented Feb 28, 2017

I just read though this discussion here. To be honest, I don't think this is going into the right direction. The problem I see is that the question whether the auto-update should be active or not not only depends on the binding developer's point of view, but also in many cases on the concrete setup and user preferences.
As a matter of fact, this must be a property of each ItemChannelLink. Clearly, the binding may announce its preference (either per binding or per channel), but this can only be an indication for a default. In the end, the user must be able to override this.
I'm currently thinking about the profile idea (as mentioned in #583, #1043, #2226 - so far mainly in the context of triggers) and prototyping a little around this. My preference currently goes into the direction of decoupling ThingManager from ThingHandlers a little by adding a "routing" (or "profile") layer in between those two for triggers, updates and commands.
So as an example, we would have a command-profile which simply delegated a command to the handler (i.e. "no-auto-update"), and another one which in addition also updates the item state (i.e. "auto-update"). Which one is taken depends on

  1. a configuration property of the ItemChannelLink
  2. the binding's preference for a channel
  3. the binding's preference generally
  4. and "auto-update = true" as a default if none of the above gave any clue (in order to preserve compatibility).

So, to be honest, I see that your approach fixing a flaw in the current auto-update binding. However, I would rather like to see it dropped completely and replaced by a core feature which is easier to handle configuration wise.

@maggu2810
Copy link
Contributor

@SJKA Thank you for your comment.
What do you think about the scenario where two channels are linked to the same item?

To use your example:

  • the first link uses the "no auto-update" profile
  • the second link uses the "auto-update" profile

If the profiles are handled separately the "no auto-update" profile will delegate the command to the handler. The "auto-update" profile will delegate the command to the other handler and update the item.
So it fits to the old "logical OR" implementation.

This is okay for me (at least I think so at the moment) because if someone uses one item for two different channels the situation could be very indeterministic at all.
But this does not fit to Kai's expectations above.

To me, this is a bit unlogical (I cannot remember the reason why it had been built like that). Imho it would be better to give every binding a veto, so that auto-update if off, if a single binding turns it off as it wants to take care of the updates itself.

@sjsf
Copy link
Contributor

sjsf commented Mar 1, 2017

if someone uses one item for two different channels the situation could be very indeterministic

That is indeed a very valid point. My intention was the opposite: that as a user, I want to stay in control of who (framework, binding - and if so: which one...) updates the state of an item, in order to make such situations as deterministic as possible.

So to stay in the above example: What if both bindings "veto" the auto-update and handle it themselves? Which one wins? And what if they don't behave in the exact same way? As a user, I want to be able to fix this.

Nevertheless, I agree that my proposal for a default with the "logical OR" was not ideal. The "first veto disables the auto-update" might be the better default strategy. However, I still think that the user needs the ability to pick a "winner" binding. But maybe that's a different story about ignoring updates coming from a specific binding where the profile idea attached to ItemChannelLinks would be a better fit. And indeed, then it would be sufficient to have the auto-update configuration directly at the Item, and not at the ItemChannelLink.

For the rest of my thoughts (i.e. influencing the communication between framework and binding wrt commands, updates, triggers in order to gain back some level control), I will of course create a separate issue once I have a valid proposal as a starting point. You convinced me that these two topics don't conflict with each other.

But back to the topic of this issue: I actually tend to agree that it would make sense to break with the existing behavior wrt auto-update:

  • Auto-Update is the default
  • Bindings may "opt-in" to take care of updating items (either binding-wide or for specific channels only) and by doing so they disable the auto-update for a linked item
  • A user has the option to enable/disable the auto-update feature on item level, hence overriding the above logic completely.

When touching all this anyway, I would even consider moving it into the core.thing bundle, dropping the extra autoupdate one. Even if there are solutions which only use bindings that handle the updates themselves and therefore don't need the auto-update feature, then exactly these bindings will have a dependency to the autoupdate bundle because they want to declare their "opt-in". So I don't see any value in having it separated, or did I miss anything?

@sjsf
Copy link
Contributor

sjsf commented Mar 1, 2017

PS: Just to make this clear

And indeed, then it would be sufficient to have the auto-update configuration directly at the Item, and not at the ItemChannelLink.

I'm very well aware that as of today we don't have configurations on item level. And probably also don't want that. So we could use ItemChannelLink properties for this, but should be aware that conceptually this would be what it is: First "opt-in" of a binding wins for the whole item and an explicit enabling of auto-update at any ItemChannelLink would overrule any binding's opt-ins and still enable the auto-update.

@digitaldan
Copy link
Contributor

ping!
We also have a need for this in an upcoming omnilink 2.0 binding, I would vote to be able to configure this on the channel level.

@triller-telekom
Copy link
Contributor

@digitaldan Currently there is a PR #4108 which introduces profiles on ItemChannelLinks and based on that there will soon be a follow-up PR introducing the ability to configure these auto-updates.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants