[api] add api to indicate if platform should request DHCPv6 PD #9775

sherysheng · 2024-01-16T07:52:52Z

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.

size-report · 2024-01-16T07:57:37Z

Size Report of OpenThread

Merging #9775 into main(00076af).

name	branch	text	data	bss	total
ot-cli-ftd	main	464256	760	66252	531268
	#9775	464256	760	66252	531268
	+/-	0	0	0	0
ot-ncp-ftd	main	434556	760	61464	496780
	#9775	434556	760	61464	496780
	+/-	0	0	0	0
libopenthread-ftd.a	main	233668	0	40198	273866
	#9775	233668	0	40198	273866
	+/-	0	0	0	0
libopenthread-cli-ftd.a	main	56654	0	8075	64729
	#9775	56654	0	8075	64729
	+/-	0	0	0	0
libopenthread-ncp-ftd.a	main	31839	0	5916	37755
	#9775	31839	0	5916	37755
	+/-	0	0	0	0
ot-cli-mtd	main	363400	760	51148	415308
	#9775	363400	760	51148	415308
	+/-	0	0	0	0
ot-ncp-mtd	main	346252	760	46376	393388
	#9775	346252	760	46376	393388
	+/-	0	0	0	0
libopenthread-mtd.a	main	157026	0	25110	182136
	#9775	157026	0	25110	182136
	+/-	0	0	0	0
libopenthread-cli-mtd.a	main	39527	0	8059	47586
	#9775	39527	0	8059	47586
	+/-	0	0	0	0
libopenthread-ncp-mtd.a	main	24719	0	5916	30635
	#9775	24719	0	5916	30635
	+/-	0	0	0	0
ot-cli-ftd-br	main	532624	768	130932	664324
	#9775	532640	768	130932	664340
	+/-	+16	0	0	+16
libopenthread-ftd-br.a	main	296643	5	104854	401502
	#9775	296661	5	104854	401520
	+/-	+18	0	0	+18
libopenthread-cli-ftd-br.a	main	70287	0	8099	78386
	#9775	70287	0	8099	78386
	+/-	0	0	0	0
ot-rcp	main	62168	564	20604	83336
	#9775	62168	564	20604	83336
	+/-	0	0	0	0
libopenthread-rcp.a	main	9522	0	5052	14574
	#9775	9522	0	5052	14574
	+/-	0	0	0	0
libopenthread-radio.a	main	18811	0	214	19025
	#9775	18811	0	214	19025
	+/-	0	0	0	0

codecov · 2024-01-16T08:34:28Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (71aed82) 85.65% compared to head (98c7b62) 76.71%.
Report is 13 commits behind head on main.

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

Files	Coverage Δ
src/core/border_router/routing_manager.cpp	`46.67% <0.00%> (-34.32%)`	⬇️

... and 292 files with indirect coverage changes

include/openthread/platform/border_routing.h

abtink · 2024-01-16T19:16:39Z

include/openthread/platform/border_routing.h

+ * @param[in] aShouldRequest  Indicate if the platform need to request/release the prefix.
+ *
+ */
+extern void otPlatBorderRoutingShouldRequestDhcp6Pd(otInstance *aInstance, bool aShouldRequest);


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

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.

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 as extern (e.g. otPlatRadioTransmit).

An otPlat API implemented by OT stack for use by platform layer should be defined as extern (e.g., otPlatRadioTxDone). These are often callbacks.

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.

Maybe I should rename the API to processStateChange()? My understanding is that different platforms will implement different methods to handle the prefix request/release

include/openthread/platform/border_routing.h

erjiaqing · 2024-01-17T07:38:24Z

include/openthread/platform/border_routing.h

+ * @param[in] aRequest  Indicate if the platform need to request/release the prefix.
+ *
+ */
+void otPlatBorderRoutingHandleDhcp6PdStateChange(otInstance *aInstance, bool aRequest);


If you use ...StateChange as the name of the API, I would prefer passing the state enum instead of a boolean to the platform.

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.

abtink · 2024-01-17T08:01:57Z

include/openthread/platform/border_routing.h

+ * @param[in] aRequest  Indicate if the platform need to request/release the prefix.
+ *
+ */
+void otPlatBorderRoutingHandleDhcp6PdStateChange(otInstance *aInstance, bool aRequest);


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() and otPlatBorderRoutingReleaseDhcp6Prefix() to be clear about the desired actions.

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.

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 (and otBorderRoutingDhcp6PdState 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.

The platform doesn't need to care the state itself, it just need to be notified to request/release prefix, how about "otPlatBorderRoutingOnDhcp6PdStateChange()"

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().

otPlatBorderRoutingRequestDhcp6Pd()

Irving-cl

LGTM 👍

Alami-Amine · 2024-01-18T16:23:42Z

Hello All,
sorry to ask here, I see all relevant parties already present,

I have a question on the implementation of DHCPv6-PD Client on the Posix Platform (for OTBR), will we use dhcpcd (or another external tool, if yes, which one?) to request a PD prefix or will this be done within the OT stack?
Also, I saw that on the OT stack we will extract the PD prefix from "platform RAs" that will be generated by PD client on the device itself and sent to the Device itself? am I understanding this correctly?
My question is on the redundancy of RAs; if we have this platform RA and our OTBR is sending RAs, couldn't that create problems for devices on the AIL?

abtink

LGTM. Thanks @sherysheng for all changes.

Couple of smaller suggestions blow and questions on order of operation and providing weak implementation

include/openthread/platform/border_routing.h

src/core/border_router/routing_manager.cpp

abtink · 2024-01-18T16:48:27Z

src/core/border_router/routing_manager.cpp

@@ -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)


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?

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).

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?

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?

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 whether posix source code should provide an empty implementation for it and leave it to be defined by ot-br-posix module.

Thoughst?

jwhui · 2024-01-18T16:54:18Z

Hello All, sorry to ask here, I see all relevant parties already present,

I have a question on the implementation of DHCPv6-PD Client on the Posix Platform (for OTBR), will we use dhcpcd (or another external tool, if yes, which one?) to request a PD prefix or will this be done within the OT stack?

Also, I saw that on the OT stack we will extract the PD prefix from "platform RAs" that will be generated by PD client on the device itself and sent to the Device itself? am I understanding this correctly?

My question is on the redundancy of RAs; if we have this platform RA and our OTBR is sending RAs, couldn't that create problems for devices on the AIL?

Moving the discussion here: https://github.com/orgs/openthread/discussions/9784

abtink · 2024-02-22T05:37:16Z

include/openthread/platform/border_routing.h

@@ -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


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

abtink · 2024-02-22T05:38:44Z

src/core/border_router/routing_manager.cpp

@@ -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


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.

abtink · 2024-02-22T05:40:36Z

src/core/border_router/routing_manager.cpp

@@ -3601,6 +3600,11 @@ void RoutingManager::PdPrefixManager::EvaluateStateChange(Dhcp6PdState aOldState
    case kDhcp6PdStateRunning:
        break;
    }
+#if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE


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?

abtink · 2024-02-22T05:42:10Z

src/core/border_router/routing_manager.cpp

@@ -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)


Should we provide an empty implementation?

sherysheng requested review from abtink, erjiaqing and Irving-cl January 16, 2024 07:52

Irving-cl reviewed Jan 16, 2024

View reviewed changes

include/openthread/platform/border_routing.h Outdated Show resolved Hide resolved

include/openthread/platform/border_routing.h Show resolved Hide resolved

sherysheng force-pushed the pdstatechangeapi branch 2 times, most recently from 591b427 to c4b4a33 Compare January 16, 2024 10:41

abtink reviewed Jan 16, 2024

View reviewed changes

abtink reviewed Jan 17, 2024

View reviewed changes

include/openthread/platform/border_routing.h Outdated Show resolved Hide resolved

sherysheng force-pushed the pdstatechangeapi branch from c4b4a33 to a3d7dde Compare January 17, 2024 06:42

erjiaqing reviewed Jan 17, 2024

View reviewed changes

abtink reviewed Jan 17, 2024

View reviewed changes

jwhui added the api: platform label Jan 17, 2024

sherysheng force-pushed the pdstatechangeapi branch from a3d7dde to eab0734 Compare January 18, 2024 05:05

sherysheng requested review from erjiaqing, Irving-cl and abtink January 18, 2024 05:05

Irving-cl approved these changes Jan 18, 2024

View reviewed changes

erjiaqing approved these changes Jan 18, 2024

View reviewed changes

abtink reviewed Jan 18, 2024

View reviewed changes

sherysheng force-pushed the pdstatechangeapi branch from eab0734 to c90d8f2 Compare January 19, 2024 06:08

[api] add api to indicate if platform should request DHCPv6 PD

98c7b62

sherysheng force-pushed the pdstatechangeapi branch from c90d8f2 to 98c7b62 Compare January 22, 2024 14:26

abtink reviewed Feb 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] add api to indicate if platform should request DHCPv6 PD #9775

[api] add api to indicate if platform should request DHCPv6 PD #9775

sherysheng commented Jan 16, 2024

size-report bot commented Jan 16, 2024 •

edited

codecov bot commented Jan 16, 2024 •

edited

abtink Jan 16, 2024

sherysheng Jan 17, 2024

abtink Jan 17, 2024 •

edited

erjiaqing Jan 17, 2024

sherysheng Jan 17, 2024

erjiaqing Jan 17, 2024

erjiaqing Jan 17, 2024

abtink Jan 17, 2024

sherysheng Jan 17, 2024

abtink Jan 17, 2024

sherysheng Jan 18, 2024

abtink Jan 18, 2024

Irving-cl left a comment

Alami-Amine commented Jan 18, 2024

abtink left a comment

abtink Jan 18, 2024

abtink Jan 18, 2024

sherysheng Jan 19, 2024

erjiaqing Jan 19, 2024

abtink Jan 19, 2024

jwhui commented Jan 18, 2024

abtink Feb 22, 2024

abtink Feb 22, 2024

abtink Feb 22, 2024

abtink Feb 22, 2024

[api] add api to indicate if platform should request DHCPv6 PD #9775

Are you sure you want to change the base?

[api] add api to indicate if platform should request DHCPv6 PD #9775

Conversation

sherysheng commented Jan 16, 2024

size-report bot commented Jan 16, 2024 • edited

Size Report of OpenThread

codecov bot commented Jan 16, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abtink Jan 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Irving-cl left a comment

Choose a reason for hiding this comment

Alami-Amine commented Jan 18, 2024

abtink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhui commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

size-report bot commented Jan 16, 2024 •

edited

codecov bot commented Jan 16, 2024 •

edited

abtink Jan 17, 2024 •

edited