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
pkg/nimble: add IP-over-BLE support via netif/GNRC #11578
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First (superficial look) at the GNRC glue-code looks good to me.
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.
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? |
examples/nimble_gnrc/ble.c
Outdated
|
||
int app_ble_cmd(int argc, char **argv) | ||
{ | ||
if ((argc == 1) || _ishelp(argv[1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit crowded and hard to read. Might be a good application for getopt().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8238b90
to
8127f88
Compare
addressed comments and rebased |
This PR is now ready for actual review and testing, so I removed the WIP tag. I is however still rebased on #11640 |
rebased |
83fdce8
to
1a579b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
examples/nimble_gnrc/README.md
Outdated
@@ -0,0 +1 @@ | |||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README still is not provided here ;-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ups
examples/nimble_gnrc/Makefile
Outdated
# Specify the mandatory networking modules for IPv6 and UDP | ||
USEMODULE += auto_init_gnrc_netif | ||
USEMODULE += gnrc_ipv6_router_default | ||
USEMODULE += gnrc_sixlowpan_router_default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant. Rather put this as a dependency of nimble
+ gnrc_ipv6_router_default
like the rest does it.
examples/nimble_gnrc/Makefile
Outdated
|
||
# Specify the mandatory networking modules for IPv6 and UDP | ||
USEMODULE += auto_init_gnrc_netif | ||
USEMODULE += gnrc_ipv6_router_default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no mesh support yet, why router?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not true, we do have (some sort of) mesh support build in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the more argument to not have this a dedicated example but to fold this into gnrc_networking
.
examples/nimble_gnrc/app.h
Outdated
|
||
int app_ble_cmd(int argc, char **argv); | ||
|
||
int app_udp_cmd(int argc, char **argv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and above: Doc missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will fix.
examples/nimble_gnrc/ble.c
Outdated
return memcmp(argv, "help", 4) == 0; | ||
} | ||
|
||
void app_ble_init(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couln't this and the callback go to netif->ops->init()
? It does not look very dependent on the application for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it is.
The scanlist´ has nothing to do with
nimble/netif`, and the callback is only valid for this application...
examples/nimble_gnrc/ble.c
Outdated
@@ -0,0 +1,364 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
;-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@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 |
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. |
1a579b1
to
a19fa34
Compare
pushed some updates:
|
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... |
64d0edb
to
52d1f38
Compare
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).
2eb5d40
to
16bbd2f
Compare
squashed. Now lets see... :-) |
Running tests on boards, to make sure we don't break the nightlies with all our rat tail fixes. |
Let's go. Thanks test caching for the quick run 😁 |
Wonderful, thanks a lot for the review effort! |
Awesome! 👍 👍 |
ditto ;)) thumbs up from over here too!! |
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:
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 mightyble
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:
nimble/host/src/ble_hs_conn.c
->ble_hs_conn_alloc()
). I can't tell why, but it seems the memblock list forble_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...