Code review for rules; checking state after 30s

Beginner here. I just wanted to make sure I’m following any best practices when creating these rules. Here’s a rule that works, but just want to make sure it’s optimal. The goal is to send a telegram when the back patio door opens, wait 30s, and if the door is still open, send another message.

One other thing I can’t figure out, is why is this door sensor named “zwave_device_fccca473_node11_sensor_door” but the light switch for the main room is “Livingroom_Lights”? How do I give this sensor a “pretty name” in the UI? (I’ve added all Things using paperUI)

    var Timer timer = null

rule "Patio Door Opened"
when
    Item zwave_device_fccca473_node11_sensor_door changed
then
    logInfo("patiodoor", "State changed on patio door: " + zwave_device_fccca473_node11_sensor_door.state)

    if (zwave_device_fccca473_node11_sensor_door.state == OPEN)
    {
        val actions = getActions("telegram","telegram:telegramBot:1bcda474")
        if (actions !== null)
        {
                val result = actions.sendTelegram("Patio door opened")
                logInfo("telegram", "Result of send action is: " + result)
        }

        // Start a timeout; If door state is still open after 30s, send another note
        if (timer === null)
        {
            timer = createTimer(now.plusSeconds(30), [ |
                if (zwave_device_fccca473_node11_sensor_door.state == OPEN)
                {
                    logInfo("patiodoor", "Patio door is still open!")
                    actions.sendTelegram("PATIO DOOR IS STILL OPEN!")
                }
                timer = null
            ])
        }
    }
end

What would you like to happen if the door is closed and then opened again in the interim? At the moment that is just ignored, timing runs from the first opening.

A fairly common alternative approach is to start the time on opening, and cancel it on closing. If the timer ever gets to the end, then it’s run the time since the most recent opening with no closings happening, no further check is needed.

Ah, I was thinking that might be an issue. How about this?

var Timer timer = null

rule "Patio Door Opened"
when
    Item zwave_device_fccca473_node11_sensor_door changed
then
	var curState = zwave_device_fccca473_node11_sensor_door.state
    logInfo("patiodoor", "State changed on patio door: " + curState)

    if (curState == OPEN)
    {
        val actions = getActions("telegram","telegram:telegramBot:1bcda474")
        if (actions !== null)
        {
                val result = actions.sendTelegram("Patio door opened")
                logInfo("telegram", "Result of send action is: " + result)
        }

		timer = createTimer(now.plusSeconds(30), [ |
			// Check state after 30s
			if (curState == OPEN)
			{
				logInfo("patiodoor", "Patio door is still open!")
				actions.sendTelegram("PATIO DOOR IS STILL OPEN!")
			}
			timer = null
		])
	}
	else if (curState == CLOSED)
	{
		// Door has closed; Cancel timer
		if (timer !== null)
		{
			timer.cancel()
			timer = null
		}
    }
	else
	{
		logInfo("patiodoor", "Patio door is unknown state: " + curState)
	}
end

The easy thing first. Livingroom_Lights is an Item you created manually, either through PaperUI or .items files. zwave_device_fccca473_node11_sensor_door is an Item that was automatically created because Simple Mode is either enabled or was enabled at some point in the past. You’ll have to disable Simple Mode, delete that Item and recreate it and link it to the right Channel on the Zwave Thing. You cannot change the name of Simple Mode created Items as far as I am aware.

Some ideas, some minor some major:

  • The rule is triggered with changed so there is a newState and previousState pair of implicit variables. You can change all the zwave_device_fccca473_node11_sensor_door.state with newState.

  • The rule only cares about the door opening, so trigger only when it changes to OPEN, changed to OPEN and you don’t need to check if the door is OPEN in the rule at all.

  • As rossko57 points out, what if the door is closed and then opened again within the 30 seconds of the timer? Let’s consider the following:

0 secs: door opens
0 secs: telegram message “Patio door opened”
20 secs: door closes
25 secs: door opens again
25 secs: telegram message “Patio door opened”
30 seconds: telegram message “Patio door is still open!”
55 secs: telegram message “Patio door is still open!”

Obviously that isn’t what is intended. What you would want is

0 secs: door opens
0 secs: telegram message “Patio door opened”
20 secs: door closes
25 secs: door opens again
25 secs: telegram message “Patio door opened”
55 secs: telegram message “Patio door is still open!”

To achieve this behavior, you need to cancel the old timer when the door closes. Make sure to set the Timer to null after cancelling it. See Design Pattern: Motion Sensor Timer for examples.

  • Since the previous bullet makes you need to care about CLOSED you have two options. Create another Rule that triggers on changed to CLOSED or keep the if statement and add an else to the one existing Rule.

  • I’ve become pretty big on meaningful error messages lately. Add an else to the check to make sure actions !== null and log an error. As written it will fail silently at first and then with an arcane error later when the Timer runs.

  • Let’s assume that you don’t want to cancel the Timer when the door opens and closes and opens again within the 30 seconds. In that case, add an else to the timer === null case to reschedule the timer. That would be an equally viable approach to the cancel approach.

  • In your new version of the code, the cancelling of the timer can be reduced to

timer?.cancel
timer = null
1 Like

Gotcha. Yes, I turned on Simple mode at some point because I was surprised that openHab did not automatically create UI controls after discovering a device. None of my devices/items are in any .items files. I was trying to do everything via UI, but that doesn’t seem possible. Sorta makes the barrier to entry with openHAB a lot higher for the normal user.

Back to the code, newState did not work so I kept it as getState()

var curState = zwave_device_fccca473_node11_sensor_door.newState
Rule 'Patio Door Opened': 'newState' is not a member of 'org.eclipse.smarthome.core.library.items.ContactItem'

Finalized code, for now:

var Timer timer = null

rule "Patio Door Opened"
when
    Item zwave_device_fccca473_node11_sensor_door changed
then
	var curState = zwave_device_fccca473_node11_sensor_door.getState()
    logInfo("patiodoor", "State changed on patio door: " + curState)

    if (curState == OPEN)
    {
        val actions = getActions("telegram","telegram:telegramBot:1bcda474")
        if (actions !== null)
        {
			val result = actions.sendTelegram("Patio door opened")
			logInfo("telegram", "Result of send action is: " + result)

			// Check state after 30s
			timer = createTimer(now.plusSeconds(30), [ |
				if (curState == OPEN)
				{
					logInfo("patiodoor", "Patio door is still open!")
					actions.sendTelegram("PATIO DOOR IS STILL OPEN!")

					// Keep checking and alerting
					timer.reschedule(now.plusSeconds(30))

					// TODO: Implement manual override via Telegram reply
				}
			])
        }
		else
		{
			logWarning("telegram", "Cannot get action of Telegram")
			// TODO: Implement backup destination of notification
		}
	}
	else if (curState == CLOSED)
	{
		// Door has closed; Cancel timer
		timer?.cancel()
		timer = null
    }
	else
	{
		logInfo("patiodoor", "Patio door is unknown state: " + curState)
	}
end

Just tested the above code. Works great. Sends TG on open, sends repeated still open messages every 30s until closed. Nice! I saw some example code for Telegram where you can send button maps and handle replies; that’ll be TODO because when it gets cooler outside, we will leave the door open more often. :slight_smile: Thanks for the assistance!

Not

var curState = zwave_device_fccca473_node11_sensor_door.newState

just

var curState = newState

You don’t need to check for still OPEN in the timer code, because if it had changed to CLOSED the timer would have already been cancelled.