Rich is talking about relative timing as a concern.
Let’s say you have a rule triggered by “ItemX received change”
And
ItemX is persisted on “everyChange”.
When ItemX does change, we’ll start a race …
The rule begins and after a little while examines ItemX.previousState() as recorded in the database…
meantime, and in parallel
Persistence writes away the change to the database.
Previous State will certainly retrieve something - but will it be the data from before or after that most recent change? Nobody knows. Will it even be consistently one or the other? Nobody knows.
In the test code I showed you earlier,it logged out the timestamp of the retrieved previousState. There was a reason to do that.
val PointType phoneLocation = MartinsiPhone_Location.state as PointType
val PointType lastLocation = previousState as PointType
logInfo("last Location", lastLocation.toString())
logInfo("Location", phoneLocation.toString())
but it fails with the log:
[ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID '110aec41c5' failed: cannot invoke method public java.lang.String org.openhab.core.library.types.PointType.toString() on null
It ‘knows’ what Item triggered the rule.
So it doesn’t work if there was no triggering Item - like with a time cron trigger.
And if some other Item triggers the rule, then it points to that and cannot guess you meant something else.
Finally it only works for a changed trigger - no change, logically no previous state.
OK, there’s the missing link for me. It’s referring to the trigger item.
So my rule is triggered by Location change, but also Network Presence (so when I join and leave my WiFi network). Presumably I need to then write some kind of if statement to deal with that?
I’m not sure this is any simpler than my method which is working right now. Except for the fact you say it needs to reference the database again unnecessarily?
val PointType lastLocation = if(triggeringItemName == "MartinsiPhone_Location") previousState as PointType else MartinsiPhone_Location.previousState().state as PointType
Note you might want to use previousState(true) so it returns the most recent state that is different from the current state.
It’s working for you, perhaps, for now. It likely won’t work for you forever nor is it likely to work for everyone because the timing will be different. Since this is a tutorial, every effort should be made to make sure it works for everyone as written.
As long as your rule depends on .perviousState().state to return the state of the Item immediately before the state that triggered the rule there will be cases where it fails and it returns the state two times before the current state or possibly it returns just the current state. It all depends on correct timing and the timing is not deterministic.
It is OK to depend on the database in the case where the rule is triggered by joining the network because the values for the phone’s location didn’t just change in the milliseconds before the rule was triggered. We don’t have that save operation taking place in parallel with the rule running. There is no race condition.
If simplicity is what you are after though i’d approach the rule a little differently.
Add a condition to not run the rule at all when iPhone_NetworkPresence_Online is ON. Move all the NetworkPresence stuff to another rule. There is no need to keep calculating the address when you already know you are home. When this Item does go to OFF, you almost certainly have not moved more than 450 M so there is really no need to run this rule until the first phone has changed location event occurs.
Add a condition not to run the phone location rule when it’s OFFLINE as well. We’ll put the test for OFFLINE in a separate Rule which populates a Switch Item we’ll call iPhoneThingStatus
Now that the rule is only running when we know the phone is not already at home and we know that the phone is online, all we have to do is check to see if it’s moved enough to make the call to get the address, calculate distance from home, and calculate the speed.
I don’t have access to my OH pages at the moment so this is just typed in and may contain typos.
triggers:
- id: "2"
configuration:
itemName: iPhone_Location
type: core.ItemStateChangeTrigger
conditions:
- inputs: {}
id: "2"
configuration:
itemName: iPhoneThingStatus
operator: =
state: ON
type: core.ItemStateCondition
- id: "3"
configuration:
itemName: iPhone_NetworkPresence_Online
operator: =
state: OFF
type: core.ItemStateCondition
actions:
- inputs: {}
id: "1"
configuration:
type: application/vnd.openhab.dsl.rule
script: >
logInfo("Location", "Location data changed. Running location script")
// Based on looking at the source code for PointType, altitude should be ignored.
// If this doesn't work open a new thread to explore as an issue may need to be filed.
// You certainly do not need to create a new PointType. You can just zero out the Altitude.
val currentLocation = (newState as PointType).setAltitude(new DecimalType(0))
val lastLocation = (previousState as PointType).setAltitude(new DecimalType(0))
val distanceTraveled = currentLocation.distanceFrom(lastLocation) as Number
logInfo("Location", "Distance from Last Location: " + distanceTraveled)
if(distanceTraveled > 450) { // we don't need to test the online state or Things status, if they weren't right the rule wouldn't be running
// geocode stuff goes here
}
val homeLocation = new PointType(...)
val distanceFromHome = currentLocation.distanceFrom(homeLocation) as Number
iPhone_DistanceFromHome.postUpdate(distanceFromHome)
logInfo("Location", "Distance from home: " + distanceFromHome)
// Speed calculation goes here.
Theory of operation: The rule triggers when ever the iPhone’s location changes. If and only if the iPhone is ONLINE and not being seen as connected to the home network does the rule run. The rule calculates the distance traveled using the implicit variables and if that distance is large enough, pulls the new geocode address. Finally it calculates the distance from home and the speed. Because of the Conditions, we don’t need to worry about whether the iPhone is online or at home already.
Things to notice:
There are very few indentations or if statements. The number of indentations you have in the code is a visual way to roughly gage the complexity of a piece of code. Fewer lines indented, and fewer levels of indentation means the code is less complex. Less complex is easier to understand and maintain in the long run.
The code fails early. In UI rules we have these nice Conditions where we can make sure the rule doesn’t run unless the conditions are right. In this case if you already know you’re home or the phone is offline there’s nothing to calculate. By handling that in the Conditions, that means we don’t need to test for that stuff inside the Script Action which makes it simpler and easier to understand.
There are absolutely no timing issues to be concerned about in the above code. newState and previousState are implicit variables which get set prior to the rule running. These will always contain the current location and the prior reported location. No problems with stuff being written to and read from a database in the background to worry about. No dependency on an update posted to an Item, which happens in the background as well and takes some time, completing before the next line of code runs. The above code will run in a deterministic manner no matter what else is going on on your machine and no matter how fast or slow of a machine you are running it on. Anything that depends on timing to work out right will always be flakey.
The distanceFrom method is supposed to work even when an altitude is set on the PointType. If it’s failing we need to know how it’s failing (error in the logs?) and file an issue. And as a temporary work around you should be able to just zero out the altitude in both points and not need to create a brand new one.
But we have lost the code that handles setting the location to HOME and iPhoneThingStatus to ON/OFF. But, by moving these to separate rules we can now make them without even needing to write any code at all. Of course you could write these with Script Actions but they would be single lines of code so why bother?
Let’s handle the Thing status first. First a rule to set the Item to ON when the Thing status changes to ONLINE.
Now you have an Item that represents the ONLINE status of your iPhone and no scripting code was required.
Finally, we want to set the location to “HOME” when the network binding changes to ON. When it changes to OFF, your location rule will run and update the current address next time the location is updated.
In programming, the key to making things simpler and easier to understand is to divide and conquer. Break the problem into smaller pieces. The sum of complexity for solving the smaller pieces will be less than the complexity to put it all in one function/rule.
That line of code works as expected, but when I try it as
val PointType lastLocation =
if
(triggeringItemName == "MartinsiPhone_Location")
previousState as PointType
else
MartinsiPhone_Location.previousState(true).state as PointType
logInfo("last Location", lastLocation.toString())
the script fails with:
[ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID '110aec41c5' failed: cannot invoke method public abstract org.openhab.core.types.State org.openhab.core.persistence.HistoricItem.getState() on null
Well, I put it all on one line for a reason. That if(condition) value1 else value2 is a special operator called the ternary operator. It must be all on one line.
If you want to break it up into separate lines:
val PointType lastLocation = null
if(triggeringItemName == "MartinsiPhone_Location"){
lastLocation = previousState as PointType
}
else {
lastLocation = MartinsiPhone_Location.previousState(true).state as PointType
}
[ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID '110aec41c5' failed: cannot invoke method public abstract org.openhab.core.types.State org.openhab.core.persistence.HistoricItem.getState() on null
var PointType lastLocation = null
if(triggeringItemName == "MartinsiPhone_Location"){
lastLocation = previousState as PointType
}
else {
lastLocation = MartinsiPhone_Location.previousState(true).state as PointType
}
logInfo("last Location", lastLocation.toString())
OK, that’s an error from persistence. It didn’t return a result in your call to previousState. That can happen when there is no data in the database or it’s been too long since the Item changed state and it can’t go back far enough to find a value that’s different. It can also be caused when the database is down or something like that.
You’ll need to narrow down the cause of the error.
A first thing to do is see what previousState is returning by logging it out before calling state. Next is to see what’s different between previousState() and previousState(true).
Without the true it returns the GPS location,
with the true it returns [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID '110aec41c5' failed: cannot invoke method public abstract org.openhab.core.types.State org.openhab.core.persistence.HistoricItem.getState() on null
Am I right in thinking I should keep rrd4j for most uses. eg Graphs. And just use MapDB for this?
I made two files:
mapdb.persist
// persistence strategies have a name and definition and are referred to in the "Items" section
Strategies {
everyMinute : "0 * * * * ?"
everyHour : "0 0 * * * ?"
everyDay : "0 0 0 * * ?"
// if no strategy is specified for an Item entry below, the default list will be used
default = everyChange, everyUpdate
}
/*
* Each line in this section defines for which Item(s) which strategy(ies) should be applied.
* You can list single items, use "*" for all items or "groupitem*" for all members of a group
* Item (excl. the group Item itself).
*/
Items {
MobileDevices* : strategy = everyChange
}
and rrd4j.persist
// persistence strategies have a name and definition and are referred to in the "Items" section
Strategies {
everyMinute : "0 * * * * ?"
everyHour : "0 0 * * * ?"
everyDay : "0 0 0 * * ?"
// if no strategy is specified for an Item entry below, the default list will be used
default = everyChange, everyUpdate
}
/*
* Each line in this section defines for which Item(s) which strategy(ies) should be applied.
* You can list single items, use "*" for all items or "groupitem*" for all members of a group
* Item (excl. the group Item itself).
*/
Items {
* : strategy = everyChange, everyUpdate
SceneSelection, HouseAlarm*, AllLightGroups*, MobileDevices* : strategy = restoreOnStartup
}
MapDB cannot satisfy previousState(true),it stores only one state record.
MapDB cannot provide charting, it stores only one state record.
You could use MapDB for your purpose here.
Edit the mapdb.persist file so that it never stores your Item.
(You could keep restore on startup if you wish)
Use your rule to run when the Item changes, do whatever it is you do with previousState() and then persist the new value under the control of your rule.
That way, the results are predictable.
If you really want a proper historical record though, you will have to use some other service - influx or mysql are popular.
Yes. After exploring this a little bit now, I think you are right.
I’m going to have a go at doing this in the next few days and will update my original post when I get something working.
Thank you.
