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

Updates to CoreBluetooth backend #209

Closed
wants to merge 32 commits into from
Closed

Conversation

bsiever
Copy link

@bsiever bsiever commented May 30, 2020

This includes several updates to the CoreBluetooth backend. These include:

  • Support for scanning w/ filtering (Using BlueZ-style flags)
  • Performance improvements (connect will use cached peripheral for faster connect following discovery and/or faster reconnecting)
  • Several bug fixes
    • Some errors were causing crashes rather than raising exceptions. (Exceptions have been moved out of delegate callbacks)
  • Initial work for supporting multiple clients (still needs further testing)
    • Different approach to disconnect callbacks for multiple peripherals
  • Misc. small consistency changed (Ex: importing/using CoreBluetooth constants rather than their values)
  • A few of these are a bit hacky and merit revision

There are a lot of changes. I'm happy to answer any questions and won't be hurt if this is rejected. I'm also able to do a PR for my test suite (pretty rough) to help quickly vet the work (requires 1+ microbit).

@5frank
Copy link

5frank commented May 31, 2020

Nice! I have not had the time to test it yet.
Some thoughts:

Not sure I like adding variables to class instances. e.g

        self[xUUID]._error = None  # Clear error prior to new event

I believe it is a bit confusing and it makes pylint unhappy. Perhaps inherit ayncio.Event instead? This would also make it easy to add a timeout to event.wait() if/when needed and remove some duplicated code. Example (untested):

class _BleakCorebluetoothEvent(asyncio.Event):
    def __init__(self):
        self._error = None
        super().__init__()

    def clear(self):
        self._error = None
        super().clear()

    def set(error: NSError = None): # or pass a Exception?
        self._error = error
        super().set()

    async def wait(self, timeout=None):
        if timeout:
            try:
                await asyncio.wait_for(super().wait(), timeout)
            except asyncio.TimeoutError:
                raise TimeoutError("")
        else:
            await super().wait()

        if self._error:
            raise SomeException(self._error)

@bsiever
Copy link
Author

bsiever commented May 31, 2020

My approach is a hack. I like the idea of a subclass (and better semantics for waits/timeouts). I'll probably try to incorporate your approach in the next few days.

@hbldh
Copy link
Owner

hbldh commented Jun 2, 2020

Whoa. This was a lot of changes in one PR... I would have liked them spread out over several to be able to review them better. Nevertheless, thank you!

I will await you changes and then plan to look into this as soon as possible though.

@bsiever
Copy link
Author

bsiever commented Jun 2, 2020

Yeah, sorry for the number of changes in one batch. It was one of my motivations for Issue #207 (Unit testing with devices). My development path was pretty haphazard. I'll try to be a little more structured in the future.

I've incorporated a first pass at @5frank's approach, which is much much cleaner. It also provides an opportunity for timeouts on all device events, which could make "hanging" almost impossible. I think this could use further review discussion, but the constructors in initWithPeripheral_ just need to specify the timeout for events. I've chosen to include the new _BleakCorebluetoothEvent class in PeripheralDelegate.py. It may be better as a separate file so it could be used elsewhere.

@f0rodo
Copy link

f0rodo commented Jun 9, 2020

+1 This fixed bleak breaking in Catalina

@dlech
Copy link
Collaborator

dlech commented Jun 21, 2020

Hi @bsiever, can we break this down into some smaller pull requests to start getting things merged? I started fixing things myself before I saw this PR. Changes are here if you want to compare notes (still a bit of a work in progress though).

@hbldh
Copy link
Owner

hbldh commented Jun 23, 2020

I have planned on devoting an entire day to bleak during next week, with explicit focus on macOS issues. @dlech Could you integrate your changes on with these ones (if needed)? I could merge this into a separate branch in my repo so that additional changes can be appended. Would that help?

@bsiever Thank you for the work done here. I will try it out first thing on the Bleak-day!

@hbldh hbldh added this to the Version 0.7.0 milestone Jun 23, 2020
@hbldh hbldh self-assigned this Jun 23, 2020
@hbldh hbldh added the Backend: Core Bluetooth Issues and PRs relating to the Core Bluetooth backend label Jun 23, 2020
@bsiever
Copy link
Author

bsiever commented Jun 23, 2020

@hbldh I hope Bleak Day goes well! If you have a micro:bit the unit tests I've made can help quickly vet that additions aren't breaking too many things.

@dlech It's clear that you caught things I missed and I restructured parts and added features you haven't hit. We both made a lot of changes and it's definitely a bit messy to integrate. The "big ones" for me were:

  • Moved error handling out of delegates. That was causing un-catchable Python crashes.
  • Added support for disconnect via weak references from the cbapp singleton.
  • Added support for multiple clients (also using the cbapp's weak references for some updates)
  • Used cached CBPeripheral objects for faster connections
  • Lots of small, misc. error handling.

I probably won't have time to make many updates for the next week or two, but I should have time to reply to any questions.

@hbldh
Copy link
Owner

hbldh commented Jun 30, 2020

I merged the PR #227 before this one, since it was smaller and contained exacty what I needed for the moment. Regrettably I have to had time to merge this with that yet, so I will leave this one for a 0.7.X release in a few days.

@hbldh hbldh modified the milestones: Version 0.7.0, Version 0.7.X Jun 30, 2020
hbldh added a commit that referenced this pull request Jul 9, 2020
…er-develop

Merge #209

# Conflicts:
#	bleak/backends/corebluetooth/CentralManagerDelegate.py
#	bleak/backends/corebluetooth/PeripheralDelegate.py
#	bleak/backends/corebluetooth/__init__.py
#	bleak/backends/corebluetooth/client.py
#	bleak/backends/corebluetooth/discovery.py
#	bleak/backends/corebluetooth/scanner.py
@hbldh
Copy link
Owner

hbldh commented Jul 9, 2020

@bsiever I tried to do a careful merge of your changes into a separate branch (bsiever-develop), but it ended up in a broken state. I do not now how to move forward from there and I think I have to abandon the attempt. Reluctantly, I will close this PR and ask you to update your changes with current develop. Thank you for the effort.

The things I liked in the PR, that wasn't solved in merging of #227:

  1. the scanning filter work: If you could integrate your scanning filter work with the current develop and make a PR I would be very grateful. If there are more changes still needed for making your test suite run, please add them in a separate PR and we can address that separately.
  2. the reuse of scan results: For the reuse of scan results I have an idea for version 0.8.0 that will collide with that, so for the time being I will go a different route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend: Core Bluetooth Issues and PRs relating to the Core Bluetooth backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants