Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Proposal 1 - extend the trigger dropdown #25

Open
macinspak opened this issue Aug 8, 2021 · 26 comments
Open

Proposal 1 - extend the trigger dropdown #25

macinspak opened this issue Aug 8, 2021 · 26 comments
Assignees
@bartbutenaers
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@macinspak
Copy link
Contributor

Originally from #24, submitted by @bartbutenaers

Currently the sensor node allows two trigger types ("all messages" and "msg.payload=true"). It would be nice if extra options could be added by using TypedInputs. For example:

image

Then it becomes possible to trigger an alarm when msg.payload=1 or msg.payload="OPEN" or ...

That way my flow would become much cleaner, since I could avoid a lot of Change-nodes (which I also see in the example flows on your readme page):

image

Of course the existing options should be migrated automatically, to make sure the existing flows are not impacted.

BTW It is not clear to me how the current filtering works. Because at first sight the triggerType seems only to be used in the sensor.html file, but I might have overlooked something. Would be nice if you could explain it!
@macinspak
Copy link
Contributor Author

Great idea, I like this. It certainly would make things easier.

Can anyone else comment on common use cases? I would think maybe even a "msg.[enter path here]" = "string value also compared as numeric" or something would meet the majority of cases, but what about more complex multi-facet matching? Ie
msg.payload.value==200 && msg.tag==3 ?

perhaps it's better to allow a small code snippet space like a basic function, then you could do any logic you like?

@macinspak macinspak mentioned this issue Aug 8, 2021
Closed
@macinspak macinspak added enhancement New feature or request help wanted Extra attention is needed labels Aug 8, 2021
@bartbutenaers
Copy link
Contributor

even a "msg.[enter path here]" = "string value also compared as numeric" or something would meet the majority of cases

Indeed that would solved most of the cases. Hadn't even thought about that...

Although that indeed might be sufficient for lots of use cases, I see some advantages in using the TypedInputs:

  • The TypedInput fields is more Node-RED standard style. If I need to check for a boolean= true then every Node-RED user knows that he needs to select a boolean TypedInput from the dropdown. Although your expression syntax is VERY easy, you have to look it up in your readme page. While the TypedInputs are more self explaining. The expression seems (to me) a bit of cheating :-)

  • The TypedInputs offer some extra types of triggers. You could introduce dynamic thresholds when using the "flow" or "global" types. An simple hypothetical example:

    • Normally you want to raise an alarm when there are more than 30 people in a bar, because that is required for fire insurance.
    • During those dark COVID days you want to raise an alarm when there are more than 10 people in the same bar, to be able to insure social distance.

    So you need some dynamic threshold, which should be easy to change live (without requiring a deploy)!

    You could again workaround this by adding some logic in your flow to determine whether this is a problem or not, and then pass that boolean to your Sensor node. But the whole idea of this feature request is to simplify our flows, by getting rid of lots of Change/Function/... nodes. By using a numeric TypedInput you can simply write your threshold in Context-memory via some user interface (e.g. the Node-RED dashboard) in flow memory flow.maximumVisistors=10 and read that threshold directly from your node ....

Note that processing the values from multiple TypedInputs can be done using evaluateNodeProperty. I can always create a proposal via a PR in a couple of weeks if you like!

@macinspak macinspak mentioned this issue Aug 17, 2021
Closed
@macinspak
Copy link
Contributor Author

macinspak commented Aug 17, 2021 •
edited
Loading

OK, had a think about this. Maybe a very easily understandable way to do it would be to replicate the switch node functionality? People are used to this node and it will make immediate sense.

Screen Shot 2021-08-18 at 8 49 57 am

Because technically we need to come up with a boolean value (To trigger or not to trigger? That is the question).

This switch node sets the type of expression at the top and then allows the user to set multiple and/or functions to trigger. It would just be for a single "output".

What do you think?

I think this would be really cool and we already have the design we can copy directly out of the switch node html and even look at the implementation of the js to tweak to our needs. Seems to me to be the most powerful and flexible option that is ultra simple to replicate.

If you feel like it, more than happy to have help doing it. It's the whole reason I released this set of nodes, to get people to help making it the best it can be.

@macinspak
Copy link
Contributor Author

macinspak commented Aug 17, 2021 •
edited
Loading

switch.html
switch.js

128590464-5c10754a-e467-491c-a1fa-54ce86d0989e

Note there is only a delete button and no output selection.

And maybe we could change (checking all ..) to ( all must be true / at least one must be true) or (all / any) or (and/or)?

Oh, and the semantic for no rules is trigger on any message (so it's upgrade equivalent) we default to no rules and then people can migrate to the new node over time by adding rules and deleting any intermediary nodes as they go.

So if no rules are in the list, we might show a message in that rule box saying "will trigger on any message unless you add at least one rule" or something.

@bartbutenaers
Copy link
Contributor

Maybe a very easily understandable way to do it would be to replicate the switch node functionality?

That would indeed be the most powerful option. Although it would make your code a LOT more complex to maintain. You also have to take into account that the Switch-node will become much more complex in the future: for example when the compound rules will be implemented (IF ... AND ... OR ...) in the Switch node, you would have to implement the same in your node. Unless you implement your own all / any rules (like you have stated above), but then your clone of the Switch node code will grow away from the Switch node in the future (which makes it very difficult to keep in sync with the new Switch node features in the future).

Personally I would keep it a bit simple. If we could only support (equality checking of) multiple trigger value types via TypedInputs, you can e.g. wire a GPIO input (from a read relay on a door) directly to your node: 0 = trigger alarm / 1 = don't trigger alarm. That way we can already simplify quite a large number of flows, by eliminating "basic" Switch/Change nodes. For less simple operations (i.e. not a simple equality check) I would suggest to keep using a Switch/Change node: each node should do only what it is good at ;-)

Of course it is your node: if you want to clone the whole Switch node functionality, then I rest my case. But I have not enough free time to implement such a big change... If you want to keep it simple by only typedinputs, then I can try to implement a first version if you want.

This switch node sets the type of expression at the top

That is indeed something very useful, again to avoid Switch nodes. If e.g. the trigger value arrives in the input message as msg.trigger_value, then you could simply specify that msg property name. I had that already in mind as 'proposal 4', but I didn't wanted to look greedy at the time being ;-).

@macinspak
Copy link
Contributor Author

Thanks @bartbutenaers, you are probably correct it is overkill to go the complete duplication of the switch node functions AND try and maintain it to remain current.

I was actually not thinking to that degree. I was more thinking we use the same sort of approach, and just replicate one or 2 of the most useful ones that will fill our immediate need, and it provides a launchpad and precedent for anyone to chip in if they would like to add (or just request) a function they think should also be ported to meet a use case they have (with suitable justification of course).

I also would not care if it was not maintained in step with the switch node as I don't think that will really help us.

So we start with about the same (or less effort) to port a small portion of the switch code that is essentially already written for us. Then make a note that people can request or contribute more functions if they want.

What do you think? It provides a quick solution now and more flexibility if needed in future.

Oh, so if I understand about msg.trigger_value, then you could just put that in the field at the top and this mod idea would also cover it, right? Or did I misunderstand?

@bartbutenaers
Copy link
Contributor

Oh, so if I understand about msg.trigger_value, then you could just put that in the field at the top and this mod idea would also cover it, right? Or did I misunderstand?

If you specify at the top msg.blabla then indeed your node will get the trigger value from that input message 'blabla' property. By adding such a typedInput field (type 'msg') you can avoid a lot of Change nodes in a flow. So I would definately add such a typedInput to your config screen.

It provides a quick solution now and more flexibility if needed in future.

To keep it simple and yet powerful, I'm not sure anymore if we should mimic partly the Switch node. It will be a lot of development, make your node much more difficult to maintain, and we will never be able to compete with the Switch node (especially if that is extended in the near future with AND/OR logic).

I am now wondering if your initial idea of using a simple expression would be a better solution perhaps?
Suppose we would e.g. use this expression parser library. Don't think it is maintained anymore, but might be sufficient for our needs.

I haven't tested it yet, but I assume we could use it like this:

  1. The user specifies an expression on your config screen, for example: "msg.payload == 1 and msg.topic == reed_relay".
  2. We compile the expression and pass the input message to it: 
    // Compile expression from the config screen to an executable function
var myfilter = compileExpression(expression);

// Execute function (with input message as parameter)
myfilter({msg: msg});

Does this makes sense to you?

@macinspak
Copy link
Contributor Author

I had another look at switch node and think you are right, on my second look at it, there is a lot of capability, but probably overkill. Something a simple expression can resolve.

So did you want to have a shot at doing the node the way you think with these considerations in mind? I think we are on the same page, we need to give a basic way to make these simple cases handled, and with an expression option it allows a lot of flexibility and it's still simpler to code.

As long as it covers those cases and works well I'd be happy to merge it in and publish an update.

Thanks again for helping work this out. We certainly get a far more powerful and better solution when more people collaborate on it.

@bartbutenaers
Copy link
Contributor

Sure I can have a look at it. Only thing that bothers me, is that the library is probably not maintained anymore. On the other hand we don't need lots of features from it ...

If you now a more recent javascript logical expression parser library, please let me know!

And I haven't checked if the library is available on npm, otherwise we have to copy its js file to your repo...

My time is up for today...

@bartbutenaers
Copy link
Contributor

bartbutenaers commented Aug 23, 2021 •
edited
Loading

Seems there is an NPM module available with the same name and functionality, but pointing to another Github repository.

As you can see it is a fork of the original repo I mentioned above:

image

This package is well maintained, which is much better in case we need assistance.
So I am going to try that one as soon as I have time ...

@bartbutenaers
Copy link
Contributor

Currently your node has a dropdown:

image

Am I correct that this dropdown simply need to be replaced by an input field, where the logical trigger expression can be entered?

If so, I need to write a short code snippet for existing nodes. Just to make sure we don't have impact on existing flows.

  • Existing nodes with option msg.payload.open == true will get an identical expression in the new expression editor field.
  • Existing nodes with option any message will get an empty expression editor field? And we consider an empty field as 'no condition' which means always true.

Is that ok for you?

@Allow2CEO
Copy link

Sure. Sounds great!

@bartbutenaers
Copy link
Contributor

Hmm, found a partystopper...
Would like to use nested input message properties:

image

However it looks like the Filtrex library doesn't support nested object properties (since it uses hasOwnProperty):

image

I could workaround this, but I think it is better if I create a pull-request for the Filtrex library to implement this feature ...

@macinspak
Copy link
Contributor Author

macinspak commented Aug 24, 2021 via email

@bartbutenaers
Copy link
Contributor

FYI:
I have registered two issues for the filtrex library:

  • One for accessing nested message properties (see here)
  • One for using boolean values (see here)

Hopefully the author can help us soon with this, so I can continue with the implementation of expressions for alarms.
He seems to be a very active contributor, so fingers crossed...

@macinspak
Copy link
Contributor Author

macinspak commented Aug 24, 2021 via email

@bartbutenaers
Copy link
Contributor

I have implemented a small endpoint, that allows us to check the syntax of the expression (on the server side).
That syntax check is runned for every character you type, to have a responsive config screen.
The result of the last syntax check is also stored in the node, so the flow editor can draw a red triangle if necessary (based on the last syntax check):

alarm_trigger_condition

BTW It is not clear to me how the current filtering works. Because at first sight the triggerType seems only to be used in the sensor.html file, but I might have overlooked something. Would be nice if you could explain it!

@macinspak
Copy link
Contributor Author

macinspak commented Aug 25, 2021 via email

@bartbutenaers
Copy link
Contributor

Hi @macinspak,
Sorry for the delay!
But I don't think it is useful that I create a pull request, as long as the features haven't been added to the filtrex expression library...
I see now that somebody else has posted the same feature request at the same day, but solving it in another way: see issue.
Hopefully the author can find some time to help us, because I'm a bit stuck now with this...

@macinspak
Copy link
Contributor Author

macinspak commented Sep 5, 2021 via email

@bartbutenaers
Copy link
Contributor

@macinspak,
We got feedback from the Filtrex author.
The poor devil needs to do his final examns in about two weeks...
I will contact you again as soon as the requested Filtrex features are available.
Bart

@bartbutenaers bartbutenaers mentioned this issue Sep 30, 2021
Merged
@bartbutenaers
Copy link
Contributor

Hi @macinspak,
I finally managed to create a pull request.

@macinspak
Copy link
Contributor Author

macinspak commented Oct 2, 2021 via email

@macinspak
Copy link
Contributor Author

macinspak commented Oct 6, 2021 via email

@bartbutenaers
Copy link
Contributor

Hey Andrew,
Sorry for the late reply. It is a busy week... Will get back to you this weekend!
Bart

@macinspak
Copy link
Contributor Author

macinspak commented Oct 14, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bartbutenaers bartbutenaers

Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Allow2CEO @bartbutenaers @macinspak