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

Nordic BLE: Allow configuration of softdevice parameters #6860

Merged
merged 3 commits into from Jun 7, 2018

Conversation

Projects
None yet
@andrewleech
Contributor

andrewleech commented May 10, 2018

Description

Allow configuration (via defines) of some of the key settings for the nordic softdevice.

  • CENTRAL_LINK_COUNT
  • PERIPHERAL_LINK_COUNT
  • gatts_enable_params.attr_tab_size
  • common_enable_params.vs_uuid_count
  • gatts_enable_params.service_changed

These settings control the range of functionality enabled in the softdevice as well as ram consumption.
In particular, reducing these values is critical to enable usage of 16K nrf51 devices.

The defaults have not been changed so this should not effect existing users.

The values can be adjusted on a per-project basis by target_overrides in mbed_app.json, eg:

{
    "target_overrides": {
        "*": {
            "nordic-ble.central_link_count": 0,
            "nordic-ble.peripheral_link_count": 1,
            "nordic-ble.gatt_attr_tab_size": "0x4A0",
            "nordic-ble.uuid_table_max_entries": 2,
            "nordic-ble.is_srvc_changed_charact_present": 1
        },
}

Note: There are a couple of minor edits to the logging lines in mbed-os/targets/TARGET_NORDIC/TARGET_NRF5/TARGET_SDK11/softdevice/common/softdevice_handler/softdevice_handler.c

These edits were not necessary in any later versions of the SDK.

Status

READY

Migrations

There should be no required changes to end user code.

Related PRs

This is a cleaned up re-submission of #5980

Steps to test or reproduce

Changing the target config values above in defines/config should result in a change of ram requirements.

When adjusting settings it is useful to enable target.nrf_enable_logging as shown above. You will then get a printout on the mbed stdout serial port during softdevice configuration as such:

sd_ble_enable: RAM START at 0x20002ef8
sd_ble_enable: RAM start should be adjusted to 0x20001f18
RAM size should be adjusted to 0x60e8

If the START value is lower than the RAM ORIGIN in the current linker file, the linker file can be copied into your project and edited to set the RAM ORIGIN and LENGTH to the printed values.
The mbed compile command can then have the linker arg set to your local linker file.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change

@andrewleech andrewleech force-pushed the andrewleech:nordic_ble_config branch from ade8c38 to 6aeda2a May 10, 2018

@andrewleech andrewleech changed the title from Nordic ble config to Nordic BLE: Allow configuration of softdevice parameters May 11, 2018

@cmonr cmonr requested a review from marcuschangarm May 11, 2018

@cmonr cmonr added the needs: review label May 11, 2018

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented May 11, 2018

The logging in this PR would do well from #5965 once it's finished

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 11, 2018

Thank you for the contribution. I think there are too many things going on at the same time in this PR:

  1. Use config system to modify BLE parameters.
  2. Nordic SDK logging.

Do you mind splitting it up in two PRs please?

Regarding 1, you should add a new mbed_lib.json file to features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5x and give it a name like nordic-ble instead of modifying target.json.

Regarding 2, just by skimming the code I'm not sure it will play nice with the current UART implementation? At the end of the day, @donatieng would have to sign off on adding this new feature.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented May 11, 2018

Yeah... Once I dropped the linker settings from the previous pr it needed some way to show the user what they needed to do, it just grew from there.

The logging stuff works really well in my testing on both nrf51 and nrf51. It uses the UART defines from the original nrf logging but it doesn't touch the actual uarts, redirecting all logging defines to stdout.

@andrewleech andrewleech force-pushed the andrewleech:nordic_ble_config branch from 6aeda2a to a8d4fc5 May 14, 2018

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented May 14, 2018

Thanks for the suggestion about adding a new mbed_lib.json file,, I've been able to update the ones from SDK14 to suit and move the configuration to a single library def file. Much cleaner than all the changes to targets.json!

I've also separated out the logging stuff to its own PR.

This has all made the changeset dramatically smaller.

@andrewleech andrewleech force-pushed the andrewleech:nordic_ble_config branch 2 times, most recently from 35786ff to a0e0a41 May 14, 2018

@0xc0170 0xc0170 requested a review from donatieng May 14, 2018

"macro_name": "NRF_SDH_BLE_SERVICE_CHANGED"
}
}
}

This comment has been minimized.

@marcuschangarm

marcuschangarm May 16, 2018

Contributor

This file should probably live here: https://github.com/ARMmbed/mbed-os/tree/master/features/FEATURE_BLE/targets/TARGET_NORDIC

Since it is BLE specific.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 16, 2018

I love it! Do you mind making the same changes to the NRF52 folder please? https://github.com/ARMmbed/mbed-os/tree/master/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF52

Unfortunately, we haven't consolidated the two implementations yet.

@cmonr cmonr added needs: work and removed needs: review labels May 16, 2018

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented May 16, 2018

I checked the nrf52 ble codebase and I'm pretty sure it doesn't need any changes, these config settings are all for existing defines in the softdevice code (notice I removed the hardcoded copies from nrf52 softdevice lib files)

That's why I put the lib for where I did, because the settings are documented by nordic as softdevice settings and moving it would now mean duplicating the file to both ble folders.

Yeah I was amazed that the settings were already nicely exposed for nfr52, after I'd put so much round and round effort into making this work for nrf51/sdk11.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 17, 2018

That's why I put the lib for where I did, because the settings are documented by nordic as softdevice settings and moving it would now mean duplicating the file to both ble folders.

But the single mbed_lib.json could live in the Nordic top-level folder and be shared by both NRF51 and NRF52: https://github.com/ARMmbed/mbed-os/tree/master/features/FEATURE_BLE/targets/TARGET_NORDIC

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented May 17, 2018

Ah, sure, yeah that does make sense. I'll change it over when I get in to work.

@pingdan32

This comment has been minimized.

Contributor

pingdan32 commented May 18, 2018

It looks good, I think we can add some more configurations, such as "NRF_SDH_BLE_GATT_MAX_MTU_SIZE=23".
"gatt_max_mtu": { "help": "max mtu value", "macro_name": "NRF_SDH_BLE_GATT_MAX_MTU_SIZE", "value": 23 },

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 21, 2018

@marcuschangarm Are you happy with this now?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 21, 2018

Are you happy with this now?

Yep! Ship it! 🚢

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 21, 2018

Once rebased, we'll start CI.

@andrewleech andrewleech force-pushed the andrewleech:nordic_ble_config branch from bac465e to 4b1cb4e May 22, 2018

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented May 22, 2018

Ok, rebased

@0xc0170 0xc0170 requested review from ARMmbed/mbed-os-pan and removed request for donatieng May 22, 2018

@0xc0170 0xc0170 added the needs: CI label May 22, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 24, 2018

@andrewleech @pan- Good to go!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 29, 2018

@andrewleech @pan- Good to go!

Needs rebase, @andrewleech Please do

pi-anl added some commits May 14, 2018

Allow configuration (via defines) of some of the key settings for the…
… NRF51 softdevice.

* CENTRAL_LINK_COUNT
* PERIPHERAL_LINK_COUNT
* gatts_enable_params.attr_tab_size
* gatts_enable_params.service_changed
* common_enable_params.vs_uuid_count

These settings control the range of functionality enabled in the softdevice as well as ram consumption.
In particular reducing these values is critical to enable usage of 16K nrf51 devices.
Move mbed_lib.json from targets folder to feature_ble folder
The functionality added all affects BLE features in use so this location is a better fit.

@andrewleech andrewleech force-pushed the andrewleech:nordic_ble_config branch from 4b1cb4e to bf313aa Jun 1, 2018

@cmonr cmonr added the needs: review label Jun 4, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 6, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 6, 2018

Build : SUCCESS

Build number : 2250
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6860/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 7, 2018

/morph mbed2-build

@cmonr cmonr merged commit bacf6a9 into ARMmbed:master Jun 7, 2018

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 919 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9567 cycles (-33 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment