Skip to content

BLE: replace obsolete services with the new services repo #14436

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 13, 2021
Merged

BLE: replace obsolete services with the new services repo #14436

merged 3 commits into from
Apr 13, 2021

Conversation

paul-szczepanek-arm
Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm commented Mar 17, 2021 •
edited
Loading

Summary of changes

We are removing the old and broken ble services in favour of the external services repository.

Impact of changes

Migration actions required

Documentation

none

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

@pan-

@paul-szczepanek-arm paul-szczepanek-arm changed the title replace obsolete services with the new services repo BLE: replace obsolete services with the new services repo Mar 17, 2021
@ciarmcom ciarmcom requested review from pan- and a team March 17, 2021 09:30
@ciarmcom
Copy link
Member

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

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 17, 2021 •
edited
Loading

Shouldn't this be first deprecated and then removed? We could have done for the current release that is almost completed. I wonder if this is rather breaking change when removing even broken functionality.

The two commits could be squashed and that would explain the removal ?

@paul-szczepanek-arm
Copy link
Member Author

The 2 useful services already have been reimplemented and the rest are of rather dubious usefulness and quality. Can a whole class be deprecated? I'm open to suggestions on how to handle this removal - squashed the commits.

@pan-
Copy link
Member

pan- commented Mar 18, 2021

I think deprecation would be more appropriate to avoid breaking existing applications. But at the same time, these services are broken from a specification standpoint and it isn't even desirable to fix them as the API will have to be changed.

The experimental service repo should be in a better state too (everything merged into master, repo renamed, ...) before we merge this.

@donatieng What's your position on this ?

@donatieng
Copy link
Contributor

Yeah, agreed - technically these service need to be deprecated even if they're not in a great state. It would be good to point users at the experimental services repo though!

@paul-szczepanek-arm
Copy link
Member Author

Yeah, agreed - technically these service need to be deprecated even if they're not in a great state. It would be good to point users at the experimental services repo though!

I am doing that -> see readme.

paul-szczepanek-arm
paul-szczepanek-arm commented Mar 25, 2021
View reviewed changes
connectivity/FEATURE_BLE/include/ble/services/services.md
@@ -0,0 +1,13 @@
# Mbed OS BLE services

BLE services are available in the https://github.com/ARMmbed/mbed-os-experimental-ble-services repository. It's a
Copy link
Member Author

Choose a reason for hiding this comment

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

@donatieng

pan-
pan- previously approved these changes Apr 7, 2021
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels Apr 7, 2021
0xc0170
0xc0170 reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One way of doing this, would be to keep the files (move them to deprecated or just rename services to services_deprecated or just keep services and add a warning to a files inside the folder, remove implementation) and print a warning if included by an app to point them to external repo? I dont see common point for users to get the warning, so it would need to be in each file removed here? :/ It still though might be better than just remove (a user would receive an error "not found") ? Either that or just as it's here.

We used it before, in mbed-os/features/deprecated_warnings.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 7, 2021

As they are not part of any API and not include anywhere, this should be good as it is.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 7, 2021

CI started

@paul-szczepanek-arm
Copy link
Member Author

I'll add a deprecation config to ease the transition. And we can dump them wholly in next major release.

@mergify mergify bot added needs: work and removed needs: CI labels Apr 7, 2021
@mbed-ci
Copy link

mbed-ci commented Apr 7, 2021

Jenkins CI Test : ❌ FAILED

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-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ❌
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ❌
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ❌
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ❌

@mergify mergify bot dismissed pan-’s stale review April 7, 2021 16:17

Pull request has been modified.

@paul-szczepanek-arm
Copy link
Member Author

OK, let's do this in two steps. First we deprecate and then next major release we dump them. Since these are not actually included anywhere no config option is needed.

pan-
pan- previously approved these changes Apr 8, 2021
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: work labels Apr 8, 2021
0xc0170
0xc0170 reviewed Apr 8, 2021
View reviewed changes
connectivity/FEATURE_BLE/include/ble/services/BatteryService.h
@@ -16,6 +16,8 @@
* limitations under the License.
*/

#warning "These services are deprecated and will be removed. Please see services.md for details about replacement services."
Copy link
Contributor

Choose a reason for hiding this comment

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

lets use here MBED_DEPRECATED, so when we are about to remove them we use search and remove

@mergify mergify bot dismissed pan-’s stale review April 8, 2021 08:55

Pull request has been modified.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 13, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 13, 2021

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_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

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

@0xc0170 0xc0170 0xc0170 approved these changes

@pan- pan- pan- left review comments

Assignees
No one assigned
Labels
component: connectivity release-version: 6.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@paul-szczepanek-arm @ciarmcom @0xc0170 @pan- @donatieng @mbed-ci @mbedmain