OTNS integration#43478
Conversation
This reverts commit d8ab9ded4324abb13a29aa318ce18b2216532c7e.
…hip-tool can provision directly through OTNS
| "repo/src/library/udp_proxy.cpp", | ||
| ] | ||
| deps = [ ":ot_commissioner_mbedtls" ] | ||
| if (chip_enable_otns) { |
There was a problem hiding this comment.
@bukepo I am having a hard time figuring out how to get chip-tool to compile with the third_party/openthread/platforms:libopenthread-platform dep, as we run into duplicate symbols for mbedtls
Is my dependency wrong, i.e. does chip-tool not need the things in chip_enable_otns (in examples/platform/linux/BUILD.gn)?
There was a problem hiding this comment.
Think I figured it out... I guess this is for off-mesh, and my goal is to get chip-tool to be on-mesh so that we have 0 dependency on external network & are fully within OTNS
So I think I want to use https://github.com/openthread/openthread/blob/main/include/openthread/commissioner.h instead?
There was a problem hiding this comment.
Code Review
This pull request integrates OTNS (OpenThread Network Simulator) into the build and test system. The changes are extensive, touching build scripts, application options, platform code, and test infrastructure. The core idea is to allow running and testing Matter-over-Thread devices in a simulated network environment provided by OTNS. While the overall direction is good, I've found several critical issues in the Python test infrastructure code that will likely cause tests to fail, particularly around how command-line arguments are constructed for subprocesses. There are also some minor issues with logging levels and code duplication that should be addressed. One comment has been enhanced with a reference to a code duplication rule.
| def apply_args(self, process: SubprocessInfo) -> SubprocessInfo: | ||
| return process.with_args("--thread-args=2") |
There was a problem hiding this comment.
The with_args method replaces all existing arguments on the SubprocessInfo object. This implementation discards any arguments that were previously set on the process object, which will lead to incorrect behavior. The arguments should be appended instead of replaced.
| def apply_args(self, process: SubprocessInfo) -> SubprocessInfo: | |
| return process.with_args("--thread-args=2") | |
| def apply_args(self, process: SubprocessInfo) -> SubprocessInfo: | |
| return process.with_args(*process.args, "--thread-args=2") |
| def apply_pairing_args(self, process: SubprocessInfo) -> SubprocessInfo: | ||
| if self._meshcop: | ||
| return process.with_args("pairing", "thread-meshcop", "--thread-ba-host", self.get_border_agent_host(), "--thread-ba-port", str(self.get_border_agent_port())) | ||
| else: | ||
| return process.with_args("pairing", "code-thread") |
There was a problem hiding this comment.
The with_args method replaces all existing arguments. When this method is called from test_definition.py, the process object already contains necessary arguments like node ID and dataset. This implementation discards them, resulting in an incomplete or incorrect pairing command. The logic for constructing the pairing command needs to be revised to preserve all necessary arguments.
| def apply_args(self, process: SubprocessInfo) -> SubprocessInfo: | ||
| socket = self._otns.get_otns_socket() | ||
| process = process.with_args("--thread-args=2") | ||
| process = process.with_args(f"--thread-args={socket}") | ||
| return process.with_args(f"--thread-args={ThreadOtns.RANDOM_SEED}") |
There was a problem hiding this comment.
The with_args method replaces existing arguments, but it is called multiple times here. Each call overwrites the arguments from the previous one, and also discards any arguments the process object already had. As a result, only the last argument (--thread-args={ThreadOtns.RANDOM_SEED}) will be passed. All arguments should be appended to the existing ones.
| def apply_args(self, process: SubprocessInfo) -> SubprocessInfo: | |
| socket = self._otns.get_otns_socket() | |
| process = process.with_args("--thread-args=2") | |
| process = process.with_args(f"--thread-args={socket}") | |
| return process.with_args(f"--thread-args={ThreadOtns.RANDOM_SEED}") | |
| def apply_args(self, process: SubprocessInfo) -> SubprocessInfo: | |
| socket = self._otns.get_otns_socket() | |
| return process.with_args(*process.args, "--thread-args=2", f"--thread-args={socket}", f"--thread-args={ThreadOtns.RANDOM_SEED}") |
| def apply_pairing_args(self, process: SubprocessInfo) -> SubprocessInfo: | ||
| return process.with_args("pairing", "thread-meshcop", "--thread-ba-host", self.get_border_agent_host(), "--thread-ba-port", str(self.get_border_agent_port())) |
There was a problem hiding this comment.
| Microseconds64 GetMonotonicMicroseconds64() override { return Microseconds64(otPlatTimeGet()); } | ||
| Milliseconds64 GetMonotonicMilliseconds64() override { return std::chrono::duration_cast<Milliseconds64>(Microseconds64(otPlatTimeGet())); } | ||
|
|
||
| CHIP_ERROR GetClock_RealTime(Microseconds64 & aCurTime) override | ||
| { | ||
| aCurTime = Microseconds64(otPlatTimeGet()); | ||
| return CHIP_NO_ERROR; | ||
| } | ||
| CHIP_ERROR GetClock_RealTimeMS(Milliseconds64 & aCurTime) override | ||
| { | ||
| aCurTime = std::chrono::duration_cast<Milliseconds64>(Microseconds64(otPlatTimeGet())); | ||
| return CHIP_NO_ERROR; | ||
| } |
There was a problem hiding this comment.
There's some code duplication in the MockClock implementation. The same call to otPlatTimeGet() and the same duration_cast logic are repeated across multiple methods. You can improve readability and maintainability by reusing the GetMonotonic... methods within the GetClock_RealTime... methods.
Microseconds64 GetMonotonicMicroseconds64() override { return Microseconds64(otPlatTimeGet()); }
Milliseconds64 GetMonotonicMilliseconds64() override { return std::chrono::duration_cast<Milliseconds64>(GetMonotonicMicroseconds64()); }
CHIP_ERROR GetClock_RealTime(Microseconds64 & aCurTime) override
{
aCurTime = GetMonotonicMicroseconds64();
return CHIP_NO_ERROR;
}
CHIP_ERROR GetClock_RealTimeMS(Milliseconds64 & aCurTime) override
{
aCurTime = GetMonotonicMilliseconds64();
return CHIP_NO_ERROR;
}References
- To improve maintainability and reduce code duplication, extract common logic from similar methods into private helper functions.
| stdin: IO[Any] | None = None) -> tuple[subprocess.Popen[bytes], LogPipe, LogPipe]: | ||
| cmd = subproc.to_cmd() | ||
| log.info('RunSubprocess starting application %s', " ".join(cmd)) | ||
| log.error('RunSubprocess starting application %s', " ".join(cmd)) |
There was a problem hiding this comment.
This log message appears to have been changed to error level, likely for debugging. This should be reverted to info or debug to avoid flooding the logs with errors during normal test execution.
| log.error('RunSubprocess starting application %s', " ".join(cmd)) | |
| log.info('RunSubprocess starting application %s', " ".join(cmd)) |
| log.error("Pairing command: %s", shlex.join(pairing_cmd.to_cmd())) | ||
| log.error("Testcase command: %s", shlex.join(test_cmd.to_cmd())) |
There was a problem hiding this comment.
These log messages appear to have been changed to error level, likely for debugging purposes. Please consider changing them to a more appropriate level, such as info or debug, to avoid unnecessary error output during test runs.
| log.error("Pairing command: %s", shlex.join(pairing_cmd.to_cmd())) | |
| log.error("Testcase command: %s", shlex.join(test_cmd.to_cmd())) | |
| log.info("Pairing command: %s", shlex.join(pairing_cmd.to_cmd())) | |
| log.info("Testcase command: %s", shlex.join(test_cmd.to_cmd())) |
Summary
Continue work from #42841 to get OTNS to work such that chip-tool can provision directly through OTNS
To build, use:
To run the app as an external node in OTNS (requires openthread/ot-ns#755), after starting otns, run:
The next step is to get chip-tool to build:
And finally, the commissioning test to pass using OTNS:
Related issues
Testing
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines