-
Notifications
You must be signed in to change notification settings - Fork 334
Bluez notify read fix #1856
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
Bluez notify read fix #1856
Conversation
|
Interesting... I'm surprised this hasn't come up before since we have much more Linux users that Mac and this is basically the same issue we recently solved on Mac. For the BlueZ case though, I think we can solve it a better way by using AcquireNotify. We do know that not all devices correctly report "notify" and "indicate" flags though. So the logic would have to go like: If d-bus characteristic object has property "NotifyAcquired" then use "AcquireNotify" else use "Value" property change. This way, it would just work as expected without a need to write a discriminator. On the other hand, since users would have to write a discriminator for Mac anyway, maybe it isn't worth the extra complexity? On the other, other hand, since Linux is much more popular, it might be worth it still since many users only care about running on Linux. |
Ah, I see you built the discriminator into the notification callback instead of using the one in |
Oh right, in the CoreBluetooth backend, we assume that if there is a pending read request then the changed value must be the response to the read request and don't send the notification in that case. However, that isn't 100% foolproof without a discriminator since a notification could be received after the read request but before the read response. |
|
I've updated it to optionally use ActiveNotify if the characteristic presents it and removed the discriminator. This is definitely a better solution. Let me know what feedback you have on the changes! Thanks! |
dlech
left a comment
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.
Nice, thanks for updating!
I made a few quick comments, then I would like to try it out when I have a chance.
|
Hi @dlech, do you have any additional thoughts on the changes above? Thanks! |
|
I added one more tweak to add some logging the the |
I ran into a problem using the BlueZ backend whereas when notifying on a characteristic and triggering a read, the data from the response comes back twice. Once as a read, and once as a notification. This occurs in instances where a peripheral sends a notification which requires a reread.
I wrote this test script to show the error:
On MacOS with a peripheral I can trigger notifications on, I get a single notification, and then single re-read (which is expected):
On my Linux machine however, I receive the data from the characteristics in both the re-read, and as an additional notification:
This change adds a 'notification_discriminator' argument to Bleak's start_notify method to suppress notifications which don't pass the discriminator.