Improve how we persist the last manually disconnected device (#51)

# Improve how we persist the last manually disconnected device ## ♻️ Current situation & Problem The feature that prevents the last known device from getting reconnected currently doesn't work properly. This PR fixes that to ensure that the last manually disconnected device won't be automatically connected again. With fully enabling Swift 6, we discovered that SwiftUI assumes that mutations are executed on the MainActor. Therefore, this PR needed to restructures how mutations are notified using Observation and all mutations are now shadowed on the MainActor for improved compatibility with SwiftUI. ## 📝 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). --------- Co-authored-by: Paul Schmiedmayer <[email protected]>
StanfordSpezi · Nov 10, 2024 · 4d64dfa · 4d64dfa
1 parent 977cc67
commit 4d64dfa
Show file tree

Hide file tree

Showing 19 changed files with 306 additions and 192 deletions.
diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml
@@ -24,16 +24,6 @@ jobs:
       scheme: SpeziBluetooth-Package
       artifactname: SpeziBluetooth-Package.xcresult
       resultBundle: SpeziBluetooth-Package.xcresult
-  packageios_latest:
-    name: Build and Test Swift Package iOS Latest
-    uses: StanfordSpezi/.github/.github/workflows/xcodebuild-or-fastlane.yml@v2
-    with:
-      runsonlabels: '["macOS", "self-hosted", "spezi"]'
-      scheme: SpeziBluetooth-Package
-      xcodeversion: latest
-      swiftVersion: 6
-      artifactname: SpeziBluetooth-Package-Latest.xcresult
-      resultBundle: SpeziBluetooth-Package-Latest.xcresult
   ios:
     name: Build and Test iOS
     uses: StanfordSpezi/.github/.github/workflows/xcodebuild-or-fastlane.yml@v2
@@ -43,26 +33,15 @@ jobs:
       scheme: TestApp
       artifactname: TestApp-iOS.xcresult
       resultBundle: TestApp-iOS.xcresult
-  ios_latest:
-    name: Build and Test iOS Latest
-    uses: StanfordSpezi/.github/.github/workflows/xcodebuild-or-fastlane.yml@v2
-    with:
-      runsonlabels: '["macOS", "self-hosted", "spezi"]'
-      path: 'Tests/UITests'
-      scheme: TestApp
-      xcodeversion: latest
-      swiftVersion: 6
-      artifactname: TestApp-iOS-Latest.xcresult
-      resultBundle: TestApp-iOS-Latest.xcresult
   macos:
     name: Build and Test macOS
     uses: StanfordSpezi/.github/.github/workflows/xcodebuild-or-fastlane.yml@v2
     permissions:
       contents: read
     with:
       runsonlabels: '["macOS", "self-hosted", "bluetooth"]'
-      setupsigning: true
       path: 'Tests/UITests'
+      setupsigning: true
       artifactname: TestApp-macOS.xcresult
       resultBundle: TestApp-macOS.xcresult
       customcommand: "set -o pipefail && xcodebuild test -scheme 'TestApp' -configuration 'Test' -destination 'platform=macOS,arch=arm64,variant=Mac Catalyst' -derivedDataPath '.derivedData' -resultBundlePath 'TestApp-macOS.xcresult' -skipPackagePluginValidation -skipMacroValidation | xcbeautify"

diff --git a/Package.swift b/Package.swift
@@ -1,4 +1,4 @@
-// swift-tools-version:5.9
+// swift-tools-version:6.0
 
 //
 // This source file is part of the Stanford Spezi open source project
@@ -12,13 +12,6 @@ import class Foundation.ProcessInfo
 import PackageDescription
 
 
-#if swift(<6)
-let swiftConcurrency: SwiftSetting = .enableExperimentalFeature("StrictConcurrency")
-#else
-let swiftConcurrency: SwiftSetting = .enableUpcomingFeature("StrictConcurrency")
-#endif
-
-
 let package = Package(
     name: "SpeziBluetooth",
     defaultLocalization: "en",
@@ -54,10 +47,6 @@ let package = Package(
             resources: [
                 .process("Resources")
             ],
-            swiftSettings: [
-                swiftConcurrency,
-                .enableUpcomingFeature("InferSendableFromCaptures")
-            ],
             plugins: [] + swiftLintPlugin()
         ),
         .target(
@@ -67,9 +56,6 @@ let package = Package(
                 .product(name: "ByteCoding", package: "SpeziNetworking"),
                 .product(name: "SpeziNumerics", package: "SpeziNetworking")
             ],
-            swiftSettings: [
-                swiftConcurrency
-            ],
             plugins: [] + swiftLintPlugin()
         ),
         .executableTarget(
@@ -79,9 +65,6 @@ let package = Package(
                 .target(name: "SpeziBluetoothServices"),
                 .product(name: "ByteCoding", package: "SpeziNetworking")
             ],
-            swiftSettings: [
-                swiftConcurrency
-            ],
             plugins: [] + swiftLintPlugin()
         ),
         .testTarget(
@@ -90,9 +73,6 @@ let package = Package(
                 .target(name: "SpeziBluetooth"),
                 .target(name: "SpeziBluetoothServices")
             ],
-            swiftSettings: [
-                swiftConcurrency
-            ],
             plugins: [] + swiftLintPlugin()
         ),
         .testTarget(
@@ -104,9 +84,6 @@ let package = Package(
                 .product(name: "NIO", package: "swift-nio"),
                 .product(name: "XCTestExtensions", package: "XCTestExtensions")
             ],
-            swiftSettings: [
-                swiftConcurrency
-            ],
             plugins: [] + swiftLintPlugin()
         )
     ]

diff --git a/Sources/SpeziBluetooth/Bluetooth.swift b/Sources/SpeziBluetooth/Bluetooth.swift
@@ -234,22 +234,24 @@ import Spezi
 public final class Bluetooth: Module, EnvironmentAccessible, Sendable {
     @Observable
     class Storage {
-        var nearbyDevices: OrderedDictionary<UUID, any BluetoothDevice> = [:]
+        @MainActor var nearbyDevices: OrderedDictionary<UUID, any BluetoothDevice> = [:]
+
+        nonisolated init() {}
     }
 
     nonisolated static let logger = Logger(subsystem: "edu.stanford.spezi.bluetooth", category: "Bluetooth")
 
-    @SpeziBluetooth private let bluetoothManager = BluetoothManager()
+    private let bluetoothManager = BluetoothManager()
 
     /// The Bluetooth device configuration.
     ///
     /// Set of configured ``BluetoothDevice`` with their corresponding ``DiscoveryCriteria``.
     public nonisolated let configuration: Set<DeviceDiscoveryDescriptor>
 
     // sadly Swifts "lazy var" won't work here with strict concurrency as it doesn't isolate the underlying lazy storage
-    @SpeziBluetooth private var _lazy_discoveryConfiguration: Set<DiscoveryDescription>?
+    private var _lazy_discoveryConfiguration: Set<DiscoveryDescription>?
     // swiftlint:disable:previous discouraged_optional_collection identifier_name
-    @SpeziBluetooth private var discoveryConfiguration: Set<DiscoveryDescription> {
+    private var discoveryConfiguration: Set<DiscoveryDescription> {
         guard let discoveryConfiguration = _lazy_discoveryConfiguration else {
             let discovery = configuration.parseDiscoveryDescription()
             self._lazy_discoveryConfiguration = discovery
@@ -304,8 +306,10 @@ public final class Bluetooth: Module, EnvironmentAccessible, Sendable {
 
     /// Stores the connected device instance for every configured ``BluetoothDevice`` type.
     @Model @MainActor private var connectedDevicesModel = ConnectedDevicesModel()
+
+    // we need to manually declare the synthesized property wrapper to avoid https://github.com/swiftlang/swift/issues/76005#issuecomment-2466703851
     /// Injects the ``BluetoothDevice`` instances from the `ConnectedDevices` model into the SwiftUI environment.
-    @Modifier @MainActor private var devicesInjector: ConnectedDevicesEnvironmentModifier
+    @MainActor private var _devicesInjector: Modifier<ConnectedDevicesEnvironmentModifier>
 
 
     /// Configure the Bluetooth Module.
@@ -322,13 +326,12 @@ public final class Bluetooth: Module, EnvironmentAccessible, Sendable {
     /// - Parameter devices: The set of configured devices.
     @MainActor
     public init(
-        @DiscoveryDescriptorBuilder _ devices: @Sendable () -> Set<DeviceDiscoveryDescriptor>
+        @DiscoveryDescriptorBuilder _ devices: () -> Set<DeviceDiscoveryDescriptor>
     ) {
         let configuration = devices()
-        let deviceTypes = configuration.deviceTypes
 
         self.configuration = configuration
-        self.devicesInjector = ConnectedDevicesEnvironmentModifier(configuredDeviceTypes: deviceTypes)
+        self._devicesInjector = Modifier(wrappedValue: ConnectedDevicesEnvironmentModifier(from: configuration))
 
         Task { @SpeziBluetooth in
             self.observeDiscoveredDevices()

diff --git a/Sources/SpeziBluetooth/CoreBluetooth/BluetoothManager.swift b/Sources/SpeziBluetooth/CoreBluetooth/BluetoothManager.swift
@@ -109,6 +109,9 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
 
     /// Currently ongoing discovery session.
     private var discoverySession: DiscoverySession?
+    /// The identifier of the last manually disconnected device.
+    /// This is to avoid automatically reconnecting to a device that was manually disconnected.
+    private(set) var lastManuallyDisconnectedDevice: UUID?
 
     /// The list of nearby bluetooth devices.
     ///
@@ -197,6 +200,7 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
     func cleanupCBCentral() {
         _centralManager = nil
         isScanningObserver = nil
+        lastManuallyDisconnectedDevice = nil
         logger.debug("Destroyed the underlying CBCentralManager.")
     }
 
@@ -407,8 +411,6 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
 
         storage.discoveredPeripherals.removeValue(forKey: id)
 
-        discoverySession?.clearManuallyDisconnectedDevice(for: id)
-
         checkForCentralDeinit()
     }
 
@@ -443,8 +445,6 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
     func handlePeripheralDeinit(id uuid: UUID) {
         storage.retrievedPeripherals.removeValue(forKey: uuid)
 
-        discoverySession?.clearManuallyDisconnectedDevice(for: uuid)
-
         checkForCentralDeinit()
     }
 
@@ -488,7 +488,7 @@ public class BluetoothManager: Observable, Sendable, Identifiable { // swiftlint
         // stale timer is handled in the delegate method
         centralManager.cancelPeripheralConnection(peripheral.cbPeripheral)
 
-        discoverySession?.deviceManuallyDisconnected(id: peripheral.id)
+        lastManuallyDisconnectedDevice = peripheral.id
     }
 
     private func handledConnected(device: BluetoothPeripheral) async {

diff --git a/Sources/SpeziBluetooth/CoreBluetooth/Model/BluetoothManagerStorage.swift b/Sources/SpeziBluetooth/CoreBluetooth/Model/BluetoothManagerStorage.swift
@@ -13,10 +13,10 @@ import OrderedCollections
 
 @Observable
 final class BluetoothManagerStorage: ValueObservable, Sendable {
-    private let _isScanning = ManagedAtomic<Bool>(false)
-    private let _state = ManagedAtomic<BluetoothState>(.unknown)
+    private let _isScanning = ManagedAtomicMainActorBuffered<Bool>(false)
+    private let _state = ManagedAtomicMainActorBuffered<BluetoothState>(.unknown)
 
-    @ObservationIgnored private nonisolated(unsafe) var _discoveredPeripherals: OrderedDictionary<UUID, BluetoothPeripheral> = [:]
+    private let _discoveredPeripherals: MainActorBuffered<OrderedDictionary<UUID, BluetoothPeripheral>> = .init([:])
     private let rwLock = RWLock()
 
     @SpeziBluetooth var retrievedPeripherals: OrderedDictionary<UUID, WeakReference<BluetoothPeripheral>> = [:] {
@@ -46,20 +46,21 @@ final class BluetoothManagerStorage: ValueObservable, Sendable {
 
     @inlinable var readOnlyDiscoveredPeripherals: OrderedDictionary<UUID, BluetoothPeripheral> {
         access(keyPath: \._discoveredPeripherals)
-        return rwLock.withReadLock {
-            _discoveredPeripherals
-        }
+        return _discoveredPeripherals.load(using: rwLock)
     }
 
     @SpeziBluetooth var state: BluetoothState {
         get {
             readOnlyState
         }
         set {
-            withMutation(keyPath: \._state) {
-                _state.store(newValue, ordering: .relaxed)
+            let didChange = _state.storeAndCompare(newValue) { @Sendable mutation in
+                self.withMutation(keyPath: \._state, mutation)
+            }
+
+            if didChange {
+                _$simpleRegistrar.triggerDidChange(for: \.state, on: self)
             }
-            _$simpleRegistrar.triggerDidChange(for: \.state, on: self)
 
             for continuation in subscribedContinuations.values {
                 continuation.yield(state)
@@ -72,10 +73,13 @@ final class BluetoothManagerStorage: ValueObservable, Sendable {
             readOnlyIsScanning
         }
         set {
-            withMutation(keyPath: \._isScanning) {
-                _isScanning.store(newValue, ordering: .relaxed)
+            let didChange = _isScanning.storeAndCompare(newValue) { @Sendable mutation in
+                self.withMutation(keyPath: \._isScanning, mutation)
+            }
+
+            if didChange {
+                _$simpleRegistrar.triggerDidChange(for: \.isScanning, on: self) // didSet
             }
-            _$simpleRegistrar.triggerDidChange(for: \.isScanning, on: self) // didSet
         }
     }
 
@@ -84,11 +88,10 @@ final class BluetoothManagerStorage: ValueObservable, Sendable {
             readOnlyDiscoveredPeripherals
         }
         set {
-            withMutation(keyPath: \._discoveredPeripherals) {
-                rwLock.withWriteLock {
-                    _discoveredPeripherals = newValue
-                }
+            _discoveredPeripherals.store(newValue, using: rwLock) { @Sendable mutation in
+                self.withMutation(keyPath: \._discoveredPeripherals, mutation)
             }
+
             _$simpleRegistrar.triggerDidChange(for: \.discoveredPeripherals, on: self) // didSet
         }
     }

diff --git a/Sources/SpeziBluetooth/CoreBluetooth/Model/DiscoverySession.swift b/Sources/SpeziBluetooth/CoreBluetooth/Model/DiscoverySession.swift
@@ -100,10 +100,6 @@ class DiscoverySession: Sendable {
 
     private var configuration: BluetoothManagerDiscoveryState
 
-    /// The identifier of the last manually disconnected device.
-    /// This is to avoid automatically reconnecting to a device that was manually disconnected.
-    private(set) var lastManuallyDisconnectedDevice: UUID?
-
     private var autoConnectItem: BluetoothWorkItem?
     private(set) var staleTimer: DiscoveryStaleTimer?
 
@@ -149,16 +145,6 @@ class DiscoverySession: Sendable {
         rssi.intValue >= minimumRSSI && rssi.intValue != 127
     }
 
-    func deviceManuallyDisconnected(id uuid: UUID) {
-        lastManuallyDisconnectedDevice = uuid
-    }
-
-    func clearManuallyDisconnectedDevice(for uuid: UUID) {
-        if lastManuallyDisconnectedDevice == uuid {
-            lastManuallyDisconnectedDevice = nil
-        }
-    }
-
     func deviceDiscoveryPostAction(device: BluetoothPeripheral, newlyDiscovered: Bool) {
         if newlyDiscovered {
             if staleTimer == nil {
@@ -204,7 +190,7 @@ extension DiscoverySession {
             return nil // auto-connect is disabled
         }
 
-        guard lastManuallyDisconnectedDevice == nil && !manager.sbHasConnectedDevices else {
+        guard manager.lastManuallyDisconnectedDevice == nil && !manager.sbHasConnectedDevices else {
             return nil
         }
 

diff --git a/Sources/SpeziBluetooth/CoreBluetooth/Model/GATTCharacteristic.swift b/Sources/SpeziBluetooth/CoreBluetooth/Model/GATTCharacteristic.swift
@@ -10,6 +10,17 @@ import CoreBluetooth
 import Foundation
 
 
+struct CharacteristicAccessorCapture: Sendable {
+    let isNotifying: Bool
+    let properties: CBCharacteristicProperties
+
+    fileprivate init(isNotifying: Bool, properties: CBCharacteristicProperties) {
+        self.isNotifying = isNotifying
+        self.properties = properties
+    }
+}
+
+
 struct GATTCharacteristicCapture: Sendable {
     let isNotifying: Bool
     let value: Data?
@@ -69,9 +80,10 @@ public final class GATTCharacteristic {
 
     private let captureLock = RWLock()
 
-    var captured: GATTCharacteristicCapture {
-        captureLock.withReadLock {
-            GATTCharacteristicCapture(from: self)
+    var captured: CharacteristicAccessorCapture {
+        access(keyPath: \.captured)
+        return captureLock.withReadLock {
+            CharacteristicAccessorCapture(isNotifying: _isNotifying, properties: properties)
         }
     }
 
@@ -86,7 +98,10 @@ public final class GATTCharacteristic {
 
     @SpeziBluetooth
     func synchronizeModel(capture: GATTCharacteristicCapture) {
+        var shouldNotifyCapture = false
+
         if capture.isNotifying != isNotifying {
+            shouldNotifyCapture = true
             withMutation(keyPath: \.isNotifying) {
                 captureLock.withWriteLock {
                     _isNotifying = capture.isNotifying
@@ -107,6 +122,14 @@ public final class GATTCharacteristic {
                 }
             }
         }
+
+        if shouldNotifyCapture {
+            // self is never mutated or even accessed in the withMutation call
+            nonisolated(unsafe) let this = self
+            Task { @Sendable @MainActor in
+                this.withMutation(keyPath: \.captured) {}
+            }
+        }
     }
 }