Read reservoir before bolusing - #2
Merged
Merged
Conversation
awaxa
pushed a commit
to awaxa/Loop
that referenced
this pull request
Jul 26, 2017
Bug: RL is now in section 2
ps2
pushed a commit
that referenced
this pull request
Dec 13, 2017
florianpreknya
pushed a commit
to florianpreknya/Loop
that referenced
this pull request
May 31, 2019
Omnipod testing
dthornley
pushed a commit
to dthornley/Loop
that referenced
this pull request
Oct 9, 2019
…5f4d96a2bbeef7 parent a4568c4 author Cal Young <cyoung1024@gmail.com> 1565904731 +0200 committer David Thornley <david.thornley@darnt.com> 1570653504 +0200 # This is a combination of 6 commits. # This is the 1st commit message: Add Spike Client and change version # This is the commit message LoopKit#2: Add Spike as CGM source # This is the commit message LoopKit#3: Add Spike frameworks # This is the commit message LoopKit#4: Add Spike to LSApplicationQueriesSchemes # This is the commit message LoopKit#5: Update Cartfile.resolved # This is the commit message LoopKit#6: Add Spike Calibration guide # This is the commit message LoopKit#11: Fixed delay between Spike update and Loop update
ardillen
pushed a commit
to ardillen/Loop
that referenced
this pull request
Oct 21, 2020
Merge Upstream
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
May 12, 2022
…n-issue-report Show profile expiration in issue report
bjorkert
pushed a commit
to bjorkert/Loop
that referenced
this pull request
Jun 10, 2023
Renaming, Logging, View changes
threecee
pushed a commit
to threecee/Loop
that referenced
this pull request
May 2, 2026
…LoopKit#2) Until now the four HandoffPolicyEngine state hooks (markCurrentOwner / markPhoneStableSince / markUserInteractedAt / markCachedPodStateAge) were called only from tests. In production they were dead code, which meant: - phoneStableReachableSince was always nil, so the auto-revert path in (.watch, .automatic) and (.watch, .manualWithAutoRevert) was unreachable. Once the watch took over, only manual user action returned control. - cachedPodStateAt was always nil, so the cached-pod-state safety gate treated any payload age as "fresh" — defeating the whole point of the freshness check. This commit wires both sides: - markCurrentOwner — called from execute(.notifyUI(state)) on every state transition + at start() with the initial state. Owner is derived via the new HandoffState.currentOwner extension (in OmniBLE). - markPhoneStableSince — Combine subscription to coordinator's $isCounterpartReachable, with a 60s debounce (matches the engine's absenceThreshold) on flip-on, immediate clear on flip-off. - markUserInteractedAt — called in userRequestHandoff(to:). - markCachedPodStateAge (watch-side only) — called after ownership.cachePayload(decoded) in handleIncoming(.pairingHandoff). BEHAVIOR CHANGE: auto-revert from watch back to phone WILL FIRE in .automatic and .manualWithAutoRevert modes when the phone reconnects after being out of range for >=60s. This is the original B.3.a design intent; today it's dead code. Worth deliberate hardware testing. Adds wiring tests on both sides using a real HandoffPolicyEngine (not a mock) to verify the wiring actually propagates state. Adds debug<X> internal accessors on HandoffPolicyEngine (currentOwner, phoneStableReachableSince, lastUserInteractionAt, cachedPodStateAt) for @testable inspection. Adds a test-only phoneStableDebounceOverride init parameter on HandoffOrchestrator (defaults to 60s) so the reachability-debounce test can run in <1s. threecee-claude
threecee
pushed a commit
to threecee/Loop
that referenced
this pull request
May 2, 2026
…wiring Three nits from code review of 93a4ddf, all mechanical: 1. Renamed `phoneStableDebounceSeconds` static constant to `defaultPhoneStableDebounceSeconds` so it stops shadowing the instance let of the same name. Both names existed (compile-legal thanks to `Self.` qualifier) but readers had to look twice. 2. Renamed the 4 `debug<X>` test-observability accessors on HandoffPolicyEngine to `<X>ForTesting` (Apple convention). The `debug` prefix wrongly suggested they could be #if DEBUG-gated and stripped from release builds — they cannot. Both iOS and watch sides. 3. Added a 2-line comment in handleReachabilityChanged() explaining why the inner `if self.coordinator.isReachable` re-check inside `await MainActor.run` is necessary after the `Task.isCancelled` guard. It defends against the actor-hop ordering window where reachability flipped off during the sleep but the off-callback hasn't been processed yet. Without the comment a future reader could "fix" the apparent redundancy and break the safety property. threecee-claude
threecee
pushed a commit
to threecee/Loop
that referenced
this pull request
May 2, 2026
Four fixes from code review of 38102e1: CRITICAL — Add gate 5: pumpManager.status.deliveryIsUncertain. iOS DeviceDataManager.swift:1416 has this guard to prevent firing a dose on top of an in-flight unconfirmed dose (e.g., last command's ack was lost or BLE reconnect happened mid-bolus). The watch driver was missing it, which meant we could double-dose under transient pump comms failures. The new gate is the ONE suppression path that does NOT call completion(nil). delivery-uncertain is transient; we WANT the algorithm to retry next tick when pump state may have settled. Returns LoopError.connectionError so LoopAlgorithmRunner does NOT clear its cached recommendation — it'll come back next tick. Function header comment updated to acknowledge gate 5's exception to the "completion(nil) is load-bearing" rule. IMPORTANT — RecordingPumpManager: change static var → static let on constants (pluginIdentifier + onboarding* properties). Mutation across tests would silently couple them. With static let, the compiler enforces immutability. IMPORTANT — makeSuppressedDecision now sets decision.controllerStatus from the controllerStatusAdapter (held as new instance property). Per Phase 1 discovery point LoopKit#4: suppression decisions should have the same battery / charging context as non-suppressed ones (analytic parity). IMPORTANT — TODO comment in enactRecommendedDose pointing at iOS's crashRecoveryManager.dosingStarted/Finished bracketing (DeviceDataManager.swift:1423). Watch has no equivalent; partial mitigation via OmniBLEPumpManager's own dose history. Future B.7 phase should add a proper crash-recovery layer. 7th unit test added: testDidRecommend_deliveryIsUncertain_returnsError- AndDoesNotEnact. Verifies the new gate returns a retryable error (non-nil) AND doesn't record a suppressed decision (the retry semantic is "try again next tick" not "this dose is suppressed"). threecee-claude
threecee
pushed a commit
to threecee/Loop
that referenced
this pull request
May 2, 2026
Two trivial follow-ups from B.5 code reviews: - WatchSettingsCache.resetForTesting() now wrapped in #if DEBUG. Previously the test-only helper was unconditionally available; the doc comment said "Production never calls this" but didn't enforce it. Compile-time gate prevents accidental production calls. - Em-dash -> semicolon in watch split-brain log message. Aligns with project writing style guidance avoiding em-dashes in log strings. Audit of B.5.1 LoopKit#2 (do synced ISF/CR/target-range/maxBolus/ suspendThreshold flow into watch algorithm decisions?) confirmed YES — WatchAlgorithmDriver.init at lines 89-122 maps every PhoneWatchSettingsSync field into settingsSnapshot.loopSettings. No code change needed. Two larger B.5.1 items deferred to B.5.2: - Add timeZone to PhoneWatchSettingsSync (protocol bump 3 -> 4 + both sides plumbing + watch BasalRateSchedule construction adopts phone's zone instead of TimeZone.current). - Widen commandsAllowedCheck gate to runTemporaryBasalProgram / cancelBolus / suspendDelivery (each needs a semantically appropriate "blocked" completion response). threecee-claude
threecee
pushed a commit
to threecee/Loop
that referenced
this pull request
May 4, 2026
…ngle source Closes the two B.7 follow-ups deferred from the 2026-05-02 algorithm- fidelity audit (see ~/Projects/loop-and-learn/.claude/skills/algorithm- fidelity-audit/references/known-deviations.md "Code smells" section). Issue LoopKit#1 — enabledEffects duplication: - Adds canonical `static let allEnabled: PredictionInputEffect = .all` in LoopAlgorithmCore/PredictionInputEffect.swift. - LoopSettings.enabledEffects (iOS, Loop/Models/LoopSettings+Loop.swift) now delegates: returns `PredictionInputEffect.allEnabled` when `LoopConstants.retrospectiveCorrectionEnabled` is true (today: always), else `.allEnabled.subtracting(.retrospection)`. - LoopSettings.loopAlgorithmCore_enabledEffects (LoopAlgorithmCore, LoopAlgorithmRunner.swift) now returns `PredictionInputEffect.allEnabled`. Issue LoopKit#2 — PredictionInputEffect.swift deduplication: - Removed Loop/Models/PredictionInputEffect.swift (was on disk, but Phase 1 discovery confirmed it was already a true orphan with no pbxproj entry — the dual-target wiring described in the known-deviations note had been silently fixed previously, both PBXBuildFile entries already point at LoopAlgorithmCore/ PredictionInputEffect.swift). - LoopAlgorithmCore/PredictionInputEffect.swift remains the canonical source, wired into both LoopAlgorithmCore + LoopAlgorithmCore-watchOS framework targets. - iOS callers (LoopDataManager, PredictionTableViewController, LoopSettings+Loop, BolusEntryViewModelTests) all already `import LoopAlgorithmCore`; no callsite changes needed. Tests: - New LoopTests/PredictionInputEffectTests.swift with 3 invariant tests: `allEnabled == .all`, iOS `enabledEffects == allEnabled`, LoopAlgorithmCore `loopAlgorithmCore_enabledEffects == allEnabled`. - Wired into LoopTests target via xcodeproj gem (B85 UUID prefix). - Full LoopTests run: 303 tests, 1 skipped, 0 failures. - Full WatchApp ExtensionTests run: 121 tests, 0 failures. Phase 1 finding: pbxproj surgery was NOT needed (the prior orphan wiring described in known-deviations was already corrected). Phase 3 ruby script ran as a verification gate only — no pbxproj edits beyond the new test file's wiring. threecee-claude
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
Jul 10, 2026
…ts on temp-over-temp) Changing an active temp from the watch faulted bench pod LoopKit#2 terminally (0x31 -> critical fault 049): PodProofKit.setTempBasal programmed a temp while one was running. Stock OmniBLE always safe-cancels a possibly-running temp first (enactTempBasal, OmniBLEPumpManager.swift:2104-2136) and guards against temping during a bolus or while suspended; the facade skipped all three because the EMULATOR accepts overlapping temps. setTempBasal now mirrors the stock idiom: cancelDelivery(.tempBasal) first (plumbing, not journaled; the new .tempBasal event closes the prior segment in the net-basal math), then the bolusing/suspended guards, then the program. Full incident + pod LoopKit#3 test matrix in WATCH_LOAN_TESTING_BUGS.md BUG-6. Build-verified: PodSDK + full LoopWorkspace scheme. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
Jul 10, 2026
…p basal input guards Three independent code audits of the remaining command paths for the 0x31 class that killed bench pod LoopKit#2 (suspend / resume / zero-rate temp). Verdicts: suspend is structurally safe (0x1f stop, not a program; stock suspends over temps and mid-bolus); resume is safe on every watch-reachable path because the vendored setBasalSchedule already cancel-alls unless certain the pod is suspended; dialing 0 routes to suspend() and never reaches the temp path. Hardening the two residuals found: - resume() now does a fresh getStatus before resumeBasal so the driver's cancel-skip decision rests on live pod state, not local mirrors (closes the dual-controller stale-suspend hole; belt-and-suspenders on top of the formal handoff's single-writer contract). - setTempBasal snaps rates to the 0.05 pulse grid (negatives clamp to a legal zero temp instead of trapping in UInt16 encoding) and refuses durations outside Pod.supportedTempBasalDurations — off-grid inputs made the 0x1a table and 0x16 extra command encode inconsistently, a pair stock can never emit and the emulator would silently accept. Not UI-reachable today; closed as pure defense-in-depth. Full audit written up under BUG-6 in WATCH_LOAN_TESTING_BUGS.md. 7 new guard unit tests; PodSDK suite 150/150. Workspace scheme build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
Jul 16, 2026
…asal — maxTempBasalRate computed from settings, pod proof limit set to match before each temp (no more TEMP-TEST-CAP 3.0, no revert chore, no display/enact mismatch). (LoopKit#4) IOB clamp now EXACT phone parity: min(rate, (maxBolus*2 - IOB)*2 + scheduledBasal) floored at 0, matching DoseMath additionalActiveInsulinClamp — replaces the cruder hold-at-schedule
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
Jul 17, 2026
manualSuspendUntil was mutated optimistically before the command was even sent: a busy/phase-guard drop (fileLog only) or a definitive pod refusal left the HUD showing Suspended and the closed loop blocked for up to 3 h while the pod kept delivering (review C4); mirror image on Resume/Set Basal (cleared upfront, showing delivery running while the pod stayed at zero). runPodCommand now reports acceptance and takes an onCertainFailure rollback: manual suspend/resume/setBasalRate capture prior state, apply the new state only when the command is accepted (optimistic in flight, matching the dose screens), and restore it on a definitive refusal. UNCERTAIN outcomes keep the rider's intent - the existing may-have-applied alert fires. Busy-drops of MANUAL commands are now loud (retry haptic + "Not Sent - pod is busy" alert) instead of a silent log line; this also makes the crown-confirmed meal-bolus drop visible (part of high finding on runPodCommand's silent guard). Loop-issued enacts/status refreshes keep quiet drops (they retry on their own cadence). Approved by Jeremy (fix batch LoopKit#2). watchOS build verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
Jul 17, 2026
…niature New WatchApp Extension/StockLoop/ (app code, no PODLOAN needed): - WatchLoopManager.swift (801 lines): mirrors the phone's stock policy paths method-for-method, cited inline — loop()/update(for:) effect refresh over the same GlucoseStore/CarbStore/DoseStore entry points, recency gating via LoopCoreConstants.inputDataRecencyInterval/futureGlucoseDataInterval, retrospective correction, LoopMath.predictGlucose, and recommendation via DoseMath.recommendedTempBasal with additionalActiveInsulinClamp (= maxBolus×2 − IOB) passed INSIDE the call — never post-hoc. Manual bolus via the recency-validated path, no fabricated glucose placeholder. WatchDoseEnactor mirrors DoseEnactor over stock PumpManager methods. The enact seam is typed against the stock PumpManager protocol (the M2 OmniPumpManager) but UNCONNECTED: pumpManager stays nil, and a nil seam yields an explicit error, never a silent success. Missing settings deny dosing — no fabricated defaults. Four TODO(M5-ruling) markers where the work ends at owner-reserved dosing-semantics decisions (LoopKit#1, LoopKit#2, LoopKit#4). - StockLoopStack.swift (125 lines): the single assembly point — constructs stores + the M3 CGM stack and wires G7CGMManager.cgmManagerDelegate → WatchLoopManager → GlucoseStore → loop(). UNINVOKED in M4: construction + compile proof, no behavior change to the stock watch app. Scaffolding retired (purpose absorbed): - StoreBringup.swift (M1) deleted — store construction lives verbatim in StockLoopStack.makeStores(), caveats carried along. - G7TransportBringup (M3) deleted from G7ClientTransportAdapter.swift — assemble() builds the same stack and adds the delegate wiring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
Jul 18, 2026
…tingsManager exists (stock bug) The build-77 pre-unlock crash (symbolicated: AppDelegate.didRegister... → LoopAppManager.remoteNotificationRegistrationDidFinish, SIGTRAP) is the SIBLING of the resetLoopManager crash (e4e347f): an async launch callback touching an IUO manager that only exists after the protected-data gate. registerForRemoteNotifications() ran in initialize() (pre-unlock). On a pre-first-unlock launch, checkProtectedDataAvailable() defers the launch so launchManagers() — which assigns `settingsManager` (SettingsManager!) — never runs; the async push-token callback then force-unwraps nil settingsManager at LoopAppManager.swift:340. (Line 326 already optional-chains settingsManager? — 340 was the missed sibling.) Reliably hit because the always-on watch app relaunches Loop pre-unlock at every boot. The crashing code is pure STOCK (verified byte-identical to origin/dev: settingsManager! decl, the registration block, line 340). We did not author it; our watch project's pre-unlock relaunch surfaces a latent base-Loop bug — same class as e4e347f. Fix: move the registration block into launchManagers() after settingsManager is wired, so the token callback always has a live settingsManager (remote commands can't be serviced pre-unlock anyway). Upstream-candidate stock fix, NOT a watch-feature change; commented as such. Closes the boot-crash saga: it was TWO stacked pre-unlock IUO crashes on different async paths (resetLoopManager + settingsManager), not one. Diagnosis confirmed by symbolication (build-77 dSYM UUID match). iOS build verified. Approved by Jeremy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jeremybarnum
pushed a commit
to jeremybarnum/Loop
that referenced
this pull request
Jul 18, 2026
…er (upstream candidates) Building the watch feature surfaced two latent base-Loop launch crashes — the code is byte-identical to upstream/dev; our always-on watch app just relaunches Loop pre-first-unlock on every boot, exposing them. Both are the same shape: an async launch callback force-unwraps an IUO manager that launchManagers() (which a pre-unlock launch defers) hasn't assigned. LoopKit#1 resetLoopManager: askUserToConfirmLoopReset() optional-chained. LoopKit#2 settingsManager: registerForRemoteNotifications() moved from initialize() to launchManagers() so the push-token callback has a live settingsManager. Marked STOCK LAUNCH-CRASH FIX in-code (NOT PODLOAN — not part of the watch module). Upstream candidates; Jeremy submits the PRs separately. Documented in M4_NOTES. Full-workspace build verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #1.