-
-
Notifications
You must be signed in to change notification settings - Fork 422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add getLastChange
, getLastUpdate
, and getPreviousState
to GenericItem
#4351
base: main
Are you sure you want to change the base?
Conversation
2cadc5e
to
75bc9c9
Compare
I like the idea of this information being part of the Item. Of course it won't go back beyond OH startup but something is better than nothing. It certainly handles this use car better than persistence I think given all the different ways perspective can be configured. How does this interact with restoreOnStartup? If an item is restored and not changed does this return the restored time or null? What makes the most sense? I'm not sure I know (it's close to bed time here so maybe I'll sleep on it). |
75bc9c9
to
0c7eeed
Compare
It is not aware of persistence restoration, so I guess once persistence restores the state, it will return the time the state was restored. |
Imho it should be possible to query these values from the db so restore on startup should work as expected. |
@spacemanspiff2007 would you mind elaborating this please?
Good point. I will add them there too. |
It's possible to query the last persisted state from persistence and subsequently restore this state as the current state on startup. It's also possible to query the persisted state before that. |
Yes, isn't this what
I don't quite understand the above statements. This PR doesn't interfere with persistence's |
I think it's a misunderstanding. |
…icItem Signed-off-by: Jimmy Tanagra <[email protected]>
0c7eeed
to
178df65
Compare
@spacemanspiff2007 I've added it to the REST response {
"link": "http://192.168.1.10:8484/rest/items/TestSwitch1",
"state": "ON",
"previousState": "NULL",
"lastUpdate": 1723872081231,
"lastChange": 1723871965049,
"editable": false,
"type": "Switch",
"name": "TestSwitch1",
"tags": [],
"groupNames": [
"TestSwitches"
]
} |
That depends on the persistence service. And it is not the case for mapDB, the most used persistence service for restoreOnStartup. I would argue that, when the item state is updated as a consequence of restoreOnStartup, it should not update these lastChange and lastUpdate fields because now is probably the wrong time. But I am not sure this is feasible. I doubt the item knows anything about the source of the update/change. In general, I think having the extra fields is a good idea. I just feel like we are still not there to give an answer to a simple question: when did my item last change?
In an ideal world, mapDB would just store the lastChange time of an item with the state and we could restore that together with the state. I am just not sure it is easily feasible. We can keep adding layers to this, but there are always issues and exceptions. I like the idea of keeping lastChange and lastUpdate with the item. But I think the real simplification comes when we have a simpler restore mechanism on startup. Because than, you always get the right answer. Other persistence services then have the focus on data for graphing and calculations, which makes sense in my view. |
OK, I think I now understand what @spacemanspiff2007 was alluding to, thanks to @mherwege's explanation.
Not a bad idea. |
Why would this not be feasible? I beleive all the apis already exists in persistence to get the time from persistence. Once you have that it's just a matter of adding the last change and last update time as a parameter on the Item.
I'm not sure we can make that case that's it's always only used for restoreOnStartup. It's useable if you only care about the most recent change to the Item in a rule (given it's current default strategies). One could change the strategies and it would work for most recent update. But if these properties are added to the Item and we go down a path where those are made available and accurate on the the Item this use case is handled.
Be careful here. Not all uses want all Items restored. Not all users want all Items restored to their most recent change or update (e.g they use a rule with .persist to control when the state gets saved). These use cases are now currently supported. Any move of restoreOnStartup to core would need to handle these use cases too. How the Item's state is saved and which Items are restored needs to be configurable on an Item by Item basis like it is now in persistence. |
It would be trivial to modify mapDB persistence service to save the additional three values. A quick glance at the docs shows it should be possible. If not it's always possible to name mangle additional values with a separator that's not a valid item name (e.g.
No - it should return
I strongly agree. For many devices that report the state themselves I explicitly do not save the item state so I don't have the wrong state after startup. If the restore makes sense depends strongly on the device, the user and how the rules are written. Just a hint: That would also result in a proper behavior when restoring from persistence.
So I am basically suggesting the opposite of what I wrote above 🙈 🤣 |
Each item can have a special metadata |
I think, what @rkoshak also implied is that one may want to use different rules for persisting values, not just on every change, but e.g. persist it once and always use that same value at startup. And that would mean we also need to support that part of the configuration in core. And then we are back to doing it the way it is done now anyway. Thinking further about it:
With the above logic in place, it also would make sense to revisit the persistence extension actions to not return null, but return what is actually in the database, and document it as a breaking change, advising users to use the Item methods for this use case. |
Looking at MapDB code, it is actually not that difficult. The item state and time are already stored as a JSON string. Adding extra fields should not be a problem. |
Since a user needs a rule for calling This new core service doesn't need to do all the use cases. We just need to ensure that all the use cases are covered somehow. With #4324 I think we will reliably be able to count on persistence being there by runlevel 100. So maybe a system started rule with
Maybe we are thinking too hard about this. What do we intend these new properties to represent? I see two options:
If it's just 1, we don't really need to worry about most of these complexities. If an Item doesn't change after OH startup, If we want to make 2 happen then it gets much more complicated, as we can see and we probably need something built into core. But do we really need to solve 2 here? Maybe 1 is sufficient? I think users who prefer not to use persistence at all would love to have 1. We also need to thing of second tier impacts like the increase in writes for those running on SD cards and the like if this core restoreOnStartup feature becomes the default behavior. |
So what's the next step?
If we are going to proceed in this direction, I would suggest:
|
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 am fine with this. |
I think this is the best way, too. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/lastupdate-and-null-values/158134/6 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/how-to-monitor-if-item-is-frequently-updated/158260/6 |
how do we resolve the access to these methods vs the persistence methods with the same name within rulesDSL? |
Maybe I miss something here, but what gets called would be determined by the imports I would think. As both items and persistence are imported by default, it indeed leads to a conflict which will force changing the rules to have a fully qualified name. The options then become:
In both cases, rules would work without being changed, but 2 would have a slight change in behaviour. I would still prefer 2 because it is more natural not to call persistence if not needed. If one absolutely wants the value from persistence the rule would need to be changed, but it would be very clear what is expected. At the same time, the persistence actions could be modified again to not return null, but the last effective persisted value. |
I think I like this option too! |
Should you not rather have these methods on the Channels rather than the Items? In that case one would be able to create respective Items with (say) a profile that reads the Channel's getLastUpdated (say) property. |
That would make it impossible to restore these values from persistence at startup. Items have a state, channels only pass on a state. I don´t think we should do this. |
If you insist on doing it at Item level, then I think you need to find a mechanism whereby the new values from these functions can be accessed in the UI. (e.g. via a profile). Otherwise they are only accessible via rules.. |
They are available through the REST API and it is already a part of this PR. |
^ |
I don't see why not. It's available in the REST response, along with the current item state |
I take that to actually mean that this PR does NOT have a mechanism for that. Or?? I think the Item shall have an attribute that selects if the 'display value' of the item is its actual value (as currently) or the respective value from one of these new methods. This would allow the Item to be configured in OH core, and it's display value in (say) a sitemap would simply depend on the Item config. i.e. it would simply work as a drop in replacement for existing sitemap code. |
I would expect to get all values from the Item through the Items endpoint, so the UI (Main UI) could display all values. |
Same for widgets too. |
Yes, I don’t want to transfer too much over SSE. |
No, that would need to be done in the UI itself |
Coming back to my original point, I believe that these proposed new properties are architecturally speaking attributes of the Channels and NOT attributes of the Items. (Your argument about persistence notwithstanding). And when an Item is created on that Channel, you can select whether it pulls the actual state, or the previous state, or the instant of last change or last update. Whereby that selection is made via a profile, in the normal OH manner. And it requires no downstream changes in MainUI or any other UI. IMHO the current PR is a kludge; it breaks the OH architectural logic; and (therefore) will require a whole lot of other kludges downstream from this. |
Items have states. The state can be set by a Thing through a channel, but it can also be set by a rule or through the rest api. A virtual item, one without any channels, can still have a state. Furthermore, all 4 attributes (of the item) are needed at the same time
Moving these to the channel makes no sense to me and it means that the linked item state can only hold one of the 4 attributes. That would completely miss the point of this PR. |
And keeping these in an Item makes no sense to me. It means that the respective Item's values for these properties are not accessible and cannot be displayed in UIs such as site maps or widgets using the normally accepted widget/sitemap design semantics. |
^
|
We don't to propose to do this at all. This PR is not meant to help with UI representation of these properties. This PR is meant to solve a long standing issue with getting these values in rules. As of now, persistence needs to be used for that. But this does not work well and often gives the wrong result, or no result at all. By keeping it with the item state, we don't need to query persistence for this anymore. I would argue what you would want is another problem to solve, and there may indeed be arguments for that. But doing what you ask in this PR will make it impossible to solve the issue this PR is intending to solve. |
There are many ways, but the easiest is using label format with scripting
But as @mherwege said above, this PR has nothing to do with this. You are trying to solve a different problem. |
Just to make this clear: I am not trying to solve any problem. I am trying to ensure that your 'solution' here shall not start a chain reaction of creating other new problems elsewhere. |
Since HABApp already provides the timestamps, it would be nice if those could be in sync with openHAB. |
I'm trying to understand what problem this could create. |
These methods are handy in many situations and isn't possible with mapdb persistence, so previously it would require something like influxdb.
It also solves the issue of needing to wait until persistence write operation is finished (if it was at all set up to persist on change), so this gives us a cheap and reliable / dependable way of getting the last change / last update time.
See discussions in
https://community.openhab.org/t/persistence-on-off-transitions-with-influx/156271/
https://community.openhab.org/t/blockly-persistence-last-changed-returns-no-result-quite-often/157842