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

BLE: updates for v1.3.1-rc.1 #1847

Merged
merged 17 commits into from Aug 8, 2019

Conversation

@XuGuohui
Copy link
Contributor

commented Jul 9, 2019

Features:

  • API for selecting BLE antenna for BLE radio [CH35282]
  • API for setting/getting BLE device name
  • API for setting BLE device address [CH35667]
  • API for manually discovering peer device's BLE services and characteristics
  • API for fetching discovered peer's services and characteristics
  • API in BlePeerDevice for establishing BLE connection without scanning required.
  • API for manually subscribing/unsubscribing peer characteristic's notification
  • API for disconnecting all on-going BLE connections.

Improvements:

  • Refactors BLE event dispatching [CH35123]
    • Dedicated callback for whom creating a local characteristic to accept data written and CCCD changed events.
    • Dedicated callback for whom starting scanning procedure to accept scan result.
    • Dedicated callback for whom starting the service discovery procedure to accept service discovery events.
    • Dedicated callback for whom starting the characteristic discovery procedure to accept char discovery events.
    • Peripheral link callbacks for those have subscribed the peripheral link events
    • Dedicated link callback for whom creating a central link to accept central link events.
    • Dedicated callback for whom enabling a peer characteristic CCCD to accept data notified event.
  • Decreases BLE runtime RAM consumption [CH35124]
    Xenon with threading enabled:
    +---------------+---------------+----------------------+-------------+
    | Version       | Central Links | User Characteristics | Free Memory |
    +---------------+---------------+----------------------+-------------+
    | 1.2.1-rc.2    | N/A           | N/A                  | 95888       |
    +---------------+---------------+----------------------+-------------+
    | 1.3.0-alpha.1 | 5             | 7                    | 65000       |
    +---------------+---------------+----------------------+-------------+
    | 1.3.0-rc.1    | 1             | 7                    | 86960       |
    +---------------+---------------+----------------------+-------------+
    | Current       | 3             | 20                   | 88480       |
    +---------------+---------------+----------------------+-------------+
    
  • Supports up to 23 local characteristics, 20 of them are available for user application
  • Supports up to 3 central links
  • More convenient methods provided in BleUuid class.

Bugfixes:

Known Issues:

  • Effective BLE scanning timeout always aligns with seconds
  • BLE.off() will terminate the on-going mobile setup process, but the mobile setup next time it entering the Listening mode is still functional after this API being called.
  • Mobile setup process will not work if the BLE device name is changed in user application. Use BLE.setDeviceName(nullptr) to restore the default device name before entering the Listening mode to make sure the mobile setup works as expected.
  • BLE applications compiled against 1.3.0-rc.1 will not work on the device that is running 1.3.1-rc.1 or above.
  • BLE doesn't work properly when device in sleep mode.
@XuGuohui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Closes #1855

@XuGuohui XuGuohui force-pushed the fix/ble/v1.3.1-rc.1 branch 2 times, most recently from 17d8261 to 453f873 Jul 16, 2019

@XuGuohui

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Closes #1859

@XuGuohui

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Closes #1874

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@XuGuohui After you change the maximum number of central connections, could you please update this PR with info on runtime RAM usage compared to latest release that doesn't support BLE?

  1. BLE unused in the user application
  2. BLE is used in the user application
hal/inc/ble_hal.h Show resolved Hide resolved
hal/inc/ble_hal.h Outdated Show resolved Hide resolved
hal/inc/ble_hal.h Outdated Show resolved Hide resolved
wiring/inc/spark_wiring_ble.h Show resolved Hide resolved
wiring/inc/spark_wiring_ble.h Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
hal/inc/ble_hal.h Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
wiring/src/spark_wiring_ble.cpp Show resolved Hide resolved
wiring/src/spark_wiring_ble.cpp Show resolved Hide resolved
wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved
wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved
wiring/src/spark_wiring_ble.cpp Show resolved Hide resolved
wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved
wiring/src/spark_wiring_ble.cpp Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
hal/src/nRF52840/ble_hal.cpp Outdated Show resolved Hide resolved
@XuGuohui

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@XuGuohui After you change the maximum number of central connections, could you please update this PR with info on runtime RAM usage compared to latest release that doesn't support BLE?

  1. BLE unused in the user application
  2. BLE is used in the user application

I can compare the runtime RAM consumption between 1.3.0-rc.1 and this branch, but not the case that when BLE is used in user application or not, because the BLE is enabled anyway to make sure the BLE control request functional, so the memory pool in BLE HAL will be always initialized after startup.

@XuGuohui XuGuohui requested a review from avtolstoy Aug 7, 2019

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@XuGuohui Please rebase on develop.

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

I can compare the runtime RAM consumption between 1.3.0-rc.1 and this branch, but not the case that when BLE is used in user application or not, because the BLE is enabled anyway to make sure the BLE control request functional, so the memory pool in BLE HAL will be always initialized after startup.

I still feel that we can optimize things here and stop the dispatcher thread and deallocate the pool unless absolutely necessary (i.e. user application uses BLE or device is in listening mode). That way we'll be losing only the RAM reserved for SoftDevice in 99% cases.

@XuGuohui

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

I can compare the runtime RAM consumption between 1.3.0-rc.1 and this branch, but not the case that when BLE is used in user application or not, because the BLE is enabled anyway to make sure the BLE control request functional, so the memory pool in BLE HAL will be always initialized after startup.

I still feel that we can optimize things here and stop the dispatcher thread and deallocate the pool unless absolutely necessary (i.e. user application uses BLE or device is in listening mode). That way we'll be losing only the RAM reserved for SoftDevice in 99% cases.

It can be. But we should also evaluate the side-effects if we going forwad. Besides, AFAIK, the atomic pool librariy doesn't have an API to deallocate a pool. So I'd like to keep this in mind and investigate that in a separated PR in the future.

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

So I'd like to keep this in mind and investigate that in a separated PR in the future.

👍 Not saying we should do now, but something to consider.

the atomic pool librariy doesn't have an API to deallocate a pool

It can simply be destroyed.

@XuGuohui XuGuohui force-pushed the fix/ble/v1.3.1-rc.1 branch from 0b6c216 to bd58bc6 Aug 8, 2019

@XuGuohui XuGuohui requested a review from avtolstoy Aug 8, 2019

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@XuGuohui Could you also please make sure that the changelog is actual in the description of the PR.

@XuGuohui

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@XuGuohui Could you also please make sure that the changelog is actual in the description of the PR.

Yes, it is.

@XuGuohui XuGuohui merged commit 9128aea into develop Aug 8, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@XuGuohui XuGuohui deleted the fix/ble/v1.3.1-rc.1 branch Aug 8, 2019

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