pkg/nimble: add IP-over-BLE support via netif/GNRC by haukepetersen · Pull Request #11578 · RIOT-OS/RIOT

haukepetersen · 2019-05-24T16:05:45Z

Contribution description

This PR provides support for IP-over-BLE by providing a GNRC-netif wrapper for Nimble. The provided behavior is should be conform to RFC7668 and the Bluetooth IPSP profile, but some details do still need to be verified.

The PR provides an application for 'fully manual' control of BLE behavior. Other means for BLE connection management (e.g. towards ipv6-mesh-over-ble) will follow in upcoming PRs.

Current State:

code (architecture, structure, implementation) is functional and ready to be looked at
a lot of documentation (high-level and doxygen) is missing (hence the WIP label)
some debug helpers are left in the code, will remove them once everything runs stable

I PR this code now, as it would be wonderful if someone could look over the code to see if there are some critical things I overlooked. I've been staring at this stuff for too long now and a bad tunnel vision :-)

Testing procedure

The provided nimble_gnrc application provides a mighty ble shell command, that can be used for advertising, scanning, and connecting to other nodes. Once at least two nodes are connected, all the IP magic (udp, icmp, ifconfig, rpl) is available...

At least in theory, this code should also work for connecting a RIOT BLE node to Linux. But I did not try this, yet.

Issues/PRs references

rebased on #11296

Known issues:

using the unpatched Nimble version a node can only have a single connection. If a second connection is opened, Nimble fails to allocate a second connection context from its context buffer (see nimble/host/src/ble_hs_conn.c -> ble_hs_conn_alloc()). I can't tell why, but it seems the memblock list for ble_hs_conn_pool is corrupted somehow. But when renaming that var to _ble_hs_conn_pool everything works again. So really now idea yet, what is causing this...
I did not fully verify, that GNRC behaves fully to spec of RFC7668 (e.g. neighbor discovery options...)

miri64

First (superficial look) at the GNRC glue-code looks good to me.

ghost · 2019-05-27T10:31:49Z

I tested this PR with a little tweaking as you explained to me in person. The workaround targets a bug in nimble that led to broken states, when trying to connect to multiple boards simultaniously.
Workaround:

Clone and checkout https://github.com/haukepetersen/mynewt-nimble/tree/tmp_ble_hs_conn_fix
change the pkg source in .../RIOT/pkg/nimble/Makefile by adding PKG_SOURCE_LOCAL=<location of the mynewt-branch> below PKG_LICENSE.

When doing so multihop routing with rpl works like a charm. I tested this with 2 nrf52dk as leafs and a nrf52840dk as root. Also sending pings in parallel over 2 hops works well as well as sending a udp package to a server.

Without this workaround I can reproduce the bug that broke the statemachine and hence disabled the first connection as well. However udp and ping communication work without the workaround between exactly 2 nodes (nrf52840dk <-> nrf52dk) as expected.

Whats your opinion @haukepetersen, do you want this PR in without fixing the bug in nimble first or is this PR on hold until the bug is purged?

ghost · 2019-05-27T11:58:53Z

examples/nimble_gnrc/ble.c

+
+int app_ble_cmd(int argc, char **argv)
+{
+    if ((argc == 1) || _ishelp(argv[1])) {


A bit crowded and hard to read. Might be a good application for getopt().

true, but I would take this as optimization that might be done in the future. This is IMO not the most relevant code path anyway.

miri64

Now some more in-depth review. As stated at first-glance, for the most part the glue code is ok, but the error codes are not mapped properly for the upper layer.

pkg/nimble/netif/nimble_netif.c

haukepetersen · 2019-06-04T12:45:37Z

addressed comments and rebased

haukepetersen · 2019-06-06T08:40:29Z

This PR is now ready for actual review and testing, so I removed the WIP tag.

I is however still rebased on #11640

haukepetersen · 2019-06-06T12:18:39Z

rebased

miri64

My review of the example application. I think with introducing shell commands instead and fixing the gnrc_ipv6_default/gnrc_sixlowpan_default dependencies it is not necessary to duplicate the gnrc_networking application. Sorry I did not spot this earlier.

miri64 · 2019-06-06T17:48:14Z

examples/nimble_gnrc/README.md

@@ -0,0 +1 @@
+TODO


The README still is not provided here ;-).

miri64 · 2019-06-06T17:49:06Z

examples/nimble_gnrc/Makefile

+# Specify the mandatory networking modules for IPv6 and UDP
+USEMODULE += auto_init_gnrc_netif
+USEMODULE += gnrc_ipv6_router_default
+USEMODULE += gnrc_sixlowpan_router_default


This is redundant. Rather put this as a dependency of nimble + gnrc_ipv6_router_default like the rest does it.

miri64 · 2019-06-06T17:49:18Z

examples/nimble_gnrc/Makefile

+
+# Specify the mandatory networking modules for IPv6 and UDP
+USEMODULE += auto_init_gnrc_netif
+USEMODULE += gnrc_ipv6_router_default


If there is no mesh support yet, why router?

not true, we do have (some sort of) mesh support build in

All the more argument to not have this a dedicated example but to fold this into gnrc_networking.

miri64 · 2019-06-06T18:08:13Z

examples/nimble_gnrc/app.h

+
+int app_ble_cmd(int argc, char **argv);
+
+int app_udp_cmd(int argc, char **argv);


Here and above: Doc missing?

yes, will fix.

miri64 · 2019-06-06T18:10:20Z

examples/nimble_gnrc/ble.c

+    return memcmp(argv, "help", 4) == 0;
+}
+
+void app_ble_init(void)


Couln't this and the callback go to netif->ops->init()? It does not look very dependent on the application for me.

but it is.

The scanlist´ has nothing to do with nimble/netif`, and the callback is only valid for this application...

miri64 · 2019-06-06T18:12:15Z

examples/nimble_gnrc/ble.c

@@ -0,0 +1,364 @@
+


This file could also go to shell_commands, couldn't it? I believe then (and with the dependency fixes proposed in the Makefile) we could get away with not duplicating gnrc_networking ;-).

not really. The thing is, that these shell commands are quite application specific. I have some other code almost ready to be PRed, that does some sort of automated connection management (nimble_autoconn (connect to anything that fits some filter criteria) and nimble_rble (RPL-over-BLE)). Both use their own specific event callback implementations and would clash badly with these shell command implemenation here. So the shell command is not as generic as it seems...

maybe sc_nimble_manual_scan then? You say yourself, that these commands has nothing to do with nimble or gnrc_netif themselves, but rather are for managing connections. So a potential for duplication is there while they do not contributing to examplify GNRC+Nimble (i.e. have not much to do with this example).

haukepetersen · 2019-06-11T07:54:30Z

@miri64 I spend some more thoughts on the shell-command issue: I think I have a rough idea what I could do so take it out of the application, and hence allow for using the gnrc_netoworking example after all. I need a little time to evaluate this idea though. So for now: I'd like to keep this example as is, and would prefer to rework it in a follow up. Would that work for you?

miri64 · 2019-06-11T11:02:45Z

So for now: I'd like to keep this example as is, and would prefer to rework it in a follow up. Would that work for you?

Let's do it like this: I still need some time to review the whole PR anyways, so if until then your evaluation comes to a close we decide then. Overall, I'd really like to avoid setting yet another precedence of code duplication within the code base if it is avoidable.

haukepetersen · 2019-07-05T11:58:38Z

pushed some updates:

made NimBLE the default link layer for GNRC on nrf52 based boards (instead of the Nordic Softdevice)
removed the nimble_gnrc example (in favor of using it as default for gnrc_networking)
moved the nimble_netif (ble) shell command to the sys/shell module

haukepetersen · 2019-07-05T11:59:54Z

And just a heads up: I would like to get this code in as is (after its approved and possible fixed, that is). I will then restructure parts of it again to cater for the upcoming CCNLIte integration and some automatic connection manager modules...

pkg/nimble/Makefile.dep

The blacklisted boards use NimBLE as default network stack. But NimBLE and ndn-riot do not build simultaneously, as they use clashing crpyto libraries (uECC vs tinycrypt).

haukepetersen · 2019-08-28T14:49:49Z

squashed. Now lets see... :-)

miri64 · 2019-08-28T14:52:10Z

Running tests on boards, to make sure we don't break the nightlies with all our rat tail fixes.

miri64 · 2019-08-28T15:10:44Z

Let's go. Thanks test caching for the quick run 😁

haukepetersen · 2019-08-28T15:11:27Z

Wonderful, thanks a lot for the review effort!

kaspar030 · 2019-08-29T12:58:53Z

Awesome! 👍 👍

emmanuelsearch · 2019-08-29T13:06:34Z

ditto ;)) thumbs up from over here too!!

miri64 self-requested a review May 24, 2019 16:09

miri64 reviewed May 24, 2019

View reviewed changes

ghost self-requested a review May 27, 2019 09:24

ghost reviewed May 27, 2019

View reviewed changes

miri64 requested changes May 27, 2019

View reviewed changes

haukepetersen force-pushed the add_nimble_netif branch from 8238b90 to 8127f88 Compare June 4, 2019 12:42

haukepetersen mentioned this pull request Jun 6, 2019

sys/bluetil: add bluetil_addr_from_str() #11640

Merged

haukepetersen added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jun 6, 2019

haukepetersen force-pushed the add_nimble_netif branch from 83fdce8 to 1a579b1 Compare June 6, 2019 12:18

haukepetersen removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 6, 2019

miri64 requested changes Jun 6, 2019

View reviewed changes

haukepetersen mentioned this pull request Jul 4, 2019

tests: add Nordic SoftDevice test app #11797

Merged

haukepetersen force-pushed the add_nimble_netif branch from 1a579b1 to a19fa34 Compare July 5, 2019 11:57

haukepetersen mentioned this pull request Jul 5, 2019

npl/riot: strip down the default configuration apache/mynewt-nimble#508

Merged

miri64 requested changes Jul 5, 2019

View reviewed changes

pkg/nimble/Makefile.dep Outdated Show resolved Hide resolved

pkg/nimble/Makefile.dep Outdated Show resolved Hide resolved

miri64 self-assigned this Jul 30, 2019

haukepetersen force-pushed the add_nimble_netif branch from 64d0edb to 52d1f38 Compare July 31, 2019 15:59

haukepetersen added 13 commits August 28, 2019 16:48

boards/nrf52dk: add nimble_netif dependency

14662da

net/ble.h: add l2cap cid and gapp svc id constants

1cb5465

pkg/nimble: add netif wrapper

6bbe166

sys/shell: add 'ble' shell cmd for nimble_netif

a3b8c87

Makefile.dep: add dep for nimble submodules

ce3fedf

boards/nrf52: set NimBLE for gnrc_netdev_default

d127d18

pkg/nimble: add README on how to connect to Linux

eb6673f

board/nrf52840dk: fix order in Makefile.dep

da49425

examples/ndn-ping: blacklist nrf52-based boards

3a381d8

The blacklisted boards use NimBLE as default network stack. But NimBLE and ndn-riot do not build simultaneously, as they use clashing crpyto libraries (uECC vs tinycrypt).

pkg/nimble: temporary LLVM workaround for NimBLE

891294d

boards/reel: make nrf802154 the default radio

dff436b

tests/l2util: adapt to changes for BLE

a237971

tests/unittests/bluetil: adapt to byteorder fixes

16bbd2f

haukepetersen force-pushed the add_nimble_netif branch from 2eb5d40 to 16bbd2f Compare August 28, 2019 14:49

miri64 merged commit d6ac49c into RIOT-OS:master Aug 28, 2019

haukepetersen deleted the add_nimble_netif branch August 28, 2019 15:11

haukepetersen mentioned this pull request Aug 29, 2019

pkg/nimble: fix doxygen group name prefix #12112

Merged

aabadie mentioned this pull request Aug 29, 2019

boards/nrf52: introduce shared dependencies in common/nrf52 and update boards #11792

Merged

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

fjmolinas mentioned this pull request Oct 16, 2019

gnrc networking crashes on nRF51dk #11390

Closed

haukepetersen mentioned this pull request Nov 12, 2019

IPv6 support over Bluetooth apache/mynewt-nimble#25

Closed


		int app_ble_cmd(int argc, char **argv);

		int app_udp_cmd(int argc, char **argv);

		@@ -0,0 +1 @@
		TODO

Conversation

haukepetersen commented May 24, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented May 27, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haukepetersen commented Jun 4, 2019

Uh oh!

haukepetersen commented Jun 6, 2019

Uh oh!

haukepetersen commented Jun 6, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jun 11, 2019

Uh oh!

miri64 commented Jun 11, 2019

Uh oh!

haukepetersen commented Jul 5, 2019

Uh oh!

haukepetersen commented Jul 5, 2019

Uh oh!

Uh oh!

Uh oh!

haukepetersen commented Aug 28, 2019

Uh oh!

miri64 commented Aug 28, 2019

Uh oh!

miri64 commented Aug 28, 2019

Uh oh!

haukepetersen commented Aug 28, 2019

Uh oh!

kaspar030 commented Aug 29, 2019

ghost commented May 27, 2019 •

edited by ghost

Loading