I’d be interested to see the code. I’m not pushing the Rules DSL but I probably am one of the few experts on them running around. I usually find that looping like that is caused more by not taking advantage of events and/or Timers and have less to do with a limitation of the language.
Are you referring to identifying the Item that triggered a Rule using lastUpdate? Or just generally MyGroup.members.filter[i.name == "SomeName"].head? Because the latter isn’t some workaround, it is a core feature of the language. The former is indeed a work-around though. 
I’ve never liked it but it works reasonably well.
val is identical to final in Java. I don’t understand why you would have a problem with that.
I actually do recommend developers go with JSR223 because they tend to try to force the Rules DSL to bend to the Java/C#/C/Python/Name your language approach than to try and learn and take advantage of the way the Rules DSL wants to do things. But be aware that the JSR223 binding is still very much beta, perhaps even alph at this stage. You will have to figure a lot out on your own. There simply isn’t that much documentation on it yet and I’m not sure it is feature complete with the OH 1.x JSR223 yet. Plus there is a looming issue where JSR223 goes away in Java 1.9 and is replaced by something else.
Multitail is fantastic for this. See this thread to get color coding for OH logs.
You can just hit <enter> and get a big read line across all the logs you are following. And it lets you connect to a remote machine and tail a log there over ssh.
That would be fantastic. We LOVE examples and I’m very happy to help make people’s rules more effecient.
To help you get your head around the Rules DSL way of wanting you to do things, look through the Design Pattern postings.
Here are a few comments on your code thus far:
-
Use code fences. Either highlight your code and press the
</>button at the top of the post enter text area or use three ``` at the top and the bottom of your code and it will retain indentation and add some syntax highlighting. -
You don’t need to import anything from joda in OH 2.
-
Your Zone Heat probably only needs to run when either gTemperature or gThermostat change. Assuming you set up your gTemperature and gThermostat groups such that they will change when any of their members change (e.g. AVG would probably do it) this is one of the cases where using changed on a Group would work as expected.
-
I always forget about findFirst. That is so much better than using a filter like I did above. Thanks for reminding me.
-
Use the log actions rather than println and your logs will go to openhab.log. You can even configure the logs to go to their own file if you want. See http://docs.openhab.org/administration/logging.html#create-log-entries-in-rules
logInfo("logname", "message") -
Your “Zone Heat” rule looks pretty tight. I would write it like the following though it is not significantly different and any mneaningful way.
val deadzone = 0.5 // I would probably put this into an Item with restoreOnStartup so I could adjust it as desired on the sitemap
gRoom.members.forEach[room |
logInfo("heating", "Processing zone for '" + room.name + "'")
val zone = gZoneHeat.members.findFirst[name.equals("iZoneHeat_" + room.name)]
val actual = gTemperature.allMembers.findFirst[name.equals("iTemperature_" + room.name)].state
val target = gThermostat.allMembers.findFirst[name.equals("iThermostat_" + room.name)].state
// Fail fast and avoid unnecessary nesting
if(actual == NULL || target == NULL) {
logWarn("heating", "Heating temp and target are not initialized: temp = " + temp.state + " therm = " + therm.state)
return false; // this is one place the ; actually means something
}
val diff = target as Number - actual as Number
if(diff > deadzone) {
logInfo("heating", "Current temp is more than deadzone lower than target, turning on the heat")
zone.sendCommand(ON)
}
else if(diff < 0){
logInfo("heating", "Current temp is more than deadzone higher than target, turning off the heat")
zone.sendCommand(OFF)
}
// else do nothing
]
It has fewer nestings and therefore would have a lower Mccabe complexity rating. It uses the built-in logging. And it avoids some unnecessary variables by jumping straight to storing the state in a var instead of grabbing the Items first in one val and then its state in another.
Again, it is questionable whether this is an improvement or not. It might be a few lines of code shorter.
-
While not really a problem, it is usually good practice to avoid sendCommand to an Item if it is already in that state. Some devices it can cause problems for but over all it clutters your logs and can unnecessarily trigger some rules and such.
-
“Boiler Control” could be shortened with another filter to
val numOn = gZoneHeat.members.filter[zone|sone.state == ON].size
val newCommand = if(numOn > 0) ON else OFF
if(iBoiler_Heating.state != newCommand) {
logInfo("heating", "Requesting boiler to turn " + newCommand)
iBoiler_Heating.sendCommand(newCommand)
}
Alternatively, if you define gZoneHeat using Group:Switch:OR(ON,OFF) gZoneHeat than gZoneHeat’s state will be ON if any member is ON and the rule would become:
when
Item gZoneHeat changed
then
logInfo("Heating", "Requesting boiler to turn " + gZoneHeat.state)
iBoiler_Heating.sendCommand(gZoneHeat.state)
end
We are trusting that gZoneHeat and iBoiler_Heating will always stay in sync so don’t need to test if iBoiler_Heating is already in the right state.
Note, this will also keep the rule from triggering a bunch of times if more than one Zone needs heat as gZoneHeat will only change when the first zone turns ON or ALL zones turn OFF.
I do think this is an improvement in both number of lines of code and in the code’s behavior.
- I usually recommend the [Deprecated] Design Pattern: Time Of Day for doing the sort of schedule modeling you do in “Update Thermostats” This separates the code so you can easily change things up and it lets you use the same time based states in other rule areas (e.g. lighting). Finally, it lets you take advantage of other events like presence and Astro events more easily. Since you are hard coding the schedule anyway in a JSON string, you can use cron triggers to drive it rather than a looping 15 minute poll.
It will take some modification of the example of the DP but should be pretty easy, especially since you are a coder and probably have seen and used design patterns before.
-
Having said the above, you can also save your JSON to a text file and load it into your rule with:
val schedule = executeCommandLine("cat /path/to/schedule", 1000) // or equivalent if running on Windows -
Personally, what I would do is store your default temp, start, and end times in Number and DateTime Items. You can populate them in a System started Rule to start and use restoreOnStartup to persist their values if desired and get rid of the System started Rule. Then if you name them using Design Pattern: Associated Items (which you’ve already done above in “Zone Heat”) the rule becomes
// loop through each thermostat
gThermostat.members.forEach[therm |
// Get a list of all the start times for this thermostat
val startTimes = gScheduleItems.members.filter[i|i.name.startsWith(therm.name+"_StartTime"]
// Loop through each start time
startTimes.forEach[start |
// Get this start time's number
val num = start.name.substring(therm.name+"_StartTime".length) // may be off by one, double check
// Parse and get the actual startTime
val startTimeStr = parseInt(start.state.toString).split(":")
val startHours = Integer::parseInt(startTimeStr.get(0))
val startMins = Integer::parseInt(startTimeStr.get(1))
val startTime = now.withTime(startHours, startMins, 0, 0)
// Parse and get the actual endTime
val endTimeStr = gEndTimes.members.findFirst(therm.name+"_EndTime"+num).state.toString.split(":")
val endHours = Integer::parseInt(endTimeStr.get(0))
val endMins = Integer::parseInt(endTimeStr.get(1))
val endTime = now.withTime(endHours, endMins, 0, 0)
// If now is between startTime and endTime
if(now.isAfter(startTime) && now.isBefore(endTime)) { // spanning midnight is a special case that need special handling
// Get the target temp
var targetItem = gTargetTemps.members.findFirst(therm.name+"_Target"+num)
// No target temp, fall back to Default
if(targetItem == null) {
logWarn("heating", "Could not find " + therm.name+"_Target"+num+" attempting to use default")
targetItem = gTargetTemps.members.findFirst(therm.name+"_Default"+num)
// No Default, fall back to hard coded default
if(targetItem == null) {
logWarn("heating", "Could not find " + therm.name+"_Default"+num+" using 10")
}
}
var targetTemp = if(targetItem == null) 10 else targetItem.state as Number
if(therm.state as Number != targetTemp) therm.sendCommand(targetTemp)
else logInfo("heating", therm.name + " is already at the target temp")
}
]
]
I do think this is an improvement on the original. It has several levels less nesting. I THINK it does the same thing your current one does. And it is far fewer lines of code. You can change the schedule without modifying the Rule and since everything is stored in Items you don’t have as much error checking that you have to do. It does come at the cost of a host of new Items to create and populate though.
However, I would probably not implement it myself like that above. Instead, I would probably create separate cron triggered rules for each start/end/zone and use a lambda to encapsulate the if(currTarget != new target) logic. Since your schedule is hard-coded anyway you can eliminate almost all of the looping and filtering and parsing of date time strings in exchange for a bunch of new, but relatively simple rules.
val Functions$Function2<SwitchItem, Number, Boolean> heating = [therm, tgt |
if(therm.state as Number != targetTemp) therm.sendCommand(targetTemp)
else logInfo("heating", therm.name + " is already at the target temp")
true
]
rule "iThermostat_gLivingRoom start 1"
when
Time cron "0 0 6 * * ?"
then
heating.apply(iThermostat_gLivingRoom, 20)
end
rule "iThermostat_gLivingRoom end 1"
when
Time cron "0 30 7 * * ?"
then
heating.apply(iThermostat_gLivingRoom, 10)
end
rule "iThermostat_gLivingRoom start 1"
when
Time cron "0 30 16 * * ?"
then
heating.apply(iThermostat_gLivingRoom, 20)
end
rule "iThermostat_gLivingRoom end 1"
when
Time cron "0 0 21 * * ?"
then
heating.apply(iThermostat_gLivingRoom, 10)
end
...
In a case like this, I’m willing to give up a little bit in terms of length and adding lots of rules if it makes the overall logic simpler. And the above logic is really simple when compared to your original or my reworking. Simple rules are easir to reason about, debug, and maintain. I’m just not sure that it is worth writing the rules in the complicated monolith when compared to the simplicity of using separate rules. But that is my opinion.
And I notice that most of your start and end times overlap so you can combine rules where it makes sense (e.g. call heatng.apply for all the thermostats that start at 16:30 in the one rule). Also, you can use globals or Items to store the target temps if desired. But since you know which termostat is being commanded you don’t have to go through any Group filtering or parsing or the like to get the value.