Skip to content
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

[controller] allows polymorphism of the controller #2309

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

Irving-cl
Copy link
Contributor

@Irving-cl Irving-cl commented Jun 3, 2024

This PR is also a breakdown of #2283

This PR adds a factory method CreateInstance to provide polymorphism
for the ThreadController. This PR moves the construction and initialization
of the components to RCP specific methods and only contructs these
modules when the Co-processor is an RCP.

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 74.24242% with 17 lines in your changes missing coverage. Please review.

Project coverage is 39.91%. Comparing base (2b41187) to head (fac7dd6).
Report is 683 commits behind head on main.

Files Patch % Lines
src/ncp/thread_controller.cpp 51.72% 13 Missing and 1 partial ⚠️
src/agent/application.cpp 88.46% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2309       +/-   ##
===========================================
- Coverage   55.77%   39.91%   -15.87%     
===========================================
  Files          87       91        +4     
  Lines        6890     9897     +3007     
  Branches        0      732      +732     
===========================================
+ Hits         3843     3950      +107     
- Misses       3047     5745     +2698     
- Partials        0      202      +202     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Irving-cl Irving-cl force-pushed the controller_polymorphism branch 2 times, most recently from 2b267de to 9a9d979 Compare June 3, 2024 04:52
@Irving-cl Irving-cl marked this pull request as draft June 3, 2024 04:59
@Irving-cl Irving-cl force-pushed the controller_polymorphism branch 4 times, most recently from 544c54d to 6e6a9e7 Compare June 3, 2024 08:13
@Irving-cl Irving-cl requested a review from superwhd June 3, 2024 08:28
@Irving-cl Irving-cl marked this pull request as ready for review June 3, 2024 08:28
src/trel_dnssd/trel_dnssd.hpp Outdated Show resolved Hide resolved
src/trel_dnssd/trel_dnssd.cpp Outdated Show resolved Hide resolved
src/ncp/thread_controller.cpp Outdated Show resolved Hide resolved
@@ -194,9 +188,19 @@ class RcpHost : public MainloopProcessor, public ThreadController

~RcpHost(void) override;

// Thread Control APIs
// Thread Control virtual methods
void GetDeviceRole(const DeviceRoleHandler aHandler) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in a future PR: make this parameter either a non-const value or a const reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. Let's do it in a seperate PR.

src/agent/application.cpp Show resolved Hide resolved
src/ncp/thread_controller.hpp Outdated Show resolved Hide resolved
src/ncp/rcp_host.hpp Outdated Show resolved Hide resolved
src/ncp/thread_controller.cpp Outdated Show resolved Hide resolved
src/ncp/thread_controller.hpp Outdated Show resolved Hide resolved
src/ncp/thread_controller.hpp Outdated Show resolved Hide resolved
@Irving-cl Irving-cl force-pushed the controller_polymorphism branch 3 times, most recently from e7f80e8 to 0eba402 Compare June 5, 2024 03:34
@Irving-cl Irving-cl force-pushed the controller_polymorphism branch 5 times, most recently from 302de00 to d083088 Compare June 7, 2024 06:22
@Irving-cl Irving-cl requested a review from superwhd June 7, 2024 06:24
Copy link
Contributor

@superwhd superwhd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall

src/common/code_utils.hpp Outdated Show resolved Hide resolved
src/ncp/thread_controller.hpp Outdated Show resolved Hide resolved
src/ncp/thread_controller.hpp Outdated Show resolved Hide resolved
Copy link
Member

@abtink abtink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Irving-cl Irving-cl requested a review from jwhui June 12, 2024 00:44
src/ncp/thread_controller.hpp Outdated Show resolved Hide resolved
src/agent/application.cpp Outdated Show resolved Hide resolved
src/agent/application.cpp Outdated Show resolved Hide resolved
@jwhui jwhui merged commit 0c034e6 into openthread:main Jun 13, 2024
30 checks passed
@Irving-cl Irving-cl deleted the controller_polymorphism branch June 25, 2024 06:27
@Irving-cl Irving-cl mentioned this pull request Jul 23, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants