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

Explicit CCC added descriptor does not send notification/indication t… #10568

Closed
wants to merge 1 commit into from

Conversation

cy-kishore
Copy link

…o client

Description

Pull request type

[ x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested review from a team May 13, 2019 11:00
@paul-szczepanek-arm
Copy link
Member

I apologise the issue hasn't been triaged.

The lack of CCC callback is a problem that needs addressing ASAP but this needs to be fixed at the creation of the attribute at:
mbed-os\features\FEATURE_BLE\targets\TARGET_CORDIO\source\CordioGattServer.cpp

Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm left a comment

Choose a reason for hiding this comment

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

This is a workaround, a fix is need to solve the root cause of the problem: CCC should not have a ATTS_SET_WRITE_CBACK set in attribute settings insert_descriptor(). The special case handling of CCC should include checking for this conflict.

if (properties & WRITABLE_PROPERTIES && !(attribute_it->settings & ATTS_SET_CCC)) {
    attribute_it->settings |= ATTS_SET_WRITE_CBACK;
}

@paul-szczepanek-arm
Copy link
Member

paul-szczepanek-arm commented May 13, 2019

I really appreciate your taking the time to make the PR and I apologise for your issue not being triaged. This is a workaround rather than fix. I will make the PR for this and the other issue you reported with a proposed fix, link to it in this PR and tag you for comments to make sure it addresses your issue.

@cy-kishore
Copy link
Author

I really appreciate your taking the time to make the PR and I apologise for your issue not being triaged. This is a workaround rather than fix. I will make the PR for this and the other issue you reported with a proposed fix, link to it in this PR and tag you for comments to make sure it addresses your issue.

Sure. Please let me know if you require any further info from me.
FYI
Initially ATTS_SET_WRITE_CBACK was not set for CCCD. But this resulted in write callback not coming to the application. we have raised the following Jira for the same and this was resolved
#9771.

After this fix we saw that Notifications and indications are not being sent. for which I have raised following Jira.
#10316

What we found is because cccCback callback is not called, the attsCccCb.pCccTbl value is not getting updated. Because of this the stack is not sending out the notification which the application is sending.

@paul-szczepanek-arm
Copy link
Member

paul-szczepanek-arm commented May 14, 2019

superseeded by #9771, please close

@cy-kishore
Copy link
Author

superseeded by #9771, please close

Hi,
I did not understand the comment. As mentioned above in my previous comment, #9771 is raised by me and it fixes the issues of CCCD read and write callback not coming to the application. But this fix caused a side effect.

After this fix when application sends notification it is not being sent out. This PR fixes that.

What we found is, after the fix for #9771 cccCback callback is not called as it is in else if condition, the attsCccCb.pCccTbl value is not getting updated. Because of this the stack is not sending out the notification which the application is sending.

regards
Kishore

@paul-szczepanek-arm
Copy link
Member

paul-szczepanek-arm commented May 15, 2019

I pasted the wrong link in my comment above, it's superseded by #10575 - apologies for the confusion caused.

This can now be closed.

@cy-kishore
Copy link
Author

This is fixed in #10575 . This pull request is no longer needed.

@cy-kishore cy-kishore closed this May 16, 2019
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.

4 participants