[linux] add ThreadStackManager for Linux Device layer #1320

gjc13 · 2020-06-28T08:58:06Z

Problem
This PR adds ThreadStack manager in the linux device layer.
It relies on otbr-agent as the Thread daemon.

Summary of Changes

Add ot-br-posix submodule
Implementation of ThreadStackManager
Functional tests for ThreadStackManager

fixes #740

andy31415 · 2020-06-29T12:56:22Z

configure.ac

+# otbr-client
+#
+
+AC_ARG_ENABLE(otbr-client,


In my mind "OpenThread Linux Client" would be 'otlc' ... alternatively could we make the command line argument longer to be easier to read?

Like 'openthread-border-router-client'? This abbreviation is not as intuitive, even though it follows the short repo name.

In my reply to Grant's comment, I've proposed to combine ENABLE_OPENTHREAD and DEVICE_LAYER==Linux to replace this option. The only concern is whether there will be other linux Thread solutions.

andy31415 · 2020-06-29T13:03:48Z

src/include/platform/internal/DeviceNetworkInfo.h

+namespace Internal {
+
+/**
+ * Ids for well-known network provision types.


The items below contains IDs and lengths.

Could we separate them? Also lenghts are generally not enums. I believe it would be clearer if they would be constexpr, like:

constexpr int kThreadMeshPrefixLength = 8;

and similar.

int -> size_t since lengths generally are not negative.

andy31415 · 2020-06-29T13:04:25Z

src/include/platform/internal/DeviceNetworkInfo.h

+    kThreadMeshPrefixLength     = 8,
+    kThreadNetworkKeyLength     = 16,
+    kThreadPSKcLength           = 16,
+    kThreadChannel_NotSpecified = UINT8_MAX,


wny are channels and PANId mixed in the same anum with a different max? I find this a bit too data dependent: one assumes max 8 bit and one max 16 bit.

This enum is for defining constants. And the width of channel and panid is specified in the 15.4 standard. The channel is between 11 and 26 and the panid bust be 16 bits long.

I believe this may be an unusual enum usage. Enum seems to be an enumeration of distinct values. Here tehe values are not distinct (lengths may have similar values) and not as related.

I believe constexpr actual constants would be better than enums.

andy31415 · 2020-06-29T13:06:02Z

src/include/platform/internal/DeviceNetworkInfo.h

+public:
+    // ---- Thread-specific Fields ----
+    char ThreadNetworkName[kMaxThreadNetworkNameLength + 1];
+    /**< The Thread network name as a NULL-terminated string. */


could we add some spacing here? I am used to having comments on top of thigs (like method comments) and as code is so tight together, it tents to make me believe that comments apply to the next item instead of the previous. Messes with my brain a lot.

andy31415 · 2020-06-29T13:11:22Z

src/platform/Linux/ThreadStackManagerImpl.cpp

+    case DeviceRole::OTBR_DEVICE_ROLE_ROUTER:
+    case DeviceRole::OTBR_DEVICE_ROLE_LEADER:
+        type = ConnectivityManager::ThreadDeviceType::kThreadDeviceType_Router;
+    default:


should going to the default log an error?

andy31415 · 2020-06-29T13:15:23Z

src/platform/Linux/ThreadStackManagerImpl.h

+namespace chip {
+namespace DeviceLayer {
+
+class ThreadStackManagerImpl : public ThreadStackManager


Could we get some comments on what this does? I see it as an implementation of the tread stack using dbus to talk to the ble chip ... is that correct?

The Thread network stack use the 802.15.4 radio rather than BLE. It's talking to the Thread border router daemon process otbr-agent. This process is in charge of Thread network interface management and all the Thread border routing features.

configure.ac

gerickson · 2020-06-29T16:14:22Z

src/platform/Makefile.am

@@ -142,6 +142,18 @@ libDeviceLayer_a_SOURCES               += \
    Linux/SystemTimeSupport.cpp           \
    $(NULL)

+if CHIP_WITH_OTBR_CLIENT


Should this also be conditioned with CHIP_ENABLE_OPENTHREAD? Is it possible to have CHIP_ENABLE_OPENTHREAD but not CHIP_WITH_OTBR_CLIENT or vice versa?

Yes, the otbr-client doesn't depend on the openthread code. The current implementation of CHIP_ENABLE_OPENTHREAD will download the openthread code once enabled so I decided to add an extra option.

If otbr-client is the only OpenThread solution for linux, maybe we can combine DEVICE_LAYER==LINUX and CHIP_ENABLE_OPENTHREAD to decide it.

gerickson · 2020-06-29T16:15:31Z

configure.ac

+# otbr-client
+#
+
+AC_ARG_ENABLE(otbr-client,


How does this interplay with CHIP_ENABLE_OPENTHREAD or vice versa?

gerickson · 2020-06-29T16:21:54Z

src/platform/tests/Makefile.am

@@ -68,13 +68,31 @@ AM_CPPFLAGS                                    = \
    $(NLUNIT_TEST_CPPFLAGS)                      \
    $(NULL)

+if CHIP_DEVICE_LAYER_TARGET_LINUX
+if CHIP_WITH_OTBR_CLIENT


See prior comments about CHIP_ENABLE_OPENTHREAD.

gerickson · 2020-06-29T16:22:29Z

src/platform/tests/Makefile.am

 CHIP_LDADD                                      = \
    $(top_builddir)/src/platform/libDeviceLayer.a \
    $(top_builddir)/src/inet/libInetLayer.a       \
    $(top_builddir)/src/system/libSystemLayer.a   \
    $(top_builddir)/src/lib/support/libSupportLayer.a   \
    $(NULL)

+if CHIP_DEVICE_LAYER_TARGET_LINUX


The block above is also conditioned with CHIP_WITH_OTBR_CLIENT, why not this block?

third_party/Makefile.am

gerickson

Some naming and conditional rationalization requested.

gjc13 · 2020-06-30T11:02:50Z

@gerickson @andy31415
I've updated the configure logic.
Now we are using option --enable-openthread for all the Thread related features.
Then we'll automatically determine the project (openthread/ot-br-posix) to use based on the current device layer.

The build flags CHIP_WITH_OPENTHREAD and CHIP_WITH_OT_BR_CLIENT marks which library we are using.
Does this look fine to you?

chip-bot · 2020-07-01T10:33:46Z

Size increase report for "Build Examples [nRF]"

File	Section	File	VM
chip-nrf52840-lock-example.out	[LOAD #2 [RW]]	0	4
chip-nrf52840-lock-example.out	.data	-12	-12
chip-nrf52840-lock-example.out	.bss	0	-24
chip-nrf52840-lock-example.out	.text	-304	-304

Full report output

Bloat report for job 'Build Examples [nRF]'

Files found only in the baseline:
    bloat_report.txt

Comparing master_binaries/nrf-build/chip-nrf52840-lock-example.out and example_binaries/nrf-build/chip-nrf52840-lock-example.out:

sections,vmsize,filesize
[Unmapped],0,307
[LOAD #2 [RW]],4,0
.data,-12,-12
.bss,-24,0
.debug_aranges,0,-72
.debug_frame,0,-264
.text,-304,-304
.symtab,0,-432
.debug_ranges,0,-448
.strtab,0,-541
.debug_macro,0,-607
.debug_loc,0,-1649
.debug_line,0,-1694
.debug_str,0,-2922
.debug_abbrev,0,-3253
.debug_info,0,-39729

chip-bot · 2020-07-01T10:35:14Z

Size increase report for "Build Examples [ESP32]"

File	Section	File	VM
chip-wifi-echo.elf	.flash.rodata	152	152
chip-wifi-echo.elf	.flash.text	-496	-496
chip-wifi-echo.elf	[28 Others]	-784	-24

Full report output

Bloat report for job 'Build Examples [ESP32]'

Files found only in the baseline:
    bloat_report.txt

Comparing master_binaries/esp32-build/chip-wifi-echo.elf and example_binaries/esp32-build/chip-wifi-echo.elf:

sections,vmsize,filesize
.debug_loc,0,462
.xt.lit._ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE12_M_constructIPKcEEvT_S8_St20forward_iterator_tag,0,288
.flash.rodata,152,152
.xt.prop._ZN4chip8Callback13CallbackDeque9DequeueByEPFbPvPKNS0_10CancelableEES2_,0,-124
[Unmapped],0,-129
.xt.prop._ZN4chip16ReferenceCountedINS_24SecureSessionMgrCallbackEE7ReleaseEv,0,-140
.debug_ranges,0,-160
.debug_aranges,0,-200
.xt.prop._ZN4chip8Callback10CancelableD5Ev,0,-212
.xt.prop._ZN4chip8Callback13CallbackDequeD2Ev,0,-236
.xt.prop._ZNSt8_Rb_treeIhSt4pairIKhN4chip27OptionalQRCodeInfoExtensionEESt10_Select1stIS4_ESt4lessIhESaIS4_EE7_M_copyINSA_11_Alloc_nodeEEEPSt13_Rb_tree_nodeIS4_EPKSE_PSt18_Rb_tree_node_baseRT_,0,-240
.flash.text,-496,-496
.debug_frame,0,-600
.symtab,0,-736
[28 Others],-24,-784
.strtab,0,-1126
.shstrtab,0,-1236
.debug_line,0,-2392
.debug_str,0,-2720
.debug_abbrev,0,-2860
.debug_info,0,-37975

chip-bot · 2020-07-01T10:36:57Z

Size increase report for "Build Examples [main-build]"

File	Section	File	VM
chip-standalone-demo.out	[LOAD #2 [RX]]	-8	-8
chip-standalone-demo.out	.data.rel.ro	-56	-56
chip-standalone-demo.out	.eh_frame_hdr	-56	-56
chip-standalone-demo.out	.rela.dyn	-120	-120
chip-standalone-demo.out	.eh_frame	-224	-224
chip-standalone-demo.out	.text	-544	-544

Full report output

Bloat report for job 'Build Examples [main-build]'

Files found only in the baseline:
    bloat_report.txt

Comparing master_binaries/main-build/chip-standalone-demo.out and example_binaries/main-build/chip-standalone-demo.out:

sections,vmsize,filesize
[LOAD #2 [RX]],-8,-8
.data.rel.ro,-56,-56
.eh_frame_hdr,-56,-56
.debug_aranges,0,-112
.rela.dyn,-120,-120
.symtab,0,-216
.eh_frame,-224,-224
.debug_macro,0,-286
.text,-544,-544
.strtab,0,-618
.debug_ranges,0,-752
.debug_line,0,-835
.debug_loc,0,-1981
.debug_abbrev,0,-2128
[Unmapped],0,-3092
.debug_str,0,-3383
.debug_info,0,-22037

gerickson

OT-BR-POSIX package needs to be capable of being sourced out of project.
What happened to the openthread package dependency? I suspect OT-BR-POSIX takes care of that for POSIX systems; however, what about non-POSIX systems?

gerickson · 2020-07-01T15:15:03Z

configure.ac

-# OpenThread
-#
-
-NL_WITH_OPTIONAL_INTERNAL_PACKAGE(


Why did we get rid of this?

Since we need to determine whether or not we are using the linux device layer, I added the --enable-openthread option.
Maybe we can have both --enable-openthread and --with-openthread.

We shouldn't need both.

In autoconf, --enable-foo is equivalent to --with-foo and --disable-foo is equivalent to --without-foo or --with-foo=no. However, the added benefit of the --with-foo syntax is that an argument can be provided that is the location of the final artifacts for that package. The NL_WITH_PACKAGE family of constructs provides a convenience semantics around that --with-foo syntax by also providing --with-foo-libs and --with-foo-includes variants for formalizing FOO_CPPFLAGS, FOO_LDFLAGS, and FOO_LIBS.

gerickson · 2020-07-01T15:17:33Z

src/platform/Makefile.am

+endif
+
+libDeviceLayer_a_CPPFLAGS              += \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/include    \


What if I want to use an ot-br-posix package outside of CHIP, for example were Google integrating CHIP into a repo-managed project in which --with-ot-br-posix=${BUILD_ROOT}/tps/ot-br-posix or some such?

This should be indirected via something like OT_BR_POSIX_CPPFLAGS.

gerickson · 2020-07-01T15:17:40Z

src/platform/Makefile.am

+
+libDeviceLayer_a_CPPFLAGS              += \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/include    \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/src        \


gerickson · 2020-07-01T15:18:14Z

src/platform/tests/Makefile.am

+if CHIP_WITH_OT_BR_CLIENT
+AM_CPPFLAGS                                   += \
+    $(DBUS_CFLAGS)                               \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/include    \


gerickson · 2020-07-01T15:18:20Z

src/platform/tests/Makefile.am

+AM_CPPFLAGS                                   += \
+    $(DBUS_CFLAGS)                               \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/include    \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/src        \


gerickson · 2020-07-01T15:18:30Z

src/platform/tests/Makefile.am

+if CHIP_WITH_OT_BR_CLIENT
+CHIP_LDADD                                    += \
+    $(DBUS_LIBS)                                 \
+    $(top_builddir)/third_party/ot-br-posix/libotbrclient.a \


gerickson · 2020-07-01T15:19:50Z

third_party/ot-br-posix/Makefile.am

+    $(NULL)
+
+libotbrclient_a_CPPFLAGS                                  = \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/include    \


These should be fine as-is since you're already in the third_party directory and building the source there; however, it wouldn't be harmful to indirect this through OT_BR_POSIX_CPPFLAGS.

gerickson · 2020-07-01T15:19:56Z

third_party/ot-br-posix/Makefile.am

+
+libotbrclient_a_CPPFLAGS                                  = \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/include    \
+    -I$(top_srcdir)/third_party/ot-br-posix/repo/src        \


CLAassistant · 2020-07-01T18:24:32Z

All committers have signed the CLA.

gjc13 · 2020-07-02T03:07:06Z

OT-BR-POSIX package needs to be capable of being sourced out of project.

What happened to the openthread package dependency? I suspect OT-BR-POSIX takes care of that for POSIX systems; however, what about non-POSIX systems?

The openthread package dependency is passed by the NRF build system(which is the only place it's used now). The autoconf option is for feature flag only now. This holds true for the current master as well.

andy31415

Could we update the code to not use enums for constants but rather real constants (usually consexpr size_t or similar)?

This reverts commit d9660a4.

gjc13 · 2020-07-03T10:40:24Z

@gerickson @andy31415
Due to the open source of Chip project. Looks like I suddenly cannot push to my previous fork to update this PR (though the rebase worked).
Move to #1438 for further updates. Sorry for the inconvenience.

gjc13 requested review from andy31415, anush-apple, BroderickCarlin, chrisdecenzo, hawk248, jelderton, robszewczyk, saurabhst and woody-apple as code owners June 28, 2020 08:58

restyled-io bot mentioned this pull request Jun 28, 2020

Restyle [linux] add ThreadStackManager for Linux Device layer #1321

Closed

gjc13 force-pushed the thread-linux branch from 561bc44 to ef1afc0 Compare June 28, 2020 10:34

andy31415 reviewed Jun 29, 2020

View reviewed changes

gerickson reviewed Jun 29, 2020

View reviewed changes

configure.ac Show resolved Hide resolved

gerickson reviewed Jun 29, 2020

View reviewed changes

third_party/Makefile.am Show resolved Hide resolved

gerickson suggested changes Jun 29, 2020

View reviewed changes

gjc13 force-pushed the thread-linux branch from a7dac8d to 6305a2a Compare June 30, 2020 10:58

woody-apple added tools feature work and removed tools labels Jun 30, 2020

gjc13 force-pushed the thread-linux branch from 6305a2a to d9660a4 Compare July 1, 2020 04:48

gjc13 requested review from andy31415 and gerickson July 1, 2020 10:33

gerickson suggested changes Jul 1, 2020

View reviewed changes

andy31415 requested changes Jul 2, 2020

View reviewed changes

gjc13 added 4 commits July 3, 2020 17:57

[linux] add ThreadStackManager for Linux Device layer

83a3356

new configure logic

ab63f8b

add role check in tests

b5ad124

Revert "new configure logic"

741bdad

This reverts commit d9660a4.

gjc13 force-pushed the thread-linux branch from 248d430 to c4a1658 Compare July 3, 2020 10:07

gjc13 added 2 commits July 3, 2020 18:33

fixes

758345a

fix ci

b8223f4

gjc13 closed this Jul 6, 2020

[linux] add ThreadStackManager for Linux Device layer #1320

[linux] add ThreadStackManager for Linux Device layer #1320

Conversation

gjc13 commented Jun 28, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjc13 Jun 30, 2020 • edited Loading

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

gerickson left a comment

Choose a reason for hiding this comment

gjc13 commented Jun 30, 2020 • edited Loading

chip-bot commented Jul 1, 2020

chip-bot commented Jul 1, 2020

chip-bot commented Jul 1, 2020

gerickson 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

CLAassistant commented Jul 1, 2020 • edited Loading

gjc13 commented Jul 2, 2020

andy31415 left a comment

Choose a reason for hiding this comment

gjc13 commented Jul 3, 2020

gjc13 commented Jun 28, 2020 •

edited

Loading

gjc13 Jun 30, 2020 •

edited

Loading

gjc13 commented Jun 30, 2020 •

edited

Loading

CLAassistant commented Jul 1, 2020 •

edited

Loading