Code Cleanup HELP! ;)

Hey All,

This code works perfectly but i would like to clean it up without breaking it like i have for the last two hours lol… Can anyone help? :slight_smile:

1: Disarmed 2: Arm Away 3: Arm Stay

If I could do something like:

if (AlarmSysStatus.state == 2 && gMotionSensors.state == OPEN || gDoorSensors.state == OPEN)

It would be a good start. Basically if the alarm is armed in away AND a door OR motion goes off it sets off alarm… The above don’t work like that though.

// Alarm System Active Action
rule "rAlarmActivated"
when
 Item gMotionSensors changed from CLOSED to OPEN or
    Item gDoorSensors changed from CLOSED to OPEN
then
 {
    if (AlarmSysStatus.state == 2 && gMotionSensors.state == OPEN)
        {
        logInfo("Security", "SECURITY: ARMED-AWAY Motion Intrusion Detected!")
        gAllLights.sendCommand(ON)
        lWf.sendCommand(ON)
        sendMail("alerts@cidcomm.com", "SECURITY: ARMED-AWAY Motion Intrusion!", "Home Intrusion Detected!")
        Thread::sleep(3000)
        gAllSirens.sendCommand(ON)
        Thread::sleep(1200000)
        gAllSirens.sendCommand(OFF)
        }
    if (AlarmSysStatus.state == 2 && gDoorSensors.state == OPEN)
        {
        logInfo("Security", "SECURITY: ARMED-AWAY Door Intrusion Detected!")
        gAllLights.sendCommand(ON)
        lWf.sendCommand(ON)
        sendMail("alerts@cidcomm.com", "SECURITY: ARMED-AWAY Door Intrusion!", "Home Intrusion Detected!")
        Thread::sleep(3000)
        gAllSirens.sendCommand(ON)
        Thread::sleep(1200000)
        gAllSirens.sendCommand(OFF)
        }
    if (AlarmSysStatus.state == 3 && gDoorSensors.state == OPEN)
        {
        logInfo("Security", "SECURITY: ARMED-STAY Door Intrusion Detected!")
        gAllLights.sendCommand(ON)
        lWf.sendCommand(ON)
        sendMail("alerts@cidcomm.com", "SECURITY: ARMED-STAY Door Intrusion!", "Home Intrusion Detected!")
        Thread::sleep(3000)
        gAllSirens.sendCommand(ON)
        Thread::sleep(1200000)
        gAllSirens.sendCommand(OFF)
        }
}}}}
end

1 Like

Try

if(AlarmSysStatus.state == 2 && (gMotionSensors.state == OPEN || gDoorSensors.state == OPEN))

Boolean operators are evaluated left to right (think back to your arithmetic order of operations lessons). By putting the || clause in parens you force the || to be evaluated first before the && which will give you what you want.

To fully put everything on one if:

if((AlarmSysStatus.state == 2 && (gMotionSensors.state == OPEN || gDoorSensors.state == OPEN)) ||
   (AlarmSysStatus.state == 3 && (gDoorSensors.state == OPEN))

You could calculate a boolean:

    var alarm = false
    if(AlarmSysStatus.state == 2 && gMotionSensors.state == OPEN) alarm = true
    else if(AlarmSysStatus.state == 2 && gDoorSensors.state == OPEN) alarm = true
    else if(AlarmSysStatus.state == 3 && gDoorSensors.state == OPEN) alarm = true

    if(alarm) {
        ...
    }

You could put the alarm code into a lambda and call the lambda inside your if statements.

You could create an Alarm Item and send a command to Alarm (I’d probably use a String and the message would be the new State) and put the alarm code into a separate rule.

You could create gAlarmSysStatus2 and gAlarmSysStatus3 Groups, put the appropriate items in each group:

if((AlarmSysStatus.state == 2 && gAlarmSysStatus2.state == OPEN) ||
   (AlarmSysStatus.state == 3 && gAlarmSysStatus3.state == OPEN))

Personally I would probably go with using the Alarm Item as there might be additional cases where I might want to trigger the alarm and this method supports having those cases in separate .items files than the others.

3 Likes

Rich,

You are the man. That’s all i gotta say! :slight_smile:

Ok Rich,

Here’s one for ya. Let’s see how good ya are! :wink: haha

So here is my code - My Config Collection - Might help Someone

Basically when any of the motion or door sensors sets off the alarm in stay or away a generic email gets sent out to me. Is there a way to have for example:

Current Email:
sendMail("alerts@cidcomm.com", “SECURITY: ARMED-STAY Door Intrusion!”, “Home Intrusion Detected!”)

What I would like to see:
sendMail("alerts@cidcomm.com", “SECURITY: Front Door Intrusion!”, “Home Intrusion Detected! (Front Door)”)
or
sendMail("alerts@cidcomm.com", “SECURITY: Patio Door Intrusion!”, “Home Intrusion Detected! (Patio Door)”)

Have the item name that set off the alert in the email kind thing…

First a comment.

The way your rule currently triggers you will only get an alert for the first Item in gMotionSensors or gDoorSensors to go from CLOSED to OPEN. Each subsequent motion sensor that goes off or door that opens will not trigger the alert until they all go back CLOSED.

So here is how I do it. I’m going to describe the how and then post an example I use in my setup which isn’t exactly what you are asking for but illustrates what you are after. Making it work in your setup is an exercise for the student. :wink:

  1. Name your Items so it is easy to parse out the pieces of information you want. I use underscores and in my code do Item.name.split("_") to get the various parts of the name which I can then use to construct the messages.

  2. Use the following trick to detect which Item triggered the rule to be triggered. This assumes you have persistence set up.

    Thread::sleep(250) // give persistence a chance to catch up, experiment with the best value for your setup
    val latest = gMyGroup.members.sortBy[lastUpdate].last

This is a little complicated in your case because you have two groups to check. You need to get the latest from both groups and determine which one is the most recent.

  1. If your Item is named something like “Front_Door_Reed”:

    val nameParts = latest.name.split("_")
    val msg = "SECURITY: " + nameParts.get(0) + " " + nameParts.get(1) + " Intrusion!“
    sendMail("alerts@cidcomm.com”, msg)

Come up with a consistent naming scheme (e.g. in above I use Location_Thing_Type where Front=location, Door=Thing, and Reed=Type). The Type is mainly there so you can distinguish between for example a reed switch on the front door from a vibration sensor on the same front door.

In my example below I use a T_O_Device_Location/Instance_Thing where T is the Item type (e.g. N means sensor) and O is the Object type (e.g. V means value)

Example:

#Items:

Group:Number:MIN gBattery         "Minimum Battery Level [%d]" <temperature>
Number N_V_COSmoke_Main_Battery   "Main Floor Smoke/CO Alarm Battery [%d]" <temperature> (gCOSmoke_Battery, gBattery) {zwave="5:command=battery"}
Number N_V_COSmoke_Top_Battery    "Top Floor Smoke/CO Alarm Battry [%d]"   <temperature> (gCOSmoke_Battery, gBattery) {zwave="9:command=battery"}
Number N_V_Minimote_1_Battery   "Minimote 1 Battery [%d]" <temperature> (gMinimote_Battery, gBattery) {zwave="15:command=battery"}
Number N_V_Minimote_2_Battery   "Minimote 2 Battery [%d]" <temperature> (gMinimote_Battery, gBattery) {zwave="20:command=battery"}

#Rules:

rule "Process Battery Level Updates"
when
        Item gBattery received update
then
    gBattery.members.forEach[ battery |
        if((battery.state as DecimalType) < 100) {
            val name = battery.name.split("_")
            switch(name.get(2)){
                case "COSmoke":  Notification_Proxy_Info.postUpdate("Replace the " + name.get(3) + " Floor " + "CO/Alarm battery, " + battery.state.toString + "% left")
                case "Minimote": Notification_Proxy_Info.postUpdate("Charge Minimote " + name.get(3) + ", " + battery.state.toString + "% left")
             }
         }
    ]
end

An example message copied from my most recent alert:

Replace the Top Floor CO/Alarm battery, 89% left

Notes:

  • In the above rather than wanting to know which Item triggered the rule I want to know all of the devices that need their batteries attended to so I don’t use the trick of pulling the latest changed Item out of the group.
  • This rule will trigger multiple times per change to a single Item. I have a check in my rule that processes the Notification_Proxy_Info events to prevent multiple copies of the same message being sent.
  • This is not the only way to accomplish this. It is my preferred approach though.
  • I highly recommend adopting the Notification_Proxy approach because then you can centralize all of your notifications into one set of rules and if you want to change the behavior of your notifications you have only one place to do it. For example, I have mine setup to send Alarms via my.openhab during the day which goes to both me and my wife’s phone and at night they only get sent to me via PushBullet. Info notifications only go only to me via PushBullet. If I had scattered my notifications throughout my rules implementing that would be a nightmare.
  • I’m currently testing so all battery updates (which occur about once a day for zwave battery devices) generate an alert. This is why I have < 100 in my if statement.
1 Like

Thanks Rich!

I’ll go through this with a fine tooth comb :wink: Also I am aware on the first comment. I only really need the initial something to go off to respond. I will be able to fix that up though I’m sure once I go through this!!

Nice post on the ZCOMBO as well… That helped me a lot!.. thanks!!

So this is what i’m trying, hows it looking? Also for case “Battery”: portion, are the double quotes basically wildcarding any “Battery” item names in items?

rule "Dispatch Notification"
when
        Item Notification_Proxy received update
then
        logWarn("Notification", Notification_Proxy.state.toString)

       // if(Night.state == OFF)
        sendNotification("alerts@cidcomm.com", Notification_Proxy.state.toString)
       // else sendPushToDefaultDevice(Notification_Proxy.state.toString)
end





rule "Process Battery Level Updates"
when
        Item gBattery received update
then
    gBattery.members.forEach[ battery |
        if((battery.state as DecimalType) < 100) {
            val name = battery.name.split("_")
            switch(name.get(2)){
                case "Battery":  Notification_Proxy_Info.postUpdate("Replace the " + name.get(3) + " Floor " + "CO/Alarm battery, " + battery.state.toString + "% left")
               // case "Minimote": Notification_Proxy_Info.postUpdate("Charge Minimote " + name.get(3) + ", " + battery.state.toString + "% left")
             }
         }
    ]
end

I get it - val nameParts = latest.name.split("_")

Being that I don’t have the underscores yet till i get time to go through and redo the item names is their another way to handle it if the item name is for example cFrontDoorBattery or would i have to do individual case “cFrontDoorBattery”: for each battery device?

Ok,

got it working. My only issue now is “sendNotification("alerts@cidcomm.com”, Notification_Proxy.state.toString)" isn’t working. I do not use my.openhab which i think this is needed for? If so can i do something like “sendMail("alerts@cidcomm.com”, Notification_Proxy.state.toString)" instead?

ok, figured that out also… Grasshopper is learning lol

sendMail("alerts@cidcomm.com", Notification_Proxy.state.toString, Notification_Proxy.state.toString)

Run it and see but so far it looks good.

A switch statement is an alternate way to do a bunch of if/else statements. So

switch(name.get(2)) {
    case "Battery": Notification...
    case "Minimote": Notification...
}

is the exact same functionally as

if(name.get(2) == "Battery") Notification...
else if(name.get(2) == "Minimote") Notification...

In short, it is looking for an exact match.

You would have to do it individually for each device.

Correct, that is how you send a notification through my.openhab. You need to change it to your preferred method of sending alerts (e.g. sendmail).

1 Like

Thanks! This is what i did so far and it seems to work perfectly…

rule "Process Battery Level Updates"
when
        Item gBattery received update
then
    gBattery.members.forEach[ battery |
        if((battery.state as DecimalType) < 10) {
            val name = battery.name.split("_")
            switch(name.get(1)){
                case "Kitchen":  Notification_Proxy.postUpdate("Replace the " + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Battery, " + battery.state.toString + "% left!")
                case "LivingRoom":  Notification_Proxy.postUpdate("Replace the " + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Battery, " + battery.state.toString + "% left!")
                case "BedRoom":  Notification_Proxy.postUpdate("Replace the " + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Battery, " + battery.state.toString + "% left!")
                case "Hall":  Notification_Proxy.postUpdate("Replace the " + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Battery, " + battery.state.toString + "% left!")
 }
         }
    ]
end





rule "Process Tamper Updates"
when
        Item gTamper changed from CLOSED to OPEN
then
    gTamper.members.forEach[ tamper |
        if((tamper.state == OPEN) {
            val name = tamper.name.split("_")
            switch(name.get(1)){
                case "Kitchen":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Tamper Alert!, " + tamper.state.toString + "")
                case "LivingRoom":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Tamper Alert!, " + tamper.state.toString + "")
                case "BedRoom":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Tamper Alert!, " + tamper.state.toString + "")
                case "Hall":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Tamper Alert!, " + tamper.state.toString + "")
 }
         }
    ]
end

I’m working on this now, like you said i have multiple groups so i may need to make two of these if this don’t work out… So far not working at all lol

rule "Process Intrusion Updates"
when
//        Item gMotionSensors changed from CLOSED to OPEN or
        Item gDoorSensors changed from CLOSED to OPEN
then
//    gMotionSensors.members.forEach[ sensor |
    gDoorSensors.members.forEach[ sensor |
        if((sensor.state == OPEN) {
//         if (AlarmSysStatus.state == 1)
            val name = sensor.name.split("_")
            switch(name.get(1)){
                case "Kitchen":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Intrusion Alert!, " + sensor.state.toString + "")
                case "LivingRoom":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Intrusion Alert!, " + sensor.state.toString + "")
                case "BedRoom":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Intrusion Alert!, " + sensor.state.toString + "")
                case "Hall":  Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + "Intrusion Alert!, " + sensor.state.toString + "")
 }
         }}
    ]
end
rule "Process Intrusion Updates"
when
    Item gMotionSensors changed from CLOSED to OPEN or
    Item gDoorSensors changed from CLOSED to OPEN
then
    val motionLatest = gMotionSensors.members.sortBy[lastUpdate].last
    val doorLatest = gDoorSensors.members.sortBy[lastUpdate].last

    val latest = if(motionLatest.lastUpdate.after(doorLatest.lastUpdate) motionLatest else doorLatest

    val name = latest.name.split("_")
    Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " Intrusion Alert!, " + latest.state.toString)
end

I needed the switch statement in my example above because I had a different message I wanted to send if the battery for the Minimote was low verses the ZCOMBO. Since the static parts of all of your messages are the same you don’t need the switch.

Yea this wont work if any doors are already open on nice days and such. Have to employ your previous code you posted on this :wink:

Awesome. Yea the only time i need the alerts is when all doors etc are closed and the system is armed so i think the groups are fine cause all there states will be “CLOSED”

How can i add

if (AlarmSysStatus.state != 1

To your last post as i want it to only work if the alarm system state is NOT set to 1 or “DISARMED” which will cover my other two states 2 and 3 for away and stay.

actually looking at the sequence i would need to do:

if (AlarmSysStatus.state == 2 || if (AlarmSysStatus.state == 3

for away mode, and

if (AlarmSysStatus.state == 3

for stay mode.

so Essentially:

rule "Process Away Mode Intrusion Updates"
when
//    Item gMotionSensors changed from CLOSED to OPEN or
//    Item gDoorSensors changed from CLOSED to OPEN
then
    **if (AlarmSysStatus.state == 2 || if (AlarmSysStatus.state == 3)**
    val motionLatest = gMotionSensors.members.sortBy[lastUpdate].last
    val doorLatest = gDoorSensors.members.sortBy[lastUpdate].last

    val latest = if(motionLatest.lastUpdate.after(doorLatest.lastUpdate) motionLatest else doorLatest

    val name = latest.name.split("_")
    Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " Intrusion Alert!, " + latest.state.toString)
end

and

 rule "Process Stay Mode Intrusion Updates"
when
//    Item gMotionSensors changed from CLOSED to OPEN or
//    Item gDoorSensors changed from CLOSED to OPEN
then
    **if (AlarmSysStatus.state == 2)**
    val motionLatest = gMotionSensors.members.sortBy[lastUpdate].last
    val doorLatest = gDoorSensors.members.sortBy[lastUpdate].last

    val latest = if(motionLatest.lastUpdate.after(doorLatest.lastUpdate) motionLatest else doorLatest

    val name = latest.name.split("_")
    Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " Intrusion Alert!, " + latest.state.toString)
end

Just add if statements as described in my original response to this thread.

The code in this rule goes inside the if’s brackets. NOTE: I noticed a typo in my original posting so I’m including the whole rule.

then
    if((AlarmSysStatus.state as DecimalType) == 2 || (AlarmSysStatus.state as DecimalType) == 3) {
        val motionLatest = gMotionSensors.members.sortBy[lastUpdate].last
        val doorLatest = gDoorSensors.members.sortBy[lastUpdate].last

        val latest = if(motionLatest.lastUpdate.after(doorLatest.lastUpdate)) motionLatest else doorLatest

        val name = latest.name.split("_")
        Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " Intrusion Alert!, " + latest.state.toString)
    }
end

If statements are VERY basic to writing rules. Please review the Rule’s wiki page, the Script’s wiki page and Xtend documentation and look at examples.

However, in your original code you only want door sensors to trigger when the Alarm is in state 3. So the rule needs to be something like:

then
    val alarm = AlarmSysStatus.state as DecimalType

    // Ignore the event unless alarm is in state 2 or 3
    if(alarm == 2 || alarm == 3) {

        // Get the latest triggered contact from both groups
        val motionLatest = gMotionSensors.members.sortBy[lastUpdate].last
        val doorLatest = gDoorSensors.members.sortBy[lastUpdate].last

        // Determine which of the two is the most recent (i.e. which Item triggered the rule)
        val latest = if(motionLatest.lastUpdate.after(doorLatest.lastUpdate)) motionLatest else doorLatest

        // Only send the alert if the latest is a door or if alarm == 2 and latest is a motion detector
        // In other words always send an alert if the door opens, only send an alert for motion detectors if alarm is 2
        if(latest == doorLatest || (alarm == 2 && latest == motionLatest)) {
            val name = latest.name.split("_")
            Notification_Proxy.postUpdate("" + name.get(1) + " " + name.get(2) + " " + name.get(3) + " Intrusion Alert!, " + latest.state.toString)
        }
    }
end

Personally I would probably simplify this by splitting it into two separate rules and maybe use a lambda. It will be simpler to understand and not really use more code to write.

import org.openhab.core.items.*
import org.eclipse.xtext.xbase.lib.*

val Functions$Function1 intrusion = [GroupItem g |
    val latest = g.members.sortBy[lastUpdate].last
    val name = latest.name.split("_")
    Notification_Proxy.postUpdate(name.get(1) + " " + name.get(2) + " " + name.get(3) + " Intrusion Alert!, " + latest.state.toString)
]

rule "Process door intrusion updates"
    Item gDoorSensors changed from CLOSED to OPEN
then
    val aState = AlarmSysStatus.state as DecimalType
    if(aState == 2 || aState == 3) {
        intrusion.apply(gDoorSensors)
    }
end

rule "Process motion intrusion updates"
    Item gMotionSensors changed from CLOSED to OPEN
then
    if((AlarmSysStatus.state as DecimalType) == 2) {
        intrusion.apply(gMotionSensors)
    }
end

With this approach all that messy nested if business goes away and it is very clear what will happen when.

You my friend have way too many tricks up your sleeve lol… I like that last part alot. I replicated it like this,

val Functions$Function1 intrusion = [GroupItem g |
    val latest = g.members.sortBy[lastUpdate].last
    val name = latest.name.split("_")
    Notification_Proxy.postUpdate(name.get(1) + " " + name.get(2) + " " + name.get(3) + " Intrusion Alert!, " + latest.state.toString)
]
val Functions$Function1 tamper = [GroupItem g |
    val latest = g.members.sortBy[lastUpdate].last
    val name = latest.name.split("_")
    Notification_Proxy.postUpdate(name.get(1) + " " + name.get(2) + " " + name.get(3) + " Tamper Alert!, " + latest.state.toString)
]
val Functions$Function1 smoke = [GroupItem g |
    val latest = g.members.sortBy[lastUpdate].last
    val name = latest.name.split("_")
    Notification_Proxy.postUpdate(name.get(1) + " " + name.get(2) + " " + name.get(3) + " Co/Smoke Alert!, " + latest.state.toString)
]

Works very well and way easier.

Try:

val Functions@Function2 alert = [GroupItem g, String type |
    val latest = g.members.sortBy[lastUpdate].last
    val name = latest.name.split("_")
    Notification_Proxy.postUpdate(name.get(1) + " " + name.get(2) + " " + name.get(3) + " " + type + " Alert!, " + latest.state.toString)
]

...
    alert.apply(gMotionSensors, "Intrusion")
...
    alert.apply(gTamperSensors, "Tamper")
...
    alert.apply(gSmoke, "CO/Smoke")

When possible, DRY: Don’t Repeat Yourself.

1 Like

Done!, Thanks. Very nice and clean! That’s one thing i’ve come to love about OH is the ability to write this stuff a million different ways. :grin: