From 6553179e067182a7afdd1f5d05d1653c1b888754 Mon Sep 17 00:00:00 2001 From: Pete Schwamb Date: Thu, 27 Dec 2018 23:40:20 -0600 Subject: [PATCH 1/2] Fixes race condition during initial RileyLink configuration --- .../PumpManager/MinimedPumpManager.swift | 6 ++- MinimedKit/PumpManager/PumpOps.swift | 23 +++++++--- RileyLink.xcodeproj/project.pbxproj | 2 + RileyLinkBLEKit/BLEFirmwareVersion.swift | 27 +++++++++++- RileyLinkBLEKit/CommandSession.swift | 2 +- RileyLinkBLEKit/PeripheralManager.swift | 36 ++++++++++++++-- RileyLinkBLEKit/RadioFirmwareVersion.swift | 25 ++++++++--- RileyLinkBLEKit/RileyLinkDevice.swift | 18 ++++++-- RileyLinkBLEKit/RileyLinkDeviceManager.swift | 43 +++++++++++-------- .../RileyLinkDeviceTableViewController.swift | 9 +++- 10 files changed, 150 insertions(+), 41 deletions(-) diff --git a/MinimedKit/PumpManager/MinimedPumpManager.swift b/MinimedKit/PumpManager/MinimedPumpManager.swift index 4249f5ea8..4039a36c4 100644 --- a/MinimedKit/PumpManager/MinimedPumpManager.swift +++ b/MinimedKit/PumpManager/MinimedPumpManager.swift @@ -203,7 +203,7 @@ public class MinimedPumpManager: RileyLinkPumpManager, PumpManager { pumpOps.runSession(withName: "Tune pump", using: device) { (session) in do { let scanResult = try session.tuneRadio() - self.log.error("Device %{public}@ auto-tuned to %{public}@ MHz", device.name ?? "", String(describing: scanResult.bestFrequency)) + self.log.default("Device %{public}@ auto-tuned to %{public}@ MHz", device.name ?? "", String(describing: scanResult.bestFrequency)) } catch let error { self.log.error("Device %{public}@ auto-tune failed with error: %{public}@", device.name ?? "", String(describing: error)) self.rileyLinkDeviceProvider.deprioritize(device, completion: nil) @@ -230,6 +230,8 @@ public class MinimedPumpManager: RileyLinkPumpManager, PumpManager { private func updatePumpStatus(_ status: MySentryPumpStatusMessageBody, from device: RileyLinkDevice) { dispatchPrecondition(condition: .onQueue(queue)) + log.default("MySentry message received") + var pumpDateComponents = status.pumpDateComponents var glucoseDateComponents = status.glucoseDateComponents @@ -407,7 +409,7 @@ public class MinimedPumpManager: RileyLinkPumpManager, PumpManager { return } - self.log.debug("Pump data is stale, fetching.") + self.log.default("Pump data is stale, fetching.") rileyLinkDeviceProvider.getDevices { (devices) in guard let device = devices.firstConnected else { diff --git a/MinimedKit/PumpManager/PumpOps.swift b/MinimedKit/PumpManager/PumpOps.swift index c0c32dbc6..bf9f6a641 100644 --- a/MinimedKit/PumpManager/PumpOps.swift +++ b/MinimedKit/PumpManager/PumpOps.swift @@ -9,6 +9,7 @@ import Foundation import RileyLinkKit import RileyLinkBLEKit +import os.log public protocol PumpOpsDelegate: class { @@ -18,6 +19,8 @@ public protocol PumpOpsDelegate: class { public class PumpOps { + private let log = OSLog(category: "PumpOps") + public let pumpSettings: PumpSettings private var pumpState: PumpState { @@ -69,10 +72,15 @@ public class PumpOps { sessionQueue.async { let semaphore = DispatchSemaphore(value: 0) - device.runSession(withName: name) { (session) in - let session = PumpOpsSession(settings: self.pumpSettings, pumpState: self.pumpState, session: session, delegate: self) + device.runSession(withName: name) { (commandSession) in + let session = PumpOpsSession(settings: self.pumpSettings, pumpState: self.pumpState, session: commandSession, delegate: self) self.sessionDevice = device - self.configureDevice(device, with: session) + if !commandSession.firmwareVersion.isUnknown { + self.configureDevice(device, with: session) + } else { + self.log.error("Skipping device configuration due to unknown firmware version") + } + block(session) self.sessionDevice = nil semaphore.signal() @@ -88,10 +96,13 @@ public class PumpOps { return } + log.default("Configuring RileyLinkDevice:%{public}@", String(describing: device)) + do { _ = try session.configureRadio(for: pumpSettings.pumpRegion, frequency: pumpState.lastValidFrequency) - } catch { + } catch let error { // Ignore the error and let the block run anyway + log.error("Error configuring device: %{public}@", String(describing: error)) return } @@ -108,7 +119,9 @@ public class PumpOps { NotificationCenter.default.removeObserver(self, name: .DeviceRadioConfigDidChange, object: device) NotificationCenter.default.removeObserver(self, name: .DeviceConnectionStateDidChange, object: device) - configuredDevices.remove(device) + + // TODO: Unsafe access + self.configuredDevices.remove(device) } public func getPumpState(_ completion: @escaping (_ state: PumpState) -> Void) { diff --git a/RileyLink.xcodeproj/project.pbxproj b/RileyLink.xcodeproj/project.pbxproj index e243624ca..84921ad37 100644 --- a/RileyLink.xcodeproj/project.pbxproj +++ b/RileyLink.xcodeproj/project.pbxproj @@ -51,6 +51,7 @@ 431CE7A51F9D78F500255374 /* RFPacket.swift in Sources */ = {isa = PBXBuildFile; fileRef = 431CE7A41F9D78F500255374 /* RFPacket.swift */; }; 431CE7A71F9D98F700255374 /* CommandSession.swift in Sources */ = {isa = PBXBuildFile; fileRef = 431CE7A61F9D98F700255374 /* CommandSession.swift */; }; 4322B75620282DA60002837D /* ResponseBufferTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4322B75520282DA60002837D /* ResponseBufferTests.swift */; }; + 43260F6F21C9B21000DD6837 /* Locked.swift in Sources */ = {isa = PBXBuildFile; fileRef = 435D26B720DC83E400891C17 /* Locked.swift */; }; 432847C11FA1737400CDE69C /* RileyLinkBLEKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 431CE76F1F98564100255374 /* RileyLinkBLEKit.framework */; }; 432847C31FA57C0F00CDE69C /* RadioFirmwareVersion.swift in Sources */ = {isa = PBXBuildFile; fileRef = 432847C21FA57C0F00CDE69C /* RadioFirmwareVersion.swift */; }; 432CF9061FF74CCB003AB446 /* RileyLinkKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 43722FAE1CB9F7630038B7F2 /* RileyLinkKit.framework */; }; @@ -3118,6 +3119,7 @@ 431CE7A11F9D195600255374 /* CBCentralManager.swift in Sources */, 431CE7911F985D8D00255374 /* RileyLinkDeviceManager.swift in Sources */, 431CE78D1F985B5400255374 /* PeripheralManager.swift in Sources */, + 43260F6F21C9B21000DD6837 /* Locked.swift in Sources */, 431CE79E1F9BE73900255374 /* BLEFirmwareVersion.swift in Sources */, 432847C31FA57C0F00CDE69C /* RadioFirmwareVersion.swift in Sources */, 431CE7931F985DE700255374 /* PeripheralManager+RileyLink.swift in Sources */, diff --git a/RileyLinkBLEKit/BLEFirmwareVersion.swift b/RileyLinkBLEKit/BLEFirmwareVersion.swift index 00973270b..83e2cc435 100644 --- a/RileyLinkBLEKit/BLEFirmwareVersion.swift +++ b/RileyLinkBLEKit/BLEFirmwareVersion.swift @@ -20,8 +20,26 @@ public struct BLEFirmwareVersion { return nil } + self.init( + components: versionString[versionIndex...].split(separator: ".").compactMap({ Int($0) }), + versionString: versionString + ) + } + + init(components: [Int], versionString: String) { + self.components = components self.versionString = versionString - components = versionString[versionIndex...].split(separator: ".").compactMap({ Int($0) }) + } +} + + +extension BLEFirmwareVersion { + static var unknown: BLEFirmwareVersion { + return self.init(components: [0], versionString: "Unknown") + } + + public var isUnknown: Bool { + return self == BLEFirmwareVersion.unknown } } @@ -33,6 +51,13 @@ extension BLEFirmwareVersion: CustomStringConvertible { } +extension BLEFirmwareVersion: Equatable { + public static func ==(lhs: BLEFirmwareVersion, rhs: BLEFirmwareVersion) -> Bool { + return lhs.components == rhs.components + } +} + + extension BLEFirmwareVersion { var responseType: PeripheralManager.ResponseType { guard let major = components.first, major >= 2 else { diff --git a/RileyLinkBLEKit/CommandSession.swift b/RileyLinkBLEKit/CommandSession.swift index 19fc09c75..29cfca190 100644 --- a/RileyLinkBLEKit/CommandSession.swift +++ b/RileyLinkBLEKit/CommandSession.swift @@ -75,7 +75,7 @@ public struct RileyLinkStatistics { public struct CommandSession { let manager: PeripheralManager let responseType: PeripheralManager.ResponseType - let firmwareVersion: RadioFirmwareVersion + public let firmwareVersion: RadioFirmwareVersion /// Invokes a command expecting a response /// diff --git a/RileyLinkBLEKit/PeripheralManager.swift b/RileyLinkBLEKit/PeripheralManager.swift index ad8206768..c845733f0 100644 --- a/RileyLinkBLEKit/PeripheralManager.swift +++ b/RileyLinkBLEKit/PeripheralManager.swift @@ -27,7 +27,7 @@ class PeripheralManager: NSObject { oldValue.delegate = nil peripheral.delegate = self - queue.async { + queue.sync { self.needsConfiguration = true } } @@ -52,8 +52,15 @@ class PeripheralManager: NSObject { // Confined to `queue` private var needsConfiguration = true - weak var delegate: PeripheralManagerDelegate? + weak var delegate: PeripheralManagerDelegate? { + didSet { + queue.sync { + needsConfiguration = true + } + } + } + // Called from RileyLinkDeviceManager.managerQueue init(peripheral: CBPeripheral, configuration: Configuration, centralManager: CBCentralManager) { self.peripheral = peripheral self.central = centralManager @@ -100,6 +107,7 @@ protocol PeripheralManagerDelegate: class { extension PeripheralManager { func configureAndRun(_ block: @escaping (_ manager: PeripheralManager) -> Void) -> (() -> Void) { return { + // TODO: Accessing self might be a race on initialization if !self.needsConfiguration && self.peripheral.services == nil { self.log.error("Configured peripheral has no services. Reconfiguring…") } @@ -107,14 +115,20 @@ extension PeripheralManager { if self.needsConfiguration || self.peripheral.services == nil { do { try self.applyConfiguration() + self.log.default("Peripheral configuration completed") } catch let error { self.log.error("Error applying peripheral configuration: %@", String(describing: error)) // Will retry } do { - try self.delegate?.completeConfiguration(for: self) - self.needsConfiguration = false + if let delegate = self.delegate { + try delegate.completeConfiguration(for: self) + self.log.default("Delegate configuration completed") + self.needsConfiguration = false + } else { + self.log.error("No delegate set for configuration") + } } catch let error { self.log.error("Error applying delegate configuration: %@", String(describing: error)) // Will retry @@ -439,3 +453,17 @@ extension PeripheralManager: CBCentralManagerDelegate { } } } + + +extension PeripheralManager { + public override var debugDescription: String { + var items = [ + "## PeripheralManager", + "peripheral: \(peripheral)", + ] + queue.sync { + items.append("needsConfiguration: \(needsConfiguration)") + } + return items.joined(separator: "\n") + } +} diff --git a/RileyLinkBLEKit/RadioFirmwareVersion.swift b/RileyLinkBLEKit/RadioFirmwareVersion.swift index 2a1668be6..3c1d51ccf 100644 --- a/RileyLinkBLEKit/RadioFirmwareVersion.swift +++ b/RileyLinkBLEKit/RadioFirmwareVersion.swift @@ -19,17 +19,26 @@ public struct RadioFirmwareVersion { return nil } - self.versionString = versionString - components = versionString[versionIndex...].split(separator: ".").compactMap({ Int($0) }) + self.init( + components: versionString[versionIndex...].split(separator: ".").compactMap({ Int($0) }), + versionString: versionString + ) } - private init(components: [Int]) { - versionString = "Unknown" + private init(components: [Int], versionString: String) { + self.versionString = versionString self.components = components } +} + +extension RadioFirmwareVersion { static var unknown: RadioFirmwareVersion { - return self.init(components: [1]) + return self.init(components: [0], versionString: "Unknown") + } + + public var isUnknown: Bool { + return self == RadioFirmwareVersion.unknown } } @@ -40,6 +49,12 @@ extension RadioFirmwareVersion: CustomStringConvertible { } } +extension RadioFirmwareVersion: Equatable { + public static func ==(lhs: RadioFirmwareVersion, rhs: RadioFirmwareVersion) -> Bool { + return lhs.components == rhs.components + } +} + // Version 2 changes extension RadioFirmwareVersion { diff --git a/RileyLinkBLEKit/RileyLinkDevice.swift b/RileyLinkBLEKit/RileyLinkDevice.swift index abcb67002..6e682faf5 100644 --- a/RileyLinkBLEKit/RileyLinkDevice.swift +++ b/RileyLinkBLEKit/RileyLinkDevice.swift @@ -155,9 +155,16 @@ extension RileyLinkDevice { extension RileyLinkDevice { public func runSession(withName name: String, _ block: @escaping (_ session: CommandSession) -> Void) { sessionQueue.addOperation(manager.configureAndRun({ [weak self] (manager) in - self?.log.debug("======================== %{public}@ ===========================", name) - block(CommandSession(manager: manager, responseType: self?.bleFirmwareVersion?.responseType ?? .buffered, firmwareVersion: self?.radioFirmwareVersion ?? .unknown)) - self?.log.debug("------------------------ %{public}@ ---------------------------", name) + self?.log.default("======================== %{public}@ ===========================", name) + let bleFirmwareVersion = self?.bleFirmwareVersion + let radioFirmwareVersion = self?.radioFirmwareVersion + + if bleFirmwareVersion == nil || radioFirmwareVersion == nil { + self?.log.error("Running session with incomplete configuration: bleFirmwareVersion %{public}@, radioFirmwareVersion: %{public}@", String(describing: bleFirmwareVersion), String(describing: radioFirmwareVersion)) + } + + block(CommandSession(manager: manager, responseType: bleFirmwareVersion?.responseType ?? .buffered, firmwareVersion: radioFirmwareVersion ?? .unknown)) + self?.log.default("------------------------ %{public}@ ---------------------------", name) })) } } @@ -307,6 +314,8 @@ extension RileyLinkDevice: PeripheralManagerDelegate { } else { self.log.error("Unknown idle response: %@", value.hexadecimalString) } + } else { + self.log.error("Skipping parsing characteristic value update due to missing BLE firmware version") } self.assertIdleListening(forceRestart: true) @@ -341,6 +350,7 @@ extension RileyLinkDevice: PeripheralManagerDelegate { func completeConfiguration(for manager: PeripheralManager) throws { // Read bluetooth version to determine compatibility + log.default("Reading firmware versions for PeripheralManager configuration") let bleVersionString = try manager.readBluetoothFirmwareVersion(timeout: 1) bleFirmwareVersion = BLEFirmwareVersion(versionString: bleVersionString) @@ -361,7 +371,7 @@ extension RileyLinkDevice: CustomDebugStringConvertible { "isTimerTickNotifying: \(manager.timerTickEnabled)", "radioFirmware: \(String(describing: radioFirmwareVersion))", "bleFirmware: \(String(describing: bleFirmwareVersion))", - "peripheral: \(manager.peripheral)", + "peripheralManager: \(String(reflecting: manager))", "sessionQueue.operationCount: \(sessionQueue.operationCount)" ].joined(separator: "\n") } diff --git a/RileyLinkBLEKit/RileyLinkDeviceManager.swift b/RileyLinkBLEKit/RileyLinkDeviceManager.swift index e536bc028..f76423464 100644 --- a/RileyLinkBLEKit/RileyLinkDeviceManager.swift +++ b/RileyLinkBLEKit/RileyLinkDeviceManager.swift @@ -12,6 +12,7 @@ import os.log public class RileyLinkDeviceManager: NSObject { private let log = OSLog(category: "RileyLinkDeviceManager") + // Isolated to centralQueue private var central: CBCentralManager! private let centralQueue = DispatchQueue(label: "com.rileylink.RileyLinkBLEKit.BluetoothManager.centralQueue", qos: .utility) @@ -34,16 +35,18 @@ public class RileyLinkDeviceManager: NSObject { super.init() - central = CBCentralManager( - delegate: self, - queue: centralQueue, - options: [ - CBCentralManagerOptionRestoreIdentifierKey: "com.rileylink.CentralManager" - ] - ) + centralQueue.sync { + central = CBCentralManager( + delegate: self, + queue: centralQueue, + options: [ + CBCentralManagerOptionRestoreIdentifierKey: "com.rileylink.CentralManager" + ] + ) + } } - // MARK: - Configuration. Not thread-safe. + // MARK: - Configuration public var idleListeningEnabled: Bool { if case .disabled = idleListeningState { @@ -53,22 +56,27 @@ public class RileyLinkDeviceManager: NSObject { } } - public var idleListeningState: RileyLinkDevice.IdleListeningState = .disabled { - didSet { - let newState = self.idleListeningState - + public var idleListeningState: RileyLinkDevice.IdleListeningState { + get { + return lockedIdleListeningState.value + } + set { + lockedIdleListeningState.value = newValue centralQueue.async { for device in self.devices { - device.setIdleListeningState(newState) + device.setIdleListeningState(newValue) } } } } + private let lockedIdleListeningState = Locked(RileyLinkDevice.IdleListeningState.disabled) - public var timerTickEnabled = true { - didSet { - let newValue = timerTickEnabled - + public var timerTickEnabled: Bool { + get { + return lockedTimerTickEnabled.value + } + set { + lockedTimerTickEnabled.value = newValue centralQueue.async { for device in self.devices { device.setTimerTickEnabled(newValue) @@ -76,6 +84,7 @@ public class RileyLinkDeviceManager: NSObject { } } } + private let lockedTimerTickEnabled = Locked(true) } diff --git a/RileyLinkKitUI/RileyLinkDeviceTableViewController.swift b/RileyLinkKitUI/RileyLinkDeviceTableViewController.swift index b2f66100a..32b66eb8b 100644 --- a/RileyLinkKitUI/RileyLinkDeviceTableViewController.swift +++ b/RileyLinkKitUI/RileyLinkDeviceTableViewController.swift @@ -10,11 +10,14 @@ import UIKit import LoopKitUI import RileyLinkBLEKit import RileyLinkKit +import os.log let CellIdentifier = "Cell" public class RileyLinkDeviceTableViewController: UITableViewController { + private let log = OSLog(category: "RileyLinkDeviceTableViewController") + public let device: RileyLinkDevice private var bleRSSI: Int? @@ -105,7 +108,9 @@ public class RileyLinkDeviceTableViewController: UITableViewController { DispatchQueue.main.async { self.uptime = statistics.uptime } - } catch { } + } catch let error { + self.log.error("Failed to get stats for uptime: %{public}@", String(describing: error)) + } } } @@ -118,7 +123,7 @@ public class RileyLinkDeviceTableViewController: UITableViewController { self.frequency = frequency } } catch let error { - print("Error: \(error)") + self.log.error("Failed to get base frequency: %{public}@", String(describing: error)) } } From 62c21d88a7e5c59878fa93f2955eccef3aec7c7c Mon Sep 17 00:00:00 2001 From: Pete Schwamb Date: Wed, 2 Jan 2019 13:57:41 -0600 Subject: [PATCH 2/2] Avoid deadlock in logging --- MinimedKit/PumpManager/PumpOps.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MinimedKit/PumpManager/PumpOps.swift b/MinimedKit/PumpManager/PumpOps.swift index bf9f6a641..6b6ff57b9 100644 --- a/MinimedKit/PumpManager/PumpOps.swift +++ b/MinimedKit/PumpManager/PumpOps.swift @@ -96,7 +96,7 @@ public class PumpOps { return } - log.default("Configuring RileyLinkDevice:%{public}@", String(describing: device)) + log.default("Configuring RileyLinkDevice: %{public}@", String(describing: device.deviceURI)) do { _ = try session.configureRadio(for: pumpSettings.pumpRegion, frequency: pumpState.lastValidFrequency)