Skip to content

pkg/openthread: rework of FTD and MTD support#9336

Merged
aabadie merged 6 commits intoRIOT-OS:masterfrom
jia200x:openthread_ftd
Oct 16, 2018
Merged

pkg/openthread: rework of FTD and MTD support#9336
aabadie merged 6 commits intoRIOT-OS:masterfrom
jia200x:openthread_ftd

Conversation

@jia200x
Copy link
Member

@jia200x jia200x commented Jun 12, 2018

Contribution description

This is a rework of #7149. The original PR by @biboc includes support for FTD, MTD and NCP.
This is (almost) and exact copy of his original work, but removes NCP support (see discussion)

It also sets the pkg version to the OpenThread Release Thread Reference 2017-07-16.

I had to modify a couple of stuff in order to make it work with the Release version, and I pushed those commits as fixup!. The initial commit was already reviewed in #7149.

#7149 should still be open, because it contains the NCP implementation that can be used after this gets merged.

Issues/PRs references

#7149
Fixes #10031

@jia200x jia200x requested a review from aabadie June 12, 2018 15:53
@jia200x
Copy link
Member Author

jia200x commented Jun 12, 2018

Tested on IoTLAB and all multihop is working as expected. MTD is also working as it should.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Thanks for moving this forward @jia200x!

The changes were indeed already mostly reviewed. I found one thing related to module dependencies. Not sure if it was already in the original PR.
I can test on iot-lab in a few days.

Makefile.dep Outdated
USEMODULE += gnrc_sock_udp
endif

ifneq (,$(filter openthread,$(USEPKG)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the package dependencies could be added to a new pkg/openthread/Makefile.dep file instead. xtimer module could also be added as a dependency there.


USEMODULE += $(DRIVER)

USEMODULE += xtimer
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add xtimer module as a dependency here if it's already a dependency of the openthread package.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, you are right. I will address the proposed comments

Copy link
Contributor

Choose a reason for hiding this comment

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

Still there.

@jia200x
Copy link
Member Author

jia200x commented Jun 13, 2018

@aabadie done!

@jia200x jia200x added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 13, 2018
OPENTHREAD_PANID ?= 0xbeef
OPENTHREAD_CHANNEL ?= 26

CFLAGS += -DOPENTHREAD_PANID=${OPENTHREAD_PANID}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: use parenthesis instead of curly braces for accessing variables in Makefiles

uint8_t res = ot_call_command("panid", NULL, (void*)&panid);
printf("Current panid: 0x%x (res:%x)\n", panid, res);

openthread_uart_run();
Copy link
Contributor

Choose a reason for hiding this comment

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

why not keep the openthread shell ?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the OpenThread shell. The flow is OpenThread Core <-> Openthread CLI <-> UART

Copy link
Contributor

Choose a reason for hiding this comment

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

yes but here it's removed, so how can the OT shell be used ?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I see what you mean. Now there's no "openthread_uart_run" simulator. It directly handles UART at interrupt level in the file platform_uart.c

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, got it now !

@jia200x
Copy link
Member Author

jia200x commented Jun 13, 2018

@aabadie addressed

@jia200x
Copy link
Member Author

jia200x commented Jun 13, 2018

@aabadie just addressed Murdock issues. Let's see how it goes

@aabadie
Copy link
Contributor

aabadie commented Jun 13, 2018

@jia200x, seems like Murdock workers are missing automake.

@jia200x
Copy link
Member Author

jia200x commented Jun 13, 2018

@aabadie I opened this PR some time ago in the riotdocker repo. It adds automake

@Hyungsin
Copy link

Hyungsin commented Jul 2, 2018

Any update here?

@aabadie
Copy link
Contributor

aabadie commented Jul 3, 2018

I opened this PR some time ago in the riotdocker repo. It adds automake

I just merged it but now I think we have to wait for the new docker image to be deployed on Murdock workers.

@aabadie
Copy link
Contributor

aabadie commented Jul 3, 2018

@jia200x, how about moving the openthread test application to the examples directory ?

@jia200x
Copy link
Member Author

jia200x commented Jul 5, 2018

Back from holidays, sorry the delay.
It makes sense to me to move the test. I will do it.

@jia200x
Copy link
Member Author

jia200x commented Jul 5, 2018

@aabadie just moved the examples folder. Do you know if the Murdock workers were updated?

@jia200x
Copy link
Member Author

jia200x commented Jul 5, 2018

@aabadie there's an error with Murdock I'm not able to reproduce:

Building application "openthread" for "fox" with MCU "stm32f1".

Compile OpenThread for FTD device
$OPENTHREAD_ARGS is [--enable-cli-app=ftd --enable-application-coap]
make[1]: Nothing to be done for 'prepare'.
Compile OpenThread for FTD device
$OPENTHREAD_ARGS is [--enable-cli-app=ftd --enable-application-coap]
cd /tmp/dwq.0.9284869489919173/0d721c1ca21ff9c139e349583b111504/examples/openthread/bin/pkg/fox/openthread && PREFIX="/" ./bootstrap
./third_party/nlbuild-autotools/repo/scripts/bootstrap: 280: ./third_party/nlbuild-autotools/repo/scripts/bootstrap: --automake: not found
cd /tmp/dwq.0.9284869489919173/0d721c1ca21ff9c139e349583b111504/examples/openthread/bin/pkg/fox/openthread && CPP="arm-none-eabi-gcc -E" CC="arm-none-eabi-gcc" CXX="arm-none-eabi-g++"\
	OBJC="" OBJCXX="" AR="arm-none-eabi-ar" RANLIB="arm-none-eabi-ranlib" NM="" \
	STRIP="" \
	CPPFLAGS="-fdata-sections -ffunction-sections -Os -Wno-implicit-fallthrough -Wno-unused-parameter -mcpu=cortex-m3 -mlittle-endian -mthumb -mfloat-abi=soft -DOPENTHREAD_PROJECT_CORE_CONFIG_FILE='\"platform_config.h\"'" \
	CFLAGS="-fdata-sections -ffunction-sections -Os -Wno-implicit-fallthrough -Wno-unused-parameter -mcpu=cortex-m3 -mlittle-endian -mthumb -mfloat-abi=soft " \
	CXXFLAGS="-fdata-sections -ffunction-sections -Os -Wno-implicit-fallthrough -Wno-unused-parameter -mcpu=cortex-m3 -mlittle-endian -mthumb -mfloat-abi=soft -fno-exceptions -fno-rtti " \
	LDFLAGS="-fdata-sections -ffunction-sections -Os -Wno-implicit-fallthrough -Wno-unused-parameter -mcpu=cortex-m3 -mlittle-endian -mthumb -mfloat-abi=soft -nostartfiles -specs=nano.specs \
	-specs=nosys.specs -Wl,--gc-sections -Wl,-Map=map.map " \
	./configure --disable-docs --host=arm-none-eabi --target=arm-none-eabi \
	--prefix=/ --enable-default-logging --enable-cli-app=ftd --enable-application-coap
/bin/sh: 1: ./configure: not found
Makefile:24: recipe for target 'all' failed
make[1]: *** [all] Error 127

Any clues on what could be wrong?

@aabadie
Copy link
Contributor

aabadie commented Jul 5, 2018

bootstrap: --automake: not found: It's still the same problem, Murdock workers are not providing automake. We can't merge this PR because of this.

@jia200x jia200x added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 12, 2018
@jia200x
Copy link
Member Author

jia200x commented Jul 12, 2018

Triggered Murdock again, the riotdocker image is updated. So the workers I guess

@jia200x jia200x added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 17, 2018
@jia200x jia200x added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 10, 2018
open source implementation of [Thread](https://threadgroup.org/) on RIOT.
The [Command Line Interface](https://github.com/openthread/openthread/blob/master/examples/apps/cli/README.md) of
OpenThread was ported. Please check the [full documentation]
(https://github.com/openthread/openthread/blob/master/src/cli/README.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

The link to full documentation is broken, see here
If you want to separate the paragraph, a new line is not enough, you have to add a blank line.

Copy link
Member Author

Choose a reason for hiding this comment

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

right. Just pushed the fix (fd96a55)

@jia200x
Copy link
Member Author

jia200x commented Oct 10, 2018

@smling @kaspar030 for some reason Murdock still complains about automake, m4, libtool stuff :(
I tried to compile it with BUILD_IN_DOCKER=1 and the image works.

Any clue?

@aabadie
Copy link
Contributor

aabadie commented Oct 10, 2018

Any clue?

The Murdock workers need to be updated with the latest docker image version. This is not done automatically and other remaining things may change apparently (use ubuntu bionic, etc)

@jia200x
Copy link
Member Author

jia200x commented Oct 10, 2018

The Murdock workers need to be updated with the latest docker image version. This is not done automatically and other remaining things may change apparently (use ubuntu bionic, etc)

ping? @kaspar030 @smlng

@jia200x jia200x added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 15, 2018
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Tested this PR on IoT-LAB with iotlab-m3 and samr21-xpro boards. It works as expected: I could exchange pings and also tried the coap command with success.

Let's try to have this one into the release. ACK

@jia200x
Copy link
Member Author

jia200x commented Oct 16, 2018

awesome! I will squash then ;)

@jia200x jia200x added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 16, 2018
@jia200x
Copy link
Member Author

jia200x commented Oct 16, 2018

@aabadie there's an unrelated issue with pkg_libhydrogen

@aabadie
Copy link
Contributor

aabadie commented Oct 16, 2018

there's an unrelated issue with pkg_libhydrogen

Saw that. I'm wondering why execution tests are run when only CI:ready for build label is set. Any idea ?

@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 16, 2018
@aabadie
Copy link
Contributor

aabadie commented Oct 16, 2018

and go !

@aabadie aabadie merged commit 77d97a6 into RIOT-OS:master Oct 16, 2018
@jia200x
Copy link
Member Author

jia200x commented Oct 16, 2018

after 1 year!!! Thank you so much to everyone involved and special thanks to @biboc who was the original author of this PR!

@Hyungsin
Copy link

This is very encouraging! Thank you guys! 👍

@biboc
Copy link
Member

biboc commented Nov 21, 2018

Good job @jia200x !

How about NCP port?
I do think it is important since it can replace the actual RIOT BorderRouter and the port will be almost completed
Last thing missing was commissioning if I remember and I had problems with amount of free RAM for samr21-xpro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pkg/openthread: fails to build on macOS

7 participants