Skip to content
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

OpenThread port #5552

Closed
wants to merge 279 commits into from
Closed

OpenThread port #5552

wants to merge 279 commits into from

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Jun 15, 2016

Hi.

This is the WIP PR for the port of OpenThread.
Best regards.


-ROOTDIR=${PREFIX}/
+#ROOTDIR=${PREFIX}/
+ROOTDIR=/
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a conflict between mkversion's PREFIX and RIOT PREFIX variable. This is not the cleanest solution, but for now it's working.

@jia200x
Copy link
Member Author

jia200x commented Jun 15, 2016

For now the pkg is being cloned, compiled and the openthread library is copied in BINDIR.
Static linking is working.

.PHONY: all

all: git-download
$($(PKG_BUILDIR)/bootstrap)
Copy link
Member

Choose a reason for hiding this comment

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

One tab should suffice here?

@biboc biboc added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports labels Jul 5, 2016
@OlegHahm
Copy link
Member

Just checking: Is this WIP? (If yes, we should set the label, if not, we should update the title.)

@jia200x jia200x changed the title [WIP] OpenThread port OpenThread port Jul 17, 2016
@jia200x
Copy link
Member Author

jia200x commented Jul 17, 2016

Not WIP anymore (now is in test phase).
There are some files that shouldn't be in the PR. I will remove them asap.
Also, I have to move the example to tests.

Cheers!

@jia200x
Copy link
Member Author

jia200x commented Jul 18, 2016

2 known bugs after testing with IotLAB:

  • When a node disconnects and connects again to the same leader, sometimes the leader reject the packets. This produces 2 OpenThread leaders.
  • When there are a lot of nodes, sometimes they crash if you try to do an IEEE802.15.4 scan or pinging (it seems to be an assert of OpenThread's otPlatRadioTransmit that expects the driver to be ready).

Since the pkg version is outdated, I'm migrating to the latest to see if this fixes the problems.
Cheers

@OlegHahm OlegHahm added the Area: network Area: Networking label Jul 18, 2016
PKG_NAME=openthread
PKG_URL=git://github.com/openthread/openthread.git
PKG_VERSION=4779fb20eb6f027b4ca3ff56f58ee789829d72f8
PKG_BUILDDIR ?= $(BINDIRBASE)/pkg/$(BOARD)/$(PKG_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't exactly this line part of pkg.mk?

@OlegHahm OlegHahm added the PR-award-nominee Deprecated. Will be removed soon. label Jul 20, 2016
@jonas-rem
Copy link
Contributor

For testing with the phyNODE board I rebased the updated driver from #5469 on this PR. There are some minor adaptions necessary, which can be found at [1]. I tested with the branch at my Github [2].

Status:

  • Building works
  • RX and TX packets seems to work
  • when starting the Thread Network, device sends out multicast messages which I can see in Wireshark
  • The other device receives these packets and forwards them within the function recv_pkt

However, when starting the commissioning procedure with start at both boards, they fail pair to each other. Both devices state that they are leader. When I try the same with two SAMR21 devices, everything works. @jia200x any idea whats going wrong?

[1] - 92e5b91
[2] - https://github.com/jremmert-phytec-iot/RIOT/tree/kw2xrf

@jia200x
Copy link
Member Author

jia200x commented Jul 25, 2016

Hi Jonas.

I sometimes had that problem when the packets where not being forwarded to OpenThread via otPlatRadioReceiveDone (in function recv_pkt.)

Also, I added a NETOPT_RX_LISTENING function in the driver for turning of the receiver. It's required by OpenThread, since does the rx/tx synchronization with otPlatRadioIdle, otPlatRadioReceive and otPlatRadioTransmit. If the driver triggers a RX_COMPLETE event while OT is in idle, unexpected results my occur.

Can you turn on OpenThread debug and post the logs? This is done in file openthread_example/bin/pkg/{BOARD}/openthread/src/core/openthread-core-config.h, line 144, changing OPENTHREAD_LOG_LEVEL_CRIT to OPENTHREAD_LOG_LEVEL_DEBG.

Cheers

@kaspar030 kaspar030 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 29, 2016
@jia200x jia200x force-pushed the openthread branch 2 times, most recently from e0be1fa to d525f20 Compare August 1, 2016 15:06
@adjih
Copy link
Contributor

adjih commented Aug 1, 2016

tested last commit on two SAMR21-xpro, works for me 👍

@aabadie
Copy link
Contributor

aabadie commented Aug 1, 2016

@jia200x, it seems this one needs some rebase ;)

@jia200x
Copy link
Member Author

jia200x commented Aug 2, 2016

@aabadie Heavy rebase actually. I'm working on it :)

2016-08-01 18:38 GMT+02:00 Alexandre Abadie notifications@github.com:

@jia200x https://github.com/jia200x, it seems this one needs some
rebase ;)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5552 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABM8SIfB44xAPJ0NygopYAw66yNZrQcKks5qbiD1gaJpZM4I2b6I
.

{
netopt_enable_t enable = true;

_dev->driver->set(_dev, NETOPT_RX_LISTENING, &enable, sizeof(enable));
Copy link
Contributor

Choose a reason for hiding this comment

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

@jia200x what is the reason to not using set_state(NETOPT_STATE_IDLE)? This method bypasses the radio driver state handling and requires modification of each radio driver implementations.

Copy link
Member Author

@jia200x jia200x Aug 2, 2016

Choose a reason for hiding this comment

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

Hi Jonas.

In the current pkg version of OpenThread there's a race condition between Idle state and packet arrival.
When the driver is in NETOPT_STATE_IDLE, is listening to packets.

When OpenThread sends packets, it calls otPlatRadioIdle first and then otPlatRadioTransmit. From the first call it expects the driver to be Idle and with receiver off. If there's a packet arrival between otPlatRadioIdle and otPlatRadioTransmit, otPlatRadioTransmit will throw kThreadError_Busy. This case shouldn't be possible in the OT implementation, so it executes an assert(false) and crashes. Although this could be fixed by simulating driver states (and dropping packets), I added this option for easily enabling/disabling the receiver and making it work properly.

The new implementation of OpenThread doesn't have this race condition (otPlatRadioIdle is not needed anymore), so it shouldn't be necessary to implement this RX_LISTENING option (I don't know if it's useful for other applications). I'm currently working in the pkg update to deal with this.

Best.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Jose, ah ok, thx for your explanation! Then I'll wait for the pkg update and give it an other try then.

From the first call it expects the driver to be Idle and with receiver off.

But isn't the "Idle" state (NETOPT_STATE_IDLE) in the current RIOT netdev2 interface to put the transceiver in RX mode? Idle state with receiver off would be called NETOPT_STATE_SLEEP.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi again.

I will try to get the new pkg working asap.

Concerning the SLEEP state, is not exactly the same as IDLE without RX. Although the result is very similar, in SLEEP the device is in a low power mode with the transceiver off. It can't send packets unless it goes again to an awake state (SLEEP -> IDLE -> TX).
In the enable/disable rx option you are still able to send packets. OpenThread puts the device to Idle (rx off), then sends a packet in that state.

Also OpenThread expects to have a SLEEP state as well, that matches with NETOPT_STATE_SLEEP.

Anyway, in the new version of OpenThread this RX_LISTENING option becomes unnecessary since there's no race condition in packet sending/arrival.

Best.

Copy link
Member

Choose a reason for hiding this comment

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

Would it maybe possible to wrap this behavior device-agnostic in the glue-code layer between OT and netdev2?

@biboc
Copy link
Member

biboc commented May 16, 2017

@jia200x @aabadie
I created a PR on @jia200x repo (jia200x#1):

  • Add support for OpenThread FTD (Full Thread Device), MTD (Minimal Thread Device) and NCP (Network Co-Processor) device
  • Apply correction from aabadie review

Known bug:

  • After a while a FTD catches an assert
  • A MTD does not manage to connect to a FTD on SAMR21

@aabadie
Copy link
Contributor

aabadie commented May 16, 2017

I created a PR on @jia200x repo (jia200x#1)

Thanks this helps a lot. I'm looking forward to see this merged in this PR. I still think that it would be a good thing to split this PR in several PRs:

  • one for the basic OT port
  • one for sock adaption
  • one for each other tests: ncp, ftd, mtd

Enable debug to show bug fram too long
@jia200x
Copy link
Member Author

jia200x commented May 18, 2017

@aabadie Roger. I proposed Baptiste to push his PR directly, so I can use this one for the core part. I will post another one with the sock adoption.

Cheers

@aabadie
Copy link
Contributor

aabadie commented May 18, 2017

I proposed Baptiste to push his PR directly, so I can use this one for the core part. I will post another one with the sock adoption

Do you mean that @biboc will open a new PR here with the core part ? That would be a good since GitHub has some lag with this huge thread ;)

@jia200x
Copy link
Member Author

jia200x commented May 18, 2017

I meant, the FTD and MTD part. But anyway, If it's easier, I can open another PR for the core and coordinate with @biboc.
Cheers!

@jia200x
Copy link
Member Author

jia200x commented May 18, 2017

@aabadie I think I addressed all issues in #7082. I was having inconsistencies in the Github viewer, and seemed to point to older files.

Cheers

@miri64
Copy link
Member

miri64 commented Jun 1, 2017

With #7082 next to being merged: is this PR still needed?

@aabadie
Copy link
Contributor

aabadie commented Jun 1, 2017

No I think we can safely close

@miri64
Copy link
Member

miri64 commented Jun 1, 2017

The line count indicates, that there is some code here, that isn't in #7082. So I close as memo.

@miri64 miri64 closed this Jun 1, 2017
@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable PR-award-nominee Deprecated. Will be removed soon. State: archived State: The PR has been archived for possible future re-adaptation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.