ot-br-posix icon indicating copy to clipboard operation
ot-br-posix copied to clipboard

[ncp] add initial support of NCP

Open Irving-cl opened this issue 1 year ago • 3 comments

This PR adds initial support of NCP in otbr-agent. When otbr-agent starts, it will detect the Co-processor type. If the Co-processor is NCP, otbr-agent will still run. But at this moment, it will provide a dbus API to get the device role of the NCP, which is 'Disabled' because no other functions are supported now.

This PR implements NcpSpinel which is the main class to communicate with the NCP. Different from RadioSpinel in openthread, NcpSpinel is totally async. There are no blocking operations like WaitResponse in RadioSpinel.

Currently when the Co-processor type is NCP, most modules in otbr-agent will be initialized and run. These may be added gradually later. And the dbus server also initiates differently when Co-processor type is NCP. When it's NCP, the dbus server registers AsyncGetPropertyHandler instead of GetPropertyHandler used now.

Depends-on openthread/openthread#10272

Irving-cl avatar May 11 '24 05:05 Irving-cl

@bukepo @zhanglongxia @abtink @superwhd @wgtdkp

I think the change is a non-trivial one. The PR is still under development. There are many things to do: adding test, refactoring. But I hope that I can get some feedback on the framework and the direction in early stage. PTAL when you have a moment.

Irving-cl avatar May 11 '24 05:05 Irving-cl

This PR is ready for review now. It seems to be difficult to break the PR down into smaller PRs. Here is an abstraction of what the PR does:

  1. Defined an interface (IControllerOpenThread) where we place the unified thread control APIs (for both NCP and RCP cases). In this PR, only one API (GetDeviceRole) is added for testing.
  2. The original core class ControllerOpenThread is renamed as ControllerOpenThreadRcp and refactor'ed as an implementation class of IControllerOpenThread. Most of its original methods still exist.
  3. Another class ControllerOpenThreadNcp is added to implement IControllerOpenThread and is used under NCP case.
  4. The dependencies on ControllerOpenThread of the current modules are not changed at this staged. For example, BackboneAgent, NdProxyManager, BorderAgent all depend on an instance of ControllerOpenThread. Now they still depend on ControllerOpenThreadRcp, instead of IControllerOpenThread. Currently these modules won't be initialized and started under NCP mode. We may change this when later working on enabling these modules under NCP mode.
  5. Refactor'ed the initialization process. Currently the modules hold a reference to ControllerOpenThread. And the reference is passed in the class constructors. In this PR, th reference is changed to pointer and pointer is passed in Init method because the instance is created later based on the co-processor type.
  6. Renamed the variable Ncp. Currently the variable name for an instance of ControllerOpenThread is mNcp or aNcp. This is confusing from my perspective. The PR changes all these names to Ctrlr (Controller).
  7. Added a test case to test the dbus API under NCP mode.

There is still a pending PR in openthread. Without it the CI here fails. Please take a loook when you have a chance. @zhanglongxia @abtink @superwhd

Irving-cl avatar May 20 '24 05:05 Irving-cl

Per discussion, the PR is still too large for review. I'll try to breakdown it.

The first breakdown PR is: #2294

Please review the simple one first. Thanks!

Irving-cl avatar May 22 '24 03:05 Irving-cl

The second breakdown PR is: #2299

Irving-cl avatar May 27 '24 04:05 Irving-cl

Codecov Report

Attention: Patch coverage is 11.76471% with 105 lines in your changes missing coverage. Please review.

Project coverage is 1.63%. Comparing base (2b41187) to head (f1ccb6a). Report is 703 commits behind head on main.

Files Patch % Lines
src/ncp/ncp_spinel.cpp 10.43% 103 Missing :warning:
src/ncp/ncp_host.cpp 66.66% 1 Missing :warning:
src/ncp/ncp_spinel.hpp 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #2283       +/-   ##
==========================================
- Coverage   55.77%   1.63%   -54.15%     
==========================================
  Files          87      91        +4     
  Lines        6890    9925     +3035     
  Branches        0     717      +717     
==========================================
- Hits         3843     162     -3681     
- Misses       3047    9756     +6709     
- Partials        0       7        +7     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 28 '24 02:06 codecov[bot]

This PR has been breakdown into a few PRs. Close this one.

Irving-cl avatar Jun 28 '24 03:06 Irving-cl