bleak: raise BleakBluetoothNotAvailableError from BleakAdapter.get() on Linux and Windows - #1986
bleak: raise BleakBluetoothNotAvailableError from BleakAdapter.get() on Linux and Windows#1986Vodur wants to merge 1 commit into
Conversation
Vodur
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1986 +/- ##
===========================================
- Coverage 52.45% 52.40% -0.05%
===========================================
Files 43 43
Lines 4097 4110 +13
Branches 504 508 +4
===========================================
+ Hits 2149 2154 +5
- Misses 1817 1826 +9
+ Partials 131 130 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
1c9c6da to
37603c4
Compare
|
I've been hoping to move Bleak to using structured concurrency. If we add a property that can be read, then people are just going to poll it in a loop with a sleep, which is not good program design. So I would prefer to leave out the And callbacks aren't good program design for async programming either. They were just the only thing available when Bleak was written. So I would prefer to avoid that too. Structured concurrency might look something like this: async def main():
while True:
try:
async with BleakAdapter.get():
# if Bluetooth adapter state changes inside of this
# context, it will cancel the task
await do_stuff_with_bluetooth()
except BleakBluetoothNotAvailableError as ex:
try_again = await inform_user_of_reason_and_and_ask(ex)
if try_again:
continue
breakI think this is going to take more though and trying out different things to figure out if this can actually work as intended. I consider it lower priority that other things we have open right now. |
It looks like it does. https://learn.microsoft.com/en-us/uwp/api/windows.devices.radios.radio.statechanged?view=winrt-28000 |
|
Thanks - structured concurrency does make sense, and
Building on your sketch, this is roughly how the user side could look: from contextlib import asynccontextmanager
async def main():
while True:
try:
# async ctx mgr: raises immediately if not POWERED_ON;
# cancels the body if state changes mid-context.
async with BleakAdapter.acquire() as adapter:
await do_stuff_with_bluetooth(adapter)
except BleakBluetoothNotAvailableError as ex:
if not await inform_user_and_retry(ex):
break
Implementation-side it'd be something like (rough sketch):
@asynccontextmanager
async def acquire(cls, *, bluez: BlueZAdapterArgs = {}):
backend = await PlatformBleakAdapter.get(bluez=bluez)
if backend.state != AdapterState.POWERED_ON:
raise BleakBluetoothNotAvailableError(...)
async with asyncio.TaskGroup() as tg:
async def _watch() -> None:
async for state in backend.state_events(): # platform-native source
if state != AdapterState.POWERED_ON:
raise BleakBluetoothNotAvailableError(...)
tg.create_task(_watch())
try:
yield cls(backend)
finally:
tg.cancel()If you'd rather close this and revisit later, happy to. If the sketch above is roughly the direction you had in mind, I can take a stab at it as a fresh PR (using |
Actually, |
|
As for the async context manager implementation and async iterator, this can be really tricky to get right. I've been considering using anyio to help with this as it has solved a lot of these hard-to-get-right things already. And we can't use |
|
Splitting the work: this PR ships building blocks only - The cancel-on-state-change wrapper goes in a follow-up where the anyio decision lives. Whether it will be anyio or dropping support for Python 3.10. Ok to push? |
|
I would prefer to just consider making raising of All other proposed changes here are lower priority for me. |
ec95c93 to
7fc5f3e
Compare
Vodur
changed the title
f31734e to
dd25b6e
Compare
Vodur
changed the title
dd25b6e to
1341a68
Compare
|
Only codecov patch is failing here. The WinRT raise paths in Fine to leave the WinRT paths uncovered, or would you rather I add a Windows integration job? |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
This one is hard to test since it requires very conditions on the OS. So I am OK to leave figuring out some good tests for later.
|
|
||
| Changed | ||
| ------- | ||
| * Changed ``BleakAdapter.get()`` to raise ``BleakBluetoothNotAvailableError`` on Linux and Windows when the local Bluetooth adapter is not powered on, matching the existing CoreBluetooth behaviour. |
There was a problem hiding this comment.
There has still not been a release with BleakAdapter, so changelog for anything changing BleakAdapter does not make sense. The Added section already covers adding this new class.
| radio = None | ||
| for candidate in await Radio.get_radios_async(): | ||
| if candidate.kind == RadioKind.BLUETOOTH: | ||
| radio = candidate | ||
| break | ||
| if radio is None: |
There was a problem hiding this comment.
| radio = None | |
| for candidate in await Radio.get_radios_async(): | |
| if candidate.kind == RadioKind.BLUETOOTH: | |
| radio = candidate | |
| break | |
| if radio is None: | |
| for radio in await Radio.get_radios_async(): | |
| if radio.kind == RadioKind.BLUETOOTH: | |
| break | |
| else: |
Can be a bit simpler this way.
There was a problem hiding this comment.
radio loop is gone, get() now uses BluetoothAdapter.get_default_async() instead.
| adapter_path = ( | ||
| f"/org/bluez/{adapter}" if adapter else manager.get_default_adapter() | ||
| ) | ||
|
|
There was a problem hiding this comment.
It would probably be better to just call BlueZManager.get_default_adapter() from here so that we don't have to duplicate the logic.
…on Linux and Windows CoreBluetooth already raises BleakBluetoothNotAvailableError from BleakAdapter.get() via wait_until_ready() when Bluetooth is not powered on. This makes the BlueZ and WinRT backends do the same, so callers can rely on the exception across all platforms. - BlueZ: checks for adapter presence in the manager's cached properties and the Powered property; raises NO_BLUETOOTH or POWERED_OFF. - WinRT: fetches the Bluetooth radio via Radio.get_radios_async() and checks RadioState; raises NO_BLUETOOTH or POWERED_OFF.
1341a68 to
eca7a80
Compare
2 participants
I would prefer to just consider making raising of
BleakBluetoothNotAvailableErrorfromBleakAdapter.get()consistent on all platforms first.BleakAdapter.get()now raisesBleakBluetoothNotAvailableErroron all platforms when the adapter is not powered on. CoreBluetooth already did this viawait_until_ready(); this adds the same behavior to the BlueZ and WinRT backends:Poweredis set; raises with reasonNO_BLUETOOTHorPOWERED_OFF.Radio.get_radios_async()and checksRadioState; raises with reasonNO_BLUETOOTHorPOWERED_OFF.Testing
test_get_raises_when_powered_off) that power-cycles the adapter viabluetoothctlin the vhci VM.codecov/patchgap.The
stateproperty andsubscribe_state_changes()work (native event sources, no polling) is deferred to a follow-up PR.Refs #320, #1060.
Notes: