Conversation
|
This PR contains unrelated commits from the dev branche, which I think is undesired? |
|
In addition, it is stated: "It should be reviewed on top of PR #4467" |
|
Nice 👍 |
|
This PR contains lots of file changes and commits. |
df5aa07 to
189629d
Compare
|
@vanelsberg I think I fixed it, there are two commits now in this PR: the first one equals 4467 (quantifying the basal drift) and the second one is resolving the basal drift. does this make sense? I think that also would mean we can close #4467 as the distinction is now just clear in this PR. |
Yes! This looks a lot better, making it easier to follow implementation. Helps reviewing/testing and - eventually - when ok, acceptance for merging with dev. (When 4467 is no longer of use, I think best to close it to prevent confusion?) |
...rc/main/kotlin/app/aaps/pump/omnipod/dash/driver/pod/state/OmnipodDashPodStateManagerImpl.kt
Outdated
Show resolved
Hide resolved
6b01383 to
189629d
Compare
|
@nl-ruud Proposed way to testing this PR: To see if AAPS is affected first test this whiteout the PR, preferably for 2 or more days. Then merge this PR and retest with current AAPS (this can be current dev or current 3.4 release). Test:
Validation: Optional, check the AAPS logfiles, like filtering on the terms below: |
Actually no more than 0.05U. If a bigger difference is perceived then I’d love to see the AAPS logs of that 24 hour period. Also, the docs currently state “When using SMB, limit the interval to 5 minutes minimum to avoid this issue.”. Even though that does not avoid the issue (as stated there), it may reduce the severity. So reset that setting to default (i believe it’s 3 minutes) before you test if you followed that direction from the docs. Especially if you use a sensor like FSL that updates every minute. |
|
With the fix, I notice that the loop has become more consistent and shows less unpredictable behavior. I’ve been using this since January 23 (14 days now), and I’m satisfied with the result. |
If you cherry pick 254dfe3 then the beeps should be gone. Will wait for the others to complete testing and then add a final commit for the PR. |
|
Confirming beeps for correction bolus. Noticed this only twice during testing: 1 unexpected beep (ignored it) and unexpected bolus notification for 0.05U on Watch. The last one triggered some questions (forgot all about this when posting test results on TDD deviations). Currently starting to test DASH without PR4499 under similar circumstances. After 4 days testing I'l post results here.
@nl-ruud You may want to check if your code change not only skips the beep but also the notification? |
Seems to have resolved beeping, thanks! In terms of test data here are my results so far:
|
|
I have tested this PR for several days now on v3.4. Before building with the PR, I observed a drift of 1.5-2.5 units per day. After the PR, I'm getting no drift or 0.05 units (which I suspect may be more due to a challenge in determining the exact Dash delivered amounts at midnight if the only timestamps are 5-10 mins before or after). I also have manually bolused while testing the PR and found this also was reported correctly. The AAPS statistics included the manual bolus while the Dash history did not. |
|
I just submitted a (final?) update to the PR that addresses two issues that were reported by @xitation, which are:
|
Will retest this for a few days. |
|
Applied latest commit but so far only 0,05+- difference between pump and AAPS TDD. I think this PR is ready for merge, but will keep monitoring / logging. CC: @MilosKozak
|
100% Agree. @MilosKozak Ik think this is an important bugfix, affects users especially when on lower TDD (like for kids on DASH) |
|
My percent difference ob about 24 units was more like 12% or more, on 1 minute aISF 1st 2 days modified more like 3.5% and 5.5% of 20u tdd |
be1177d to
6fbf11f
Compare
|
To merge this PR into dev, and after discussing with @vanelsberg, I added a semaphore file to enable basal drift compensation. You need an empty file called
I believe this PR is ready to merge. Please react with 👍 if you agree or 👎 if you have concerns, and share reasoning if you disagree. Release notes:
|
|
I know that this PR will help a bunch of people I've had conversations with. Especially in the AutoISF fork/branch community where minor things like this can have a pretty big impact on a system with a lot of power to adjust doses. Also for anyone with little people on low doses this could be a real big issue for them too. Appreciate you taking the time to put this together @nl-ruud! From my testing it's been working well for me and I vote it's ready for merge. |
6fbf11f to
e7dec05
Compare
|
Rebased onto dev; no conflicts or changes. |
|
Cherry picked the 3 commits into the latest 3.4.1.0 Master and it builds ok. 1st commit name - Quantify the basil drift Merging the issue-4158-dev-fix branch into a non Dev Master ended up building a Dev build. |
|
Took some time before I could test (was running Medtrum), my test results on deviation for the past 3 days. Code review: afaikt no issues Personal evaluation:
|
|
@MilosKozak This PR has been tested over the past couple of months with very positive feedback, everyone agrees it’s ready to merge. The issue it addresses might seem minor at first glance, but the changes make a real difference in practice. It’s now guarded by a semaphore file, allowing broader testing before it becomes the default. |
|
Got another data point that this PR helps. No bugs or issues present. |
|
Working nicely. |
|
Using the newest " basal drift" has smoothed out control heaps.. and using this PR we just got a 100% TIR week for the first time i can remember; (as well .. other factors have helped) |
PR Review: Omnipod Dash Basal Drift CompensationWell-motivated PR addressing a real and impactful hardware limitation (~10% TDD under-delivery). The safety guards are reasonable and the semaphore-file gating makes it low-risk for non-opt-in users. Here are my findings: Strengths
Concerns1.
|
|
@MilosKozak Thanks for your detailed feedback — much appreciated. I’ve reviewed all points carefully. I’ll add testcases (which address points 1 and 6). For the other items, I’ve considered the feedback and decided that the current approach is intentional. I’d appreciate your thoughts on whether this seems reasonable, or if I might be overlooking something.
I used Short here to stay consistent with totalPulsesDelivered. basalPulsesDelivered and bolusPulsesDelivered are aligned with that type. Even at the maximum value (32767 pulses × 0.05U = 1638.35U), we’re well within pod limits (200U), and both values reset on pod change. I understand the concern about silent truncation from Int → Short, but since totalPulsesDelivered is already a Short, any overflow would originate there first. From that perspective, keeping these fields as Short is consistent. Fully eliminating this risk would require changing the underlying type of totalPulsesDelivered, rather than addressing it only here.
I agree with this point. Properly addressing it would require a broader change to the driver internals, so the current implementation is intentional. At the moment, lastBolus carries a BS.Type, but conceptually it should be a BolusType, only converted to BS.Type when communicating back to AAPS. If you agree with that direction, I’m happy to take it on — but I’d propose handling it in a separate PR to keep this one focused. It would for example also mean a change to the underlying database structure and probably need a data conversion upon upgrade. I will need some help there...
I agree this isn’t ideal. I currently see bolusDeliveryInProgress as a workaround. I expect this concern would be resolved as part of addressing point 3, once basal corrections are given their own dedicated BolusType.
The primary goal here is to prevent overdelivery at all costs. In uncertain situations, I prefer resetting rather than compensating to avoid potential over-correction. For example, when enabling drift compensation after it has been off for a while, the drift can be large — resetting first prevents overdelivery. Even if this occasionally means missing a small compensation, the practical impact is negligible. It’s also worth noting that I haven’t observed this scenario in any real-world test logs. I’m aware the thresholds are set quite tightly, but this was intentional. They can be increased if needed; so far, however, all tests — even with 1-minute loops using AutoISF, where I would have expected to see issues due to high basals and the 2-minute cooldown — have not triggered this scenario.
This file was recently moved to common in preparation for starting work on Omnipod 5 support (#4574). It's even called
My observations:
Thanks again — this is very helpful feedback. I’ll: |
…e for basal drift calculation and correction.
|
|
@MilosKozak I added unit tests for the drift calculation and correction logic, updated the timezone handling in Note: #4574 moved the Dash driver to common. The existing test cases are still under dash, but since common is now the location, I placed the new tests there: testing the new internal methods would otherwise add unnecessary complexity. |
|
For the record: I may have tested the Atlas pods before without realizing it, but I have now explicitly verified that the basal drift is not specific to the SAW pods—it is also present in the Atlas pods. |
DST VerificationAs there was a DST transition today, I verified the behavior of Case 1: Phone time (DST jump)Observation
It also correctly splits the interval at the basal change at 02:00, applying:
Case 2: Pump time (post-DST sync)ObservationThe interval 05:24:30.7 → 05:41:15.1 = 1004.4s is handled correctly, matching real elapsed time. Case 3: Profile transition after DSTObservationThe basal profile change occurs at 06:00, which is correct after the DST adjustment (an incorrect implementation would have shifted this to 07:00.) Conclusion
LimitationThere was no basal/profile transition before the pump timezone update in this dataset, so that specific edge case is not covered here. |















Omnipod Dash basal drift resolution
This pull request introduces a new "basal correction" feature for the Omnipod Dash pump integration. The main goal is to detect under-delivery of basal insulin and automatically trigger a small correction bolus when needed. The implementation tracks delivered basal and bolus pulses, calculates expected vs. actual insulin delivery, and manages correction logic with safety checks and cooldowns.
Recap of the issue
The Omnipod Dash pump exhibits behavior that causes it to deliver less basal insulin than AAPS expects (issue #4158). This is effectively a hardware limitation of the pump.
The Dash uses an internal timer to determine when a basal pulse of 0.05 U is delivered. Once the timer interval elapses, the pulse is delivered. However, this timer is restarted whenever a basal rate change occurs (and possibly also when a bolus or SMB is delivered).
When used in combination with looping, this leads to structural under-delivery of basal insulin, as the algorithm adjusts the basal rate frequently. While the Medtrum pump compensates for this behavior internally, the Dash does not, and neither does the current Dash driver implementation in AAPS.
The issue is most apparent during the night. During daytime operation, SMBs often result in a basal rate of 0, which masks the effect. In observed usage, this results in approximately 10% of the expected Total Daily Dose (TDD) not being delivered over a 24-hour period. Additionally, glucose targets are often not reached overnight, particularly after meals with prolonged glucose impact (e.g. pasta).
Approach
This pull request introduces automatic basal drift compensation by correcting the pump lag with small correction boluses. A correction bolus of 0.05 U is delivered as soon as the detected basal deficit reaches 0.025 U. Several safety constraints are applied. For example, corrections are not delivered during a temporary basal of 0 unless that temporary basal is the direct result of a recently delivered bolus.
With this approach, the deviation between expected and delivered basal insulin is kept within a bounded range of approximately −0.050 U to +0.025 U.
With this fix applied:
Without this fix, a clear discrepancy between reported and delivered TDD can be observed.
Main Changes
Basal Correction Feature
CommandDeliverBasalCorrection, and integrated it into the command queue and handling logic inOmnipodDashPumpPluginto trigger a basal correction bolus when under-delivery is detected.deliverBasalCorrectionmethod to handle the actual delivery of the correction bolus, including safety checks (reservoir level, bolus in progress, cooldowns, etc.).Basal Delivery Tracking and Drift Calculation
OmnipodDashPodStateManagerImplto track basal and bolus pulses separately, compute actual delivered basal, and calculate "drift" (difference between expected and actual basal delivery). This includes new fields in the pod state and methods for integrating expected delivery over time.needsBasalCorrectionmethod determines when a correction is needed based on drift, cooldowns, and other safety criteria.Pod State Update and Logging Refactor
updatePodStatemethod, which also updates basal tracking fields and logs basal delivery/drift for debugging. This method is now used in both default and alarm status response handlers and removes code duplication from these methods.Logging example:
Legend for the log fields:
acttot−bol)totbolexperract-exp)dErrerron this pump status responseInterface and State Enhancements
OmnipodDashPodStateManagerinterface and its implementation to support new fields and methods for basal correction tracking (lastBasalCorrectionTime,basalCorrectionInProgress,needsBasalCorrection).These changes together enable the system to automatically detect and correct small basal under-deliveries, improving insulin delivery reliability and safety.
Tests Performed
To verify the fix, we compared the total insulin delivered at 00:00 at the start of a day with the total at 00:00 at the end of the same day.
Before this fix, the difference consistently resulted in a lower number than the TDD reported by AAPS, confirming under-delivery. After applying the fix, this discrepancy no longer occurs: AAPS TDD matches the pump’s delivered total.