Skip to content

DEV-974: derive both slow-sensor CSV gap windows from the payload header rate - #295

Open
marknolan wants to merge 6 commits into
masterfrom
DEV-974_slow_sensor_rate_from_header
Open

marknolan wants to merge 6 commits into
masterfrom
DEV-974_slow_sensor_rate_from_header

Conversation

@marknolan

@marknolan marknolan commented Sep 10, 2026

Copy link
Copy Markdown
Member

Summary

The VD6283 light and MLX90632 skin-temp CSV gap windows were derived from the data: the parser differenced consecutive same-sensor block end ticks, took the median, and built the window from that. This takes the configured rate from the payload header instead.

Measuring only works on healthy data, which is the wrong property for a check whose whole job is to notice unhealthy data. A payload carries two or three slow-sensor blocks, so the window was built from one or two inter-block gaps. If one of those gaps was itself a dropped block, it became the median, the window centred on it, and the gap was judged continuous while the healthy boundary beside it was flagged instead. That is DEV-974 Bug A, and it was live for both sensors. The same fragility in the other direction produced Bug B, where a fast edge sitting on the median met the VD6283's bimodal cadence — two modes a factor 1.10 apart, exactly the tolerance — and split healthy files.

Neither can happen now. The window is [configured/1.5, configured*1.1], a pure function of the header, so a dropped block presents half the configured rate and is reported from the first boundary of a CSV set rather than once enough boundaries have accumulated to form an estimate. Nothing is learned, so no amount of loss can teach the parser that the loss is normal.

Where the rates come from

  • MLX90632 — the refresh code has always been in header byte 32, so the configured output rate was already known and the tick arithmetic was never needed. Removing it also removes an aliasing failure: at the 0.25 Hz configuration a 16-sample block spans ~64 s, two can land in one payload, and differencing their sub-minute end ticks re-based a real 64 s gap to ~4 s. Every genuine boundary then read as a gap, which fragmented a 3-day recording into thousands of CSVs and, through the algorithm-buffer reset on each split, stopped non-wear detection producing any output.
  • VD6283 — the rate is in header byte 30 bits 6:3 from FW v2.02.000 (DEV-1011, linked below). Before that it was stored nowhere and the only number available was 1/exposure, which bounds the rate from above and is ten times too fast at the firmware default of 1 Hz. That is what laid a 10 s block over 0.9 s and made the remaining 9.1 s look like a gap (DEV-979).

No firmware-version gate decides anything

The header field is self-describing. Earlier firmware always left those bits clear, and the firmware that writes them stores the effective index, which is never zero while light blocks exist. CCF_GEN2_LIGHT_RATE and isPayloadDesignV14orAbove are read in exactly one place, warnIfLightRateFieldMissingOnNewFirmware, and never on the parsing path, so a wrong placeholder version cannot change a parse - it can only mis-word a warning. Please still confirm the constant against the firmware release before this is relied upon.

That warning is the one signal the design cannot otherwise produce. A payload claiming firmware new enough to store the rate, carrying light blocks with the field clear, is a firmware fault - but a zero field is indistinguishable from an old recording, so without the warning the file would parse, the light data would come out, and nothing would say that gap detection for it is nearly blind. It prints once per recording. test023 to test025 cover old firmware staying silent, new firmware with a stored rate staying silent, and new firmware without one warning exactly once and again on the next file.

(An earlier revision of this PR described this diagnostic but did not implement it - the three symbols had no callers at all. Caught in review and fixed in d7d9124.)

A recording with no stored rate falls back to a window spanning the whole firmware rate table. That is deliberately generous, and its blind spot is asserted rather than left implicit: up to 20 s of lost light data at 1 Hz goes unreported there. It still keeps such a recording in one CSV instead of one per block, and being stateless it cannot be moved by the data.

Carried forward from the reverted DEV-979 work

Kept because it was independently correct: SensorMLX90632's output-rate bounds derived across both sub-measurement modes, the 1.15 conversion-slip tolerance measured on the DEV-927 recording, the gap-window shape, and the corrected getRateFreq javadoc.

Not carried forward: the observation history, medians, plausibility filters and tick-aliasing guards, all of which existed only to support inferring a rate that is now simply read. To be exact about "cross-payload state": what went is the local median list inside the deleted method. SAMPLING_RATE_LIMITS_PER_SENSOR is still a process-global static, and this adds a warn-once latch beside it. Both are cleared at file start and end by AsmBinaryFileParse in ASM_PC, so the single-threaded Lambda path is fine; two concurrent parses in one container would still clobber each other, as they would on master.

Known trade, stated plainly

getRateFreq() returns the configured rate clamped by the exposure bound, because the chip measures every max(inter-measurement, exposure) plus dead time: 10 Hz configured with a 100 ms exposure achieves ~9.09 Hz. Timing blocks at the configured period is therefore slightly less accurate than measuring them, bounded at roughly 90 ms on the oldest sample of a 10-sample block. On that one axis the inference was better. The trade is a small, bounded, predictable error against unbounded data-dependent failure, and it argues for validating rate against exposure in firmware, which is listed as a follow-up on DEV-1011.

Tests

API_00011 (28 tests) drives the real window seeder and takes every split decision through the real isDataBlockContinuous on synthetic header-built payloads. The physical scenarios from the reverted suite are kept and re-pointed rather than deleted, because they are the coverage that does not depend on a recording happening to contain the anomaly: a dropped block, a failed I2C read costing one period, the bimodal cadence, an overlapping boundary, a minute crossing.

Four earn specific mention:

  • test006 pins Bug A as fixed on the first boundary.
  • test021 pins the DEV-979 regression by showing a healthy 1 Hz boundary falls outside the old exposure-derived band and inside the new one.
  • test015 states the unknown-rate blind spot so it cannot be quietly forgotten.
  • test017 pins the skin-temp gap edge from both sides, and is worth reading before touching that constant. See below.

Full driver suite: 78 tests, 0 failures.

The skin-temp gap edge is tighter than it looks

Review found only 1.4% of margin on the case this window exists to detect. A dropped block normally doubles the block spacing, but one landing on the chip's documented 12.5% catch-up presents cfg/1.75 rather than cfg/2, against a gap edge of cfg/1.725.

The obvious repair is to narrow the slow side to cfg/1.5, as the VD6283 uses. That was tried and reverted. Test_065 contains a healthy skin-temp boundary at 1.63x nominal spacing during start-up settling - 1531 ms measured against a nominal 937.5 ms, no samples lost - and narrowing split it. So this sensor's healthy behaviour genuinely reaches into the region a dropped block occupies, and no single threshold separates them cleanly. 1.725 sits 6% above the worst healthy boundary observed and 14% below a clean dropped block.

test017 now asserts both ends of that, and asserts the margin stays under 10% so the window cannot be quietly widened. Worth a second opinion: the alternative is to stop treating block spacing as the only evidence.

Timestamps on pre-v2.02.000 light recordings

Stated plainly because it is a real loss, not only a gain. Removing the per-payload tick median means a light recording from firmware that never stored the rate now has its blocks timed from the exposure bound, which is ten times too fast at the 1 Hz default. Master measured those blocks and got them right when a payload held two or more.

For the file parser this costs the CSV header start time, up to 8.1 s at 1 Hz. Measured on Test_062, the one unknown-rate dataset, it moves by 90 ms and the Payload_Metadata start timestamps do not change at all. Neither the light nor the skin-temp CSV carries a per-sample timestamp column, so no CSV row values move.

For other consumers it costs more, and an earlier revision of this PR said otherwise. The ObjectCluster stream is stamped from each block's start time stepping by its timestampDiffInS, and the deleted refine step set exactly those. So on a pre-v2.02.000 light recording at the 1 Hz default, anything reading that stream - Android API, live streaming, algorithm modules - sees a 10-sample block laid over 0.9 s of the 10 s it really spans, then a 9.1 s jump. Caught in review.

Second-generation firmware never shipped to a customer, so this reaches internal recordings only, and anything from v2.02.000 onwards is unaffected either way. Open question for review: measured block timing could be kept for pre-v14 light while the splitting window still comes from the header - the two concerns are separable, and the objection was only ever to the window being self-poisoning. Not done here because it keeps a second timing path alive for a population that is entirely internal; worth deciding deliberately.

Note on the build

gradlew in this repository cannot resolve its dependencies: grpc 1.71 pulls Guava 33, whose variant metadata Gradle 6.1 cannot disambiguate. Pristine master fails identically, so this is pre-existing dependency drift, not this change. ASM_PC's Gradle 8.14.3 builds and tests the same code fine, which is how the suite above was run. The wrapper deserves its own ticket.

Merge order

Merge this before the companion ASM_PC PR, which pins the submodule to this branch's head.

Related: DEV-974, DEV-979, DEV-1011.

Review follow-up (12080ddb)

  • Reserved rate indices 7..15 are their own state, not folded into "not stored". Both get the wide fallback window but they mean opposite things, and the diagnostic was reporting a populated field as "clear". The raw nibble is kept so the warning names it, and a reserved index is reported whatever the payload design says - on an old recording those bits should be clear by construction, so a non-zero one is an anomaly in its own right.
  • The rate is cleared when a payload disables the light sensor. Stale gain and exposure only affected calibration; a stale rate now drives timing and splitting.
  • Configured = reports the configured rate, not the exposure-clamped one. A 20 Hz configuration was printing as 10.0 Hz, which is the one field a user reads to check what they asked for. The clamp still governs block timing and the window.
  • The "earlier firmware left bits 6:3 clear" assumption is now proved in the comment, against the firmware history, rather than asserted. A firmware-version gate was suggested and deliberately not taken: the header-patched datasets keep their true firmware version, so a gate would stop the parser reading the field those datasets exist to exercise.
  • Stale javadoc on SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO, which also silently sets the MLX90632 gap edge; SensorMLX90632 saying the slip widens one side when it widens both; AsmBinaryFileConstants documenting byte 30 as bits 2:0 and 7 only.
  • New coverage: test026 pins the reserved-index report at both firmware versions, test027 drives PayloadContentsDetailsV8orAbove's seeding layer - the layer that decides whether the seeder runs at all, and which nothing reached before.

marknolan and others added 2 commits September 10, 2026 21:31
…der rate

The VD6283 light and MLX90632 skin-temp CSV gap windows were derived from the
data: the parser differenced consecutive same-sensor block end ticks, took the
median and built the window from that. This replaces that with the configured
rate the payload header states.

Measuring only works on healthy data, which is the wrong property for a check
whose whole job is to notice unhealthy data. A payload carries two or three
slow-sensor blocks, so the window was built from one or two inter-block gaps; if
one of those gaps was itself a dropped block it became the median, the window
centred on it, and the gap was judged continuous while the healthy boundary
beside it was flagged instead. That is DEV-974 Bug A, and it was live for both
sensors. The same fragility in the other direction produced Bug B, where a fast
edge sitting on the median met the VD6283's bimodal cadence - two modes a factor
1.10 apart, exactly the tolerance - and split healthy files.

Neither can happen now. The window is [configured/1.5, configured*1.1], a pure
function of the header, so a dropped block presents half the configured rate and
is reported from the FIRST boundary of a CSV set rather than after enough
boundaries have accumulated to form an estimate. Nothing is learned, so no
amount of loss can teach the parser that the loss is normal.

The rates come from where they were always available, or now are:

- MLX90632: the refresh code has always been in header byte 32, so the
  configured output rate was already known and the tick arithmetic was never
  needed. Removing it also removes an aliasing failure: at the 0.25 Hz
  configuration a 16-sample block spans ~64 s, two can land in one payload, and
  differencing their SUB-MINUTE end ticks re-based a real 64 s gap to ~4 s, i.e.
  an apparent ~4 Hz. Every genuine boundary then read as a gap, which fragmented
  a 3-day recording into thousands of CSVs and, through the algorithm-buffer
  reset on each split, stopped non-wear detection producing any output at all.
- VD6283: the rate is now in header byte 30 bits 6:3 from FW v2.02.000
  (DEV-1011). Before that it was stored nowhere and the only available number
  was 1/exposure, which bounds the rate from ABOVE - ten times too fast at the
  firmware default of 1 Hz. That is what laid a 10 s block over 0.9 s and made
  the remaining 9.1 s look like a gap, giving one CSV per block (DEV-979).

The header field is self-describing, so no firmware-version gate decides
anything. Earlier firmware always left those bits clear, and the firmware that
writes them stores the EFFECTIVE index, which is never zero while light blocks
exist. CCF_GEN2_LIGHT_RATE and isPayloadDesignV14orAbove are added but used only
to word a diagnostic, so a wrong placeholder version cannot change a parse. A
recording with no stored rate falls back to a window spanning the whole firmware
rate table: deliberately generous, and its blind spot is asserted rather than
left implicit - up to 20 s of lost light data at 1 Hz goes unreported there. It
still keeps such a recording in one CSV instead of one per block, and being
stateless it cannot be moved by the data.

getRateFreq() now returns the configured rate clamped by the exposure bound,
because the chip measures every max(inter-measurement, exposure) plus dead time:
10 Hz configured with a 100 ms exposure achieves ~9.09 Hz. The window's 1.5x
slow-side tolerance absorbs the remainder. The VD6283 CSV sensor-config line
consequently reports "Configured = X Hz" like every other sensor, but only when
the header actually carried it, so a reader can still tell a known rate from an
exposure-derived guess.

Carried forward from the reverted DEV-979 work, because it was independently
correct: SensorMLX90632's output-rate bounds derived across both sub-measurement
modes, the 1.15 conversion-slip tolerance measured on the DEV-927 recording, the
gap-window shape itself, and the corrected getRateFreq javadoc. Not carried
forward: the observation history, medians, plausibility filters, cross-payload
state and tick-aliasing guards - all of which existed only to support inferring
a rate that is now simply read.

New API_00009 (21 tests) drives the real window seeder and takes every split
decision through the real isDataBlockContinuous on synthetic header-built
payloads. The physical scenarios from the reverted suite are kept and re-pointed
rather than deleted, since they are the coverage that does not depend on a
recording happening to contain the anomaly: a dropped block, a failed I2C read
costing one period, the bimodal cadence, an overlapping boundary, a minute
crossing. test006 pins Bug A as fixed on the first boundary; test021 pins the
DEV-979 regression by showing a healthy 1 Hz boundary falls outside the old
exposure-derived band and inside the new one; test015 states the unknown-rate
blind spot so it cannot be quietly forgotten.

Verified: 31 tests pass across API_00004, API_00008 and API_00009. Note that
`gradlew` cannot resolve dependencies in this environment - grpc 1.71 pulls
Guava 33, whose variant metadata Gradle 6.1 cannot disambiguate - and pristine
origin/master fails identically, so this is pre-existing dependency drift rather
than anything in this change. Compiled and run with javac/java directly against
the Gradle cache instead; the build failure is worth its own ticket.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…found

The diagnostic this PR described was never written. CCF_GEN2_LIGHT_RATE,
PayloadContentsDetails.isPayloadDesignV14orAbove and
VerisenseDevice.isPayloadDesignV14orAbove had no callers at all, so the PR body
was wrong to say they were "used only to word a diagnostic" - they were used for
nothing, and the case they existed to catch was silently unreported.

That case is a payload claiming firmware new enough to store the ambient-light
rate while carrying light blocks with the field clear. A zero field is
indistinguishable from an old recording, so the parser falls back to the wide
rate-table window, the light data still comes out, and nothing says that gap
detection for the file is nearly blind. If the firmware ever ships with the
field broken, this line is the only thing that will say so.
UtilCsvSplitting.warnIfLightRateFieldMissingOnNewFirmware now reports it once
per recording, and three tests cover it: old firmware stays silent, new firmware
with a stored rate stays silent, new firmware without one warns exactly once and
warns again on the next file. Nothing about parsing keys on the firmware
version; it is read there and nowhere else, so a wrong placeholder can still
only mis-word the warning.

Seeding the gap window no longer happens for a sensor the payload has no blocks
for. The sensor-class lookup behind it CREATES and caches its entry, so parsing
a first-generation file - which has neither slow sensor - was quietly populating
mappings and rate limits for hardware the recording does not have.

Two comments asserted things that are not true. The seeding call site said the
blocks "are already correctly timed", which holds only where the rate is known;
for pre-v2.02.000 light recordings they are timed from the exposure bound, which
is ten times too fast at the 1 Hz default. That is a real loss against measuring
the spacing and it is deliberate, so the comment now states the cost - the CSV
header start time, up to 8.1 s at 1 Hz, and no per-sample timestamps because
neither slow-sensor CSV carries any. SensorMLX90632 still described a per-payload
refinement from tick spacing that this work removed.

On the skin-temp gap window, review found only 1.4% of margin between a dropped
block landing on the chip's documented 12.5% catch-up and the gap edge.
Narrowing the slow side to match the VD6283's was tried and REVERTED: Test_065
contains a healthy skin-temp boundary at 1.63x nominal spacing during start-up
settling, measured at 1531 ms against a nominal 937.5 ms, and narrowing split it.
This sensor's healthy behaviour genuinely reaches into the region a dropped block
occupies, so no single threshold separates them cleanly and 1.725 is the
measured midpoint. test017 now pins the constraint from both sides, and asserts
the margin stays small so nobody widens the window without saying so.

Also: test001 pins the rate enum against the firmware millisecond table, since
that table is a stored-data contract shared across two repositories with no
build-time link; isSlowSensorConfiguredRateKnown has a caller instead of
duplicating its logic; and the tests are renumbered so declaration order matches.

Full driver suite: 76 tests, 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@jyong15 jyong15 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review of the driver change. Core arithmetic checks out and the suite is real; the items below are about claims and edges rather than the central design.

Verified in code, not from the description:

  • Byte 30 bits 6:3 ((b >> 3) & 0x0F), no overlap with gain (2:0) or dark (7); byte 30 is parsed in exactly one place. Rate table 1..6 matches slowSensorRateMs exactly.
  • VD6283 window [cfg/1.5, cfg*1.1]; MLX window [(cfg/1.15)/1.5, cfg*1.15*1.1], whose slow edge equals cfg/1.725 bit-identically. Dropped block on a 12.5% catch-up lands 1.43% outside. Edges are inclusive and consistent; no == on doubles.
  • Unknown-rate fallback [0.333, 22] Hz accepts a 10-sample boundary up to 30 s, i.e. 20 s unreported at 1 Hz.
  • isPayloadDesignV14orAbove and CCF_GEN2_LIGHT_RATE are read only in the warning path. No dead references to the removed inference machinery. Gen-1 payloads never reach the seeder thanks to containsDataBlockForSensor; SENSORS.CLOCK is skipped so fast-sensor windows are untouched.
  • API_00009: 25 of 25 pass locally; fixtures write the same offsets and shifts production reads; test024 asserts an exact count of one warning.

Findings, inline:

  1. Should-fix: "no per-sample timestamps are affected" is true only for the CSV files. The ObjectCluster stream is stamped from getStartTimeRwcMs() + getTimestampDiffInS(), and the deleted refine step was what set those for slow blocks. On a pre-v2.02.000 light recording at the 1 Hz default they are now 10x compressed.
  2. Should-fix: reserved rate indices 7..15 collapse into NOT_STORED, so the firmware-fault warning says "field clear" when it is not, and older headers say nothing.
  3. Question: isConfiguredRateKnown() trusts bits 6:3 with no design-version gate; rate is never reset when the light enable bit is clear.
  4. Question: the CSV Configured = value is the exposure-clamped rate, so 20 Hz configured prints as 10.0 Hz, and odd exposures print unrounded.
  5. Should-fix: stale UtilCsvSplitting javadoc describing the median design; the MLX 1.725 edge is an emergent product of a VD6283-motivated constant with no cross-reference.
  6. Nits: dead SKIN_TEMP branch and unreachable MLX fallback; MLX javadoc says one side widens, code widens both; "removed cross-payload state" overstates it since the static map remains and a static latch was added; AsmBinaryFileConstants byte-30 comment (not in this diff) still documents only bits 2:0 and 7.

Generated by Claude Code

Comment thread ShimmerDriver/src/main/java/com/shimmerresearch/verisense/VerisenseDevice.java Outdated
Reserved rate indices are no longer folded into "not stored". Bits 6:3 holding
7..15 now read as VD6283_RATE.RESERVED_INDEX, distinct from NOT_STORED. Both end
up with the wide fallback window, but they mean opposite things - one is an old
recording behaving exactly as designed, the other is a payload carrying a rate
nobody can decode - and the diagnostic was reporting the second as "field clear",
which sends a reader after the wrong firmware bug. The raw nibble is kept so the
warning can name it, and a reserved index is now reported whatever the payload
design version says: on an old recording those bits should be clear by
construction, so a non-zero one is an anomaly in its own right.

The rate is cleared when a payload has the light sensor disabled. Gain and
exposure going stale only affected calibration; the rate now drives block timing
and CSV splitting, so a stale one would mis-time a segment after a mid-file
configuration change.

The claim that no per-sample timestamps are affected was true only of the CSV
files. Neither slow-sensor CSV carries a timestamp column, but the ObjectCluster
stream is stamped from each block's start time stepping by its timestampDiffInS,
and the deleted refine step was what set those. So on a pre-v2.02.000 light
recording every consumer of that stream - Android API, live streaming, algorithm
modules - now sees a 10-sample block laid over 0.9 s of the 10 s it spans. The
comment says so, rather than claiming the opposite.

"Earlier firmware always left bits 6:3 clear" is load-bearing and was asserted
rather than shown, so the proof is now written down: pre-2.02.000
backupConfigSettings built the byte as (gain & 0x07) | (dark ? 0x80 : 0), which
cannot set those bits, and resetPayloadBackupConfig zeroed it. Adding a
firmware-version gate as well was considered and rejected, and the reason is in
the comment: the header-patched regression datasets deliberately keep their true
firmware version, so a version gate would stop the parser reading the very field
those datasets exist to exercise. A reserved index is the residual risk and it is
now reported rather than silently accepted.

The CSV Configured field reports the configured rate again, not the
exposure-clamped one. A device set to 20 Hz with a 100 ms exposure was printing
Configured = 10.0 Hz, which is the one field a user reads to check what they
asked for. The clamp still governs block timing and the gap window, and Exposure
is on the same line for anyone deriving the bound.

Documentation the review caught: SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO still
described medians and ">= 2 blocks" seeding, and hid that it also sets the
MLX90632 gap edge - retuning it for the light sensor alone would move that edge
to 1.61x and re-split Test_065. SensorMLX90632 said the slip widens one side
when it widens both. AsmBinaryFileConstants documented byte 30 as bits 2:0 and 7
only.

Also: isSlowSensorConfiguredRateKnown has a caller on the skin-temp path instead
of being a dead branch, the unreachable MLX fallback says why it stays, and
RESERVED_INDEX is kept out of the config-value lookup map.

New coverage. test026 pins the reserved-index report at both firmware versions.
test027 drives PayloadContentsDetailsV8orAbove's seeding layer, which nothing
reached before - it is the layer that decides whether the seeder runs at all, and
it is what keeps gen-1 payloads away from a lookup that creates sensor mappings
as a side effect.

Full driver suite: 78 tests, 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@marknolan

Copy link
Copy Markdown
Member Author

Thanks — this found two things I had got wrong rather than merely under-documented. All addressed; the one place I pushed back is marked as such and argued rather than waved away.

1. ObjectCluster timestamps — you are right, and the PR text was wrong

Verified: parseDataBlockData stamps from getStartTimeRwcMs() stepping by getTimestampDiffInS(), and the deleted refine step set exactly those for slow blocks. So "no per-sample timestamps are affected" was true only of the CSV files, and I had generalised it.

Taken option (a), reworded, but more explicitly than "no CSV per-sample timestamps" — the comment and the PR text now spell out that the file-parser output moves only by its header start time while every ObjectCluster consumer sees a 10-sample block laid over 0.9 s of the 10 s it spans, with a 9.1 s jump to the next.

On option (b), keeping measured block timing for pre-v14 light: I agree the two concerns are separable, and I think it is the better end state. Not doing it here, for a reason I want on the record rather than assumed — it means keeping the tick-median code alive for a population that is entirely internal recordings from firmware that never shipped, while the whole argument of this change is that the parser stops carrying a second timing path. Worth deciding deliberately rather than as a review footnote, so: Mark, this is a real call and I have not made it. If the Android API or the algorithm modules are expected to re-parse pre-v2.02.000 gen-2 recordings, (b) is right and I will do it.

2. Reserved indices 7..15 — fixed

You are right that collapsing them into NOT_STORED made the diagnostic lie. Added VD6283_RATE.RESERVED_INDEX as a distinct state, kept the raw nibble, and the warning now names it: "the reserved index 9, which no firmware rate table defines" rather than "field clear".

Also took your second point — it now reports regardless of payload design version. On a pre-v14 header those bits should be clear by construction, so a non-zero one is an anomaly in its own right and the version tells you nothing useful about it. test026 pins the report at both v2.00.009 and v2.02.000.

3. The unguarded firmware assumption — proof rather than a gate

Fair challenge, and the assumption was asserted rather than shown. It is now shown, in the comment at the parse site: pre-2.02.000 backupConfigSettings built the byte as (lightGainIndex & 0x07) | (lightDarkEnable ? 0x80 : 0), which cannot set bits 6:3, and resetPayloadBackupConfig zeroed the whole byte. There is no third writer. I checked this against the firmware history rather than reasoning from the docs.

Not taking the isPayloadDesignV14orAbove() gate, and the reason is concrete: the header-patched regression datasets deliberately keep their true firmware version — that is what lets their reference CSVs stay honest about which firmware produced them. A version gate would stop the parser reading the very field those datasets exist to exercise, so Test_066 would go green by not testing anything. The residual risk you identify is real, and RESERVED_INDEX is what bounds it: a fabricated non-zero nibble that happens to be 7..15 is now reported rather than silently trusted. A fabricated 1..6 would still be trusted, but that needs pre-2.02.000 firmware to have written a valid index into bits it demonstrably never touched.

Your smaller related point is fixed outright: rate is now cleared when a payload has the light sensor disabled. You are right that it matters more than gain and exposure going stale, because the rate now drives timing and splitting rather than only calibration.

4. CSV Configured = — fixed, as you suggested

Now reports rate.freqHz, so a 20 Hz configuration prints Configured = 20.0 Hz. The clamp still governs block timing and the gap window. Exposure is on the same line for anyone deriving the achievable bound, and the v6 doc row in the companion PR now says all of this explicitly. No reference moved: Test_066 is 1 Hz against a 100 ms exposure, so clamped and unclamped agree.

5. Stale javadoc and the hidden MLX coupling — fixed

Both. The constant's javadoc no longer describes medians or ">= 2 blocks" seeding, and it now carries the coupling in bold: it also sets the MLX90632 gap edge, that edge is pinned against a measured 1.63x boundary in Test_065, and retuning to 1.4 would move it to 1.61x and re-split that recording. Cross-referenced from getSlowSensorPlausibleRateRangeHz in both directions.

6. Nits

  • Dead SKIN_TEMP branch: isSlowSensorConfiguredRateKnown now has a caller on the skin-temp path instead of the check being duplicated inline.
  • Unreachable MLX fallback: kept, with a comment saying it is unreachable today and why it stays.
  • SensorMLX90632 javadoc: "widens the fast side" corrected to both sides. That was my error from reverting an earlier attempt to widen only one.
  • "Removed cross-payload state": overstated, corrected in the PR text — the static map remains and I added a static latch to it.
  • AsmBinaryFileConstants byte-30 comment now documents bits 6:3.

Coverage gap

Closed. test027 constructs a PayloadContentsDetailsV8orAbove, asserts an empty payload seeds nothing, then adds a light block and asserts only the light window appears. That is the layer that decides whether the seeder runs at all, and the guard that keeps gen-1 payloads away from a lookup which creates sensor mappings as a side effect.

On test008/test016 recomputing from production constants: agreed, and test017 is the deliberate counterweight — it asserts absolute rates measured off Test_065 rather than anything derived from the constants.

Full driver suite: 78 tests, 0 failures.

@jyong15 jyong15 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 12080ddb. All nine round-one threads are addressed and resolved. Nothing here blocks this repo on its own, but one change breaks the companion and the two must land together.

Verified fixed: RESERVED_INDEX distinct from NOT_STORED with the raw nibble kept and the warning naming it at any firmware version (test026 straddles the 2.2.0 gate); isConfiguredRateKnown() false for both sentinels; getRateFreq() cannot divide by zero for either; Configured = uses the unclamped table rate behind the known-rate guard so the sentinels never reach it; the skin-temp reroute is the identical predicate as before; MLX javadoc and byte-30 constants comment corrected; test027 constructs the real PayloadContentsDetailsV8orAbove and calls the production wrapper. Full ShimmerDriver suite here: 78 tests, 0 failures.

New, inline:

  • Should-fix: the else if(SD) rate-clear branch is unreachable as described, because sensorAndConfigMapsCreate() rebuilds the sensor before every SD config parse. My round-one comment overstated the risk. Delete it or reword as defence in depth.
  • Heads-up: the RESERVED_INDEX return breaks ASM_PC's patcher guard; ASM_PC_00036 Test_007 fails there. Fix flagged on the companion PR.
  • Nits: latch is cleared per CSV set not per file; latch name and reserved-index wording; one-way javadoc cross-reference; no test pins the unclamped CSV value at a rate where clamping bites.

The open question in the PR body about keeping measured block timing for pre-v14 light is a real product decision and I have not tried to make it here.


Generated by Claude Code

@marknolan marknolan self-assigned this Sep 18, 2026
… doc

Second round of review fixes.

- VD6283_RATE.isStorable() replaces the driver-side triple test and gives the
  ASM_PC header patcher one predicate to call. The two asking the same question
  in two places with two different answers is what let RESERVED_INDEX through
  the patcher once it existed.
- The light-disabled SD branch is documented as defence in depth rather than as
  a live fix: VerisenseDevice.configBytesParse rebuilds the sensor before every
  SD config parse, so there is no stale value for it to find today.
- The warn-once latch is renamed for what it now latches, and its javadoc says
  once per CSV set rather than once per recording, since the limits map is
  cleared at every split.
- The reserved-index message no longer asserts that the recording carries light
  blocks, which is something only the caller knows.
- getSlowSensorPlausibleRateRangeHz links back to seedSlowSensorGapWindow, so
  the cross-reference is no longer one-way.
- test028 pins the CSV Configured value at 20 Hz, where the exposure clamp bites
  and the configured and clamped rates are different numbers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@marknolan

Copy link
Copy Markdown
Member Author

All three addressed, details on the threads. Head is now 86332d31.

  • isStorable() on VD6283_RATE is the fix for both your enum nit and the companion break. One predicate, called by isConfiguredRateKnown() here and by the ASM_PC patcher guard there — the break existed because the same question had two implementations, so the fix removes one rather than adding a sentinel to the other. ASM_PC #370 is re-pinned to this commit and Test_007 passes there, checked to fail when the guard is reverted.
  • The unreachable branch is kept with the comment rewritten as defence in depth, leading with your point that sensor state does not carry from payload to payload. I verified the mechanism myself before acting (configBytesParse line 555 before the sensor loop at 617). No test, deliberately: any test of it passes without the branch, so it would look pinned and not be.
  • Latch doc: renamed hasWarnedLightRateFieldUnusable, once per CSV set with the mechanism stated, reserved-index text no longer claims light blocks the method has not checked for, and the cross-reference goes both ways now.
  • test028 pins the unclamped CSV Configured value at index 6, where the configured 20 Hz and the exposure-clamped 10 Hz are different numbers, and asserts the fixture really clamps so it cannot quietly stop testing anything.

Full ShimmerDriver suite on this head: 79 tests, 0 failures. Through the ASM_PC build, ASM_PC_00005 is 73/73 with the submodule at this commit.

The open product question in the PR body — whether to keep measured block timing for pre-v14 light recordings — is still open, and still not something to settle in a review thread.

marknolan and others added 2 commits September 18, 2026 09:14
Merging master brought DEV-1023's API_00009_TimestampUnwrapTest alongside this
branch's API_00009_VerisenseSlowSensorGapWindow. Different packages, so javac is
content, but the number is a traceability identifier and two live API_00009s
defeat it. DEV-1023 also took API_00010, so this takes the next free number.

The only reference outside the class itself is the comment in
PayloadContentsDetailsV8orAbove explaining why seedSlowSensorGapWindow is
package-private.

ShimmerDriver suite: 103 tests, 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@marknolan

Copy link
Copy Markdown
Member Author

Merged current master in (75818669) and renumbered the test suite. Head is fb49f691.

The merge was clean — no conflicts. It brings DEV-1020 (the Guava declaration fix), DEV-1013 (Gradle wrapper alignment) and DEV-1023 (timestamp unwrap). The first of those matters for anyone reviewing this: ShimmerDriver could not be built standalone before, because guava 19.0 resolved to 33.3.1 with ambiguous androidApiElements / jreApiElements variants. cd ShimmerDriver && ./gradlew test now works on its own — 103 tests, 0 failures.

API_00009_VerisenseSlowSensorGapWindow is now API_00011_VerisenseSlowSensorGapWindow. DEV-1023 added API_00009_TimestampUnwrapTest in driverUtilities, so after the merge two live classes carried API_00009. Different packages, so javac does not care, but the number is a traceability identifier. DEV-1023 also took API_00010, so this takes API_00011. Same 28 tests, unchanged; the only other edit is the comment in PayloadContentsDetailsV8orAbove that explains why seedSlowSensorGapWindow is package-private.

Where your reviews cite API_00009, read API_00011. The PR body is updated.

DEV-1023 moves no parser output. Worth stating because ASM_PC main still pins a driver from before it, so the companion PR's pin now crosses it. ASM_PC_00005 was re-run against this driver through the ASM_PC build: 73 tests, 0 failures, every reference matching.

@jyong15 jyong15 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round three of fb49f691. All round-two items are addressed in code, the master merge is clean and does not touch this PR's files or the Verisense timestamp path, and the standalone ShimmerDriver build is green. Approving; the items below are follow-ups, not blockers.

Verified fixed: isStorable() is true for exactly indices 1..6 and false for both sentinels; isConfiguredRateKnown() is that one call and no storability call site names a sentinel any more, including the ASM_PC patcher. Latch renamed and documented as once per CSV set, with the clearing mechanism confirmed in ASM_PC. Reserved-index text no longer asserts light blocks. Back-link from getSlowSensorPlausibleRateRangeHz resolves. test028 goes through the real config parse and the real header string, asserts the clamp is real, and pins Configured = 20.0 Hz. Rename to API_00011 is a 99% similarity move with no stale references in either repo. The merge commit has an empty combined diff, and DEV-1023's unwrap code is referenced only from ShimmerObject and SensorShimmerClock, never from the Verisense package.

Should-fix, inline: no test asserts the enum invariant isStorable() documents, so a future member with freqHz left at 0.0 would silently read as "not stored" everywhere. API_00011 is not registered in API_00005_Suite_ShimmerDriver while the DEV-1023 classes are; CI is unaffected because the suite class is excluded from the test task.

Nits, inline: the v14 fault branch still asserts light blocks the method has not checked for, and test024's name and message still say "per file". The ObjectCluster cost comment quantifies light only; skin-temp blocks also lost measured re-timing and can now carry up to the documented 12.5% slip within a block.

Local run: cd ShimmerDriver && ./gradlew test standalone gives 103 tests, 0 failures, API_00011 contributing 28. The compile prints pre-existing unmappable character warnings from VerisenseDevice javadoc, present on master.

The open product question about measured block timing for pre-v14 light recordings is still Mark's call and not something this review settles.


Generated by Claude Code

Comment on lines +342 to +345
fault = "stores the VD6283 sample rate in payload header byte 30 bits 6:3, but this"
+ " recording carries ambient light blocks with that field clear. This is a"
+ " firmware fault, not an old recording";
} else {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the second branch still makes the claim the first was just cleaned of. The comment a few lines up says the reserved branch deliberately says nothing about the recording carrying light blocks, because this method is public and does not check. This branch, same method, same entry point, still says "this recording carries ambient light blocks with that field clear". test024 calls warnIfLightRateFieldUnusable directly with no blocks at all, so the test itself shows the string can print when it is false. Either fix both or drop the justification comment.

Related, in the test file: test024_newFirmwareWithNoStoredRateIsReportedOncePerFile and its assertion message "exactly one line per file" still teach the "once per file" model that the javadoc here was corrected away from. Two-line rename, no behaviour change.


Generated by Claude Code

*/
public boolean isStorable() {
return configValue>0 && freqHz>0;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix: nothing asserts the invariant this javadoc says the predicate enforces. The comment above says a member carrying one of configValue > 0 / freqHz > 0 without the other is a mistake in the enum. No test iterates VD6283_RATE.values() to check it; the header-index fixtures for 0..15 can only reach members already in BY_CONFIG_VALUE. Failure scenario: firmware adds index 7 and someone appends RATE_50_HZ("50.0Hz", 7, 0.0) with the frequency left to fill in later. isStorable() returns false, the driver reports "not stored", the ASM_PC patcher refuses the index, and the whole suite stays green. A loop asserting r.configValue > 0 == r.freqHz > 0 over values() is the missing half of "put the storable-rate test on the enum".


Generated by Claude Code

Comment on lines +106 to +115
//
// Be precise about what it costs, because it is more than the CSV header.
// The light and skin-temp CSVs carry no timestamp column, so the FILE PARSER
// output moves only by its header start time, up to 8.1 s at 1 Hz. But the
// per-sample ObjectCluster stream is stamped from each block's start time
// stepping by its timestampDiffInS, so for those recordings every consumer
// of that stream - the Android API, live streaming, algorithm modules - now
// sees a 10-sample block laid over 0.9 s of the 10 s it really spans, and a
// 9.1 s jump to the next block. Second-generation firmware never shipped to
// a customer, so this reaches internal recordings only, and a recording from

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the cost comment quantifies only the light case. Removing the refine step also dropped setSamplingRate and calculateTimestampDiffInS for skin-temp blocks, which were previously re-timed from measured spacing. They are now spaced at the configured header rate, and this PR's own constant documents the MLX90632 slipping up to 12.5%, so a 16-sample block can carry up to that much intra-block drift in the ObjectCluster stream where before there was none. Block start times still come from the RWC backfill so it does not accumulate, and it is almost certainly acceptable, but a comment framed as "be precise about what it costs" should name it.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix: this class is not registered in API_00005_Suite_ShimmerDriver. The suite lists API_00002, 00003, 00004, and the two DEV-1023 classes 00009 and 00010, but not API_00011. The renumber's own rationale is that the number is a traceability identifier, and DEV-1023 added its classes to the suite in the same window. Anyone running the suite class as their entry point gets green without these 28 tests. Not a CI hole: build.gradle excludes the suite class from the test task, so ./gradlew test picks this up by class scan, and 00006 to 00008 are likewise unregistered. Consistency with the DEV-1023 precedent, not coverage.

Verified on this head: rename is a 99% similarity move with one content line changed, 28 tests, no remaining reference to the old name anywhere in either repo, and ./gradlew test standalone in ShimmerDriver gives 103 tests, 0 failures.


Generated by Claude Code

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.

2 participants