-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[posix] Add POSIX Cli Daemon module #2676
base: main
Are you sure you want to change the base?
Conversation
a34aa8a
to
4cc806f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2676 +/- ##
===========================================
- Coverage 55.77% 43.41% -12.37%
===========================================
Files 87 110 +23
Lines 6890 13448 +6558
Branches 0 963 +963
===========================================
+ Hits 3843 5838 +1995
- Misses 3047 7303 +4256
- Partials 0 307 +307 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick implementation 👍
I have a few comments.
src/host/posix/daemon.cpp
Outdated
DieNow(otbrErrorString(OTBR_ERROR_ERRNO)); | ||
} | ||
|
||
// TODO:: Send Spinel message to NCP to initialize CLI (otCliInit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the CLI module on NCP should initialize at first and the initialization isn't triggered by and message from the host. Thoughts?
src/host/posix/daemon.cpp
Outdated
buffer[rval] = '\0'; | ||
|
||
otbrLogWarning("Received CLI command: %s", reinterpret_cast<char *>(buffer)); | ||
// TODO:: Send Spinel message to NCP to process the command (otCliInputLine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please abstract this process as an interface and this interface can be implememted by NcpSpinel
(or some classes under RCP mode in the future). In this way, we can use this module for both NCP and RCP (later we transition to use this module). In addition, we can have a test implementation of this interface and write unit tests.
You may refer to Netif::Dependencies and tests/gtest/test_netif.cpp. The scheme will be very similar.
Then please add a unit test similar as test_netif.cpp
. In the test, we simply create an instance of the Daemon class and a Mainloop. And we can also create a client socket and write cli commands to the socket. Then run the mainloop. Next we can check the call of our test implementation of the interface.
To run the gtest, build otbr-agent with -DBUILD_TESTING=ON
flag. (By default it should be turned on)
Then run ${YOUR_BUILD_DIR}/tests/gtest/${YOUR_GTEST_PROGRAM}
.
251470f
to
e74d5b0
Compare
e74d5b0
to
8a9e0cc
Compare
b133533
to
d95dc9c
Compare
d95dc9c
to
89c757f
Compare
de6e6e9
to
d0bdde4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks for breaking it into small ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
66e5031
to
b9057c5
Compare
b9057c5
to
b5e7645
Compare
This PR introduces the CLI daemon module for OTBR on POSIX in NCP mode. This initial implementation focuses on setting up the module and establishing the listen socket. Further CLI functionalities and integration with NcpHost will be implemented in subsequent PRs.