FR: Subclassing exceptions to make them easier to catch

[mihtjel]

Description

I've been writing a piece of code that's getting .. more complicated than it probably should, I guess. But I have gotten a little annoyed by one thing in particular: The difficulty in handling the exceptions I get from Bleak:

Logger.error(f"Bleak error encountered: {e} ({e.__class__.__name__})")
gives me lines in my log like:

Bleak error encountered: org.bluez.Error.Failed (BleakDBusError)
Bleak error encountered: Device with address 01:02:03:04:05:06 was not found. (BleakError)
Bleak error encountered: Not connected (BleakError)

etc.

The first one is maybe difficult to do anything about, okay. But I have to do a manual parsing of the contents in my except BleakError as e: handler of the second two - and I think it would be more obvious if they were for example BleakDeviceNotFoundError and BleakNotConnectedError^*.

So, the suggestion: Make some exception classes for the expected types of returns from the library, and use BleakError for the rest.

(Obviously I'd be willing to contribute the code as well, but I won't start working on it if it's against a project principle.)

For example:

• BleakNotConnectedException
• BleakDeviceNotFoundException
• BleakPairingFailedException (Maybe used for Unpair as well? Or a separate one.)
• BleakBluetoothDisabledException
• BleakNotificationsAlreadyStartedException / BleakNotificationsNeverStartedException (Maybe these are genuinely errors)
• BleakReadException (when reading a characteristic fails)
• BleakWriteException (when writing a characteristic fails)

*: Or indeed BleakDeviceNotFoundException, as it is really not considered an error in the classical sense - a device not being found? But that's a different matter, and an issue with the Python naming conventions.

[mihtjel]

If possible, I'd like just a little feedback - if you are not directly opposed to the idea I would start working on a PR :-)

[bojanpotocnik]

Just one one about

The first one is maybe difficult to do anything about, okay.

since #522 this one usually also has description and can be further classified into above mentioned categories.

[hbldh]

I am not against this in principle, but I tend to not implement specific subclass errors until I actually need them, and I have not needed them as of yet in Bleak.

@mihtjel you are free to try. If you can improve Bleak by doing this, I am all for it. The hassle is that there are four backends in three different OS:es to implement and test, so it is a daunting task...

[mihtjel]

Thanks for the reply - I'll try to do it for the most obvious ones that I have wanted to catch and treat separately myself as a start at least. :-)

[jeffsf]

"Device not found" is an "expected exception" for applications where the target device comes and goes and the application periodically checks for the device's return.

Current check (based on what has been observed, not searching the code) is

                    if e.args[0].startswith(
                                'Device with address') \
                            or e.args[0].startswith(
                                'Connection was not successful!'):

In the same type of application, I can see it being desirable to "blanket catch" the class of errors related to trying an operation on a device that was connected, but no longer is, rather than checking for connectivity prior to every operation. (Not presently implemented in my code.)

[hbldh]

There should be more specific exceptions thrown, I agree.

But in you application, it must be possible to narrow the scopes of your try-catch blocks to catch what happens in each BLE call, no? You should not have to parse the text to handle the error programmatically?

[jeffsf]

The try-catch is very narrow and it is likely than a BleakException is that the device isn't present. However, it is certainly possible that there was a problem with the bus calls, contention for the device, or who knows what else.

[jochenjagers]

How can we proceed here? For my use case also "device not found" is a expected exception for unpair operations.
What do you think about starting with this exception and add other ones later on?
DeviceNotFound should be easy to find and implement in all backends. Also NotConnected seems to be an easy and obvious one.

[dlech]

"device not found" seems like a good starting point since it has been mentioned several times here. What would the specs be? My initial thoughts are that it should mean "the OS bluetooth stack has never seen this device or it was removed and forgotten". So for BlueZ, this would mean that there is no D-Bus object that corresponds to this device. On macOS, it would mean that retrievePeripheralsWithIdentifiers: doesn't contain the device. Some research is needed to figure out the equivalent on Windows and Android. The exception would be throw from connect, pair and unpair methods of BleakClient. This would not be applicable to BleakScanner.

[dlech]

For a "not connected" error, we could save some work on the implementation by waiting for #982 to be merged. Then it only has to be implemented in the front end instead of each individual backend. Although this one is a bit tricky since a device can be disconnected during an operation in addition to not being connected when the operation is connected. Is that the same error or a different error?

[jochenjagers]

"the OS bluetooth stack has never seen this device or it was removed and forgotten"

That's exactly what I would expect from this exception.

The exception would be throw from connect, pair and unpair methods of BleakClient

Is is theoretically possible to pair with an device without being connected? Pairing requires communication so I had expected calling connect is required before calling pair. Or do the backends establish the connection implicitly when pairing?

For the WinRT backend:
connect searches for the device with BleakScanner.find_device_by_address first and used the returned advertising data to create the BluetoothLEDevice with from_bluetooth_address_async
unpair also calls from_bluetooth_address_async for creating the device object and uses data from a previous connect call or alternatively the Bluetooth device address.
calling pair is not possible without connection at this moment

Besides from_bluetooth_address_async there is only one other way to create the BluetoothLEDevice. This is the FromIdAsync function with uses the windows device id. But this function has a big limitation. It must be called from a UI thread which makes it useless for many use cases.
https://learn.microsoft.com/en-us/uwp/api/windows.devices.bluetooth.bluetoothledevice.fromidasync?view=winrt-22621#windows-devices-bluetooth-bluetoothledevice-fromidasync(system-string)

So for now we should be fine with raising DeviceNotFound in connect if find_device_by_address returns None and in unpair if from_bluetooth_address_async returns None.

This might be still a problem for devices with resolvable private addresses but as we don't change anything to the device discovery we won't make it worse for them.

What about naming and data?
I suggest BleakDeviceNotFoundError as name and would add the identifier used for searching the device as an extra parameter.

[dlech]

Is is theoretically possible to pair with an device without being connected? Pairing requires communication so I had expected calling connect is required before calling pair. Or do the backends establish the connection implicitly when pairing?

I was wondering the same thing 😉. #309 (comment)

So I'll answer the questions there so we don't get too off topic here.

---

It must be called from a UI thread which makes it useless for many use cases.

This may be true for Window Store apps, but I'm not sure that it is true for "desktop" apps.

I suggest BleakDeviceNotFoundError as name and would add the identifier used for searching the device as an extra parameter.

👍 Would the identifier be the BLEDevice or just the Bluetooth address (UUID on macOS)?

[jochenjagers]

I created a fist draft for further discussion:
#1022