Problem with timer variables

Setup, Configuration and Use Scripts & Rules
1
  • Platform information:
    • Raspberry Pi3B+
    • OS: Rasbian Stretch
    • java version “1.8.0_201”
    • openHAB version: 2.4

I have an alarm clock which by and large works nicely. However, there is one issue I’m struggling with:
Sometimes (not everytime) when I change the waking time (let’s say from 6:00 am to 7:00 am) the alarm still goes off at both times - in the example at 6:00 am and at 7:00 am. Somehow, the timers don’t seem to get reset.

Any hint as to what I’m doing wrong is is appreciated.

Here is the code (shortened)

var Timer timer1 = null
var Timer timer2 = null
var Timer timer3 = null
rule "alarm clock"
	when 
		Item ubHour changed or 
		Item ubMin changed or
		Item ubMo changed or
		Item ubDi changed or
		Item ubMi changed or
		Item ubDo changed or
		Item ubFr changed or 
		Item ubSa changed or
		Item ubSo changed or
		Item ubWeckeraktiv received command OFF or
		System started
	then
		var java.util.concurrent.locks.ReentrantLock lock1 = new java.util.concurrent.locks.ReentrantLock()
		lock1.lock
		try{
			postUpdate(ubweckerZeitMessage, "-")
			var ZonedDateTime dt2 = ZonedDateTime.now(ZoneId.of("Europe/Berlin"))
			var ZonedDateTime dt = ZonedDateTime.now(ZoneId.of("Europe/Berlin")) //DateTime dt = new DateTime()
			var LocalDate date = LocalDate.now()
			var int offsetCoffee = -15*60
			var int offsetOff = 30*60
**//Here I try to reset the timer variables**
			if (timer1 != null) {
		       timer1.cancel()
		       timer1 = null
		       timer2.cancel()
		       timer2 = null
		       timer3.cancel()
		       timer3 = null
		    } else {
        	}
			var List<Integer> dayArray = newArrayList(0,0,0,0,0,0,0)
			if (ubMo.state == ON) {
				dayArray.set(0, 1)
			} else {
				dayArray.set(0, 0)
			}
			if (ubDi.state == ON) {
				dayArray.set(1, 1)
			} else {
				dayArray.set(1, 0)
			}
			if (ubMi.state == ON) {
				dayArray.set(2, 1)
			} else {
				dayArray.set(2, 0)
			}
			if (ubDo.state == ON) {
				dayArray.set(3, 1)
			} else {
				dayArray.set(3, 0)
			}
			if (ubFr.state == ON) {
				dayArray.set(4, 1)
			} else {
				dayArray.set(4, 0)
			}
			if (ubSa.state == ON) {
				dayArray.set(5, 1)
			} else {
				dayArray.set(5, 0)
			}
			if (ubSo.state == ON) {
				dayArray.set(6, 1)
			} else {
				dayArray.set(6, 0)
			}
			var hour = (ubHour.state as DecimalType).intValue 
			var minute = (ubMin.state as DecimalType).intValue
			var int i = dt.getDayOfWeek().getValue()
			var int daysAdd = 0
			var int n = i
			var boolean exitFlag = true
			while (daysAdd < 8 && exitFlag ) {
				logInfo("Exec2", daysAdd.toString + " - " + i.toString)
				if (dayArray.get(i-1) == 1) {
					if (daysAdd == 0) {
						date = LocalDate.now()
						dt2 = ZonedDateTime.of(date.getYear(), date.getMonthValue(), date.getDayOfMonth(), hour, minute,0,0, ZoneId.of("Europe/Berlin"))
						if (dt2.isBefore(dt)) {
							if ((n + daysAdd) >= 7) {
								i = 1
							} else {
								i++;
							}
						} else {
							exitFlag = false
						}
					} else {
						exitFlag = false
					}
				} else {
					if ((n + daysAdd) >= 7) {
						i = 1
					} else {
						i++;
					}
				}
				if (exitFlag == true) {
					daysAdd++
				}
			}
			var String msg = ""
			var String spk = ""
			if (exitFlag == false) {
				ubWeckeraktiv.sendCommand(ON)
				if (daysAdd == 0) {
					date = LocalDate.now()
					dt2 = ZonedDateTime.of(date.getYear(), date.getMonthValue(), date.getDayOfMonth(), hour, minute,0,0, ZoneId.of("Europe/Berlin"))
				} else {
					date = LocalDate.now()
					var DayOfWeek dayOfWeek = DayOfWeek.of(i)
					var TemporalAdjuster adj = TemporalAdjusters.next(dayOfWeek)
					date = date.with(adj)
					dt2 = ZonedDateTime.of(date.getYear(), date.getMonthValue(), date.getDayOfMonth(), hour, minute,0,0, ZoneId.of("Europe/Berlin"))
				}
				switch(dt2.getDayOfWeek().toString) { 
					case "MONDAY" : ubDummy.sendCommand("Montag")
					case "TUESDAY" : ubDummy.sendCommand("Dienstag")
					case "WEDNESDAY" : ubDummy.sendCommand("Mittwoch")
					case "THURSDAY" : ubDummy.sendCommand("Donnerstag")
					case "FRIDAY" : ubDummy.sendCommand("Freitag")
					case "SATURDAY" : ubDummy.sendCommand("Samstag")
					case "SUNDAY" : ubDummy.sendCommand("Sonntag")
				}
				spk = "Die nächste Weckzeit ist am " + dt2.getDayOfMonth().toString + "." + dt2.getMonthValue().toString + "." + dt2.getYear().toString + " um " + hour + " Uhr " + minute
				ubDummy.sendCommand(spk)
				if (hour < 10) {
					msg = "0"
				}
				msg = msg + hour.toString + ":"
				if (minute < 10) {
					msg = msg + "0"
				}
				msg = msg + minute.toString
				msg = dt2.getDayOfWeek().toString + ", " + dt2.getDayOfMonth().toString + "." + dt2.getMonthValue().toString + "." + dt2.getYear().toString + " - " + msg
				postUpdate(ubweckerZeitMessage, msg) 
				var Duration dur = Duration.between(dt, dt2)
				var int diff = dur.getSeconds.intValue
				var int sdays = diff / 86400
				var int shours = (diff % 86400) / 3600
				var int smin = diff % 3600 / 60
				var int ssek = diff % 60
				pushover("Zeit verbleibend: " + sdays.toString + " T " + shours.toString + ":" + smin.toString + ":" + ssek.toString)
				timer1 = createTimer(now.plusSeconds(dur.getSeconds.intValue)) [|
				         //stuff gets done here correctly
				]
				timer2 = createTimer(now.plusSeconds(dur.getSeconds.intValue + offsetCoffee)) [|
					//stuff gets done here correctly
				]
				timer3 = createTimer(now.plusSeconds(dur.getSeconds.intValue + offsetOff)) [|
					ubWeckeraktiv.sendCommand(OFF)
					//stuff gets done here correctly
                                        ubSleeping.sendCommand(OFF)
				]				
			} else {
				msg = "- no alarm set -"
				postUpdate(ubweckerZeitMessage, msg)
			}
			
		}  catch(Throwable t) { }
	finally {
		lock1.unlock()
	}
end
2

You’re going to have to help us out a bit here.
It’s quite long code, it’s uncommented so we have to work out or guess what it is supposed to be doing and what variables represent.
And apparently, it isn’t actually the code that goes wrong anyway?
Don’t make it difficult to help.

Quick look, the cancel-all-timers code is conditional on just one timer. What happens if the others are active? Why not just cancel them all?

There’s a nifty shorthand for if-timer-not-null-then-cancel using ?.
someTimer?.cancel
someTimer = null

You should be aware that if you edit a rules file that has already spawned timers, which seems likely in your case, the running timers stay scheduled even though they are ‘orphaned’ from their original owning rule.
The newly edited rule file can then spawn some new versions of the old timers as well.
The orphans often give errors when their scheduled time comes around, but that very much depends on what they do.

3

Thank you for your reply. I will include a drastically shortened version of my code that basically only contains the lines relating to the timers.

The reason I check timer1 before resetting it is, that if I don’t do that and the timers are “empty” an error gets thrown.

I suspect that timers get orphaned somehow when I adjust the waking time. It happens even if I don’t edit the rules file but just set the time.

rule "alarm clock"
	when 
                 //when the time or the day(s) on the UI gets set
		Item ubHour changed or 
		Item ubMin changed or
		Item ubMo changed or
		Item ubDi changed or
		Item ubMi changed or
		Item ubDo changed or
		Item ubFr changed or 
		Item ubSa changed or
		Item ubSo changed or
                //when item gets set to OFF when the alarm clock has executed all 3 timers
		Item ubWeckeraktiv received command OFF or
		System started
	then
		var java.util.concurrent.locks.ReentrantLock lock1 = new java.util.concurrent.locks.ReentrantLock()
		lock1.lock
		try{
                       // I reset the timers to null
			if (timer1 != null) {
		             timer1.cancel()
		             timer1 = null
		             timer2.cancel()
		             timer2 = null
		             timer3.cancel()
		             timer3 = null
		        } 

			//in the original code the date/time of  'now' (dt) and the next instance of the alarm execution (dt2) is calculated
				//calculate the Duration between the two dates/times
				var Duration dur = Duration.between(dt, dt2)

				timer1 = createTimer(now.plusSeconds(dur.getSeconds.intValue)) [|
					//here stuff is done correctly - the alarm wakes me - Amazon echo plays a radio station, sleeping room light gets switched on ...
				]
				timer2 = createTimer(now.plusSeconds(dur.getSeconds.intValue + offsetCoffee)) [|
					//here stuff is done correctly - 15 minutes before waking time the caffee machine gets switched on
				]
				timer3 = createTimer(now.plusSeconds(dur.getSeconds.intValue + offsetOff)) [|
                                        //here stuff is done correctly - this is executed 30 min. after the waking. It switches off the coffee machine and the Amazon echo playing a radio station if I didn't tell the alarm clock, that I'm up.
                                        //the next line switches triggers the rule to be executed again to calculate the next waking time 
					ubWeckeraktiv.sendCommand(OFF)
					
				]
				
			} else {
				msg = "- no alarm set -"
				postUpdate(ubweckerZeitMessage, msg)
			}
			
		}  catch(Throwable t) { }
	finally {
		lock1.unlock()
	}
end
4

Yes, that’s why I showed you a shorthand method to do this without error.

In your current rule, what happens if timer 2 and timer3 are still running, but timer1 is null?

Do your secret timer codeblocks set themselves to null upon completion?

5

I don’t do anything with timer1 or any other timer inside the timer blocks (except timer3 - see below). In order of their trigger times, timer2 starts the coffee machine, timer1 sets off the Amazon echo, switches on the light and the media system, timer3 switches off everything if I haven’t told Amazon echo that I’m up (which switches a virtual switch item). In the timer block of timer3 the virtual item ubWeckeraktiv is sent an OFF command which triggers the rule again for the next ‘round’. This of course cases the 3 timers to be reset.

6

Okay, so lets clarify a few non-obvious features of Timers.

When a timer does its thing, expires and executes its code, any variable being used as a handle for it does not get set to null. Doesn’t matter, unless you do something like looking to see if the handle is null in order to determine if the timer is still running.

Hence my question about if you explicitly set the handles null, and you don’t. Still doesn’t matter, as I don’t think you check for null for that purpose.

If you re-use a variable as a handle for a new timer, in the style of
x = createTimer( ....
that does not cancel or destroy any previous timer that handle pointed to. You have to do that explicitly. The “old” timer exists independently and carries on as usual, you’ve only lost your handle on it.
Plus of course, you’ve made a new timer as well.
That all sounds like a good possibility to cause the effect you describe, the “same” timer going off twice.

Of course your rule should prevent that, it does try to cancel existing timers before making new ones. But the symptom is such a good fit, you need to take a very critical look in this area.

I worry at this part

             if (timer1 != null) {
		       timer1.cancel()
		       timer1 = null
		       timer2.cancel()
		       timer2 = null
		       timer3.cancel()
		       timer3 = null
		    } else {
        	}

because it only executes if timer1 is not null.
Now you shouldn’t get in the situation where timer1 is null but the others aren’t - but you do know that something goes wrong sometimes.
I’d make the whole section more surefire like so

timer1?.cancel
timer1 = null
timer2?.cancel
timer2 = null
timer3?.cancel

Just does it, no ifs or buts.
In fact, you can omit the null settings - you never check for that later, and there is no other harm in having a handle to a cancelled timer.
You could choose to relocate this section, so that it only destroys the old timers after you have successfully validated new parameters.

Nevertheless, there is some other underlying problem.
The rule has a lot of triggers, fair enough. There will be a lot of calculating and cancelling/creating of timers every time. Here I suspect it could run into re-entrancy - kicking off another run of the rule before the last one has completed. Here I think is how it can get in a mess with creating multiple timers.

You’ve clearly thought about this already, and attempted locking.
But I’m afraid it’s messed up - every time the rule begins it creates a brand new lock, so that’s always available and never blocks.
You’d need to make the lock a global variable, outside of the rule, so that there is only one.
I’m not convinced you need the lock at all…

You could improve your chances by putting the cancel much closer in time to the createTimer.
You could make it even better by not cancelling at all, just rescheduling. You’d need some logic to create-if-null, reschedule-if-already-exists.
I think that should be sufficient without the use of locks.

1 Like
7

Thanks a lot for your thorough analysis and answer - I will try to reschedule the timers instead of cancelling them. I see now that there are too many stumbling blocks in the design of my alarm clock. Plus, I will reduce the times the timers get reset. Right now, the timers get reset each time I press “up” or “down” on a corresponding setpoint element in the UI. I will add a switch which triggers the rule to be executed - this will also make the lock unnecessary.

Actually, I just modified an alarm clock which I’d found here (Example 3) - basically just edited for the aspect of taking into account daylight saving time (which hopefully will soon be unnecessary in the EU).

8

I guessed that, but I really don’t think that’s a problem provided we make the rule robust.
On the other hand, a UI requiring set-up-and-press-save isn’t that strange either.

9

Doesn’t this need to be triple === /!== for null comparison?

if (timer1 !== null){
10

Yes indeed, it should be (although it will work)