fix: stop rejecting fresh CamAPS readings during pump-only Attempting#231
Merged
Conversation
PR #222 added a per-package keyword filter that rejected any CamAPS notification containing the status word "Attempting", on the premise that it meant the sensor was offline and reposted values were stale. That premise was wrong. "Attempting" in CamAPS is the AID loop's "trying to reconnect" state — it fires for both CGM-loss AND pump-loss-only. During a pump-only event the CGM is fine and reposts carry fresh, continuously-changing values. The keyword filter rejected all of them, leaving Strimma stuck on "Waiting for glucose data..." while CamAPS was visibly showing live BG. Replace the keyword filter with a value-pattern heuristic that keys off the actual differentiator: drop a parsed value only when it equals the last forwarded SGV AND a "---" notification from the same package was seen within the last 10 min. Suspicion clears on any different SGV; a long stable plateau without "---" is never blocked. Tests cover both cases the old filter conflated: - pump-only Attempting with continuously changing values → all pass - May-11 alternation pattern (---/5,9/5,9/---/5,9) → reposts dropped Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Extract the per-package state into a dedicated `StuckValueDetector` class: - **Clear suspicion on accept** — recordAccept() wipes lastNoValueAt so a legitimate plateau on the recovered value is admitted. The previous inline implementation only updated lastForwardedSgv, leaving stale suspicion that would silently drop real same-value reads after a recovery and trigger a false stale-data alert (medical-safety bug). - **Monotonic timing** — the in-memory window comparison uses SystemClock.elapsedRealtime, immune to wall-clock changes (NTP corrections, manual time changes that previously could turn a 1-min glitch into an indefinite suspicion window). - **Persistence across rebinds** — wall-clock no-value timestamps are written to SharedPreferences and re-hydrated on detector creation, validated against a 30-min freshness cap. NotificationListenerService rebinds (boot, app update, memory pressure) no longer wipe the detector at exactly the moment it matters most. - **Cleaner logging** — every stuck rejection logs (the previous per- (pkg, sgv) loggedRejections key permanently silenced repeats of the same value). Also in this commit: - Use Math.round(mgdl).toInt() to match ReadingPipeline's storage rounding (prevents future trap where listener and DB disagree on what "same value" means). - Hoist the sawNoValueMarker recording above the early return so a notification carrying both --- and a parseable value still arms suspicion. - Add KDoc explaining the design tradeoffs the agents flagged: 10-min window is sensor-recovery time (not sample-cadence), and the global CGM-package scope rests on "---" being a near-universal CGM idiom. - Add `StuckValueDetectorTest` (pure-logic, injected clocks + real SharedPreferences) covering window expiry, clear-on-accept, wall- clock immunity, and persistence rehydration. - Add listener-level tests for after-recovery plateau admission and extras-only "---" arming the detector. - Drop the user-name reference from the test KDoc per the privacy rule. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Android lint UseKtx flagged the chained edit().apply() pattern. Switch
to the KTX `edit { }` block, matching the existing convention in
SettingsRepository, AlertManager, and WidgetSettingsRepository. I missed
running ./gradlew lintDebug locally before pushing.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ehavior The CamAPS FX user manual (modes of operation, page 16; failure modes, chapter 12) confirms that "Attempting" is the AID-loop's "trying to enter Auto mode" state, with six possible triggers including pump comms loss, pump-suspended delivery, Bluetooth off, and sensor data unavailable. Crucially, when sensor data is unavailable the value field shows "---"; a numeric value being shown means the CGM is currently delivering. The May-11 log pattern that PR #222 was built around (--- / 5,9 / 5,9 / --- / 5,9) was therefore a sensor blipping in and out during a real BG plateau at 5.9 mmol — every 5,9 was a fresh reading. There is no "stuck-value reposting" behavior to defend against. Drop StuckValueDetector + its tests. Trust the parser: parseable values forward to the pipeline, "---" fails to parse and is silently dropped. ReadingPipeline's per-bucket dedup continues to absorb same-bucket reposts as before. Math.round(mgdl).toInt() in the listener log line matches the pipeline's storage rounding so the debug output equals what gets stored. The new AttemptingNotificationTest covers the regression that motivated this PR: Attempting + numeric value forwards (Per's pump-loop bug) and Attempting + "---" is naturally dropped by the parser. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
psjostrom
changed the title
7 tasks
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.
Summary
PR #222 added a per-package mode-word filter that rejected any CamAPS notification containing the status word "Attempting", on the premise that it indicated stale sensor data. The CamAPS FX user manual (Chapter 2, "Modes of Operation", page 16) shows that premise was wrong:
"Attempting" is the AID-loop's "trying to enter Auto mode" state. It fires for any of six conditions — only ONE of which is sensor unavailability, and even then the value field shows
---(per Chapter 12 failure modes — when no CGM data is arriving, CamAPS shows explicit "Signal loss" / "No Readings" / "Sensor error" alerts and---in place of a number).Crucial corollary: a numeric value displayed in the notification means the CGM is currently delivering. The mode word "Attempting" alongside a numeric value just means the loop is in a degraded state for one of the OTHER five reasons (most commonly pump comms).
Captured today on a pump-loop event:
CGM trending up by 0.6 mmol over four minutes — clearly fresh data. Strimma showed "Waiting for glucose data..." for the entire pump-loop event because every notification was filtered out.
Re-examining PR #222's evidence
PR #222 cited the May-11 alternation pattern
--- / 5,9 / 5,9 / --- / 5,9and labelled the 5,9 readings as stale reposts. The manual contradicts that interpretation:---= sensor genuinely not delivering (one of the listed Attempting triggers)5,9= sensor delivered 5.9 mmol/L — a real readingThere is no documented "show last-known value as fresh" behavior in the manual. Each numeric value in a CamAPS notification represents the current sensor reading.
Fix
Remove the stale-value filter entirely. No replacement heuristic — the May-11 pattern was a misdiagnosis, not a bug class to defend against.
What stays:
---(no regex match), so "no current data" notifications are silently dropped.ReadingPipeline's per-bucket dedup continues to absorb same-bucket reposts (visible in logs as "Dedup-rejected: sgv=…" lines).Math.round(mgdl).toInt()so the value reported as "Parsed: …" equals whatReadingPipeline.processReadingrounds to and stores.Three deletions, one addition:
STALE_STATUS_KEYWORDS_BY_PACKAGEmap andisStaleStatusNotification()(PR fix: drop stale CGM notifications when source app is in degraded state #222's filter)StaleStatusFilterTestandStaleStatusListenerIntegrationTest(tests for the removed filter)AttemptingNotificationTest— Robolectric coverage for the regression: Attempting + numeric value forwards; Attempting +---is naturally dropped by the parserThe PR briefly contained a value-pattern detector (
StuckValueDetector) as a "smarter" replacement, but reading the manual proved that even the smarter version would suppress legitimate readings whenever a transient---(sensor blip during a real BG plateau) coincided with a stable value. Removed it before merge.Test plan
./gradlew testDebugUnitTest— full suite green./gradlew detekt— clean./gradlew lintDebug— clean---, and the existing stale-data alert fires after the configured interval🤖 Generated with Claude Code