DEV-793 Widen + always apply the slow-sensor CSV gap window

[marknolan]

Problem

The DEV-927 hardware-validation recording (25 min of MLX90632 skin temp at 16 Hz output — 1,506 blocks, tick-verified no samples lost) fragmented into 7 skin-temp CSVs when parsed. Healthy MLX conversion jitter (block spacing occasionally +12.5%, then catching up) was being treated as a data gap.

Two defects, both in the slow-sensor gap-window seeding

1. The measured window never engaged. It is seeded from the first payload carrying ≥ 2 slow-sensor blocks — but when a recording's first payload holds only one block, refineSlowSensorSamplingRateFromBlockTicks returns early and populateExpectedPayloadTsDiffLimitMapIfNeeded claims the global map key with a configured-rate ±10% band. The refine path's containsKey guard then skips forever. The put is now unconditional: the measured window wins as soon as it exists, and re-measures each payload.
2. Even when measured, the gap side was too tight. The window came from the observed per-payload period spread, which with 2–3 blocks per payload is often a single inter-block gap — no spread information at all. The gap side is now SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO (1.5×) of the achieved median spacing: healthy jitter stays continuous, while a genuinely dropped block (2× spacing) still splits with comfortable margin. The fast side keeps the observed-minimum-period basis with the standard ±10%.

Result

The recording parses into the correct CSV sets — splits only at genuine device events (a reset at recording start; an RWC re-sync during the download BLE connection, uptime vs wall-clock verified).

Validation

ASM_PC ASM_PC_00005_VerisenseFileParserPC 72/72 green (including the new Test_065 built from this recording) + ASM_PC_00032 7/7, run against this branch. All pre-existing tests byte-identical — the widened window changes no previously-passing output (it only removes spurious splits).

Sequencing

ASM_PC PR (Test_065 + UTF-8 writers) is blocked on a Jenkins shimmerdriver publish containing this — same flow as the recent 0.11.8_beta.

Jira: DEV-793 / DEV-927

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adjusts how CSV-splitting gap windows are derived for slow sensors (VD6283 light, MLX90632 skin temp) so that normal conversion jitter and limited early payload information don’t cause false “data gap” splits.

Changes:

• Introduces a slow-sensor-specific maximum inter-block gap ratio constant (SLOW_SENSOR_MAX_INTER_BLOCK_GAP_RATIO).
• Updates slow-sensor gap-window seeding to use achieved median spacing for the “gap side” and to overwrite the global limits on every qualifying payload (instead of being locked by an early configured-rate seed).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
ShimmerDriver/src/main/java/com/shimmerresearch/verisense/payloaddesign/UtilCsvSplitting.java	Adds a slow-sensor-specific gap-ratio constant used when building splitting limits.
ShimmerDriver/src/main/java/com/shimmerresearch/verisense/payloaddesign/PayloadContentsDetailsV8orAbove.java	Changes slow-sensor rate refinement to widen the gap-side window and always apply measured limits per payload.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jyong15]

Code review — DEV-793 slow-sensor CSV gap window

Recommended disposition: request changes. The diagnosis of the original bug is correct — I verified that populateExpectedPayloadTsDiffLimitMapIfNeeded (UtilCsvSplitting.java:85) runs after refineSlowSensorSamplingRateFromBlockTicks in parsePayloadContentsMetaData, so a first payload carrying <2 slow blocks did permanently lock in the configured-rate ±10% band. But dropping the containsKey guard trades that bug for a dropped-block blind spot.

Major

1. The gap window is now self-referential, so a dropped slow-sensor block goes undetected — and the split lands at the wrong boundary instead (PayloadContentsDetailsV8orAbove.java:399-407). With the guard removed, the window is re-derived every payload from the very block spacings it then judges — and the two quantities are identically the same expression: refine computes perSamplePeriodsS[i] = (deltaTicks/32768.0)/sampleCount, and the gap check computes calcSamplingRate(prevEnd, nextEnd, nextSampleCount) = sampleCount/Δt(s). So 1/measuredPeriod is exactly the rate the checker computes for that boundary, and it is always inside [rate/1.5, rate*1.1].

Failure scenario with true per-sample period P and a payload carrying exactly 2 skin-temp blocks (2–3 per payload is the norm per the PR body) with one block's worth of data dropped between them (spacing 2P):

	value
perSamplePeriodsS	[2P] → medianPeriodS = 2P, achievedRateHz = 0.5/P
new limits	[0.333/P, 0.55/P]
intra-payload boundary rate 0.5/P	inside → real gap judged continuous, silently absorbed
both blocks	setSamplingRate(0.5/P) → that payload's CSV timestamps stretched 2×
previous-payload boundary (healthy 1/P)	> 0.55/P → flagged → CSV splits one block early, at a boundary with no gap

Pre-PR, the seeded fixed window (e.g. [0.667/P, 1.1/P]) correctly split at the real gap. Suggested direction: keep re-measuring, but don't let one payload's measurement replace the window wholesale — e.g. only accept a measurement from a payload with ≥3 observed periods, or carry a running/first-robust estimate forward and only widen it, rather than puting the latest value unconditionally.

2. The fast side is now volatile too, though the PR describes it as unchanged (:402). The two sides have inconsistent bases: gap side = median (robust), fast side = 1.1/min(observedPeriods) — a single-sample extremum, now re-sampled every payload. Too tight: the scenario above gives limits[1] = 0.55/P, rejecting the true healthy rate. Too loose: one catch-up gap of 0.5P (the PR body's own "slip then catch up" mechanism) gives limits[1] = 2.2/P, effectively disabling overlap/fast detection for that payload's intra- and cross-payload checks. Consider achievedRateHz * UPPER or a symmetric ratio constant, so both sides derive from the median.

Minor

3. perSamplePeriodsS.get(size/2) is an upper-median (:366) — on a 2-element sorted list it returns the larger value, so for a 3-block payload with one dropped block ([P, 2P]), the "median" is 2P and achievedRateHz is half the truth, contradicting the comment on line 364. That value now feeds both setSamplingRate() and limits[0], and the resulting window [0.333/P, 1.1/P] contains both boundary rates, so the gap is missed entirely and all three blocks are stamped at half rate. Pre-existing line, but the new formula gives it a much larger role — worth fixing here (even size → average the two middles).

4. The new constant's javadoc is stale and self-contradictory (UtilCsvSplitting.java:24-28): "the window is seeded from the first payload that carries >= 2 blocks" is precisely the behavior this PR removes (commit 2 fixed the equivalent wording in PayloadContentsDetailsV8orAbove.java but not here), and "1.5x keeps comfortable margin on both sides" is wrong — the ratio applies only to the gap side; the fast side uses UPPER.

5. No driver-side unit test for the new arithmetic. Nothing in ShimmerDriver/src/test touches UtilCsvSplitting, SAMPLING_RATE_LIMITS_PER_SENSOR, or refineSlowSensorSamplingRateFromBlockTicks; validation is entirely ASM_PC integration tests in a separate, currently-blocked PR. The window computation is a pure function of a List<Double> of periods — the scenarios in findings 1 and 3 would each be a few-line unit test and would have caught them. Note the passing Test_065 recording is described as having no lost samples, so it never exercises the dropped-block path.

Nits

6. ShimmerDriver/build.gradle:52 stays 0.11.8_beta — if the Jenkins publish ASM_PC is blocked on reuses those coordinates, consumers can resolve a stale cached artifact of the same version. Flagging only so the uprev sequencing is explicit.

7. public class FILE_GAP_TOLERANCE_MULTIPLIER (UtilCsvSplitting.java:14) should be static — a non-static inner class holding only constants. Pre-existing; this PR adds a third constant to it.

Checked and cleared

• No public/protected signature changes; the new constant is additive. SAMPLING_RATE_LIMITS_PER_SENSOR is only written internally, so the removed guard can only be overriding the configured-rate fallback, as claimed.
• No cross-file leakage: ASM_PC calls clearMapOfSamplingRateLimitsPerSensor() on every CSV-set boundary.
• No new threading race within this repo (no parallel payload parsing) — though the unsynchronised static HashMap remains one if a consumer ever parses two files concurrently in one JVM.
• Unit consistency of achievedRateHz/1.5 holds whenever the median is measured correctly.
• maxPeriodS removal leaves no dead code; FILE_GAP_TOLERANCE_MULTIPLIER.LOWER is still used by calculateSamplingRateLimits.

Findings 1 and 3 diminish if real payloads typically carry ≥4 slow-sensor blocks — the 2–3 figure is taken from the PR description, not measured.

---

Generated by Claude Code

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.