Skip to content

Fix handling of comms error on resume & all pod DeliveryStatus cases #123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

marionbarker
Copy link
Collaborator

@marionbarker marionbarker changed the title confirm pod DeliveryStatus to prevent 0x31 fault Fix handling of comms error on resume & all pod DeliveryStatus cases May 31, 2024
@marionbarker marionbarker requested review from ps2 and itsmojo May 31, 2024 14:56
@marionbarker
Copy link
Collaborator Author

I'm not sure how to reproduce the failure.

I built the improved_delivery_status branch (from this PR) onto a test phone with an rPi simulator. Both the Loop and Trio versions behaved nominally for 24 hours.

I have now installed this version on my real Loop phone with a DASH pod. I will continue to monitor the behavior.

@dnzxy
Copy link

dnzxy commented Jun 7, 2024

This LGTM and should be good to go.

Side note: All these vars that handle the various cases within checks and return a boolean value could be rewritten to be a bit more readable with essentially the same time complexity (O(1) for both, with one-time cost of O(n) for creating these Sets) and identical functionality.

Example:

// current implementation
 public var bolusing: Bool {
        return self == .bolusInProgress || self == .bolusAndTempBasal || self == .extendedBolusRunning || self == .extendedBolusAndTempBasal || self == .priming || self == .extendedBolusWhileSuspended
    }

// suggestion
public var bolusing: Bool {
    let bolusingStates: Set<DeliveryStatus> = [
        .bolusInProgress,
        .bolusAndTempBasal,
        .extendedBolusRunning,
        .extendedBolusAndTempBasal,
        .priming,
        .extendedBolusWhileSuspended
    ]
    return bolusingStates.contains(self)
}

However, this totally is personal preference and not necessary. PR is 100% good as-is.

@marionbarker
Copy link
Collaborator Author

While testing another feature in Trio, Trio got into a state where it thought the rPI DASH had resumed delivery when it was still suspended. This was using the dev branch of OmniBLE (commit 91abd9a).

See details below.

See nightscout/Trio#277 (comment) for the testing associated with this circumstance: Repeat the information here:

OmniBLE Test for OmniBLE PR 125

While running the DASH testing for Trio to first suspend and then resume:

  • got into a state where pod state of rPi does not match what Trio thinks
  • time stamp is just before 13:00 PDT
    • reading the HEX from the rPi simulator: hx="1724b075240a1d08001f100000000fff0000"
    • extended_bolus_active = False, immediate_bolus_active = False, temp_basal_active = False, basal_active = False
    • but Trio thinks pod resumed successfully and is running scheduled basal of 0.7 U/hr
    • The only allowed button to push on Trio was to Suspend delivery - except it was already suspended.
    • modify the OmniBLE submodule to point at improved_delivery_status branch for OmniBLE
    • rebuild the app between 2024-06-07 12:56:53 and 2024-06-07 12:59:55
    • Using the new version of OmniBLE, Trio reports insulin is suspended - this is correct! (Add this log to OmniBLE PR 123 as proof this fix works)
    • upload the log file and csv file
    • This demonstrates that the code modified by this PR worked as expected.

That tests involved some Medtronic tests as well as DASH. Shorten the log file to be just the pod portion, then parse the file:

@marionbarker
Copy link
Collaborator Author

Thank you for your review and your proposed more readable method for the boolean statements.
I'm going to delay that for now. The code as written has been tested and works.

In the future, if we ever combine the code for OmniBLE and OmniKit into a single sub-module, improvements like this will certainly be considered.

@marionbarker marionbarker merged commit 61a6045 into dev Jun 8, 2024
itsmojo added a commit to itsmojo/OmniBLE that referenced this pull request Nov 18, 2024
+ Remove unneeded & redundant didSend() & didReceive() message logging
+ Improved and additional PodState debugDescription display for insulin values
+ Improved pod suspend testing when updating delivery status
+ Update suspended, bolusing, tempBasalRunning, extendedBolusRunning definitions
  for better efficency and clarity using @dnzxy suggestion on OmniBLE PR LoopKit#123
+ Use supportedTempBasalRates var for better OmniKit consistency
@marionbarker marionbarker deleted the improved_delivery_status branch February 5, 2025 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants