-
Notifications
You must be signed in to change notification settings - Fork 22
fix ble bug and compatible with the use cases from the documentation #18
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
Conversation
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.
Thanks for the contribution!
I have made some suggestions. Please have a look.
|
|
||
| print("Connecting to", device) | ||
| self.client = BleakClient(device) | ||
| self.client = BleakClient(device.address) |
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.
By passing the address instead of device object will cause Bleak to have to scan for the device again. This would make connections much slower.
| if device.name != name: | ||
| if device_or_address in [device.name, device.address]: | ||
| queue.put_nowait(device) | ||
| else: |
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.
else: return is not needed since there is nothing in the function after this.
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.
that is ok
| " address = await find_device('Pybricks Hub', timeout=5)\n", | ||
| " await hub.connect(address)\n", | ||
| " device = await find_device('Pybricks Hub', timeout=5)\n", | ||
| " await hub.connect(device.address)\n", |
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.
Shouldn't this be device and not device.address?
| async def find_device(name: str, timeout: float = 5) -> BLEDevice: | ||
| """Quickly find BLE device address by friendly device name. | ||
| async def find_device(device_or_address: str, timeout: float = 5) -> BLEDevice: | ||
| """Quickly find BLE device address by friendly device name or address. |
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.
We should also mention that "address" does not work on macOS. Instead, macOS assigns each device a UUID that has to be used instead of the Bluetooth address.
| Device was not found within the timeout. | ||
| """ | ||
| print("Searching for {0}".format(name)) | ||
| print("Searching for {0}".format(device_or_address)) |
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.
Since we need to make some other changes anyway, we could change this to:
| print("Searching for {0}".format(device_or_address)) | |
| print(f"Searching for {device_or_address}") |
edc8734 to
069206f
Compare
|
This should be fixed in v1.0.0-alpha.8. |
use cases from the documentation:
work on Mac!