Dates in rules - have I got this right

I wanted to have a play and thought of using the VSCode.

So I loaded a rules file into it and saw a bunch of warnings. One about getLocalMills not visible.

I found this thread: The method getLocalMillis() is not visible, error
which pointed me at: Design Pattern: Time Of Day

So for my code, which is check the current time is > timeon (which is a 5, so 5:00) and < timeoff (which is 19, so 19:00)

if( ((new LocalTime().getLocalMillis()) > (new LocalTime(timeon, 0, 0, 0).getLocalMillis()) ) && 
                    (new LocalTime().getLocalMillis()) < (new LocalTime(timeoff, 0, 0, 0).getLocalMillis()) )  

Would this now become:

if( now.isAfter(now.withTimeAtStartOfDay.plusHours(timeon))  &&
                   now.isBefore(now.withTimeAtStartOfDay.plusHours(timeoff)) )

I’m not sure I got the daylight saving reference. But to work that out, would I have to add a day, then remove 19 hours for 5am and remove 5 hours for 19:00?

You are comparing the current hour of the day to integer variables?

if ((new LocalTime).getHourOfDay > timeon && (new LocalTime).getHourOfDay < timeoff) {

or…

if (now.getHourOfDay > timeon && now.getHourOfDay < timeoff) {

or…

if (now > now.withTime(timeon,0,0,0) && now < now.withTime(timeoff,0,0,0)) {

or (my favorite)…

if (new Interval(now.withTime(timeon,0,0,0),now.withTime(timeoff,0,0,0)).contains(now)) {

If you’re comparing to an item’s state, you’ll need some slight modifications…

if (new Interval(now.withTime((timeon.state as Number).intValue,0,0,0),now.withTime((timeoff.state as Number).intValue,0,0,0)).contains(now)) {

All of these handle daylight savings too.

I like the middle one.

if (now.getHourOfDay > timeon && now.getHourOfDay < timeoff)

So much shorter.

1 Like

Two things to be aware of when comparing time ranges, one of which you already mentioned.

  1. Comparing a time range that spans midnight. The comparisons above will fail if timeon is before midnight and timeoff is after midnight. You would need to convert the comparisons into two clauses:
    if((now.getHourOfDay > timeon && now.getHourOfDay < now.withTimeAtStartOfDay.plusDays(1)) ||
       (now.getHourOfDay > now.withTimeAtStartOfDay && now.getHourOfDay < timeoff))
  1. Daylight Savings. In this case what you suggest is right.
    if(timeon > now.plusDays(1).withTimeAtStartOfDay.minusHours(19))

Jump to tomorrow and subtract hours to avoid the hour change problem.

Ok.but that’s if checking now against something passed midnight.

Where I tend to just check 0500 to 0800, or 0500 to 1900.

My next trick is to see if I can break up my central heating rule. As it’s huge.

The main bit I have problems with is dealing with the sequencing of different events if you broke up the rule into smaller rules.

I tried reading the design patterns but couldn’t see a way through it.

Without seeing the rule I can’t offer much beyond Design Pattern: How to Structure a Rule and Design Pattern: Working with Groups in Rules.

Wasn’t sure if I should post a new thread or not.

Here goes, then I’ll break some of it up for explanation.

var String PreviousMode = "Off"

rule "Morning Central Heating Rules"
when
        Item g_temperature_average received update  or
                Item check_temperature_min changed or
                Item temperature_average changed or
                Item heating_powermode changed or
                Time cron "0 */5 * * * ?"
then
    logInfo("MorningCentralHeating","Command Called")
        var boolean heaterState = true;
        var boolean withinTimer = false;
        logInfo("MorningCentralHeating",String::format("Checking start/end time against current.  Start: %s End: %s and current %s",heating_morning_time_on.state,heating_morning_time_off.state,new LocalTime()))
        var int timeon = (heating_morning_time_on.state as DecimalType).intValue
        var int timeoff = (heating_morning_time_off.state as DecimalType).intValue
        logInfo("MorningCentralHeating",String::format("(0)  Getting average from manually calulated average as default"))
        var float avg = (temperature_average.state      as DecimalType).floatValue
        logInfo("MorningCentralHeating",String::format("(0)   Calculated average: %s",avg))

        var String ruleSection = "temp"

        if (g_temperature_average.state === NULL)
                logError("MorningCentralHeating",String::format("Couldn't get a reading from g_temperature_average: %s",g_temperature_average))
        else
        {
                logInfo("MorningCentralHeating",String::format("(0) Getting average group"))
                avg = (g_temperature_average.state      as DecimalType).floatValue
                logInfo("MorningCentralHeating",String::format("(0)   Calculated average from group: %s",avg))

        } //if (g_temperature_average.state === NULL)


        logInfo("MorningCentralHeating",String::format("(0) Checking start/end time against current.  Start: %s End: %s and current %s temp: %s",timeon,timeoff,new LocalTime(),avg))
        if (heating_powermode.state == "Schedule" )
        {
                logInfo("MorningCentralHeating",String::format("(1.1) Got avg: %s, Getting max",avg))
                var float max = (heating_morning_maximum_setpoint.state as DecimalType).floatValue
                logInfo("MorningCentralHeating",String::format("(1.2) Got max: %s, Getting offset",max))
                var float offset = (heating_morning_maximum_offset.state as DecimalType).floatValue
                logInfo("MorningCentralHeating",String::format("(1.3) Got offset: %s",offset))

                //CheckHeatingSettings.apply(  avg , max)
                //,(heating_morning_maximum_offset.state as DecimalType).floatValue )
                        //,(heating_morning_time_on.state as Number).intValue
                        //(heating_morning_time_off.state as Number).intValue
                //)
                //logError("MorningCentralHeating",String::format("Completed lambda"))

                logInfo("MorningCentralHeating","(1.4) Looking at morning Time on/time off")
                //if((new LocalTime().getLocalMillis()) > (new LocalTime(timeon, 0, 0, 0).getLocalMillis()) &&
                //      (new LocalTime().getLocalMillis()) < (new LocalTime(timeoff, 0, 0, 0).getLocalMillis()))
                if (now.getHourOfDay >= timeon && now.getHourOfDay <= timeoff)
                {
                        withinTimer = true
                        logInfo("MorningCentralHeating","(1.5) Looking at temps")
                        logWarn("MorningCentralHeating",String::format("(1.6) Within Start and stop time, but checking current temp (%s) below maximum (%s) including offset of (%s) = (%s)",
                                avg,max,offset,max-offset))
                        if (avg >= max)
                        {
                                logWarn("MorningCentralHeating",String::format("(1.7) Temperature (%s) above maximum (%s) , time to turn off heating - SET HEATER STATE OFF",avg,max))
                                heaterState = false
                                ruleSection = "Time = off"
                        } //if (g_temperature_average.state >= heating_minimum_setpoint.state )
                        else if (avg  < (max-offset))
                        {
                                logWarn("MorningCentralHeating",String::format("(1.8) Temperature (%s) is %s below maximum (%s) , time to turn on heating - SET HEATER STATE ON.",avg,
                                        offset,max))
                                heaterState = true
                                ruleSection = "Time = On"
                        } //if (g_temperature_average.state >= heating_
                        else if (zwave_switch_central_heating.state == OFF)
                        {
                                //heaterState = true
                                logError("MorningCentralHeating",String::format("(1.9) Temperature (%s) should be below max (%s) , but above max - offset (%s) So stay off - SET HEATER STATE OFF",avg,max,max-offset))
                                heaterState = false
                                ruleSection = "Time = off"
                        } //else if (g_temperature_average.state >= heating_minimum_setpoint.state )
                        else
                                logInfo("MorningCentralHeating",String::format("(1.10) heater already on"))
                }  //after morning time on and off
                else
                {
                        logInfo("MorningCentralHeating",String::format("(1.11) Outsides time period to run heater. Start: %s End: %s and current %s - SET HEATER STATE OFF this can be overridden",timeon,timeoff,new LocalTime()))
                        heaterState = false;
                        ruleSection = "Time = off"
                } //  //after morning time on and off


                logInfo("MorningCentralHeating",String::format("(2.1) Getting minimum temp threshold (%s)" , heating_overnight_minimum_override.state))
                var float min = (heating_overnight_minimum_override.state as DecimalType).floatValue
                logInfo("MorningCentralHeating",String::format("(2.2) Checking Average Temperature (%s) for cold override (%s) with offset (%s) = %s" , avg,min,offset,min-offset))
                if (avg < min-offset && !withinTimer)
                {
                        logWarn("MorningCentralHeating",String::format("(2.3) Current action:(%s Timer: %s), Current average (%s) is below minimum %s and offset %s = %s - SET HEATER STATE ON" ,heaterState,withinTimer,avg,min,offset,min-offset))
                        heaterState = true
                        ruleSection = "Min = on"
                        //Notify_Info.postUpdate(String::format("Turn on heating as current temperature (%s) below minimum override temp: %s", avg,heating_minimum_override.state))
                } else if (avg > min && !withinTimer)
                {
                        logWarn("MorningCentralHeating",String::format("(2.4) Current action:(%s Timer: %s), Current average (%s) is above minimum of %s  - SET HEATER STATE OFF" ,heaterState,withinTimer,avg,min))
                        heaterState = false;
                        ruleSection = "Min = off"
                }
                //2018-05-28 - Removed this between section as it caused system to cycle on and off
                //else if ((avg > (min-offset) && avg < min) && !withinTimer && zwave_switch_central_heating.state == ON)
                //{
                //      logWarn("MorningCentralHeating",String::format("(2.5) Current action:(%s Timer: %s), Current average (%s) is between %s and %s  - LEAVING HEATER STATE ON " ,heaterState,withinTimer,avg,min-offset,min))
                //      heaterState = true;
                //      ruleSection = "Min = on"
                //}
                else if (zwave_switch_central_heating.state == ON)
                {
                        heaterState = true;
                        ruleSection = "Min = stay on"
                        logInfo("MorningCentralHeating",String::format("(2.6) Current action:(%s) - within timer %s, Current average (%s) is above minimum of %s  - LEAVING HEATER ON" ,heaterState,withinTimer,avg,min-offset))
                }

                logInfo("MorningCentralHeating",String::format("(3) Checking if difference between weather and inside if > 10 Degrees: %s" , heating_diff_weather_internal.state))
                if (heating_diff_weather_internal.state >=10)
                {
                        logWarn("MorningCentralHeating",String::format("(3.1) Current Temperature inside (%s) is >= 10 Degrees difference to the weather (%s) = %s." ,avg,weather_temperature.state,heating_diff_weather_internal.state))
                        //heaterState = true
                        Notify_Warn.postUpdate(String::format("Maybe look at turning on the heather as the difference to outside > 10°C"))
                }

                logInfo("MorningCentralHeating",String::format("(4.0) Checking current heater state (%s) and performing action (%s)",zwave_switch_central_heating.state ,heaterState))
                if  (zwave_switch_central_heating.state == ON && heaterState == false)
                {
                        logWarn("MorningCentralHeating",String::format("(4.1) Turning off heater (%s)",ruleSection))
                        Notify_Info.postUpdate(String::format("OH - Turn off (%s) heating as between %s and %s Current Temp (%s) above maximum (%s)" ,ruleSection,timeon,timeoff,avg,max))
                        //replace with pretend_central_heating
                        zwave_switch_central_heating.sendCommand(OFF)

                }
                else if (zwave_switch_central_heating.state == OFF && heaterState == true)
                {
                        logWarn("MorningCentralHeating",String::format("(4.2) Turning on heater (%s)",ruleSection))
                        Notify_Info.postUpdate(String::format("OH - Turn on (%s) heating as between %s and %s Current Temp (%s) below maximum (%s)" ,ruleSection, timeon,timeoff,avg,max))
                        zwave_switch_central_heating.sendCommand(ON)
                }
                else
                        logWarn("MorningCentralHeating","(4.3) Leaving Heater alone: " + zwave_switch_central_heating.state)


        } //if (g_temperature_average.state !== NULL)
        else
                logWarn("MorningCentralHeating","(0) Leaving Heater alone as average temperature is NULL or powerMode != schedule: " + heating_powermode.state)

        logInfo("MorningCentralHeating","End of rule: heating state: " + zwave_switch_central_heating.state)

end

So g_temperature_average is the main driver in terms on should I turn on or not.
heating_morning_maximum_setpoint is the max temp at which if g_temperature_average is above, don’t turn on.
offset is the lower threshold under the average temperature that g_temperature_average must drop to, to start the system.
timeon and timeoff are integers, set to 5 and 8 respectively.
heating_powermode is string item, where it can be On, Off, Schedule, Boost. Schedule is what is used to make the timeon and timeoff active. On is a manual on, off is a manual off. Not shown in this rule, is another endpoint at 2200 that sets the system back to schedule.

The theory is if the temp is between heating_morning_maximum_setpoint and heating_morning_maximum_setpoint -offset and the time is between timeon and timeoff - then turn on the heating system. Otherwise don’t.

You can see it’s quite large, and while I could break it up to the:
am I in between timeon timeoff
is my temp lower than heating_morning_maximum_setpoint -offset
I’m not really sure if that what I should be doing. I’m just conscious this is a large block of code I have to be quite careful of.

Your the OP. If the updates bother Scott he can unfollow the thread. :slight_smile:

OK, so as I suggested, I’d move the time based stuff completely out of the Rule using the Time of Day Design Pattern. That would eliminate both the cron and a good number of lines of code would be moved to another Rule where the calculations could be reused.

Presumably you also have an Evening and a Night central heating rules so Iet’s build a Time of Day Rule with those three states.

rule "Time of Day"
when
    System started or
    Time cron "0 0 8 * * ? *" or
    Time cron "0 0 16 * * ? *" or
    Time cron "0 0 22 * * ? *"
then 
    // get the DateTime for the start of the time periods
    val morningStart = now.plusDays(1).minusHours(24-8)
    val eveningStart = now.plusDays(1).minusHours(24-16)
    val nightStart = now.plusDays(1).minusHours(24-22)

    var newToD = "UNKNOWN"
    switch now {
        case now.isAfter(morningStart): newToD = "MORNING"
        case now.isAfter(eveningStart): newToD = "EVENING"
        case now.isAfter(nightStart): newToD = "NIGHT"
    }

    if(TimeOfDay.state.toString != newToD) TimeOfDay.postUpdate(newToD)
end

Now we have the time of day represented in an Item and we can drive our Rules off of changes to this Item and know what time it is by a simple check on this Item. Adjust as necessary based on your actual times. If you need to change the starts of each of these time periods using an Item it’s simple enough.

val morningStart = now.plusDays(1).minusHours(24-heating_morning_time_on.state)

Replace the cron check in your Rule with Item TimeOfDay changed to MORNING or Item TimeOfDay changed if you need to run this same Rule based on other times of day.

Your log statements are needlessly complex. You only ever use %s so why not make the code easier to read by using string concatenation?

logInfo("MorningCentralHeating", "Checking start/end time against current. Start: " + heating_morning_time_on.state + " and current " + now.toString)

Avoid converting Numbers to primitives unless you need to. It can cause significant delays in loading of the .rules files (reported by some), can cause casting problems, and are almost never needed. Better to keep them as Numbers.

OK, now let’s apply the How to Structure a Rule design pattern. First we see if we need to run the rule at all and return if not. Next we calculate what needs to be done. Finally we do it.

With these suggestions, let’s see where that gets us.

rule "Morning Central Heating Rules"
when
    Item g_temperature_average changed or // we don't care about updates that don't change anything
    Item check_temperature_min changed or
    Item temperature_average changed or
    Item heating_powermode changed or
    Item TimeOfDay changed to MORNING
then
    // 1. Exit when Rule doesn't need to run

    // 2. Calculate the new heater state
    // we will assume the heater needs to be OFF by default. We mainly need to test to see if we need
    // to turn the heater ON so we can usually ignore cases where the heater needs to be OFF and not
    // explicitly test for it, simplifying the code
    var newHeaterState = OFF

    // get the average temp
    // I'm not sure why there is an average Item and an average Group but this is what the original Rule does
    var avg = temperature_average.state as Number
    if(g_temperature_average.state !== NULL) avg = g_temperature_average.state as Number
    logInfo("MorningCentralHeating", "Using average temp " + avg)

    // get the max, offset, and min, let's put like code together
    val max = heating_morning_maximum_setpoint.state as Number
    val offset = heating_morning_maximum_offset.state as Number
    val min = heating_overnight_minimum_override.state as Number

    // Use max as the threshold if it is MORNING and Schedule mode, use min if not
    val threshold = if(TimeOfDay.state != "MORNING" && heating_powermode.state == "Schedule") max else min

    // calculate the new heater state
    switch avg {
        case avg >= threshold:           newHeaterState = OFF
        case avg < (threshold - offset): newHeaterState = ON
        default: zwave_switch_central_heating.state // keep the same state if in hysteresis range
    }

    // Warn if difference between inside and outside is >= 10 degrees
    if(heating_diff_weather_internal.state >= 10) { 
        logWarn("MorningCentralHeating", "Current temperature inside " + avg + " is >= 10 Degrees difference to the weather " + weather_temperature.state + " = " + heating_diff_weather_internal.state)
        Notify_Warn.postUpdate(String::format("Maybe look at turning on the heather as the difference to outside > 10°C"))
    }

    // 3. Change the heater state if necessary
    if(zwave_switch_central_heating.state != newHeaterState) {
        logWarn("MorningCentralHeating", "Turning " + newHeaterState)
        if(newHeaterState == ON) Notify_Info.postUpdate("OH - Turn on heating as " + TimeOfDay.state + " Current Temp " + avg + " below maximum " + max)
        zwave_switch_central_heating.sendCommand(newHeaterState)
    }
    else {
        logWarn("MorningCentralHeating", "Keeping the furnace in the current state")
    }
end

If you get rid of the comments we have reduced it down to 61 lines of code. I have fewer log statements largely because I think you can get the same information with far fewer lines of logging. This compares to the original 128 (not counting comments). 61 lines split across 2 Rules is a pretty reasonable size. And the TimeOfDay state can be used in other Rules.

Note, I just typed this in. It probably has typos.

Hey thanks. I’ll digest that, and implement against a “fake” switch to see how it runs. I work much better by examples that concepts, so having this broken apart according to your design patterns is really beneficial. THanks for spending the time to do it.

Regarding some of the comments.

Probably. But I thought there were benefits for %s based type string replacements. And given this was such a complex rule, I tended to need a lot of logging ot check what it was doing (now that it’s generally stable, it could have been reduced.

I never really got the link between openhab types and primitive java types…one of the issues I see with having a pretty complex rules engine, which is both a blessing and a curse.

Because for a while I was having issues with the group g_temperature_average. So ended up calculating it by hand (also in restarts, or item updated, one of both of the values can be null, so the group is undefined…so it’s a good back stop.

Can you see a point where the experimental rules engine would be able to create a rule such as this, or is it designed for much simpler tasks?

This is a good practice. Remember you can just comment out (some of) your logging so that it is easy to reinstate when you come back to tweak something later!

There is advantage when you are using %d or %.1f or the like to format the values of numbers or DateTimes. But if you are just using Strings it just makes the logs statements harder to read. I suppose there is a good argument to be consistent, but for this rule it only ever uses %s so why not be consistent with using the easier to read String concatenation?

This Rule is not complex. It was long but it isn’t complex. The Experimental Rules Engine could implement this right now. The big problem with the Experimental Rules Engine is that there is no support for Timers and other Actions yet. I’m also not sure whether/how it supports working with members of Groups.

But this Rule just has simple if/else/switch statements and simple math using the states of Item. Except for the logging this Rule could be implemented in the Experimental Rules right now. Though the syntax would be significantly different since it uses JavaScript instead of the custom Rules DSL.

There is a balance though. At some point all the logging statements and commented out code starts to make the code harder to read. Then it becomes a problem, not a good thing.

As a case in point, there were a number of branches in the original code whose only reason for existing was to log out something. There was a variable whose only purpose was to log out something. That’s a lot of added complexity to leave in the code (after the problem that caused the addition of the logging statements is solved).

Were I to have included all the same logging statements in my rewrite it would nearly double the length of the Rule. And the benefit is dubious since all the same information is available in far fewer lines of logging.

As with all things, a balance needs to be found because TANSTAAFL.