[epskc] enable by default for Thread 1.4 #2429

mia1yang · 2024-08-09T12:09:36Z

codecov · 2024-08-09T12:25:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.35%. Comparing base (2b41187) to head (16d79d8).
Report is 766 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2429       +/-   ##
===========================================
- Coverage   55.77%   45.35%   -10.42%     
===========================================
  Files          87      100       +13     
  Lines        6890    11345     +4455     
  Branches        0      821      +821     
===========================================
+ Hits         3843     5146     +1303     
- Misses       3047     5943     +2896     
- Partials        0      256      +256

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

src/border_agent/border_agent.cpp

abtink

👍

superwhd · 2024-08-11T03:59:52Z

src/border_agent/border_agent.cpp

@@ -163,6 +165,9 @@ BorderAgent::BorderAgent(otbr::Ncp::RcpHost &aHost, Mdns::Publisher &aPublisher)
    , mBaseServiceInstanceName(OTBR_MESHCOP_SERVICE_INSTANCE_NAME)
 {
    mHost.AddThreadStateChangedCallback([this](otChangedFlags aFlags) { HandleThreadStateChanged(aFlags); });
+    mIsEphemeralKeyEnabled = (otThreadGetVersion() >= kThreadVersion14);


I think in a previous discussion our consensus was to provide a macro to configure whether to enable ePSKc on init and define the default value according to the version.

Could you explain why your prefer to implement this by directly checking the version? I think it's less configurable in this way.

Another review concern is too many macro; I suppose the target of macro is easy to config, so in the new commit, add a kEphemeralKeyEnableOnInit for config; Let me know your concerns. Thanks!

Another review concern is too many macro

IMHO the suggested macro is not that harmful because it doesn't guard code blocks. Instead, it's just used as a configurable boolean value.

Have modified to Macro accordingly; Thanks for re-reviewing

src/border_agent/border_agent.cpp

superwhd · 2024-08-11T04:02:40Z

src/border_agent/border_agent.cpp

@@ -77,6 +77,8 @@ static const char    kBorderAgentServiceType[]      = "_meshcop._udp";   ///< Bo
 static const char    kBorderAgentEpskcServiceType[] = "_meshcop-e._udp"; ///< Border agent ePSKc service
 static constexpr int kBorderAgentServiceDummyPort   = 49152;

+static constexpr uint16_t kThreadVersion14 = 5; ///< Thread Version 1.4


Maybe you can just use OT_THREAD_VERSION_1_4?

This definition is in core and could not be used directly? Open an issue to track refactor of thread version in OTBR b/358571361

Sorry I didn't pay attention to where the macro was defined.

IMHO we should expose the OT_THREAD_VERSION_* macros as part of Openthread API. Currently otThreadGetVersion doesn't tell what the returned integer means.

@abtink @jwhui what do you think?

@superwhd Just FYI, there is a dbus API GetThreadVersion can tell the version mapping (slightly differences of version1.3.1);

Temperately add version definition in BA in this PR and try to refactor OT version in OTBR, tracking here: b/358571361. Appreciate to leave comments for the refactor.

IMHO we should expose the OT_THREAD_VERSION_* macros as part of Openthread API. Currently otThreadGetVersion doesn't tell what the returned integer means.

I 100% agree. I was expecting OT_THREAD_VERSION_* to be defined in thread.h header, but for some reason they are defined in openthread-core-config.h (which is not a public header). I will submit a PR on this.

src/border_agent/border_agent.cpp

bukepo · 2024-08-13T02:46:56Z

src/border_agent/border_agent.cpp

@@ -157,12 +163,13 @@ BorderAgent::BorderAgent(otbr::Ncp::RcpHost &aHost, Mdns::Publisher &aPublisher)
    : mHost(aHost)
    , mPublisher(aPublisher)
    , mIsEnabled(false)
-    , mIsEphemeralKeyEnabled(false)
+    , mIsEphemeralKeyEnabled(OTBR_CONFIG_BORDER_AGENT_EPHEMERAL_KEY_ON_INIT)


do we need the macro OTBR_CONFIG_BORDER_AGENT_EPHEMERAL_KEY_ON_INIT?

Suggested change

, mIsEphemeralKeyEnabled(OTBR_CONFIG_BORDER_AGENT_EPHEMERAL_KEY_ON_INIT)

, mIsEphemeralKeyEnabled(otThreadGetVersion() >= OT_THREAD_VERSION_1_4)

Use this simple way for this PR; Link detailed discussion for ref

src/border_agent/border_agent.cpp

mia1yang requested review from superwhd, librasungirl and bukepo August 9, 2024 12:09

mia1yang marked this pull request as ready for review August 9, 2024 12:11

mia1yang force-pushed the epskc_version branch from bfae1d4 to 1919e5b Compare August 9, 2024 12:13

bukepo reviewed Aug 9, 2024

View reviewed changes

src/border_agent/border_agent.cpp Outdated Show resolved Hide resolved

mia1yang force-pushed the epskc_version branch from 1919e5b to e1a436b Compare August 9, 2024 12:54

mia1yang requested a review from bukepo August 9, 2024 12:55

bukepo reviewed Aug 9, 2024

View reviewed changes

src/border_agent/border_agent.cpp Outdated Show resolved Hide resolved

librasungirl reviewed Aug 9, 2024

View reviewed changes

src/border_agent/border_agent.cpp Outdated Show resolved Hide resolved

mia1yang force-pushed the epskc_version branch 2 times, most recently from 8c9966c to 2e22cb9 Compare August 9, 2024 13:09

mia1yang requested a review from bukepo August 9, 2024 13:09

librasungirl approved these changes Aug 9, 2024

View reviewed changes

src/border_agent/border_agent.cpp Outdated Show resolved Hide resolved

mia1yang force-pushed the epskc_version branch from 2e22cb9 to ea421c1 Compare August 9, 2024 13:21

librasungirl changed the title ~~[epskc] set ePSKc init status according to otversion~~ [epskc] enable by default for Thread 1.4 Aug 9, 2024

mia1yang requested a review from abtink August 9, 2024 13:26

abtink approved these changes Aug 9, 2024

View reviewed changes

superwhd reviewed Aug 11, 2024

View reviewed changes

mia1yang force-pushed the epskc_version branch 2 times, most recently from 57b7266 to d5602d8 Compare August 12, 2024 07:38

mia1yang requested a review from superwhd August 12, 2024 07:55

mia1yang force-pushed the epskc_version branch 2 times, most recently from 368aa32 to 93b3426 Compare August 12, 2024 09:40

superwhd reviewed Aug 12, 2024

View reviewed changes

src/border_agent/border_agent.cpp Outdated Show resolved Hide resolved

superwhd reviewed Aug 12, 2024

View reviewed changes

src/border_agent/border_agent.cpp Outdated Show resolved Hide resolved

mia1yang force-pushed the epskc_version branch from 93b3426 to b7ea789 Compare August 12, 2024 10:06

bukepo reviewed Aug 13, 2024

View reviewed changes

mia1yang force-pushed the epskc_version branch from b7ea789 to d16d856 Compare August 19, 2024 03:03

mia1yang force-pushed the epskc_version branch 2 times, most recently from d47d09a to 3c40915 Compare August 19, 2024 08:52

superwhd reviewed Aug 19, 2024

View reviewed changes

src/border_agent/border_agent.cpp Outdated Show resolved Hide resolved

mia1yang force-pushed the epskc_version branch 2 times, most recently from d58b711 to b39b38e Compare August 19, 2024 09:35

superwhd approved these changes Aug 19, 2024

View reviewed changes

mia1yang requested a review from bukepo August 19, 2024 09:37

[epskc] enable by default for Thread 1.4

16d79d8

mia1yang force-pushed the epskc_version branch from 31de4aa to 16d79d8 Compare August 19, 2024 09:40

bukepo approved these changes Aug 19, 2024

View reviewed changes

librasungirl requested a review from jwhui August 20, 2024 05:48

jwhui approved these changes Aug 20, 2024

View reviewed changes

jwhui merged commit 791828c into openthread:main Aug 20, 2024
32 checks passed

librasungirl added the comp:epskc label Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[epskc] enable by default for Thread 1.4 #2429

[epskc] enable by default for Thread 1.4 #2429

mia1yang commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading

abtink left a comment

superwhd Aug 11, 2024

mia1yang Aug 12, 2024

superwhd Aug 12, 2024

mia1yang Aug 12, 2024

superwhd Aug 11, 2024

mia1yang Aug 12, 2024

superwhd Aug 12, 2024

mia1yang Aug 12, 2024

abtink Aug 12, 2024

bukepo Aug 13, 2024

mia1yang Aug 19, 2024

	, mIsEphemeralKeyEnabled(OTBR_CONFIG_BORDER_AGENT_EPHEMERAL_KEY_ON_INIT)
	, mIsEphemeralKeyEnabled(otThreadGetVersion() >= OT_THREAD_VERSION_1_4)

[epskc] enable by default for Thread 1.4 #2429

[epskc] enable by default for Thread 1.4 #2429

Conversation

mia1yang commented Aug 9, 2024 • edited Loading

codecov bot commented Aug 9, 2024 • edited Loading

Codecov Report

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

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

mia1yang commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading