[rcp] add support for OTNS - #11637
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @francoismichel, 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 support for OpenThread Network Simulator (OTNS) within the Radio Co-Processor (RCP) build configuration. The primary goal is to enable OpenThread Border Routers (OTBR) to seamlessly integrate with OTNS by allowing the RCP to emit necessary events for simulation, ensuring proper behavior and visibility in the simulated environment.
Highlights
- OTNS Support for RCP: Extended the
Utils::Otnsmodule to be compiled and utilized when OpenThread is built as a Radio Co-Processor (OPENTHREAD_RADIO), enabling OTBR to seamlessly integrate with OTNS. - Conditional OTNS Features: Refactored the
Utils::Otnsclass to conditionally compile certain features (like IPv6-related ping events, CoAP events, and Thread-specific events) only when building for MTD (Minimal Thread Device) or FTD (Full Thread Device) modes, as these features are not relevant or available in the RCP build. - RCP Event Emission: Modified the radio layer to ensure that the RCP emits essential events, such as extended address, short address, and transmit events, to OTNS when enabled, providing necessary visibility for simulation.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
Thank you for this contribution. This pull request successfully adds support for OTNS in RCP mode, which will be a valuable addition for testing OTBR with OTNS.
The changes are well-structured and correctly implemented. You've properly updated the build files to include otns.cpp for RCP builds, modified the Instance class to include the Otns member in RCP mode, and used conditional compilation to exclude FTD/MTD-specific functionality from otns.cpp and otns.hpp when building for RCP. The code appears correct and robust.
Overall, this is a great addition that will improve testing capabilities with OTNS. I have no further comments.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for OTNS when compiling the RCP mode by updating preprocessor conditions, extending the Instance class, and incorporating new OTNS functions. Key changes include:
- Updating preprocessor guards in multiple files to include OPENTHREAD_RADIO.
- Adding a new EmitTransmit function with its corresponding implementation.
- Modifying Instance construction to instantiate an mOtns member and updating the build to include otns.cpp.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/utils/otns.hpp | Updated preprocessor conditions and reordered includes; added EmitTransmit declaration. |
| src/core/utils/otns.cpp | Updated preprocessor conditions; added implementation for EmitTransmit and moved OTNS‐guarded ping functions. |
| src/core/radio/radio.cpp | Adjusted preprocessor guards for OTNS event emission in radio operations. |
| src/core/instance/instance.hpp | Added and reconditioned the mOtns member with updated preprocessor guards. |
| src/core/instance/instance.cpp | Updated constructor initialization for mOtns per revised preprocessor conditions. |
| src/core/CMakeLists.txt | Added otns.cpp to the RCP build sources. |
Size Report of OpenThread
|
abtink
left a comment
abtink
left a comment
There was a problem hiding this comment.
Thanks @francoismichel for the PR. Looks good overall.
Couple of smaller suggestions below:
EskoDijk
left a comment
EskoDijk
left a comment
There was a problem hiding this comment.
Thanks @francoismichel ! Agree with Abtin's comments. Looks good for the rest.
Size Report of OpenThreadMerging PR into main
Library files
|
51154b3 to
9640cc8
Compare
Library files
|
|
I applied Abtin and Esko's suggestions and rebase the branch. Let me know what you think! |
EskoDijk
left a comment
EskoDijk
left a comment
There was a problem hiding this comment.
Looks ok now - the CLA check looks to be failing, so @francoismichel you may need to check if you need to/can sign the CLA?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11637 +/- ##
==========================================
- Coverage 75.91% 72.29% -3.62%
==========================================
Files 658 656 -2
Lines 95991 96313 +322
==========================================
- Hits 72867 69630 -3237
- Misses 23124 26683 +3559
🚀 New features to boost your workflow:
|
9b52bb2 to
30b2f85
Compare
|
I'm rebasing and doing a few tests to ensure everything works. |
213fc73 to
29fb6d9
Compare
|
@jwhui I confirm that it works well on a Linux VM. Could you re-trigger the tests here ? It seems I don't have the right to re-trigger the tests myself. |
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request correctly enables OpenThread Network Simulator (OTNS) support for the Radio Co-processor (RCP) build configuration. The changes primarily involve refactoring preprocessor directives to conditionally compile OTNS-related code, moving MTD/FTD-specific functionalities into their own compilation guards, and ensuring that RCP builds include the necessary components for OTNS integration. The code modifications are clean, logical, and effectively achieve the stated goal. I have a couple of minor suggestions to improve code consistency.
abtink
left a comment
abtink
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks @francoismichel
Couple of smaller suggestions belwo:
4989e21 to
e4b83e8
Compare
EskoDijk
left a comment
EskoDijk
left a comment
There was a problem hiding this comment.
Thanks for the updates!
This commit allows compiling the OpenThread RCP in OTNS mode. This allows OTBR to attach seamlessly to OTNS. * Add the mOtns attribute to Instance::Instance when compiled with OPENTHREAD_RADIO=1 * Add src/core/utils/otns.cpp to the RCP build. Only the necessary functions are compiled in RCP mode. Other functions such as EmitPingRequest and EmitPingReply that require IPv6 stack support are not compiled in RCP and are therefore moved into adequate `#if OPENTHREAD_MTD || OPENTHREAD_FTD` guards. * Ensure the RCP emits the needed events in src/core/radio/radio.cpp when compiled with OTNS support.
4a1ec8f to
8fdd67f
Compare
Hi folks,
This PR allows compiling the RCP in OTNS mode. This will allow OTBR to attach seamlessly to OTNS.
Once these changes are part of the OpenThread upstream, I will be able to submit a PR on OTNS that makes use of this.
Changes:
mOtnsattribute toInstance::Instancewhen compiled withOPENTHREAD_RADIO=1src/core/utils/otns.cppto the RCP build. Only the necessary functions are compiled in RCP mode. Other functions such asEmitPingRequestandEmitPingReplythat require IPv6 stack support are not compiled in RCP and are therefore moved into adequate#if OPENTHREAD_MTD || OPENTHREAD_FTDguards.src/core/radio/radio.cppwhen compiled with OTNS support.