Repository navigation
ecg: the pure research modules — allow-list, stats and log - #1941
Merged
Merged
Conversation
… tests
Three self-contained Kotlin modules for the MG ECG research work, with no caller
yet. Database-free, no Android framework types, no BLE, and fully unit-tested
without a strap.
- EcgResearchAllowList: a positive allow-list of the opcodes a research probe
may send, as a pure predicate over raw opcode ints. Deliberately NOT wired to
the send path in this change -- that is a separate, review-sensitive one
- EcgResearchStats: windowed scoring of a probe run
- EcgResearchLog: the capture record and its formatting
Unused on purpose. Landing them alone keeps the reviewable surface pure logic
that CI can verify end to end, and leaves the send-path question -- which widens
what the command sender can express -- to its own change with its own argument.
One guard is deliberately deferred rather than shipped here. The allow-list test
originally asserted that every dispatchable opcode is a constructible
CommandNumber. That is an invariant of the SEND PATH: send() forms a frame from a
CommandNumber, so an allow-listed opcode the enum cannot express could not be
dispatched at all. Opcodes 124/125/139 are absent from that enum on purpose --
upstream excluded them saying Android has no ECG app layer and sends none of
them -- so asserting their presence here fails for the correct reason. The guard
belongs with the wiring that makes it true. Its testable half, that no FORBIDDEN
opcode is reachable through the probe cases, is kept and passes.
The Swift twin is deliberately absent. Apple drives its ECG probe through
WhoopCommand and its own layer; there is no macOS/iOS consumer for these three,
so a twin now would be untested code with no caller on either platform. The
parity contract applies when a consumer exists.
…es split EcgResearchStats.estimateBpm ships in this split, but the suite that defends it did not. EcgResearchStatsTest covers the estimator with one injected rate plus noise and a short buffer -- it does not cover the artefact that got NOOP's earlier PPG->HR estimate withdrawn (ryanbr#194). EcgBpmEstimatorTest is that cover: eight injected rates rather than one, because a single match proves nothing and the method has to move with the input; the tiled-record artefact that unguarded reports as a confident 59 bpm; both sides of the notch; the declined-band cost stated rather than hidden; and the no-high-rate-bias case the unnormalised version failed. Recovered from the unsplit tree, where it was written. No source change -- it targets EcgResearchStats.estimateBpm, which this branch already carries.
estimateBpm took excludeLag as an optional parameter defaulting to null, so the shortest and most natural call, estimateBpm(samples, fs), silently selected the unguarded form. Measured on a tiled 101-sample record at 100 Hz carrying no rhythm whatsoever, that form returns 59, a believable resting heart rate manufactured from nothing. It is the withdrawn PPG->HR failure exactly, which is the failure this method's whole shape exists to avoid. The guard itself is right and costs nothing: on the same buffer, supplying the record period returns null, and a genuine 72 bpm sine reads 72 either way. So the parameter simply loses its default. Passing the period switches the guard on; passing null says this source is not record-tiled and accepts an unguarded estimate. Both remain available, but only as something somebody chose. The compiler then finds every caller that had not chosen: nine call sites across the two test files, six of which had taken the unguarded path without saying so. All now state excludeLag = null explicitly, which is the correct reading for a synthetic non-tiled fixture and makes the choice visible where it is made. No behavioural change. The existing artefact test already pins both sides, that an unguarded call reports the artefact near 59 and a guarded one declines, so the contract stays covered. Android suite 5466 green.
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.
Split 3 of the MG ECG research series; splits 1 (#1765) and 2 (#1727) are merged.
What this PR does
Adds three self-contained Kotlin modules under
com.noop.protocolfor the MG ECG research probe(#891/#1100), with no caller yet:
EcgResearchAllowListPROBE_OPCODES(123/124/125/139),GATE_OPCODES(119/121) — as pure predicates over raw opcode ints, plus aFORBIDDENcensus of the opcodes it must never form (firmware load / DFU, wipe, reboot, high-frequency sync, AFE writes)EcgResearchStatsestimateBpm, see belowEcgResearchLogDatabase-free, no Android framework types, no BLE. Fully unit-testable without a strap, which is what
makes them reviewable on their own and verifiable end to end by CI. Same shape as the pure
Whoop5EcgProbe.ktalready on Android; opcodes come fromWhoop5Ecg's constants, labels from #893'sCommandNamestable — no literals.Why it lands alone, unused. The allow-list is deliberately not wired to the send path here. Wiring it
is what widens what
CommandNumbercan express, and upstream excluded the three Labrador opcodes onpurpose ("Android has no ECG app layer and sends none of them"). Making that untrue deserves its own
review, with the allow-list and the deny-list census as the argument. So this PR keeps the surface to pure
logic and leaves the sensitive change to the PR that follows.
The heart-rate estimate, and why it is safe here.
EcgResearchStats.estimateBpmautocorrelates asample buffer — the method behind the withdrawn PPG→HR estimate (#194), whose failure was that fixed-size
records are periodic at the record period (~59 bpm at 100 Hz / 101 samples) and manufacture a plausible
resting rate. This implementation refuses that lag band, requires a local maximum (shortest first, so the
fundamental beats the sub-harmonic), and returns
nullfor "cannot tell" rather than a number. The cost isstated: a genuine rate inside the excluded band is declined. It feeds nothing and sits behind no UI in
this PR. It is not validated against a real, moving heart rate; that is the #194 lesson and it stays
open.
The Swift twin is deliberately absent. Apple drives its probe through
WhoopCommandandBLEManagerand has no consumer for these modules.
EcgResearchLog's row/CSV shape is written to be the contract afuture Swift export would match byte-for-byte, per the parity rule, rather than something to reconcile
later.
Type of change
How it was tested
Android unit tests, full suite on
f9ff5a21(this head), basemain@8c3ddaca: 5,466 tests, 0 failures,0 errors across 657 classes (6 skipped, pre-existing) — main's 5,430 plus these four suites:
EcgResearchAllowListTest9/9 — the four probe opcodes, the two gate verbs, the FORBIDDEN census, andthat no forbidden opcode is reachable through the probe cases.
EcgResearchLogTest11/11 — CRC verdict as its own column with "unchecked" distinct from "failed",CRC-failed rows never plotted, zeros kept, the byte cap.
EcgResearchStatsTest9/9 — the windowed statistics.EcgBpmEstimatorTest7/7 — recovers eight injected rates and is monotonic across five (one matchproves nothing); refuses the tiled-record artefact that an unguarded estimator reports as 59 bpm; works
either side of the excluded band; pins the declined-band cost; rejects noise, flatline and short buffers;
pins the no-high-rate-bias case an unnormalised score fails.
Counted from the JUnit XML with the results directory cleared first. No new build warnings.
Compile alone:
compileFullDebugKotlinis clean againstmainwith no files beyond these seven —the load-bearing check for "can land first": no dependency on the
CommandNumberwidening or on any UI.Gates:
Tools/doc_comment_lint.pyclean (2,039 files, no new detached doc comments).Tools/i18n_audit.py --ci upstream/mainclean — no strings touched.Not run on hardware — nothing here talks to a strap. Swift: n/a, no Swift touched.
What this does NOT claim
nobody has checked it against a real heart rate that moves.
Checklist
./gradlew testFullDebugUnitTest, full suite)StrandDesigntokens — n/a, no UIWhoop5Ecgand
DeviceConfigWriteGateconstantsdocs/CONTRIBUTING.mdRelated issues
#891, #1100 (the probe this serves) · follows #1765 and #1727. Does not close anything.