search

zwave-js / node-red-contrib-zwave-js

The most powerful, high performing and highly polished Z-Wave node for Node-RED based on Z-Wave JS. If you want a fully featured Z-Wave framework in your Node-RED instance, you have found it.
MIT License
47 stars 6 forks source link

UI Stopping certain Controller events from bubbling up to the node. #28

Closed marcus-j-davies closed 3 years ago

marcus-j-davies commented 3 years ago

For controller Events : 'node added' and 'node removed' (the ones I have witnessed)

The console is spitting out.

11 Mar 14:07:04 - [warn] Communication send error: TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Console'
    |     property 'parent' -> object with constructor 'DerivedLogger'
    |     property '_readableState' -> object with constructor 'ReadableState'
    --- property 'pipes' closes the circle

I disabled the UI from being initialised, and it was working once again. When this error occurs, it stops, the event reaching the user in their flow.

I don't know enough about the node-red UI API. Some of these events, are not convertible to JSON, so this may be why?

EDIT: Its very possible, that recent updates to ZWJS, has included more info, In these events, that are not convertible

hufftheweevil commented 3 years ago

Sounds plausible. Might have to pick out the parts of the object that we need for the UI. I can take a closer took when I get home later today.

marcus-j-davies commented 3 years ago

I think its happening here (i could be wrong)

https://github.com/zwave-js/node-red-contrib-zwave-js/blob/863ba5b3d89233caa11e5e63290d1671b56a7f17/zwave-js/ui/server.js#L72

Any fixes to be had - target dev

hufftheweevil commented 3 years ago

Yeah, you're right. Node-RED doesn't check for circular refs before simply stringifying the data packet before sending it over websockets. And so it's getting caught here:

https://github.com/node-red/node-red/blob/master/packages/node_modules/%40node-red/editor-api/lib/editor/comms.js#L168

So the more difficult part is figuring out where the circular ref is. Looking through the ZWJS docs, it doesn't appear that anything is ever circular. All of the controller events have basic-type arguments, or a ZWaveNode object, which does not appear to reference the controller or anything else that would loop around. I'll need to figure out how to run some mock node adding tests.

marcus-j-davies commented 3 years ago

Ok,

It looks as if you're not doing too much with the event data its self - your more interested on the type by the looks if it.

Type : Node, Controller etc etc Event : Node Added, Node Removed, Heal Done, Inclusion Started etc etc...

With regards to Node events, you cut ties with the original event, and just call getNodes() anyway. I think the best way, is to reshape the outgoing event, and only include the bits we need.

I may make a start on this - unless you plan to?

I'm planning on adding the native ZWJS logging options on the config UI at the same. this is all prep work for V7

hufftheweevil commented 3 years ago

Go for it. I'm slammed with work lately so I haven't been able to spend much time on this.

marcus-j-davies commented 3 years ago

@hufftheweevil,

I have fixed (and at the same time, altered some behaviour) with the UI. Do you just want to cast your eyes over the changes/feedback?

Hopefully, I haven't messed up anything with the work you have done - but if you could let me know

marcus-j-davies commented 3 years ago

Scratch that - it all works.

I now also expose the result of ZWaveNode.isControllerNode(), to ascertain, if its the controller, instead of checking the node ID (which isn't always safe to do)

Leaving this open, until it;s merged with main

hufftheweevil commented 3 years ago

I installed the latest dev branch, and there seems to be some changes that I can't even track down in the latest commits.

  1. Controller node is still listed. But it has a (Controller) string next to the number, which is making the first column be much wider than it needs to be. Edit: Just found the place where it does that: https://github.com/zwave-js/node-red-contrib-zwave-js/commit/391cf2169845753a1de3ffd7f5f741c9e64cac5b
  2. The ready thumbs-up are missing. Edit: It's missing because there are 4 columns that all have 33% width. Did you make any changes to that recently? 2nd Edit: Nope - that was me. It just didn't seem to have an effect until that first column was wider. image
hufftheweevil commented 3 years ago

I think your first thought on the controller node was better. There's no need to display the controller in the list of nodes. There are no properties that go along with it anyway.

marcus-j-davies commented 3 years ago

Controller node is still listed. But it has a (Controller) string next to the number, which is making the first column be much wider than it needs to be. Edit: Just found the place where it does that: 391cf21

Yeah, I reversed my decision, to hide, and instead show the word 'Controller', you think hiding it is better?

The ready thumbs-up are missing. Edit: It's missing because there are 4 columns that all have 33% width. Did you make any changes to that recently? 2nd Edit: Nope - that was me. It just didn't seem to have an effect until that first column was wider.

The ready thumb, is there, but it it is now only shown, on the 'ready' specifically , there are some big changes with V7, that removes the status 'RestartFromCachce' - I still need to tweak this a little.

I want to try and decouple the ready thumb away from status- and tie it quite literally to the ready event (that's the most important), originally it was using a combination of both, any ideas on how to retain the thumb (after the ready event)

Edit: Oops I think the 33% was actually me - My bad (Fixed)

hufftheweevil commented 3 years ago

I hear what you're saying about decoupling. However, if the UI is loaded after the ready event fires, then we need to be able to check a property. I don't think there's anything wrong with using .interviewStage, but now we just check for Complete only.

And yeah, the only properties that show up for a controller node are Manuf. CC, which is displayed in the Node Info anyway.

I already have a few changes to take care of both of those things if you don't mind me commiting them.

marcus-j-davies commented 3 years ago

Sure - please go ahead 👍, this is more your realm 😉

Edit We could expose the NodesReadyArray - for showing the thumb after the fact?

marcus-j-davies commented 3 years ago

try the logging also - really nice info in there 👍 By default it places it in the Node-red user dir - but you can specify an absolute path

hufftheweevil commented 3 years ago

We could expose the NodesReadyArray - for showing the thumb after the fact?

That would require tying more of the node functionality in with the UI, which I think we've got a good separation going right now. I guess I'm missing why we can't rely on .interviewStage? That seems to be directed connected with the ready event, unless I'm mistaken.

Edit: I mean we could keep our own list of ready nodes in server.js, but still makes me ask the question above.

marcus-j-davies commented 3 years ago

The interview stage, is about the runtime, gathering a kind of 'footprint' for each device. As of V7, interviews are no longer carried out on a restart (providing it was already interviewed)

Therefore the interview stage will be Complete even after a restart. This however, does not mean Zwave JS is ready to talk to said device

The device DB needs to be initialised, and various internal storage states being loaded (per device) once that is done Zwave JS is ready to talk to the device - hence the ready event.

Granted, it does happen pretty quickly.

if we can somehow check the ready state on the UI being loaded up, it should work as designed. but please go with what you feel is best - even if it means using the interview stage (as mentioned the ready event happens pretty fast)

Edit: A good example here is FLiRS devices, a ping is occurred on them, and providing a response is received, its ready event is triggered.

hufftheweevil commented 3 years ago

Perhaps we should suggest a .isReady node property for the ZWJS library. I imagine that would have uses for a lot of applications.

marcus-j-davies commented 3 years ago

Mmm,

That is kind of already provided, with the event itself I think the expectation here, is that the application should hold the state - we do technically already via the NodesReady. array.

Most implementations are heavily dependant on the ready event, so not sure the benefit will be seen. This will require having to poll device properties, until one is happy - maybe only once/twice in our setup - but not sure the idea will take - I could be wrong of course.

I do like your suggestion however, holding the state inside server.js, and updating when ready does occur. the thumb will then just reference/check the array on load

Edit: There is no harm in asking your question, via an issue - use the feature request template

marcus-j-davies commented 3 years ago

Just saw the commit. I now pass the string instead of the status ID 😁

sorry, should have mentioned , the status is already mapped, to a string - this now happen on GetNodes

hufftheweevil commented 3 years ago

That is kind of already provided, with the event itself I think the expectation here, is that the application should hold the state - we do technically already via the NodesReady. array.

Interesting, that doesn't really go in line with how most APIs work (that I've used, at least), including basic JS APIs. For example, WebSockets have a .readyState property that will align with events that are emitted.

But yes, we can hold our own state in server.js if necessary.

Just saw the commit. I now pass the string instead of the status ID 😁

sorry, should have mentioned , the status is already mapped, to a string - this now happen on GetNodes

You know what, I had seen that and totally forgot. And then I failed to fully test - which is my fault. I will correct it now

marcus-j-davies commented 3 years ago

Yeah, I concur with the status thing. I guess it may have something to do with the potentially large collection of nodes, that may be present. in our slack community, there are users with 100+ nodes

You on slack?

Feel free to expose the array that exists in the main module or hold your own in server.js.

Again, do what you feel is best.

hufftheweevil commented 3 years ago

I am on slack. Is there a Zwave-JS workspace?

marcus-j-davies commented 3 years ago

Sure is.

https://join.slack.com/t/zwave2mqtt/shared_invite/zt-8ns655f6-d407vtI~KjU~1z11jyaQ9Q