I understand from a post by Rich that we will hopefully see item Groups as a thing for consolidation going forward, this is indeed good news.
I would like to know of better ways to consolidate the rule below into a ideally shorter lines of code and improved performance.
The rules is quite simple, disabling one vSwitch disables two other vSwitches and the same goes in reverse when you enable the primary vSwitch. a parent/Children switching relationship.
I previously set up a rule for every room (BR1,BR2,BR3,BR4,LR,GR.HO) and used IF statements this made for easy to read code but a larger amount of it; the rule below decreased the number of lines in total but seems to be very repetitive, I am hoping the guru’s here can advise of even better ways for this rule to evolve.
rule "Automated Temperature Control"
when
Item BR1_Climate_auto received command or
Item BR2_Climate_auto received command or
Item BR3_Climate_auto received command or
Item BR4_Climate_auto received command or
Item LR_Climate_auto received command or
Item GR_Climate_auto received command or
Item HO_Climate_auto received command
then
logDebug("autoclimate.rules", triggeringItem.name + " received a command: " + receivedCommand)
switch receivedCommand {
case receivedCommand == ON : {
switch triggeringItem.name {
case "BR1_Climate_auto" : {
BR1_Temp_auto.sendCommand(ON)
BR1_Humid_auto.sendCommand(ON)
}
case "BR2_Climate_auto" : {
BR2_Temp_auto.sendCommand(ON)
BR2_Humid_auto.sendCommand(ON)
}
case "BR3_Climate_auto" : {
BR3_Temp_auto.sendCommand(ON)
BR3_Humid_auto.sendCommand(ON)
}
case "BR4_Climate_auto" : {
BR4_Temp_auto.sendCommand(ON)
BR4_Humid_auto.sendCommand(ON)
}
case "LR_Climate_auto" : {
LR_Temp_auto.sendCommand(ON)
LR_Humid_auto.sendCommand(ON)
}
case "GR_Climate_auto" : {
GR_Temp_auto.sendCommand(ON)
GR_Humid_auto.sendCommand(ON)
}
case "HO_Climate_auto" : {
HO_Temp_auto.sendCommand(ON)
HO_Humid_auto.sendCommand(ON)
}
default : {logError("autoclimate.rules", "Unexpectedly reached default case")}
}
logInfo("autoclimate.rules", triggeringItem.name + " Climate contorl has been enabled")
}
case receivedCommand == OFF : {
switch triggeringItem.name {
case "BR1_Climate_auto" : {
BR1_Temp_auto.sendCommand(OFF)
BR1_Humid_auto.sendCommand(OFF)
}
case "BR2_Climate_auto" : {
BR2_Temp_auto.sendCommand(OFF)
BR2_Humid_auto.sendCommand(OFF)
}
case "BR3_Climate_auto" : {
BR3_Temp_auto.sendCommand(OFF)
BR3_Humid_auto.sendCommand(OFF)
}
case "BR4_Climate_auto" : {
BR4_Temp_auto.sendCommand(OFF)
BR4_Humid_auto.sendCommand(OFF)
}
case "LR_Climate_auto" : {
LR_Temp_auto.sendCommand(OFF)
LR_Humid_auto.sendCommand(OFF)
}
case "GR_Climate_auto" : {
GR_Temp_auto.sendCommand(OFF)
GR_Humid_auto.sendCommand(OFF)
}
case "HO_Climate_auto" : {
HO_Temp_auto.sendCommand(OFF)
HO_Humid_auto.sendCommand(OFF)
}
default : {logError("autoclimate.rules", "Unexpectedly reached default case")}
}
logInfo("autoclimate.rules", triggeringItem.name + " Climate control has been disabled")
}
default : {logError("autoclimate.rules", "Unexpectedly reached default case")}
}
end
rule "Automated Temperature Control"
when
Item BR1_Climate_auto received command or
Item BR2_Climate_auto received command or
Item BR3_Climate_auto received command or
Item BR4_Climate_auto received command or
Item LR_Climate_auto received command or
Item GR_Climate_auto received command or
Item HO_Climate_auto received command
then
logDebug("autoclimate.rules", triggeringItem.name + " received a command: " + receivedCommand)
val sourceRoom = triggeringItem.name.split("_").get(0)
sendCommand(sourceRoom + "_Temp_Auto",receivedCommand.toString)
sendCommand(sourceRoom + "_Humid_Auto",receivedCommand.toString)
logInfo("autoclimate.rules","{} Climate control has been {}",triggeringItem.name,if (receivedCommand == ON) "enabled" else "disabled")
end
As you mentioned, when you get to OH 2.3 (or if you use a snapshot build), you can add the items you are using in the triggers into a group (gClimateAuto) and then use this for the trigger…
However, I’ve seen your original approach used by many before so I wanted to address a way of thinking about coding that can save you some complexity in the future even when tricks like Associated Items are not available.
So let’s work on the rule as written without that but just restructure what is written and see what we can do.
I like to apply Don’t Repeat Yourself (DRY) not only across rules but within Rules as well. I suspect that is the source of your discomfort with what you have in the OP. One way to apply DRY within a Rule is to only call Actions or methods that cause side effects (e.g. sendNotification, postUpdate, sendCommand, etc) once in the Rule. This will often require restructuring your Rule along the lines of:
first determine if something needs to be done at all
then calculate the new states or messages or whatever side effects need to be performed
finally perform the side effects.
Looking at the original rule I see the overall task is to send an ON or OFF command to two other Items based on a received command. So what I would do is:
Since the Rule is triggered by a command we know that we have to do something in this Rule every time so 1. is already done.
The command that gets sent to the other Items is the same as receivedCommand so we will just use receivedCommand later in the rule. So the next part of 2. is to determine what Items to send the command to. there are lots of approaches including continuing to use the switch statement and assigning the Items to call to variables. There are other approaches I can think of but they all lead to Scott’s approach so I won’t go down that path right now.
Finally, we do the side effects which are in this case a sendCommand.
rule "Automated Temperature Control"
when
Item BR1_Climate_auto received command or
Item BR2_Climate_auto received command or
Item BR3_Climate_auto received command or
Item BR4_Climate_auto received command or
Item LR_Climate_auto received command or
Item GR_Climate_auto received command or
Item HO_Climate_auto received command
then
logDebug("autoclimate.rules", triggeringItem.name + " received a command: " + receivedCommand)
// 1. decide if we need to do something at all, in this case we always
// want to do something so nothing needs to be done here
// 2. Calculate what states need to change and what they need to change to
var temp = null
var humid = null
switch triggeringItem.name {
case "BR1_Climate_auto" : { temp = BR1_Temp_auto humid = BR1_Humid_auto }
case "BR2_Climate_auto" : { temp = BR2_Temp_auto humid = BR2_Humid_auto }
case "BR3_Climate_auto" : { temp = BR3_Temp_auto humid = BR3_Humid_auto }
case "BR4_Climate_auto" : { temp = BR4_Temp_auto humid = BR4_Humid_auto }
case "LR_Climate_auto" : { temp = LR_Temp_auto humid = LR_Humid_auto }
case "GR_Climate_auto" : { temp = GR_Temp_auto humid = GR_Humid_auto }
case "HO_Climate_auto" : { temp = HO_Temp_auto humid = HO_Humid_auto }
}
if(temp === null || humid === null) {
logError("autoclimate.rules", "Unexpectedly reached default case")
return;
}
// 3. Perform the actions that have side effects
temp.sendCommand(receivedCommand)
humid.sendCommand(receivedCommand)
logInfo("autoclimate.rules", triggeringItem.name + " Climate control has been disabled")
end
Besides being somewhat shorter, one reason that the above is better is that should you decide that you need to do something else before sending the command or provide some additional checking or the like before sending the command you only have to do it once and in one place in the Rule. That switch statement is still kind of long and ugly (hence Scott’s solution truly being the correct one) and I would look for ways to improve that but as I mentioned, all those roads lead to variations of Scott’s solution. And that isn’t the point of this post.
The point is to show how adjusting the structure of a Rule applying DRY can help one consolidate and overcome lots of repetitive code.
@rikoshak Thanks for the advise.
I figured the best way to fully understand your approach was to try it on another rule set and convert it to use your technicians.
The rule below is my failed attempt to convert an existing rule into the type you provided and it seems to get stuck around the case statements and errors regarding null.
LOG
2018-03-31 12:09:32.150 [INFO ] [arthome.model.script.BR4_Humid.rules] - Humidity Room, actioning: BR4
2018-03-31 12:09:32.160 [INFO ] [arthome.model.script.BR4_Humid.rules] - Checkpoint 0:
2018-03-31 12:09:32.173 [ERROR] [ntime.internal.engine.RuleEngineImpl] - Rule 'Humid Action': An error occurred during the script execution: Could not invoke method: org.eclipse.smarthome.model.script.lib.NumberExtensions.operator_equals(java.lang.Number,java.lang.Number) on instance: null
RULE
rule "Humid Action"
when
Item BR1_Humid received update or
Item BR1_Humid_State changed or
Item BR2_Humid received update or
Item BR2_Humid_State changed or
Item BR3_Humid received update or
Item BR3_Humid_State changed or
Item BR4_Humid received update or
Item BR4_Humid_State changed or
Item LR_Humid received update or
Item LR_Humid_State changed or
Item GR_Humid received update or
Item GR_Humid_State changed or
Item HO_Humid received update or
Item HO_Humid_State changed
then
val sourceRoom = triggeringItem.name.split("_").get(0)
logInfo(sourceRoom + "_Humid.rules", "Humidity Room, actioning: " + sourceRoom)
var _Humid = null
var _State = null
var _Heater = null
logInfo(sourceRoom + "_Humid.rules", "Checkpoint 0: ")
switch triggeringItem.name {
case "BR1_Humid" : { _Humid = BR1_Humid_auto _State = BR1_Humid_State _Heater = MQTTBR1Heater}
case "BR2_Humid" : { _Humid = BR2_Humid_auto _State = BR2_Humid_State _Heater = MQTTBR2Heater}
case "BR3_Humid" : { _Humid = BR3_Humid_auto _State = BR3_Humid_State _Heater = MQTTBR3Heater}
case "BR4_Humid" : { _Humid = BR4_Humid_auto _State = BR4_Humid_State _Heater = MQTTBR4Heater}
case "LR_Humid" : { _Humid = LR_Humid_auto _State = LR_Humid_State _Heater = MQTTLRHeater}
case "GR_Humid" : { _Humid = GR_Humid_auto _State = GR_Humid_State _Heater = MQTTGRHeater}
case "HO_Humid" : { _Humid = HO_Humid_auto _State = HO_Humid_State _Heater = MQTTHOHeater}
}
if(_Humid == null || _State == null || _Heater == null) {
logError(sourceRoom + "_Humid.rules", "Unexpectedly reached default case")
return;
}
logInfo(sourceRoom + "_Humid.rules", "Checkpoint 1: ")
logInfo(sourceRoom + "_Humid.rules", "Measured: " + triggeringItem.name.state)
//logInfo(sourceRoom + "_Humid.rules", sourceRoom + " Humidity: Measured: " + sourceRoom + "_Humid.state" + " Max: " + sourceRoom + "_Max_Humid.state" + " Tgt: " + sourceRoom + "_Tgt_Humid.state" + " Min: " + sourceRoom + "_Min_Humid.state" + " State: " + sourceRoom + "_Humid_State.state")
if (_Humid.state == ON) {
logInfo(sourceRoom + "_Humid.rules", "Checkpoint 2: ")
// set actions if we're asleep
if (sleep_state.state == OFF ) {
if (_State.state == "NULL" || _State.state == "DRY" || _State.state == "NORMAL" || _State.state == "DAMP") {
sendCommand(_Heater, OFF)}
else {sendCommand(_Heater, ON)}
}
// set actions if we're awake
if (sleep_state.state == ON ) {
if (_State.state == "NULL" || _State.state == "DRY" || _State.state == "NORMAL") {
sendCommand(_Heater, OFF)}
else {sendCommand(_Heater, ON)}
}
}
else if (_Humid.state != ON) {
logInfo(sourceRoom + "_Humid.rules", sourceRoom + " Humid control is disabled so no action taken")
}
end
[EDIT] Rule was updated with some corrections, the same error occurs.
When comparing to null, you need to use ===. I suspect this is the source of the problem. I’ve a long post about it somewhere but the tl;dr version of it is == is the same as call the .equals method on an object but null is a primitive and doesn’t have a .equals method to call so we have to use === which looks to see if the two operands have the same address in memory.
You should be seeing warnings in openhab.log when this rule is loaded that mentions you should use === with null.
All that being said, I want to emphasize that Scott’s approach is the better one and it should work here as well. Though because you need access to the Item themselves you will want to use the Group method in the Associated Item DP. Put all your Humid_auto Items into a Group and do the same for State (we only need the name of _Heater) and the rule collapses to:
then
val sourceRoom = triggeringItem.name.split("_").get(0)
logInfo(sourceRoom + "_Humid.rules", "Humidity Room, actioning: " + sourceRoom)
logInfo(sourceRoom + "_Humid.rules", "Checkpoint 0: ")
val _Humid = HumidAutos.members.findFirst[ auto | auto.name == sourceRoom+"_Humid_auto" ]
val _State = HumidStates.members.findFrist[ state | state.name == sourceRoom+"_Humid_State" ]
val _Heater = "MQTT"+sourceRoom+"Heater"
if(_Humid === null || _State === null || _Heater === null) {
logError(sourceRoom + "_Humid.rules", "Unexpectedly reached default case")
return;
}
logInfo(sourceRoom + "_Humid.rules", "Checkpoint 1: ")
logInfo(sourceRoom + "_Humid.rules", "Measured: " + triggeringItem.name.state)
// the bottom part of the rule remains the same
...
end
