Darwin: Bug fixes in BleConnectionDelegateImpl (#29059)

* Avoid clearing the `ble` global if it points to a different instance. * Capture the CBPeripheral strong ref instead of the void * in dispatches. * Also read characteristic value before dispatching and improve logging.
project-chip · Oct 12, 2023 · 3560666 · 3560666
1 parent 5340fec
commit 3560666
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 11 deletions.
diff --git a/src/platform/Darwin/BleConnectionDelegate.h b/src/platform/Darwin/BleConnectionDelegate.h
@@ -31,6 +31,9 @@ class BleConnectionDelegateImpl : public Ble::BleConnectionDelegate
     virtual void NewConnection(Ble::BleLayer * bleLayer, void * appState, const SetupDiscriminator & connDiscriminator);
     virtual void NewConnection(Ble::BleLayer * bleLayer, void * appState, BLE_CONNECTION_OBJECT connObj);
     virtual CHIP_ERROR CancelConnection();
+
+private:
+    CHIP_ERROR DoCancel();
 };
 
 } // namespace Internal

diff --git a/src/platform/Darwin/BleConnectionDelegateImpl.mm b/src/platform/Darwin/BleConnectionDelegateImpl.mm
@@ -103,7 +103,7 @@ - (void)removePeripheralsFromCache;
             // Make a copy of the device discriminator for the block to capture.
             SetupDiscriminator deviceDiscriminator = inDeviceDiscriminator;
 
-            ChipLogProgress(Ble, "%s", __FUNCTION__);
+            ChipLogProgress(Ble, "ConnectionDelegate NewConnection with discriminator");
             if (!bleWorkQueue) {
                 bleWorkQueue = dispatch_queue_create(kBleWorkQueueName, DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
             }
@@ -134,12 +134,13 @@ - (void)removePeripheralsFromCache;
         {
             assertChipStackLockedByCurrentThread();
 
-            ChipLogProgress(Ble, "%s", __FUNCTION__);
+            ChipLogProgress(Ble, "ConnectionDelegate NewConnection with conn obj: %p", connObj);
 
             if (!bleWorkQueue) {
                 bleWorkQueue = dispatch_queue_create(kBleWorkQueueName, DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
             }
 
+            CBPeripheral * peripheral = (__bridge CBPeripheral *) connObj; // bridge (and retain) before dispatching
             dispatch_async(bleWorkQueue, ^{
                 // The BLE_CONNECTION_OBJECT represent a CBPeripheral object. In order for it to be valid the central
                 // manager needs to still be running.
@@ -157,15 +158,15 @@ - (void)removePeripheralsFromCache;
                 ble.appState = appState;
                 ble.onConnectionComplete = OnConnectionComplete;
                 ble.onConnectionError = OnConnectionError;
-                [ble updateWithPeripheral:(__bridge CBPeripheral *) connObj];
+                [ble updateWithPeripheral:peripheral];
             });
         }
 
         void BleConnectionDelegateImpl::StartScan(BleScannerDelegate * delegate)
         {
             assertChipStackLockedByCurrentThread();
 
-            ChipLogProgress(Ble, "%s", __FUNCTION__);
+            ChipLogProgress(Ble, "ConnectionDelegate StartScan%s", (delegate ? " with delegate" : ""));
 
             if (!bleWorkQueue) {
                 bleWorkQueue = dispatch_queue_create(kBleWorkQueueName, DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
@@ -191,15 +192,19 @@ - (void)removePeripheralsFromCache;
 
         void BleConnectionDelegateImpl::StopScan()
         {
-            assertChipStackLockedByCurrentThread();
-            CancelConnection();
+            ChipLogProgress(Ble, "ConnectionDelegate StopScan");
+            DoCancel();
         }
 
         CHIP_ERROR BleConnectionDelegateImpl::CancelConnection()
         {
-            assertChipStackLockedByCurrentThread();
+            ChipLogProgress(Ble, "ConnectionDelegate CancelConnection");
+            return DoCancel();
+        }
 
-            ChipLogProgress(Ble, "%s", __FUNCTION__);
+        CHIP_ERROR BleConnectionDelegateImpl::DoCancel()
+        {
+            assertChipStackLockedByCurrentThread();
             if (bleWorkQueue == nil) {
                 return CHIP_NO_ERROR;
             }
@@ -284,6 +289,7 @@ - (void)setupTimer:(uint64_t)timeout
 
     _timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, _workQueue);
     dispatch_source_set_event_handler(_timer, ^{
+        ChipLogProgress(Ble, "ConnectionDelegate timeout");
         [self stop];
         [self dispatchConnectionError:BLE_ERROR_APP_CLOSED_CONNECTION];
     });
@@ -374,7 +380,7 @@ - (void)centralManager:(CBCentralManager *)central
     }
 
     const uint8_t * bytes = (const uint8_t *) [serviceData bytes];
-    if ([serviceData length] != 8) {
+    if ([serviceData length] != sizeof(ChipBLEDeviceIdentificationInfo)) {
         NSMutableString * hexString = [NSMutableString stringWithCapacity:([serviceData length] * 2)];
         for (NSUInteger i = 0; i < [serviceData length]; i++) {
             [hexString appendString:[NSString stringWithFormat:@"%02lx", (unsigned long) bytes[i]]];
@@ -521,10 +527,11 @@ - (void)peripheral:(CBPeripheral *)peripheral
         chip::Ble::ChipBleUUID svcId;
         chip::Ble::ChipBleUUID charId;
         [BleConnection fillServiceWithCharacteristicUuids:characteristic svcId:&svcId charId:&charId];
+        auto * value = characteristic.value; // read immediately before dispatching
 
         dispatch_async(_chipWorkQueue, ^{
             // build a inet buffer from the rxEv and send to blelayer.
-            auto msgBuf = chip::System::PacketBufferHandle::NewWithData(characteristic.value.bytes, characteristic.value.length);
+            auto msgBuf = chip::System::PacketBufferHandle::NewWithData(value.bytes, value.length);
 
             if (msgBuf.IsNull()) {
                 ChipLogError(Ble, "Failed at allocating buffer for incoming BLE data");
@@ -583,7 +590,9 @@ - (void)stop
             _centralManager.delegate = nil;
             _centralManager = nil;
             _peripheral = nil;
-            chip::DeviceLayer::Internal::ble = nil;
+            if (chip::DeviceLayer::Internal::ble == self) {
+                chip::DeviceLayer::Internal::ble = nil;
+            }
         });
     });
 }