zwave-js / zwave-js-ui

Full featured Z-Wave Control Panel UI and MQTT gateway. Built using Nodejs, and Vue/Vuetify
https://zwave-js.github.io/zwave-js-ui
MIT License
919 stars 196 forks source link

feat: zniffer #3706

Closed robertsLando closed 1 month ago

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 9298507831

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
api/lib/SocketManager.ts 0 1 0.0%
api/config/store.ts 0 2 0.0%
src/components/nodes-table/nodes-table.js 0 2 0.0%
api/lib/SocketEvents.ts 0 3 0.0%
src/lib/SocketEvents.js 0 7 0.0%
src/lib/items.js 0 10 0.0%
src/router/index.js 0 11 0.0%
src/stores/base.js 0 33 0.0%
api/lib/utils.ts 4 62 6.45%
api/app.ts 0 103 0.0%
<!-- Total: 4 837 0.48% -->
Files with Coverage Reduction New Missed Lines %
api/lib/SocketEvents.ts 1 0.0%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 9272143414: -0.6%
Covered Lines: 3899
Relevant Lines: 19590

๐Ÿ’› - Coveralls
AlCalzone commented 1 month ago

I'll just put my thoughts here...

The settings for Zniffer should probably not offer the serial port that's already in use by the controller: grafik

I don't think generating keys there makes sense, as you want to spy on an existing network with existing keys: grafik We may also have to consider setting them on the fly to retroactively decode some commands, but that will require support in the driver first.

AlCalzone commented 1 month ago

Filtering nodes makes sense in the table view but not in the settings IMO: grafik

AlCalzone commented 1 month ago

Saving settings for the Zniffer should probably not restart the driver for the controller.

AlCalzone commented 1 month ago

grafik

CAPTURE should be renamed so it's clear that it saves to a file.

AlCalzone commented 1 month ago

There's an error in the devtools and nothing is being shown in the table: grafik

Speaking of the table, we probably want infinite scrolling, not pagination. 10 frames per page can easily be surpassed with a single communication attempt.

robertsLando commented 1 month ago

The settings for Zniffer should probably not offer the serial port that's already in use by the controller:

Yeah but what if the driver is disabled? (still have to add this option)

I don't think generating keys there makes sense, as you want to spy on an existing network with existing keys:

Should I drop keys from there and use the one set on driver? Or add a button to clone the ones on driver?

We may also have to consider setting them on the fly to retroactively decode some commands, but that will require support in the driver first.

๐Ÿ‘๐Ÿผ

Saving settings for the Zniffer should probably not restart the driver for the controller.

Yeah agree

There's an error in the devtools and nothing is being shown in the table:

That error happens because I'm using ZWaveFrameType that seems not exported in safe: https://github.com/zwave-js/zwave-js-ui/pull/3706/files#diff-71a3cb674c70b3fa9b3f52e9afc7d873f7d0eeefcbccb7175f1448a816811a99

AlCalzone commented 1 month ago

Should I drop keys from there and use the one set on driver? Or add a button to clone the ones on driver?

Cloning might make sense to debug the current network.

Other than that, some observations (I know the layout is not optimized yet):

AlCalzone commented 1 month ago

Additional findings in the latest version:

AlCalzone commented 1 month ago

More things:

AlCalzone commented 1 month ago
AlCalzone commented 1 month ago
AlCalzone commented 1 month ago
kpine commented 1 month ago
  1. Typo: image

  2. Once you enter an invalid search query, the error message is never cleared and the tooltip never returns. Only option is to leave the window and come back.

image

  1. If you leave the Zniffer window (e.g. go to settings) 1) the existing trace on screen is lost completely and 2) no traffic is captured to the screen. Some possible options:

    • Open a modal dialog with a warning message for confirmation when leaving
    • Preserve trace and restore when returning, keep capturing in the background. Or restore some X percentage of past logs.
    • Allow opening the zniffer in a separate window, like the debug log

    My imagined use case for this would be to leave the zniffer running in the background unattended. Is this use case supported? I left the window and performed some activity, and when I came back the trace window was empty, but saving to file produced something, I just haven't checked the contents.

  2. Is there a plan to add the ability to load a zlf file and display in the same UI?

5. Is there a future use case for this window resizing? Wondering why it just doesn't expand to the entire height of the window automatically.

AlCalzone commented 1 month ago

5. Is there a future use case for this window resizing? Wondering why it just doesn't expand to the entire height of the window automatically.

Click one of the captured frames :)

kpine commented 1 month ago
  1. Is there a future use case for this window resizing? Wondering why it just doesn't expand to the entire height of the window automatically.

Click one of the captured frames :)

Yeah, I did that and still asked the question. :man_facepalming:

robertsLando commented 1 month ago

Thanks @kpine ! About the lost frames, I dunno if there is an api actually to fetch in memory frames from zniffer, @AlCalzone ?

I could store them on my side but I think the zniffer instance already have them somewhere in order to create the capture.

Also it's a nice idea to be able loading a capture file from UI, didn't thought about that. Cc @AlCalzone

About the rest I can take a look tomorrow ๐Ÿ™๐Ÿป

AlCalzone commented 1 month ago
AlCalzone commented 1 month ago

The layout bothers me a bit: grafik

Also, is there any way to give the columns a fixed/min width to avoid this? grafik Maybe even disable the sorting altogether?

kpine commented 1 month ago

Thanks @kpine ! About the lost frames, I dunno if there is an api actually to fetch in memory frames from zniffer, @AlCalzone ?

I could store them on my side but I think the zniffer instance already have them somewhere in order to create the capture.

Not critical functionality in my mind. The warning when leaving via navigation, or an "open in new window" are both good solutions. I can of course open my own new window to avoid the problem. The ease of losing the current trace with a miss-click is the main problem.

AlCalzone commented 1 month ago

Yep I should really just expose the captured frames so Z-UI can restore the state from those, and add a hook to actually clear them.

EDIT: Next release will include that ->> https://github.com/zwave-js/node-zwave-js/pull/6852

AlCalzone commented 1 month ago

I feel like this is going into nitpick territory, but number columns should be right-aligned: grafik

AlCalzone commented 1 month ago

Regarding formatting of the route - I'd also put that in the overview table, so you can see the route there immediately. Routed frames have a hop property that tells us where along the route we are. The direction property tells us in which direction along the route we are going and does not have to be listed separately. grafik

For reference, this is how this frame's route is formatted in the logs: grafik

AlCalzone commented 1 month ago
AlCalzone commented 1 month ago
robertsLando commented 1 month ago

TODOs:

AlCalzone commented 1 month ago
AlCalzone commented 1 month ago
  • [x] Add a button to resume auto-scrolling

Am I blind? :)

robertsLando commented 1 month ago

@AlCalzone About the button I forgot to ask you what you mean because actually auto scroll is always enabled except if you have a frame opened in details, do you want to be able to re-enable it when you have a frame opened so?