Skip to content

Commit

Permalink
Peripheral discovery as async operations (#46)
Browse files Browse the repository at this point in the history
# Peripheral discovery as async operations

## ♻️ Current situation & Problem
While SpeziBluetooth exposes all Peripheral interactions as
async/await-based operations, embracing Swift Concurrency, it doesn't
always do that for internal operations. For example, the initial
discovering phase does not use async/await and instead performs checks
within the delegate method implementations. This has the result, that
code is relatively hard to read as state checking is scattered around
the implementation (e.g., we check in several places if we can call
`signalFullyDiscovered()`).
This PR improves code readability and state management by making all
peripheral interactions relying on async/await-based operations.
Operations like service and characteristic discovery can now be
performed via an async call. Further, enabling notifications can be
performed using an async method call. As a result, all code relating to
service discovery is now placed in one async method. Potential errors
can be easily propagated back to the `connect` continuation making it
possible to properly communicate if a connecting attempt failed (e.g.,
because we weren't able to subscribe to a characteristic after several
tries).

Further, this PR changes the point in time when we consider a device
fully connected. While we are still subscribing to characteristic
notifications, the device will still report `connecting` state. This
makes it easier for library users to determine when a device is fully
ready.

## ⚙️ Release Notes 
* Internal restructure of BluetoothPeripheral resulting in better
maintainable code.
* Errors from the service discovery are now reported back to the
`connect()` method call and errors are thrown into the continuation. An
unsuccessful discovery will result in the device being disconnected.
* Device is considered `connecting` while characteristic notifications
are enabled.
* Fixed an issue were the onChange handler would not be called if the
`initial` parameter was set to `false` if the characteristic is
notify-only or an error occurred in the initial read.
* `DiscoveryCriteria` now supports specifying multiple advertised
services.
* Fixed a potential race condition when enabling notifications for a
`@Characteristic` before the peripheral was injected.
* `CurrentTimeService/synchronizeDeviceTime(now:threshold:)` is now
`async throws` allowing to inspect potential errors.

## 📚 Documentation
Documentation was updated to reflect changes in this PR.

## ✅ Testing
Existing testing infrastructure was used to validate correctness.

## 📝 Code of Conduct & Contributing Guidelines 

By submitting creating this pull request, you agree to follow our [Code
of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md):
- [x] I agree to follow the [Code of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md).
  • Loading branch information
Supereg authored Aug 16, 2024
1 parent 5e78aed commit 361acec
Show file tree
Hide file tree
Showing 45 changed files with 1,198 additions and 489 deletions.
9 changes: 5 additions & 4 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ let package = Package(
.library(name: "SpeziBluetooth", targets: ["SpeziBluetooth"])
],
dependencies: [
.package(url: "https://github.com/StanfordSpezi/SpeziFoundation", from: "1.1.0"),
.package(url: "https://github.com/StanfordSpezi/Spezi", from: "1.6.0"),
.package(url: "https://github.com/StanfordSpezi/SpeziFoundation", from: "2.0.0-beta.1"),
.package(url: "https://github.com/StanfordSpezi/Spezi", from: "1.7.1"),
.package(url: "https://github.com/StanfordSpezi/SpeziNetworking", from: "2.1.0"),
.package(url: "https://github.com/apple/swift-nio.git", from: "2.59.0"),
.package(url: "https://github.com/apple/swift-collections.git", from: "1.0.4"),
Expand All @@ -55,7 +55,8 @@ let package = Package(
.process("Resources")
],
swiftSettings: [
swiftConcurrency
swiftConcurrency,
.enableUpcomingFeature("InferSendableFromCaptures")
],
plugins: [] + swiftLintPlugin()
),
Expand Down Expand Up @@ -123,7 +124,7 @@ func swiftLintPlugin() -> [Target.PluginUsage] {

func swiftLintPackage() -> [PackageDescription.Package.Dependency] {
if ProcessInfo.processInfo.environment["SPEZI_DEVELOPMENT_SWIFTLINT"] != nil {
[.package(url: "https://github.com/realm/SwiftLint.git", .upToNextMinor(from: "0.55.1"))]
[.package(url: "https://github.com/realm/SwiftLint.git", from: "0.55.1")]
} else {
[]
}
Expand Down
8 changes: 5 additions & 3 deletions Sources/SpeziBluetooth/Bluetooth.swift
Original file line number Diff line number Diff line change
Expand Up @@ -293,10 +293,10 @@ public final class Bluetooth: Module, EnvironmentAccessible, Sendable {
/// Devices might be part of `nearbyDevices` as well or just retrieved devices that are eventually connected.
/// Values are stored weakly. All properties (like `@Characteristic`, `@DeviceState` or `@DeviceAction`) store a reference to `Bluetooth` and report once they are de-initialized
/// to clear the respective initialized devices from this dictionary.
@SpeziBluetooth private var initializedDevices: OrderedDictionary<UUID, AnyWeakDeviceReference> = [:]
private var initializedDevices: OrderedDictionary<UUID, AnyWeakDeviceReference> = [:]

@Application(\.spezi)
@MainActor private var spezi
private var spezi

private nonisolated var logger: Logger {
Self.logger
Expand Down Expand Up @@ -389,7 +389,7 @@ public final class Bluetooth: Module, EnvironmentAccessible, Sendable {
} else {
let advertisementData = entry.value.advertisementData
guard let configuration = configuration.find(for: advertisementData, logger: logger) else {
logger.warning("Ignoring peripheral \(entry.value.debugDescription) that cannot be mapped to a device class.")
logger.warning("Ignoring peripheral \(entry.value) that cannot be mapped to a device class.")
return
}

Expand All @@ -402,6 +402,7 @@ public final class Bluetooth: Module, EnvironmentAccessible, Sendable {
}


let spezi = spezi
Task { @MainActor [newlyPreparedDevices] in
var checkForConnected = false

Expand Down Expand Up @@ -548,6 +549,7 @@ public final class Bluetooth: Module, EnvironmentAccessible, Sendable {
let device = prepareDevice(id: uuid, Device.self, peripheral: peripheral)
// We load the module with external ownership. Meaning, Spezi won't keep any strong references to the Module and deallocation of
// the module is possible, freeing all Spezi related resources.
let spezi = spezi
await spezi.loadModule(device, ownership: .external)

// The semantics of retrievePeripheral is as follows: it returns a BluetoothPeripheral that is weakly allocated by the BluetoothManager.´
Expand Down
3 changes: 3 additions & 0 deletions Sources/SpeziBluetooth/Configuration/Discover.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,6 @@ public struct Discover<Device: BluetoothDevice> {
self.discoveryCriteria = discoveryCriteria
}
}


extension Discover: Sendable {}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
public enum DiscoveryDescriptorBuilder {
/// Build a ``Discover`` expression to define a ``DeviceDiscoveryDescriptor``.
public static func buildExpression<Device: BluetoothDevice>(_ expression: Discover<Device>) -> Set<DeviceDiscoveryDescriptor> {
[DeviceDiscoveryDescriptor(discoveryCriteria: expression.discoveryCriteria, deviceType: expression.deviceType)]
[DeviceDiscoveryDescriptor(discoveryCriteria: expression.discoveryCriteria, deviceType: Device.self)]
}

/// Build a block of ``DeviceDiscoveryDescriptor``s.
Expand Down
37 changes: 19 additions & 18 deletions Sources/SpeziBluetooth/CoreBluetooth/BluetoothManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
// SPDX-License-Identifier: MIT
//

@preconcurrency import class CoreBluetooth.CBCentralManager // swiftlint:disable:this duplicate_imports
import CoreBluetooth
import NIO
import Observation
Expand Down Expand Up @@ -84,15 +83,15 @@ import OSLog
public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint:disable:this type_body_length
private let logger = Logger(subsystem: "edu.stanford.spezi.bluetooth", category: "BluetoothManager")

private var _centralManager: CBCentralManager?
private var _centralManager: CBInstance<CBCentralManager>?

private var centralManager: CBCentralManager {
guard let centralManager = _centralManager else {
let centralManager = supplyCBCentral()
self._centralManager = centralManager
self._centralManager = CBInstance(instantiatedOnDispatchQueue: centralManager)
return centralManager
}
return centralManager
return centralManager.cbObject
}

private lazy var centralDelegate: Delegate = { // swiftlint:disable:this weak_delegate
Expand Down Expand Up @@ -472,7 +471,7 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
}

func connect(peripheral: BluetoothPeripheral) {
logger.debug("Trying to connect to \(peripheral.debugDescription) ...")
logger.debug("Trying to connect to \(peripheral) ...")

let cancelled = discoverySession?.cancelStaleTask(for: peripheral)

Expand All @@ -484,18 +483,18 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
}

func disconnect(peripheral: BluetoothPeripheral) {
logger.debug("Disconnecting peripheral \(peripheral.debugDescription) ...")
logger.debug("Disconnecting peripheral \(peripheral) ...")
// stale timer is handled in the delegate method
centralManager.cancelPeripheralConnection(peripheral.cbPeripheral)

discoverySession?.deviceManuallyDisconnected(id: peripheral.id)
}

private func handledConnected(device: BluetoothPeripheral) {
device.handleConnect()

private func handledConnected(device: BluetoothPeripheral) async {
// we might have connected a bluetooth peripheral that was weakly referenced
ensurePeripheralReference(device)

await device.handleConnect()
}

private func discardDevice(device: BluetoothPeripheral, error: Error?) {
Expand All @@ -522,7 +521,7 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
Task { @SpeziBluetooth [storage, _centralManager, isScanning, logger] in
if isScanning {
storage.isScanning = false
_centralManager?.stopScan()
_centralManager?.cbObject.stopScan()
logger.debug("Scanning stopped")

Check warning on line 525 in Sources/SpeziBluetooth/CoreBluetooth/BluetoothManager.swift

View workflow job for this annotation

GitHub Actions / Build and Test macOS / Test using xcodebuild or run fastlane

capture of 'logger' with non-sendable type 'Logger' in a `@Sendable` closure
}

Expand Down Expand Up @@ -575,6 +574,8 @@ extension BluetoothManager {
static let defaultMinimumRSSI = -80
/// The default time in seconds after which we check for auto connectable devices after the initial advertisement.
static let defaultAutoConnectDebounce: Int = 1
/// The amount of times we try to automatically (if enabled) subscribe to a notify characteristic.
static let autoSubscribeAttempts = 3
}
}

Expand Down Expand Up @@ -673,7 +674,7 @@ extension BluetoothManager {
return
}

logger.debug("Discovered peripheral \(peripheral.debugIdentifier) at \(rssi.intValue) dB (data: \(String(describing: data))")
logger.debug("Discovered peripheral \(peripheral.debugIdentifier) at \(rssi.intValue) dB with data \(data)")

Check warning on line 677 in Sources/SpeziBluetooth/CoreBluetooth/BluetoothManager.swift

View workflow job for this annotation

GitHub Actions / Build and Test macOS / Test using xcodebuild or run fastlane

capture of 'logger' with non-sendable type 'Logger' in a `@Sendable` closure

let descriptor = session.configuredDevices.find(for: data, logger: logger)

Expand Down Expand Up @@ -705,10 +706,10 @@ extension BluetoothManager {
return
}

logger.debug("Peripheral \(peripheral.debugIdentifier) connected.")
manager.handledConnected(device: device)

logger.debug("Peripheral \(device) connected.")
await manager.storage.cbDelegateSignal(connected: true, for: peripheral.identifier)

await manager.handledConnected(device: device)
}
}

Expand All @@ -730,9 +731,9 @@ extension BluetoothManager {
}

if let error {
logger.error("Failed to connect to \(peripheral.debugDescription): \(error)")
logger.error("Failed to connect to \(device): \(error)")
} else {
logger.error("Failed to connect to \(peripheral.debugDescription)")
logger.error("Failed to connect to \(device)")
}

// just to make sure
Expand All @@ -756,9 +757,9 @@ extension BluetoothManager {
}

if let error {
logger.debug("Peripheral \(peripheral.debugIdentifier) disconnected due to an error: \(error)")
logger.debug("Peripheral \(device) disconnected due to an error: \(error)")
} else {
logger.debug("Peripheral \(peripheral.debugIdentifier) disconnected.")
logger.debug("Peripheral \(device) disconnected.")
}

manager.discardDevice(device: device, error: error)
Expand Down
Loading

0 comments on commit 361acec

Please sign in to comment.