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

Fix propagation of CoreBluetooth errors #554

Merged
merged 1 commit into from
Jun 3, 2021
Merged

Fix propagation of CoreBluetooth errors #554

merged 1 commit into from
Jun 3, 2021

Conversation

rtoijala
Copy link

@rtoijala rtoijala commented Jun 1, 2021

Errors from CBPeripeheralDelegate calls were being raised in the asyncio
event loop, but not in the correct task. In order to reach the user's
code, the error has to be passed through the event to the user call that
is awaiting the event.

Without these changes, e.g. ATT errors sent by the peripheral caused
error messages to be printed, but since the event that the user's call
was waiting on was never set, the user's code was blocked.

The implementation was adapted from #209

I have not been able to test with anything older than macOS Big Sur (11.4).

flake8, tox and pytest report loads of unrelated errors, but I did not see any
new errors from these changes.

Pull Request Guidelines for Bleak

Before you submit a pull request, check that it meets these guidelines:

  1. If the pull request adds functionality, the docs should be updated.
  2. Modify the CHANGELOG.rst, describing your changes as is specified by the
    guidelines in that document.
  3. The pull request should work for Python 3.6+ on the following platforms:
    • Windows 10, version 16299 (Fall Creators Update) and greater
    • Linux distributions with BlueZ >= 5.43
    • OS X / macOS >= 10.11
  4. Squash all your commits on your PR branch, if the commits are not solving
    different problems and you are committing them in the same PR. In that case,
    consider making several PRs instead.
  5. Feel free to add your name as a contributor to the AUTHORS.rst file!

Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

I was noticing this was a problem recently too, so I'm glad you have taken the time to fix it!

Instead of creating our own class based on asyncio.Event, how about using asyncio.Future instead?

@rtoijala
Copy link
Author

rtoijala commented Jun 2, 2021

Updated to use asyncio.Future.

@rtoijala rtoijala requested a review from dlech June 2, 2021 07:33
@dlech
Copy link
Collaborator

dlech commented Jun 2, 2021

This looks great. It looks like there are some formatting issues. Since we need to fix that anyway, it would be nice to change the messages to f"" strings instead of using .format().

Errors from CBPeripeheralDelegate calls were being raised in the asyncio
event loop, but not in the correct task. In order to reach the user's
code, the error has to be passed through the event to the user call that
is awaiting the event.

Without these changes, e.g. ATT errors sent by the peripheral caused
error messages to be printed, but since the event that the user's call
was waiting on was never set, the user's code was blocked.

The implementation was adapted from #209
Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

I've tested this now. Seems to be working. Thanks again!

@dlech dlech merged commit b5faaae into hbldh:develop Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants