Extending Blockly with new openHAB commands

Hi Rich,

I hope I didn’t offend you but I am really trying to find an MVP (minimal viable product) to get this whole functionality delivered as soon as possible and then learn and extend from it.

Having slept over it I think the whole became unclear because I was mixing up the simple timer and the complex timer functionality. The simple timer was meant to have a really simple version of the timer and which would work for 80% of all tasks. Actually this didn’t have the else-branch but I may want to move this there while the more flexible shouldn’t have one. Note that the simple one by intention doesn’t even allow you to provide a name.

simple timer
image

if (this.timers['simpleTimer'] === null) {
	this.timers['simpleTimer'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(10), function(){
	  logger.error('abc')
	})

	} else {
		this.timers['simpleTimer'].reschedule(zonedDateTime.now().plusSeconds(2))
}

Complex timer functionality

However, we have all the other methods of a timer available individually with which we probably can do most of you want to achieve.

Note that it does now allow yet to use dynamic names as I have no clue how to do that as blockly uses static code generation, so the name in the below case are always named “richEvent”. If I ever find out

Let me go through each of your examples and try to reproduce these.

1st example: this.tm.check("richEvent", "5m", runme); Call runme after five minutes.
If called and the timer exists cancel the timer

producing

function runme() {
  logger.info('abc')
}
if (this.timers['richEvent'] === null) {
	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusMinutes(5), function(){
	  runme();
	})
}

2nd example: this.tm.check(“richEvent”, “5m”, runme, true);`
Call runme after five minutes. If called and the timer exists, reschedule it for five minutes

  • I changed the time to 5 seconds to allow easier testing
  • Note that the rescheduling happens over and over again. If you didn’t want that put the rescheduling to the upper block
  • I added a manual cancelation possibility via a switch to have a chance to “kill” the rescheduling timer.

function runme() {
  logger.info('abc')
  this.timers['richEvent'].reschedule(zonedDateTime.now().plusSeconds(2))
}
if (itemRegistry.getItem('cancelRichTimer').getState() == 'OFF') {
  logger.info('canceling Timer')
  if (this.timers['richEvent'] !== null) { this.timers['richEvent'].cancel()}
} else {
  if (this.timers['richEvent'] === null) {
  	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(2), function(){
  	  runme();
  	})
  }
}

3rd example:
this.tm.check(event.itemName, “1m”, null, false, flapping);
Create a timer for five minutes. Do nothing if the timer expires. If called again when the timer already exists call flapping. Do not reschedule

  • the empty run bracket may look weird but it actually creates a noop-function like in your code

function flapping() {
  logger.info('flapping ')
}
logger.error('running the rule')
if ((this.timers['richEvent'] && this.timers['richEvent'].isActive()) == true) {
  flapping();
}
if (this.timers['richEvent'] === null) {
	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusMinutes(1), function(){
		})

}

4th example:
this.tm.check(“richEvent”, “5m”, null, true, flapping);
Create a timer for five minutes. Do nothing if the timer expires. If called again when the timer already exists, call flapping. Reschedule the timer for five minutes.

  • Reschedule the timer for five minutes: What should happen there? If I get your code right it does reschedule the rule that doesn’t do anything

function flapping() {
  logger.info('flapping')
}
if ((this.timers['richEvent'] && this.timers['richEvent'].isActive()) == true) {
  flapping();
  this.timers['richEvent'].reschedule(zonedDateTime.now().plusMinutes(5))
}
if (this.timers['richEvent'] === null) {
	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusMinutes(5), function(){
  })

}

5th example:
this.tm.check(event.itemName, “5m”, runme, false, flapping);
Create a timer for five minutes. Call runme if the timer expires. If called again when the timer already exists, call flapping. The timer will be cancelled and runme will not be called.

function runme() {
  logger.info('runme was called')
}
function flapping() {
  logger.info('flapping')
}
if ((this.timers['richEvent'] && this.timers['richEvent'].isActive()) == true) {
  flapping();
  if (this.timers['richEvent'] !== null) { this.timers['richEvent'].cancel()}
}
if (this.timers['richEvent'] === null) {
	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusMinutes(5), function(){
	  runme();
	})
}

6th example:
this.tm.check(event.itemName, “5m”, runme, true, flapping);
Create a timer for five minutes. Call runme if the timer expires. If called again when the timer already exists, call flapping. Reschedule the timer for five minutes.

function runme() {
  logger.info('runme was called')
}
function flapping() {
  logger.info('flapping')
}
if ((this.timers['richEvent'] && this.timers['richEvent'].isActive()) == true) {
  flapping();
  this.timers['richEvent'].reschedule(zonedDateTime.now().plusSeconds(5))
}
if (this.timers['richEvent'] === null) {
	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusMinutes(5), function(){
	  runme();
  })
}

Summary
I think we can achieve all of these examples you provided. I hope that this us both a good feeling to move forward :slight_smile: :pray:

Final note:

In theses case I took out the automatically generated else branch of rescheduling which we probably should for the non-simple named timer.
For the simple one (at least for the MVP I would add the automatic rescheduling and maybe in a later version we can add another block that would allow options for retriggering.

I have applied all above mentioned changes. Also, from the fact showing that the above is possible with the blocks, I could imagine that it should be possible to create a block similar to your API that allows to create the code that is similar to what the individual blocks do. If you would provide an idea how the block would look like (rather big I think) I could start approaching it as the next step.

Dear all,
i dont want to interrupt your discussion however, I do have a question.
I really like the idea behind blockly but i am really missing the timer or delay functions.
Is there a way to use your custom blocks already.
If yes, can you give me a hint how to implement / install them to my openhab 3.1 version?
Thank you a lot for your work!!

They are work in progress and not available yet. Everything you see above is currently in the making and under review. All screenshots you see above have been done by me and are part of the Pull Request which is under review.

So stay tuned - I hope we can make them available soon but reviewing the code is a lot of work for the people doing it (which is important to have a good design and good implementation quality).

Hope that clarifies it.

1 Like

Cool, thank you for your answer!
Than i will stay tuned and thank you for your ongoing work! :slight_smile:

I’m not using timers in my rules (actually prefer to avoid them and use e.g. the expire profile instead) so I really have no strong opinion on these regarding implementation. It looks fine though!

On the design though, I’d put the “cancel on retrigger” part below the statements.

image

If the timer names are now just string identifiers, I’m wondering why we would need a separate “timer MyTimer” for those - just use regular text blocks and let the users manage the timer names themselves.
Same for the other blocks that should avoid technical names, and for instance

  • “isActive {shadow “timer MyTimer” block}” => “timer {shadow regular string block MyTimer} is active”
  • “hasTerminated {shadow “timer MyTimer” block}” => “timer {shadow regular string block MyTimer} has terminated”
  • etc.

If you’re hardcoding all simple timers with the "simpleTimer" name then you can’t have several of them at the same time - maybe even among multiple rules.

No worries. It usually takes quite a bit to offend me. I just didn’t have anything more to say and then the weekend came and I went offline.

That make some sense. I wish there were a way to figure out what the most common use case is for timers. I could also see cancelling being used just as often as rescheduling. But rescheduling more closely follows the behavior for Expire so maybe it makes sense to be consistent. On-the-other-hand, if that reschedule behavior for Expire doesn’t do what is needed, maybe it’d be nice to have a simple block one that cancels.

Is it feasible to have two simple timer blocks, one that reschedules and another that cancels? Those combined will be by far the most commonly used I suspect.

Then the complex one can handle all the other use cases and the complex one can wait.

The code doesn’t match the blocks. Where is runme?

As examples though that looks good. I assume the code that sets up this.timers is there and didn’t make the copy and paste? I’m just looking for potential errors. The approach looks fine.

That’s going to be a problem for rules that trigger on a Member of a Group and sets a separate timer for each Item. We might need to do something a little hacky to get it working. I don’t think we can ignore that use case entirely.

The complex examples otherwise look good. Not too complex from the blocks perspective and the code looks OK too. The only comment I have is I generally prefer to use the items dict to get the state of Items rather than pulling the Item from the IR. It makes the code a little more concise and it works better in a couple of cases. So

if (itemRegistry.getItem('cancelRichTimer').getState() == 'OFF') {

would become

if(items['cancelRickTimer'] == 'OFF') {

It’s a minor thing that probably doesn’t matter in the long run.

Looks good. Straight forward and simple.

There is a bug here. Nothing is setting the timer to null in all cases. For example, when the timer is cancelled it wont return to null and therefore a new one can never be rescheduled. Similarly, runme doesn’t set the timer to null either so once the timer expires once a new one will never be created again.

If there is a way we can handle that for the user it would be kind of cool. For example, keeping the same blocks as above but produce the following code.

function runme() {
  logger.info('abc')
  this.timers['richEvent'].reschedule(zonedDateTime.now().plusSeconds(2))
}
if (itemRegistry.getItem('cancelRichTimer').getState() == 'OFF') {
  logger.info('canceling Timer')
  if (this.timers['richEvent'] !== null) { 
    this.timers['richEvent'].cancel(); 
    this.timers['richEvent'] = null;
  }
} else {
  if (this.timers['richEvent'] === null) {
  	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(2), function(){
  	  runme();
      this.timers['richEvent'] = null;
  	})
  }
}

This applies to all the examples I think. We need to manage setting the timer back to null for the user. It looks like it’s pretty possible.

Need to set the timer back to null for the user if we are using the fact that it’s null as the flag to indicate a new timer needs to be created.

Correct. We only care in this case whether the timer exists. When the time is up we don’t do anything. If the timer does exist, we need to do something though (the flapping function).

Same null bug here as well. We probably need to replace the noop with setting the timer back to null.

Same null bug needs to be handled here too.

Same null bug to handle here too.

Of course. I just wanted to make sure we didn’t exclude any use case for a timer which always rescheduling would have done. The blocks above look easy enough to use and the code looks right.

Well, I’m not certain that we would necessarily need to implement my Timer Manager in Blockly, though that would be kind of cool. In text based rules it does save a ton of work, at least for me.

We have the problem with creating a unique name for the Timer to figure out though. Once we have that we would need the five arguments:

  • key: unique name for the timer
  • when: what time in the future to run the timer, in the block that would probably be two arguments
  • function: what to run when the timer goes off
  • reschedule: flag for what to do if the timer exists
  • flapping: code to run if the timer exists

I can see leaving flapping out and having the users code that on their own like your examples above.

So if we leave the flapping out I don’t know that there are any additional arguments. We’d just need to be able to manage timers by something more dynamic than a hard coded name (i.e. the name is based on a variable). And maybe the best way to handle that is with a separate block. Then it’d be more generic and we can store almost anything on an Item by Item basis, not just Timers.

Can a text block be populated with a variable? That’s a pretty big need for writing rules where one needs to manage one timer per Item that triggers the rule, which is a really common use case.

I’m about 95% certain that isn’t a problem. Individual scripts (Script Actions/Script Conditions) appear to be completely isolated from each other. Reuse of variable names, isn’t a problem like it was when using global variables in .rules files.

No worries. It usually takes quite a bit to offend me.

That relieves me :hugs:

Is it feasible to have two simple timer blocks, one that reschedules and another that cancels? Those combined will be by far the most commonly used I suspect.

Yes, probably, let me see what I can do quickly here

The code doesn’t match the blocks. Where is runme?

It was just forgotten. It is actually in the code.

As examples though that looks good. I assume the code that sets up this.timers is there and didn’t make the copy and paste?

Yes, it is all there. I just didn’t want the boiler plate the comment.

That’s going to be a problem for rules that trigger on a Member of a Group and sets a separate timer for each Item. We might need to do something a little hacky to get it working. I don’t think we can ignore that use case entirely.

I need to find a way and will try to find one but currenty yet I have non yet. Will be part of the next release then.

The only comment I have is I generally prefer to use the items dict to get the state of Items rather than pulling the Item from the IR

ok, that should be a big deal to change. In any case that is a simple refactoring behind the scenes anyway.

Nothing is setting the timer to null in all cases.

I need to get back to that because I don’t understand it

This one is clear to me. When I cancel I don’t want to do anything with the timer anymore.

this.timers[‘richEvent’].cancel();
this.timers[‘richEvent’] = null;

I’m confused with setting the timer to null whenever I create it: Why would I set the timer to null if I may reschedule it later ? See Use case Number (4) below. If I would set the timer to null here it would break the code (null pointer) on the reschedule.

function flapping() {
  logger.info('flapping')
}
if ((this.timers['richEvent'] && this.timers['richEvent'].isActive()) == true) {
  flapping();
  this.timers['richEvent'].reschedule(zonedDateTime.now().plusMinutes(5))
}
if (this.timers['richEvent'] === null) {
	this.timers['richEvent'] = scriptExecution.createTimer(zonedDateTime.now().plusMinutes(5), function(){
       // no function called
       //but still reset timer to null ->  would break the reschedule.
  })
}

On the design though, I’d put the “cancel on retrigger” part below the statements.

Yes, I agree, I was just too quick and lazy when I did the image :wink:

If the timer names are now just string identifiers, I’m wondering why we would need a separate “timer MyTimer” for those

Technically true, let me think about it. Usually typed things are nicer but I agree that this is currently not checked yet.

Regarding this here

image

in changing it to “timer rich” - “is active”:

  • Would you expect to completely omit the logical comparison against true / false? I am even not sure if this works on the if clause. I need to check that because the if, I think, requires a logical comparison block.
  • How would I then ask if the timer is not active? “timer rich” - “is not active”? This would result into two blocks for each state (“is active” and “is not active”) which is something I wouldn’t like but I may misunderstand you.

Consider the following sequence of events.

Step conditions Comments
1 timer === null Initial state, timer is initialized to null
2 timer === null We skip the first if statement because the timer is null
3 timer === null We execute the second if statement because timer is null
4 timer == active We’ve created a timer. The function to call does not reset the timer to null. So far so good.
5 timer == active The rule triggers again before the timer goes off.
6 timer == active The first if statement runs because the timer exists and is active.
7 timer == active The second if statement is skipped because the timer is not null
8 timer == terminated The rule triggers again after the timer has gone off and run.
9 timer == terminated The first if statement doesn’t run because the timer exists but is not active.
10 timer == terminated The second if statement doesn’t run because the timer is not null.

8 through 10 will be what happens from now on. You’ll never get a chance to reschedule the timer because when the timer has terminated it’s no longer active. NOTE: when you cancel the timer it will no longer be active either.

However, if you set the timer to null when the timer has cancelled and when the timer expires then your code as written will work.

Step conditions Comments
1 timer === null Initial state, timer is initialized to null
2 timer === null We skip the first if statement because the timer is null
3 timer === null We execute the second if statement because timer is null
4 timer == active We’ve created a timer. The function to call does not reset the timer to null. So far so good.
5 timer == active The rule triggers again before the timer goes off.
6 timer == active The first if statement runs because the timer exists and is active.
7 timer == active The second if statement is skipped because the timer is not null.
8 timer == null The timer went off and the last thing done in the body of the timer function was reset the timer variable to null.
9 timer == null The rule triggers again after the timer has gone off and run.
10 timer == null The first if statement doesn’t run because the timer exists but is not active.
11 timer == null The second if statement doest run because the timer is null, causing a new timer to be created.

That’s the traditional way to use timers which you’ll find in all sorts of examples on the forum. Basically, once the timer expires we get rid of it and next time the timer will be recreated. But that was before we had the isActive method. Looking at the code above an alternative method could be to add a check for hasTerminated to that first if statement in addition to isActive.

I have a bit of a preference for the use of null though. Consider the following. Let’s say that the user pulls an Item from the IR and saves it to a variable. Their runme function depends on that variable. We’ve been running for days without ever recreating the timer. That variable they are using in “runme” will still be the reference pulled days ago. That Item Object will be stale and may no longer work. By recreating the timer it will pick up the most recent reference to that Item which should still be OK.

So just to be clear, the timer is reset back to null when ever it’s not active. That means when it’s has terminated (i.e. the last line of “runme” or immediately after “runme” is called) or when it’s cancelled. Next time around the timer will be recreated anew. Reschedule only happens if the old timer didn’t terminate.

Yes and yes it will work. Anytime you see foo == true or false, just using foo will suffice. So both in the block and in the code I wouldn’t expect to see the explicit comparison to true or false.

Hmmm, good point. Can you not put a not block in front of the isActive block? If not then we’d need to decide whether to compare it directly to true or false or add an is not active block. Adding a new block reads better like prose but might lead to a proliferation of new blocks.

If the block output is a Boolean type, then it can be inserted in a if block directly.
To test if a timer is not active, there’s a “not” block in Logic that can be inserted.

image

1 Like

Finally I think that this is a good idea. So it turns to be like that:

and

Each of the is/has-block have been typed to be a Boolean to link safely.

Regarding the inner timer block

image

I would like to keep it as it makes the fact that this is a timer more clear.
Btw, I tried to make this to really become a typed block like so

this.setOutput(true, ‘Timer’)

and use that in “setCheck” but I couldn’t get it to work unfortunately.

Regarding the simple timer

image

I was think to add a timer name as well but then the block would loolk exactly like “normal timer”

and couldn’t be distinguished. The alternative would be to add the retrigger option like so

image

What do you think?

I disagree, I think extracting the “timer” label and making clear the timer identifier is a string makes sense.
For instance if you were to compute timer identifiers with Text blocks like this:

image

If you didn’t have the “timer” prefix in the outer block itself, how would you know what actually is active?

Like so?

I edited my comment while you were answering. See also my comment on the simple timer above with the retrigger. Feedback is appreciated there as well.

2 Likes

These are looking quite good now :+1:

2 Likes

Looks good. Very usable I think. Users will be very happy to get access to timers like this! :smiley:

Thanks Rich, I haven’t worked on the null-topic above due to the comprehensive answer I need comprehend first.

Meanwhile, can you please check if this is the expected code (with null-setting) for the “simple”-timer?

var scriptExecution = Java.type("org.openhab.core.model.script.actions.ScriptExecution")
var zonedDateTime = Java.type("java.time.ZonedDateTime")
if (this.timers === undefined) {
	 this.timers =[]
}
if(this.timers['MyTimer'] === undefined){
	 this.timers['MyTimer'] = null
}
if (this.timers['MyTimer'] === null) {
	this.timers['MyTimer'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(10), function(){
		})
}else {
		this.timers['MyTimer'].reschedule(zonedDateTime.now().plusSeconds(10))
}

// left out initialization part
if (this.timers['MyTimer'] === null) {
	this.timers['MyTimer'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(10), function(){
		})

}else {
		this.timers['MyTimer'].cancel()
}

and

if (this.timers['MyTimer'] === null) {
	this.timers['MyTimer'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(10), function(){
		})

}else {
}

The last line of the function passed to createTimer needs to be

this.timers['MyTimer'] = null

Though in this case I think it will work just fin without it because even after the timer expires it will be rescheduled. However, there still might be an issue with the variables inside that function becoming stale over time. I think it is safer to recreate the timer after it’s expired rather than rescheduling a timer that has terminated who knows how long ago.

In addition, the line right after calling cancel() needs to be

this.timers['MyTimer'] = null

also because the function passed to the timer never had a chance to run and set it to null itself. Without that, a new timer will never be created because `this.timers[‘MyTimer’] will never be null.

For the last one, again the last line of the function passed to the timer needs to set the timer variable to null. Then remove the else entirely as it’s not needed (unless it’s easier to keep it there as a noop.

Sorry I missed this, yes you can of course.

like so

if (this.timers['MyTimer'] === null) {
	this.timers['MyTimer'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(10), function(){
	  logger.info('abc')
	  this.timers['MyTimer'] = null
	})

}else {
	this.timers['MyTimer'] = scriptExecution.reschedule(zonedDateTime.now().plusSeconds(10))
}

I think it is safer to recreate the timer after it’s expired rather than rescheduling a timer that has terminated who knows how long ago.

Not sure what I should do in this case? do a timer.create in the else branch? The the else branch would become equal to the if branch which wouldn’t make sense

In addition, the line right after calling cancel() needs to be

like so?

if (this.timers['MyTimer'] === null) {
	this.timers['MyTimer'] = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(10), function(){
	  logger.info('abc')
	  this.timers['MyTimer'] = null
	})

}else {
		this.timers['MyTimer'].cancel()
		this.timers['MyTimer'] = null
}

If that’s okay, I need to do thorough and comprehensive testing tomorrow to make sure all this works as expected (only did the coding so far for the changes)

Unfortunately it doesn’t behave as expected