Repository navigation
Alarm: log live HR at arm time, to test a morning/evening hypothesis (#34) - #707
Merged
Merged
Conversation
…yanbr#34) Log-only, no behaviour change. ryanbr#34's remaining open question: since v9.0.0, morning wake-up and morning short-horizon alarm tests fire reliably, but an identical evening short-horizon test does not - same code path, same commands, no day/night branch anywhere in armStrapAlarm. One firmware-side explanation that would fit every reported case: the physical alarm haptic might only fire while the strap's own sleep/rest detection considers the wearer sleep-adjacent, independent of anything NOOP sends. This doesn't prove or fix that - it's a free read of HR already tracked live, persisted alongside the existing Alarm-block fields (sent epoch, connected, clock skew) and surfaced in the debug export, so the next reported failure (ideally a captured evening one) can be compared against the resting HR of the successful arms already on file. Mirrors the ryanbr#392 precedent of both-platform log-only alarm diagnostics. No stored analytics/formula changed; both platforms touched for parity per usual.
digitalerdude
marked this pull request as ready for review
July 21, 2026 15:15
DX23876
pushed a commit
to DX23876/noop
that referenced
this pull request
Jul 22, 2026
…nostic Alarm: log live HR at arm time, to test a morning/evening hypothesis (#34)
tanarchytan
added a commit
to tanarchytan/noop
that referenced
this pull request
Jul 24, 2026
…tics Log-only, no behaviour change. Persists the live HR at the moment a strap alarm is armed alongside the existing arm fields, and surfaces it in the debug export, so a reported morning-vs-evening haptic-timing failure can be compared against the resting HR of successful arms already on file. (ryanbr/noop ryanbr#707 / issue ryanbr#34, Max Schaedlich; Android only, Swift dropped) Verified compileFullDebugKotlin + testFullDebugUnitTest green.
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Aug 15, 2026
Four accessibility gaps in the charts, all of them "the infrastructure is here
and unused" rather than new capability.
1. Audio graphs. The charts were never silent — TrendChart, Sparkline and
Hypnogram each speak one summary ("N points, mean X, range Y to Z"). What a
summary cannot give is the shape: where the dip is, whether last night's deep
sleep came early or late. There were 0 AXChartDescriptors in the tree. Adding
one on the three shared components gives VoiceOver's rotor an Audio Graph at
every call site at once.
AudioGraphPlan holds the part that can be wrong — axis bounds, ordering — as
plain values, so it is unit-tested with no view and no VoiceOver. Two
invariants it exists to guarantee: points are sorted by x (VoiceOver steps
them in array order, and an unsorted series scrubs back and forth in time
while announcing itself as a trend), and neither axis is ever zero-width (a
flat series has min == max, and mapping into a zero-width range is a
divide-by-zero that plays every tone identically or as NaN). Both fail
silently: the descriptor still builds and still appears in the rotor.
On Hypnogram this reads like the per-interval accessibility layer that ryanbr#707
deliberately removed for scroll performance. It is not, and the code says so:
an AXChartDescriptor is not a semantics subtree, adds nothing to the element
tree the accessibility walk copies each scroll frame, and is built only when
an assistive technology opens the graph.
2. Names instead of "Trend". Charts without an accessibilityLabel fall back to a
generic "Trend", which on a screen entirely about one metric is the one word
carrying no information. The Mi Band and Apple Health cards each funnelled
~11 charts through one helper whose title was a LocalizedStringKey — opaque,
so it could not be reused as a label. Switching those to
LocalizedStringResource fixes every chart on both screens with one signature
change, and keeps compiler extraction into the catalog. MetricDetailView and
the coach chart bubbles had the name in hand already.
(StressView's remaining unlabelled chart is inside StressPreviewHarness —
preview-only fake data, not shipped UI.)
3. Colour is no longer the only encoding. differentiateWithoutColor was read
nowhere, while the two multi-series charts — the Compare overlay and the
workout recovery curves — separate their series by hue alone. A legend naming
four colours a reader cannot distinguish is not a legend. When the system
switch is on, each series also takes a distinct dash pattern, point marks take
distinct symbols, and the recovery legend's swatch shows the same dash so
legend and chart cannot disagree. Off, the default look is byte-identical.
The ladder is ordered by dash LENGTH (solid, long dash, dot, dash-dot), the
feature the eye separates fastest at chart scale.
4. Reduce Transparency, enforced at the surface. It was honoured in three places
and missed by the glass chrome most likely to sit over busy content:
glassEffect does not opt out of the setting on its own. NoopPanelSurface now
clamps its own opacity and the search chrome falls back to the opaque pill, so
the rule holds without every future caller having to remember it.
Verified: swift test in Packages/StrandDesign — 15 new tests pass, 82/83 overall.
The one failure (QuietMotionCoverageTests, CoachBreathHalo.swift:79) is
pre-existing: it fails identically with these changes stashed, and comes from the
breathing-halo merge. Not touched here — separate concern.
Strand (macOS) and NOOPiOS (generic/iOS) both build. 4 new StrandDesign catalog
keys, all nine languages, appended without reordering; i18n_audit reports zero
gaps in de/es/fr/pt-PT and no new un-extracted literals.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
…nostic Alarm: log live HR at arm time, to test a morning/evening hypothesis (ryanbr#34)
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
Perf audit of this PR, second finding. The epoch count built a set over EVERY sample -
mapTo(HashSet()) on Kotlin, Set(hrS.map{}) on Swift, the latter allocating a full intermediate array
as well - to arrive at a few thousand distinct keys. That is ~160k boxed values per scored day across
the 21-day rescore, and it is precisely the allocation shape #707 names as the heap-churn source
under the OOM.
hrS is already sorted by timestamp, so the bucket key is non-decreasing and a running comparison
counts them with no allocation at all. Pinned by a test, because the zero-allocation version trades
the set for that ordering assumption: unsorted input would silently over-count rather than fail.
The rest of the audit came back clean, and the parts that look expensive are not this PR's. The bpm
boxing in the percentile sort and the epoch HashMap inside hrOnlySleepRuns both predate it - they
arrived with the spine in #1805/#1806 - and this change does not add to either. Worth a separate look
if the spine ever runs on every day rather than only motionless ones; not worth widening a
diagnostic PR to chase.
26 HR-only tests, 0 failures; swift builds; doc_comment_lint clean.
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
#1812) #1801 landed a whole detection path that emits nothing. A field log on 11.1.0 build 412 then showed NO-NIGHT reason=no-motion with provided=0 and no way to tell whether the spine had run and found nothing or never ran at all - zero mentions of it in 3,702 lines. Wrong shape for a file whose entire sleep story is a funnel, and the no-motion comment had been reworded to claim heart rate was tried while leaving nothing to show for it. Two lines now. The spine's own funnel names what it derived and where the candidates went - anchor and band, the wearer's p50/p90 spread, epochs, raw runs, merged, sleep runs, longest candidate, staged and kept. anchorBpm and bandBpm are printed because they are DERIVED, not configured: the anchor is a percentile of the wearer's own window, so the same code hands every wearer a different threshold. sleepRuns against longestMin separates the two failures that actually happen, a band too tight from a duration gate eating real runs. And a gate line, because the funnel only exists when the spine is CALLED - so its absence carried the same ambiguity one layer up. Every day with no motion now states the decision: attempted, or skipped for a stored hypnogram, or for an imported owner. A day WITH motion prints nothing. Six passes of review, each finding something the last had introduced. The rounding diverged: round1 used String.format against String(format:), which are not twins - Java rounds HALF_UP on the decimal expansion, Swift half-to-even on the binary value, so 64.05 gave Kotlin 64.1 and Swift 64.0 on a line whose whole purpose is side-by-side comparison. Both now multiply and round, verified across ten values on both toolchains. The Apple sink was ungated and would have collected on every scored day. The line was Kotlin-only on a twinned path. epochs reported the SAMPLE count. Three percentiles meant three full sorts of ~160k samples per scored day, and the epoch count boxed every sample to find a few thousand keys - both the allocation shape #707 names under the OOM, now one sort and no set. A guard test caught its own brittleness: it asserted the gate within 1200 characters of the call, measuring proximity, and broke when two diagnostic lines were inserted while the gate still held. Now scoped to the expression and asserting containment. Both platforms. 26 HR-only tests, 0 failures; CI ran the Swift twins for real. Refs #1801.
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.
Not a fix — a diagnostic addition to help pin down #34's remaining open question.
Where #34 stands
Since v9.0.0 the
connectSettled-gated re-arm (#340) has resolved the original silent-drop failure mode. The reporter's latest log (2026-07-20) shows:result=0x01readbackarmStrapAlarmhas no day/night branch at all — the exact same commands go out either way. That rules out an obvious app-side conditional bug, but nobody has yet attached a log of the specific failing evening case, so there isn't enough evidence yet to justify a code fix — that would just be guessing.What this PR does
One hypothesis fits every reported data point: the physical alarm haptic might only fire while the strap's own sleep/rest detection (not anything NOOP sends) considers the wearer sleep-adjacent. This logs the live HR already tracked in
LiveState/_stateat the exact moment of each arm — a free read, zero new BLE traffic, zero behavior change — persisted alongside the existing Alarm-block fields (sent epoch, connected, clock skew) and surfaced in the debug export's "Last arm" line (… · HR 58 bpm at arm).This doesn't confirm or refute the hypothesis by itself. It means the next reported failure — ideally a captured evening one — can be compared against the resting HR of the already-successful arms on file, without needing another round-trip to ask for more instrumentation.
Mirrors the #392 precedent (log-only alarm diagnostics landing on both platforms together):
Strand/BLE/BLEManager.swift(recordAlarmArm) +Strand/System/DebugDataDiagnostics.swift(Alarm block)android/app/src/main/java/com/noop/ble/WhoopBleClient.kt(recordAlarmArm) +android/app/src/main/java/com/noop/testcentre/AndroidDiagnostics.kt(Alarm block)Testing
xcodegen generate && xcodebuild -project Strand.xcodeproj -scheme Strand -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO build— BUILD SUCCEEDEDxcodebuild -project Strand.xcodeproj -scheme NOOPiOS -destination 'generic/platform=iOS Simulator' CODE_SIGNING_ALLOWED=NO build— BUILD SUCCEEDED (BLEManager.swiftis shared with iOS)./gradlew compileFullDebugKotlin✓./gradlew testFullDebugUnitTest --tests "com.noop.ble.*" --tests "com.noop.testcentre.*"— 444/444 pass under-Duser.language=en -Duser.country=US; 2 unrelated locale-dependent failures (StandardHrSensorFormatTest, decimal-comma vs decimal-point, untouched by this diff) reproduce under the sandbox's defaultde_DElocale, matching the same class of pre-existing flake noted in Alarm: defer the re-arm-on-bond call so it doesn't race the connect handshake (#34) #340's own test plan