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

Deprecate indications event onConfirmationReceived #14602

Merged
merged 1 commit into from
May 3, 2021

Conversation

paul-szczepanek-arm
Copy link
Member

Summary of changes

The onConfirmationReceived event is never used. Instead, both notifications and indications use only onDataSent event.

Notifications fire the event immediately upon sending the data and indications only fire the onDataSent event upon receiving the confirmation from the client that transfer was successful.

This PR correct the documentations and deprecates the event.

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


@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Apr 28, 2021
@ciarmcom ciarmcom requested a review from a team April 28, 2021 13:00
@ciarmcom
Copy link
Member

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

@mergify mergify bot added needs: CI and removed needs: review labels Apr 28, 2021
* part of a notification/indication.
* Function invoked when the server has sent data to a client. For
* notifications this is triggered when data is sent, for indications
* it's only triggered when the confirmation has been received.
Copy link

@noonfom noonfom Apr 28, 2021

Choose a reason for hiding this comment

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

So, Cordio executes the "application callback function with confirmation event" whether the client is subscribed to notifications or indications, which triggers the call stack that leads to onDataSent. The difference is it is triggered in the sending function for notifications but in the confirming function for indications. In other words, attsExecCallback is called in both cases, which calls attExecCallback with ATTS_HANDLE_VALUE_CNF as the event, so the Gatt Server cannot differentiate between notifications and indications.

Perhaps, we could define a separate ATT server callback event for notifications? This way we can call attExecCallback directly during the sending function, enabling the Gatt Server to differentiate between notifications and indications using the new event type. Of course, this requires modifications to Cordio which might be undesirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess you weren't at the standup but that's pretty much my conclusion as well - we could keep the event but then we have to make changes to Cordio and try to upstream them - it's up to Vincent.

Copy link

@noonfom noonfom left a comment

Choose a reason for hiding this comment

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

@paul-szczepanek-arm I added a comment here about a potential improvement.

For now, it make sense to deprecate onConfirmationReceived.

@mbed-ci
Copy link

mbed-ci commented Apr 30, 2021

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

@0xc0170 0xc0170 merged commit dace32a into ARMmbed:master May 3, 2021
@mergify mergify bot removed the ready for merge label May 3, 2021
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants