Please critique my first rule


#1

Howdy! I recently got HE and installed my Konnected and luckily everything seems to be working so far.
I am completely new to automation but I am learning as best I can.
Here is the first rule I've made and it wasn't as simple as I would have thought. I tried to use some of the examples here in the forums but they either did not work or did not have the options they did. This rule seems to be working flawlessly but please take a look at it and give me feedback as to how it could be more efficient or elegant.

The goal:
Announce the door opening
If door remains open after 2 minutes, ask for it to be closed,
If the door remains open, repeat request every 5 minutes.
If the door is closed immediately cancel the repeat and finish the rule.

Thanks!

Wayne


#2

Let's see:

If you open and close the door twice the rule will now be running twice and you'll have the rule looping twice. Do it 3 times and you'll have it running 3 times.

To avoid you can handle a door close and cancel the delay and repeats at the top level instead of in the repeat. Maybe something like this, I removed the delay to use the time from the repeat...to avoid having to cancel the delay/repeat. (does it run once right away? can't remember)

if (door open) then
  say door open
  repeat every 00:05:00 (cancel)
    say close door
else if(door close) then
  cancel repeat
end if

#3

Here's a rule that I have that does sort of the same thing.

If the front door is unlocked, it sends a message after 5 minutes (that's what the POST does).

It then waits 7 minutes, and the repeats having Alexa announce that the door is open 5 times every two minutes. (The virtual contact sensor is an Alexa trigger.)

Then it locks the front door.

If at any time the door is locked, the delays and repeats are cancelled.


#4

Well, I'm not the best at this, so the other guys will probably critique me too, and that's fine with me. It's how we learn.

When you use an IF statement, you want to have either a counter statement to tell the rule what to do if the statement is not true. If you don't want it to do anything when the IF statement is not true, then you simple stop evaluation. That is done with END IF, which by the way, should be at the end of your IF statements, regardless. It's kind of like putting a period on a sentence.

So if you take your rule example above, then you should probably start by changing the trigger event. I mean, what you have will work, but you normally just want something to kick off the rule (e.g. Trigger) and then you're going to specify the exact way to trigger is like you've done in your first action. So you would normally make the trigger be simply changed, rather than specifically Open or specifically Closed. That'll start the first action, where the door sensor will be evaluated to see if it's Open.

Next in your first action, you've ask to evaluate Stairway door to see if the contact sensor is Open. If it is, then the sensor state will be True and the rule up to that point is also True. THEN it will speak that the "stairway door opened". If that's what you want up to that point, then it should work as you expect.

In the next step, you're asking for a delay of 2 minutes. Again, that's fine, but if the stairway door closes before that and then opens again, it cannot evaluate it because it's waiting for that 2 minute timeout to finish. So add Cancel to that. What that does is cancels the delay if the Stairway door is closed, and it can then immediatley start in on the next part of your rule where it checks the Stairway door contact sensor again to see if it's still open. But, before you can ask another IF question, you need to end the first one. So you should also have END-IF after your Delay

Now that you've stopped asking the first question in your rule, you can add another IF and evaluate the stairway door contact sensor again to see if it's still open. If it is, you want to repeat every 5 minutes until it's done. I see you asked it to stop on truth change, and that's good because otherwise it would just keep repeating even when the door was closed. However, then next thing you want is for it to speak Please close the Stairway door. The way you've written it, that won't happen. You want speak to follow Repeat every 5 min in the rule when Stairway door open is TRUE, ELSE Stop Repeating Actions. You don't need to ask again if the Stairway door is closed, because when the first and second IF statements in your rule are not TRUE, then the Stairway doors IS indeed closed, so the result is FALSE.

There's a better way to do this, but I don't know what it is, and what you have should work, just needs a few changes.

So this is what I think will work for you.

IF (Stairway door open) THEN
Speak on Living Room Mini: '%device% open'
END-IF
IF (Stairway door open) THEN
Delay 0:02:00 <- Cancel
Repeat every 0:05:00 (stop)
Speak on Living Room Mini: 'Please close %device%'
ELSE
Stop Repeating Actions


#5

Thanks for the replies! While it may not be pretty or proper it does appear to work. I appreciate your opinions and you all have made some good suggestions and / or taught me more. Evidently 4.0 is relatively new and there isn't real complete documentation on it yet. I know 3.0 is going away so I figured I'd just learn 4.0 from the start. But so many of the posts are from 3.0 it can be confusing. For exp: It took me a while to figure out repeat "stop" was the same as the "truth change"...

@asj Is there a way to test if the rule is already running and prevent it from multiplying? Also, every time I placed the cancel repeat at the end of the rule, it would always say "close the door" 1 more time after the door was already closed. That could happen as late as 5 minutes after the door was closed... Not good for the WAF.,,

@jabecker I added the cancel to the delay. I kinda thought it was giving permission for the "cancel delay" action to affect it.
Is "end-rep" necessary if your not using a set amount of times? Also if you don't mind I am going to save your lock rules for when I can find some reliable economical locks. I have 6 exterior doors and would prefer them to all be the same.

@SmartHomePrimer You should write documentation! :slight_smile: Great job explaining the processes. My first attempts were trying to use the changed trigger and it seemed to work on the IF open but I couldn't get it to work correct on the closed state. Plus the closed state kept coming up as false instead of true making the rule behave as if it was open state. I added the cancel everyone suggested. And I suppose it does make good habit to always close the function even if it is not needed. I'll make sure all my IF- statements are ended. As mentioned above, every time I placed the stop repeating after the speak I always got 1 more message after the door was closed. and sometimes that message would play 4-5 mins after closing the door. I guess it clears the repeat but doesn't clear the queued msg. So I had to move it before the speak to ensure it never got queued. Didn't make sense to me either.

I've made the recommended changes to my rule and will also try out yall's rules tomorrow. Cant test anything tonight because wife really really likes her sleep. :slight_smile:

At least I can just clone this rule when it is perfect. Good thing too, I have 8 doors...

Thanks again guys!

Wayne


#6

I think, actually you should try this. Plus I forgot the last END-IF I said you should put there :crazy_face:

IF (Stairway door open) THEN
Speak on Living Room Mini: '%device% open'
END-IF
IF (Stairway door closed) THEN
Stop Repeating Actions
ELSE
Delay 0:02:00 <- Cancel
Repeat every 0:05:00 (stop)
Speak on Living Room Mini: 'Please close %device%'
END-IF


#7

This looks good to me, but you'll need an END-REP after "Speak on Living Room Mini..."--the names are a bit confusing, but Repeat... and END-REP just enclose the actions that should be repeated, much like how IF and END-IF enclose a conditional (something that in C-style languages, including Groovy, would be handled with curly braces; Rule Machine is apparently going for a BASIC-esque syntax). The latter is not to be confused "Stop Repeating Actions," which does what it says.

I feel like you might know this and may have just left it out, so I'm explaining for anyone who may see it but wasn't sure; I think part of the confusion may be that unlike IF and END-IF, the two delinerators of this block differ in capitalization convention so it's less intuitively clear.


#8

So it should be like this?

IF (Stairway door open) THEN
Speak on Living Room Mini: '%device% open'
END-IF
IF (Stairway door closed) THEN
Stop Repeating Actions
ELSE
Delay 0:02:00 <- Cancel
Repeat every 0:05:00 (stop)
Speak on Living Room Mini: 'Please close %device%'
END-REP
END-IF


#9

When you add a Cancel to a delay, or a Stop to a repeat, you are correct that's what gives a Cancel Delayed Actions or Stop Repeating Actions to actually cancel/stop the action.

It's always best practice to use END-IF and END-REP to close out IFs and REPEATs. It's not always technically required, depending on the structure of your rule and what comes after the IF or REPEAT. But if you get in the habit of always using END-IF and END-REP, it will make seeing what's happening in complex rules a lot easier.

Feel free to copy my rule if it's useful. That's why I posted a screen shot. :wink:


#10

I see. But really the primary reason you needed to add END-REP, for sure in your rule is that you requested another action afterward, and you don't want that to repeat. Is that correct? Or did I misunderstand what @dan.t writes here?

Appreciate the guidance by the way. Both you and @bertabcd1234


#11

That's correct. The END-REP keeps following actions out of the repeat loop. If you don't have any following actions, then it's not strictly required.

But what if you want to add actions next month? Gonna remember that END-REP?

Probably it's a hangover from many years of programming in many different languages on a variety of platforms, and having to debug code written by others, but if there's a way to make it more clear, I usually use it. It just makes it a lot easier to see what actions should be considered as a group.


#12

I understand the best practice of END-IF, END-REP, and Delay Cancel so I will make sure I close each function.
I copied the final suggested rule and it appears to work OK except: If the door is closed before the initial delay time is up, it will continue to repeat forever. Even after a hub reboot. The only way I found to stop it (besides a pause) is to delete the rule. Below is the rule and the log. I did shorten the time periods for testing purposes but I wouldn't think that would cause an issue. Could it be that the trigger being changed started a duplicate run of the rule?

app:3552019-09-06 09:03:34.142 infoCommunity Advice Test: Action Repition stopped
app:3552019-09-06 09:03:34.089 infoCommunity Advice Test: Paused
app:3552019-09-06 09:03:33.883 infoAction: END-REP (waiting for next)
app:3552019-09-06 09:03:33.831 infoAction: Speak on Living Room Mini: 'Beep'
app:3552019-09-06 09:03:33.803 infoCommunity Advice Test: Repeating Actions
app:3552019-09-06 09:03:23.689 infoAction: END-REP (waiting for next)
app:3552019-09-06 09:03:23.625 infoAction: Speak on Living Room Mini: 'Beep'
app:3552019-09-06 09:03:23.590 infoCommunity Advice Test: Repeating Actions
app:3552019-09-06 09:03:13.501 infoAction: END-REP (waiting for next)
app:3552019-09-06 09:03:13.441 infoAction: Speak on Living Room Mini: 'Beep'
app:3552019-09-06 09:03:13.406 infoCommunity Advice Test: Repeating Actions
app:3552019-09-06 09:03:03.294 infoAction: END-REP (waiting for next)
app:3552019-09-06 09:03:03.232 infoAction: Speak on Living Room Mini: 'Beep'
app:3552019-09-06 09:03:03.203 infoCommunity Advice Test: Repeating Actions
app:3552019-09-06 09:03:03.151 infoDelay Over: Delay 0:00:30 (cancel)
app:3552019-09-06 09:02:46.715 infoAction: END-IF
app:3552019-09-06 09:02:46.709 infoAction: END-REP (skipped)
app:3552019-09-06 09:02:46.705 infoAction: Speak on Living Room Mini: 'Beep' (skipped)
app:3552019-09-06 09:02:46.697 infoAction: Repeat every 0:00:10 (stop) (skipped)
app:3552019-09-06 09:02:46.692 infoAction: Delay 0:00:30 (cancel) (skipped)
app:3552019-09-06 09:02:46.688 infoAction: ELSE (skipping)
app:3552019-09-06 09:02:46.684 infoAction: Stop Repeating Actions
app:3552019-09-06 09:02:46.679 infoAction: IF (Stairway door closed(T) [TRUE]) THEN
app:3552019-09-06 09:02:46.641 infoAction: END-IF
app:3552019-09-06 09:02:46.637 infoAction: Speak on Living Room Mini: '%device% opened' (skipped)
app:3552019-09-06 09:02:46.626 infoAction: IF (Stairway door open(F) [FALSE]) THEN (skipping)
app:3552019-09-06 09:02:46.549 infoCommunity Advice Test Triggered
app:3552019-09-06 09:02:46.533 infoCommunity Advice Test: Stairway door contact closed
app:3552019-09-06 09:02:33.008 infoAction: Delay 0:00:30 (cancel)
app:3552019-09-06 09:02:33.003 infoAction: ELSE (do actions)
app:3552019-09-06 09:02:32.998 infoAction: Stop Repeating Actions (skipped)
app:3552019-09-06 09:02:32.992 infoAction: IF (Stairway door closed(F) [FALSE]) THEN (skipping)
app:3552019-09-06 09:02:32.954 infoAction: END-IF
app:3552019-09-06 09:02:32.875 infoAction: Speak on Living Room Mini: '%device% opened'
app:3552019-09-06 09:02:32.865 infoAction: IF (Stairway door open(T) [TRUE]) THEN
app:3552019-09-06 09:02:32.792 infoCommunity Advice Test Triggered
app:3552019-09-06 09:02:32.772 infoCommunity Advice Test: Stairway door contact open


#13

I tried changing the trigger to contact open but it does the same thing...


#14

I'd seen Bruce mention this with END-IF (though he admonished us to include it anyway) but hadn't seen anything about END-REP (and I have seen malformed rules with weird indentation where a missing END-REP was the only thing I could see that might have been a problem--not above, but elsewhere). Either way, definitely good practice to close!

I again think part of the confusion might be the contrast here:

  • IF...THEN goes with END-IF
  • Repeat... goes with END-REP

Maybe capitalizing "REPEAT" would help make this pairing clearer?


#15

It's because the rule is waiting on the delay, and you cancel the repeating action, there's nothing to cancel yet. You have to cancel the delay and the repeat.

So the rule finishes the delay and starts repeating. It's impossible to cancel since every time you open the door it starts a new delay->repeat loop.

But if you just add a cancel delay as is, I think you'll find it repeats still. So the delay needs to be inside the repeat to make sure you get them both. If you just make the say delayed by 5 mins and cancellable I think you'll have a nice non-race condition way to cancel both.


#16

This is so complicated for such a simple rule.... I miss these days.

is that not simple? I'm hoping RM gets there eventually.


#17

I'm not sure if this will help, but I'll post it anyway. The first difference I see is that you are putting the actions you are focusing on in the "Else" section. I prefer to do it the other way. The second is that I like using ELSE-IF to give me more control over what the actions are. Again this is a preference, but it's helped me control the outcome of the rule.

As I think it through, you could add the initial announce above the delay. Then set the delay to 2 minutes. The final piece I think would then be setting the Repeat value to 5 minutes. Almost exactly what @asj wrote above.

if (door open) then
say door open
delay 00:02:00
repeat every 00:05:00 (cancel)
say close door
else if(door close) then
cancel repeat
end if


#18

That's real close to what I came up with. I put the delay above the repeat so it runs once and done. I had to place the Stop inside the repeat function for it to kill the repeat reliably.
Here is what I came up with that seems to be working...

Select Actions for Community Advice Test
IF (Stairway door open(F) [FALSE]) THEN
Speak on Living Room Mini: '%device% opened'
END-IF
Delay 0:02:00 (cancel)
Repeat every 0:05:00 (stop)
IF (Stairway door closed(T) [TRUE]) THEN
Stop Repeating Actions
ELSE
Speak on Living Room Mini: 'Please close the %device%'
END-REP
END-IF


#19

That's pretty much how I do it, too. See my example above near the top.

Except I use an ELSE instead of ELSE-IF because it's a straight forward, binary IF. The lock is either locked or it's unlocked. Using ELSE-IF forces the rule to evaluate both conditions, when that's not really necessary.


#20

Ah that looks good! To save yourself from edge cases, when canceling the repeat also cancel the delay.