pkg/nimble: adapt to nrf51 family by aabadie · Pull Request #11463 · RIOT-OS/RIOT

aabadie · 2019-04-29T09:16:05Z

Contribution description

This PR is adapting the Nimble package to make it compliant with nrf51 based boards. ~~For the moment it needs a specific branch on my Apache-nimble fork (see apache/mynewt-nimble#429).~~ Some changes were applied on the upstream project, so the package version had to be updated.

I tested examples/nimble_gatt and it worked with nrf51dk. The examples/nimble_scanner doesn't work, maybe because of insufficient memory.

Testing procedure

Build and flash examples/nimble_gatt on an nrf51 based board. It should appear in NRF connect application running on a Phone or using bluetoothctl on a Linux with bluetooth radio enabled.

Issues/PRs references

None

haukepetersen · 2019-04-29T13:09:30Z

Nice one, wanted to do this for a while now :-)

See my review comments below.

haukepetersen

I just see a problem when (in the near future) building Nimble for non-Nordic platforms. But easy to fix...

haukepetersen · 2019-04-29T13:11:20Z


-nimble_drivers_nrf52:
-	"$(MAKE)" -C $(PDIR)/nimble/drivers/nrf52/src/ -f $(TDIR)/drivers.nrf52.mk
+nimble_drivers:


I would leave this as NRF specific submodule, as it is only valid for these CPUs at the moment. So how about nimble_drivers_nrf5x?!

haukepetersen · 2019-04-29T13:11:51Z

 PKG_NAME    = nimble
 PKG_URL     = https://github.com/apache/mynewt-nimble.git
-PKG_VERSION = a7b3c939146e735b59d55bff740c906bde6f86f9
+PKG_VERSION = 976bb8f84bf9547efdea444ae79a62d5a355a613


If we merge #11273, this line should not be needed, right?

Not sure, because apache/mynewt-nimble@976bb8f was merged this morning so it's more recent.

True, didn't refresh my page. But the trouble with updating the version holds (see #11273). So I think it makes much sense to merge #11273 first.

No problem, this is not urgent.

haukepetersen · 2019-04-29T13:12:23Z

-  ifeq (nrf52,$(CPU_FAM))
-    USEMODULE += nimble_drivers_nrf52
-  endif
+  USEMODULE += nimble_drivers


don't drop the ifdef, as the nimble_drivers submodule should only be included for nordic boards...

haukepetersen · 2019-04-29T13:13:25Z

 #endif

 #ifdef MODULE_NIMBLE_CONTROLLER
-#ifdef CPU_FAM_NRF52


the ifdef needs to stay, and simply be extended with the CPU_FAM_NRF51. Otherwise we get in trouble when building for non Nordic targets (in the future...).

aabadie · 2019-04-29T14:51:51Z

Comments addressed, @haukepetersen !

haukepetersen · 2019-05-02T13:08:06Z

Looks good! However, I would like to wait until the issues from #11273 are resolved. I will look into this, but also have a paper deadline next Friday with a slightly higher prio :-)

aabadie · 2019-05-06T13:36:13Z

However, I would like to wait until the issues from #11273 are resolved.

No problem, there is no hurry.

I did some more testing on microbit and nrf51dk and there are problems:

microbit seems to crash at startup: no output displayed
nrf51dk appears in nRFConnect but the connection fails afterwards and I have to reset the board to see it again in the scan results.

ghost · 2019-05-17T09:32:51Z

So, now that #11273 is merged, please rebase.

aabadie · 2019-05-17T14:28:01Z

now that #11273 is merged, please rebase.

Done!

ghost · 2019-05-24T08:45:09Z

I had multiple errors on calliope-mini (nRF51822). I got adviced by @haukepetersen on compiling.

I tried compiling tests/nimble_l2cap but had overflow of 48kB. We tried to fit it in anyway by lowering APP_MTU ?= 1000, MYNEWT_VAL_MSYS_1_BLOCK_COUNT=7 and MYNEWT_VAL_MSYS_1_BLOCK_SIZE=110, but without success.
I tried examples/nimble_gatt, it does compile and it fits, yet I was not able to connect to the calliope from my phone for unknown reason.

Sadly we dont have a nrf52dk here to verify this PR atm.

aabadie · 2019-05-24T08:48:54Z

Sadly we dont have a nrf52dk here to verify this PR atm.

Do you mean nrf51dk ? If yes try one on IoT-LAB :)

ghost · 2019-05-24T08:56:44Z

Sadly we dont have a nrf52dk here to verify this PR atm.

Do you mean nrf51dk ? If yes try one on IoT-LAB :)

Yes, sorry for the confusion ;)

janosbrodbeck · 2019-05-29T13:27:30Z

We found a problem with the xtimer module with this adaption. We used an airfy-beacon board (nrf51822) for testing.

xtimer functions without the nimble package are working properly. But if we just add the nimble pkg into our project/makefile xtimer does not work anymore.
xtimer_now_usec() returns 0 everytime.

xtimer_sleep therefore no longer behaves in a comprehensible way. Sometimes the device wakes up after a short time, sometimes it takes minutes or probably infinite .

nimble itself seems to work properly. Advertising, scanning works.

haukepetersen · 2019-05-29T16:00:56Z

I can confirm the issue using the examples/nimble_scanner application for both microbit and calliope-mini. I re-checked the nimble configuration regarding timers, but couldn't find anything there: nimble's controller is using NRF_RTC0, while xtimer is using NRF_TIMER0, so no clash there.

This will need more investigation :-)

haukepetersen · 2019-05-29T16:15:15Z

I might have found something interesting (still using the nimble_scanner example):
a) the perih timer 0 (used by xtimer) is stopped when the main() function is executed -> this should not be, so might be that Nimble disables it somehow()
b) enabling the periph timer manually (calling timer_start(0)), will allow to use xtimer_sleep() before starting the shell. But when calling e.g. the scan 100, the periph timer ends up triggering its ISR in an endless loop.

So it seems to me, that something regarding interrupt handling is clashing between RIOT and Nimble on the nrf51. So more debugging :-)

haukepetersen · 2019-05-29T16:40:34Z

found it: Nimbles radio driver is also using NRF_TIMER0, hard-coded in nimble/drivers/nrf52/src/ble_phy.c. This explains of course, why both xtimer and Nimble are not working in parallel and the timer is disabled...

Quick fix: mapping xtimer to use timer 1 seems to fix the issue at hand. But caution: timer 1 runs in 16-bit mode, so one has to define custom values for all XTIMER_DEV, XTIMER_WIDTH, and XTIMER_CHAN.

So I guess we need to include some nimble specific xtimer configuration?!

aabadie · 2019-05-29T20:24:41Z

Good catch @haukepetersen :)

Quick fix: mapping xtimer to use timer 1 seems to fix the issue at hand. But caution: timer 1 runs in 16-bit mode, so one has to define custom values for all XTIMER_DEV, XTIMER_WIDTH, and XTIMER_CHAN

All nrf51 based boards share the same xtimer defines (in board_common.h) and, except microbit and calliope-mini, the same timer peripherals configuration. So this is doable without too many changes. We could also have some conditional macros to use timer 1 if nimble pkg is loaded and timer 0 otherwise.

haukepetersen · 2019-05-31T11:01:03Z

We could also have some conditional macros to use timer 1 if nimble pkg is loaded and timer 0 otherwise.

+1! I was thinking the same thing. We should just put a comment to it, stating that with nimble xtimer falls back to a 16-bit timer -> more wakeups...

aabadie · 2019-06-03T12:09:02Z

I was thinking the same thing. We should just put a comment to it, stating that with nimble xtimer falls back to a 16-bit timer -> more wakeups...

Do you want this in another PR or I can just update this one ?

haukepetersen · 2019-06-03T12:14:55Z

Do you want this in another PR or I can just update this one ?

Definitely in this PR. Wanted to PR something against your branch, but couldn't find the time so far. So just go ahead, I will happlily review it then :-)

haukepetersen · 2019-06-04T10:44:34Z

I will look into refactoring the code accordingly right now, so please wait for a PR from my side before addressing this issue.

janosbrodbeck · 2019-06-24T00:26:21Z

Well, I've found a new bug :)

The random module makes problem. Compiled together with the package nimble the board does not react anymore after flashing. Tested on airfy-beacon and nrf51dk (iotlab).

It works on nrf52dk!

haukepetersen · 2019-08-01T09:56:57Z

@j-brodbeck the random module issue seems to be caused by both NimBLE and RIOT using the nRFs hwrng peripheral, interfering with each other. I have not looked into the complete details, but the solutions is probably somewhere in one of both falling back to using the other ones implementation...

So as roadmap for this PR I would like to proceed as follows:

wait until npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum apache/mynewt-nimble#477 and npl/riot: strip down the default configuration apache/mynewt-nimble#508 are merged
update the NimBLE package verison
merge this PR as is
create a separate issue for the hwrng issue and take it from there

haukepetersen · 2019-08-01T14:56:13Z

Alright, all dependencies have been merged to nimble master and the nimble package version has been bumped -> #11949

@aabadie would you mind to rebase this PR?!

aabadie · 2019-08-04T14:07:17Z

would you mind to rebase this PR?

Done :)

haukepetersen

Works as expected and code looks good -> ACK

Note: so far, this PR does not enable the ble_nimble feature for any nrf51-based board. I think we should leave it like this and I/we can do this an a separate PR...

haukepetersen · 2019-08-05T11:23:36Z

oh, and please squash :-)

aabadie · 2019-08-05T11:26:26Z

this PR does not enable the ble_nimble feature for any nrf51-based board

I'll update this PR otherwise the adaption for nrf51 is not built by Murdock for nrf51 based boards.

haukepetersen · 2019-08-05T11:28:13Z

true, makes sense.

haukepetersen · 2019-08-05T12:44:25Z

made a PR against your branch, fixing the nimble_l2cap test application. Maybe you have a better idea how to do this a little nicer? -> aabadie#13

aabadie · 2019-08-07T07:54:46Z

@haukepetersen, I applied your commit directly, squashed and rebased. Let's now see how Murdock behaves.

haukepetersen · 2019-08-07T07:58:57Z

thanks! Go Murdock, go :-)

aabadie · 2019-08-07T11:49:30Z

It's all green @haukepetersen, ok to merge ?

haukepetersen · 2019-08-07T11:54:27Z

yes. -> go :-)

aabadie added Area: pkg Area: External package ports Area: BLE Area: Bluetooth Low Energy support labels Apr 29, 2019

aabadie requested a review from haukepetersen April 29, 2019 09:16

aabadie mentioned this pull request Apr 29, 2019

examples/nimble_scanner: fix typo in scan command description #11464

Merged

aabadie force-pushed the nimble_nrf51 branch from 7ce3a23 to 0b440fb Compare April 29, 2019 09:51

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 29, 2019

haukepetersen requested changes Apr 29, 2019

View reviewed changes

aabadie force-pushed the nimble_nrf51 branch from 0b440fb to 8ab25c3 Compare April 29, 2019 14:51

fjmolinas mentioned this pull request Apr 30, 2019

examples/nimble*: add more nrf52 based boards in whitelist #11372

Closed

aabadie force-pushed the nimble_nrf51 branch from 8ab25c3 to a531b76 Compare May 17, 2019 14:27

ghost self-requested a review May 24, 2019 08:46

haukepetersen mentioned this pull request Jun 4, 2019

boards/nrf51-based: split shared timer/rtt config #11625

Merged

haukepetersen mentioned this pull request Aug 1, 2019

drivers/nrf5x: do not assign IRQ prio when building for RIOT apache/mynewt-nimble#483

Merged

haukepetersen mentioned this pull request Aug 1, 2019

pkg/nimble: bump version to 997dad8 #11949

Merged

aabadie force-pushed the nimble_nrf51 branch from d06fac0 to 67b123f Compare August 4, 2019 14:06

haukepetersen approved these changes Aug 5, 2019

View reviewed changes

aabadie force-pushed the nimble_nrf51 branch from 67b123f to b6ed33a Compare August 5, 2019 12:58

aabadie and others added 4 commits August 7, 2019 09:53

pkg/nimble: adapt to nrf51 family

701ba9c

boards/common/nrf51: add alternative xtimer mapping

c9e5d2c

boards/nrf51: provide ble_nimble feature

430d807

tests/nimble_l2cap: adjust buffer sizes for nrf51

b72f606

aabadie force-pushed the nimble_nrf51 branch from 89a8e70 to b72f606 Compare August 7, 2019 07:54

haukepetersen merged commit 4a75d0b into RIOT-OS:master Aug 7, 2019

aabadie deleted the nimble_nrf51 branch August 8, 2019 18:45

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

Uh oh!

Conversation

aabadie commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

haukepetersen commented Apr 29, 2019

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

haukepetersen Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

haukepetersen Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

haukepetersen Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

haukepetersen Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie commented Apr 29, 2019

Uh oh!

haukepetersen commented May 2, 2019

Uh oh!

aabadie commented May 6, 2019

Uh oh!

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

Uh oh!

Uh oh!

aabadie commented May 17, 2019

Uh oh!

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

Uh oh!

Uh oh!

aabadie commented May 24, 2019

Uh oh!

ghost commented May 24, 2019

Uh oh!

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

Uh oh!

Uh oh!

haukepetersen commented May 29, 2019

Uh oh!

haukepetersen commented May 29, 2019

Uh oh!

haukepetersen commented May 29, 2019

Uh oh!

aabadie commented May 29, 2019

Uh oh!

haukepetersen commented May 31, 2019

Uh oh!

aabadie commented Jun 3, 2019

Uh oh!

haukepetersen commented Jun 3, 2019

Uh oh!

haukepetersen commented Jun 4, 2019

Uh oh!

janosbrodbeck commented Jun 24, 2019

Uh oh!

haukepetersen commented Aug 1, 2019

Uh oh!

haukepetersen commented Aug 1, 2019

Uh oh!

aabadie commented Aug 4, 2019

Uh oh!

haukepetersen left a comment

aabadie commented Apr 29, 2019 •

edited

Loading

ghost commented May 17, 2019 •

edited by ghost

Loading

ghost commented May 24, 2019 •

edited by ghost

Loading

janosbrodbeck commented May 29, 2019 •

edited

Loading