-
Notifications
You must be signed in to change notification settings - Fork 782
Reworked auto-update infrastructure completely #5011
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/auoupdate-for-item-in-paperui/39602/2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a short look at the moment.
Shouldn't it also possible to set the auto update policy for all channels of a thing (handler) or for the whole binding? At least the handler should be able to set it easily for all channels of its things.
public enum AutoUpdatePolicy { | ||
/** | ||
* No automatic state update should be sent by the framework. The handler will make sure it sends a state update and | ||
* it can to it better than just converting the command to a state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: ... can do it ...
|
||
/** | ||
* The binding does not care and the framework may do what it deems to be right. The state update which the | ||
* framework will send out normally will correspond the command state anyway. This is the default of no other policy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: ... default if no ...
/** | ||
* Give advice to the framework whether automatic state updates should be sent for the given channel or not. | ||
* | ||
* @param channelUID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A description should be added if it is ready for a final review (also if the comment is "the channel UID" only).
Same below and also in other classes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a pretty nice enhancement of the functionality.
After an initial review, I'd suggest a few changes, though:
- As the policy will depend on the channels, this information should imho be added to the channel definition (and the channel types) directly. This way, binding developers can simply add the information in the thing XMLs and do not require any programmatic logic and calls to the callback.
- The CommandResultPredictionListener could be removed when introducing a new "StatePrediction" event - the advantage of using events would be that not only sitemap-based UIs would benefit from this feature, but also UIs like the Paper UI or HABPanel.
- The auto-updates should be disabled in general for Group items as those will calculate their state based on their members and predictions aren't really possible in that case (or at least would be highly complex).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SJKA, looks pretty good to me!
I didn't actively test it, but code-wise I think it is an excellent refactoring.
|
||
return channelXmlResult; | ||
} | ||
|
||
private AutoUpdatePolicy readAutoUpdatePlicy(NodeIterator nodeIterator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: readAutoUpdateP[o]licy
private AutoUpdatePolicy readAutoUpdatePolicy(NodeIterator nodeIterator) { | ||
String string = (String) nodeIterator.nextValue("autoUpdatePolicy", false); | ||
if (string != null) { | ||
return AutoUpdatePolicy.valueOf(string.toUpperCase(Locale.ENGLISH)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any special reason for the locale parameter here? All defined values are plain ASCII, aren't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Just (good) habit.
*/ | ||
@NonNullByDefault | ||
@Component(immediate = true, service = { | ||
AutoUpdateManager.class }, configurationPid = "org.eclipse.smarthome.autoupdatemanager", configurationPolicy = ConfigurationPolicy.OPTIONAL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to change the pid to org.eclipse.smarthome.autoupdate
Recommendation autoUpdate = shouldAutoUpdate(itemName); | ||
|
||
// consider user-override via item meta-data | ||
MetadataKey key = new MetadataKey("autoupdate", itemName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you want to put that string in a constant?
MetadataKey key = new MetadataKey("autoupdate", itemName); | ||
Metadata metadata = metadataRegistry.get(key); | ||
if (metadata != null && !metadata.getValue().trim().isEmpty()) { | ||
boolean override = Boolean.getBoolean(metadata.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful! Boolean.getBoolean
expects a system property name as a parameter - I doubt that this is what you want here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, thanks! Of course...
ItemStatePredictedEvent prediction = (ItemStatePredictedEvent) event; | ||
Item item = itemUIRegistry.get(prediction.getItemName()); | ||
if (item instanceof GroupItem) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave a comment on why we ignore group items here?
@@ -44,6 +47,7 @@ | |||
*/ | |||
public class PageChangeListener implements StateChangeListener { | |||
|
|||
private final ScheduledExecutorService scheduler = ThreadPoolManager.getScheduledPool("ui"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might this be a candidate for the new "common" pool?
public void keepCurrentState(Item item) { | ||
scheduler.schedule(() -> { | ||
constructAndSendEvents(item, item.getState()); | ||
}, 200, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
200ms might be a bit tight, usually we go for 300ms for UI related feedback.
The following policies are supported: | ||
|
||
* **veto**: No automatic state update should be sent by the framework. The thing handler will make sure it sends a state update and it can to it better than just converting the command to a state. | ||
* **default**: The binding does not care and the framework may do what it deems to be right. The state update which the framework will send out normally will correspond the command state anyway. This is the default of no other policy is set explicitly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: [i]f no other
|
||
* **veto**: No automatic state update should be sent by the framework. The thing handler will make sure it sends a state update and it can to it better than just converting the command to a state. | ||
* **default**: The binding does not care and the framework may do what it deems to be right. The state update which the framework will send out normally will correspond the command state anyway. This is the default of no other policy is set explicitly. | ||
* **recommend**: An automatic state update should be sent by the framework because no updates will be sent by the binding. This usually is the case when devices don't expose their current state to the handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better: will be sent -> are sent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates - may I ask you to rebase the branch?
@SJKA My comment above has not been answered, yet. I am currently using a service that implements that interface: https://github.com/maggu2810/shk/blob/master/bundles/shk-addon-autoupdate-configurator/src/main/java/de/maggu2810/shk/addon/autoupdate/configurator/AutoUpdateConfigurator.java The implementation also implements the The implementation looks like: https://github.com/maggu2810/shk/blob/master/bundles/shk-addon-autoupdate-configurator/src/main/java/de/maggu2810/shk/addon/autoupdate/configurator/impl/AutoUpdateConfiguratorImpl.java As you removed the |
@maggu2810 Sorry, I tried addressing your comments by making it a characteristic of the Channel/ChannelType, but clearly forgot to explicitly mention it here.
Do you think one of these would cover your scenario? |
Just fresh from testing with the ESH demo setup:
|
Oh well, nasty little conflict-resolution-mistake I did when rebasing. |
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
… in advent of eclipse-archived/smarthome#5011 Signed-off-by: Kai Kreuzer <kai@openhab.org>
Related to eclipse-archived/smarthome#5011 Signed-off-by: Kai Kreuzer <kai@openhab.org>
Related to eclipse-archived/smarthome#5011 Signed-off-by: Kai Kreuzer <kai@openhab.org>
Related to eclipse-archived/smarthome#5011 Signed-off-by: Kai Kreuzer <kai@openhab.org>
670a86c
to
c86d5e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I have rebased the branch in order to resolve the conflicts.
…390) * adapted AutoUpdateDelegate to the new ESH auto update infrastructure Related to eclipse-archived/smarthome#5011 Signed-off-by: Kai Kreuzer <kai@openhab.org>
Could we have a clear example what has to be changed in the *.items files please ? |
If you mean |
Ok, thank you @kaikreuzer for the information. |
…penhab#390) * adapted AutoUpdateDelegate to the new ESH auto update infrastructure Related to eclipse-archived/smarthome#5011 Signed-off-by: Kai Kreuzer <kai@openhab.org>
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
… in advent of eclipse-archived/smarthome#5011 (#5652) Signed-off-by: Kai Kreuzer <kai@openhab.org>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/what-is-itemstatepredictedevent/52387/2 |
Any binding example on how to apply this autoupdate feature? After reading this, i am still not clear what need to be done. Cheers |
It depends on your use case.
to my channel types in the XML. |
ok.. thanks. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/zwave-binding-updates/51080/290 |
It would be great if there was a global setting we could use to set to work the way it did before, or an easy way to make it work like it was before. I use custom items to trigger my rules, but that's not really working well now. |
This PR re-works the mechanism to automatically send item state events automatically when a command event was received.
Bindings (i.e.
ThingHandlers
via theThingHandlerCallback
) can set a AutoUpdatePolicy for any channel:The behavior of the AutoUpdateManager can be summarized like this:
changeStateTo
keepCurrentState
Therefore, in comparison to the previous implementation there are two major improvements:
Sending automatic state updates before the binding can report back the real device status always happened based on the optimistic assumption that "everything will be alright". This doesn't hold true though when the framework already knows that this can't work, e.g. when the channel or thing does not exist, the ThingHandler is not there (yet) or the thing is in OFFLINE mode. In such cases there will be no automatic state update sent anymore.
In the OPTIMISTIC case where the framework can assume that the binding eventually will send such a state update itself, by default now there won't be a automatic state update event sent out anymore. Instead, the prediction callback will be informed so that UIs still can react nicely, but the "real" item state is not affected anymore. This has the huge advantage that e.g. persistence services won't get fed anymore with such "false", optimistic state updates but the item state rather represents the "real" values. RECOMMENDED and REQUIRED cases remain the same, events are still sent out as before.
As a rough summary, these are the changes on technical level:
fixes #595
fixes #4356
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com