-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[api] add api to indicate if platform should request DHCPv6 PD #9775
base: main
Are you sure you want to change the base?
Conversation
Size Report of OpenThread
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9775 +/- ##
==========================================
- Coverage 85.65% 76.71% -8.94%
==========================================
Files 561 533 -28
Lines 74707 73887 -820
==========================================
- Hits 63991 56684 -7307
- Misses 10716 17203 +6487
|
591b427
to
c4b4a33
Compare
* @param[in] aShouldRequest Indicate if the platform need to request/release the prefix. | ||
* | ||
*/ | ||
extern void otPlatBorderRoutingShouldRequestDhcp6Pd(otInstance *aInstance, bool aShouldRequest); |
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.
This API seems to be a normal platform API so should not have extern
.
- A normal API is implemented by platform layer and OT stack will call it
- Use of
extern
in platform API means this is callback from platform to OT stack, i.e. OT stack implements and provides this API for the platform layer will call it
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.
This is an API for other teams to reference. Ideally there could be another APP/Process calling this API to get the state, or being notified, and then control the platform to request/release prefix.
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.
Should remove the extern
in the declaration.
Here is the policy generally followed in all platform headers:
- An
otPlat
API implemented by platform layer for use by OT stack should not be defined asextern
(e.g.otPlatRadioTransmit
). - An
otPlat
API implemented by OT stack for use by platform layer should be defined asextern
(e.g.,otPlatRadioTxDone
). These are often callbacks.
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.
This is an API for other teams to reference. Ideally there could be another APP/Process calling this API to get the state, or being notified, and then control the platform to request/release prefix.
No, I guess the app will implement this API to get the state.
For querying the state, the app should call otBorderRoutingDhcp6PdGetState
.
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.
Maybe I should rename the API to processStateChange()? My understanding is that different platforms will implement different methods to handle the prefix request/release
c4b4a33
to
a3d7dde
Compare
* @param[in] aRequest Indicate if the platform need to request/release the prefix. | ||
* | ||
*/ | ||
void otPlatBorderRoutingHandleDhcp6PdStateChange(otInstance *aInstance, bool aRequest); |
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.
If you use ...StateChange as the name of the API, I would prefer passing the state enum instead of a boolean to the platform.
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.
Also, I would suggest update the statement to include that:
- WHen the state is Running, the platform should request a prefix via DHCPv6 PD
- For other states, the platform should not request a prefix and release the DHCPv6 PD lease.
* @param[in] aRequest Indicate if the platform need to request/release the prefix. | ||
* | ||
*/ | ||
void otPlatBorderRoutingHandleDhcp6PdStateChange(otInstance *aInstance, bool aRequest); |
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.
It may be better to frame platform API name as a command that directly instruct the platform to perform specific actions. It is up to platform layer to decide how to implement this but it should be clear what we are asking platform to do.
- In this case, we are asking the platform to request or release DHCP6 Prefix
- I suggest
otPlatBorderRoutingRequestDhcp6Prefix()
andotPlatBorderRoutingReleaseDhcp6Prefix()
to be clear about the desired actions.
- I suggest
- Or we can alternatively think that we are enabling/disabling DHCP6 PD at the platform layer:
- So can use
otPlatBorderRoutingDhcp6PdSetEnabled(otInstance *, bool aEnable)
to convey this.
- So can use
The name otPlatBorderRoutingHandleDhcp6StateChange()
sound as if we are notifying platform instead of controlling/instructing it.
For querying the state, the app should call
otBorderRoutingDhcp6PdGetState
.
I would prefer passing the state enum instead of a boolean to the platform.
We have layered architecture:
- OpenThread (OT) Public APIs: These APIs provide control over the OT stack's behavior and allow higher-layer code to initiate specific functions/actions.
- OT Platform APIs: The OT stack utilizes these APIs to interact with and instruct the underlying platform layer.
It is always good to have layer separation and avoid cross-layer interactions.
- The
otBorderRoutingDhcp6PdGetState()
API (andotBorderRoutingDhcp6PdState
enum) are part of public OT APIs, and therefore primarily intended for consumption by applications and higher-layer components. - The platform layer implementation of Prefix Delegation (PD) should typically obtain the necessary information directly from its internal data and/or from
otPlat
API calls and not rely or use any public OT APIs.
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.
The platform doesn't need to care the state itself, it just need to be notified to request/release prefix, how about "otPlatBorderRoutingOnDhcp6PdStateChange()"
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.
OnDhcp6PdStateChange()
brings the question of "which state" and is notifying platform rather than instructing it to take an action.
This name reflects how the OT stack intends to use this API (i.e., whenever its internal PD prefix state gets changed) rather than what command/instruction is being asked of the platform layer to perform. Platform should not care how/what states are being tracked by OT stack (related to Prefix PD feature) and how the API will be used.
Also would be good to adhere to common OT API naming convention of "verb + [noun(s)]" to help improve consistency and readability. Example:otPlatRadioTransmit
, otPlatSettingsAdd
, otPlatInfraIfSendIcmp6Nd()
, otPlatBorderRoutingProcessIcmp6Ra()
, otPlatDsoEnableListening()
.
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.
otPlatBorderRoutingRequestDhcp6Pd()
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.
👍
a3d7dde
to
eab0734
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 👍
Hello All,
|
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 @sherysheng for all changes.
Couple of smaller suggestions blow and questions on order of operation and providing weak implementation
@@ -3762,6 +3762,12 @@ extern "C" void otPlatBorderRoutingProcessIcmp6Ra(otInstance *aInstance, const u | |||
{ | |||
AsCoreType(aInstance).Get<BorderRouter::RoutingManager>().ProcessPlatformGeneratedRa(aMessage, aLength); | |||
} | |||
|
|||
extern "C" OT_TOOL_WEAK void otPlatBorderRoutingRequestDhcp6Pd(otInstance *aInstance, bool aRequest) |
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.
Do we need this weak implementation in OT stack?
Usually we provide weak implementation if the the API is optional for platform (platform can choose to not provide it)? Is this the case here? I am not sure?
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.
Providing empty weak implementation of platform API in OT core mask potential errors (e.g., incorrect implementation using wrong name or wrong param by platform layer).
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.
In this case, some platforms need to implement this API to control it's platform PD, while some devices don't need to implement this API(eg. some CPE). So I think define a WEAK is more feasible here.
For the incorrect implementations, I think it's out of our control, we've already provided the usage guide in the API description, developers should just follow it, if they misunderstood it they can come and ask us?
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.
Providing empty weak implementation of platform API in OT core mask potential errors (e.g., incorrect implementation using wrong name or wrong param by platform layer).
Makes sense.
I'm thinking if it is "optional" or it "can have an empty implementation on some platforms". I can see some CPE devices in the market does not have a DHCPv6 PD server embedded, and if they integrate the PD feature, they will use a static prefix allocation, and they can assume there are no second BR in the network that provides GUA prefixes (so they don't have to withdrawn the prefix, and nowhere to return the lease).
Previously I inteprete the above seenrio as "optional", but now I think maybe a macro like "OPENTHREAD_BORDER_ROUTING_EMPTY_DHCP6_PD_REQUESTOR
" (or other names) to treat this as a "special case" instead of a "optional API" since the above case is not a standard path?
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.
In this case, some platforms need to implement this API to control it's platform PD, while some devices don't need to implement this API(eg. some CPE). So I think define a WEAK is more feasible here.
I think we can leave this to platform, if the platform has nothing to do, they can provide empty implementation of this API.
For the incorrect implementations, I think it's out of our control, we've already provided the usage guide in the API description, developers should just follow it, if they misunderstood it they can come and ask us?
The difference is when we provide a weak implementation, and platform defines improper name/format for the API, the build will succeed and we then need to debug the issue at run-time. But if we don't provide a weak implementation, we get an error at build time which is much easier to fix.
Previously I inteprete the above seenrio as "optional", but now I think maybe a macro like "OPENTHREAD_BORDER_ROUTING_EMPTY_DHCP6_PD_REQUESTOR" (or other names) to treat this as a "special case" instead of a "optional API" since the above case is not a standard path?
Adding a config for this make sense, but may be better to add it in platform. Since such a config is really confiuguration the platform behavior and not core OT stack BorderRouting
behavior. We can consider adding such a config in posix
platform.
Here are my suggestions on this:
- From OT platform API, we require this API to be provided by platform when
BORDER_ROUTING_DHCP6_PD_ENABLE
is enabled (dont treat as optional). - We leave it up to platform to decide whether to provide an empty implementation or do something.
- We can add the empty implementation of this API in
posix
platform source files- And an add a config in
POSIX
to indicate whetherposix
source code should provide an empty implementation for it and leave it to be defined byot-br-posix
module.
- And an add a config in
Thoughst?
Moving the discussion here: https://github.com/orgs/openthread/discussions/9784 |
eab0734
to
c90d8f2
Compare
c90d8f2
to
98c7b62
Compare
@@ -65,6 +65,22 @@ extern "C" { | |||
*/ | |||
extern void otPlatBorderRoutingProcessIcmp6Ra(otInstance *aInstance, const uint8_t *aMessage, uint16_t aLength); | |||
|
|||
#if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE | |||
#if !OPENTHREAD_BORDER_ROUTING_DHCP6_PD_REQUESTOR |
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.
We should not add any #if
in any of public/platform headers (since they may be included in other projects/libraries and wont see the same set of configs as OT core stack).
We instead document that the public API is available or platform API is used when certain OPENTHREAD_CONFIG
is enabled
@@ -3601,6 +3600,11 @@ void RoutingManager::PdPrefixManager::EvaluateStateChange(Dhcp6PdState aOldState | |||
case kDhcp6PdStateRunning: | |||
break; | |||
} | |||
#if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE | |||
#if !OPENTHREAD_BORDER_ROUTING_DHCP6_PD_REQUESTOR |
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 guess OPENTHREAD_BORDER_ROUTING_DHCP6_PD_REQUESTOR
is a new config?
Let's use OPENTHREAD_CONFIG_...
prefix for it?
Also let's define it in the config/border_routing.h
header (for its default value) and document its function.
@@ -3601,6 +3600,11 @@ void RoutingManager::PdPrefixManager::EvaluateStateChange(Dhcp6PdState aOldState | |||
case kDhcp6PdStateRunning: | |||
break; | |||
} | |||
#if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE |
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 whole PdPrefixManager
class and all its methods are under a #if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
block.
So dont need to add this #if
check again here?
@@ -3762,6 +3766,15 @@ extern "C" void otPlatBorderRoutingProcessIcmp6Ra(otInstance *aInstance, const u | |||
{ | |||
AsCoreType(aInstance).Get<BorderRouter::RoutingManager>().ProcessPlatformGeneratedRa(aMessage, aLength); | |||
} | |||
|
|||
#if !OPENTHREAD_BORDER_ROUTING_DHCP6_PD_REQUESTOR | |||
extern "C" void otPlatBorderRoutingRequestDhcp6Pd(otInstance *aInstance, bool aRequest) |
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.
Should we provide an empty implementation?
Add API to notify the platform whether they should request an prefix via DHCPv6 PD for Thread interface.
When aShouldRequest is false, the platform must release the prefix lease.