Skip to content

ecg: 124's argument mapping is inverted on hardware — 2 starts generation, 1 stops it, 0 is refused - #1727

Merged
ryanbr merged 2 commits into
ryanbr:mainfrom
Zebsi235:fix/ecg-controlsignal-mapping
Aug 29, 2026
Merged

ecg: 124's argument mapping is inverted on hardware — 2 starts generation, 1 stops it, 0 is refused#1727
ryanbr merged 2 commits into
ryanbr:mainfrom
Zebsi235:fix/ecg-controlsignal-mapping

Conversation

@Zebsi235

Copy link
Copy Markdown

What this PR does

Whoop5Ecg.ControlSignal maps mainControlECGDataGeneration's argument as stop = 0 / start = 1 / restart = 2. That reading came from the vendor client's enum declaration order and shipped explicitly unverified in #896. On a WHOOP MG it is wrong.

Each argument sent on its own, watching the type-43 stream rather than the ack, across two sessions on 2026-08-28:

arg shipped name ACK observed
0 stop FAILURE(0) ×10 refused, generation unchanged
1 start SUCCESS(1) ×20 stops generation
2 restart SUCCESS(1) ×20 starts generation

Three things back that up beyond the ack counts.

An isolation test. With TOGGLE_LABRADOR_FILTERED(139) closed, 124 = 2 was sent three times on its own with a 30 s listen each: zero type-43 packets. Then 139 = 1 followed by 124 = 2 and the stream returned within 10 s. Three negative controls, one positive, one variable.

A 10-cycle automated run. A reliability pass alternating the two arguments 5 s apart, scoring each window: startFlat=10/10, restartActive=9/10. The argument the shipped enum calls start produced a flat stream in all ten cycles.

The strap's own firmware. Its CONSOLE_LOGS narrate the analog front end, and NOOP already mirrors those into the strap log:

27, 81488729: MAX86176: Set ECG ON

That line appears once per 124 = 2, eight sends and eight lines. The console carries the strap's own uptime, so the correlation does not depend on my clock: the seven gaps between sends match the seven between firmware events to within 0.2 s.

That last one also sharpens what 139 does, and the doc comment says so rather than repeating the older reading: Set ECG ON fires whether or not 139 is open, so 139 gates the stream, not the front end. Detail on #891.

The load-bearing part is not the label

requestsRealtimeData accepted both 1 and 2:

arg == ControlSignal.START.raw || arg == ControlSignal.RESTART.raw

arg == 1 returned true. But 124 = 1 stops generation, so it asks for nothing. A run whose last act was to stop the stream was scored as having requested realtime data, and Whoop5EcgProbe's "acknowledged and then not honoured" verdict could fire on silence the run itself caused. The predicate's own doc states the intent it was failing to deliver: "A run built only from such commands has asked for nothing, and its silence is the expected outcome rather than a finding." The fail-safe design was intact; it was fed a wrong ordinal.

Shape of the change

  • The enum carries the hardware values, with device and firmware named in the doc block and the single-device limit stated, mirroring how WristSelection right above it already flags its inference.
  • RESTART is gone rather than renumbered. 0 is refused by the strap, so there is no third case to name, and nothing can send a value the strap rejects.
  • requestsRealtimeData accepts only START.
  • docs/PROTOCOL.md and ATTRIBUTION.md updated. ATTRIBUTION previously said no strap had been asked whether it honours these commands, which is no longer true.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • CI / tooling

How it was tested

On hardware. WHOOP MG, hw WS50_r00, fw 50.39.1.0 (DIS-attested this session), no subscription, Pixel 9 Pro / Android 15, on a local build with per-command logging so each argument could be sent alone and the stream watched instead of the ack. Two capture sessions, 2026-08-28 22:02 and 22:33.

Android unit tests, full suite, ./gradlew testFullDebugUnitTest --rerun-tasks --no-build-cache on Windows 11 / JDK 17 against main @ 911a02c: 4,761 tests across 584 classes, 0 failures, 0 errors (6 skipped, pre-existing).

Fail-then-pass, --tests "com.noop.protocol.Whoop5Ecg*":

  • with the fix: 58 tests, 0 failures
  • with the enum reverted to STOP(0) / START(1) and the tests kept: 4 failedcommandPayloadIsRevisionThenArg, everyCommandFrameBuilderMatchesItsOpcodeAndArg, onlyTheStreamAndGenerationVerbsCanProduceRealtimeData, attestedResultCodesOutrankTheShapeHeuristic
  • fix restored: 58 tests, 0 failures

The tests now assert literal wire bytes alongside the symbol, on both platforms. The previous cases asserted through ControlSignal.X.raw, so they moved with the enum and stayed green whatever the argument meant on the wire, which is exactly how this survived. Two of the four failures above are in that category and could not have failed before.

One existing test needed its fixture corrected, and it is worth naming because it shows how the wrong mapping was pinned: attestedResultCodesOutrankTheShapeHeuristic built its "the data request was refused" case as sent(124, 1, Failure). Under the corrected mapping 124 = 1 requests nothing, so the fixture became sent(124, 2, …). The test encoded the assumption.

python Tools/doc_comment_lint.py clean.

Not run: swift test. No Apple machine here, so the Swift half is compile-unverified by me and left to swift-packages.yml, which covers Packages/**. The Swift edits mirror the Kotlin ones line for line and the Kotlin twins pass, but that is reasoning rather than a compile. Same for CLAUDE.md's oracle rule: I could not run swiftc to generate the expected literals.

What this does NOT claim

Checklist

  • Swift package tests pass — not run locally (no Apple machine); left to swift-packages.yml
  • Android unit tests pass (./gradlew testFullDebugUnitTest, full suite)
  • No new build warnings introduced
  • UI changes use only StrandDesign tokens — n/a
  • No hardcoded hex frame bytes; protocol facts live in the schema / decoders
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output or any secrets/keystores

Related issues

Refs #891, #1100, #896, #1103.

Does not close any of them — see "What this does NOT claim".

Zebsi235 added 2 commits August 29, 2026 10:18
The vendor client's enum order shipped as stop=0/start=1/restart=2 in
ryanbr#896, explicitly unverified. On a WHOOP MG (WS50_r00, fw 50.39.1.0)
each argument sent alone shows 0 is refused (FAILURE), 1 stops
generation and 2 starts it; the strap's own console logs
'MAX86176: Set ECG ON' once per 124=2, eight for eight. The enum now
carries the attested values, RESTART is gone (0 is not a valid
argument, nothing may send it), and requestsRealtimeData accepts only
START so a stop is never scored as having asked for data. Tests assert
literal wire bytes on both platforms so a renumber can never move the
fixtures with it.
testOffPathIsTheExactInverseOfTheOnPath pinned all three OFF frames to
arg 0, written from the enum-order mapping. The generation stop is arg 1
on hardware (0 is refused); the two toggles still turn off with 0. This
test is Swift-only with no Kotlin twin, which is why the Android runs
could not catch it - exactly the declared gap, caught by CI as intended.
@Zebsi235

Copy link
Copy Markdown
Author

CI caught the gap I declared: testOffPathIsTheExactInverseOfTheOnPath is Swift-only with no Kotlin twin, so my Android runs could not reach it. It pinned all three OFF frames to arg 0, written from the old enum-order mapping. Fixed in a9ea638: the generation stop now expects arg 1 (0 is refused on hardware), the two toggles still turn off with 0, and the comment says why the three are no longer uniform.

Nothing else in the Swift suite references the old values; I swept the package tests and the app-target senders, which all go through the enum symbols.

@ryanbr
ryanbr merged commit f2476f9 into ryanbr:main Aug 29, 2026
15 checks passed
ryanbr pushed a commit that referenced this pull request Aug 31, 2026
…tforms (#1765)

Adds a decoder for the 240-byte type-43 REALTIME_RAW_DATA record the MG streams
once the turn-on order has opened the stream. Pure protocol on both platforms: no
transport, no UI, no storage, no enum changes, nothing sent to a strap.

Observed layout, across 315 records in one session on a WHOOP MG (WS50_r00, fw
50.39.1.0): an 8-byte frame header whose [8] is the inner record's type byte, a
constant 5x i16 sub-header at 24-33 that is NOT waveform, 101 i16-LE samples at
34-235, and a CRC32 trailer at 236-239. The arithmetic is internally consistent -
10 bytes is 5 x i16, 202 bytes is 101 x i16, and the three spans plus the trailer
total 240.

It belongs in the protocol layer because three consumers need it - a live view, a
signal classifier, a waveform export - and each had grown its own copy with magic
offsets and a bare 43, two of them having already drifted to DIFFERENT definitions
of "signal present". One definition here, unit-testable without a strap. None of
those consumers is in this diff.

Verified rather than taken on trust: samplesPerRawRecord is (236-34)/2 = 101, the
loop bound i+1 < 236 yields exactly 101 iterations with frame[234..235] read last,
and the sign conversion is correct two's complement on both sides. Parity is real -
same constants, same bounds, same threshold, six tests per platform mirrored by
name. Ran the Kotlin suite locally against the PR head: 6 tests, 0 failures. All 15
CI checks green.

Two observations, neither blocking. Swift hardcodes rawRecordType 43 where Kotlin
uses PacketType.REALTIME_RAW_DATA.rawValue - but Swift has no PacketType enum at
all, so the literal is the only option and the asymmetry is pre-existing. And
rawBodyActiveNonZeroBytes = 20, out of 212 body bytes, is an unexplained threshold
from one session; it is honestly labelled as observational, sits in exactly one
place, and has no consumer yet.

One structural note for whoever touches this next: Kotlin pre-sizes IntArray(101)
and fills through an independently bounded loop while Swift appends. Identical
today because the span is even. If an offset ever made it odd, Kotlin would keep a
trailing zero and Swift would return a shorter array - the one place these two can
drift.

Thanks to @Zebsi235, whose full-mode HCI snoop on #1635 is the reason that
investigation moved at all: it established that the 5/MG's SMP refusal is a strap
STATE rather than a permanent property, and that pairing mode flips it. That
overturned a conclusion the maintainers had already drawn from their own capture,
and it is a far harder thing to contribute than a patch.

Refs #891, #1100, #1727.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ECG negative result: the 1584-byte flash record is layout v16, and it is empty with the circuit closed

2 participants