[network-diag] introduce AnswerBuilder to manage answer messages - #12887
Conversation
Summary of ChangesHello, 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 introduces a new 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. Footnotes
|
Library files
|
There was a problem hiding this comment.
Code Review
This pull request refactors the Network Diagnostic answer generation logic by introducing a new AnswerBuilder class to manage message allocation, segmentation, and TLV insertion. It also adds a MessageQueue::EnqueueAllFrom utility to the core message library to facilitate moving messages between queues. The review feedback suggests initializing the mQueryId member in the AnswerBuilder constructor to ensure defined behavior if the Query ID TLV is not found in the request.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12887 +/- ##
==========================================
- Coverage 74.11% 73.79% -0.33%
==========================================
Files 695 698 +3
Lines 95981 96448 +467
==========================================
+ Hits 71139 71169 +30
- Misses 24842 25279 +437
🚀 New features to boost your workflow:
|
44aa797 to
dd87c24
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Network Diagnostic answer generation logic by introducing a new AnswerBuilder class, which encapsulates the management of CoAP answer messages and TLVs. Additionally, it adds a MessageQueue::EnqueueAllFrom method to the core message library to allow moving all messages between queues, supported by new unit tests. The review feedback suggests adding a defensive check in the EnqueueAllFrom method to prevent potential circular references if a queue is enqueued into itself.
This commit introduces `AnswerBuilder` class to track and manage Network Diagnostic answer messages. This class is used when the response to a query requires multiple CoAP answer messages. It automatically manages the inclusion of the Query ID and the Answer TLVs(providing message indexing and "more-to-follow" flags) in each allocated answer message, while maintaining all answer messages in a queue. The `NetworkDiagnostic::Server` is updated to use the `AnswerBuilder`, simplifying the logic for preparing and sending answers. The `AnswerBuilder` class is added in a new header file `network_diagnostic_types.hpp` to allow for its reuse by other modules in the future.
…penthread#12887) This commit introduces `AnswerBuilder` class to track and manage Network Diagnostic answer messages. This class is used when the response to a query requires multiple CoAP answer messages. It automatically manages the inclusion of the Query ID and the Answer TLVs(providing message indexing and "more-to-follow" flags) in each allocated answer message, while maintaining all answer messages in a queue. The `NetworkDiagnostic::Server` is updated to use the `AnswerBuilder`, simplifying the logic for preparing and sending answers. The `AnswerBuilder` class is added in a new header file `network_diagnostic_types.hpp` to allow for its reuse by other modules in the future.
|
@jwhui could this break ot-br-posix rest API? here is what I get after pulling the new |
@AlixBa, it is very unlikely.
|
|
@abtink Thanks, I'll try to pinpoint then. I don't see any particular commit on ot-br-posix so I guess it's somewhere around here. I might not look at the right place though, haha. |
This commit introduces
AnswerBuilderclass to track and manage Network Diagnostic answer messages. This class is used when the response to a query requires multiple CoAP answer messages. It automatically manages the inclusion of the Query ID and the Answer TLVs(providing message indexing and "more-to-follow" flags) in each allocated answer message, while maintaining all answer messages in a queue. TheNetworkDiagnostic::Serveris updated to use theAnswerBuilder, simplifying the logic for preparing and sending answers.The
AnswerBuilderclass is added in a new header filenetwork_diagnostic_types.hppto allow for its reuse by other modules in the future.