Please critique my first rule

mrbigkid · September 6, 2019, 4:33am

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! 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.

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

Thanks again guys!

Wayne

SmartHomePrimer · September 6, 2019, 4:48am

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

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

bertabcd1234 · September 6, 2019, 4:58am

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.

SmartHomePrimer · September 6, 2019, 5:11am

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

jabecker · September 6, 2019, 5:11am

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.

SmartHomePrimer · September 6, 2019, 5:16am

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

jabecker · September 6, 2019, 5:23am

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.

mrbigkid · September 6, 2019, 1:17pm

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

mrbigkid · September 6, 2019, 1:25pm

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

bertabcd1234 · September 6, 2019, 1:28pm

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?

asj · September 6, 2019, 1:32pm

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.

jrfarrar · September 6, 2019, 2:47pm

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

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

homeauto2112 · September 6, 2019, 3:38pm

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

mrbigkid · September 6, 2019, 3:51pm

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

jabecker · September 6, 2019, 3:52pm

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.

asj · September 6, 2019, 4:18pm

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

mrbigkid · September 6, 2019, 4:21pm

@asj I'll add a delay cancel at the end JIC, but excuse my ignorance, What is an edge case?

asj · September 7, 2019, 5:38am

An edge case is part of a problem that happens in frequently, and also a bit outside the norm. So in your case if you don't cancel the delay if the door open, closes, then opens again you would not get the delay you're looking for before the warning played. Sometimes edge cases don't mater much since they're so infrequent no one cares. Other times when things appear random/hay wire etc it really hurts the WAF.

BorrisTheCat · September 7, 2019, 12:04pm

What's wrong with this? This will work correctly.

Trigger stairs door changed.

IF(Stairway door open) THEN
   Speak on living room mini: '%device% 
   %value%'
   Delay 0:02:00 (cancel)
   Repeat every 0:05:00 (stop)
   Speak on living room Mini: ' please close the 
   %device%'
   END-REP
ELSE
Stop repeating actions
Cancel delays
END-IF

My rule I have had for months

mrbigkid · September 7, 2019, 12:13pm

Thanks @asj, that makes sense. might as well write the code the best way to begin with. and the WAF is a Huuge consideration in this household...