Conversation
Summary of ChangesHello @rochaferraz, 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 OpenThread Commissioner's capabilities by introducing advanced network diagnostic and provisioning tools. It enables comprehensive network topology discovery, improves the reliability of large diagnostic data transfers through fragmentation support, and adds a flexible proxy mode for joiner interactions. Furthermore, new Python scripts provide convenient utilities for QR code-based provisioning and network visualization, collectively offering a more powerful and user-friendly experience for managing Thread networks. 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. Changelog
Activity
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
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial pull request that introduces significant enhancements to the OT Commissioner, including network traversal and topology discovery, improved diagnostics with support for fragmented responses, and new utility scripts for QR code generation and network visualization. The changes are well-structured and the new features are thoughtfully implemented. My review focused on correctness and maintainability, and I've identified a couple of areas for improvement: one related to TLV validation logic and another concerning packet handling in the new proxy mode. Overall, this is a very solid contribution.
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements, including network traversal capabilities, improved diagnostics with support for fragmented responses, and new utility scripts for network visualization and QR code generation. The changes are well-structured and the new features are valuable. I have one suggestion to improve the consistency of the JSON output from the traversenetwork command, which would make it easier for consumer scripts to parse.
rochaferraz
force-pushed
the
traverser-5
branch
3 times, most recently
from
9291836 to
1f99d5e
Compare
- Added safety check in CoAP SetToken to prevent undefined behavior with empty vectors. - Enhanced TLV parsing utilities to support fragmented diagnostic data.
- Extended NetDiagData to support additional TLVs (e.g., Child Table, Address List). - Implemented serialization helpers and constants in network_diag_data.cpp. - Updated CMakeLists.txt to include the new source file.
- Implemented logic to handle fragmented diagnostic responses using Query ID and Answer TLVs. - Added accumulation of multi-packet responses into a single NetDiagData structure. - Enhanced debug logging for diagnostic transactions.
…tion - Added NetDiagGetQuery method to CommissionerApp interface. - Implemented dummy and mock implementations for testing. - Added json.cpp helper for serializing NetDiagData to JSON format.
- Introduced 'traversenetwork' command in CLI. - Implemented Traverser class to discover leader, routers, and children. - Added support for generating detailed JSON network topology reports. - Updated Interpreter to handle new command syntax and logic.
- Added CommissionerImplFragmentTest to verify handling of fragmented diagnostic responses. - Covered scenarios for legacy, single-packet, and multi-packet (reassembly) responses. - Added script/node_diagram to visualize the network topology from the JSON output.
This script converts between Thread Operational Datasets (as TLV hex strings) and a custom QR code text string format used for provisioning Thread devices. It can also generate PNG image files of the QR codes. The custom "THREAD:" format is modeled after the standard Wi-Fi QR code provisioning format. The Wi-Fi format itself is derived from the MeCard specification, originally developed by NTT DoCoMo in Japan for sharing contact information via QR codes on cellular phones. Format Structure: THREAD:S:<Network Name>;P:<Network Key>;I:<PAN ID>;E:<Ext PAN ID>;C:<Channel>;; Removed unused network_evaluator.cpp and network_evaluator.hpp as they were not referenced or compiled anywhere.
The leader was observed sending 0-byte kNetworkDiagChildIpv6Address (type 30) TLVs, which would previously fail validation and be dropped. Update the validation logic to gracefully accept these empty TLVs.
rochaferraz
force-pushed
the
traverser-5
branch
from
1f99d5e to
14e6a67
Compare
In proxy mode, if a received UDP packet is larger than the buffer provided to Receive(), the packet was not fully consumed and the remainder was left in the queue. This could lead to processing a partial packet as a full packet on the next read, which is incorrect for datagram-based communication. Drop the oversized packet entirely by removing it from the queue.
The JSON field MacAddr holds the RLOC16, which is inconsistent with RouterNeighbor and Child objects that use Rloc16 for the same data type. This changes the key from MacAddr to Rloc16 in the C++ JSON generator and updates the Python node diagram script to use the new key.
…failure Forward joiner messages to OnJoinerMessage callback when in Proxy Mode, bypassing DTLS. Also added missing FRIEND_TEST.
rochaferraz
force-pushed
the
traverser-5
branch
from
d3f5620 to
a0f1f67
Compare
Reverted content to 299e537 to remove unnecessary refactoring and restore original Proxy Mode logic. Re-applied fix for dropping oversized packets in Proxy Mode by flushing the receive queue.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
- Coverage 72.75% 68.92% -3.83%
==========================================
Files 72 76 +4
Lines 7444 9011 +1567
==========================================
+ Hits 5416 6211 +795
- Misses 2028 2800 +772
🚀 New features to boost your workflow:
|
- Added src/library/network_traverser_test.cpp with tests for: - Happy path traversal (Leader, Routers, Children). - Data merging from multiple chunks. - Timeout and retry logic. - Cancellation (Stop). - Chunk skipping upon failure. - Fixed missing headers in console.cpp and coap.cpp. - Minor updates to NetworkTraverser implementation to support testing.
rochaferraz
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the OT Commissioner with a new network traversal feature, support for fragmented diagnostic responses, and improved CLI capabilities. The implementation of the Traverser class and the reassembly logic for DIAG_GET.ans messages are well-structured. However, there are critical concerns regarding memory management in the diagnostic reassembly logic, where stale pending queries could lead to a memory leak. Additionally, the derivation of child RLOC16s during traversal relies on potentially ambiguous data and should be made more robust by using the authoritative Router ID from the Leader Data. Several improvements to TLV validation and error handling have also been identified to ensure better adherence to the Thread specification and overall system stability.
- Added traverser_test.cpp to test global timeout functionality - Refactored ProcessTraverseNetworkJob to accept a configurable timeout - Updated CMakeLists.txt to include new test file - Included minor formatting updates
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the OT Commissioner, primarily focusing on network discovery and diagnostics. The new traversenetwork command and its underlying Traverser implementation provide a powerful tool for mapping network topology. The rework of DIAG_GET.ans message handling to support fragmented responses is a crucial improvement for dealing with large diagnostic reports.
The code is well-structured, and the addition of comprehensive unit tests for the new fragmentation logic is commendable. I've also noted several important bug fixes, including a memory leak fix in ProcessBr, prevention of undefined behavior when handling empty tokens in CoAP messages, and a fix for an infinite loop in the joiner session logic. These improvements significantly enhance the robustness and reliability of the commissioner.
I have a few minor suggestions for improvement. Overall, this is an excellent contribution.
Added explicit error checking and logging to: - internal::DecodeNetworkData - internal::DecodePrefixEntry - internal::DecodeChildTable This improves diagnosability of malformed or unexpected TLV data during network traversing.
The Traverse_Flow_LeaderOnly, Traverse_Flow_RoutersAndChildren, and Diff_Chunks_Merged tests were simulating network diagnostic responses where some chunks (specifically Network Data chunk which includes Leader Data) were empty but marked as present. This caused the NetworkTraverser to parse an empty LeaderData structure, resulting in a Router ID of 0 (default). Since the Leader RLOC16 (Chunk 0) was 0xFC00 (checks against Router ID 63), this mismatch caused the traverser to incorrectly identify the Leader as a regular Router and queue it for querying, leading to unexpected behavior strings in the test flow. This commit updates the test simulations to populate mRouterId correctly whenever kLeaderDataBit is requested, ensuring the traverser correctly identifies the Leader and proceeds with the test flow.
| /** | ||
| * @brief Handler for network traversal events. | ||
| */ | ||
| struct TraverseHandler |
There was a problem hiding this comment.
Why don't we define this a class with virtual methods? Just like the existing CommisionerHandler class
| * @retval kInvalidState Commissioner is not active. | ||
| * @retval kAlready Traversal is already in progress. | ||
| */ | ||
| virtual Error TraverseNetwork(TraverseHandler aHandler) = 0; |
There was a problem hiding this comment.
If you API receives a handler, then it would be better that all errors are handled in the handler so that the caller doesn't need to write error handling in two places.
| virtual Error TraverseNetwork(TraverseHandler aHandler) = 0; | |
| virtual void TraverseNetwork(TraverseHandler aHandler) = 0; |
| struct QueryId | ||
| { | ||
| uint16_t mQueryId = 0; | ||
| }; |
| #include <sys/socket.h> | ||
| #include <sys/time.h> | ||
|
|
||
| #include <fcntl.h> | ||
| #ifdef __linux__ | ||
| #include <sys/socket.h> |
There was a problem hiding this comment.
| #include <sys/socket.h> | |
| #include <sys/time.h> | |
| #include <fcntl.h> | |
| #ifdef __linux__ | |
| #include <sys/socket.h> | |
| #include <fcntl.h> | |
| #ifdef __linux__ | |
| #include <sys/socket.h> | |
| #include <sys/time.h> |
| {"config", &Interpreter::ProcessConfig}, | ||
| {"start", &Interpreter::ProcessStart}, | ||
| {"stop", &Interpreter::ProcessStop}, | ||
| {"active", &Interpreter::ProcessActive}, | ||
| {"token", &Interpreter::ProcessToken}, | ||
| {"br", &Interpreter::ProcessBr}, | ||
| {"domain", &Interpreter::ProcessDomain}, | ||
| {"network", &Interpreter::ProcessNetwork}, | ||
| {"sessionid", &Interpreter::ProcessSessionId}, | ||
| {"borderagent", &Interpreter::ProcessBorderAgent}, | ||
| {"joiner", &Interpreter::ProcessJoiner}, | ||
| {"commdataset", &Interpreter::ProcessCommDataset}, | ||
| {"opdataset", &Interpreter::ProcessOpDataset}, | ||
| {"bbrdataset", &Interpreter::ProcessBbrDataset}, | ||
| {"reenroll", &Interpreter::ProcessReenroll}, | ||
| {"domainreset", &Interpreter::ProcessDomainReset}, | ||
| {"migrate", &Interpreter::ProcessMigrate}, | ||
| {"mlr", &Interpreter::ProcessMlr}, | ||
| {"announce", &Interpreter::ProcessAnnounce}, | ||
| {"panid", &Interpreter::ProcessPanId}, | ||
| {"energy", &Interpreter::ProcessEnergy}, | ||
| {"exit", &Interpreter::ProcessExit}, | ||
| {"quit", &Interpreter::ProcessExit}, | ||
| {"help", &Interpreter::ProcessHelp}, | ||
| {"state", &Interpreter::ProcessState}, | ||
| {"netdiag", &Interpreter::ProcessNetworkDiag}, | ||
| {"traversenetwork", &Interpreter::ProcessTraverseNetwork}, |
There was a problem hiding this comment.
Are those changes generated by the format tool? Otherwise, prefer keep the format unchanged and only add your new command
| if (len > 0) | ||
| { | ||
| // Accessing &aToken[0] when size is 0 is undefined behavior. | ||
| memcpy(mHeader.mToken, &aToken[0], len); | ||
| } |
There was a problem hiding this comment.
Use std::vector::data():
| if (len > 0) | |
| { | |
| // Accessing &aToken[0] when size is 0 is undefined behavior. | |
| memcpy(mHeader.mToken, &aToken[0], len); | |
| } | |
| memcpy(mHeader.mToken, aToken.data(), len); |
|
|
||
| // Add Query ID TLV | ||
| { | ||
| uint16_t queryId = mNextQueryId++; | ||
| SuccessOrExit(error = AppendTlv(request, {tlv::Type::kNetworkDiagQueryID, queryId})); | ||
| } | ||
|
|
||
| LOG_DEBUG(LOG_REGION_MESHDIAG, "sent DIAG_GET.qry with Query ID {}", mNextQueryId - 1); | ||
|
|
There was a problem hiding this comment.
| // Add Query ID TLV | |
| { | |
| uint16_t queryId = mNextQueryId++; | |
| SuccessOrExit(error = AppendTlv(request, {tlv::Type::kNetworkDiagQueryID, queryId})); | |
| } | |
| LOG_DEBUG(LOG_REGION_MESHDIAG, "sent DIAG_GET.qry with Query ID {}", mNextQueryId - 1); | |
| // Add Query ID TLV | |
| { | |
| uint16_t queryId = mNextQueryId++; | |
| SuccessOrExit(error = AppendTlv(request, {tlv::Type::kNetworkDiagQueryID, queryId})); | |
| LOG_DEBUG(LOG_REGION_MESHDIAG, "sent DIAG_GET.qry with Query ID {}", queryId); | |
| } | |
| if (error != ErrorCode::kNone) | ||
| { | ||
| LOG_ERROR(LOG_REGION_MESHDIAG, "DecodeNetworkData: failed to get Prefix TLV list: {}", error.ToString()); | ||
| goto exit; |
There was a problem hiding this comment.
Here and else where, please use ExitNow()
| goto exit; | |
| ExitNow(); |
| { | ||
| LOG_WARN(LOG_REGION_JOINER_SESSION, "session(={}) insufficient buffer size {}, {} needed", | ||
| static_cast<void *>(&mJoinerSession), aMaxLen, buf.size()); | ||
| mRecvBufs.pop(); |
| { | ||
| LOG_WARN(LOG_REGION_JOINER_SESSION, "session(={}) insufficient buffer size {}, {} needed", | ||
| static_cast<void *>(&mJoinerSession), aMaxLen, buf.size()); | ||
| mRecvBufs.pop(); |
There was a problem hiding this comment.
@rochaferraz we should really submit fixes like this in separate PRs. Otherwise, reviewers will be very likely going to ignore those changes.
3 participants
This PR introduces enhancements to the OT Commissioner, focusing on network discovery, improved diagnostics, and new utility scripts.
Core Enhancements:
traversenetworkcommand to the CLI to actively discover and map the Thread network topology.Traverserclass responsible for querying the leader, routers, and children to gather network diagnostics.--json <filename>) containing the collected network data and topology.DIAG_GET.ansmessages to support fragmented responses using theQuery IDandAnswerTLVs. This allows for the reassembly of large diagnostic reports split across multiple CoAP messages.NetDiagDatawith aMerge()function to combine data from multiple fragments.ClearNetDiagTlvs()to manage diagnostic state.CommissionerImplFragmentTest) to verify the reassembly logic for various scenarios.NetDiagData:NetDiagDatastructure now supports a wider range of TLVs, providing more detailed network information.SetTokento prevent potential issues with empty tokens.Other Changes: