Try to be careful with your indents. It really makes it easier to tell what code is in what context. When you have an opening { all the code should be indented. The closing } should line up with the the line that started it.
And it should be consistent through the whole file. If you are using an indent of 4, you should always indent by 4 spaces (2 and 4 are the most common indents) and never have a closing } on any but a column divisible by 4. You’ve a mix of 2 and 4 indents and the closing } are all over the place and not every line is properly indented.
This isn’t just to make the code look pretty. It’s vital for you, the human coder, to understand (for example) where the code that will run when the if evaluates to true starts and stops.
For example, this:
var Number temp = 0
if (Temperature_Setpoint_Mode.state == 1) {
// off
temp = 0
Sonoffdual02P1.sendCommand(OFF)
Sonoffdual02P2.sendCommand(OFF)
} else if (Temperature_Setpoint_Mode.state == 2) {
// on
temp = Temperature_Setpoint.state as DecimalType
}
Should be
var Number temp = 0
if (Temperature_Setpoint_Mode.state == 1) {
// off
temp = 0
Sonoffdual02P1.sendCommand(OFF)
Sonoffdual02P2.sendCommand(OFF)
} else if (Temperature_Setpoint_Mode.state == 2) {
// on
temp = Temperature_Setpoint.state as DecimalType
}
If the problem is with a wrongly placed closing bracket this will become apparent once you fix the indents.
It can also be helpful to label the closing brackets so you know which opening bracket it goes with
var Number temp = 0
if (Temperature_Setpoint_Mode.state == 1) {
// off
temp = 0
Sonoffdual02P1.sendCommand(OFF)
Sonoffdual02P2.sendCommand(OFF)
} // Mode == 1
else if (Temperature_Setpoint_Mode.state == 2) {
// on
temp = Temperature_Setpoint.state as DecimalType
} // Mode == 2
This is particularly useful when the code is long enough that the opening bracket is off the screen.
It can also be helpful to avoid creating contexts. For example, check for a valid temp and if there isn’t one send the off commands and return;. Then you don’t need to indent the code that runs when the temp is valid at all.
if ( Sonoffdual02_temp.state == NULL ||
Sonoffdual02_temp.state == UNDEF ||
Sonoffdual02_temp.state <= 0) {
// temp sensor broken, turn off heater for safety
Sonoffdual02P1.sendCommand(OFF)
Sonoffdual02P2.sendCommand(OFF)
return;
}
// Temperature sensor is valid
var Number temp = 0
if (Temperature_Setpoint_Mode.state == 1) {
...
One thing I like to suggest is to use Strings instead of Numbers for things like Mode. Then you can use meaningful names in your code instead of needing to remember that 1 is ON and 2 is OFF (if these are the only two modes, why not use a Switch?).
In fact since 1 is OFF and you do the same thing for OFF as you do for a broken sensor, you can get rid of that whole if statement and just add another conditional to the first if statement.
if ( Temperature_Setpoint_Mode.state == 1 ||
Sonoffdual02_temp.state == NULL ||
Sonoffdual02_temp.state == UNDEF ||
Sonoffdual02_temp.state <= 0) {
// temp sensor broken, turn off heater for safety
Sonoffdual02P1.sendCommand(OFF)
Sonoffdual02P2.sendCommand(OFF)
return;
}
// On and temp sensor is valid
var temp = Temperature_Setpoint.state as Number
// HEAT
if(Sonofffdual02_temp.state < (temp - 0.5)) {
...
Use meaningful names for your Items. Instead of Sonoffdual02_temp which doesn’t really mean much on it’s own, why not a name like Fermenter_temp? Maybe someday you’ll replace the Sonoff with a Shelly or something. What it does hasn’t changed but because your old Item name was tied to the technology used either the Item name will be incorrect or it will need to be changed.
I find applying Design Pattern: How to Structure a Rule greatly helps to simplify Rules like this which also greatly helps with debugging problems.
We’ve already got step 1 sorted above with the return;. So what we need to do is separate the calculation of what needs to be done and actually doing it.
then
// 1. Turn everything off and exit if we can't determine what to do or we are off.
if ( Temperature_Setpoint_Mode.state == 1 ||
Sonoffdual02_temp.state == NULL ||
Sonoffdual02_temp.state == UNDEF ||
Sonoffdual02_temp.state <= 0) {
// temp sensor broken, turn off heater for safety
Sonoffdual02P1.sendCommand(OFF)
Sonoffdual02P2.sendCommand(OFF)
return;
}
// 2. Calculate what state the heater and cooler should be in.
// Initialize variable to the current state. If there is nothing to do nothing will change.
var p1Cmd = Sonoffdual02P1.state
var p2Cmd = Sonoffdual02P2.state
var currTemp = Sonoffdual02_temp.state as Number // saves typing
// If under the setpoint minus hysteresis turn off the cooler (shouldn't have been on anyway) and on the heater
if(currTemp < (temp - 0.5)) {
p1Cmd = ON
p2Cmd = OFF
}
// If heater were ON and we reached the setpoint turn off the heater
else if(p1Cmd == ON && currTemp >= temp) p1Cmd = OFF
// If over the setpoint turn off the heat (shouldn't have been on anyway) and on the cool
else if(SonoffDual02_temp > (temp + 0.5)) {
p1Cmd = OFF
p2Cmd = ON
}
// If the cooler were ON and we reached the setpoint turn off the cooler
else if(p2Cmd == ON && currTemp <= temp) p2Cmd = OFF
// 3. Do it, command the relays but only if they are not already in the needed state
if(Sonoffdual02P1.state != p1Cmd) Sonoffdual02P1.sendCommand(p1Cmd)
if(Sonoffdual02P2.state != p2Cmd) Sonoffdual02P1.sendCommand(p2Cmd)
end
The way it works is if the temp sensor is bad or the mode is OFF, turn off both the heater and cooler and return.
If we didn’t return we know the temp is valid and the mode is ON so we don’t need to have an if to check for that. Now we want to determine whether to turn on or off the heater or cooler based on the temp. So we initialize a variable with the current state of the Switches.
The order of the if/else checks is important. They will evaluate from top to bottom. So we first check to see if the currTemp is below (temp - 0.5) and if so turn ON the heating and off the cooling.
If that clause doesn’t run we know that the temp is above (temp - 0.5). Next we check to see if the heater is ON but the temp is at or above the target temp. If so we turn off the heater.
If that clause doesn’t run we know that the currTemp is above temp and the heater is not ON. Now we check to see if currTemp is greater than (temp + 0.5) and if so we turn on the cooler and off the heating.
Finally if that clause didn’t run we know that the currTemp is between (temp + 0.5) and (temp - 0.5) and the heater is not ON. If that were not the case than one of the other three clauses would have run already and this one skipped. When this is the case we want to turn off the cooler if it’s on and the currTemp is below temp.
In all other situations we want the heater and cooler to keep on doing what ever they were doing.
Now that we are past the checks and know what state the heater and cooler need to be in, we sendCommand to both devices, but only if they are not already doing what they need to be doing.
I believe the above version is easier to understand and modify should you need to in the future.
It is hard to say because the indents are all messed up and I don’t want to go through and reformat it properly, but I suspect the problem is else if (Sonoffdual02P1.state > temp) {. That will evaluate to true when the cooler is supposed to come on which will prevent the code that actually does turn on/off the cooler from running.