Improved verification and checking to prevent possible 0x31 pod faults

[itsmojo]

• Add new non-persistent deliveryStatusVerified podState boolean
• Validate podState insulin delivery state against each pod status return
• Never schedule a basal if pod might possibly be not suspended
• Never start a bolus if one might be possibly be running
• Never start a temp basal if one might possibly be running
• Add optimzation to skip cancelTB before setting a TB when safe to do so
• Add new PodCommsError for pod setup not complete condition
• Add checks to never do a cancel command unless pod setup is complete
• Use matching generic PumpManager error comment strings
• Add unacknowledgedCommand checking to acknowledgeAlerts()
• Add unacknowledgedCommand checking to configureAlerts()
• Update residual OmnipodKit framework comment to OmniBLE
• Update some Unacknowledged command logging to match elsewhere
• Expanded and updated variable use comments

[ps2]

Thanks Joe. I think tackling these errors is really needed, and I appreciate you working on it. See my comments about deliveryStatusVerified. I think at least by that name, that's what unacknowledgedCommand is trying to track. It looks like this new var is maybe also trying to track when we've detected a state corruption of some sort, like when a TB is running, but our state says it's not. Maybe for this particular use, the variable could be renamed to something that indicates that, maybe "deliveryStateInconsistencyDetected" or something like that?

[marionbarker]

Tested with both the previous version and the newly renamed variable with updated logic.

• iOS 16, using rPi DASH simulator
• In normal operation, the behavior is nominal

I'm not sure how to trigger a fault that would have caused an 0x31 to be sent in before making this modification.

[ps2]

The deliveryStateInconsistencyDetected variable seems to be tracking three things:

1. Our PodState is out of sync with DeliveryStatus (updateDeliveryStatus setting deliveryStateInconsistencyDetected to true))
2. We haven’t talked to the pod yet (PodState initializers)
3. We sent a message (podCommsSession.send), but didn’t process the response (updateDeliveryStatus setting deliveryStateInconsistencyDetected to false)

I'm ok adding something for no. 1, and this var name makes sense, and the calls around this use case make sense, but maybe could be persisted; if we detect a problem, and the app then quits, it'd still be appropriate to be tracking this, no?
For no. 2, piggybacking and using the same var for the no. 1 use case is maybe not great. This use case doesn't need persistence.
For no. 3, this is what the unacknowledged command system is tackling. I really don't like having another layer trying to track this. If we are missing things, like the acknowledgeAlerts not checking for unacknowledged commands, then they should be added. But please focus on fixing the system we already have instead of adding another mostly redundant system.

[marionbarker]

Code review:

• I compared OmniKit PR 24 with OmniBLE PR 110
• Both PR are consistent
• These PR add lastDeliveryStatusReceived as a double check to the internal state that Loop maintains for the pod
• In addition to adding protection against 0x31 faults, PR 110 adds the same optimization that is used by the OmniKit code

Test with an rPi DASH (simulated) pod running code that has OmniBLE PR 110 applied to the code.

• The test phone was fed with a glucose pattern that stimulated Loop to modify dosing
• Both automatic bolus and temp basal only dosing methods were applied
• Loop behaved nominally over many hours
• I also confirmed that a new rPi DASH pod goes through the pairing process as expected

I did a few test cases:

Standard code (with PR)
Case 1: Automatic temp basal is issued when the internal state indicates that scheduled basal is running

• Expect Loop to send the setTB command only
• I confirmed this to be true

Standard code (with PR)
Case 2: Automatic temp basal is issued when the internal state indicates that temporary basal is running

• Expect Loop to send the cancelTB command, get a response, and then send the setTB command
• I confirmed this to be true

I then applied a test patch that overwrites the lastDeliveryStatusReceived with a flag indicating a temporary basal is running:

Standard code (with PR and test patch)
Case 3: Automatic temp basal is issued when the internal state indicates that scheduled basal is running

• Loop uses the extra logic check with lastDeliveryStatusReceived
  • Because this check fails (with this patch), an extra cancelTB is sent
• Expect Loop to send the cancelTB command, get a response, send the setTB command
• I confirmed this to be true

Standard code (with PR and test patch)
Case 4: Update scheduled basal program

• This always starts with a cancelAll to suspend pod
• New check adds lastDeliveryStatusReceived being in suspended state in addition to internal state
  • Because this check fails (with this patch), an extra cancelAll is sent
  • I confirmed this is true

Loop Sent: CancelAll
Pod  Sent:  Basal suspended
Loop Sent: CancelAll
Pod  Sent:  Basal suspended
Loop Sent: Modify Basal Schedule

Restored to Standard code (with PR):
Case 5: Update scheduled basal program

• Observed nominal sequence

Loop Sent: CancelAll
Pod  Sent:  Basal suspended
Loop Sent: Modify Basal Schedule

[ps2]

👍