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

[host] add network properties class #2387

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

Irving-cl
Copy link
Contributor

@Irving-cl Irving-cl commented Jul 18, 2024

This PR adds a new interface NetworkProperties to provide a unified, synchronized way to access network properties for both NCP and RCP.

  • For RCP, the property getters are implemented by OT APIs with an OT instance.
  • For NCP, the property getters are implemented by the replica of the data that comes from NCP.

This is important for adding NCP support because in some cases the network properties are required by some functionality and we don't want to make an asynchronous call simply to get the property. As one example, in AdvertisingProxy, we need to the MeshLocal Prefix to check if an address is mesh local address. Currently this is done by OT API, which is not available under NCP mode. However, the information is contained in the active dataset and NCP will always notify the host about the change. With NetworkProperties, we can always get these properties in a synchronized way.

This PR also replaces a few usage of otThreadGetDeviceRole by NetworkProperties. In this way we will gradually remove the direct usage of OT APIs and mInstance in ot-br-posix modules. In this way we can develop for both NCP&RCP more easily.

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 60.60606% with 13 lines in your changes missing coverage. Please review.

Project coverage is 45.37%. Comparing base (2b41187) to head (51e700e).
Report is 734 commits behind head on main.

Files Patch % Lines
src/ncp/ncp_host.cpp 42.85% 4 Missing ⚠️
src/ncp/ncp_spinel.cpp 57.14% 3 Missing ⚠️
src/dbus/server/dbus_thread_object_ncp.cpp 0.00% 2 Missing ⚠️
src/utils/thread_helper.cpp 50.00% 2 Missing ⚠️
src/ncp/ncp_spinel.hpp 0.00% 1 Missing ⚠️
src/ncp/rcp_host.cpp 90.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2387       +/-   ##
===========================================
- Coverage   55.77%   45.37%   -10.41%     
===========================================
  Files          87       94        +7     
  Lines        6890    10590     +3700     
  Branches        0      769      +769     
===========================================
+ Hits         3843     4805      +962     
- Misses       3047     5546     +2499     
- Partials        0      239      +239     

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

@Irving-cl Irving-cl marked this pull request as ready for review July 18, 2024 15:14
@superwhd
Copy link
Contributor

With NetworkProperties, we can always get these properties in a synchronized way.

The idea is to maintain a cache of the properties at the host (by receiving updates from the callbacks). Right?

I think 'synchronized' may not be accurate here.

src/ncp/ncp_host.cpp Outdated Show resolved Hide resolved
src/ncp/rcp_host.cpp Outdated Show resolved Hide resolved
src/ncp/rcp_host.cpp Outdated Show resolved Hide resolved
src/ncp/ncp_spinel.cpp Outdated Show resolved Hide resolved
@Irving-cl Irving-cl requested a review from superwhd July 19, 2024 09:39
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 👍

@Irving-cl Irving-cl force-pushed the add_network_properties branch 2 times, most recently from ffb4ed1 to 24edda5 Compare July 23, 2024 09:46
Copy link
Contributor

@zhanglongxia zhanglongxia left a comment

Choose a reason for hiding this comment

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

LGTM

src/ncp/ncp_host.cpp Outdated Show resolved Hide resolved
src/ncp/ncp_spinel.cpp Outdated Show resolved Hide resolved
src/ncp/ncp_spinel.cpp 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.

Looks great. Thanks @Irving-cl.

One (optional) suggestion below (above) 😄

@Irving-cl Irving-cl force-pushed the add_network_properties branch 2 times, most recently from 93a5832 to c19aa5f Compare July 26, 2024 03:10
@jwhui jwhui merged commit 87b671d into openthread:main Jul 30, 2024
32 checks passed
@Irving-cl Irving-cl mentioned this pull request Jul 31, 2024
16 tasks
@Irving-cl Irving-cl deleted the add_network_properties branch August 22, 2024 05:58
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.

None yet

6 participants