Skip to content

Commit

Permalink
Darwin: Bug fixes in BleConnectionDelegateImpl (#29059)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
ksperling-apple authored and pull[bot] committed Oct 12, 2023
1 parent 5340fec commit 3560666
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 11 deletions.
3 changes: 3 additions & 0 deletions src/platform/Darwin/BleConnectionDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 20 additions & 11 deletions src/platform/Darwin/BleConnectionDelegateImpl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
Expand All @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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];
});
Expand Down Expand Up @@ -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]]];
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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;
}
});
});
}
Expand Down

0 comments on commit 3560666

Please sign in to comment.