Extending Blockly with new openHAB commands

stefan.hoehn · December 10, 2021, 7:22pm

Unfortunately I need to come back to the event-Block because if want to return anything back different from String then we need to think about it more in depth and we should not mix that up with the comparison thing above because that would it make even harder.

        ['newState', 'itemState'],            // Change and Updated Event -> State => String or State-Type?
        ['previousState', 'oldItemState'],    // Change and Updated Event -> State =>  String or State-Type?
        ['triggering item name', 'itemName'], // all -> String
        ['type', 'type']]),                   // all -> String
        ['receivedCommand', 'itemCommand'],   // Command -> has attributes but hard to handle the different attributes per type => String
        ['channel', 'channel'],               // ChannelTriggeredEvent  -> has attributes but hard to handle the different attributes per type => String

Look at the comments above

State
The first two return a State-Object which in OH is basically a type. A type has several subclasses all of which have different methods. Most methods of those types should not be of any interest (like OnOff, Decimal…) as the only thing you would probably want is the value anyway. HSBType does have getters for RGB but that is pretty specific. A State-Block would need to have a “free-attribute” name or you would have to provide the State-Type and from there it could “intelligently” tell which attribute would be available as an attribute.

Note that these can be undefined in case it is CommandEvent.

So in summary at least for the time being I would not implement a Blockly-State-Block which would provide a state back but just return String and name that field “state value”. Later we could provide the state as well that would feed into a block that takes that state and with choosing the expected type it would allow you to retrieve specific attributes for that block (we could also make that an individual block as it returns a different type State then)

Agreed?

Type and Name
Are just Strings. Type actually provides the “short name” of the types class

Channel
is only available when the channel was triggered
This is actually a Subtype of Event which has the name attribute and a type which we already have. As the only interesting information is the channel name, I would not give back the channel object but again a String that contains the channel name only.

Agreed?

Event
event.event doesn’t exist actually. We could return “event.” in the code and something (any attribute or method) could be added as a free string but if there was anything we would need that is not covered above in any of the fields for one of the event’s subtypes we should rather explicitly provide them as an attribute in this block here like the channel.

Agreed?

Hopefully you say “Yes” to all because then I would be able finish that topic tonight and can provide a PR by tomorrow which should not be too big for Yannick to review and merge soon (it contains some very minor fixes and additions).

PS: The comparison topic will be only tackled next year (or maybe over the winter holidays). I really want to put some major thoughts into that before doing it.

rlkoshak · December 10, 2021, 7:55pm

I’d call it “state string” to be clear that we a reworking with Strings.

My only concern is you can’t do number comparisons and math with a String and that’s a pretty big need.

If I can’t do something like event.itemState > 20 or event.oldItemState > event.itemState that’s a pretty big limitation.

Agreed.

event.event exists sometimes when a rule is triggered by an event Channel and it contains information about the event as a String. We can’t skip this but you don’t need to do anything special about it. Just return it as it is and specify it as a String.

stefan.hoehn · December 10, 2021, 8:04pm

Oh sure, I think that should work because as mentioned further above Javascript is rather resilient to this. If you compare a string with a number (like I described in my ‘own bug’ that I files about that) that comparing is working as JS coverts the string into a number… but to be very sure in this case I do a test case after the implementation.

rlkoshak · December 10, 2021, 8:09pm

One thing to be careful of though is that probably won’t work in JSScripting or when using strict.

I’ve been spending a lot of time working with JSScripting and that’s one place where I ran into trouble. We will have to get Blockly to support JSScripting at some point so if we can avoid extra pain later it’s worth at least consideration.

For the short term though if it works in Nashron as Blockly is generating it then that’s fine by me.

stefan.hoehn · December 10, 2021, 9:58pm

Yep, then this is a general program not only at this place. We will have a look as soon as we work on GraalVM. I am sure that Yannick has that somewhat on the roadmap.

Back to work - the PR will come soon @ysc

rlkoshak · December 10, 2021, 10:41pm

I think he already has a PR started and I posted a couple tutorials. It’s not too bad really making something work and I learned something here that I didn’t know about which might make it even better (namely event.type) in some cases.

I started a tutorial over at Migrate from Nashorn to JSScripting for UI Rules. To start I would think Blocky would use the Object.require approach so the differences between Nashorn and JSScripting are minimal. Long therm though I think a number of things that will exist in the Helper Library could make the Blocky implementation easier. I think the biggest gotcha is Thread.sleep breaks and there is as of yet no alternative. That and the String issue are the two big gotchas.

For example, what if the library provided a method to compare two States or compare a State to anything and as long as they are compatible it will give the right answer? That should make your job with the comparison stuff easier I would think.

stefan.hoehn · December 10, 2021, 10:53pm

Will the helper library be part of openhab by default?

Not having sleep is really a big deal breaker. I am sure a lot of people are using it. The problem is that I just quickly researched that topic that this was actually a design session not to have multi-Threading.

rlkoshak · December 10, 2021, 10:57pm

Yep. It’s embedded in the add-on now.

Yes but the usual work around is to do a Promise and await but that only works when you have a loop which normal JS in the browser and Node have but seems to be lacking in GraalVM JavaScript. So you make the promise and try to await and the rule just exits.

We might have to do something ugly in the library like a busy while loop. It’ll be super ugly though because we can’t even yield or nice the loop so it will spike the CPU while “sleeping”.

stefan.hoehn · December 10, 2021, 10:57pm

Here are the final blocks and I can confirm that the comparison works (I used a number = Decimal Type state)

The blocks also have tooltips that tell the user with which events the blocks to use, e.g.

stefan.hoehn · December 10, 2021, 11:33pm

@ysc here is the PR to be reviewed and merged.

TIA
Stefan

stefan.hoehn · December 11, 2021, 7:39pm

Hi Rich,

I need your feedback. We have an issue in the timer handling and there is a bug to it: [BLOCKY] reschedule block triggers only once · Issue #1214 · openhab/openhab-webui · GitHub

It has to do with your proposal to set the timers to undefined. Either I misunderstood something and implemented it wrong or we have an issue that has to be fixed.

See the following

The intention is obvious: it should start after 5 seconds and retrigger over and over again but it doesn’t … and I will explain why…

First let’s make this simpler first by taking out the manual retrigger in the timer like so:

which generates the following code:

1 if (typeof this.timers['MyTimer'] === 'undefined') {
2  this.timers['MyTimer'] = scriptExecution.createTimer(zdt.now().plusSeconds(5), function () {
3   logger.error('timer triggered');
4   this.timers['MyTimer'] = undefined;  // see THIS HERE
5  })
6 } else {
7 this.timers['MyTimer'].reschedule(zonedDateTime.now().plusSeconds(5));
8 }

As far as I remember you wanted to reset (=undefined) the timers after having used it, so I implemented it to appear in line 4.

The problem is if I put back the manual reschedule

it is added before line 4 from above (becoming line 7) and then the whole thing doesn’t work anymore because upon entry in line 4 it is undefined and then line 5 won’t be executed (which is good because it would result into an NPE anyway)

1 if (typeof this.timers['MyTimer'] === 'undefined') {
2 this.timers['MyTimer'] = scriptExecution.createTimer(zdt.now().plusSeconds(5), function () {
3    logger.error('timer triggered');
4    if (typeof this.timers['MyTimer'] !== 'undefined') {
5       this.timers['MyTimer'].reschedule(zdt.now().plusSeconds(5));
6    }
7    this.timers['MyTimer'] = undefined;
8  })
9 } else {
10  this.timers['MyTimer'].reschedule(zonedDateTime.now().plusSeconds(5));
11}

So the only way to fix this is to remove

this.timers[‘MyTimer’] = undefined;
from the generated code but I remember you wanted to have this for a good reason…

rlkoshak · December 13, 2021, 3:03pm

It needs to be an if/else.

if (typeof this.timers['MyTimer'] === 'undefined') {
this.timers['MyTimer'] = scriptExecution.createTimer(zdt.now().plusSeconds(5), function () {
   logger.error('timer triggered');
   if (typeof this.timers['MyTimer'] !== 'undefined') {
      this.timers['MyTimer'].reschedule(zdt.now().plusSeconds(5));
   }
   else {
     this.timers['MyTimer'] = undefined;
   }
 })
} else {
  this.timers['MyTimer'].reschedule(zonedDateTime.now().plusSeconds(5));
}

If one is going to create a looping timer, there really needs to be additional criteria to determine whether or not to reschedule or not or else the timer will loop forever. The user must define some other test (e.g. an Item reaches a given state, a certain number of loops have been performed, a certain amount of time has passed, etc.).

A looping timer is a pretty advanced need so I don’t know how much we need to spend on supporting them right now. The more I think about it, it’s probably going to require a whole new block to generate that if/else construct. There really is no way to code a looping timer without that sort of construct.

Removing the setting of the timer back to undefined breaks a bunch of other more common timer use cases.

stefan.hoehn · December 13, 2021, 9:45pm

rlkoshak:

1 if (typeof this.timers['MyTimer'] !== 'undefined') {
2      this.timers['MyTimer'].reschedule(zdt.now().plusSeconds(5));
3   }
4  else {
5     this.timers['MyTimer'] = undefined;
6   }

While thinking about finding a solution I wonder of that makes sense because isn’t “this.timers[‘MyTimer’]” in line 6 undefined already because it is the else branch of !== ‘undefined’?
So why would I set it to undefined if is already undefined?

I think the right thinking is that want to suppress the setting to undefined only in case I reschedule a timer?

rlkoshak · December 14, 2021, 2:32am

Here is a less theoretical example of a looping timer.

var count = 5;
var this.timer['MyTimer'] = ScriptExectution.createTimer(zdt.now().plusSeconds(5), function() {
    if(count > 0) {
        count = count - 1;
        this.timers['MyTimer'].reschedule(zdt.now().plusSeconds(5));
    }
    else {
        this.timers['MyTimer'] = undefined;
    }
});

Compare that to your code. You’ve missed the whole inside the Timer function part of it all.

But now you have to consider the case that the Timer exists and is counting down when the rule is triggered again. So we add:

if(this.timer['MyTimer'] === undefined) {
  var count = 5;
  var this.timer['MyTimer'] = ScriptExectution.createTimer(zdt.now().plusSeconds(5), function() {
    if(count > 0) {
      count = count - 1;
      this.timers['MyTimer'].reschedule(zdt.now().plusSeconds(5));
    }
    else {
        this.timers['MyTimer'] = undefined;
    }
  })
}
// else do nothing

If the timer doesn’t exist (i.e. is undefined) when the rule runs we initialize a count variable. We create a looping timer. *Inside the function that the timer runs we check to see if it’s run five times. If not, we decrement the count and reschedule the timer. If it has we set the timer to undefined and are done.

If the timer does exist when the rule runs we do nothing. Of course, if the timer did exist you could cancel the old one and start a new one. That’s a valid use case to.

But the key point is inside the body of the Timer you have two choices: reschedule or exit. The User needs to define under what circumstances the timer is rescheduled (or conversely when the timer should stop looping).

It’s not as simple as not setting the timer to undefined when looping and setting it to undefined all other times.

NOTE: the count variable is just an example. A user may want to wait for an Item to reach a certain state, or a certain amount of time to have passed, or some other criteria.

stefan.hoehn · December 15, 2021, 7:07am

Hi Yannick,

I only today stumbled over something that really took me quite a while to find out why it didn’t work and I wonder how we can improve this.

I took the following block

and then I used the cog to add a value like so

and ended up having this

then I tried to attach a value like so

but it wouldn’t connect. I even looked into the code and didn’t find a setCheck that validates the block type. There isn’t any. The problem is completely different and completely unobvious.

You must not use the default dictionary block (the shadow block, that is) that comes with the run script

Instead you have to bring in a new dictionary block (which by the way has a darker color than the shadow_block)

and then everything works as expected

Do you have any idea what the root case is that the shadow block doesn’t behave like the real one and what we can do to guide the user more clearly the s/he must add a real dictionary first before it will work?

At least we should make this clearer via the tooltip, I think.

ysc · December 15, 2021, 8:03am

Yes I noticed too but don’t have an explanation. Maybe blocks with mutations simply cannot be put as shadow blocks, or maybe it’s possible to hide the mutation “cog” button so the shadow block is an empty dictionary that you can’t change unless by dragging a real block. Otherwise I’m not hostile to the idea of making it a real block, it’s contrary to the “norm” but at least it would work.

What about something like

if (typeof this.timers['MyTimer'] === 'undefined' || this.timers['MyTimers'].hasTerminated()) {
...

I suspect the rationale was also to avoid keeping references to terminated timer instances indefinitely in memory causing them to never be GC’ed, but I’m not sure either setting them to undefined would do that.
And there’s also the logic flaw that the “timer has terminated” block

which generates code like

typeof this.timers['MyTimer'] !== 'undefined' && this.timers['MyTimer'].hasTerminated();

is pretty much useless: it would never return true - either the timer is not set, or pending, or running, or has effectively terminated and thus unset again.

rlkoshak · December 15, 2021, 3:17pm

I’m not sure either. One would have to look at the scheduler itself to see how well it cleans up after itself.

That would certainly be a little more fool proof I think. Indeed the the timers would never be GC’d but I can’t imagine one would have so many stale timers hanging about where that would make a difference. My assumption is that the scheduler that handles the timers would be smart enough to clean up terminated timers so it should only be the reference in the rule that would keep it from being GC’d. But that’s an assumption and not actual knowledge.

Changing to a check like that would let us eliminate the setting of the timer back to undefined automatically at the minor expense of some extra memory usage.

stefan.hoehn · December 15, 2021, 3:45pm

So for this

the result would become

// Change LINE 1
1 if (typeof this.timers['MyTimer'] !== 'undefined' && this.timers['MyTimer'].hasTerminated()) {
2 this.timers['MyTimer'] = scriptExecution.createTimer(zdt.now().plusSeconds(5), function () {
3    logger.error('timer triggered');
4    if (typeof this.timers['MyTimer'] !== 'undefined') {
5       this.timers['MyTimer'].reschedule(zdt.now().plusSeconds(5));
6    }
7    // this.timers['MyTimer'] = undefined; <= take out this line
8  })
9 } else {
10  this.timers['MyTimer'].reschedule(zonedDateTime.now().plusSeconds(5));
11}

or simplified

the code would be as follows

// change LINE 1
1 if (typeof this.timers['MyTimer'] !== 'undefined' && this.timers['MyTimer'].hasTerminated()) {
2  this.timers['MyTimer'] = scriptExecution.createTimer(zdt.now().plusSeconds(5), function () {
3   logger.error('timer triggered');
4   // this.timers['MyTimer'] = undefined;  // Take this out 
5  })
6 } else {
7 this.timers['MyTimer'].reschedule(zonedDateTime.now().plusSeconds(5));
8 }

Note that I haven’t tried that out yet (but I sure will as soon as you confirm)

rlkoshak · December 15, 2021, 4:15pm

I think those will work. Once the timer is created there will always be that Timer. But I think that is probably OK.

The looping timer is a little contrived and would like to see any docs or examples include an if statement to determine whether or not to reschedule inside the timer. That is a more typical use of a looping timer instead of a timer that loops forever. If someone wants that, a cron triggered rule would be a better choice.

ysc · December 15, 2021, 4:19pm

No, if the timer is undefined or has terminated - the first code fence in my post above. The above condition will never be satisfied since the timer is defined just below in the if branch.

Apart from that this seems reasonable. We would keep timers indefinitely in this.timers, and the create timer would simply redefine them after they are terminated. The reference to the old timer would be lost and maybe it would be GC’d eventually.
A minor optimization would be to simply reschedule them if they still exist but are terminated (see the remark in Timer (openHAB Core 3.2.0-SNAPSHOT API) - “This can also be called after a timer has terminated, which will result in another execution of the same code.”), but it would lead to more code we can do without.