-
-
Notifications
You must be signed in to change notification settings - Fork 206
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.
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
feat: implement health reporting #2205
Conversation
Pull Request Test Coverage Report for Build 1772814093
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the user needs to distinguish between the lifeline and the route health check. Just do the right thing™ depending on the target node
|
This means the failed pings to node are 0, this because in the health response there could also be controller failed pings (in this case them are undefined. I noticed there are many props set to undefined in the response dunno if this is an error on zwave-js side or what else. cc @AlCalzone
If you check @AlCalzone comment in my original implementation there were 2 buttons one for lifeline check and one for the route check, I have leave just one button and trigger the correct api call based on the target node, if it is a controller I will use the lifeline otherwise I use the route check |
Some statistics can't be determined for all nodes because they requires support for the Powerlevel CC. Also, if you check the type definitions - route health has different properties than lifeline health.
It should route - got any driver logs? @LordMike
This would have to go in the driver. Currently it does a fixed amount of checks and when the route health is low, this takes longer. It would have to stop quicker when the intermediate results are bad. |
Yeah I know that, in fact the table headers change based on the api called |
I see - so it's actually a bug... I just assumed it was direct, hence I didn't grab anything. Will look into this later today. The Node I chose as target was Node 1, which is the controller.
I confirmed this when I then chose another, closer, node, which had 10 failed pings from the controller instead. Something stupid like suffixing "failures" could mitigate this: Imagine if I had two failed pings, it would say "Node: 2", which could leave me wondering why Node 2 has anything to do with the test I just made. |
Fails suffix added |
Or you could be specific, like node -> ctrlr: 10/10 I have thought about this quite a bit when developing this feature and the reports in the driver logs could serve as a guideline for the UI. |
10 is the total number of pings you do, right? |
Yup. To show an example, I'm referring to this:
And if there were failures pinging the controller at normal power (here -8dBm was ok), it would display this aswell:
|
@AlCalzone That helps a lot! Thanks |
I've also made zwave-js/node-zwave-js#4130, which we might wanna look at first. But for completeness sake, here is a driver output for my node 32, which is my most-far-away node according to the map. Starting a check for that, yields this (and no results). Log snippet
|
Ahh ok I see what's happening. The node is instructed to ping the controller but it never makes any progress. The driver expects it to either make some progress or report failure at some point. |
Some thoughts while testing this: UIs need more feedback from the driver to show what's happening. The driver does a lot of different things but UI's can't show any actual progress. This is my 🚧👷🏻♂️. @robertsLando The current results table is suboptimal, compare this
to the information included in the table:
|
@AlCalzone Ready for new review |
Updated, this is still open:
What about this for the failed ping display? |
I also gave this a run wtih a new
|
Actually nope, I have only added an export button to allow users export them BTW I have synced test pr with this so fell free to give it a try with latest changes |
I didn't yet: zwave-js/node-zwave-js#4075 for updates
Not sure if this is practical. Testing the signal takes long and if you have many nodes, this exponentially takes longer if you were to measure each possible connection. |
Fixes #2113