Review comments / tidyup

Add assertChipStackLockedByCurrentThread() to CB delegate methods.
Use assign instead of unsafe_unretained for non-retainable types.
Add comment to BLE Base UUID.

Author: ksperling-apple

src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm

@@ -26,9 +26,11 @@
 #include <controller/CHIPDeviceController.h>
 #include <lib/dnssd/platform/Dnssd.h>
 #include <platform/CHIPDeviceLayer.h>
+#include <platform/Darwin/BleUtils.h>
 
 using namespace chip::Dnssd;
 using namespace chip::DeviceLayer;
+using namespace chip::DeviceLayer::Internal;
 
 #if CONFIG_NETWORK_LAYER_BLE
 #include <platform/Darwin/BleScannerDelegate.h>
@@ -305,7 +307,7 @@ void OnBleScanAdd(BLE_CONNECTION_OBJECT connObj, const ChipBLEDeviceIdentificati
         result.discriminator = @(info.GetDeviceDiscriminator());
         result.commissioningMode = YES;
         result.params = chip::MakeOptional(chip::Controller::SetUpCodePairerParameters(connObj, false /* connected */));
-        result.peripheral = (__bridge CBPeripheral *) connObj; // avoid params holding a dangling pointer
+        result.peripheral = CBPeripheralFromBleConnObject(connObj); // avoid params holding a dangling pointer
 
         MATTER_LOG_METRIC(kMetricBLEDevicesAdded, ++mBLEDevicesAdded);
 

src/platform/Darwin/BleConnectionDelegateImpl.mm

@@ -64,13 +64,13 @@ @interface BleConnection : NSObject <CBCentralManagerDelegate, CBPeripheralDeleg
 @property (nonatomic, readonly, nullable) dispatch_source_t timer;
 @property (nonatomic, readonly) BleConnectionMode currentMode;
 @property (strong, nonatomic) NSMutableDictionary<CBPeripheral *, NSDictionary *> * cachedPeripherals;
-@property (unsafe_unretained, nonatomic) bool found;
-@property (unsafe_unretained, nonatomic) chip::SetupDiscriminator deviceDiscriminator;
-@property (unsafe_unretained, nonatomic) void * appState;
-@property (unsafe_unretained, nonatomic) BleConnectionDelegate::OnConnectionCompleteFunct onConnectionComplete;
-@property (unsafe_unretained, nonatomic) BleConnectionDelegate::OnConnectionErrorFunct onConnectionError;
-@property (unsafe_unretained, nonatomic) chip::DeviceLayer::BleScannerDelegate * scannerDelegate;
-@property (unsafe_unretained, nonatomic) chip::Ble::BleLayer * mBleLayer;
+@property (assign, nonatomic) bool found;
+@property (assign, nonatomic) chip::SetupDiscriminator deviceDiscriminator;
+@property (assign, nonatomic) void * appState;
+@property (assign, nonatomic) BleConnectionDelegate::OnConnectionCompleteFunct onConnectionComplete;
+@property (assign, nonatomic) BleConnectionDelegate::OnConnectionErrorFunct onConnectionError;
+@property (assign, nonatomic) chip::DeviceLayer::BleScannerDelegate * scannerDelegate;
+@property (assign, nonatomic) chip::Ble::BleLayer * mBleLayer;
 
 - (instancetype)initWithDelegate:(chip::DeviceLayer::BleScannerDelegate *)delegate prewarm:(bool)prewarm;
 - (instancetype)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator;
@@ -129,7 +129,7 @@ - (void)removePeripheralsFromCache;
             ChipLogProgress(Ble, "ConnectionDelegate NewConnection with conn obj: %p", connObj);
             CBPeripheral * peripheral = CBPeripheralFromBleConnObject(connObj);
 
-            // The BLE_CONNECTION_OBJECT represent a CBPeripheral object. In order for it to be valid the central
+            // The BLE_CONNECTION_OBJECT represents a CBPeripheral object. In order for it to be valid the central
             // manager needs to still be running.
             if (!ble || [ble isConnecting]) {
                 if (OnConnectionError) {
@@ -308,6 +308,7 @@ - (void)dispatchConnectionComplete:(CBPeripheral *)peripheral
 
 - (void)centralManagerDidUpdateState:(CBCentralManager *)central
 {
+    assertChipStackLockedByCurrentThread();
     MATTER_LOG_METRIC(kMetricBLECentralManagerState, static_cast<uint32_t>(central.state));
 
     switch (central.state) {
@@ -340,6 +341,8 @@ - (void)centralManager:(CBCentralManager *)central
         advertisementData:(NSDictionary *)advertisementData
                      RSSI:(NSNumber *)RSSI
 {
+    assertChipStackLockedByCurrentThread();
+
     NSData * serviceData = advertisementData[CBAdvertisementDataServiceDataKey][_chipServiceUUID];
     if (!serviceData) {
         return;
@@ -405,8 +408,10 @@ - (BOOL)checkDiscriminator:(uint16_t)discriminator
 
 - (void)centralManager:(CBCentralManager *)central didConnectPeripheral:(CBPeripheral *)peripheral
 {
+    assertChipStackLockedByCurrentThread();
     MATTER_LOG_METRIC_END(kMetricBLEConnectPeripheral);
     MATTER_LOG_METRIC_BEGIN(kMetricBLEDiscoveredServices);
+
     [peripheral setDelegate:self];
     [peripheral discoverServices:nil];
     [self stopScanning];
@@ -418,6 +423,8 @@ - (void)centralManager:(CBCentralManager *)central didConnectPeripheral:(CBPerip
 
 - (void)peripheral:(CBPeripheral *)peripheral didDiscoverServices:(NSError *)error
 {
+    assertChipStackLockedByCurrentThread();
+
     if (nil != error) {
         ChipLogError(Ble, "BLE:Error finding Chip Service in the device: [%s]", [error.localizedDescription UTF8String]);
     }
@@ -442,6 +449,7 @@ - (void)peripheral:(CBPeripheral *)peripheral didDiscoverServices:(NSError *)err
 
 - (void)peripheral:(CBPeripheral *)peripheral didDiscoverCharacteristicsForService:(CBService *)service error:(NSError *)error
 {
+    assertChipStackLockedByCurrentThread();
     MATTER_LOG_METRIC_END(kMetricBLEDiscoveredCharacteristics, CHIP_ERROR(chip::ChipError::Range::kOS, static_cast<uint32_t>(error.code)));
 
     if (nil != error) {
@@ -457,6 +465,8 @@ - (void)peripheral:(CBPeripheral *)peripheral
     didWriteValueForCharacteristic:(CBCharacteristic *)characteristic
                              error:(NSError *)error
 {
+    assertChipStackLockedByCurrentThread();
+
     if (nil == error) {
         ChipBleUUID svcId = BleUUIDFromCBUUD(characteristic.service.UUID);
         ChipBleUUID charId = BleUUIDFromCBUUD(characteristic.UUID);
@@ -473,6 +483,8 @@ - (void)peripheral:(CBPeripheral *)peripheral
     didUpdateNotificationStateForCharacteristic:(CBCharacteristic *)characteristic
                                           error:(NSError *)error
 {
+    assertChipStackLockedByCurrentThread();
+
     bool isNotifying = characteristic.isNotifying;
 
     if (nil == error) {
@@ -503,6 +515,8 @@ - (void)peripheral:(CBPeripheral *)peripheral
     didUpdateValueForCharacteristic:(CBCharacteristic *)characteristic
                               error:(NSError *)error
 {
+    assertChipStackLockedByCurrentThread();
+
     if (nil == error) {
         ChipBleUUID svcId = BleUUIDFromCBUUD(characteristic.service.UUID);
         ChipBleUUID charId = BleUUIDFromCBUUD(characteristic.UUID);
@@ -550,28 +564,19 @@ - (void)stop
     [self stopScanning];
     [self removePeripheralsFromCache];
 
-    if (!_centralManager && !_peripheral) {
-        return;
+    if (_peripheral) {
+        // Close all BLE connections before we release CB objects
+        _mBleLayer->CloseAllBleConnections();
+        _peripheral = nil;
     }
 
-    // Properly closing the underlying ble connections needs to happens
-    // on the chip work queue. At the same time the SDK is trying to
-    // properly unsubscribe and shutdown the connection, so if we nullify
-    // the centralManager and the peripheral members too early it won't be
-    // able to reach those.
-    // This is why closing connections happens as 2 async steps.
-    {
-        if (_peripheral) {
-            // Close all BLE connections before we release CB objects
-            _mBleLayer->CloseAllBleConnections();
-        }
-
+    if (_centralManager) {
         _centralManager.delegate = nil;
         _centralManager = nil;
-        _peripheral = nil;
-        if (chip::DeviceLayer::Internal::ble == self) {
-            chip::DeviceLayer::Internal::ble = nil;
-        }
+    }
+
+    if (chip::DeviceLayer::Internal::ble == self) {
+        chip::DeviceLayer::Internal::ble = nil;
     }
 }
 

src/platform/Darwin/BlePeripheral.h

@@ -0,0 +1,45 @@
+/*
+ *
+ *    Copyright (c) 2025 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#if !__has_feature(objc_arc)
+#error This file must be compiled in ObjC++ mode with ARC.
+#endif
+
+#include <ble/Ble.h>
+
+#import <CoreBluetooth/CoreBluetooth.h>
+
+namespace chip {
+namespace DeviceLayer {
+namespace Internal {
+
+// Holds a CBPeripheral and the associated CBCentralManager
+struct BlePeripheral
+{
+    CBPeripheral * const peripheral;
+    CBCentralManager * const centralManager;
+
+    BlePeripheral(CBPeripheral * aPeripheral, CBCentralManager * aCentralManager) :
+        peripheral(aPeripheral), centralManager(aCentralManager)
+    {}
+};
+
+} // namespace Internal
+} // namespace DeviceLayer
+} // namespace chip

src/platform/Darwin/BleUtils.mm

@@ -35,6 +35,7 @@ ChipBleUUID BleUUIDFromCBUUD(CBUUID * uuid)
             // CBUUID handles the expansion to 128 bit automatically internally,
             // but doesn't expose the expanded UUID in the `data` property, so
             // we have to re-implement the expansion here.
+            // The Base UUID 00000000-0000-1000-8000-00805F9B34FB is defined in the BLE spec.
             constexpr ChipBleUUID baseUuid = { { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB } };
             NSData * uuidData = uuid.data;
             switch (uuidData.length) {