support sensor start, stop, calibrate #76

thebookins · 2018-02-27T12:08:54Z

This PR adds support for starting, stopping and calibrating sensors for in-date transmitters.

Create new protocol TimedTransmitterTxMessage, with a date and that requires an activationDate from the transmitter for synchronisation. SessionStartTxMessage, SessionStopTxMessage and CalibrateGlucoseTxMessage now conform to this protocol.
Add message queue to Transmitter, which holds any user-initiated messages for processing at the next sensor read. This queue is processed in the control step.
Add new message CalibrateGlucoseRxMessage.
Append CRC (if used) after converting byte sequence to Data (for longer messages).
Add public functions startSensor, stopSensor and calibrateSensor to Transmitter
Add UI for starting a session and calibrating a sensor in Example app.

ps2

Thanks for the changes, Paul. I have another PR that I'm going to land that may create some conflicts, but I think it provides a better base for some of these changes to build on.

ps2 · 2018-03-03T05:11:51Z

CGMBLEKit Example/ViewController.swift


        transmitterIDField.text = AppDelegate.sharedDelegate.transmitter?.ID
+
+        titleLabel.isUserInteractionEnabled = true


This is just an example app... but this is what UIButton is for.

Fair call. I was trying to keep it simple, but will change.

ps2 · 2018-03-03T05:13:01Z

xDripG5/Transmitter.swift


    private var delegateQueue = DispatchQueue(label: "com.loudnate.xDripG5.delegateQueue", qos: .utility)

+    private var messageQueue = MessageQueue()


The biggest concern I have here is that this doesn't support persistence, and that's important if the app were to be closed before being awoken when the transmitter appears next.

I don't think this framework should manage persistence, so I think the better approach is to ask the TransmitterDelegate for its pending commands, and to publicly expose a Command enum.

It's also important to allow re-queing of failed commands, and this should be up to the delegate to determine.

enum Command: RawRepresentable { typealias RawValue = Data case startSensor(at: Date()) case stopSensor(at: Date()) case calibrateSensor(to: HKQuantity()) // Provide serialization for clients to easily persist and restore } public protocol TransmitterDelegate: class { func dequeuePendingCommand(for transmitter: Transmitter) -> Command? func transmitter(_ transmitter: Transmitter, didFail command: Command, with error: Error) func transmitter(_ transmitter: Transmitter, didComplete command: Command) }

Agreed. I did wonder how to manage persistence - this is a better solution.

ps2 · 2018-03-03T05:13:52Z

xDripG5/Transmitter.swift


        let activationDate = Date(timeIntervalSinceNow: -TimeInterval(timeMessage.currentTime))

+        while let message = getMessage() {


These queued messages should run after the GlucoseRxMessage is received (and the delegate method called).
The client should consider the most up-to-date status of the sensor when choosing which additional messages to run, for example, a session start wouldn't be necessary if the sensor was already started, or the index of a new glucose value would inform whether backfill should be read.

The official iOS app calls the queued messages before the GlucoseRxMessage is asked for and received (confirmed by watching the messages on the control characteristic while running xDripG5 in passive mode). One advantage of this is that when the glucose message is read it is immediately updated based on the queued messages. For example, a calibration will be immediately reflected in the glucose that is read; and after a sensor start message the state will immediately switch to .warmup. Otherwise one would have to wait 5 minutes for the next read.
I agree that the client should restrict which messages can be sent based on the previous state, but IMO we should just test for freshness of the last read glucose (perhaps < 15 mins old) and use the state contained in that message. In the event that a message is not compatible with the current state (e.g. a start message while in warmup) the transmitter just ignores it.
Backfill is an exception. The official app asks for backfill after all the other messages. The official app sends the Tx messages in this order (if there is a calibration pending and backfill is required):

TransmitterTimeTxMessage (0x24) CalibrationTxMessage (0x34) GlucoseTxMessage (0x30) CalibrationDataTxMessage (0x32) BackfillTxMessage (0x50)

thebookins · 2018-03-03T08:04:45Z

Thanks for the feedback. These changes, along with #77, make for a much better solution. Will submit a new PR against the current dev.

thebookins · 2018-03-11T06:23:25Z

I have followed most of these suggestions on https://github.com/thebookins/xDripG5/tree/start-stop-calibrate2, but not sure how to call the transmitter delegate functions from within the control function (as it doesn't have access to the private members of Transmitter). Is it acceptable to use closures like this

    fileprivate func control(shouldWaitForBond: Bool = false, getCommand: () -> Command?, didFail: (_ command: Command, _ error: Error ) -> Void, didComplete: (_ command: Command) -> Void) throws -> Glucose {

and then call control like this

                    let glucose = try peripheral.control(shouldWaitForBond: status.bonded != 0x1, getCommand: {
                        self.delegate?.dequeuePendingCommand(for: self)
                    }, didFail: {(command: Command, error: Error) -> Void in
                        self.delegateQueue.async {
                            self.delegate?.transmitter(self, didFail: command, with: error)
                        }
                    }, didComplete: {(command: Command) -> Void in
                        self.delegateQueue.async {
                            self.delegate?.transmitter(self, didComplete: command)
                        }
                    })

or is there a better way?

UPDATE: IMO flow is cleaner if we split up the control function into several functions (enableNotify(), synchronize(), sendCommand(), readGlucose(), disconnect()); then we don't need to rely so heavily on closures. I'll post changes shortly.

ps2 · 2018-03-22T04:12:46Z

xDripG5/Transmitter.swift

 import os.log


+public enum Command: RawRepresentable {


This belongs in its own file.

ps2 · 2018-03-22T04:14:02Z

xDripG5/Transmitter.swift

+        }
+
+        switch command {
+        case 0:


0, 1, and 2 shouldn't be used as free-floating values. Define a private, Int-backed enum. If Command continues to expand, it should probably be expanded into a protocol instead of a single enum.

Thanks. I've added an Action enum. Please advise if the implementation isn't ideal.

ps2 · 2018-03-22T04:16:39Z

xDripG5/Messages/SessionStartTxMessage.swift

    typealias Response = SessionStartRxMessage

-    let startTime: UInt32
+    let time: UInt32


time and timeEpoch need more disambiguation, either via naming or documentation comments

thebookins · 2018-03-22T11:36:48Z

Thanks @ps2 . I believe I have addressed your latest change requests. The Example app is not ideal (does not yet support persistence) but I can move those changes to a separate PR if best.

How do you want to manage merge conflicts with #82?

ps2 · 2018-03-24T23:03:42Z

CGMBLEKit Example/NSUserDefaults.swift

+
+    var glucoseUnit: HKUnit {
+        return HKUnit.milligramsPerDeciliter()
+    }


This doesn't make sense here.

Plan was to support mmol/L, and set the units in one place. Probably better not to complicate things for example app. Reverted.

ps2 · 2018-03-24T23:04:18Z

xDripG5/Messages/CalibrationDataRxMessage.swift

            return nil
        }
+
+        glucose = data[11..<13].to(UInt16.self)


You can use toInt() on this line like the one below. The type is inferred due to being declared above.

Changed. Thanks.

ps2 · 2018-03-24T23:04:47Z

xDripG5/Messages/SessionStartTxMessage.swift

+    let time: UInt32
+
+    /// Time in seconds since Unix Epoch
+    let timeEpoch: UInt32


The platform epoch (known as the reference date) is actually January 1, 2001. A better name for this would be secondsSince1970.

ps2 · 2018-03-24T23:05:32Z

xDripG5/Messages/SessionStartTxMessage.swift


-    let startTime: UInt32
+    /// Time since activation in Dex seconds
+    let time: UInt32


Why rename this from startTime?

Just for consistency between CalibrateTx, StartSessionTx and StopSessionTx, and startTime and stopTime and calibrateTime seem to me a bit verbose. Happy to revert to startTime and stopTime. Reverted.

ps2 · 2018-03-24T23:06:04Z

xDripG5/Messages/SessionStopTxMessage.swift

    typealias Response = SessionStopRxMessage

-    let stopTime: UInt32
+    let time: UInt32


why rename this from stopTime?

ps2 · 2018-03-24T23:07:35Z

xDripG5/Transmitter.swift

-        let activationDate = Date(timeIntervalSinceNow: -TimeInterval(timeMessage.currentTime))
+    }

+    fileprivate func readGlucose(timeMessage: TransmitterTimeRxMessage, activationDate: Date) throws -> Glucose {


This method is an extension of PeripheralManager, which is focused on providing easy, safe command-response API on top of Core Bluetooth. It doesn't make sense for this method to take timeMessage and activationDate arguments for the sole purpose of constructing a Glucose object.

readGlucose() throws -> GlucoseRxMessage seems more appropriate.

Thanks. Changed, here and also for readCalibrationData. Are you happy for sendCommand to take an activationDate?

ps2 · 2018-03-24T23:08:07Z

xDripG5/Transmitter.swift

        return Glucose(glucoseMessage: glucoseMessage, timeMessage: timeMessage, activationDate: activationDate)
    }

+    fileprivate func readCalibrationData(activationDate: Date) throws -> Calibration {


Same comment applies here

ps2 · 2018-03-24T23:08:34Z

xDripG5/Transmitter.swift


    func transmitter(_ transmitter: Transmitter, didRead glucose: Glucose)

+    func transmitter(_ transmitter: Transmitter, didRead calibration: Calibration)


Instead of returning this value separately and expanding the surface area for clients to manage, it would be more natural for a Calibration? property to be added to Glucose.

ps2 · 2018-03-24T23:09:05Z

CGMBLEKit Example/ViewController.swift

+                    return
+                }
+                let glucose = HKQuantity(unit: unit, doubleValue: Double(entry))
+                AppDelegate.sharedDelegate.commandQueue.enqueue(.calibrateSensor(to: glucose, at: Date()))


It's not safe to mutate commandQueue here on the main thread, and also mutate it on a background thread during TransmitterDelegate.dequeuePendingCommand(for:)

ps2 · 2018-03-24T23:09:31Z

CGMBLEKit Example/AppDelegate.swift

    }
 }
+
+struct CommandQueue {


This doesn't provide anything on top of Array<Command>, and it isn't thread-safe to use. You might consider making this type a class, and internally thread-safe using os_unfair_lock

thebookins · 2018-03-30T01:59:18Z

@ps2 I think I have addressed the issues in the latest review, and I've also merged with latest dev. Would be great to get your feedback.

ps2 · 2018-03-30T13:48:21Z

CGMBLEKit Example/AppDelegate.swift


    var transmitter: Transmitter?

+    var commandQueue = CommandQueue()


This should be let.

ps2 · 2018-03-30T13:48:52Z

CGMBLEKit Example/CommandQueue.swift

+    }
+
+    func dequeue() -> Command? {
+        if !list.isEmpty {


This has to be accessed inside the same lock.

ps2 · 2018-03-30T13:49:10Z

CGMBLEKit Example/CommandQueue.swift

+    private var lock = os_unfair_lock()
+
+    var isEmpty: Bool {
+        return list.isEmpty


This doesn't have any callers, so it should either be removed or moved inside the lock.

ps2 · 2018-03-30T13:49:41Z

xDripG5/CalibrationState.swift

+    public var description: String {
+        switch self {
+        case .known(let state):
+            return "\(state)"


return String(describing: state)

ps2 · 2018-03-30T13:50:31Z

xDripG5/CalibrationState.swift

+        switch self {
+        case .known(let state):
+            return "\(state)"
+        default:


This should be the .unknown case; default cases should be avoided whenever possible.

ps2 · 2018-03-30T13:51:16Z

xDripG5/Messages/CalibrationDataRxMessage.swift

            return nil
        }
+
+        glucose = data[11..<13].toInt()


Are you sure the first bit isn't used for something else like in GlucoseRxMessage?

You mean the bytes 1 to 10? Yes I am sure they are used for something, I just can't work out what that is. Here's an example: 3300352d0090012d00da00 cb00 3bce5100 9e19. There's nothing in the bytes 1 to 10 that looks like timestamp, status, or anything else that's familiar. I was hoping to be able to extract calibration parameters from these bytes (e.g. slope and offset), but they change every read despite having the same last calibration.

What I mean is that the highest-order bit of byte[12] is used to indicate a "display-only" glucose value.

OK, good question. It's the 4 bits with highest order, isn't it, going by the mask glucoseIsDisplayOnly = (glucoseBytes & 0xf000) > 0? And it's the lowest order bit of those 4 bits that is used to indicate display glucose (i.e. 0001 in binary).

They could certainly be used for something else, but I don't know what that would be. This is a meter BG, so "display-only" doesn't make sense here. To be safe, we could use a mask (glucoseBytes & 0xfff). These transmitter messages aren't particularly optimized for size, so it would surprise me if they tried to fit something else in those 4 bits.

I also can't think of a reliable way to test. All I can say is I have never seen those 4 bits set to anything other than zero.

Yep – we should go ahead and mask the value. Glucose values will never use those bits anyway.

ps2 · 2018-03-30T13:51:57Z

xDripG5/Transmitter.swift

+
+                    while let command: Command = {
+                        var c: Command?
+                        self.delegateQueue.sync {


Don't dispatch to another queue here... there's no reason for it.

ps2 · 2018-03-30T13:52:24Z

xDripG5/Transmitter.swift

+                    }() {
+                        do {
+                            _ = try peripheral.sendCommand(command, activationDate: activationDate)
+                            self.delegateQueue.async {


The function of the didComplete and didFail methods is to allow the commander to make an immediate decision about how to handle subsequent commands, e.g. it might decide to not continue if an earlier command fails.

This needs to be synchronous. No need to dispatch.

Thanks. I hope I've understood correctly. In the latest commit I'm not calling any of the TransmitterCommandSource from the delegate queue. I assume that's what you meant; rather than calling self.delegateQueue.sync {.

ps2 · 2018-03-30T13:52:48Z

xDripG5/Transmitter.swift

+                                self.commandSource?.transmitter(self, didComplete: command)
+                            }
+                        } catch let error {
+                            self.delegateQueue.async {


ps2 · 2018-04-07T23:54:12Z

xDripG5/Messages/CalibrationDataRxMessage.swift

            return nil
        }
+
+        glucose = data[11..<13].toInt()


Yep – we should go ahead and mask the value. Glucose values will never use those bits anyway.

thebookins added 3 commits February 27, 2018 22:06

support start, stop, calibrate

3ad969c

use let not var

fbc6270

remove print statetment

8f69c04

ps2 requested changes Mar 3, 2018

View reviewed changes

thebookins added 2 commits March 11, 2018 16:17

add Command enum, and logic for start, stop and calibrate

2c09c15

use closures to call transmitter delegate methods

0778ab0

thebookins added 17 commits March 13, 2018 20:35

merge with LoopKit/dev

832b74f

remove extra whitespace and HealthKit dependency

549df79

add opcode for calibrateGlucoseRx

71442e2

add CRC to SessionStopTxMessage

ea60bc4

add seconds since epoch, add CRC to SessionStartTxMessage

167f5d6

build CalibrateGlucoseTxMessage

5b3ee24

add stubs for TransmitterDelgate methods

383b5b2

split control into several functions, to avoid excessive use of closures

d8f7b4e

remove actions from example app temporarily

76f8506

revert change to start restoration, add rawValue to log

97deddf

restore line return at eof

b507741

add calibration button

d2b7200

set glucose units in one place

fd4bf0d

add else clause

a76ea2b

revert previous commit

15b8b6c

rename synchronize readTimeMessage

ac841ce

add method to read calibration data

623bfc3

ps2 requested changes Mar 22, 2018

View reviewed changes

thebookins added 4 commits March 22, 2018 21:28

fix glucose units in calibrate function

82d82cc

add documentation comments for disambiguation

80cc54b

move Command enum to new file

237b1b9

add private Action enum

4d57dc4

add changes to project

1acb6a8

thebookins added 3 commits March 22, 2018 22:46

round before cast

55195a6

serialize action as raw value

80abe97

just use Int, not UInt8

faad37d

ps2 requested changes Mar 24, 2018

View reviewed changes

thebookins added 13 commits March 25, 2018 13:04

remove glucoseUnits user preference

99a8e95

use toInt()

6c051c3

use startTime, stopTime rather than time

fbd0721

add lastCalibration as optional property of Glucose

b27b41e

add default value of nil

0d3b20a

add thread-safe CommandQueue

93afb9c

create protocol TransmitterCommandSource

0920e65

merge with latest dev

6eb4e6b

remove HealthKit dependency

4558322

remove dev team

ce56a02

add custom string for CalibrationState

bd14665

add start and stop actions to example app

39b1700

fix button names

1af862a

ps2 requested changes Mar 30, 2018

View reviewed changes

thebookins added 4 commits March 31, 2018 09:27

use let not var

b02be6e

remove isEmpty, always access list inside lock

1df3cf3

don't use default case; use String(describing:)

6203380

don't dispatch TransmitterCommandSource methods

f1cb6c3

ps2 requested changes Apr 7, 2018

View reviewed changes

mask glucose value to exclude 4 most-significant bits

7934547

ps2 merged commit cf8d6d7 into LoopKit:dev Apr 8, 2018

thebookins deleted the start-stop-calibrate branch August 25, 2018 23:05


		transmitterIDField.text = AppDelegate.sharedDelegate.transmitter?.ID

		titleLabel.isUserInteractionEnabled = true


		private var delegateQueue = DispatchQueue(label: "com.loudnate.xDripG5.delegateQueue", qos: .utility)

		private var messageQueue = MessageQueue()


		let activationDate = Date(timeIntervalSinceNow: -TimeInterval(timeMessage.currentTime))

		while let message = getMessage() {


		func transmitter(_ transmitter: Transmitter, didRead glucose: Glucose)

		func transmitter(_ transmitter: Transmitter, didRead calibration: Calibration)


		var transmitter: Transmitter?

		var commandQueue = CommandQueue()

support sensor start, stop, calibrate #76

support sensor start, stop, calibrate #76

Uh oh!

Conversation

thebookins commented Feb 27, 2018

Uh oh!

ps2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebookins Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebookins commented Mar 3, 2018

Uh oh!

thebookins commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebookins commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebookins commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebookins Mar 3, 2018 •

edited

Loading

thebookins commented Mar 11, 2018 •

edited

Loading

thebookins Apr 3, 2018 •

edited

Loading

thebookins Mar 30, 2018 •

edited

Loading