Skip to content

BLE: remove deprecated APIs from Gatt and SecurityManager #12742

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 3 commits into from
Apr 16, 2020
Merged

BLE: remove deprecated APIs from Gatt and SecurityManager #12742

merged 3 commits into from
Apr 16, 2020

Conversation

LDong-Arm
Copy link
Contributor

@LDong-Arm LDong-Arm commented Apr 1, 2020 •
edited
Loading

Preceding PR: #12676

Summary of changes

  • Remove deprecated APIs from Gatt
  • Remove deprecated APIs from SecurityManager

Impact of changes

Deprecated APIs from Gatt and SecurityManager do not exist anymore, applications that use them will not compile until updated with current APIs.

Migration actions required

Applications that compile without deprecation warnings on mbed-os-5.15 will continue to work. Otherwise, please fix any deprecated API usages, for example by referring to compilation warnings from mbed-os-5.15.

Documentation

None.

Pull request type 

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

Test results 

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

Manual testing: BLE_SM, BLE_GattClient and BLE_GattServer examples compile and function as expected on NRF52840_DK.

Reviewers

@ARMmbed/mbed-os-pan @evedon

@ciarmcom ciarmcom requested review from evedon and a team April 1, 2020 15:00
@ciarmcom
Copy link
Member

ciarmcom commented Apr 1, 2020

@LDong-Arm, thank you for your changes.
@evedon @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-maintainers please review.

@LDong-Arm LDong-Arm force-pushed the gatt_sm_deprecated_cleanup branch from 55292fe to bee6819 Compare April 3, 2020 12:30
@LDong-Arm
Copy link
Contributor Author

rebased

@0xc0170 0xc0170 requested a review from bulislaw April 3, 2020 12:55
0xc0170
0xc0170 previously approved these changes Apr 3, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels Apr 3, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 5, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 6, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added needs: work and removed needs: CI labels Apr 6, 2020
pan-
pan- requested changes Apr 6, 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.

Thanks @LDong-Arm That looks good however removing the default event handler makes the code more fragile if an event handler hasn't been set.

In GenericSecurityManager.tpp, could you guard access to a null event handler ?

@LDong-Arm
Copy link
Contributor Author

@pan- Thanks, that's a good point. Since nothing in EventHandler is pure virtual, maybe we can use a default handler (as before) that overrides nothing. I'll make the change.

@LDong-Arm LDong-Arm force-pushed the gatt_sm_deprecated_cleanup branch from bee6819 to 70759b2 Compare April 7, 2020 09:25
@mergify mergify bot dismissed 0xc0170’s stale review April 7, 2020 09:26

Pull request has been modified.

@LDong-Arm LDong-Arm force-pushed the gatt_sm_deprecated_cleanup branch from 70759b2 to 07c25bb Compare April 7, 2020 09:27
@LDong-Arm
Copy link
Contributor Author

LDong-Arm commented Apr 7, 2020 •
edited
Loading

@pan- I've brought back defaultEventHandler which overrides nothing instead of providing legacy support.

@LDong-Arm LDong-Arm requested a review from pan- April 7, 2020 09:32
@mbed-ci
Copy link

mbed-ci commented Apr 7, 2020

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM-lts
  • jenkins-ci/mbed-os-ci_build-GCC_ARM-lts

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 7, 2020

Please ignore lts jobs here , not valid for this PR. We will restart testing and fix lts status here later once 5.15 jobs are in

pan-
pan- approved these changes Apr 8, 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 Apr 9, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 9, 2020

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@LDong-Arm
Copy link
Contributor Author

@0xc0170 The failures are specific to a few targets and unrelated to BLE?

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 14, 2020

The CI was fixed on Friday, CI restarted

@LDong-Arm
Copy link
Contributor Author

@0xc0170 Thanks.
@pan- This PR is relatively trivial and doesn't touch any new APIs, hopefully we can get it in before I make an update to ble-cliapp to test Gap.

@mbed-ci
Copy link

mbed-ci commented Apr 14, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 merged commit 7b0c38a into ARMmbed:master Apr 16, 2020
@mergify mergify bot removed the ready for merge label Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bulislaw bulislaw bulislaw approved these changes

@pan- pan- pan- approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@evedon evedon Awaiting requested review from evedon

Assignees
No one assigned
Labels
BREAKING-CHANGE release-version: 6.0.0-beta-1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@LDong-Arm @ciarmcom @0xc0170 @mbed-ci @bulislaw @pan-