Count the packet types a history offload drops, instead of dropping them silently (#891) - #927
Merged
Merged
Conversation
extractHistoricalStreams handles four of the schema's sixteen packet types and drops the rest at `default:`, and rejectedHistoricalRecords archives only type-47 — non-47 frames are, in its own words, "excluded by construction". So a record type nobody has mapped is dropped twice and counted zero times, and the sync reports clean. That is not hypothetical: HISTORICAL_IMU_DATA_STREAM(52) is a banked raw-stream type our own schema names and the funnel does not handle. Every one would vanish. The immediate use is #891. Whether a 5/MG banks ECG to flash after the Labrador toggles is now the leading hypothesis, and the open ask is whether anyone has seen an unrecognised record type in an offload. Nobody can answer that today: there is no instrument that would show one, so running the experiment cannot produce a finding either way. Streams.unhandledPacketTypes / StreamBatch.unhandledPacketTypes census the types that fall through, and the Backfiller logs each the first time it appears. Transient diagnostics like droppedImplausible: not persisted, not in CodingKeys, empty by default so golden fixtures stay byte-identical. METADATA and CONSOLE_LOGS are excluded — an offload legitimately carries both and they decode to zero rows by design, so counting them would put a scary line in every strap log and train people to ignore the one that matters. Named-but- unhandled types are NOT excluded; those are the interesting ones. PacketType 53-55 were missing from the Kotlin enum, so the same byte would have rendered "type53" on Android and "RELATIVE_PUFFIN_EVENTS" on Apple. Added as decode-only names. 56 stays out deliberately: both platforms alias it onto METADATA. Kotlin dispatches on the raw type byte before any CRC check where Swift skips CRC-failed frames before its switch, so the Kotlin branch checks CRC explicitly — otherwise a corrupt frame would read as an unknown firmware feature on one platform only.
Two gaps found re-reviewing this. The Swift tests hand-built ParsedFrame(ok: true), which assumes the thing most likely to be wrong: extractHistoricalStreams skips !r.ok before its switch, so if parseFrame marked an unmapped type not-ok the census would never fire on a real frame and every assertion would still pass. It does not — spec == nil falls through to ok: true — but that is a fact about the parser and belongs in a test. Added the end-to-end case the Kotlin twin already had. The naming was not family-aware. Swift renders a WHOOP 4.0 frame through schema.typeName (no aliasing) and a 5/MG frame through canonicalTypeName; the Kotlin census always aliased. A 4.0 frame of type 56 would have been counted as PUFFIN_METADATA on Apple and silently EXCLUDED as METADATA on Android — the two platforms disagreeing about an anomalous frame, which is the one case this exists for. Kotlin now picks the rendering by family, PacketType gains 56, and both suites pin it.
The end-to-end Swift case failed with crcOK nil rather than true: the envelope's length field covers the inner record PLUS the 4-byte CRC32 trailer, and both builders declared the record length alone. The parser then cannot locate the trailer, so it never verifies. The Kotlin twin had the identical defect and passed anyway, because crcOk is null (not false) in that state and the census excludes only false. So it was feeding the decoder a malformed frame and asserting on the result. Fixed both.
vipulog
pushed a commit
to vipulog/noop
that referenced
this pull request
Jul 31, 2026
…ce, dormant default-off (ryanbr#477) (ryanbr#478) * feat(ble): GATT connection-priority management, dormant default-off (ryanbr#477) Adds the battery lever coordinated in ryanbr#477: request CONNECTION_PRIORITY_HIGH during an offload burst / live-HR session (a shorter interval → faster sync, can't cause a supervision-timeout drop) and, behind an opt-in idle throttle, LOW_POWER when idle. Ships DORMANT — the master gate defaults off, so refreshConnectionPriority() early-returns and issues zero new BLE ops, leaving the link at the stack default (BALANCED) exactly as today. - Pure connectionPriorityFor(offloadActive, liveHrActive, idleThrottleEnabled) in the companion (the scanModeForReconnectAttempts idiom), unit-tested. - GattOps.requestConnectionPriorityCompat seam (RealGattOps delegate). - Wired at the real transitions: offload begin → HIGH, exitBackfilling → idle, reconcileRealtime → HIGH/idle. Reads the authoritative internal flags. - Android-only by necessity: CoreBluetooth has no app-side connection-priority equivalent, so there is no Swift twin — documented divergence, not a gap. Compiles (:app:compileFullDebugKotlin); ConnectionPriorityTest green. Behaviour change is gated OFF pending on-strap validation per ryanbr#477. * fix(ble): connection-priority hint must not trigger link teardown (ryanbr#478 re-review) refreshConnectionPriority routed the requestConnectionPriority call through safeGatt, whose policy is 'any throw ⇒ teardownAfterGattFailure()'. That's right for load-bearing writes/subscriptions but wrong for a battery HINT: a transient throw would sacrifice the whole connection for an optimization. Swallow locally instead — a dead binder is handled by the next real op. * feat(ble): make the idle LOW_POWER throttle battery-adaptive (ryanbr#477) Instead of a static on/off, the RISKY idle throttle now engages only while the phone is DISCHARGING and at/below a user-selectable battery %% (picker: 10/15/ 20/25/30; 0 = never). Confines the drop-risk to when the user actually wants power saving. Pure idleThrottleActive(batteryPct, charging, thresholdPct) + tests; battery read from the sticky ACTION_BATTERY_CHANGED intent (no persistent receiver), fails SAFE (unknown → 100%%, never throttles). Still dormant behind the default-off master gate; the safe HIGH-escalation half is unaffected. * feat(ble): battery-adaptive periodic-offload cadence (ryanbr#477) Stretch the 15-min periodic offload to 45 min while discharging at/below a user-selectable battery %% (picker 10/15/20/25/30; 0 = off). The offload tick is a pure sync timer — the live-stream keep-alive is a separate mechanism — so this can't affect link health; worst case is data arriving in larger batches (the strap banks to flash meanwhile, no loss). Pure offloadIntervalMsFor(...) + tests. Ships DORMANT: threshold defaults 0, and nextBackfillDelayMs early-returns the normal cadence with ZERO battery reads until opted in. Reuses the ryanbr#478 battery snapshot; complements the connection-priority lever, lower-risk (no drop risk). * perf(ble): skip the battery read in safe-half-only mode (ryanbr#478 re-review) refreshConnectionPriority read the battery on every transition even when the idle throttle was off (idleThrottleBatteryPct == 0), where the result is always false. Short-circuit so the SAFE HIGH-escalation half issues no battery read — mirrors the nextBackfillDelayMs dormancy fix. * feat(ble): respect Android Battery Saver in the battery-adaptive levers (ryanbr#477) Both levers (idle LOW_POWER throttle + offload-cadence stretch) now also engage when the OS Battery Saver is on — the user's explicit 'save power' signal, with its own hysteresis + charging-awareness. Battery Saver is an OR-WITHIN an already-armed lever (threshold > 0): it can trigger a lever you opted into at any battery level, but it can NOT force the risky idle throttle against a deliberate 'off' (threshold 0), respecting the drop-risk asymmetry. powerSave threads through the pure gates (idleThrottleActive / offloadIntervalMsFor) + tests; read only when a threshold is armed, so still dormant by default. * feat(ble): optional pause of continuous-HRV capture under Battery Saver (ryanbr#477) A separate default-off toggle: while the OS Battery Saver is on, release the held-open BACKGROUND continuous-capture realtime stream (one of the larger live drains). A visible Live screen is unaffected (screenWantsRealtime is separate), and it re-arms automatically when Battery Saver turns off. Rides the existing ryanbr#927 window-gating machinery: the gate lives in continuousCaptureWantsNow(), which the 30s keep-alive tick already re-derives and arms/disarms on the edge — so no new receiver or re-arm path, and no stuck-paused risk. Dormant by default (pauseCaptureOnPowerSave=false short-circuits the PowerManager read). setPauseCaptureOnPowerSave reconciles immediately. * feat(ui): Settings — Power saving section (battery % + HRV pause) (ryanbr#477) Wires the two BENIGN battery levers to a user-facing Settings section, so the dormant mechanism is now reachable: - 'Power saving mode' toggle + a 10/15/20/25/30 %% picker → the offload-cadence stretch (setLowBatteryOffloadThrottle). Default off / 20%. - 'Pause HRV capture in Battery Saver' tick box → the continuous-capture pause (setPauseCaptureOnPowerSave). Default off. Persisted in NoopPrefs; AppViewModel.applyPowerSaving() pushes them at launch and on every change. The riskier connection-priority idle throttle is deliberately NOT surfaced — it stays dormant pending on-strap validation (ryanbr#478). Compiles (:app:compileFullDebugKotlin). * feat(ui): use a slider for the power-saving battery threshold (ryanbr#477) Swap the 10/15/20/25/30 pill picker for a stepped Slider (10-30%, snapping to 5% increments) with a live '20%' read-out. Persists on onValueChangeFinished (not every drag frame). Default stays 20%. * feat(ui): HRV pause is a power-saving sub-option, on by default (ryanbr#477) Make the HRV-capture pause a sub-option of the Power saving master, not an independent toggle: it shows only when Power saving is on, defaults ON (pref default flipped to true), and stays user-disableable. Effective HRV pause is now master && pref, so turning Power saving off disables it too. Resolves the master/sub-toggle inconsistency from re-review. * l10n(de): localize the Power saving Settings section (ryanbr#477) Wire the 7 new Settings strings to R.string.* + German translations, so the ryanbr#452 i18n gate passes (it correctly failed the new hardcoded literals). HRV=HFV per the existing ryanbr#451 vocabulary; %1$d%% readout keeps the format specifier.
ryanbr
added a commit
that referenced
this pull request
Aug 1, 2026
…f) (#1019) * HRV: default Overnight only ON for fresh installs (#1008, minimum half) WHOOP publishes no daytime HRV figure - its reading is an overnight one - so a 24/7 stream has no official-app analogue, and the setting's own copy says overnight-only roughly halves the battery cost. Turning on "Continuous HRV capture" currently gives you the expensive, non-WHOOP-like behaviour unless you separately find "Overnight only", which is the wrong way round. EXISTING USERS ARE UNCHANGED. The unset case resolves from whether the base Continuous HRV key exists: present means the user has been through this screen and experienced always-on, so they keep it; absent means a fresh install, which gets overnight-only. That preserves the deliberate choice the #927 comment records ("Default OFF, so existing Continuous HRV users keep the always-on behaviour with no migration") while changing what new users get. Resolved at read time rather than by writing a migration, because the only thing that must not happen is silently narrowing capture for someone relying on it for daytime Stress - and a migration that runs at the wrong moment does exactly that. An explicit choice always wins in both directions, including an explicit OFF on a fresh install. This is the SAFE half of #1008. The other half - defaulting the HRV window to DEEP_SLEEP to match WHOOP's reading - is deliberately not here: it moves every existing HRV number and its baselines, and it depends on deep-sleep staging quality that is still unsettled on 5/MG. That needs SleepPSG evidence first. Rule lifted into continuousHrvOvernightDefault on both platforms so it is testable without a Context or UserDefaults. Four twin tests each; the Kotlin ones run in CI, the Swift ones live in StrandTests which app-build.yml would run if it were enabled. * Fix the default flip: decide once at launch, not on every read The first version of this resolved the default at READ time, keyed on whether Continuous HRV had ever been enabled. That fact is created by the user's own opt-in, so a fresh install read overnight-only ON and then flipped to OFF the moment they enabled Continuous HRV - the exact opposite of the intent. State-by-state tests could not see it: every individual state was correct, and only the sequence was wrong. Replaced with a one-time launch migration on both platforms: an install that has used Continuous HRV and never chose an overnight setting is pinned to the old OFF; everything else is left alone and takes the new ON default. Taken before the user can reach either toggle, so the inputs cannot move under it. A @AppStorage onChange hook cannot substitute on iOS - @AppStorage writes the value BEFORE the handler runs, so a first-ever toggle is indistinguishable from any other by then. That asymmetry is why this is a launch migration rather than something wired to the toggle. The extracted rule now describes the MIGRATION decision rather than the read, because the read is simply getBoolean(key, true) - the previous extraction had become dead code describing the broken design, and its tests were exercising that dead code rather than production. Five twin tests each, including the sequence that broke it and idempotence. * Remove the Swift copy of the rejected read-time rule The replacement added shouldPinLegacyOvernightDefault but left continuousHrvOvernightDefault behind on the Swift side - dead code whose doc describes the design this PR abandoned, and points at a Kotlin twin that no longer exists. Worse than ordinary dead code: it documents the read-time approach as though it were current, which is the specific mistake the migration exists to prevent someone repeating. Found by grepping for stale references after the rewrite; nothing referenced it, so removal is inert. * Match the iOS toggle's default to the behaviour it controls Changing the read default left SettingsView's @AppStorage on false, so a fresh install would show "Overnight only" OFF while capture was actually overnight-only. They read the same key by different routes. The failure mode is worse than a wrong label: a user "correcting" the toggle by flipping it on and off would write an explicit false and end up with the 24/7 behaviour they were trying to avoid. Android was never affected - its toggle reads through NoopPrefs.continuousHrvOvernight, so it cannot disagree with what the BLE client acts on. Its stale "Default OFF" comment is corrected too. Checked the rest of SettingsView for the same class: journalReminderEnabled and experimentalSleepV2Enabled are the only other true-default toggles, and both already pair with an accessor that handles the unset case - the first with the exact `object(forKey:) as? Bool ?? true` spelling used here. So this now matches how the codebase already solves it, and no other instance exists.
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
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.
Read-only. No new opcode, no hardware write, no storage, no strings.
The gap
A history offload silently discards any packet type the decoder has no case for:
extractHistoricalStreamsswitches on four of the schema's sixteen packet types anddefault: continues the rest — no counter, no log.rejectedHistoricalRecordswill not catch it either. It is gatedframe[typeIndex] == 47, and itsown comment says non-47 types "never pass this gate — they are excluded by construction".
So an unmapped record is dropped twice and counted zero times, and the sync reports clean.
Not hypothetical:
HISTORICAL_IMU_DATA_STREAM(52)is a banked raw-stream type our own schemaalready names and this funnel does not handle. Every one would vanish.
Why now
#891's leading hypothesis (b) is that the 5/MG banks Labrador ECG to flash rather than streaming it,
and the open ask is "has anyone seen an unrecognised record type or buffer type in a 5/MG offload?"
Nobody can answer that today. There is no instrument that would show one, so running the
experiment cannot produce a finding in either direction — a negative would be indistinguishable from
not looking. This is the instrument.
It is worth having regardless of how #891 resolves: any future firmware record type is invisible right
now, on both platforms.
What it does
Streams.unhandledPacketTypes/StreamBatch.unhandledPacketTypescensus the types that fall through,keyed by rendered name. The Backfiller logs each the first time it appears — a 30k-record offload
must not emit 30k lines — naming the type and asking for a report on #891 if it is unfamiliar.
Transient diagnostics exactly like
droppedImplausible, whose pattern this follows: not persisted, notin
CodingKeys, excluded fromisEmpty, empty by default so golden fixtures stay byte-identical.METADATAandCONSOLE_LOGSare excluded. An offload legitimately carries both and they decode tozero rows by design; counting them would put a scary line in every strap log and train people to ignore
the one that matters. Named-but-unhandled types are not excluded — those are the interesting ones.
The exclusion set is pinned by a test on both platforms, because quietly adding to it is how this stops
reporting the thing it was built for.
Two parity defects found while writing it
PacketType53-56 were missing from the Kotlin enum, so the same byte would rendertype53onAndroid and
RELATIVE_PUFFIN_EVENTSon Apple — one strap, two different report lines in a diagnosticmeant to be pasted into an issue. Added as decode-only names; nothing dispatches on them.
schema.typeName(no aliasing) and a 5/MG frame throughcanonicalTypeName; the Kotlin census alwaysaliased. A 4.0 frame of type 56 would have been counted as
PUFFIN_METADATAon Apple and silentlyexcluded as
METADATAon Android — the two platforms disagreeing about an anomalous frame, which isthe single case this exists for. Kotlin now picks the rendering by family, and both suites pin it.
before its switch. Left alone, a corrupt frame would have been reported as an unknown firmware feature
on Android and not on Apple. The Kotlin branch now checks CRC explicitly — a CRC failure is a
different finding with its own archive.
Framing.typeNamewentprivate->internalso both the census and the frame labeller render throughone copy of the aliasing rules rather than two that can drift.
Verification
distinct types tallied separately, METADATA/CONSOLE_LOGS not counted, the exclusion set pinned, a
normal offload producing an empty census, a CRC-failed frame not counted, the census absent from the
Codable path, both puffin types keeping their own names on 4.0, and packet-type names matching across
platforms byte for byte.
assumes the thing most likely to be wrong: the decoder skips not-ok frames BEFORE its switch, so if the
parser marked an unmapped type not-ok the census would never fire and every other assertion would still
pass. Writing that test found a second defect — both builders declared the envelope length as the inner
record alone, where it covers the record PLUS the 4-byte CRC32 trailer, so the parser could not locate the
trailer and never verified. Kotlin passed anyway (
crcOknull, not false); Swift caught it. Both fixed.Tools/doc_comment_lint.pyclean.HistoricalStreams.kt,Enums.ktandFraming.ktcompile clean locally (Gradle itself doesnot run on this arm64 host —
aapt2is x86-64 only — so this was the embeddable compiler over theprotocol+datapackages; remaining errors in that partial compile are packages I did not pass it).Android CI is the real gate.
Strand/Collect/Backfiller.swiftis app-target Swift, which nothing compiles whileapp-build.ymlis disabled. The package half is covered by
swift-packages. The Backfiller hunk is aforover adictionary and a log call, but it is uncompiled and I would rather say so.
Where it is surfaced, and where it is not
Verified reachable rather than assumed: there is no early return between decode and the log on either
platform, which matters because a chunk of ONLY unmapped records has
isEmpty == trueand anempty-chunk shortcut would have made this inert in exactly the case it exists for. Both
logsinks arewired in production (
BLEManager/WhoopBleClient), so the line reaches the strap log a user can paste.Not surfaced on Android's capture-replay path.
CaptureImportercalls the same decoder, so thecensus is computed and sits on the
StreamBatch, but that caller does not log it. The #891 experiment isa live offload, so the path that matters is covered — noting it rather than leaving it to be discovered.
One judgement call worth flagging
RELATIVE_BATTERY_PACK_CONSOLE_LOGS(55)is left in the census rather than excluded with the otherconsole type. If it turns out a 5/MG carries it routinely in an offload, it becomes noise and wants a
one-line addition to the exclusion set — but there is no evidence in-tree that it appears at all, and
over-reporting once is recoverable in a way that never reporting is not.
Refs #891.