added BleakDeviceNotFoundError #1022
Conversation
jochenjagers
changed the title
|
I basically only replace the BleakErrors with device not found messages by BleakDeviceNotFoundError CoreBluetooth:
WinRT:
BlueZ:
p4android:
The Android backend does not have a check for the availability of the device yet. |
| raise BleakError( | ||
| "Device with address {0} was not found.".format(self.address) | ||
| raise BleakDeviceNotFoundError( | ||
| self.address, f"Device with address {self.address} was not found." |
There was a problem hiding this comment.
I left the messages in there to not break compatibility with programs which check for the text as described in #527 (comment)
bleak/exc.py
Outdated
| def __init__(self, identifier: str, *args: object) -> None: | ||
| """ | ||
| Args: | ||
| identifier (str): The identifier which was used to search the device (e. g. Bluetooth address or UUID) |
There was a problem hiding this comment.
I only added the identifier here for now. But it can also be the BLEDevice to share some more information about the device if they are available. I'm a bit ambivalent about this.
There was a problem hiding this comment.
I think it would be best to use the BLEDevice object to wrap the platform-specific details.
There was a problem hiding this comment.
I changed it to the BLEDevice.
There was a problem hiding this comment.
In the connect methods, in addition to the optional scanner not finding the device, we need to handle the case for when the device is removed from the OS between the external scan and the connection request.
For example, in the BlueZ backend, if the "Connected" D-Bus method reply contains a org.freedesktop.DBus.Error.UnknownObject error, then we should raise the new BleakDeviceNotFoundError .
I'm not sure what it would be in the other backends, so I guess we will have to do some experimentation to find out.
bleak/exc.py
Outdated
| def __init__(self, identifier: str, *args: object) -> None: | ||
| """ | ||
| Args: | ||
| identifier (str): The identifier which was used to search the device (e. g. Bluetooth address or UUID) |
There was a problem hiding this comment.
I think it would be best to use the BLEDevice object to wrap the platform-specific details.
jochenjagers
force-pushed
the
feature/device-not-found-error
branch
from
9fc5682 to
42c5066
Compare
| self._peripheral, disconnect_callback, timeout=timeout | ||
| ) | ||
| except asyncio.TimeoutError as error: | ||
| raise BleakDeviceNotFoundError( |
There was a problem hiding this comment.
I seems like corebluetooth has no real mechanism for detecting "device not found" errors. the connect function just receives a Timeout error. Can we assume that the device was not found in case of this?
There was a problem hiding this comment.
I think we should probably just leave this as a timeout.
If we really want a "device not found" here, I suppose we could check if the peripheral is still in the list returned by CBCentalManager.retrievePeripheralsWithIdentifiers_ before attempting to connect.
There was a problem hiding this comment.
but I think it is fine to just leave it as-is for now.
There was a problem hiding this comment.
If we really want a "device not found" here, I suppose we could check if the peripheral is still in the list returned by
CBCentalManager.retrievePeripheralsWithIdentifiers_before attempting to connect.
This is already done a few lines above where the device is searched using the bleak scanner. The catch for the TimeoutError was only for the edge-case in which the device goes out of sight in between.
But I reverted this change as it might be misleading if the connection attempt times out for other reasons.
| if ( | ||
| reply is not None | ||
| and reply.message_type == MessageType.ERROR | ||
| and reply.error_name == ErrorType.UNKNOWN_OBJECT |
There was a problem hiding this comment.
Are you sure about the UnknownObject error? The documentation does not mention it:
https://github.com/bluez/bluez/blob/master/doc/device-api.txt
There was a problem hiding this comment.
Yes. It is a standard D-Bus error. (Searching the issues in this repo for "UnknownObject" reveals quite a few hits)
|
For Android the documentation is ambiguous.
On the other hand:
|
It sounds like we shouldn't worry about it on android then. |
...
I suppose it could be the case that the non-BlueZ backends actually allow connecting to a device without having ever "seen" it before, in which case I guess there isn't an extra case to handle. (As a side note, BlueZ has an Adapter1.ConnectDevice experimental method that in theory could be used to do the same, but we removed it recently because it doesn't seem to work as expected). |
jochenjagers
force-pushed
the
feature/device-not-found-error
branch
from
42c5066 to
8c237e4
Compare
|
From my point of view this is ready to merge now. |
bleak/backends/winrt/client.py
Outdated
| requester = BluetoothLEDevice.from_bluetooth_address_async(*args) | ||
| if requester is None: | ||
| raise BleakDeviceNotFoundError( | ||
| BLEDevice(self.address), |
There was a problem hiding this comment.
Sorry I didn't think about this before when I suggested using BLEDevice, but if we don't have any other information besides the address, then it doesn't make sense to create an incomplete BLEDevice object. So probably better to just stick with the "identifier" as you originally had it.
There was a problem hiding this comment.
No problem. I changed it back to the identifier version.
Nevertheless storing the whole BLEDevice might be an interesting improvement for the future.
jochenjagers
force-pushed
the
feature/device-not-found-error
branch
from
8c237e4 to
b6e811a
Compare
|
Thanks for updating! |
Thanks for your fast and constructive feedback. |
This is a first draft for BleakDeviceNotFoundError as discussed in #527