Skip to content

ble/softdevice: add ble_nordic_softdevice feature #11769

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2019
Merged

ble/softdevice: add ble_nordic_softdevice feature #11769

merged 1 commit into from
Jul 4, 2019

Conversation

haukepetersen
Copy link
Contributor

Contribution description

This PR is a minor step on the path to make NimBLE the default solution for IP-over-BLE in RIOT, e.g. making it the default when building the gnrc_networking example.

But currently the softdevice is broken and there was some confusion about testing it also for the nrf52840, for which it was never verified to be working. This PR should clarify this situation once and for all :-)

Note about the approach: apparently the CPU_MODEL variable is not set when parsing the Makefile.features. At least I have not seen a more generic way in setting this feature explicitly for all nrf52832-based boards. Anything nicer is welcome :-)

Testing procedure

Build the gnrc networking example for the e.g. nrf52dk (-> should be all good), and for the e.g. nrf52840dk (-> should complain about the missing feature).

Issues/PRs references

partly related to #11559 and subsequently #11091

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: BLE Area: Bluetooth Low Energy support labels Jul 1, 2019
@haukepetersen haukepetersen mentioned this pull request Jul 1, 2019
Merged
@aabadie
Copy link
Contributor

aabadie commented Jul 3, 2019

At least I have not seen a more generic way in setting this feature explicitly for all nrf52832-based boards

This may also work: introduce a Makefile.nrf52832.features file in boards/common/nrf52 and include it in each nrf52832 boards features. That won't decrease the number of lines but provides a way to share nrf52832 specific features, so should a little more generic. I won't push toward this though and I think the current state of this PR is ok. Just needs testing.

@haukepetersen
Copy link
Contributor Author

Yes, I thought about that option too, but found it a little overkill at first. But thinking about it now, its probably the cleanest solution after all -> will adapt.

@haukepetersen
ble/softdevice: add ble_nordic_softdevice feature
03153ac 
The softdevice is only verified to be working on nrf52832-based
boards. This feature prevents the softdevice from being build for
other, similar targets (e.g. nrf52840-based boards).
@haukepetersen haukepetersen force-pushed the opt_softdevice_feature branch from 9fd194f to 03153ac Compare July 4, 2019 07:23
@haukepetersen
Copy link
Contributor Author

pushed the fix. Due to further complications through the nrf52xxxdk-common path I added the Makefile.nrf52832.features as additional include to the boards in question.

aabadie
aabadie approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes are ok for me.
Also tested on nrf52dk and nrf52840dk and no problem found. The testing procedure described in this PR is not totally valid since softdevice is not pulled in for nrf52840 based board => no feature missing warning (these boards now use the nrf802154 driver directly).

Let's merge this.

ACK

@aabadie aabadie merged commit 0791174 into RIOT-OS:master Jul 4, 2019
@haukepetersen haukepetersen deleted the opt_softdevice_feature branch July 4, 2019 10:04
@aabadie aabadie mentioned this pull request Jul 4, 2019
Merged
@cladmi
Copy link
Contributor

cladmi commented Jul 4, 2019 •
edited
Loading

Note about the approach: apparently the CPU_MODEL variable is not set when parsing the Makefile.features. At least I have not seen a more generic way in setting this feature explicitly for all nrf52832-based boards. Anything nicer is welcome :-)

I found out about this PR while rebasing #11477. Makefile.features it is now the correct place to put CPU and CPU_MODEL as "Makefile.features: prerequisites for moving CPU/CPU_MODEL to boards/Makefile.features #11478" was merged (3 days ago)

The migration was not started due to being close to the release, and it will be announced on the mailing list too.

As it appears to be CPU_MODEL specific, when migrating this, I think I will after movingCPU and CPU_MODEL to board/.../Makefile.features, change to do a -include cpu/$(CPU)/Makefile.$(CPU_MODEL).features in the cpu/nrf52/Makefile.features.

@cladmi cladmi mentioned this pull request Jul 4, 2019
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees
No one assigned
Labels
Area: BLE Area: Bluetooth Low Energy support CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haukepetersen @aabadie @cladmi