-
Notifications
You must be signed in to change notification settings - Fork 29
Drawing of link statistics in GUI - Tx power and Rx RSSI #738
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
base: main
Are you sure you want to change the base?
Conversation
…g for links in JS.
Summary of ChangesHello @EskoDijk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the simulation's visualization capabilities by introducing dynamic display of radio link quality metrics (Tx power and Rx RSSI) between nodes in the GUI. The architectural approach centralizes the control of these visualizations on the Go backend, streamlining future development. This change provides users with more immediate and detailed insights into network performance and link health during simulations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a valuable feature for visualizing link statistics, specifically Tx power and Rx RSSI, in the GUI. The implementation spans across the Go backend, gRPC definitions, and the JavaScript frontend, and is well-structured. The backend logic correctly hooks into the radio frame dispatching process to gather statistics, and the frontend introduces a new LinkStats visual component to display this data. While the overall implementation is solid, I've identified a few potential issues concerning correctness and robustness that I recommend addressing.
| logger.AssertNotNil(gn.extaddr) | ||
| logger.AssertTrue(gn.extaddr > 0) | ||
| f.nodes[id] = gn | ||
| f.extAddrMap[gn.extaddr] = id |
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.
There's a potential race condition here. When a new node is added, its extaddr is InvalidExtAddr. The line f.extAddrMap[gn.extaddr] = id maps InvalidExtAddr to the new node's ID. If another node is added before the first one gets a valid extaddr, this map entry will be overwritten, leading to incorrect state. The extAddrMap should only be updated with valid extended addresses, which is correctly handled in onExtAddrChange.
Additionally, logger.AssertNotNil(gn.extaddr) is called on a uint64, which is not a pointer type and will never be nil. This assertion might not be behaving as intended.
Consider removing these lines. The extAddrMap is correctly managed in onExtAddrChange.
| childId := d.extaddrMap[extaddr].Id | ||
| d.vis.SetParent(childId, node.ExtAddr) |
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.
Accessing d.extaddrMap[extaddr].Id could cause a panic if extaddr is not present in the map, as d.extaddrMap[extaddr] would be nil. While it's likely that a child node is known at this point, adding a check for existence would make the code more robust against unexpected states.
| childId := d.extaddrMap[extaddr].Id | |
| d.vis.SetParent(childId, node.ExtAddr) | |
| child := d.extaddrMap[extaddr] | |
| if child != nil { | |
| d.vis.SetParent(child.Id, node.ExtAddr) | |
| } |
| } | ||
|
|
||
| if opt.RxRssi { | ||
| if nbInfo.lastTxPower == RssiInvalid { |
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.
There seems to be a copy-paste error here. When checking if the RSSI value is available to create the label, nbInfo.lastTxPower is checked instead of nbInfo.lastRssi. This will result in "N/A" being displayed for RSSI even when a valid RSSI value is available, if lastTxPower is invalid.
| if nbInfo.lastTxPower == RssiInvalid { | |
| if nbInfo.lastRssi == RssiInvalid { |
Aims to add drawing of link statistics in the GUI, under control of the Go code: so it's easier to add new visualizations for links in the future by only updating the Go code (server side).
As a start/try, it can now visualize latest Tx power (dBm) and Rx RSSI (dBm) between nodes, based on the radio model. This requires no changes in OpenThread w.r.t. reporting of internal changes in link quality or the like.
Multiple commits will later be squashed. Currently in draft.