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 conf #9790

Merged
merged 100 commits into from Mar 1, 2019

Conversation

Projects
None yet
8 participants
@paul-szczepanek-arm
Copy link
Member

commented Feb 21, 2019

Description

Allow users to configure the BLE feature to disable functionality not needed by the device to save memory.

Pull request type

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

Reviewers

@pan-

Release Notes

No changes made to existing programs. By default all options are enabled which is the current state.

@paul-szczepanek-arm

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

Please add needs work.
This PR relies on a previous PR which is not yet made. This PR for now is for review and visibility.

@paul-szczepanek-arm paul-szczepanek-arm referenced this pull request Feb 21, 2019

Closed

Ble conf #61

@0xc0170 0xc0170 added the needs: work label Feb 21, 2019

@ciarmcom ciarmcom requested review from pan- and ARMmbed/mbed-os-maintainers Feb 21, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

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

@cmonr cmonr removed the needs: work label Feb 21, 2019

@pan-

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@0xc0170 We need the 5.12 tag please.

pan- added some commits Feb 25, 2019

BLE - Devirtualize pal::ConnectionEventMonitor
The event handler has been extracted out of the monitor declaration.
BLE - Devirtualize pal::Gap
The event handler has been taken out of Gap declaration and the instantiation must provide an implementation and the type that plays the event handler role.
BLE - Devirtualize pal::GattClient
The event handler has been taken out of GattClient declaration and an instantiation requires the actual implementation and the type that handle events.
BLE - Devirtualize pal::SecurityManager
The event handler has been extracted out of SecurityManager declaration and instantion of the interface requires the implementation and event handler type.
BLE - Devirtualize pal::SigningEventMonitor
The event handler has been extracted out of SigningEventMonitor declaration and SigningEventMonitor instantion requires the implementation and event handler type.
BLE - Devirtualization of ::Gap
The interface definition now lives in ::ble::interface::LegacyGap. Implementation must export the implementation type as ble::impl::LegacyGap.
BLE - Devirtualize ::ble::Gap
The interface definition now lives in ::ble::interface::Gap.
The implementation must export the implementation types as ::ble::impl::Gap.
BLE - Devirtualize ::GattClient
Interface definition now lives in ble::interface::GattClient. An implementation must export the implementation type in ::ble::impl::GattClient.
BLE - Devirtualize ::GattServer.
The interface is defined in ::ble::interface::GattServer and an implementation must export the implementation type ::ble::impl::GattServer.
BLE - Devirtualize ::SecurityManager
The interface now lives in ::ble::interface::SecurityManager. The implementation type is expectected to exported as ble ::ble::impl::SecurityManager by the implementation.
@@ -14,11 +14,6 @@
* limitations under the License.
*/

#include "BLERoles.h"

#if BLE_FEATURE_GATT_SERVER

This comment has been minimized.

Copy link
@paul-szczepanek-arm

paul-szczepanek-arm Feb 28, 2019

Author Member

does in fact need gattserver

@@ -14,11 +14,6 @@
* limitations under the License.
*/

#include "BLERoles.h"

#if BLE_FEATURE_GATT_SERVER

This comment has been minimized.

Copy link
@paul-szczepanek-arm

paul-szczepanek-arm Feb 28, 2019

Author Member

reinstate pls

@pan-

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@paul-szczepanek-arm I've addressed your comments and benchmarked our examples:

Example TEXT BSS DATA
BLE_BatteryLevel 32947 (-66510) 162 (-29) 4817 (-1343)
BLE_Beacon 16240 (-82745) 66 (-125) 3613 (-2547)
BLE_Button 32943 (-66510) 162 (-29) 4817 (-1343)
BLE_GAP 36162 (-63960) 67 (-124) 3958 (-2202)
BLE_GapButton 22680 (-77003) 66 (-125) 3889 (-2271)
BLE_GattClient 34278 (-65255) 78 (-113) 4353 (-1807)
BLE_GattServer 32567 (-66532) 162 (-29) 4817 (-1343)
BLE_Heartrate 32975 (-66512) 162(-29) 4817(-1343)
BLE_LED 32701 (-66752) 162 (-29) 4817 (-1343)
BLE_LEDBlinker 33905 (-64905) 78 (-113) 4360 (-1800)
BLE_PeriodicAdvertising 45953 (-53851) 163 (-28) 4886 (-1274)
BLE_Privacy 52012 (-47160) 94 (-97) 4717 (-1448)
BLE_SM 49652 (-49560) 94 (-97) 4702 (-6160)
BLE_Thermometer 33005 (-66518) 162 (-29) 4817 (-1343)

Note: Numbers provided are just for the BLE API

@@ -25,6 +25,8 @@

#endif

#if BLE_FEATURE_GATT_SERVER

This comment has been minimized.

Copy link
@paul-szczepanek-arm

paul-szczepanek-arm Feb 28, 2019

Author Member

still fails

Suggested change
#if BLE_ROLE_BROADCASTER
@pan-

pan- approved these changes Feb 28, 2019

@donatieng

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

@donatieng Pending a successful merge dryrun test, #9727 will be getting merged. Wanted to wait before that was in.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

ETA ~20 mins

@donatieng

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Sounds good, thanks @cmonr

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Well then. Really glad I did that. A couple things appear to have broken. Checking it out.

Will update when able.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

#9790 (comment)

@pan- Thanks for the diff.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 1, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@@ -0,0 +1,866 @@
/* mbed Microcontroller Library

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 1, 2019

Contributor

Ooohhh. This was deleted, renamed, then added.

I was wondering why it looked like the GH interface was being weird.

git mv <file>{cpp,tpp} appears to not have been done.

@cmonr

cmonr approved these changes Mar 1, 2019

Copy link
Contributor

left a comment

This is a really nice refactor!

@cmonr cmonr merged commit f8d254f into ARMmbed:master Mar 1, 2019

28 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-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 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-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10999 cycles (+1799 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the needs: CI label Mar 1, 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.