Skip to content

Fix enabling/disabling BLE-Features #13545

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
Sep 21, 2020
Merged

Fix enabling/disabling BLE-Features #13545

merged 1 commit into from
Sep 21, 2020

Conversation

DBS06
Copy link
Contributor

@DBS06 DBS06 commented Sep 3, 2020

Summary of changes

Fixes #13541

The current implementation does not allows to disable BLE-Features to reduce the CORDIO-Stack-Size.

Impact of changes

BLE-Features can now be disabled

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 3, 2020
@ciarmcom ciarmcom requested a review from a team September 3, 2020 14:30
@ciarmcom
Copy link
Member

ciarmcom commented Sep 3, 2020

@DBS06, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@pan-
Copy link
Member

pan- commented Sep 4, 2020

@paul-szczepanek-arm Can you review ?

@paul-szczepanek-arm
Copy link
Member

The diff is really hard to read. I think you reordered the functions. Is that needed?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 4, 2020

The diff is really hard to read. I think you reordered the functions. Is that needed?

I also reviewed but because of some many changes, it was not clear if there are additions or just refactoring (moving functions in within the same file) and applying ifdef afterwards. Can you leave the functions/methods at the same place just wrapping them around with ifdef , even if you do it multiple times. The cleanup/merge all together could be then a separate commit rather than all in one.

@DBS06
Copy link
Contributor Author

DBS06 commented Sep 7, 2020

The diff is really hard to read. I think you reordered the functions. Is that needed?

I also reviewed but because of some many changes, it was not clear if there are additions or just refactoring (moving functions in within the same file) and applying ifdef afterwards. Can you leave the functions/methods at the same place just wrapping them around with ifdef , even if you do it multiple times. The cleanup/merge all together could be then a separate commit rather than all in one.

I already thought, that this would be an issue. I could change that of course.
My intention was, that the methods in the cpp-file have the same order as in the header-file.

0xc0170
0xc0170 reviewed Sep 7, 2020
View reviewed changes
connectivity/FEATURE_BLE/libraries/ble-api-implementation/source/GapImpl.cpp Outdated


#if BLE_ROLE_BROADCASTER
#if BLE_FEATURE_PERIODIC_ADVERTISING
Copy link
Contributor

Choose a reason for hiding this comment

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

would all these make sense to be #if ... && .... rather than these if if ?

Copy link
Contributor Author

@DBS06 DBS06 Sep 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

my changes mirror how it is done in the header, if we change it here to #if ... && .... it should also be changed in the header.
BTW: for me it looks really awful how the code in the header and cpp is organized.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, all fine then from my side

Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what would you improve in the headers and cpp files ?

0xc0170
0xc0170 previously approved these changes Sep 7, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels Sep 7, 2020
@0xc0170 0xc0170 requested a review from a team September 8, 2020 07:20
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 8, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 8, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@pan-
Copy link
Member

pan- commented Sep 8, 2020 •
edited
Loading

@0xc0170 Hold your fire, I would like #13475 to be merged first.
@DBS06 Can you squash your commits ?

@DBS06
Copy link
Contributor Author

DBS06 commented Sep 8, 2020 •
edited
Loading

@DBS06 Can you squash your commits ?
I Can do that

EDIT: Done!

@DBS06 DBS06 force-pushed the fix_enable_disable_ble_features branch from f7397f3 to 59996f2 Compare September 8, 2020 08:47
@mergify mergify bot dismissed 0xc0170’s stale review September 8, 2020 08:47

Pull request has been modified.

@mergify
Copy link

mergify bot commented Sep 9, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@mergify mergify bot added the needs: work label Sep 9, 2020
@paul-szczepanek-arm
Copy link
Member

paul-szczepanek-arm commented Sep 10, 2020 •
edited
Loading

@DBS06 Your changes have been incorporated into the refactor PR but it's not visible in the diff anymore due to conflict since the refactor moved the implementation back to a separate class.

actually, my bad, it was not incorporated

@DBS06 DBS06 force-pushed the fix_enable_disable_ble_features branch from 59996f2 to c0021e2 Compare September 17, 2020 09:10
pan-
pan- approved these changes Sep 17, 2020
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 17, 2020

Ci started

@mbed-ci
Copy link

mbed-ci commented Sep 17, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@0xc0170 0xc0170 merged commit bc7e4d6 into ARMmbed:master Sep 21, 2020
@mergify mergify bot removed the ready for merge label Sep 21, 2020
@mbedmain mbedmain added release-version: 6.4.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pan- pan- pan- approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-version: 6.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BLE-Cordio-Stack: Unable to disable ble features
8 participants
@DBS06 @ciarmcom @pan- @paul-szczepanek-arm @0xc0170 @mbed-ci @mbedmain @steinerphilipp