Need help with rule (Velux windows controlled from outside temp, alarm and Lumiance), now optimizing

It have two problems.
First, I need to wait for an update of the netamo item, (ten minutes).
And I just looked at the logfile. There is something wrong with the alarm state… This is the error I get when temperature is firing the rule:

2018-08-08 23:09:25.437 [ERROR] [ntime.internal.engine.RuleEngineImpl] - Rule 'Automatic control of all skylight windows': Could not cast OFF to void; line 15, column 17, length 35

Alarm is OFF atm.

Line 15 is val alarm = alarm_totalalarm.state as OnOfftype?

Let’s leave off the cast. It usually isn’t needed in the first place.

val alarm = alarm_totalalarm.state

One way to test a Rule like this is to add an Unbound Switch Item as a trigger to the Rule and put that Switch on your sitemap so you can trigger the Rule by hand whenever you want without needing to wait for the other Items to update. Once the Rule works you can remove the Switch, or keep it around and use it to test future Rules.

It helped. Windows closed when netamo just updated.
Also did the switch for triggering, tiil next time :slight_smile:

2018-08-08 23:29:31.048 [vent.ItemStateChangedEvent] - NetamoUdendoersTemperature changed from 23.3999996185302734375 ℃ to 23.299999237060546875 ℃
2018-08-08 23:29:33.265 [ome.event.ItemCommandEvent] - Item 'VeluxAlleVent' received command ON
2018-08-08 23:29:37.345 [INFO ] [ding.velux.bridge.VeluxBridgeExecute] - execute() finished successfully.

Rich, if I want to insert a loginfo each time a case is run, how or where should it be placed in the rule?

So ask yourself, does it make sense to insert a log statement for each case, or would one log statement suffice? For example

    logInfo("skylight", "Sending ON command to " + velux.name + " because Temp = " + NetamoUdendoersTemperature.state + "  Lux = " + Node13_SensorLuminance.state + " and Alarm = " + alarm_totalalarm.state)
    velux.sendCommand(ON)

That one log statement contains everything you need to know to understand not only what the Rule is doing by why.

But from a syntax perspective, if you have more than one line to run for a case statement, put them in curly brackets.

switch value {
    case one: {
        // line one
        // line two
    }
    case two: {
        // line one
        // line two
    }
}

I thought it did, untill I saw you code :slight_smile:
Just forgot to think, I guess. I already have the val´s in the rule to use. So it makes sense using one line covering each case.

And it work likes a charm!

2018-08-09 00:32:24.398 [ome.event.ItemCommandEvent] - Item 'dummy1' received command ON
2018-08-09 00:32:24.409 [vent.ItemStateChangedEvent] - dummy1 changed from OFF to ON

2018-08-09 00:32:24.519 [INFO ] [ipse.smarthome.model.script.skylight] - Sending ON command to VeluxAlleVent because Temp = 22.1000003814697265625 ℃  Lux = 0 and Alarm = OFF

2018-08-09 00:32:24.524 [ome.event.ItemCommandEvent] - Item 'VeluxAlleVent' received command ON
2018-08-09 00:32:28.601 [INFO ] [ding.velux.bridge.VeluxBridgeExecute] - execute() finished successfully.

There is something wrong with the cases.
When I got up this morning, all windows was 100% open, even though the temperature isn´t high enough.
This is from the log where it opens the windows.

2018-08-09 06:03:32.706 [INFO ] [ipse.smarthome.model.script.skylight] - Sending ON command to VeluxAlleVent because Temp = 17.700000762939453125 ℃  Lux = 54 and Alarm = OFF
2018-08-09 06:03:36.957 [INFO ] [ding.velux.bridge.VeluxBridgeExecute] - execute() finished successfully.
2018-08-09 06:13:34.381 [INFO ] [ipse.smarthome.model.script.skylight] - Sending ON command to VeluxAlleVent because Temp = 17.6000003814697265625 ℃  Lux = 54 and Alarm = OFF
2018-08-09 06:13:38.568 [INFO ] [ding.velux.bridge.VeluxBridgeExecute] - execute() finished successfully.
2018-08-09 06:17:02.086 [INFO ] [ipse.smarthome.model.script.skylight] - Sending ON command to VeluxAlleAaben100 because Temp = 17.6000003814697265625 ℃  Lux = 109 and Alarm = OFF
2018-08-09 06:17:06.193 [INFO ] [ding.velux.bridge.VeluxBridgeExecute] - execute() finished successfully.
2018-08-09 06:31:44.901 [INFO ] [ipse.smarthome.model.script.skylight] - Sending ON command to VeluxAlleAaben100 because Temp = 17.6000003814697265625 ℃  Lux = 160 and Alarm = OFF

It seems like, as soon as the lux is above 100, then it sends ON to VeluxAlleAaben100 which is not what the rule is suppose to. This is probably cause because I changed the criteria for the cases last night before I went to bed, (stupid change).

Anyway… After I have been thinking about once again, and had some sleep. I´ve come up with this idea, which I hope make more sense.

rule "Automatic control of all skylight windows"
when
    Item NetamoUdendoersTemperature changed or
    Item Node13_SensorLuminance changed or
    Item alarm_totalalarm changed or
    Item dummy1 changed
then
    // Exit the rule when there is nothing to do
    if(alarm_totalalarm.state instanceof Switch ) return;
    if(!(Node13_SensorLuminance.state instanceof Number)) return;
    if(!(NetamoUdendoersTemperature.state instanceof Number)) return;

    // Calculate which velux to send the ON command to
    val Number fTemp = NetamoUdendoersTemperature.state as Number
    val Number lux = Node13_SensorLuminance.state as Number
    val alarm = alarm_totalalarm.state
    var velux = VeluxAlleLuk
    
    switch fTemp {
        case fTemp >= 25 && lux >= 100: velux = VeluxAlleAaben100
        case fTemp >= 23 && lux >= 100: velux = VeluxAlleAaben75
        case fTemp >= 21 && lux >= 100: velux = VeluxAlleAaben50
        case fTemp < 21 || lux < 100, && alarm == OFF,
        case alarm == ON: velux = VeluxAlleVent
        case fTemp <= 20 || lux < 100, && alarm == OFF,
        case alarm == ON: velux = VeluxAlleLuk
    }

    // Send the command
	logInfo("skylight", "Sending ON command to " + velux.name + " because Temp = " + NetamoUdendoersTemperature.state + "  Lux = " + Node13_SensorLuminance.state + " and Alarm = " + alarm_totalalarm.state)
    velux.sendCommand(ON)
end

Question is… Will it work as suppose to?

There is a wrong comma :

case fTemp < 21 || lux < 100, && alarm == OFF,
                            ^ here
case fTemp <= 20 || lux < 100, && alarm == OFF,
                             ^ and here

I’m pretty sure the || statement will not work as intended in this case (at least you would have to do some brackets around)
Please keep in mind, that only one case will be executed, even when there are several cases which are true.

Ahh yes, I see.

Why shouldn´t || work, when && do work?
Isn´t || the same as ´or´ ?

Indeed || means or. So let’s take case fTemp < 21 || lux < 100, && alarm == OFF,. The operators are evaluated left to right so if fTemp < 21, the expression will always evaluate to true regardless if whether alarm is OFF or not. If fTemp >= 21 then the expression will only evaluate to true if lux < 100 and the alarm is OFF. You need to add parens to force the fTemp and lux comparisons to occur first.
case (fTemp < 21 || lux < 100) && alarm == OFF,. This way it checks both the fTemp and lux and if either condition is true then it checks the alarm and only if both evaluate to true does the case evaluate to true.

Even with the simplifications this is starting to get really complex and hard to keep in ones head. Try to see if there is a way to consider the three conditions in a different order that may make it takes more sense.

For example, you only care if lux is over it under 100, so perhaps evaluate that separately into a boolean. We can do the same with the alarm.

And perhaps it would help if we split it up a bit. Maybe handle the lux separately.

    val Number fTemp = NetamoUdendoersTemperature.state as Number
    val  lux = Node13_SensorLuminance.state as Number >= 100
    val alarm = alarm_totalalarm.state == ON
    var velux = VeluxAlleLuk
    
    if(lux) {
        switch fTemp {
            case fTemp >= 25: velux = VeluxAlleAaben100
            case fTemp >= 23: velux = VeluxAlleAaben75
            case fTemp >= 21: velux = VeluxAlleAaben50
        }
    }
    else {
        if(fTemp < 20 && !alarm) velux = VeluxAlleVent
        if(alarm) velux = VeluxAlleVent
    }

    // Send the command
	logInfo("skylight", "Sending ON command to " + velux.name + " because Temp = " + NetamoUdendoersTemperature.state + "  Lux = " + Node13_SensorLuminance.state + " and Alarm = " + alarm_totalalarm.state)
    velux.sendCommand(ON)

Ahh I see, it makes sense, cause ofcouse I needed fTemp and lux to evaluate first (or together).

My head was about to explode trying to make the conditions more simple. I simple couldn´t find a way, untill you just showed me.

Correct, rule need to evaluate Lux beeing over or under 100. This is to make sure the windows always will either closed to ventilation at night, OR windows should close totally, all depending on the temperature.

Been away for a few days with a trip to London. But now I´m back an ready to conitnue…

First - The rule still doesn´t work as it´s suppose to. It stil ignores criterias, which I simply cant figure.

This is the latest rule:

rule "Automatic control of all skylight windows"
when
    Item NetamoUdendoersTemperature changed or
    Item Node13_SensorLuminance changed or
    Item alarm_totalalarm changed or
    Item dummy1 changed
then
    // Exit the rule when there is nothing to do
    if(alarm_totalalarm.state instanceof Switch ) return;
    if(!(Node13_SensorLuminance.state instanceof Number)) return;
    if(!(NetamoUdendoersTemperature.state instanceof Number)) return;

    // Calculate which velux to send the ON command to
    val Number fTemp = NetamoUdendoersTemperature.state as Number
    val Number lux = Node13_SensorLuminance.state as Number
    val alarm = alarm_totalalarm.state
    var velux = VeluxAlleLuk
    
    switch fTemp {
        case fTemp >= 25 && lux >= 100: velux = VeluxAlleAaben100
        case fTemp >= 23 && lux >= 100: velux = VeluxAlleAaben75
        case fTemp >= 21 && lux >= 100: velux = VeluxAlleAaben50
        case fTemp < 21 || lux < 100 && alarm == OFF,
        case alarm == ON: velux = VeluxAlleVent
        case (fTemp <= 20 || lux < 100) && alarm == OFF,
        case alarm == ON: velux = VeluxAlleLuk    }

    // Send the command
	logInfo("skylight", "Sending ON command to " + velux.name + " because Temp = " + NetamoUdendoersTemperature.state + "  Lux = " + Node13_SensorLuminance.state + " and Alarm = " + alarm_totalalarm.state)
    velux.sendCommand(ON)
end

When i got up this morning, all windows were 100% open.
This is the log entry:

2018-08-15 08:01:14.723 [INFO ] [ipse.smarthome.model.script.skylight] - Sending ON command to VeluxAlleAaben100 because Temp = 16.6000003814697265625 ?  Lux = 439 and Alarm = OFF

As you can see, the temperature is way below 20 degrees. Lux is fine and alarm is fine. But windows should not be able to open 100% (VeluxAlleAaben100) when temperature is 16 degrees, infact windows should be fully closed (VeluxAlleLuk).

And I cant seem to find the reason :frowning:

That behavior isn’t what you have coded.

Temp = 16, Lux = 439, Alarm = OFF

Now lets go through the switch statement line by line and see what evaluates to true…

The first three cases evaluate to false because fTemp < 21.

The next case evaluates to true. fTemp < 21. It doesn’t matter what lux and alarm are set to because the boolean operations are evaluated left to right, unless you group them using parens.

fTemp < 21 || lux < 100 && alarm == OFF is the same as fTemp < 21 II (lux < 100 && alarm == OFF)

If you change the evaluation to fTemp < 21 && lux < 100 && alarm == OFF then it would evaluate to false because all three cases need to be true for the condition to be true.

If you change the evaluation to (fTemp < 21 || lux < 100) && alarm == OFF then it would evaluate to true because either fTemp < 21 or lux < 100 evaluates to true, then we check to see if alarm is also OFF. Since it is OFF that means the whole condition evaluates to true.

Thus, because fTemp < 21 VeluxAlleVent gets the command.

There is another problem here. You have two case alarm == ON. Only the first one will ever evaluate. Switch statements work in order and execute the first case whose condition evaluates to true. So if alarm==ON VeluxAlleVent will always get the command, never VeluxAlleLuk.

But according to the log, it´s the first case, (VeluxAllAaben100) which is beeing executed. Thats what I cant figure.

But what to do when Alarm is OFF and temperature is below 20? (lux is no more a criteria in case where temperature is below 20).

Well, as I suggested above, it is very hard for experienced programmers even to keep such complicated boolean logic in their mind all at the same time.

Split the boolean logic up to see if you can make it simpler to understand like the example I provided above that separates out the calculation of the lux.

You must explicitly test for this case. And you need to place this test in the proper order compared to the other tests because only the first one that evaluates to true will execute.

This is really really complicated logic. It’s not complicated to write, it’s complicated to understand and figure out. It’s like a test on the final from a college level Discrete Math course.

That is why I recommend splitting it up as I did in the post above.

I know you did, but I have difficulties understanding what exactly you mean by splitting it up. Thats why I took the other version of the rule, and trying to get a grib from it. But I really do wish to understand how things works.

Rather than putting all the conditions in one line with a bunch of || and &&, use separate if statements to separate the cases from each other.

Above I split the cases using lux. If the lux is 100 or over then there is the simple switch statement that chooses the right setting based on the temp.

If the lux is below 100 it chooses the setting based on the temp and the alarm.

The code above is based on the conditions you already wrote which apparently are not correct. But I’m not in a position where I can correct them because you have so many conditions and subconditions and special cases it is all but impossible for my to just say “do this”. Unfortunately this is going to require you to work at understanding both your conditions.

I don’t know how to tell you how to figure out the right conditions but I suspect that you might have contradictory conditions.

Maybe see if you can create a chart that lists all the combinations of the three conditions and maybe a contradiction will become apparent. But consider this, you have 522 (5 temps, 2 lux, and 2 alarm) = 20 separate conditions. It is all but impossible to reason through that many things all at the same time.

But, when you split it up a bit, like the way I split it on lux above, I can remote one of those factors which takes the total from 20 down to two separate cases of 10. That is getting closer to reasonable (5-7 is the rule of thumb for the number of different pieces of data you can reason about mentally).

This is the best I can come up with. I have simplified the temperature values a bit.

windows%20scheme

Maybe the solution is to code this in two different rules. One for Alarm ON, and one for Alarm OFF.

Also I wonder if it would be possible to have a switch turning this automatic controle rules ON/OFF. But this is just a ‘nice to heave feature’ for now.

Too lazy to write it down correct…

if (Alarm.state == OFF && (LUX.state as Number) >= 100) {
    first part of table 
}
else {
if ((fTemp.state as Number) >= 22) VeluxAlleVent
else VeluxAlleLuk
}

:slight_smile:

I don’t think that is necessary. You can split it into three if statements with switch inside. This is what I was meaning by splitting up a bit. Rather than trying to handle Alarm, Lux, and Temp all in one expression.

To fill out Udo’s idea:

rule "Automatic control of all skylight windows"
when
    Item NetamoUdendoersTemperature changed or
    Item Node13_SensorLuminance changed or
    Item alarm_totalalarm changed or
    Item dummy1 changed
then
    // Exit the rule when there is nothing to do
    if(alarm_totalalarm.state instanceof Switch ) return;
    if(!(Node13_SensorLuminance.state instanceof Number)) return;
    if(!(NetamoUdendoersTemperature.state instanceof Number)) return;

    // Calculate which velux to send the ON command to
    val Number fTemp = NetamoUdendoersTemperature.state as Number
    val Number lux = Node13_SensorLuminance.state as Number
    val alarm = alarm_totalalarm.state
    var velux = VeluxAlleLuk

    // Third table
    if(alarm == ON) velux = if(fTemp >= 22) VeluxAlleLuk else VeluxAlleLuk

    // Second table, we already know alarm isn't ON so we don't have to test it for OFF here
    else if(lux < 100) velux = if(fTemp >= 22) VeluxAlleVent else VelusAlleLuk

    // First table, we know that alarm isn't ON and we know lux >= 100 so we don't have to test for it here
    else {
        switch fTemp {
            case fTemp >= 25: velux = VelusAlleAaben100
            case fTemp >= 24: velux = VeluxAlleAaben75
            case fTemp >= 23: velux = VeluxAlleAaben50
            case fTemp >= 22: velux = VeluxAlleVent
            default: velux = VeluxAlleLuk
        }
    }

    // Send the command
	logInfo("skylight", "Sending ON command to " + velux.name + " because Temp = " + NetamoUdendoersTemperature.state + "  Lux = " + Node13_SensorLuminance.state + " and Alarm = " + alarm_totalalarm.state)
    velux.sendCommand(ON)
end

I notice that the behavior is the same if Alarm is ON or if Lux < 100 so we can combine those into one if statement.

if(alarm == ON || lux < 100) velux = if(fTemp >= 22) VeluxAlleVent else VelusAlleLuk

But, just because we can do this doesn’t mean we should. There is a lot to be said for the human readability of keeping them separate.

The velux = if(fTemp >= 22) VeluxAlleVent else VelusAlleLuk line is an example of the trinary operator. It is a short hand way to code the following:

if(fTemp >= 22) velux = VeluxAlleVent
else velux = VeluxAlleLuk

Anything is possible and having an override switch is a good idea. Just create your Switch and add

if(Override.state == ON) return;

to the top of the Rule and the Rule will not do anything while the Override is ON, disabling the Rule.

It makes alot of sense now, I think.
I did however notice, there might be a slight error in the third table:

if(alarm == ON) velux = if(fTemp >= 22) VeluxAlleLuk else VeluxAlleLuk

Shouldn´t it be like this:

if(alarm == ON) velux = if(fTemp >= 22) VeluxAlleVent else VeluxAlleLuk

Agree, it´s gives a better overview, specially for a not knowledged person like me.

Ahh, ofcouse… Stupid me :smiley:

Thank you very much Rich… I´ll give it a try.