-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
@paul-szczepanek-arm, thank you for your changes. |
b88b3fb
to
b61fc6b
Compare
* 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
.
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
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
Test results
Reviewers