Extending Blockly with new openHAB commands

Please also take note of the ongoing discussion at https://github.com/openhab/openhab-core/issues/2433 regarding the fact that we have two JS scripting engines available, and their API to access OH objects are currently incompatible with one another.

(I for one would advocate for the GraalVM JS add-on to be standardized as the default JS script engine, since Nashorn is due to be removed anyways, and it would be foolish to keep capitalizing on it, especially if it would prevent from upgrading supported JVM versions.)

But until we reach a decisive conclusion on that matter it will remain a major hurdle for generating dependable JS code from Blockly blocks.

2 Likes

Ok, I currently test it against the rules that says ECMAscript in the UI. Not sure what I can do more, though I will read the thread thoroughly.

If we generate scripts that are incompatible, the worst thing would be that if we update the engine, we also change the code generation at the same time and then the would only habe to opened once to regenerate the code basically (not really optimal but at least a solution)

Concerning 2, I’m actually not too concerned about regressions. Tests absolutely need to be performed. However, if a block hasn’t changed there should be no regressions. An unchanged block will produce the exact same code every time.

Concerning 3, I think that’s mostly organizational and how it appears in the UI. The key place where it could impact existing code is again if an existing block generates different code than before.

Concerning 5, I’m not certain that I should be the sole arbiter on what makes “good” code. I definitely have opinions but some of them can be controversial. I do have some ideas on what makes “clean” rules code but what makes for clean text code and what makes for a clean Blockly code may not be the same. We definitely should focus more on making Blockly as easy to use as possible. As long as it generates valid JS code behind the scenes I think it’s OK if that JS code is a little messy.

A few comments on the blocks:

  • Groups: Network Tools isn’t very descriptive, as a user I’d find it hard to guess what’s in that category.

  • Variables: Is there a way to set a flag on a regular old variable block to determine whether it gets recreated/reinitialized every time the rule runs or if it’s previous state is preserved? I fear about the proliferation of blocks.

  • Variables: What’s the difference between getting any old variable and a Persist variable? Maybe we don’t need that block at all?

  • Things: What the difference between the purple MyItem and the red get item item MyItem? Is this the Items category?

  • Network Tools: OK, rename this category to something else, like Core openHAB Actions or something like that. executeCommandLine isn’t a Network Tool. I’d never guess to look there for it. Maybe create a separate category for exec and add the sendHttpXRequest Actions to Network Tools would be another option.

  • Timers: I like the Timers blocks. They are very intuitive looking. Does the “Timer Name” produce a persisted variable? If not it should.

  • Notifications: It’s not clear what the difference is between the three.

  • Ephemeris: I assume it only supports the default day types of weekend, weekday, and holiday. That’s reasonable.

  • Persistence: Are the store and restore blocks working with the storeStates and restoreStates core Actions? If so they really are not Persistence actions. I expect putting them in this category would cause confusion. Based on the name of the category and the name of the blocks I’d expect the persistence database to be involved.

  • Persistence: At some point it would be nice to be able to select the persistence to select from.

  • Persistence: We are missing most of the rest of the persistence actions like historicState, sumSince, etc.

What ever makes it easier for you to implement. Unless we change blocks that are already implemented and in use the risk of breaking changes is pretty low. When the changes get merged users will get a bunch of new options, but their old Blockly stuff should work unchanged.

Also, @ysc, correct me if I’m wrong, but even if we do change the existing blocks, it won’t go and change their already existing rules. It will only apply for new rules created with that block, or modifications to rules that use that block.

So I think the risk for disruption is relatively low over all.

I don’t think so.

The whole rules documentation needs to be reorganized in my opinion. It’s horribly incomplete, only mentions the UI rules rarely, and it doesn’t really cover everything that is needed.

@Confectrician, as the docs master, what do you think? Does it make sense to add comprehensive Blockly (and eventually the other languages) to the Rules page, or should we break it up a bit sort of like the UI section? We can then have one page for Rules DSL, one for UI rules creation and usage, and maybe a separate reference page where each of the blocks are documented.

Another alternative is to have just the one Rules doc page and use the tabbed examples trick to choose and show the example for the language of choice. Since all the extra languages are now separate add-ons, I’d expect the docs for those languages to live under the add-ons docs so we’d only need to cover the OH defaults: Rules DSL, Blockly, and JavaScript.

I see good arguments for either approach. But in the mean time, @stefan.hoehn, I think just getting content written might be the primary goal. A lot of the docs we’ve written by creating a Wiki posting here on the forum to get the community involved in helping writing the docs (it doesn’t work but we try). Then we transfer those to the official docs. Since both use Markdown it’s pretty easy to transfer.

That approach might let us get the docs going before needing to make a decision on where they ultimately need to live in the official docs.

I’ve still got to write the rules part of the getting started tutorial too and this can give us a change so address both.

I’m happy to help. But I’m usually away from computers in general over the weekends. So Friday evening through Monday morning I usually won’t reply. But I try to catch up on everything I missed. If I do miss something ping me and I’ll see it.

Seconded. Though that might impact more of the existing blocks a bit more than I’ve discussed above so regression testing and clearly documenting breaking changes we can’t get around will be a priority. Also, we need a way for users with preexisting Blockly rules to regenerate their code from their blocks to use GraalVM. I’m not sure how hard or easy that might be.

Yes - the Blockly script is described in XML format in a hidden configuration property of the script module (blockSource) and the code is regenerated when the blocks change.
Just confirmed it is also regenerated when you save the rule:

so even if you don’t change anything, opening the script and hitting Ctrl-S will make sure the script’s code is current (so while it gives a little wiggle room to change the generated code even after a block is used, currently the user will have to do that for every script- that’s not ideal.)

If you change the “signature” of a block, however (like adding an input), then what will happen is undetermined, if it’s a critical & incompatible change probably an error during generation will occur, so the block would have to be removed and added again.

1 Like

Just for you to know, I had quite a long conversation in the background with @Andrew_Rowe who has volunteered to step into documenting the blocks that I will produce the next week’s.

Thanks Andrew!

PS: The multimedia blocks are already done and tested and work well already. So the first drop will be soon available on my repo soon.

2 Likes

I have a question

I stumbled over the following print-block:

which generates

print(‘abc’)

  • The help tooltip says “Print a message on the console”. Which console is meant here?
  • What is it meant to be used for?
  • As the rules are run on the server side, wouldn’t the log-block enough or do I miss something?

Hi Rich,

I am currently testing the “ephemeris” blocks. Basically they are working but I get the following warnings:

2021-10-14 22:03:01.303 [WARN ] [de.jollyday.util.ClassLoadingUtil   ] - Could not load class with current threads context classloader. Using default. Reason: ClassNotFoundException: de.jollyday.impl.DefaultHolidayManager cannot be found by org.apache.aries.jax.rs.whiteboard_2.0.0
2021-10-14 22:03:01.326 [WARN ] [de.jollyday.util.ClassLoadingUtil   ] - Could not load class with current threads context classloader. Using default. Reason: ClassNotFoundException: de.jollyday.datasource.impl.XmlFileDataSource cannot be found by org.apache.aries.jax.rs.whiteboard_2.0.0
2021-10-14 22:03:01.494 [WARN ] [de.jollyday.util.XMLUtil            ] - Could not create JAXB context using the current threads context classloader. Falling back to ObjectFactory class classloader.
2021-10-14 22:03:01.957 [WARN ] [de.jollyday.util.ClassLoadingUtil   ] - Could not load class with current threads context classloader. Using default. Reason: ClassNotFoundException: de.jollyday.parser.impl.ChristianHolidayParser cannot be found by org.apache.aries.jax.rs.whiteboard_2.0.0
2021-10-14 22:03:01.972 [WARN ] [de.jollyday.util.ClassLoadingUtil   ] - Could not load class with current threads context classloader. Using default. Reason: ClassNotFoundException: de.jollyday.parser.impl.FixedParser cannot be found by org.apache.aries.jax.rs.whiteboard_2.0.0

This is the code that is generated

var logger = Java.type('org.slf4j.LoggerFactory').getLogger('org.openhab.rule.' + ctx.ruleUID);

var Ephemeris = Java.type("org.openhab.core.model.script.actions.Ephemeris");

logger.info((Ephemeris.isWeekend()));
logger.info((Ephemeris.getBankHolidayName()));
logger.info((Ephemeris.isWeekend()));
logger.info((Ephemeris.getNextBankHoliday()));
logger.info((Ephemeris.isBankHoliday()));

I know you are pretty familiar with the ephemeris and I read your pattern page and many other threads regarding this but don’t really understand what the really cause of that warning is because I configured everything in the settings of OH3, so I don’t think I need to provide a config file which is sometimes mentioned in some of the threads. Also some threads say they have an old binding but I don’t think with OH3 I need to install a binding as it is already provided, isn’t I?

170 │ Active │ 80 │ 3.2.0.M2 │ openHAB Core :: Bundles :: Ephemeris

And can I just ignore the warning it for the development of the blocks? (though I would love to know what has to be done, so it can be documented alongside with how to use the blocks in a correct way).

TIA
Stefan

Thanks for all the comments on the several groups. I won’t get into most of these as I have postponed working on these (like Persistence, Network Tools and Variables…) but regarding these here

The only usage I could imagine is to use the this in a log statement at the moment.

This is the code being generated

logger.info('MyItem');
logger.info(itemRegistry.getItem('MyItem'));

Hmmm, I’m not seeing the same warnings in my code. Which OH are you running? Maybe this is a newly introduced regression? The warnings are coming from OH core so it doesn’t really have anything to do with the rules code.

By any chance does it work despite the warnings?

I don’t think this has anything to do with anything you as a user has control over. It’s a missconfiguration or some other error internal to OH. The warning is saying that it cannot find the third party library that backs OH’s Ephemeris implementation, JollyDay.

An issue may need to be filed, particularly if you are on the latest milestone or snapshots. If you are running OH built from source, there may have been something that went wrong in the build.

Yes, it’s part of core and there by default.

If the rule is actually working correctly then yes, you can ignore it for now though I’d still file an issue.

In JS rules code, most of the time you are not actually working with the actual Item but just the name of the Item. That appears to be what item(MyItem) does. I makes sense that it’s a separate block from just a plain old String block as you can get a drop down list of all your Items to populate it rather than requiring the user to remember and type Item names in.

The get item block though is useful and required in cases where one needs more than just the Item name. For example, the Item’s label, a Group’s members, tags, etc. I would expect to see some block chains along the lines of

for each -> get members of group -> get item -> item MyGroup
log(info) > get item label -> get item -> item MyGroup

Though it might make sense to collapse those and the get item remains understood rather than requiring the users to define it.

Ok, understood.

  • I won’t care too much about the warning then as I like to concentrate getting the actual blocks done
  • The pseudo script at the end gives me the idea that we should eventually have a block that can retrieve several items either of a group or by a filter that allows iteration over it in blockly. Probably an advanced topic.

@Andrew_Rowe @rlkoshak @ysh

Here is the first drop of the new functionality. Note that I have taken out quite a lot to concentrate on a first delivery that can be easily tested and documented.
While you are testing it and start the documentation I would then continue working on it.

@ysc @kai If I provided the PR would you have time to review it? Does it make sense to provide several PRs so that it doesn’t become to big to be reviewed (like the original one)?

@Andrew_Rowe Would you mind downloading it and start documenting? I could provide tips for example how setup sound and voice via private message.

Here is what is part of the drop

New Openhab Menu
image

Items and Things

Timers and Delays
I have taken out timers for the time being as code generation is broken but it will be next thing to provide soon (which is why the label contains “timers” as well.
image

Voice / Multimedia

Logging / Output
as I mentioned above, it is not clear to me what the print() command actually does, so I couldn’t test it.
image

Note that I added help urls (hopefully everywhere) that are linked to openhab documentation pages and tooltips to all blocks.

The code has been pushed to https://github.com/stefan-hoehn/openhab-webui/tree/blockly_enhancements_1

If you are happy with the content above I am happy to do a Pull Request on the main repo.

To run the UI locally against your OH3-installation and test the functionality

  • Download the repo
  • go to directory openhab-webui/bundles/org.openhab.ui/web
  • npm install
  • OH_APIBASE=http://192.168.111.222:8080/ npm start

where 192.168.111.222 is the IP-address of you OH3-installation

I am looking forward to hearing some feedback - happy testing and documenting
Stefan

PS: In the meantime I go on with the timers section

2 Likes

Hi Rich,

I am on the timers now. Can you check if that following blocks’

generated code of that complex example

var initialNoofRepeats, noofRepeats;

var scriptExecution = Java.type("org.openhab.core.model.script.actions.ScriptExecution");

this.MyTimer=(this.MyTimer!=null) ? this.MyTimer : null

var logger = Java.type('org.slf4j.LoggerFactory').getLogger('org.openhab.rule.' + ctx.ruleUID);

var zonedDateTime = Java.type("java.time.ZonedDateTime");


initialNoofRepeats = 3;
noofRepeats = initialNoofRepeats;
if (((this.MyTimer !== null) ? this.MyTimer.isActive() : false) == false) {
  logger.info('Starting MyTimer');
  if (this.MyTimer == null) {
  	this.MyTimer = scriptExecution.createTimer(zonedDateTime.now().plusSeconds(10), function(){
  	  logger.info('Timer block has been executed');
      if (noofRepeats > 0) {
        logger.info('Retriggering timer');
        this.MyTimer.reschedule(zonedDateTime.now().plusSeconds(5))
        noofRepeats = (typeof noofRepeats == 'number' ? noofRepeats : 0) + -1;
        logger.info(noofRepeats);
      } else {
        logger.info(('Canceling timer after initial number of repeats = ' + String(initialNoofRepeats)));
        this.MyTimer.cancel()
      }
    });
   } else {  		
   this.MyTimer.reschedule(zonedDateTime.now().plusSeconds(10));
  }
} else if (((this.MyTimer !== null) ? this.MyTimer.isActive() : false) == true) {
  logger.info('Stopping MyTimer');
  this.MyTimer.cancel()
}

is the code you would expect?

What I can say is that behaves at least the way I would expect it to do:

  • Start the execution after 10 secs
  • logs “Timer block has been executed” out
  • then repeats that for configurable number of times
  • finally cancels the timer
  • if the time is running and the rule is triggered again, it is canceled (uses isActive())

Open the PR. As always if it’s ready it will be reviewed & merged in due course.

Note that I will likely insist on it working on both Nashorn & GraalVM. This depends on the discussion going on at GraalVM JavaScript replaces Nashorn, breaking Blockly and existing Nashorn rules · Issue #2433 · openhab/openhab-core · GitHub on how to provide objects that are now available globally and taken for granted, like ir, events and so on. The consensus over there seems to be that ultimately they will be available under a openhab object (implicitely available in the Nashorn script engine and manually imported in GraalVM’s case, but the UI will prepend code generated from Blockly to make sure of that).

I actually don’t understand what I have to do to make compatible for both as I honestly don’t understand from this issue what the details are here and how to deal with that.
If you can tell me now, I can try to do that. Otherwise I would propose to move forward as it is to get this new blockly functionality out quickly and then later, when we know what to do, I port the code to become compatible with both N & G. I wouldn’t like to wait until this discussion has been finally finished or do you see that happening soon?

This can be discussed over at GitHub PR once I see the code :slight_smile:

1 Like

Fair enough - well said: Here is the PR → https://github.com/openhab/openhab-webui/pull/1179

I am already working on timers which I will probably drop during the next week as I am progressing well at the moment.

Update 18th of Oct: Timers have been fully implemented

2 Likes

Hi Yannick, or anyone else who is pretty familiar with GIT

I think my github repo got into a wrong state. I am struggling to get it right. Can you have a look here: https://github.com/openhab/openhab-webui/pull/1179#issuecomment-945430821

On the topic of Nashorn / GraalVM
Just as sidenote and reference, here is a good guide for Migration from Nashorn to GraalVM: GraalVM

  • Which one are we currently running. Is it still Nashorn at the moment?

Looking at the blocks and the code generated I have a few comments.

  • One thing that I’ve never really liked about how timers work in Rules DSL is how the lambda that gets passed to the Timer to run is defined inline. There are reason why one would want to do that given how Rules DSL works. But looking at the blocks here I wonder if it makes sense to follow that. Is there a way for the Run This block to present a list of the locally defined functions? Then the body of the Timer could be defined in a separate block which provides a bit of encapsulation which might make the code a little easier to read and maintain.

    Definitely keep the option to define the code inline too. I just want to also have the option to define the code in a separate function also.

  • I don’t think the third line will work where MyTimer is defined, though apparently it’s working for you. When I first started messing with it though that exact construction wouldn’t work for me. The very first time the rule runs, this.MyTimer won’t even exist. When a variable doesn’t exist its undefined, not null. I think the test needs to be this.MyTimer = (this.MyTimer === undefined) ? null : this.MyTimer; That means if the variable doesn’t exist, set it to null, otherwise leave it alone.

  • When comparing to undefined or null use the identity operator (=== or !==).

  • In JavaScript null and undefined are considered falsy. So the test to see if the Timer is active could be reduced to

    if( !this.MyTimer || !this.MyTimer.isActive() ) {

    The or operator fails fast. As soon as one of the operands evaluates to true it returns true. So if this.MyTimer is null it will be treated as false and the negate operator ! will convert that to true. Only if the Timer exists will isActive() be called. I don’t know if there is a Blockly reason why the current more complex construction is used here. If possible though it would be good to condense the code when possible.

  • Be consistent in the use of ending semicolons. There are several lines that are missing them.

Everything else looks reasonable to me.

re 1: Regarding the anonymous block execution block: I guess the challenge would be here that the code that would have to be generated had to generate a function that had to named uniquely and not only that had to be put >outside< the code. This is probably really tough in blockly (technically the inner part block set is given to the outer part and >I< have to compose them together. The other option though is to define a function and then call that function within that block which works out of box and looks much cleaner (thanks for the idea - I wouldn’t have had this idea):

re 2: ok, I’ll change it to add “undefined” but I thing we need to say: “if it is different from null or undefined, keep it, otherwise set it to null”, i.e. the boolean inversion then is

this.MyTimer = ( this.MyTimer === undefined || this.myTimer ===null) ? null : this.MyTimer;

re 3: poor ol’ Java guys keep forgetting about the triple = … I will amend this

re 4:
That’s neat - the reason is me not blockly :wink: → So this

((this.MyTimer !== null) ? this.MyTimer.isActive() : false) == false

becomes this

( !this.MyTimer || !this.MyTimer.isActive() )

re 5: semicolons → thanks for noting that. I already had cleaned up some of them (and also change string concatenation to js string interpolation in the code) but I now did again a thorough screening of the code. I am in favour of eliminating them but note that some of the code is standard blockly (like the one with the string concatenation or variable assignment) where I cannot change that.

So, I have changed that now (Luckily I have a few days off, so I can work on that :slight_smile: ).

sendNotification
While I have you on the line: Do you happen to know how can use “sendNotification” in javascript?

  • I cannot use event.sendNotification because “event” is not available automatically. Maybe we can add it?
  • org.openhab.io.openhabcloud.NotificationAction as a class as mentioned in some thread doesn’t work either.

I already tried to find the DSL-Code but don’t find the source for that. Maybe someone can point me to that too?

This meets my intent just fine. It’s very easy to get lost in a huge chunk of blocks and this gives a nice way to separate them out.

By any chance, does it work with arguments?

If it’s different from null we absolutely fo not want to set it to null. If we do that we will lose the handle on the Timer created the last time the script was run so we won’t be able to reschedule or cancel.

On-the-other-hand, if it’s already null (which is what your code says counter to the sentence) why reset it to null?

Maybe we need to step back a moment on what this line is supposed to do.

this.myTimer = means create a variable named “this.myTimer” and set it to the result of the expression following the “=”, or if the variable does exist, set it to the result of the expression following the “=”.

No matter what, this.myTimer will exist as a variable after this line of code. Therefore only the very first time the rule runs after it’s loaded will `this.myTimer === undefined" be true.

Now the question is what does the expression evaluate to? I propose

( this.myTimer === undefined) ? null : this.myTimer

The very first time the script runs this.myTimer gets initialized to null. From that point on, this.myTimer gets set to what ever it is already assigned to. If it’s null, it will remain null. If it’s pointing to an active or even an expired timer, it will remain pointing to that same timer.

At a high level the expression is saying: if the variable doesn’t exist, initialize it to null, otherwise do nothing.

When I first started learning JavaScript many years ago I was told there was some reason why they should be used. It was presented as a best practice to keep them. I’ve since seen the contrary said so I don’t know any more.

My recommendation would be to do a quick bit of searching (or wait until tomorrow when I’ll have a chance) to find a JavaScript coding standards and follow what it recommends. Keep in mind that Nashorn is ECMAScript 5.1 and GraalVM is ECMAScript 2021 (I think) so take note in case there are different standards for the two and choose the newer standard where feasible.

In Nashorn it would be:

var NotificationAction = Java.type("org.openhab.io.openhabcloud.NotificationAction");
NotificationAction.sendNotification("....

I’ve successfully used that in the not too distant pass. I’ve always found OH notifications to be a bit delayed and unreliable so I switched to email.

I think that should work in GraalVM too, but there is a bug in GraalVM right now that is making the typing of openHAB core classes like that not work correctly. Maybe you are running into that?