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

Fixed StartUpdatesNativeAsync for indicate #858

Merged

Conversation

AskBojesen
Copy link
Contributor

To fix #847

@janusw
Copy link
Member

janusw commented May 19, 2024

This is sufficiently simple and well-contained for me to still consider it for 3.1.0, provided it actually solves the problem. @CodeMusher Does it fix #847 for you?

@CodeMusher
Copy link

@janusw, I haven't got the possibility to verify it today but it looks almost identical to the solution I provided when opening the issue so I guess it must.

@janusw
Copy link
Member

janusw commented May 19, 2024

@janusw, I haven't got the possibility to verify it today

No problem. When will you be able to verify it?

but it looks almost identical to the solution I provided when opening the issue

Yes, it's very close, but not identical.

so I guess it must.

Sorry, but guessing is not good enough here. Apparently @AskBojesen was not able to reproduce the issue and test the fix, and I'm not going to accept an untested PR at this stage (i.e. directly before a stable release).

@CodeMusher
Copy link

I'll try to test it tomorrow

@AskBojesen
Copy link
Contributor Author

BTW: I have tested using my "normal" device with Notity in char descriptors - and that still works as expected.

@CodeMusher
Copy link

Now I have tested with two different devices. One with characteristics with only INDICATE and one with a mixed population of characteristics, some INDICATE and some NOTIFY.
It works as expected!

Copy link
Member

@janusw janusw left a comment

Choose a reason for hiding this comment

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

Since both of you have tested it and code-wise it looks very clean, this is ok with me. Thanks for fixing this so quickly, @AskBojesen!

@janusw janusw merged commit b492d6d into dotnet-bluetooth-le:master May 20, 2024
3 checks passed
@AskBojesen AskBojesen deleted the windows_updates_indicate branch May 21, 2024 05:09
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.

Need to be able to register for notification from characteristics that is only INDICATE
3 participants