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

[thread-host] implement join for rcp host #2631

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

Irving-cl
Copy link
Contributor

This PR implements the RcpHost::Join method.

The implementation is exactly the same as Android binder API's implementation. This PR also adds a unit test to check the Join method.

@Irving-cl Irving-cl requested a review from sunytt December 4, 2024 11:36
@Irving-cl Irving-cl marked this pull request as ready for review December 4, 2024 11:36
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 75.37313% with 33 lines in your changes missing coverage. Please review.

Project coverage is 46.21%. Comparing base (2b41187) to head (59d2a90).
Report is 897 commits behind head on main.

Files with missing lines Patch % Lines
tests/gtest/test_rcp_host_api.cpp 78.88% 19 Missing ⚠️
src/ncp/rcp_host.cpp 68.18% 10 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2631      +/-   ##
==========================================
- Coverage   55.77%   46.21%   -9.57%     
==========================================
  Files          87      106      +19     
  Lines        6890    12984    +6094     
  Branches        0      933     +933     
==========================================
+ Hits         3843     6000    +2157     
- Misses       3047     6658    +3611     
- Partials        0      326     +326     

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

@Irving-cl Irving-cl force-pushed the add_thread_host_join branch from 54d9772 to 2f13cbe Compare December 4, 2024 12:01
tests/gtest/test_rcp_host_api.cpp Show resolved Hide resolved
src/ncp/rcp_host.cpp Show resolved Hide resolved
src/ncp/rcp_host.cpp Outdated Show resolved Hide resolved
@Irving-cl Irving-cl force-pushed the add_thread_host_join branch from 2f13cbe to 437dcd5 Compare December 5, 2024 05:59
@Irving-cl Irving-cl requested a review from sunytt December 5, 2024 06:01
@Irving-cl Irving-cl force-pushed the add_thread_host_join branch 3 times, most recently from b5ad095 to d446fab Compare December 5, 2024 10:53
Copy link
Contributor

@sunytt sunytt 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 December 6, 2024 01:57
src/ncp/rcp_host.cpp Show resolved Hide resolved
src/ncp/rcp_host.cpp Outdated Show resolved Hide resolved
@jwhui jwhui requested a review from abtink December 6, 2024 16:46
@Irving-cl Irving-cl force-pushed the add_thread_host_join branch from d446fab to b0d3f6c Compare December 10, 2024 13:35
@Irving-cl Irving-cl requested a review from jwhui December 10, 2024 13:40
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.

Looks good. Couple of questions below. Thanks

src/ncp/rcp_host.cpp Outdated Show resolved Hide resolved
src/ncp/rcp_host.cpp Show resolved Hide resolved
// unnecessary connectivity and topology disruption and save the time for re-joining. It's more useful for use
// cases where Thread networks are dynamically brought up and torn down (e.g. Thread on mobile phones).
SuccessOrExit(error = otDatasetSetActiveTlvs(mInstance, &aActiveOpDatasetTlvs),
errorMsg = "Failed to set Active Operational Dataset");
Copy link
Member

Choose a reason for hiding this comment

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

If already joined to same network, do we need to set the active dataset again?
This can trigger registration/resync of dataset with leader? Is this desired/interntional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is out of consideration for unknown TLVs in the dataset. It's very difficult to check if two datasets are completely the same with the existence of unknown TLVs (because of the order). So we simply set the active dataset again. But we want to avoid rejoin as possible. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think it sounds okay to do so. The leader should combine them. This may add some extra traffic/exchanges when the parameters fully match, but I expect it should be harmless otherwise.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@Irving-cl Irving-cl force-pushed the add_thread_host_join branch from b0d3f6c to 59d2a90 Compare December 12, 2024 02:52
@Irving-cl Irving-cl requested a review from abtink December 12, 2024 02:54
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.

👍

@jwhui jwhui changed the title [thread host] implement join for rcp host [thread-host] implement join for rcp host Dec 12, 2024
@jwhui jwhui merged commit 283066e into openthread:main Dec 12, 2024
32 checks passed
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