From ad59f6e8e35cbab8276d654faafa7856c1fd8b9b Mon Sep 17 00:00:00 2001 From: David Lechner Date: Sat, 23 Oct 2021 10:45:10 -0500 Subject: [PATCH 1/4] CoreBluetooth: delete futures after awaiting In the CoreBluetooth backend, futures are used to pass results/errors from the delegate callbacks to methods that are waiting for them. The futures are stored as part of the object state so that the delegates can access them. Prior to this change, completed futures were not removed from the object status. This worked most of the time because a second call of the same method would replace the completed future from the prior call. However, in the case of notifications and reads, the same delegate callback is shared with two methods. So when both of these were used, notifications would attempt to complete the already completed read future which raises an InvalidStateError. Fixes #675. --- CHANGELOG.rst | 5 + .../corebluetooth/CentralManagerDelegate.py | 33 +++++-- .../corebluetooth/PeripheralDelegate.py | 98 ++++++++++++++----- 3 files changed, 99 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4e5d733c..7cc5772a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,6 +10,11 @@ and this project adheres to `Semantic Versioning None: future = self.event_loop.create_future() + self._disconnect_futures[peripheral.identifier()] = future - self.central_manager.cancelPeripheralConnection_(peripheral) - await future - del self._disconnect_callbacks[peripheral.identifier()] + try: + self.central_manager.cancelPeripheralConnection_(peripheral) + await future + finally: + del self._disconnect_callbacks[peripheral.identifier()] @objc.python_method def _changed_is_scanning(self, is_scanning: bool) -> None: @@ -280,7 +293,7 @@ def centralManager_didDiscoverPeripheral_advertisementData_RSSI_( def did_connect_peripheral( self, central: CBCentralManager, peripheral: CBPeripheral ) -> None: - future = self._connect_futures.pop(peripheral.identifier(), None) + future = self._connect_futures.get(peripheral.identifier(), None) if future is not None: future.set_result(True) @@ -301,7 +314,7 @@ def did_fail_to_connect_peripheral( peripheral: CBPeripheral, error: Optional[NSError], ) -> None: - future = self._connect_futures.pop(peripheral.identifier(), None) + future = self._connect_futures.get(peripheral.identifier(), None) if future is not None: if error is not None: future.set_exception(BleakError(f"failed to connect: {error}")) @@ -331,7 +344,7 @@ def did_disconnect_peripheral( ) -> None: logger.debug("Peripheral Device disconnected!") - future = self._disconnect_futures.pop(peripheral.identifier(), None) + future = self._disconnect_futures.get(peripheral.identifier(), None) if future is not None: if error is not None: future.set_exception(BleakError(f"disconnect failed: {error}")) diff --git a/bleak/backends/corebluetooth/PeripheralDelegate.py b/bleak/backends/corebluetooth/PeripheralDelegate.py index 6c3d1bab..4cf0c852 100644 --- a/bleak/backends/corebluetooth/PeripheralDelegate.py +++ b/bleak/backends/corebluetooth/PeripheralDelegate.py @@ -87,29 +87,43 @@ def futures(self) -> Iterable[asyncio.Future]: @objc.python_method async def discover_services(self) -> NSArray: future = self._event_loop.create_future() + self._services_discovered_future = future - self.peripheral.discoverServices_(None) - await future + try: + self.peripheral.discoverServices_(None) + await future + finally: + del self._services_discovered_future return self.peripheral.services() @objc.python_method async def discover_characteristics(self, service: CBService) -> NSArray: future = self._event_loop.create_future() + self._service_characteristic_discovered_futures[service.startHandle()] = future - self.peripheral.discoverCharacteristics_forService_(None, service) - await future + try: + self.peripheral.discoverCharacteristics_forService_(None, service) + await future + finally: + del self._service_characteristic_discovered_futures[service.startHandle()] return service.characteristics() @objc.python_method async def discover_descriptors(self, characteristic: CBCharacteristic) -> NSArray: future = self._event_loop.create_future() + self._characteristic_descriptor_discover_futures[ characteristic.handle() ] = future - self.peripheral.discoverDescriptorsForCharacteristic_(characteristic) - await future + try: + self.peripheral.discoverDescriptorsForCharacteristic_(characteristic) + await future + finally: + del self._characteristic_descriptor_discover_futures[ + characteristic.handle() + ] return characteristic.descriptors() @@ -121,9 +135,14 @@ async def read_characteristic( return characteristic.value() future = self._event_loop.create_future() + self._characteristic_read_futures[characteristic.handle()] = future - self.peripheral.readValueForCharacteristic_(characteristic) - await asyncio.wait_for(future, timeout=5) + try: + self.peripheral.readValueForCharacteristic_(characteristic) + await asyncio.wait_for(future, timeout=5) + finally: + del self._characteristic_read_futures[characteristic.handle()] + if characteristic.value(): return characteristic.value() else: @@ -137,9 +156,13 @@ async def read_descriptor( return descriptor.value() future = self._event_loop.create_future() + self._descriptor_read_futures[descriptor.handle()] = future - self.peripheral.readValueForDescriptor_(descriptor) - await future + try: + self.peripheral.readValueForDescriptor_(descriptor) + await future + finally: + del self._descriptor_read_futures[descriptor.handle()] return descriptor.value() @@ -154,23 +177,32 @@ async def write_characteristic( # CBCharacteristicWriteWithoutResponse if response == CBCharacteristicWriteWithResponse: future = self._event_loop.create_future() - self._characteristic_write_futures[characteristic.handle()] = future - - self.peripheral.writeValue_forCharacteristic_type_( - value, characteristic, response - ) - if response == CBCharacteristicWriteWithResponse: - await future + self._characteristic_write_futures[characteristic.handle()] = future + try: + self.peripheral.writeValue_forCharacteristic_type_( + value, characteristic, response + ) + await future + finally: + del self._characteristic_write_futures[characteristic.handle()] + else: + self.peripheral.writeValue_forCharacteristic_type_( + value, characteristic, response + ) return True @objc.python_method async def write_descriptor(self, descriptor: CBDescriptor, value: NSData) -> bool: future = self._event_loop.create_future() + self._descriptor_write_futures[descriptor.handle()] = future - self.peripheral.writeValue_forDescriptor_(value, descriptor) - await future + try: + self.peripheral.writeValue_forDescriptor_(value, descriptor) + await future + finally: + del self._descriptor_write_futures[descriptor.handle()] return True @@ -185,9 +217,13 @@ async def start_notifications( self._characteristic_notify_callbacks[c_handle] = callback future = self._event_loop.create_future() + self._characteristic_notify_change_futures[c_handle] = future - self.peripheral.setNotifyValue_forCharacteristic_(True, characteristic) - await future + try: + self.peripheral.setNotifyValue_forCharacteristic_(True, characteristic) + await future + finally: + del self._characteristic_notify_change_futures[c_handle] return True @@ -198,9 +234,13 @@ async def stop_notifications(self, characteristic: CBCharacteristic) -> bool: raise ValueError("Characteristic notification never started") future = self._event_loop.create_future() + self._characteristic_notify_change_futures[c_handle] = future - self.peripheral.setNotifyValue_forCharacteristic_(False, characteristic) - await future + try: + self.peripheral.setNotifyValue_forCharacteristic_(False, characteristic) + await future + finally: + del self._characteristic_notify_change_futures[c_handle] self._characteristic_notify_callbacks.pop(c_handle) @@ -209,9 +249,13 @@ async def stop_notifications(self, characteristic: CBCharacteristic) -> bool: @objc.python_method async def read_rssi(self) -> NSNumber: future = self._event_loop.create_future() + self._read_rssi_futures[self.peripheral.identifier()] = future - self.peripheral.readRSSI() - return await future + try: + self.peripheral.readRSSI() + return await future + finally: + del self._read_rssi_futures[self.peripheral.identifier()] # Protocol Functions @@ -388,7 +432,7 @@ def did_write_value_for_characteristic( characteristic: CBCharacteristic, error: Optional[NSError], ): - future = self._characteristic_write_futures.pop(characteristic.handle(), None) + future = self._characteristic_write_futures.get(characteristic.handle(), None) if not future: return # event only expected on write with response if error is not None: @@ -489,7 +533,7 @@ def peripheral_didUpdateNotificationStateForCharacteristic_error_( def did_read_rssi( self, peripheral: CBPeripheral, rssi: NSNumber, error: Optional[NSError] ) -> None: - future = self._read_rssi_futures.pop(peripheral.identifier(), None) + future = self._read_rssi_futures.get(peripheral.identifier(), None) if not future: logger.warning("Unexpected event did_read_rssi") From 43c30316b6ef8915916685fdee949a9bf3120223 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Sat, 23 Oct 2021 13:07:49 -0500 Subject: [PATCH 2/4] CoreBluetooth: pass return values via futures This updates CoreBluetooth to read the changed value in the delegate callback and pass it back to the BleakClient method via the future object. By reading the value in the delegate callback rather than the method scheduled by call_soon_threadsafe(), we ensure that we get the original value from the delegate callback and not a possibly changed value later. --- .../corebluetooth/PeripheralDelegate.py | 39 +++++++++---------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/bleak/backends/corebluetooth/PeripheralDelegate.py b/bleak/backends/corebluetooth/PeripheralDelegate.py index 4cf0c852..42b32397 100644 --- a/bleak/backends/corebluetooth/PeripheralDelegate.py +++ b/bleak/backends/corebluetooth/PeripheralDelegate.py @@ -91,12 +91,10 @@ async def discover_services(self) -> NSArray: self._services_discovered_future = future try: self.peripheral.discoverServices_(None) - await future + return await future finally: del self._services_discovered_future - return self.peripheral.services() - @objc.python_method async def discover_characteristics(self, service: CBService) -> NSArray: future = self._event_loop.create_future() @@ -104,12 +102,10 @@ async def discover_characteristics(self, service: CBService) -> NSArray: self._service_characteristic_discovered_futures[service.startHandle()] = future try: self.peripheral.discoverCharacteristics_forService_(None, service) - await future + return await future finally: del self._service_characteristic_discovered_futures[service.startHandle()] - return service.characteristics() - @objc.python_method async def discover_descriptors(self, characteristic: CBCharacteristic) -> NSArray: future = self._event_loop.create_future() @@ -139,15 +135,10 @@ async def read_characteristic( self._characteristic_read_futures[characteristic.handle()] = future try: self.peripheral.readValueForCharacteristic_(characteristic) - await asyncio.wait_for(future, timeout=5) + return await asyncio.wait_for(future, timeout=5) finally: del self._characteristic_read_futures[characteristic.handle()] - if characteristic.value(): - return characteristic.value() - else: - return b"" - @objc.python_method async def read_descriptor( self, descriptor: CBDescriptor, use_cached: bool = True @@ -160,12 +151,10 @@ async def read_descriptor( self._descriptor_read_futures[descriptor.handle()] = future try: self.peripheral.readValueForDescriptor_(descriptor) - await future + return await future finally: del self._descriptor_read_futures[descriptor.handle()] - return descriptor.value() - @objc.python_method async def write_characteristic( self, @@ -261,7 +250,7 @@ async def read_rssi(self) -> NSNumber: @objc.python_method def did_discover_services( - self, peripheral: CBPeripheral, error: Optional[NSError] + self, peripheral: CBPeripheral, services: NSArray, error: Optional[NSError] ) -> None: future = self._services_discovered_future if error is not None: @@ -269,7 +258,7 @@ def did_discover_services( future.set_exception(exception) else: logger.debug("Services discovered") - future.set_result(None) + future.set_result(services) def peripheral_didDiscoverServices_( self, peripheral: CBPeripheral, error: Optional[NSError] @@ -278,12 +267,17 @@ def peripheral_didDiscoverServices_( self._event_loop.call_soon_threadsafe( self.did_discover_services, peripheral, + peripheral.services(), error, ) @objc.python_method def did_discover_characteristics_for_service( - self, peripheral: CBPeripheral, service: CBService, error: Optional[NSError] + self, + peripheral: CBPeripheral, + service: CBService, + characteristics: NSArray, + error: Optional[NSError], ): future = self._service_characteristic_discovered_futures.get( service.startHandle() @@ -300,7 +294,7 @@ def did_discover_characteristics_for_service( future.set_exception(exception) else: logger.debug("Characteristics discovered") - future.set_result(None) + future.set_result(characteristics) def peripheral_didDiscoverCharacteristicsForService_error_( self, peripheral: CBPeripheral, service: CBService, error: Optional[NSError] @@ -310,6 +304,7 @@ def peripheral_didDiscoverCharacteristicsForService_error_( self.did_discover_characteristics_for_service, peripheral, service, + service.characteristics(), error, ) @@ -374,7 +369,7 @@ def did_update_value_for_characteristic( future.set_exception(exception) else: logger.debug("Read characteristic value") - future.set_result(None) + future.set_result(value) def peripheral_didUpdateValueForCharacteristic_error_( self, @@ -396,6 +391,7 @@ def did_update_value_for_descriptor( self, peripheral: CBPeripheral, descriptor: CBDescriptor, + value: NSObject, error: Optional[NSError], ): future = self._descriptor_read_futures.get(descriptor.handle()) @@ -409,7 +405,7 @@ def did_update_value_for_descriptor( future.set_exception(exception) else: logger.debug("Read descriptor value") - future.set_result(None) + future.set_result(value) def peripheral_didUpdateValueForDescriptor_error_( self, @@ -422,6 +418,7 @@ def peripheral_didUpdateValueForDescriptor_error_( self.did_update_value_for_descriptor, peripheral, descriptor, + descriptor.value(), error, ) From 74f16b4149ee16c9d2854ce2f56a32bced2ef96f Mon Sep 17 00:00:00 2001 From: David Lechner Date: Sat, 23 Oct 2021 13:16:43 -0500 Subject: [PATCH 3/4] CoreBluetooth: drop return True in PeripheralDelegate This removes `return True` from several PeripheralDelegate methods. These methods unconditionally return `True`, so we can simplify the code by removing it. --- .../corebluetooth/PeripheralDelegate.py | 16 ++----- bleak/backends/corebluetooth/client.py | 44 ++++--------------- 2 files changed, 12 insertions(+), 48 deletions(-) diff --git a/bleak/backends/corebluetooth/PeripheralDelegate.py b/bleak/backends/corebluetooth/PeripheralDelegate.py index 42b32397..42cb4069 100644 --- a/bleak/backends/corebluetooth/PeripheralDelegate.py +++ b/bleak/backends/corebluetooth/PeripheralDelegate.py @@ -161,7 +161,7 @@ async def write_characteristic( characteristic: CBCharacteristic, value: NSData, response: CBCharacteristicWriteType, - ) -> bool: + ) -> None: # in CoreBluetooth there is no indication of success or failure of # CBCharacteristicWriteWithoutResponse if response == CBCharacteristicWriteWithResponse: @@ -180,10 +180,8 @@ async def write_characteristic( value, characteristic, response ) - return True - @objc.python_method - async def write_descriptor(self, descriptor: CBDescriptor, value: NSData) -> bool: + async def write_descriptor(self, descriptor: CBDescriptor, value: NSData) -> None: future = self._event_loop.create_future() self._descriptor_write_futures[descriptor.handle()] = future @@ -193,12 +191,10 @@ async def write_descriptor(self, descriptor: CBDescriptor, value: NSData) -> boo finally: del self._descriptor_write_futures[descriptor.handle()] - return True - @objc.python_method async def start_notifications( self, characteristic: CBCharacteristic, callback: Callable[[str, Any], Any] - ) -> bool: + ) -> None: c_handle = characteristic.handle() if c_handle in self._characteristic_notify_callbacks: raise ValueError("Characteristic notifications already started") @@ -214,10 +210,8 @@ async def start_notifications( finally: del self._characteristic_notify_change_futures[c_handle] - return True - @objc.python_method - async def stop_notifications(self, characteristic: CBCharacteristic) -> bool: + async def stop_notifications(self, characteristic: CBCharacteristic) -> None: c_handle = characteristic.handle() if c_handle not in self._characteristic_notify_callbacks: raise ValueError("Characteristic notification never started") @@ -233,8 +227,6 @@ async def stop_notifications(self, characteristic: CBCharacteristic) -> bool: self._characteristic_notify_callbacks.pop(c_handle) - return True - @objc.python_method async def read_rssi(self) -> NSNumber: future = self._event_loop.create_future() diff --git a/bleak/backends/corebluetooth/client.py b/bleak/backends/corebluetooth/client.py index e113c04b..4431a373 100644 --- a/bleak/backends/corebluetooth/client.py +++ b/bleak/backends/corebluetooth/client.py @@ -234,7 +234,7 @@ async def read_gatt_char( self, char_specifier: Union[BleakGATTCharacteristic, int, str, uuid.UUID], use_cached=False, - **kwargs + **kwargs, ) -> bytearray: """Perform read operation on the specified GATT characteristic. @@ -316,23 +316,14 @@ async def write_gatt_char( raise BleakError("Characteristic {} was not found!".format(char_specifier)) value = NSData.alloc().initWithBytes_length_(data, len(data)) - success = await self._delegate.write_characteristic( + await self._delegate.write_characteristic( characteristic.obj, value, CBCharacteristicWriteWithResponse if response else CBCharacteristicWriteWithoutResponse, ) - if success: - logger.debug( - "Write Characteristic {0} : {1}".format(characteristic.uuid, data) - ) - else: - raise BleakError( - "Could not write value {0} to characteristic {1}: {2}".format( - data, characteristic.uuid, success - ) - ) + logger.debug(f"Write Characteristic {characteristic.uuid} : {data}") async def write_gatt_descriptor( self, handle: int, data: Union[bytes, bytearray, memoryview] @@ -349,21 +340,14 @@ async def write_gatt_descriptor( raise BleakError("Descriptor {} was not found!".format(handle)) value = NSData.alloc().initWithBytes_length_(data, len(data)) - success = await self._delegate.write_descriptor(descriptor.obj, value) - if success: - logger.debug("Write Descriptor {0} : {1}".format(handle, data)) - else: - raise BleakError( - "Could not write value {0} to descriptor {1}: {2}".format( - data, descriptor.uuid, success - ) - ) + await self._delegate.write_descriptor(descriptor.obj, value) + logger.debug("Write Descriptor {0} : {1}".format(handle, data)) async def start_notify( self, char_specifier: Union[BleakGATTCharacteristic, int, str, uuid.UUID], callback: Callable[[int, bytearray], None], - **kwargs + **kwargs, ) -> None: """Activate notifications/indications on a characteristic. @@ -398,15 +382,7 @@ def bleak_callback(s, d): if not characteristic: raise BleakError("Characteristic {0} not found!".format(char_specifier)) - success = await self._delegate.start_notifications( - characteristic.obj, bleak_callback - ) - if not success: - raise BleakError( - "Could not start notify on {0}: {1}".format( - characteristic.uuid, success - ) - ) + await self._delegate.start_notifications(characteristic.obj, bleak_callback) async def stop_notify( self, char_specifier: Union[BleakGATTCharacteristic, int, str, uuid.UUID] @@ -427,11 +403,7 @@ async def stop_notify( if not characteristic: raise BleakError("Characteristic {} not found!".format(char_specifier)) - success = await self._delegate.stop_notifications(characteristic.obj) - if not success: - raise BleakError( - "Could not stop notify on {0}: {1}".format(characteristic.uuid, success) - ) + await self._delegate.stop_notifications(characteristic.obj) async def get_rssi(self) -> int: """To get RSSI value in dBm of the connected Peripheral""" From eb7f42cdf29407bebe8141c4d9d8ea455499c0ca Mon Sep 17 00:00:00 2001 From: David Lechner Date: Sat, 23 Oct 2021 13:34:40 -0500 Subject: [PATCH 4/4] CoreBluetooth: only notify if not read pending The same delegate callback is used for notifications and reading characteristics. This rearranges the logic so that if there is a read request pending, it will complete the read, otherwise it will trigger a notification callback. Previously, reads would also trigger notification callbacks. --- CHANGELOG.rst | 2 ++ .../backends/corebluetooth/PeripheralDelegate.py | 15 +++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7cc5772a..25cf7329 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,8 @@ Fixed ----- * Fixed ``InvalidStateError`` in CoreBluetooth backend when read and notification of the same characteristic are used. Fixes #675. +* Fixed reading a characteristic on CoreBluetooth backend also triggers notification + callback. `0.13.0`_ (2021-10-20) diff --git a/bleak/backends/corebluetooth/PeripheralDelegate.py b/bleak/backends/corebluetooth/PeripheralDelegate.py index 42cb4069..74353883 100644 --- a/bleak/backends/corebluetooth/PeripheralDelegate.py +++ b/bleak/backends/corebluetooth/PeripheralDelegate.py @@ -348,14 +348,17 @@ def did_update_value_for_characteristic( ): c_handle = characteristic.handle() - if error is None: - notify_callback = self._characteristic_notify_callbacks.get(c_handle) - if notify_callback: - notify_callback(c_handle, bytearray(value)) - future = self._characteristic_read_futures.get(c_handle) + + # If there is no pending read request, then this must be a notification + # (the same delagate callback is used by both). if not future: - return # only expected on read + if error is None: + notify_callback = self._characteristic_notify_callbacks.get(c_handle) + if notify_callback: + notify_callback(c_handle, bytearray(value)) + return + if error is not None: exception = BleakError(f"Failed to read characteristic {c_handle}: {error}") future.set_exception(exception)