Allow running OT-BR attached to a simulated RCP. #711
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. |
Summary of ChangesHello @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 significantly enhances the OpenThread Network Simulator (OTNS) by introducing the capability to simulate a real OpenThread Border Router (OTBR) agent. By attaching Highlights
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-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 pull request introduces support for running an OT-BR node attached to a simulated RCP, which is a great feature for testing real Thread mesh scenarios. The changes are extensive, touching Go, C, and build scripts. The core logic for integrating otbr-agent and ot-ctl seems well-thought-out. I've identified a couple of issues: a likely copy-paste error in the command runner that affects how OTBR RCP paths are displayed, and a potential race condition when starting the otbr-agent process due to a fixed-time sleep. Addressing these will make the implementation more robust. The refactoring to eliminate busy-loops in event processing is a significant improvement in efficiency. Overall, this is a solid contribution.
| cc.outputf("ot-rcp : %s\n", ec.Br) | ||
| cc.outputf("Executables search path: %s\n", ec.SearchPathsString()) | ||
| cc.outputf("Detected FTD path : %s\n", ec.FindExecutable(ec.Ftd)) | ||
| cc.outputf("Detected MTD path : %s\n", ec.FindExecutable(ec.Mtd)) | ||
| cc.outputf("Detected BR path : %s\n", ec.FindExecutable(ec.Br)) | ||
| cc.outputf("Detected OTBR RCP path : %s\n", ec.FindExecutable(ec.Br)) |
There was a problem hiding this comment.
It seems there's a copy-paste error here. The output for ot-rcp and Detected OTBR RCP path is using ec.Br instead of ec.Rcp. This will result in displaying the wrong executable path for the OTBR RCP.
| cc.outputf("ot-rcp : %s\n", ec.Br) | |
| cc.outputf("Executables search path: %s\n", ec.SearchPathsString()) | |
| cc.outputf("Detected FTD path : %s\n", ec.FindExecutable(ec.Ftd)) | |
| cc.outputf("Detected MTD path : %s\n", ec.FindExecutable(ec.Mtd)) | |
| cc.outputf("Detected BR path : %s\n", ec.FindExecutable(ec.Br)) | |
| cc.outputf("Detected OTBR RCP path : %s\n", ec.FindExecutable(ec.Br)) | |
| cc.outputf("ot-rcp : %s\n", ec.Rcp) | |
| cc.outputf("Executables search path: %s\n", ec.SearchPathsString()) | |
| cc.outputf("Detected FTD path : %s\n", ec.FindExecutable(ec.Ftd)) | |
| cc.outputf("Detected MTD path : %s\n", ec.FindExecutable(ec.Mtd)) | |
| cc.outputf("Detected BR path : %s\n", ec.FindExecutable(ec.Br)) | |
| cc.outputf("Detected OTBR RCP path : %s\n", ec.FindExecutable(ec.Rcp)) |
| logger.Errorf("could not start otbr-agent: %s", err) | ||
| return nil, err | ||
| } | ||
| time.Sleep(300 * time.Millisecond) |
There was a problem hiding this comment.
Using a fixed time.Sleep to wait for otbr-agent to start can be unreliable and may lead to race conditions, as the startup time can vary based on system load. A more robust approach would be to check if otbr-agent is ready before launching ot-ctl. For example, you could poll for the creation of the ot-ctl control socket or parse the otbr-agent logs for a specific message indicating it's ready.
It also requires a small modification to platform-rfsim.c since it uses the otIp6AddressFromString() but the OpenThread NCP does not link to the Ip6 library.
francoismichel
force-pushed
the
ot-br-simulated-rcp
branch
from
1b86351 to
cc55c2d
Compare
|
@francoismichel Thanks for submitting! I'd like to try it out soon. Some first thoughts:
Finally a meta thought: do we really need OTBR-agent from ot-br-posix? Current "BR" node type is based on OpenThread (core) only and would have nearly everything needed for an OTBR included, if we configure it to use OT-core mDNS/AdvProxy/DiscProxy I think.
|
|
@francoismichel Maybe if you have some time early next year (or this year), we could have a call to go over the questions I had. These questions are mostly on "could we do things differently" (in order to get certain benefits or simplify setup) or to understand the rationale for certain functions. Despite these questions being still open, I think the answers would imply that the current PR's approach is still a useful thing to add. Given that, I'll do suggestions also for the current PR code - hopefully still in this year! |
|
@francoismichel Based on the proposed changes here I'd like to propose that I create a separate PR first for adding generic RCP support, which re-introduces the "realtime" UART also. That would also allow running ordinary (e.g. FTD) In this new PR/design it's done in such a way that the internal OTNS code is more readable (less code) and the internals of the Dispatcher are not exposed to other classes. |
2 participants
This PR allows running the whole
otbr-agentexecutable and attach it to a simulated RCP so that it run in OTNS and be a border router for a real Thread mesh.There are a few points that probably remain to be discussed in this PR, but at least the code is available and works on Linux (Tested on Ubuntu 25).
Here are the points that remain to be discussed:
otbr-agentneeds to be pre-installed on the system, I felt lukewarm about adding ot-br as a dependency of OTNS, especially that it does not build entirely on all platforms.otbr-agentwhen clicking on the buttonuart.cfile inot-rfsim, so the code here is simply duplicated from OpenThread'suart.c. There may be better ways to reuse this code from OpenThread directly.OTOutFilter.gosince otbr-agent provides a real-time CLI through itsstdin.You should be able to test all this on a fresh ubuntu by:
node-mleidip addrwpan0interface, this is the ML-EID of the OTBR node, and therefore the ML-EID of your computer, we call itmy-mleidping -I <my-mleid> <node-mleid>. Don't forget the-Iswitch since it seems that Ubuntu can't pick the right IP address by itself.Let me know your comments and let's iterate on that PR.
Cheers !