Add support for Cordio to NRF52 Devices #8876

donatieng · 2018-11-27T01:05:34Z

Description

This PR adds optional support for the Cordio 5.0 stack to NRF52x targets.
This is achieved by running the Cordio Link Layer (released under the PBL) on these platforms.

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

cmonr · 2018-11-27T01:57:16Z

sigh
So this was the special PR...

And here I was ready to blow a gasket.

pan-

I can see a single .a file; is it a universal library ?
Could you mention qualification status in the README ?
What's the status on inclusion of uECC ?

pan- · 2018-11-27T08:29:48Z

features/FEATURE_BLE/ble/pal/GapEvents.h

@@ -306,7 +306,7 @@ struct GapAdvertisingReportEvent : public GapEvent {
    struct advertising_t {
        received_advertising_type_t type;
        connection_peer_address_type_t address_type;
-        const address_t& address;
+        address_t address;


Any reason for that change ?

The commit adding the initial change says "Fix advertising_t in GapEvents.h having a reference to an inexisting address_t instance". I'm not sure which usage of it caused the issue though.

...s/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_CORDIO/TARGET_NRF5x/NRFCordioHCIDriver.cpp

pan- · 2018-11-27T08:46:26Z

...s/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_CORDIO/TARGET_NRF5x/NRFCordioHCIDriver.cpp

+void NRFCordioHCIDriver::start_reset_sequence()
+{
+    // Make sure extended adv is init
+    DmExtAdvInit();


Any reason to put that here ?
That function is called by Cordio BLE if the controller supports extended advertising.

This needs to be called before the reset sequence to activate the extended reset sequence required to fetch extended advertising related info (number of advertising sets supported, etc)

I'm disappointed with the inconsistency of the reset sequence with EA; I suppose we should enable it by default in CordioBLE then if not supported fallback to legacy.

pan- · 2018-11-27T08:49:56Z

features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_CORDIO/TARGET_NRF5x/README.md

+* RADIO
+* TIMER0
+* PPI channels 14 and 15
+* HF clock is enabled


Is it always on ? That's gonna be an issue for power consumption.

0xc0170 · 2018-11-27T09:46:12Z

@pan- good catch, features/FEATURE_BLE/targets/TARGET_CORDIO_LL/cordio_stack/libcordio_stack.a needs license file and readme to decribe what it contains. please add it

paul-szczepanek-arm · 2018-11-27T10:36:10Z

features/FEATURE_BLE/targets/TARGET_CORDIO/source/CordioBLE.cpp

+    if (wsf_ticks > 0) {
+        WsfTimerUpdate(wsf_ticks);
+
+        _last_update_us -= (last_update_ms * 1000);


Suggested change

_last_update_us -= (last_update_ms * 1000);

_last_update_us -= (wsf_ticks * WSF_MS_PER_TICK * 1000);

otherwise you're losing milliseconds that aren't passed in as ticks

bulislaw

I skimmed the code and sanity checked the integration. Someone else needs to have a look at the code.

ghost · 2018-11-27T11:33:37Z

I can see a single .a file; is it a universal library ?

There are currently two .a files. One contains the Cordio LL library libcordio_stack.a and the other contains the nordic cordio stack lib_cordio_stack_nordic.a

Could you mention qualification status in the README ?

I'll add a license and README for each library

What's the status on inclusion of uECC ?

The uECC is included as source in TARGET_CORDIO_LL/thirdparty/uECC

donatieng · 2018-11-27T11:49:13Z

@scartmell-arm

I can see a single .a file; is it a universal library ?

There are currently two .a files. One contains the Cordio LL library libcordio_stack.a and the other contains the nordic cordio stack lib_cordio_stack_nordic.a

I think @pan- was wondering if it worked across all toolchains (both NRF52840 and NFR52832 are CM4s so we only need Armv7-m in terms of architecture)

0xc0170

The most of the files are for the library, looks good to me.

The only request is as my above comment - add licenses for binaries and readme describing them. As soon as they are in, will be approved by me

ghost · 2018-11-27T14:46:28Z

I think @pan- was wondering if it worked across all toolchains (both NRF52840 and NFR52832 are CM4s so we only need Armv7-m in terms of architecture)

While developing the compiled libraries worked with all 3 toolchains.

donatieng · 2018-11-27T14:48:08Z

Thanks for confirming @scartmell-arm

donatieng · 2018-11-27T17:46:34Z

@ARMmbed/mbed-os-maintainers we should be ready for CI

donatieng · 2018-11-27T18:21:41Z

(Added compiler info asked by @0xc0170)

Somehow during the rebase, both have been inverted.

pan- · 2018-11-28T11:34:26Z

@0xc0170 PR rebased:
*
93d8c71 fix a rebase error, somehow labels have been exchanged during the rebase.
*
c998248 reintroduce the implementation changes made for the Bluetooth 5 feature.

0xc0170 · 2018-11-28T11:51:59Z

CI started

0xc0170 · 2018-11-28T12:55:54Z

We are investigating failures now

0xc0170 · 2018-11-28T14:54:07Z

@OPpuolitaival Please review dynamic-memory-usage

0xc0170 · 2018-11-28T15:40:32Z

test and dynamic usage were restarted (issues should be fixed now in CI)

mbed-ci · 2018-11-28T15:48:02Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 4
Build artifacts
Build logs

0xc0170 · 2018-11-28T15:53:37Z

The last one for 5.11 rc1 is going in 💯

donatieng · 2018-11-28T16:31:58Z

YAY! Thanks @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-pan !

In PR ARMmbed#8876 when we added Cordio support for nRF52* targets, we attempted to use an RTOS idle hook to workaround sleep latency issues. However, the condition to bypass sleeps never gets satisfied, and BLE nRF52* targets have generally worked fine over the past year. This commit removes the hook to avoid dependency on RTOS, enabling BLE on bare metal.

donatieng requested review from a user and bulislaw November 27, 2018 01:15

donatieng added the release-version: 5.11.0-rc1 label Nov 27, 2018

cmonr added needs: review release-version: 5.12.0-rc1 release-version: 5.11.0-rc1 and removed release-version: 5.11.0-rc1 release-version: 5.12.0-rc1 labels Nov 27, 2018

cmonr requested review from a team and marcuschangarm November 27, 2018 01:56

cmonr added the risk: R label Nov 27, 2018

pan- reviewed Nov 27, 2018

View reviewed changes

paul-szczepanek-arm reviewed Nov 27, 2018

View reviewed changes

bulislaw approved these changes Nov 27, 2018

View reviewed changes

0xc0170 requested changes Nov 27, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Nov 27, 2018

adbridge approved these changes Nov 27, 2018

View reviewed changes

pan- approved these changes Nov 27, 2018

View reviewed changes

cmonr added needs: CI and removed needs: work labels Nov 27, 2018

Steve Cartmell and others added 11 commits November 28, 2018 10:26

refactor(pan-cordio): Make nordic softdevice the default link layer

4f7dcf4

doc(pan-cordio): Add documentation to README on how to enable Cordio LL

1b3efa4

Amend Nordic HCI driver to support BLE5 features

3ce1d91

Add number of advertising sets

f980814

Fix some rebasing quirks

89aabae

Fix a few more rebasing issues

5a87cfa

doc(pan-cordio): Add readme and license documents for cordio link layer

a3bae1c

doc(pan-cordio): Add toolchain version used to compile cordio-ll readme

353e991

NRF51: Add label to compile BLE softdevice implementation.

837ba72

NORDIC: Fix labels in target.

93d8c71

Somehow during the rebase, both have been inverted.

BLE: Reintroduce changes to address types lost.

c998248

pan- force-pushed the public_pr_cordio_nordic_ll branch from 7834219 to c998248 Compare November 28, 2018 11:32

0xc0170 added needs: CI and removed needs: work labels Nov 28, 2018

0xc0170 added ready for merge and removed needs: CI labels Nov 28, 2018

0xc0170 merged commit e2ae84e into ARMmbed:master Nov 28, 2018

0xc0170 removed the ready for merge label Nov 28, 2018

cmonr removed the risk: R label Nov 28, 2018

pan- mentioned this pull request Nov 28, 2018

NRF52840: setPhy method is not implemented. #8635

Closed

LDong-Arm mentioned this pull request May 11, 2020

NRFCordioHCIDriver: remove idle_hook (and RTOS dependency) #12956

Merged

	_last_update_us -= (last_update_ms * 1000);
	_last_update_us -= (wsf_ticks * WSF_MS_PER_TICK * 1000);

Add support for Cordio to NRF52 Devices #8876

Add support for Cordio to NRF52 Devices #8876

Uh oh!

Conversation

donatieng commented Nov 27, 2018

Description

Pull request type

Uh oh!

cmonr commented Nov 27, 2018

Uh oh!

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

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!

0xc0170 commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 27, 2018

Uh oh!

donatieng commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 27, 2018

Uh oh!

donatieng commented Nov 27, 2018

Uh oh!

donatieng commented Nov 27, 2018

Uh oh!

donatieng commented Nov 27, 2018

Uh oh!

pan- commented Nov 28, 2018

Uh oh!

0xc0170 commented Nov 28, 2018

Uh oh!

0xc0170 commented Nov 28, 2018

Uh oh!

0xc0170 commented Nov 28, 2018

Uh oh!

0xc0170 commented Nov 28, 2018

Uh oh!

mbed-ci commented Nov 28, 2018

Uh oh!

0xc0170 commented Nov 28, 2018

Uh oh!

donatieng commented Nov 28, 2018

Uh oh!

Uh oh!

donatieng commented Nov 27, 2018 •

edited

Loading