Skip to content
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

Default to Cordio BLE stack for NRF52* targets #10709

Merged
merged 5 commits into from Jun 9, 2019

Conversation

Projects
None yet
9 participants
@LDong-Arm
Copy link
Contributor

commented May 29, 2019

Description

The BLE stack from SoftDevice is not actively maintained and
has issues (e.g. BLE fails to initialise) when used with Nordic SDK v15.

Note: To verify this PR properly, remove/do not set target-specific labels in mbed_app.json.

Pull request type

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

Reviewers

@donatieng @pan- @paul-szczepanek-arm

Release Notes

Starting with mbed-os 5.13 and the introduction of Nordic SDK V15, Nordic SoftDevice Bluetooth stack is not supported.
Bluetooth remains supported with the help of ARM’s Cordio stack.

@paul-szczepanek-arm
Copy link
Member

left a comment

cool, in which case remove the mbed json overrides

@pan-

pan- approved these changes May 29, 2019

@ciarmcom ciarmcom requested review from donatieng, pan-, paul-szczepanek-arm and ARMmbed/mbed-os-maintainers May 29, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@LDong-Arm, thank you for your changes.
@donatieng @pan- @paul-szczepanek-arm @ARMmbed/mbed-os-maintainers please review.

@pan-

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Somehow the BLE feature is not defined for NRF52830 and NRF52840 in targets.json; can you look at that @LDong-Arm ?

@LDong-Arm

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Somehow the BLE feature is not defined for NRF52830 and NRF52840 in targets.json; can you look at that @LDong-Arm ?

I thought BLE feature was disabled by default to keep build size down, and only enabled by mbed_app.json as needed? Or should it be enable whenever available?

@LDong-Arm

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Somehow the BLE feature is not defined for NRF52830 and NRF52840 in targets.json; can you look at that @LDong-Arm ?

I thought BLE feature was disabled by default to keep build size down, and only enabled by mbed_app.json as needed? Or should it be enable whenever available?

It turns out that SOFTDEVICE_S140's SDK enables BLE, but the CORDIO stack is outside of the SDK so we need to enable BLE in targets.json as discussed with @pan-.

There will be no need to enable BLE again in mbed_app.json of individual applications.

@LDong-Arm LDong-Arm force-pushed the LDong-Arm:nrf52_cordio branch from eddf7c5 to b468802 May 30, 2019

@LDong-Arm

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

cool, in which case remove the mbed json overrides

Or shall we keep the overrides for compatibility with older mbed-os?

@donatieng
Copy link
Member

left a comment

Looks good - @ARMmbed/mbed-os-maintainers could this please target RC2?

@donatieng

This comment has been minimized.

Copy link
Member

commented May 30, 2019

cool, in which case remove the mbed json overrides

Or shall we keep the overrides for compatibility with older mbed-os?

Please keep them in the examples repo for now until this is on master :)

@pan-

pan- approved these changes May 30, 2019

@0xc0170

0xc0170 approved these changes Jun 3, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jun 3, 2019

@bulislaw
Copy link
Member

left a comment

🎉 Approved for RC2. Should that be documented somewhere? @pan- please add section to release notes in the PR body.

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 4, 2019

Test run: FAILED

Summary: 3 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Build failures related, one of the tests fails to compile:

        [Error] main.cpp@35,31: use of undeclared identifier '__NRF_NVIC_APP_IRQS_0'
        [Error] main.cpp@35,81: use of undeclared identifier '__NRF_NVIC_APP_IRQS_1'

Please review

Note, rc2 should be finalized today

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Jun 4, 2019

@LDong-Arm LDong-Arm force-pushed the LDong-Arm:nrf52_cordio branch from b468802 to ba84805 Jun 4, 2019

@LDong-Arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Build failures related, one of the tests fails to compile:
[Error] main.cpp@35,31: use of undeclared identifier '__NRF_NVIC_APP_IRQS_0'
[Error] main.cpp@35,81: use of undeclared identifier '__NRF_NVIC_APP_IRQS_1'

Please review
Note, rc2 should be finalized today

Thanks for catching that. I've updated the critical section test which failed to build, thanks to help from @pan-

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jun 4, 2019

LDong-Arm added some commits Jun 4, 2019

critical_section tests: do not check APP interrupts on NRF52
APP interrupts are masked on SoftDevice BLE stack only, but we
have switched to Cordio stack on NRF52 targets.
NORDIC_CORDIC pal_crypto: check if cryptocell310 is enabled
The config "cryptocell310-acceleration" is set by MCU_NRF52840
but individual targets may have crytocell310 feature disabled.

@LDong-Arm LDong-Arm force-pushed the LDong-Arm:nrf52_cordio branch from ba84805 to 3373c5d Jun 5, 2019

@LDong-Arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Build issues:
[Fatal Error] pal_crypto.c@27,10: 'crys_rsa_kg.h' file not found
./features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_CORDIO/TARGET_NRF5x/stack/sources/pal_crypto.c:27:10: fatal error: 'crys_rsa_kg.h' file not found

Please fix

Fixed now, let's hope everything works this time.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jun 5, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

CI restarted

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I'll restart shortly exporters - license issue with armc6

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 5, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
  • jenkins-ci/mbed-os-ci_greentea-test
@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Exporters should be OK, but test failed with related errors:

nrf52 ble tests, for ARM:

[1559730685.03][CONN][INF] found KV pair in stream: {{__testcase_name;Test multiple reset commands}}, queued...
[1559730685.03][CONN][INF] found KV pair in stream: {{__testcase_start;Test reset command}}, queued...
[1559730685.13][CONN][RXD] ++ MbedOS Error Info ++
[1559730685.13][CONN][RXD] Error Status: 0x80010133 Code: 307 Module: 1
[1559730685.23][CONN][RXD] Error Message: Mutex: 0x200078AC, Not allowed in ISR context

same test fails also for GCC/IAR but has test assert rather. Anyway, same test (features-feature_ble-targets-target_cordio-tests-cordio_hci-transport) 3 compilers , nrf52 target

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Moved to rc3, rc2 generated

@LDong-Arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Exporters should be OK, but test failed with related errors:
nrf52 ble tests, for ARM:
[1559730685.03][CONN][INF] found KV pair in stream: {{__testcase_name;Test multiple reset commands}}, queued...
[1559730685.03][CONN][INF] found KV pair in stream: {{__testcase_start;Test reset command}}, queued...
[1559730685.13][CONN][RXD] ++ MbedOS Error Info ++
[1559730685.13][CONN][RXD] Error Status: 0x80010133 Code: 307 Module: 1
[1559730685.23][CONN][RXD] Error Message: Mutex: 0x200078AC, Not allowed in ISR context

same test fails also for GCC/IAR but has test assert rather. Anyway, same test (features-feature_ble-targets-target_cordio-tests-cordio_hci-transport) 3 compilers , nrf52 target

I can locally reproduce this issue. Looking into it.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@LDong-Arm Any update, just any timebox for this - we would like to timebox rc3

@paul-szczepanek-arm

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I've got a fix for that one, working on it. Give me an a few hours. Doesn't mean there won't be more

paul-szczepanek-arm and others added some commits Jun 7, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jun 7, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jun 7, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 5
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jun 9, 2019

@0xc0170 0xc0170 merged commit dc77c40 into ARMmbed:master Jun 9, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8621 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details

@0xc0170 0xc0170 removed the ready for merge label Jun 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.