pkg/nordic_softdevice_ble: fix broken package #8068

dylad · 2017-11-17T09:12:21Z

This PR intends to fix #6379

Interaction with shell is now working when the nordic_softdevice_ble pkg is enabled
This PR is WIP & RFC because this is a very ugly revert of f656a31 (#6638) when the nordic_softdevice_ble package is used.

What must be done before merging :

~~Find a proper fix for VTOR when softdevice is enabled (not this ugly hack)~~ EDIT: Done
~~Fix the reset (when I push the reset button, the shell is not restart. How it is even possible ?)~~
~~Remove export FLASH_ADDR := 0x1f000 within boards/makefile.include and use the linker script instead (Is it possible ?)~~ (unfeasible right now)

I'll need the help of more experienced users to get this PR merged.

Related discussions #6379 #6638 #6865

miri64 · 2017-11-17T09:15:02Z

I think the title is very misleading. This is more about the interaction with the build system, not the shell. When first reading the description I thought you were talking about sys/shell ;-).

dylad · 2017-11-17T09:20:14Z

I think the title is very misleading

I know but I didn't find a suitable title for this one because the main issue was that the shell was dead with softdevice enable (#6379) but when I investigate this issue, I found that shell was OK but the CPU die early in the init process (within cortexm_init())
Feel free to rename this as you want :)

miri64 · 2017-11-17T09:33:35Z

I think the title should in general explain what the PR does. If it (coincidentally or not) fixes an issue that might describe other symptoms caused due to a bug the PR fixes, that should be mentioned in the PRs description.

Feel free to rename this as you want :)

I'm not sure was your PR does exactly, so I don't know to what ;-)

dylad · 2017-11-21T12:14:46Z

I've made a few changes on this, I had to change the CPU_DEFAULT_IRQ_PRIO value to 3 when softdevice is used because all the call to NVIC_SetPriority failed when softdevice is enabled. In fact, the IRQ prio 2 is reserved for the softdevice so we cannot have others IRQs at this level.
I also disable the call when softdevice is present NVIC_SetPriority(SVCall_IRQn,...) because my CPU was trapped in a hardfault. (But shell is working fine without... I didn't understand why yet...)

Finally I set the dedicated GPIO to the reset function which is (I think) important.

Any help on this PR is welcome :)

kYc0o

Small problem when not using softdevice, otherwise I tested and the thing seem to work again. Thanks for the patch!

kYc0o · 2017-11-21T15:35:49Z

boards/nrf52dk/Makefile.include

@@ -10,7 +10,7 @@ PORT_DARWIN ?= $(firstword $(sort $(wildcard /dev/tty.usbmodem*)))
 export JLINK_DEVICE := nrf52

 # special options when using SoftDevice
-ifneq (,$(filter nordic_softdevice_ble,$(USEPKG)))
+ifeq (,$(filter nordic_softdevice_ble,$(USEPKG)))


This change works when nordic_softdevice_ble is necessary, but for instance the hello-world example doesn't work... This needs to be defined somewhere else (after USEPKG += nordic_softdevice_ble is declared), but I don't have in mind right now where it's suitable.

As tested with @kYc0o, this check cannot work as nordic_softdevice_ble is added to USEPKG only during dependency resolution with happens after including Makefile.include.

dylad · 2017-11-21T15:42:03Z

Thanks for testing @kYc0o !
I'll try to find a solution when softdevice is not used.
Any idea how we can improve this patch ? I'm not happy with the changes within cpu/cortexm_common ...

kYc0o · 2017-11-21T16:52:52Z

Any idea how we can improve this patch ? I'm not happy with the changes within cpu/cortexm_common ...

Well as far as I see there's not too much that can be done, the softdevice takes priority over RIOT since it has most of initialisation code, vector table assignation, etc. so I don't see how can RIOT co-exist with such a blob without those modifications.

I'd say let's leave it as it is for now, since there are plans to add support for that radio on RIOT natively. By then maybe we'll deprecate softdevice.

dylad · 2017-11-21T20:33:13Z

since there are plans to add support for that radio on RIOT natively

It would be great ! But I don't think RIOT will support the BLE stack in a near future.

I'll try to find a way to fix everything soon, thanks for your help @kYc0o & @cladmi

kYc0o · 2017-11-22T15:13:36Z

Remove export FLASH_ADDR := 0x1f000 within boards/makefile.include and use the linker script instead (Is it possible ?)

AFAIK this is not possible, the linker script is already linking RIOT at that address and thus the flasher needs to know where to flash it, unless we flash HEX or ELF using openocd, which is not the case for nrf52dk, the only board supported by the softdevice.

dylad · 2017-11-22T15:19:03Z

AFAIK this is not possible, the linker script is already linking RIOT at that address and thus the flasher needs to know where to flash it

Then forget it. I'll try to find a fix for the USEPKG and I think this PR will be good to go :)

dylad · 2017-11-22T21:17:20Z

I found a quick workaround for the last issue. example/hello-world & example/gnrc_networking work fine now !

kYc0o

Some comments below. I think we can avoid to touch the general cortexm init code.

kYc0o · 2017-11-30T13:34:42Z

boards/nrf52dk/Makefile.include

@@ -10,6 +10,7 @@ PORT_DARWIN ?= $(firstword $(sort $(wildcard /dev/tty.usbmodem*)))
 export JLINK_DEVICE := nrf52

 # special options when using SoftDevice
+include $(RIOTBOARD)/$(BOARD)/Makefile.dep


As this is a workaround, I guess it can be valid. However, I think we need to put a note here stating that this is incorrect. We shouldn't include the dependencies makefile here, but only in another Makefile.dep. @cladmi might have a proposition about the needed comment.

I noticed this is also done for some arduino boards, I'll check why it's there and if we can avoid it.

Then I'll wait for your inputs for this point :)

kYc0o · 2017-11-30T13:36:13Z

cpu/cortexm_common/cortexm_init.c

 extern const void *_isr_vectors;
+#else
+#undef _isr_vectors
+#endif


Maybe this block can belong to the nrf52 cpu code?

I'll give it a try

kYc0o · 2017-11-30T13:37:40Z

cpu/cortexm_common/cortexm_init.c

    SCB->VTOR = (uint32_t)&_isr_vectors;
+#endif


Hmm... maybe this one you can also modify it in the cpu.c of nrf52. You might assign it to the starting of the flash, where I guess the vector table is located when using the specific softdevice hex firmware.

But then it will call twice during init
It is a bit weird isn't it ?

It would be, but we have no choice. I prefer to initialise it twice than having this #ifdefs in the cortexm code.

kYc0o · 2017-11-30T13:38:02Z

cpu/cortexm_common/cortexm_init.c

    /* set SVC interrupt to same priority as the rest */
    NVIC_SetPriority(SVCall_IRQn, CPU_DEFAULT_IRQ_PRIO);
+#endif


Is this really necessary?

RIOT is running wild if I call this line when I used softdevice. I didn't figure out why yet but I guess the softdevice does some black magic with this interrupt.

Ok, let's think about a way to get it out of here. I suppose we can also re-setting it at the nrf52 cpu level.

What about skipping the cortexm_init when softdevice is used ?

You're totally right!

Ok then, I'll try to push something like that :)

dylad · 2017-12-01T11:02:24Z

Thanks for your review @kYc0o ! I'll try to fix it this weekend or maybe next week. I'm really busy with company stuff right now.

dylad · 2017-12-02T22:27:02Z

@kYc0o I completely redo this PR with a --force-with-lease push and IMHO it is far more better now !
I removed the initialization of the reset pin as this should not belong to this PR and I'll provide this feature in a separate PR.
I tested with and without the softdevice and all my tests were running fine.

I didn't rebase this PR yet as I prefer we're all agree on the changes before last test and merge.

kYc0o · 2017-12-03T20:36:47Z

cpu/nrf52/cpu.c

    /* fixup swi0 (used as softdevice PendSV trampoline) */
    NVIC_EnableIRQ(SWI0_EGU0_IRQn);
    NVIC_SetPriority(SWI0_EGU0_IRQn, 6);
+#else
+    /* call cortexm default initialization */
+    cortexm_init();


Hmmm... are you sure all the functions there are unnecessary? You'll also need to increase the priority as you did before isn't it? Or with this configuration it works normally?

Well It works so far but I guess I could still add some part of the cortexm_init.
But since we have no clue of what the softdevice is doing maybe we should stay like this...
What do you think ?

I'd say as long as it works properly (and several initialisations are certainly done by the softdevice), we can leave it as is. I'll try to make some extensive tests soon.

Great ! many thanks @kYc0o

dylad · 2017-12-22T22:01:12Z

Rebased on lastest master.

kYc0o · 2018-01-02T15:08:42Z

I tried this again but still fails in the flashing step. Apparently the problem of knowing the dependencies is still present. Do you have a different behaviour @dylad ?

Prevent CPU from using cortexm_init() when softdevice is used for NRF52 devices as the softdevice already do some obscur inits Also ensure the softdevice pkg will be used by hacking Makefile due to RIOT's build system limitations (will be fix later with the new build system)

dylad · 2018-02-05T16:07:05Z

@cladmi Done.

@miri64 I did not break the logical units as you requested :)

dylad · 2018-02-05T20:02:42Z

Seems Murdock complains about unrelated stuff...
Anyone knows how to get rid of it ?

kYc0o · 2018-02-05T21:24:18Z

Is this unrelated?

gnrc_netif.c: In function '_update_l2addr_from_dev':
gnrc_netif.c:1132:5: error: expected expression before '||' token
     || defined(MODULE_NORDIC_SOFTDEVICE_BLE)

kaspar030 · 2018-02-05T21:26:13Z

sys/net/gnrc/netif/gnrc_netif.c

@@ -1121,6 +1129,8 @@ static void _update_l2addr_from_dev(gnrc_netif_t *netif)

    switch (netif->device_type) {
 #if defined(MODULE_NETDEV_IEEE802154) || defined(MODULE_XBEE)
+    || defined(MODULE_NORDIC_SOFTDEVICE_BLE)


I think the preprocessor needs a backslash here.

dylad · 2018-02-05T21:48:01Z

Oops sorry I did not pay attention enough...
Will fix.

dylad · 2018-02-05T22:03:02Z

Murdock doesn't like your message @cladmi I'm getting some funny outputs.

Add a warning message in user's console to inform them routing capabilities will be disabled with the softdevice

dylad · 2018-02-06T10:00:54Z

try $(warning ...) instead of info, that should make the output go to stderr

Thanks @kaspar030 that did the trick ! Murdock is happy :)

kaspar030 · 2018-02-06T10:03:18Z

Alright, let's get this in!

@kYc0o you happy here? :)

kYc0o · 2018-02-06T10:07:41Z

Alright!

dylad · 2018-02-06T10:12:51Z

It's finally over !!!!!!
@miri64 @cladmi @kaspar030 @kYc0o many thanks to you for helping me with this one :)

kaspar030 · 2018-02-06T10:13:28Z

It's finally over !!!!!!

Yeah, this was probably painful. Thanks a lot for following it through!

miri64 · 2018-02-06T10:35:54Z

@smlng Backport to release branch or is this too big for a hotfix?

smlng · 2018-02-06T12:34:21Z

its mentioned as broken in the release notes, so at least it is a known bug. However, if anyone cares to provide a backport, I'd still merge that into the release.

miri64 · 2018-02-06T13:28:21Z

merge that into the release.

release branch. The release is already out and immutable ;-).

smlng · 2018-02-06T13:29:56Z

yeah sure, merge into release branch ... but we could make a 2018.01.1 release later, too 😉

miri64 · 2018-02-06T13:30:41Z

yeah sure, merge into release branch ... but we could make a 2018.01.1 release later, too wink

ACK

dylad · 2018-02-06T13:31:44Z

I'll try to submit the backport tonight.

dylad assigned kaspar030, kYc0o and PeterKietzmann Nov 17, 2017

dylad changed the title ~~pkg/nordic_softdevice_ble: fix shell interaction~~ pkg/nordic_softdevice_ble: fix broken package Nov 17, 2017

kYc0o requested changes Nov 21, 2017

View reviewed changes

dylad mentioned this pull request Nov 21, 2017

boards/nrf52-based: factor out shared code #8102

Merged

kYc0o requested changes Nov 30, 2017

View reviewed changes

kYc0o mentioned this pull request Dec 1, 2017

boards/ACD52832: add new NRF52 based development board #8035

Merged

dylad force-pushed the fix_nordic_softdevice branch from a3eb0f3 to 16c9e4c Compare December 2, 2017 22:20

kYc0o reviewed Dec 3, 2017

View reviewed changes

dylad force-pushed the fix_nordic_softdevice branch from 16c9e4c to e3c2aef Compare December 22, 2017 21:58

dylad force-pushed the fix_nordic_softdevice branch from f72f94b to f6ec8b3 Compare February 5, 2018 16:05

kaspar030 reviewed Feb 5, 2018

View reviewed changes

miri64 added 3 commits February 5, 2018 22:34

nordic_softdevice_ble: fixes for gnrc_netif usage

51ae9d9

nordic_softdevice_ble: fix dependencies for NIB

0d55a8a

gnrc: 6Lo fixes for BLE

d4661af

dylad force-pushed the fix_nordic_softdevice branch from f6ec8b3 to 1a78ea2 Compare February 5, 2018 21:35

nordic_softdevice_ble: print when removing router modules

6db9baf

Add a warning message in user's console to inform them routing capabilities will be disabled with the softdevice

dylad force-pushed the fix_nordic_softdevice branch from 1a78ea2 to 6db9baf Compare February 6, 2018 09:45

kaspar030 approved these changes Feb 6, 2018

View reviewed changes

kYc0o approved these changes Feb 6, 2018

View reviewed changes

kaspar030 merged commit acaed9e into RIOT-OS:master Feb 6, 2018

dylad mentioned this pull request Feb 6, 2018

pkg/nordic_softdevice_ble: fix broken package [backport 2018.01] #8524

Merged

dylad deleted the fix_nordic_softdevice branch May 16, 2018 18:38

pkg/nordic_softdevice_ble: fix broken package #8068

pkg/nordic_softdevice_ble: fix broken package #8068

Uh oh!

Conversation

dylad commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Nov 17, 2017

Uh oh!

dylad commented Nov 17, 2017

Uh oh!

miri64 commented Nov 17, 2017

Uh oh!

dylad commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kYc0o 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!

dylad commented Nov 21, 2017

Uh oh!

kYc0o commented Nov 21, 2017

Uh oh!

dylad commented Nov 21, 2017

Uh oh!

kYc0o commented Nov 22, 2017

Uh oh!

dylad commented Nov 22, 2017

Uh oh!

dylad commented Nov 22, 2017

Uh oh!

kYc0o 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!

dylad commented Dec 1, 2017

Uh oh!

dylad commented Dec 2, 2017

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!

dylad commented Dec 22, 2017

Uh oh!

kYc0o commented Jan 2, 2018

Uh oh!

dylad commented Nov 17, 2017 •

edited

Loading

dylad commented Nov 21, 2017 •

edited

Loading

smlng commented Feb 6, 2018 •

edited

Loading

miri64 commented Feb 6, 2018 •

edited

Loading