Add support for Matter node

[bukepo]

This commit adds Matter node support, which expects the executable accepts an argument of --thread-args to be processed as the OpenThread simulator init arguments.

The driver is updated to provide Matter SDK needed select APIs. The UI is updated to provide a button to add "Matter" node.

[gemini-code-assist]

Summary of Changes

Hello @bukepo, I'm Gemini Code Assist¹! 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 expands the simulation platform by integrating comprehensive support for a new 'Matter' node type. It ensures that Matter nodes can be seamlessly added and configured through both the command-line and web interfaces, with appropriate executable management and argument parsing. Concurrently, the underlying simulation core receives several C code improvements, enhancing robustness and system event handling for a more stable and extensible platform.

Highlights

• Matter Node Support: Introduced a new 'Matter' node type, enabling its use within the simulation environment.
• CLI and Web UI Integration: The command-line interface and web user interface have been updated to allow adding and managing Matter nodes, including specific executable path configuration and command-line argument handling.
• Core Simulation Enhancements: Refactored parts of the ot-rfsim component, including improved macro definitions, type safety, and the addition of new system event handling functions (otSysUpdateEvents, otSysProcessEvents) for better integration with the operating system's event loop.

🧠 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 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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request adds support for a new 'Matter' node type across the CLI, simulation logic, and web UI. The changes are generally consistent and well-integrated. However, I've found two critical issues in the C code that will likely cause build failures or incorrect behavior. One is a syntax error in a preprocessor directive in ot-rfsim/src/openthread-core-rfsim-config.h, and the other is a logic error in timeout calculation in ot-rfsim/src/system.c. Please address these issues.

[bukepo]

/gemini review

[gemini-code-assist]

Code Review

This pull request adds support for Matter nodes, which is a significant feature enhancement. The changes span across the CLI, simulation logic, node configuration, and the web UI to provide a complete integration. The implementation appears solid and consistent with the existing architecture. I've found one area for improvement regarding code duplication in the C code, which I've commented on. Overall, this is a great contribution.

[EskoDijk]

@bukepo Great to get Matter support in! I'm currently updating code in similar locations as this PR, which is for the support of RCP nodes in realtime mode. Some updates to this or the other PR would be needed depending on which one goes first. It would be easiest I think if this one goes first.

It looks like the compiled Matter node will need to be an all-in-one (posix) program that can run in virtual-time, driven by OTNS, and uses virtual-time UART for CLI. And that it does not depend on a separate RCP process. And that it also supports the OpenThread CLI commands directly, correct? (This is somewhat surprising to me for a Matter application, but maybe this was explicitly made so that the node can run in OTNS?)

In the new function otSysUpdateEvents() we will need some comments included on how the overall design/architecture is intended to work. Right now, it's crucial that after sending the 'sleep' event the OT node actually goes to sleep immediately, waiting for a new event from OTNS to wake it again. In the new function, a sleep is sent but the code just continues and returns from the function - while OTNS assumes the node is doing nothing (asleep). This may lead to inconsistent behavior and seemingly deviates from the simulation architecture for virtual-time. Maybe we should first discuss this here and then decide on a good comment to add to explain this.
For example does the calling Matter node function go to sleep (to wait for next Unix socket event) if otSysUpdateEvents() does not set aTimeout to 0?

[gmarcosb]

What are these? Are they expected names for the binaries to be found in $PATH? Is there a way to have these configurable, i.e. to specify the values, i.e. full paths, when starting otns?

Having these as constants with a dependency on $PATH can make it awkward & difficult to use, vs being able to specify these in the command-line (having these as fallbacks for backwards-compatibility seems reasonable)

That could further make otns more flexible, i.e. if I could run it with something like
otns --new-config=Matter --config-exe='Matter:/path/to/my/exe', where --new-config is a dynamic name for the config + --config-exe then takes in that name, then you completely eliminate the need for code changes when adding new node types & allow whoever is running otns to set this up

[EskoDijk]

These are the default binary names - but they can be changed by the user; see https://github.com/openthread/ot-ns/blob/main/cli/README.md#exe

If the simulation is run from a Python script, it's easy to send some 'exe' commands and thereby change the defaults. But in interactive use, currently it would need to be copy-pasted in which is not ideal.

There are '-ot-cli' and '-ot-cli-mtd' arguments currently to change the executable name for 1 or 2 basic node types.

We can consider other ways too, such as loading from a YAML file (which is already supported for topology loading/saving, and for init-script configuration per node type, see https://github.com/openthread/ot-ns/blob/main/etc/cli-scripts/ot-script-example.yaml ). Or specifying an OTNS script that has some CLI commands executed at start. Or, some '--config' arguments: but these would look a bit messy if the user is setting up multiple of these profiles.

Also in the GUI, there could be buttons appearing for each defined custom profile.

[gmarcosb]

Ah that's perfect, so running something like otns add fed exe [matter exe] add fed exe [chip tool exe] will add 2 nodes through CLI (like from a python test run script?) - that's perfect!

I think all that's missing then is something like cliArg in addition to exe, which allows specifying how the args are passed to the exe, i.e. to reproduce the behavior here of --thread-args

YAML sounds good, as long as there's support to:

1. Set exe there
2. Set some new cliArg
3. Create a new config, e.g. Matter or GoogleHub

Effectively reproducing this PR but dynamically so that anyone can do this without having to add a new node type

Also, what do the node types mean? i.e. FED, MED... they don't seem to be documented here anywhere

[EskoDijk]

The 'pylibs' directory has some examples of Python scripting. One approach is to define the exe directly like:

ns.cmd('add router exe "./my_matter_cli"')
ns.cmd('add router exe "./my_chiptool_cli"')

and the other approach is to redefine the default executable so that for example clicking the 'Router' button will add your custom node type.

ns.cmd('exe ftd "./my_matter_cli"')

Since Routers and FEDs are both FTDs, any Router or FED added from that point on (also in the GUI) will get the custom node exe. The Thread spec is the main reference on the roles/types, but there's also some info here: https://openthread.io/guides/thread-primer/node-roles-and-types

Effectively reproducing this PR but dynamically so that anyone can do this without having to add a new node type

The PR also has changes to the OT-RFSIM platform layer. So in my view this is still ok to merge/include after review and deal with a better way for new config/preset features later on - created #751 for this.

[gmarcosb]

Agreed here, let's get the ot-rfsim stuff checked in & revert the rest

[gmarcosb]

This one wasn't addressed 😅

Don't know if it's worth the trouble reverting all the matter node changes, i.e. it doesn't harm anything but it just kinda unnecessarily clutters the code (whereas ext is more generic)

[gmarcosb]

Should this be #ifdef? Or a default defined somewhere?

Perhaps most importantly, how did the CI build not fail? 😅

[gmarcosb]

Ah nvm just found openthread/openthread#12345

[gmarcosb]

I realize not in this PR but as I'm trying to get this PR I'm running into issues; when starting the matter node, I am seeing a failure here, and it's unclear why as there's no docs & the expectation is counter-intuitive - why is it expected that the node isn't alive?

[gmarcosb]

It seems that the output of running otns ends up in 2 different paths - the Unix socket seems to end up in /tmp/otns, whereas the log output ends up in $PWD/tmp - is that intentional?

[EskoDijk]

... I am seeing a failure here, and it's unclear why as there's no docs & the expectation is counter-intuitive - why is it expected that the node isn't alive?

I did also comment on this PR on the 'sleep' design aspect: the new code called by the Matter node main loop does send a "sleep" event but the node does not stop processing thereafter, as it should. This might be the reason for this failure. (FYI CC @bukepo ) The design is currently:

1. node is started by otns;
2. otns receives all events from all nodes until no more node is alive - in RecvEvents(). This includes the startup event of the new node. The assert checks that the newly added node is also asleep, as a sanity check of the design principle.
3. otns proceeds with sending config commands to the nodes. Every time a command event is sent, the node is marked as "alive" again. Every time the node has processed and concludes with a sleep event in response, and the sleep event is responding to the most recent event that otns sent to the node, the node is marked asleep.

[gmarcosb]

@EskoDijk ah ok that makes sense - just some documentation may be helpful, then, as OTNS is very much a user tool, i.e. "use this tool to test your stuff", it's useful to know when/how "stuff" is misconfigured

So instead of an AssertFalse, an AssertWithMessage(isAsleep, "Node was sent a sleep event, should've fallen asleep, but didn't") (or whatever)

Another spot where I'm seeing this is an inability to look at the stdout for the started EXE (I'm adding code locally for this, which I can create a PR for)

I'll poke around to see why the node isn't asleep, wonder if I didn't configure virtual clock correctly

[EskoDijk]

It seems that the output of running otns ends up in 2 different paths - the Unix socket seems to end up in /tmp/otns, whereas the log output ends up in $PWD/tmp - is that intentional?

This was a deliberate choice: since the socket until now wasn't used by anyone except OTNS itself, and I didn't want it to clutter the directory with logs and flash files.

There's also an issue #684 that aims to make the ./tmp folder name be configurable and also save the PCAP and replay files in the same folder. That is useful for keeping all simulation output in one place; however the socket file doesn't have a function anymore after the simulation has ended so I would not immediately think we would need the socket file there (?)

[gmarcosb]

Not cluttering the directory totally makes sense, i.e. splitting these up into sub-paths /socket & /logs totally makes sense - but 2 completely different roots is kinda jarring & hard to find (especially with the same tmp name but different parents)

[EskoDijk]

@bukepo The PR #755 is now ready and allows any type of node (e.g. Matter) to be externally started and automatically added to a simulation. There's also documentation how to do it.

Perhaps you can check if this would impact the present PR. For example, it would not be strictly required anymore to support an "add matter" CLI command or a "add Matter" GUI button if the node can be externally started e.g. from a terminal or script. But the intended OT-RFSIM platform code changes may still be required.

( The issue #751 proposes to make it easier to add more "node types", also in the GUI. E.g. defined by a YAML config file defining the types. This could be an alternative way to get a "Matter" node into the GUI later on. However that would first require a PR made to implement what the issue #751 defines. )

[gmarcosb]

I think it makes sense to introduce a cfg.ThreadParamName which effectively allows dynamically setting a --thread-args, because obviously we can't expect an app to know that unnamed args are thread args when the app may have other args - the default would be empty, which would have the behavior of just passing args...

That seems generally useful for #751

[gmarcosb]

Addressing some outstanding comments (I can't mark resolved ones as resolved), this PR looks good & I confirm it works (with some additional changes to matter, for which I'll get a PR going)

[jwhui]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces support for a new Matter node type. The changes span across the command-line interface, simulation core, node configuration, and the web UI to fully integrate this new node. The implementation is consistent with how other node types are handled. The OpenThread RF simulator driver has also been updated to provide new select APIs required by the Matter SDK, and a minor bug related to signed/unsigned comparison has been fixed. The addition of a test case for the new node type and updates to the documentation are appreciated. Overall, this is a well-executed feature addition.