From 37ba851bddf81846bfc58a0e3cdbe86e09004f7b Mon Sep 17 00:00:00 2001 From: Mark Nolan Date: Thu, 17 Sep 2026 13:10:47 +0100 Subject: [PATCH 1/2] DEV-1023: size the timestamp reorder window from the sampling rate The unwrap already told a reordered packet from a roll-over by how far backwards the counter stepped, but it sized that threshold as a fraction of the counter's range. The two quantities are unrelated: a reorder swaps packets that are adjacent in time, whereas a dropout that happens to span the wrap point is most of a modulo. At an eighth of the range the boundary between them lands in the middle of ordinary dropout territory - on the 16-bit counter every gap between 1.75 s and 2.0 s read as a reorder and the roll-over was silently lost. The window is now eight sample periods, derived from the rate the inquiry reports, which shrinks that misread band to about 16 ms. Both clients pass the rate at stream start; `timelineState.reorderWindowTicks` reports the window in force, which is what the two new client tests assert - the wiring is one line in each client and nothing else would notice it going missing. Also restated on the modular forward distance rather than on a comparison of unwrapped values. The two look equivalent and are not: a packet arriving late from just before a wrap boundary has a candidate ABOVE its predecessor, so a comparison accepts it as forward motion of nearly a whole modulo and then reads the next real sample as a second wrap. `[2^24 - 10, 5, 2^24 - 10, 70]` lands at 33554502, two modulos out, from one out-of-order packet. A rate the client does not know yet still falls back to an eighth of the modulo, as before, so a timeline nobody has told the rate to behaves exactly as it did. The other Shimmer host APIs disable the branch outright in that case; they import files, where there is no host clock to appeal to, and this SDK has the missed-wrap recovery as a second witness. One thing the fallback does not get: overruling the invalid-zero test. A reorder is judged before that test, which is right for a window of a few sample periods - a zero that close to an origin is genuinely ambiguous and the cost of choosing wrong is 16 ms - but at 64 s wide it would read an unstamped record as a packet 64 s late, place it 64 s early and call it valid, which is worse than either answer the rule is choosing between. tests/fixtures/timestamp_unwrap.json is the shared conformance file, 26 vectors and 11 window derivations, copied byte-identically from log-and-stream-common where the rule is specified and a reference implementation regenerates it in CI: git blob 367efd9979634e8e6fdfbb60a7f7cc64df156721 in both repositories. The Java, C# and Python APIs run the same file. Nine mutations were checked rather than assumed, each caught - including dropping the rate wiring from either client. Reorder and duplicate detection came out of a parallel fix for the same defect on the Java driver; the polarity and the window sizing here are different, but the observation that a backward step is not always a roll-over is hers. Co-Authored-By: Mas Azalya <43565312+MAzalya@users.noreply.github.com> Co-Authored-By: Claude Opus 5 --- .prettierignore | 1 + README.md | 11 + src/core/StreamTimeline.ts | 214 +++++- src/devices/shimmer3/Shimmer3Client.ts | 7 + src/devices/shimmer3r/Shimmer3RClient.ts | 7 + src/index.ts | 3 + tests/calibration/stream-clients.test.ts | 43 ++ tests/core/stream-timeline-vectors.test.ts | 179 +++++ tests/core/stream-timeline.test.ts | 127 +++- tests/fixtures/timestamp_unwrap.json | 729 +++++++++++++++++++++ tests/shimmer3/transport-loopback.test.ts | 20 + 11 files changed, 1311 insertions(+), 30 deletions(-) create mode 100644 tests/core/stream-timeline-vectors.test.ts create mode 100644 tests/fixtures/timestamp_unwrap.json diff --git a/.prettierignore b/.prettierignore index 52a3a6f..b003732 100644 --- a/.prettierignore +++ b/.prettierignore @@ -2,3 +2,4 @@ node_modules dist docs coverage +tests/fixtures/timestamp_unwrap.json diff --git a/README.md b/README.md index 231fc4a..140232b 100644 --- a/README.md +++ b/README.md @@ -211,6 +211,17 @@ Shimmer3R — its packet timestamp is the low 24 bits of the same counter `GET_RWC` returns), `rwc-estimated` (Shimmer3, from the request round trip, with the uncertainty stated), or `host` (no device clock; the Consensys method). +The same unwrap also has to tell a pair of packets delivered out of order from +a genuine roll-over, and the two look identical in the counter alone — a step +backwards. What separates them is size, so the timeline is told the sampling +rate at stream start and sizes its **reorder window** at eight sample periods. +A packet that late is placed where it was taken; anything further back is a +roll-over. `timelineState.reorderWindowTicks` reports the window in force. A +host driving `StreamTimeline` itself passes `samplingRateHz`, or calls +`setSamplingRateHz`, to get the same treatment; without a rate the window falls +back to an eighth of the counter's range, which catches reorders but misreads a +dropout of 1.75–2.0 s on the 16-bit counter as one. + `oc.timestampValid` says whether a frame's timestamp means anything. Firmware stamps a packet when the sample tick starts it, so a counter field of exactly `0x000000` is a record it published without stamping — LogAndStream diff --git a/src/core/StreamTimeline.ts b/src/core/StreamTimeline.ts index 014f56a..e21d840 100644 --- a/src/core/StreamTimeline.ts +++ b/src/core/StreamTimeline.ts @@ -72,6 +72,59 @@ export const TICKS_PER_MS = TICKS_PER_SECOND / 1000; */ export const INVALID_ZERO_WINDOW_TICKS = TICKS_PER_SECOND; +/** + * How many sample periods behind its predecessor a value may be and still be + * read as a reordered packet rather than as forward motion across a wrap. + * + * A reorder swaps packets that are adjacent in time, so it spans a handful of + * sample periods; a dropout spans whatever the link lost. Eight periods sits + * orders of magnitude clear of both at any rate the hardware offers. + */ +export const REORDER_PERIODS = 8; + +/** + * The largest fraction of the counter's range a reorder window may occupy. + * + * At 1 Hz on the 16-bit counter eight sample periods is four whole modulos, and + * a window at or above the modulo leaves no backward step large enough to be a + * wrap — the unwrap would stop counting them altogether. + */ +export const MAX_WINDOW_DIVISOR = 8; + +/** + * The reorder window for a stream at a known sampling rate, in counter ticks. + * + * Sized in **sample periods**, not as a fraction of the counter's range. The + * two are easy to confuse and behave very differently: a reorder swaps adjacent + * packets, whereas a dropout that happens to span the wrap point is most of a + * modulo. Sizing the window by the modulo puts the boundary between them in the + * middle of ordinary dropout territory — at 2^16 every gap between 1.75 s and + * 2.0 s reads as a reorder and the wrap is silently lost, and 1.75 s is a gap a + * Bluetooth link produces on a bad afternoon. Eight sample periods shrinks that + * misread band to about 16 ms. + * + * `0` — the branch disabled — when the rate is not a positive finite number. + * Never guess: an unknown rate must not become an infinite window, which would + * read every backward step as a reorder and lose every wrap. That is a worse + * failure than no reorder detection at all, and it is how a parallel fix for + * this same defect reverted itself whenever the rate happened to read zero. + * + * @param samplingRateHz Samples per second. **The counter's own 32768 Hz tick + * domain is what the answer is in** — pass the rate in Hz, never a rate + * expressed against a TCXO sampling clock. + * @param modulo The counter's range, `2 ** timestampBits`. + */ +export function reorderWindowTicks( + samplingRateHz: number | null | undefined, + modulo: number, +): number { + if (samplingRateHz == null || !Number.isFinite(samplingRateHz) || samplingRateHz <= 0) return 0; + return Math.min( + (REORDER_PERIODS * TICKS_PER_SECOND) / samplingRateHz, + modulo / MAX_WINDOW_DIVISOR, + ); +} + /** * Relative drift assumed between the sensor's clock and the host's, in parts * per million, when an anchor is bound to a stream some time after the reading @@ -164,6 +217,16 @@ export interface TimelineState { wraps: number; /** The counter width in use. */ timestampBits: TimestampBits; + /** + * The reorder window in force, in counter ticks — how far behind its + * predecessor a sample may be and still be placed where it was taken rather + * than read as a wrap. + * + * Reported so a host can see that its sampling rate reached the timeline. + * See {@link reorderWindowTicks} and + * {@link StreamTimeline.setSamplingRateHz}. + */ + reorderWindowTicks: number; } interface PendingAnchor { @@ -192,6 +255,19 @@ interface ResolvedAnchor { export interface StreamTimelineOptions { /** Counter width. Default 24. */ timestampBits?: TimestampBits; + /** + * The stream's sampling rate, which sizes the reorder window. Omit, or pass + * `null`, when it is not known yet — a client normally learns it from an + * inquiry and calls {@link StreamTimeline.setSamplingRateHz} later. + */ + samplingRateHz?: number | null; + /** + * The reorder window outright, in ticks, overriding the rate. For a caller + * that knows better than the derivation — and for the shared conformance + * vectors, which specify the window rather than the rate so that every host + * API runs them identically. + */ + reorderWindowTicks?: number | null; } /** @@ -211,10 +287,14 @@ export class StreamTimeline { private _wraps = 0; /** * How far behind the previous sample a value may be and still be read as a - * reordered packet rather than as forward motion across a wrap. An eighth of - * the modulo; see {@link _unwrap} for why not half. + * reordered packet rather than as forward motion across a wrap, in ticks. + * Derived — see {@link _recomputeReorderWindow}. */ - private _reorderWindow: number; + private _reorderWindow = 0; + /** The stream's sampling rate, or `null` when it is not known. */ + private _samplingRateHz: number | null = null; + /** A window set outright by the caller, overriding the derivation. */ + private _reorderWindowOverride: number | null = null; private _pending: PendingAnchor | null = null; private _anchor: ResolvedAnchor | null = null; /** @@ -228,7 +308,75 @@ export class StreamTimeline { constructor(opts: StreamTimelineOptions = {}) { this._bits = opts.timestampBits ?? 24; this._modulo = 2 ** this._bits; - this._reorderWindow = this._modulo / 8; + this.setSamplingRateHz(opts.samplingRateHz ?? null); + this.setReorderWindowTicks(opts.reorderWindowTicks ?? null); + } + + /** + * Tell the timeline the stream's sampling rate, so that it can size the + * reorder window in sample periods. + * + * `null` — or anything that is not a positive finite number — means "not + * known", and the window falls back to an eighth of the modulo, which is what + * this class has always used. That fallback is a compromise this SDK can + * afford and a file importer cannot: on a live link the host-clock recovery + * in {@link _unwrap} is a second witness, whereas an SD file has no clock to + * appeal to and the other Shimmer host APIs therefore disable the branch + * outright when the rate is unknown. Pass the rate and the question does not + * arise: the derived window is better in every case. + * + * Cheap and idempotent — a client calls it whenever the rate changes, and the + * next sample is judged by the new window. + */ + setSamplingRateHz(samplingRateHz: number | null): void { + this._samplingRateHz = + samplingRateHz !== null && Number.isFinite(samplingRateHz) && samplingRateHz > 0 + ? samplingRateHz + : null; + this._recomputeReorderWindow(); + } + + /** + * Set the reorder window outright, in ticks, or `null` to go back to deriving + * it from the sampling rate. `0` disables the branch. + */ + setReorderWindowTicks(ticks: number | null): void { + this._reorderWindowOverride = + ticks !== null && Number.isFinite(ticks) && ticks >= 0 ? ticks : null; + this._recomputeReorderWindow(); + } + + /** The reorder window in force, in counter ticks. */ + get reorderWindowTicks(): number { + return this._reorderWindow; + } + + /** + * True when the window in force is a reorder-scale one — derived from a known + * rate, or set outright by the caller — rather than the rate-unknown + * fallback. + * + * It decides whether a reorder is allowed to overrule the invalid-zero test + * (see {@link _unwrap}). A window of a few sample periods can: a zero that + * close to an origin really is ambiguous, and the cost of choosing wrong is + * about 16 ms. An eighth of the modulo cannot: it is 64 s on the 24-bit + * counter, and reading an unstamped record as a packet 64 s late would place + * it 64 s early and call it valid, which is worse than either answer the rule + * is choosing between. + */ + private get _windowIsReorderScale(): boolean { + return this._reorderWindowOverride !== null || this._samplingRateHz !== null; + } + + /** Explicit window, else the rate-derived one, else the legacy fallback. */ + private _recomputeReorderWindow(): void { + if (this._reorderWindowOverride !== null) { + this._reorderWindow = this._reorderWindowOverride; + } else if (this._samplingRateHz !== null) { + this._reorderWindow = reorderWindowTicks(this._samplingRateHz, this._modulo); + } else { + this._reorderWindow = this._modulo / MAX_WINDOW_DIVISOR; + } } /** The counter width this timeline is unwrapping. */ @@ -248,7 +396,10 @@ export class StreamTimeline { if (bits === this._bits) return; this._bits = bits; this._modulo = 2 ** bits; - this._reorderWindow = this._modulo / 8; + /* The window is clamped against the modulo and may be derived from it, so + it has to be recomputed here. The sampling rate is not a property of the + counter width and is deliberately kept. */ + this._recomputeReorderWindow(); this.reset(); } @@ -387,6 +538,26 @@ export class StreamTimeline { const value = ((raw % this._modulo) + this._modulo) % this._modulo; if (this._lastRaw === null) return value; + /* Everything below is decided on the MODULAR forward distance from the last + sample — never by comparing candidate unwrapped values, which looks + equivalent and is not. A packet arriving late from just before a wrap + boundary has an unwrapped candidate ABOVE its predecessor, so a + comparison accepts it as forward motion of nearly a whole modulo, and + then reads the next real sample as a second wrap: `[2^24 - 10, 5, + 2^24 - 10, 70]` lands at 33554502, two modulos out, from one out-of-order + packet. The modular distance sees it for what it is. + + A duplicate (`forward === 0`) holds the timeline exactly where it is, and + falls out of the arithmetic below without a branch of its own. + + Forward motion is the DEFAULT. That is what keeps a wrap preceded by a + long dropout classified as a wrap: however much was lost, the counter + still rolled over. A rule that defaults the other way — "a backward step + is corrupt unless it clears some threshold" — fails exactly there. */ + const forward = (value - this._lastRaw + this._modulo) % this._modulo; + const backwards = this._modulo - forward; + const reordered = forward !== 0 && backwards <= this._reorderWindow; + /* A counter of exactly zero arriving from mid-range is not a roll-over: it is a record the firmware never stamped. Read as a wrap it would put every later sample in the session a clean 512 s late, which is how a customer's @@ -394,8 +565,14 @@ export class StreamTimeline { deliberately narrow — the 24-bit counter, an exact zero, and a predecessor further than {@link INVALID_ZERO_WINDOW_TICKS} from the top of the range — so a genuine wrap onto zero is still accepted and the - 16-bit counter is untouched. See the constant for why. */ + 16-bit counter is untouched. See the constant for why. + + A reorder comes first, so a zero within a window of an origin is placed + rather than rejected: that is the order every Shimmer host API uses. It + only applies to a reorder-scale window — see + {@link _windowIsReorderScale}. */ if ( + !(reordered && this._windowIsReorderScale) && this._bits === 24 && value === 0 && this._lastRaw < this._modulo - INVALID_ZERO_WINDOW_TICKS @@ -404,29 +581,7 @@ export class StreamTimeline { } const half = this._modulo / 2; - /* Forward distance from the last sample, and whether to read it as forward - motion (crossing a wrap if it has to) or as a small step BACKWARDS — - a duplicated or reordered packet. Without the backwards case one - out-of-order packet adds a whole modulo, 512 s on a Shimmer3R, for the - rest of the session. - - The threshold is the REORDER WINDOW, not half the modulo. Half looks - like the natural split and is wrong on the 16-bit counter: its whole - modulo is 2 s, so a genuine forward gap of more than a second — which a - single missed Bluetooth window produces — reads as a step backwards, and - the sample lands almost a modulo early. What actually distinguishes the - two is magnitude: a reorder swaps packets that are adjacent in time, so - it is a handful of sample periods, while a gap is whatever the link - dropped. An eighth of the modulo is 64 s on the 24-bit counter and - 0.25 s on the 16-bit one — far larger than any reorder, far smaller than - a gap worth recovering. A duplicate (`forward === 0`) is unaffected - either way. */ - const forward = (value - this._lastRaw + this._modulo) % this._modulo; - const backwards = this._modulo - forward; - let unwrapped = - backwards <= this._reorderWindow && forward !== 0 - ? this._lastUnwrapped - backwards - : this._lastUnwrapped + forward; + let unwrapped = reordered ? this._lastUnwrapped - backwards : this._lastUnwrapped + forward; /* The rule above cannot see a wrap that went by entirely — more than a whole modulo of samples missed, which is 512 s on a 24-bit counter but @@ -562,6 +717,7 @@ export class StreamTimeline { skewMs: this._anchor?.skewMs ?? null, wraps: this._wraps, timestampBits: this._bits, + reorderWindowTicks: this._reorderWindow, }; } diff --git a/src/devices/shimmer3/Shimmer3Client.ts b/src/devices/shimmer3/Shimmer3Client.ts index 67c3668..889cbf1 100644 --- a/src/devices/shimmer3/Shimmer3Client.ts +++ b/src/devices/shimmer3/Shimmer3Client.ts @@ -794,6 +794,13 @@ export class Shimmer3Client extends BaseShimmerClient { // The width is a firmware property the handshake has established by now: // 16 bits, wrapping every 2 s, on anything older than LogAndStream 0.5.4. this._timeline.setTimestampBits(this._timestampFmt === 'u16' ? 16 : 24); + /* And the rate, which sizes the reorder window: eight sample periods is + what separates a pair of packets delivered out of order from a dropout + that happens to span the counter's wrap point. Without it the window + falls back to an eighth of the modulo, which on the 16-bit counter is + 0.25 s and reads an ordinary 1.8 s gap as a reorder. Zero means the + inquiry has not run, and `null` says so rather than passing it on. */ + this._timeline.setSamplingRateHz(this.samplingRateHz > 0 ? this.samplingRateHz : null); this._timeline.reset(); if (!this.anchorStreamClock || this._timeline.hasAnchorRequest) return; /* This host's clock, the Consensys method. No round trip is spent here — diff --git a/src/devices/shimmer3r/Shimmer3RClient.ts b/src/devices/shimmer3r/Shimmer3RClient.ts index ef58ffe..64936b5 100644 --- a/src/devices/shimmer3r/Shimmer3RClient.ts +++ b/src/devices/shimmer3r/Shimmer3RClient.ts @@ -3299,6 +3299,13 @@ export class Shimmer3RClient extends BaseShimmerClient { sawtooths for its whole length. `Shimmer3Client` has always done this; this client had the same option and did not. */ this._timeline.setTimestampBits(this.forceTimestampFmt === 'u16' ? 16 : 24); + /* And the rate, which sizes the reorder window: eight sample periods is + what separates a pair of packets delivered out of order from a dropout + that happens to span the counter's wrap point. Without it the window + falls back to an eighth of the modulo, which on the 16-bit counter is + 0.25 s and reads an ordinary 1.8 s gap as a reorder. Zero means the + inquiry has not run, and `null` says so rather than passing it on. */ + this._timeline.setSamplingRateHz(this.samplingRateHz > 0 ? this.samplingRateHz : null); this._timeline.reset(); if (!this.anchorStreamClock || this._timeline.hasAnchorRequest) return; /* Nobody has read the sensor's clock, so fall back to this host's — the diff --git a/src/index.ts b/src/index.ts index 0310e2b..025b157 100644 --- a/src/index.ts +++ b/src/index.ts @@ -770,6 +770,9 @@ export { TICKS_PER_SECOND, TICKS_PER_MS, INVALID_ZERO_WINDOW_TICKS, + REORDER_PERIODS, + MAX_WINDOW_DIVISOR, + reorderWindowTicks, } from './core/StreamTimeline.js'; export type { StreamStamp, diff --git a/tests/calibration/stream-clients.test.ts b/tests/calibration/stream-clients.test.ts index 0b8bec7..d6f3eb9 100644 --- a/tests/calibration/stream-clients.test.ts +++ b/tests/calibration/stream-clients.test.ts @@ -576,6 +576,49 @@ describe('Shimmer3RClient — real-world time on the stream', () => { expect(client.timelineState.wraps).toBe(1); }); + it('sizes the reorder window from the rate the inquiry reported', async () => { + /* The wiring that is easy to drop and impossible to notice: without it the + timeline falls back to an eighth of the modulo — 2097152 ticks, 409x + wider than the window this rate asks for — and every backward step up to + 64 s reads as a reordered packet instead of a roll-over. Nothing about + the stream looks wrong until a recording comes out 512 s short. */ + const t = scriptedDevice({ channelIds: [0x0a, 0x0b, 0x0c] }); + const client = new Shimmer3RClient({ transport: t }); + await client.connect(); + await client.inquiry(); + client.anchorStreamClock = false; + await client.startStreaming(); + + expect(client.samplingRateHz).toBeCloseTo(51.2, 9); + expect(client.timelineState.reorderWindowTicks).toBe(5120); // 8 x 640 ticks + }); + + it('places a swapped pair of frames where they were taken, not a modulo later', async () => { + const t = scriptedDevice({ channelIds: [0x0a, 0x0b, 0x0c] }); + const payload = [...u16le(0), ...u16le(0), ...u16le(0)]; + const client = new Shimmer3RClient({ transport: t }); + const received: ObjectCluster[] = []; + client.onStreamFrame = (oc) => received.push(oc); + await client.connect(); + await client.inquiry(); + client.anchorStreamClock = false; + await client.startStreaming(); + + // Two adjacent frames delivered the wrong way round. The fifth is only + // there to close the fourth: double-preamble sync needs a following frame. + for (const ts of [10000, 11280, 10640, 11920, 12560]) t.notify(frame(ts, payload)); + await tick(); + + expect(received.length).toBeGreaterThanOrEqual(4); + const ms = received.map((oc) => oc.get('TIMESTAMP', 'cal')!.value); + // Each frame is placed when it was taken — so the series dips, honestly, + // rather than gaining 512 s. + for (const [i, ticks] of [10000, 11280, 10640, 11920].entries()) { + expect(ms[i], `frame ${i}`).toBeCloseTo(ticks / 32.768, 9); + } + expect(client.timelineState.wraps).toBe(0); + }); + it('unwraps the 16-bit counter too, when the client was asked for one', () => { /* `timestampFmt: 'u16'` is a public option and the packet parser honours it, but the timeline was constructed at 24 bits and never told — so a diff --git a/tests/core/stream-timeline-vectors.test.ts b/tests/core/stream-timeline-vectors.test.ts new file mode 100644 index 0000000..9ceed07 --- /dev/null +++ b/tests/core/stream-timeline-vectors.test.ts @@ -0,0 +1,179 @@ +import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { + INVALID_ZERO_WINDOW_TICKS, + MAX_WINDOW_DIVISOR, + REORDER_PERIODS, + StreamTimeline, + TICKS_PER_SECOND, + reorderWindowTicks, + type TimestampBits, +} from '../../src/core/StreamTimeline.js'; + +/** + * The timestamp-unwrap rule, run against the vectors every Shimmer host API is + * checked against. + * + * The file is specified and generated in the firmware repository — + * `log-and-stream-common`, `Test/conformance/timestamp_unwrap.json`, beside the + * prose it encodes and a reference implementation that regenerates and + * re-checks it in CI. The copy here is byte-identical: git blob + * `367efd9979634e8e6fdfbb60a7f7cc64df156721` in both repositories, which is + * what `git hash-object` on either file prints. Every other Shimmer host API + * runs the same file. + * + * Why go to that trouble for what looks like arithmetic: five implementations + * of one wire format drifted apart once already, and the same unwrap defect sat + * in all five for years — each reviewed on its own, against prose, by people + * who had no way to run the others. A shared file turns "these should agree" + * into something a suite fails on. + */ + +interface Vector { + id: string; + description: string; + timestampBits: number; + modulo: number; + reorderWindowTicks: number; + raw: number[]; + expectedUnwrapped: number[]; + expectedRejected: boolean[]; + expectedFinalCycle: number; +} + +interface DerivationCase { + samplingRateHz: number | null; + timestampBits: number; + expectedReorderWindowTicks: number; + tolerance: number; +} + +interface VectorFile { + schemaVersion: number; + revision: number; + spec: string; + ticksPerSecond: number; + invalidZeroWindowTicks: number; + reorderPeriods: number; + maxWindowDivisor: number; + windowDerivation: { rule: string; cases: DerivationCase[] }; + vectors: Vector[]; +} + +const vectorPath = fileURLToPath(new URL('../fixtures/timestamp_unwrap.json', import.meta.url)); +const doc = JSON.parse(readFileSync(vectorPath, 'utf8')) as VectorFile; + +/** + * Every vector id, written out. + * + * A vector that stops being run is a vector that stops protecting anything, and + * nothing else here would notice: a loop over whatever the file happens to + * contain passes just as happily over a shorter file. Updating this list is the + * moment to ask what changed upstream. + */ +const EXPECTED_IDS = [ + 'monotonic-24bit', + 'wrap-24bit', + 'wrap-lands-on-zero-24bit', + 'invalid-zero-signature-24bit', + 'invalid-zero-no-cascade-24bit', + 'first-sample-zero-24bit', + 'wrap-16bit', + 'zero-on-16bit-is-a-wrap', + 'backward-step-outside-window-is-a-wrap-24bit', + 'duplicate-24bit', + 'reorder-one-period-24bit', + 'reorder-one-period-16bit', + 'reorder-across-wrap-boundary-24bit', + 'wrap-after-heavy-loss-24bit', + 'wrap-after-heavy-loss-16bit', + 'wrap-spanning-dropout-1p8s-16bit', + 'wrap-spanning-dropout-152s-24bit', + 'rate-unknown-backward-step-is-a-wrap-24bit', + 'rate-unknown-zero-still-rejected-24bit', + 'zero-within-window-of-origin-24bit', + 'zero-within-window-after-wrap-24bit', + 'reorder-window-boundary-inclusive-24bit', + 'reorder-window-boundary-exclusive-24bit', + 'low-rate-clamp-16bit', + 'high-rate-reorder-24bit', + 'reorder-beyond-eight-periods-is-a-wrap-24bit', +]; + +describe('shared timestamp-unwrap vectors', () => { + it('is the revision this suite was written against', () => { + // A bumped revision means the rule moved. Read the upstream change before + // touching anything here. + expect(doc.schemaVersion).toBe(1); + expect(doc.revision).toBe(1); + }); + + it('agrees with this SDK on the constants the rule is built from', () => { + expect(doc.ticksPerSecond).toBe(TICKS_PER_SECOND); + expect(doc.invalidZeroWindowTicks).toBe(INVALID_ZERO_WINDOW_TICKS); + expect(doc.reorderPeriods).toBe(REORDER_PERIODS); + expect(doc.maxWindowDivisor).toBe(MAX_WINDOW_DIVISOR); + }); + + it('runs every vector in the file', () => { + expect(doc.vectors.map((v) => v.id)).toEqual(EXPECTED_IDS); + }); + + it.each(doc.vectors.map((v) => [v.id, v] as const))('%s', (_id, vector) => { + /* The window comes from the vector, not from a rate: a rate is a floating + divide away from a window, and the point of the file is that four + implementations classify the same sequence identically. The derivation + is checked separately below. */ + const t = new StreamTimeline({ + timestampBits: vector.timestampBits as TimestampBits, + reorderWindowTicks: vector.reorderWindowTicks, + }); + expect(t.reorderWindowTicks).toBe(vector.reorderWindowTicks); + + // No host clock: the missed-wrap recovery is a live-link extra this SDK has + // and the shared rule does not, and the vectors are the shared rule. + const stamps = vector.raw.map((raw) => t.stamp(raw)); + + expect( + stamps.map((s) => s.unwrappedTicks), + `${vector.id}: ${vector.description}`, + ).toEqual(vector.expectedUnwrapped); + expect( + stamps.map((s) => s.invalid), + `${vector.id}: rejected`, + ).toEqual(vector.expectedRejected); + + const finalUnwrapped = stamps[stamps.length - 1]!.unwrappedTicks; + expect(Math.floor(finalUnwrapped / vector.modulo), `${vector.id}: final cycle`).toBe( + vector.expectedFinalCycle, + ); + }); + + it.each( + doc.windowDerivation.cases.map( + (c) => [`${String(c.samplingRateHz)} Hz at ${c.timestampBits} bits`, c] as const, + ), + )('derives the window for %s', (_label, c) => { + const modulo = 2 ** c.timestampBits; + const got = reorderWindowTicks(c.samplingRateHz, modulo); + if (c.tolerance === 0) { + expect(got).toBe(c.expectedReorderWindowTicks); + } else { + expect(Math.abs(got - c.expectedReorderWindowTicks)).toBeLessThanOrEqual(c.tolerance); + } + }); + + it('never turns an unusable rate into an infinite window', () => { + /* The derivation cases above cover 0, null and a negative rate. This is the + one the file cannot express in JSON, and it is the one that bites: in a + language where 32768 / 0 is Infinity rather than an error, a rate that + happens to read zero produces a window wider than the modulo, every + backward step becomes a reorder, and the unwrap silently stops counting + wraps — the original bug, restored, with no symptom until a recording is + 512 s short. */ + expect(reorderWindowTicks(Number.POSITIVE_INFINITY, 2 ** 24)).toBe(0); + expect(reorderWindowTicks(Number.NaN, 2 ** 24)).toBe(0); + expect(reorderWindowTicks(undefined, 2 ** 24)).toBe(0); + }); +}); diff --git a/tests/core/stream-timeline.test.ts b/tests/core/stream-timeline.test.ts index 4c26338..c4bb97b 100644 --- a/tests/core/stream-timeline.test.ts +++ b/tests/core/stream-timeline.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from 'vitest'; -import { StreamTimeline, TICKS_PER_MS, TICKS_PER_SECOND } from '../../src/core/StreamTimeline.js'; +import { + StreamTimeline, + TICKS_PER_MS, + TICKS_PER_SECOND, + reorderWindowTicks, +} from '../../src/core/StreamTimeline.js'; const MOD24 = 2 ** 24; const MOD16 = 2 ** 16; @@ -86,6 +91,123 @@ describe('unwrapping', () => { }); }); +describe('the reorder window', () => { + it('is eight sample periods once the rate is known', () => { + const t = new StreamTimeline(); + // The fallback, for a timeline nobody has told the rate to. + expect(t.reorderWindowTicks).toBe(MOD24 / 8); + + t.setSamplingRateHz(51.2); + expect(t.reorderWindowTicks).toBe(5120); // 8 x 640 ticks + t.setSamplingRateHz(32768 / 65); // 504.123 Hz, the customer's rate + expect(t.reorderWindowTicks).toBe(520); + + // And back, so a client that loses its rate does not keep a stale window. + t.setSamplingRateHz(null); + expect(t.reorderWindowTicks).toBe(MOD24 / 8); + }); + + it('is clamped, so a very low rate still leaves wraps detectable', () => { + /* At 1 Hz eight periods is 262144 ticks — four whole 16-bit modulos. A + window at or above the modulo leaves no backward step large enough to be + a wrap, and the unwrap would stop counting them entirely. */ + const t = new StreamTimeline({ timestampBits: 16, samplingRateHz: 1 }); + expect(t.reorderWindowTicks).toBe(MOD16 / 8); + expect(reorderWindowTicks(1, MOD24)).toBe(262144); // unclamped at 24 bits + }); + + it('is recomputed when the counter width changes', () => { + // The clamp is against the modulo, so the width moves the answer. + const t = new StreamTimeline({ samplingRateHz: 1 }); + expect(t.reorderWindowTicks).toBe(262144); + t.setTimestampBits(16); + expect(t.reorderWindowTicks).toBe(MOD16 / 8); + // The rate is not a property of the counter width, and is kept. + t.setTimestampBits(24); + expect(t.reorderWindowTicks).toBe(262144); + }); + + it('can be set outright, overriding the rate', () => { + const t = new StreamTimeline({ samplingRateHz: 51.2, reorderWindowTicks: 0 }); + expect(t.reorderWindowTicks).toBe(0); + t.setReorderWindowTicks(null); + expect(t.reorderWindowTicks).toBe(5120); + }); + + it('reads a 0.3 s backward step as a wrap, which the fallback does not', () => { + /* The discriminator between sizing the window in sample periods and sizing + it as a fraction of the modulo. Sixteen samples late is not a reorder — + reordering swaps packets that are adjacent in time — so the honest + reading is the other one: the counter rolled over during a long dropout. + An eighth of the modulo is 64 s wide and calls it a reorder, placing the + sample 0.3 s back and losing the wrap for the rest of the session. */ + const rated = new StreamTimeline({ samplingRateHz: 51.2 }); + rated.stamp(16700000); + expect(rated.stamp(16690000).unwrappedTicks).toBe(16700000 + (MOD24 - 10000)); + expect(rated.state.wraps).toBe(1); + + const unrated = new StreamTimeline(); + unrated.stamp(16700000); + expect(unrated.stamp(16690000).unwrappedTicks).toBe(16690000); + expect(unrated.state.wraps).toBe(0); + }); + + it('does not misread a 1.8 s dropout across the 16-bit counter', () => { + /* The same defect where it actually bites: the 16-bit modulo is 2 s, so an + eighth of it is 0.25 s, and every dropout between 1.75 s and 2.0 s reads + as a reorder. A 1.75 s Bluetooth gap is an ordinary afternoon. */ + const t = new StreamTimeline({ timestampBits: 16, samplingRateHz: 51.2 }); + const lost = Math.round(1.8 * TICKS_PER_SECOND); + t.stamp(60000); + expect(t.stamp((60000 + lost) % MOD16).unwrappedTicks).toBe(60000 + lost); + expect(t.state.wraps).toBe(1); + }); + + it('still reads a wrap after heavy loss as a wrap', () => { + // Forward motion is the DEFAULT. However much was lost, the counter still + // rolled over — a rule that defaults to "corrupt" fails exactly here. + const t = new StreamTimeline({ samplingRateHz: 51.2 }); + t.stamp(16000000); + expect(t.stamp(100).unwrappedTicks).toBe(MOD24 + 100); + }); + + it('places a zero inside the window rather than rejecting it, but only a real window', () => { + /* Order of the two tests: a reorder is judged first, so a zero within a + window of an origin is placed where it was taken. That is the order + every Shimmer host API uses, and at 520 ticks the cost of choosing + wrongly is 16 ms. + + It does not extend to the rate-unknown fallback. At an eighth of the + modulo the same rule would read an unstamped record up to 64 s past an + origin as a packet 64 s late, place it 64 s early and call it valid — + worse than either answer it is choosing between. */ + const rated = new StreamTimeline({ samplingRateHz: 32768 / 65 }); + rated.stamp(300); + rated.stamp(365); + const placed = rated.stamp(0); + expect(placed.invalid).toBe(false); + expect(placed.unwrappedTicks).toBe(0); + + const unrated = new StreamTimeline(); + unrated.stamp(300); + unrated.stamp(365); + const rejected = unrated.stamp(0); + expect(rejected.invalid).toBe(true); + expect(rejected.unwrappedTicks).toBe(365); + }); + + it('does not let a packet late from before a wrap boundary cost two modulos', () => { + /* Why the rule is stated on the MODULAR forward distance and not on a + comparison of unwrapped values. This packet's candidate is ABOVE its + predecessor, so a comparison accepts it as forward motion of nearly a + whole modulo — and then reads the next real sample as a second wrap. */ + const t = new StreamTimeline({ samplingRateHz: 32768 / 65 }); + const got = [MOD24 - 10, 5, MOD24 - 10, 70].map((r) => t.stamp(r).unwrappedTicks); + expect(got).toEqual([MOD24 - 10, MOD24 + 5, MOD24 - 10, MOD24 + 70]); + expect(t.state.wraps).toBe(1); + }); +}); + describe('unwrapping — cases an adversarial review found', () => { it('reads a long forward gap on the 16-bit counter as forward, not backwards', () => { /* The 16-bit modulo is 2 s, so a single missed Bluetooth window is a @@ -551,6 +673,9 @@ describe('anchor lifecycle', () => { skewMs: null, wraps: 0, timestampBits: 24, + // No rate has been given, so the window is the fallback: an eighth of + // the modulo. `setSamplingRateHz` replaces it with eight sample periods. + reorderWindowTicks: 2 ** 24 / 8, }); }); }); diff --git a/tests/fixtures/timestamp_unwrap.json b/tests/fixtures/timestamp_unwrap.json new file mode 100644 index 0000000..367efd9 --- /dev/null +++ b/tests/fixtures/timestamp_unwrap.json @@ -0,0 +1,729 @@ +{ + "schemaVersion": 1, + "revision": 1, + "spec": "docs/SHIMMER3_STREAMING_DATA_FORMAT.md#21-the-timestamp", + "ticksPerSecond": 32768, + "invalidZeroWindowTicks": 32768, + "reorderPeriods": 8, + "maxWindowDivisor": 8, + "windowDerivation": { + "rule": "0 when the rate is unknown, NaN, infinite or <= 0; otherwise min(reorderPeriods * ticksPerSecond / rateHz, modulo / maxWindowDivisor). The tick domain is the 32768 Hz real-time clock the packet counter runs on, never a TCXO sampling clock.", + "cases": [ + { + "samplingRateHz": 504.12307692307695, + "timestampBits": 24, + "expectedReorderWindowTicks": 520.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 51.2, + "timestampBits": 16, + "expectedReorderWindowTicks": 5120.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 51.2, + "timestampBits": 24, + "expectedReorderWindowTicks": 5120.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 512.0, + "timestampBits": 24, + "expectedReorderWindowTicks": 512.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 1024.0, + "timestampBits": 24, + "expectedReorderWindowTicks": 256.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 1.0, + "timestampBits": 16, + "expectedReorderWindowTicks": 8192.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 1.0, + "timestampBits": 24, + "expectedReorderWindowTicks": 262144.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 0.0, + "timestampBits": 24, + "expectedReorderWindowTicks": 0.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": null, + "timestampBits": 24, + "expectedReorderWindowTicks": 0.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": -5.0, + "timestampBits": 24, + "expectedReorderWindowTicks": 0.0, + "tolerance": 0.0 + }, + { + "samplingRateHz": 512.2950819672132, + "timestampBits": 24, + "expectedReorderWindowTicks": 511.705088, + "tolerance": 0.0001 + } + ] + }, + "vectors": [ + { + "id": "monotonic-24bit", + "description": "Ordinary forward motion at 504.123 Hz; nothing is classified.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 1000, + 1065, + 1130 + ], + "expectedUnwrapped": [ + 1000, + 1065, + 1130 + ], + "expectedRejected": [ + false, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "wrap-24bit", + "description": "A genuine roll-over: the counter reached its last tick and started again.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 16777200, + 16 + ], + "expectedUnwrapped": [ + 16777200, + 16777232 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "wrap-lands-on-zero-24bit", + "description": "A roll-over that lands exactly on zero. Its predecessor is at the top of the range, which is what separates it from a record that was never stamped.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 16777116, + 0 + ], + "expectedUnwrapped": [ + 16777116, + 16777216 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "invalid-zero-signature-24bit", + "description": "The signature recovered from an affected recording: a stall, then a record the firmware never stamped. Read as a roll-over it costs 512 s.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 7406116, + 7406506, + 0, + 7406571 + ], + "expectedUnwrapped": [ + 7406116, + 7406506, + 7406506, + 7406571 + ], + "expectedRejected": [ + false, + false, + true, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "invalid-zero-no-cascade-24bit", + "description": "Rejecting one record must not disturb the next: the following sample reads above the retained predecessor and is accepted normally.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 7406506, + 0, + 7406571, + 7406636 + ], + "expectedUnwrapped": [ + 7406506, + 7406506, + 7406571, + 7406636 + ], + "expectedRejected": [ + false, + true, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "first-sample-zero-24bit", + "description": "A stream may legitimately open on zero; nothing precedes it to contradict it.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 0, + 65, + 130 + ], + "expectedUnwrapped": [ + 0, + 65, + 130 + ], + "expectedRejected": [ + false, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "wrap-16bit", + "description": "The 2-byte counter older firmware uses wraps every 2 s.", + "timestampBits": 16, + "modulo": 65536, + "samplingRateHz": 51.2, + "ticksPerSample": 640.0, + "reorderWindowTicks": 5120, + "raw": [ + 65436, + 28 + ], + "expectedUnwrapped": [ + 65436, + 65564 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "zero-on-16bit-is-a-wrap", + "description": "The invalid-zero rule is scoped to the 3-byte counter. A 2-byte counter's whole range is 2 s, so a stall really can cross it.", + "timestampBits": 16, + "modulo": 65536, + "samplingRateHz": 51.2, + "ticksPerSample": 640.0, + "reorderWindowTicks": 5120, + "raw": [ + 30000, + 0 + ], + "expectedUnwrapped": [ + 30000, + 65536 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "backward-step-outside-window-is-a-wrap-24bit", + "description": "Only an exact zero is exempt. A value of 1 is read as a roll-over, as before.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 7406506, + 1 + ], + "expectedUnwrapped": [ + 7406506, + 16777217 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "duplicate-24bit", + "description": "The same counter value twice: hold the timeline, do not count a wrap.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 1000, + 1065, + 1065, + 1130 + ], + "expectedUnwrapped": [ + 1000, + 1065, + 1065, + 1130 + ], + "expectedRejected": [ + false, + false, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "reorder-one-period-24bit", + "description": "Two adjacent packets swapped. Each is placed where it was taken, so the output is not monotonic - and no modulo is added.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 1000, + 1130, + 1065, + 1195 + ], + "expectedUnwrapped": [ + 1000, + 1130, + 1065, + 1195 + ], + "expectedRejected": [ + false, + false, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "reorder-one-period-16bit", + "description": "The same swap on the 2-byte counter at 51.2 Hz (640 ticks per sample).", + "timestampBits": 16, + "modulo": 65536, + "samplingRateHz": 51.2, + "ticksPerSample": 640.0, + "reorderWindowTicks": 5120, + "raw": [ + 40000, + 41280, + 40640, + 41920 + ], + "expectedUnwrapped": [ + 40000, + 41280, + 40640, + 41920 + ], + "expectedRejected": [ + false, + false, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "reorder-across-wrap-boundary-24bit", + "description": "A packet arriving late from BEFORE a roll-over. In the modular form this is a small step back across the boundary; a host comparing unwrapped values instead sees forward motion of nearly a modulo and never recovers.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 16777206, + 5, + 16777206, + 70 + ], + "expectedUnwrapped": [ + 16777206, + 16777221, + 16777206, + 16777286 + ], + "expectedRejected": [ + false, + false, + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "wrap-after-heavy-loss-24bit", + "description": "A roll-over preceded by a long dropout. Forward motion is the default, so this stays a wrap however much was lost before it.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 16000000, + 100 + ], + "expectedUnwrapped": [ + 16000000, + 16777316 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "wrap-after-heavy-loss-16bit", + "description": "The same on the 2-byte counter.", + "timestampBits": 16, + "modulo": 65536, + "samplingRateHz": 51.2, + "ticksPerSample": 640.0, + "reorderWindowTicks": 5120, + "raw": [ + 60000, + 1000 + ], + "expectedUnwrapped": [ + 60000, + 66536 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "wrap-spanning-dropout-1p8s-16bit", + "description": "A 1.8 s dropout across the 2 s counter - an ordinary Bluetooth gap. The window must be small enough that this is still read as forward motion.", + "timestampBits": 16, + "modulo": 65536, + "samplingRateHz": 51.2, + "ticksPerSample": 640.0, + "reorderWindowTicks": 5120, + "raw": [ + 60000, + 53446 + ], + "expectedUnwrapped": [ + 60000, + 118982 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "wrap-spanning-dropout-152s-24bit", + "description": "A long dropout spanning the 3-byte counter's roll-over.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 16000000, + 4222784 + ], + "expectedUnwrapped": [ + 16000000, + 21000000 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "rate-unknown-backward-step-is-a-wrap-24bit", + "description": "With no rate the reorder branch is disabled, so a swapped pair is read as a roll-over. Worse than knowing the rate, identical to older hosts, and safe.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": null, + "ticksPerSample": null, + "reorderWindowTicks": 0, + "raw": [ + 1000, + 1130, + 1065, + 1195 + ], + "expectedUnwrapped": [ + 1000, + 1130, + 16778281, + 16778411 + ], + "expectedRejected": [ + false, + false, + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "rate-unknown-zero-still-rejected-24bit", + "description": "Rejecting an unstamped record needs no rate, so it still happens.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": null, + "ticksPerSample": null, + "reorderWindowTicks": 0, + "raw": [ + 7406506, + 0, + 7406571 + ], + "expectedUnwrapped": [ + 7406506, + 7406506, + 7406571 + ], + "expectedRejected": [ + false, + true, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "zero-within-window-of-origin-24bit", + "description": "A zero close enough to the origin to be a reordered packet is kept as one, not rejected - the reorder test is applied first.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 300, + 365, + 0, + 430 + ], + "expectedUnwrapped": [ + 300, + 365, + 0, + 430 + ], + "expectedRejected": [ + false, + false, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "zero-within-window-after-wrap-24bit", + "description": "The origin recurs after every roll-over, so the same ordering applies there.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 16777100, + 100, + 165, + 0, + 230 + ], + "expectedUnwrapped": [ + 16777100, + 16777316, + 16777381, + 16777216, + 16777446 + ], + "expectedRejected": [ + false, + false, + false, + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "reorder-window-boundary-inclusive-24bit", + "description": "Exactly at the window: reordered. 512 Hz gives a window of exactly 512 ticks in every language, with no rounding to argue about.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 512.0, + "ticksPerSample": 64.0, + "reorderWindowTicks": 512, + "raw": [ + 10512, + 10000 + ], + "expectedUnwrapped": [ + 10512, + 10000 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "reorder-window-boundary-exclusive-24bit", + "description": "One tick past the window: a roll-over.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 512.0, + "ticksPerSample": 64.0, + "reorderWindowTicks": 512, + "raw": [ + 10513, + 10000 + ], + "expectedUnwrapped": [ + 10513, + 16787216 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "low-rate-clamp-16bit", + "description": "At 1 Hz the unclamped window would exceed the 2-byte modulo and no backward step could ever be a wrap. The clamp keeps wraps detectable.", + "timestampBits": 16, + "modulo": 65536, + "samplingRateHz": 32.0, + "ticksPerSample": 1024.0, + "reorderWindowTicks": 8192, + "raw": [ + 60000, + 1000 + ], + "expectedUnwrapped": [ + 60000, + 66536 + ], + "expectedRejected": [ + false, + false + ], + "expectedFinalCycle": 1 + }, + { + "id": "high-rate-reorder-24bit", + "description": "At 1024 Hz the window is 256 ticks; a swapped pair is still caught.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 1024.0, + "ticksPerSample": 32.0, + "reorderWindowTicks": 256, + "raw": [ + 5000, + 5032, + 5000, + 5064 + ], + "expectedUnwrapped": [ + 5000, + 5032, + 5000, + 5064 + ], + "expectedRejected": [ + false, + false, + false, + false + ], + "expectedFinalCycle": 0 + }, + { + "id": "reorder-beyond-eight-periods-is-a-wrap-24bit", + "description": "A packet more than eight sample periods late is indistinguishable from a roll-over and is read as one. This is the limit of what a counter can say.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 1024.0, + "ticksPerSample": 32.0, + "reorderWindowTicks": 256, + "raw": [ + 5000, + 5288, + 5000 + ], + "expectedUnwrapped": [ + 5000, + 5288, + 16782216 + ], + "expectedRejected": [ + false, + false, + false + ], + "expectedFinalCycle": 1 + } + ] +} diff --git a/tests/shimmer3/transport-loopback.test.ts b/tests/shimmer3/transport-loopback.test.ts index 0828b8e..e0146cc 100644 --- a/tests/shimmer3/transport-loopback.test.ts +++ b/tests/shimmer3/transport-loopback.test.ts @@ -247,6 +247,26 @@ describe('Shimmer3Client streaming', () => { expect(frames[0].deviceId).toBe('Shimmer3-TEST'); }); + it('sizes the reorder window from the rate the inquiry reported', async () => { + /* The same wiring as `Shimmer3RClient`, and the same reason to pin it: the + two clients are parallel implementations of one wire format, so a + one-line omission in either is invisible until a recording is 512 s + long. 0x80 0x02 in INQUIRY_MSG is a divider of 640 — 51.2 Hz. */ + const { t, client } = await connected(); + t.setOnWrite((bytes, tr) => { + if (bytes[0] === OPCODES.INQUIRY_COMMAND) + setTimeout(() => tr.notify([ACK, ...INQUIRY_MSG]), 0); + else if (bytes[0] === OPCODES.START_STREAMING_COMMAND) setTimeout(() => tr.notify([ACK]), 0); + }); + await client.inquiry(); + client.anchorStreamClock = false; + await client.startStreaming(); + + expect(client.samplingRateHz).toBeCloseTo(51.2, 5); + // Eight sample periods of 640 ticks — not the 2097152-tick fallback. + expect(client.timelineState.reorderWindowTicks).toBe(5120); + }); + it('labels frames from an anonymous link with the generation name', async () => { /* The fallback that must survive: an ObjectCluster needs an attributable deviceId even when the link supplies no name, which is precisely why the From 28553febc260a0d5634dae54f2d848d74b09850f Mon Sep 17 00:00:00 2001 From: Mark Nolan Date: Thu, 17 Sep 2026 14:18:29 +0100 Subject: [PATCH 2/2] DEV-1023: clamp an explicit reorder window, and take the new shared vector Two things an adversarial review of this branch turned up. The derivation clamps the window to an eighth of the counter's range, because a window at or above the modulo leaves no backward step large enough to be a roll-over - the unwrap stops counting them and a recording quietly runs short. setReorderWindowTicks and the constructor option did not clamp, so a caller could express the one thing the clamp exists to make impossible: new StreamTimeline({ timestampBits: 16, reorderWindowTicks: 100000 }) stamp(65000); stamp(100) -> unwrapped = 100, wraps = 0 A plain 2-byte roll-over, silently lost. Clamped in _recomputeReorderWindow rather than in the setter, so it follows a later setTimestampBits. Nothing passes an override today except the conformance vectors, all of which are at or below the clamp, so this changes no existing behaviour. The second was a divergence rather than a defect here: hosts that keep an unwrapped value and a cycle count encode "no sample yet" as (0, 0), which a reorder landing exactly on the counter's origin can reach mid stream. This timeline keeps lastRaw with a null sentinel and was already right, but the sequence is now a shared vector - [520, 0, 2^24 - 16] - and this suite runs it. It is the first vector whose final cycle is negative. Also: setSamplingRateHz's doc said a client calls it whenever the rate changes. Both clients call it once per stream, at _prepareStreamTimeline. Calling it mid stream does work, and a test covers that, but the sentence described a capability as though it were the wiring. 1944 tests pass; typecheck, lint and prettier clean. Co-Authored-By: Claude Opus 5 --- src/core/StreamTimeline.ts | 20 ++++++++++++++--- tests/core/stream-timeline-vectors.test.ts | 3 ++- tests/core/stream-timeline.test.ts | 20 +++++++++++++++++ tests/fixtures/timestamp_unwrap.json | 25 ++++++++++++++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/core/StreamTimeline.ts b/src/core/StreamTimeline.ts index e21d840..315fec9 100644 --- a/src/core/StreamTimeline.ts +++ b/src/core/StreamTimeline.ts @@ -325,8 +325,9 @@ export class StreamTimeline { * outright when the rate is unknown. Pass the rate and the question does not * arise: the derived window is better in every case. * - * Cheap and idempotent — a client calls it whenever the rate changes, and the - * next sample is judged by the new window. + * Cheap and idempotent. Both clients call it once per stream, from the rate + * the inquiry reported; calling it mid-stream is allowed and the next sample + * is judged by the new window. */ setSamplingRateHz(samplingRateHz: number | null): void { this._samplingRateHz = @@ -339,6 +340,10 @@ export class StreamTimeline { /** * Set the reorder window outright, in ticks, or `null` to go back to deriving * it from the sampling rate. `0` disables the branch. + * + * Clamped to an eighth of the counter's range, as a derived window is — see + * {@link _recomputeReorderWindow}. {@link reorderWindowTicks} reports what is + * actually in force. */ setReorderWindowTicks(ticks: number | null): void { this._reorderWindowOverride = @@ -371,7 +376,16 @@ export class StreamTimeline { /** Explicit window, else the rate-derived one, else the legacy fallback. */ private _recomputeReorderWindow(): void { if (this._reorderWindowOverride !== null) { - this._reorderWindow = this._reorderWindowOverride; + /* Clamped like a derived window, and for the same reason: a window at or + above the modulo leaves no backward step large enough to be a wrap, so + the unwrap stops counting them and a recording quietly runs short. That + must not be expressible, whether the number came from a rate or from a + caller. Clamped here rather than in the setter because + {@link setTimestampBits} can change the modulo afterwards. */ + this._reorderWindow = Math.min( + this._reorderWindowOverride, + this._modulo / MAX_WINDOW_DIVISOR, + ); } else if (this._samplingRateHz !== null) { this._reorderWindow = reorderWindowTicks(this._samplingRateHz, this._modulo); } else { diff --git a/tests/core/stream-timeline-vectors.test.ts b/tests/core/stream-timeline-vectors.test.ts index 9ceed07..c545124 100644 --- a/tests/core/stream-timeline-vectors.test.ts +++ b/tests/core/stream-timeline-vectors.test.ts @@ -19,7 +19,7 @@ import { * `log-and-stream-common`, `Test/conformance/timestamp_unwrap.json`, beside the * prose it encodes and a reference implementation that regenerates and * re-checks it in CI. The copy here is byte-identical: git blob - * `367efd9979634e8e6fdfbb60a7f7cc64df156721` in both repositories, which is + * `de91de25accc7c74c0422f7e279a535da92579d9` in both repositories, which is * what `git hash-object` on either file prints. Every other Shimmer host API * runs the same file. * @@ -99,6 +99,7 @@ const EXPECTED_IDS = [ 'low-rate-clamp-16bit', 'high-rate-reorder-24bit', 'reorder-beyond-eight-periods-is-a-wrap-24bit', + 'reorder-onto-origin-then-earlier-packet-24bit', ]; describe('shared timestamp-unwrap vectors', () => { diff --git a/tests/core/stream-timeline.test.ts b/tests/core/stream-timeline.test.ts index c4bb97b..4ac16d1 100644 --- a/tests/core/stream-timeline.test.ts +++ b/tests/core/stream-timeline.test.ts @@ -127,6 +127,26 @@ describe('the reorder window', () => { expect(t.reorderWindowTicks).toBe(262144); }); + it('clamps an explicit window too, so a wrap is always detectable', () => { + /* A window at or above the modulo leaves no backward step large enough to + be a wrap: the unwrap stops counting them and the recording quietly runs + short. The derivation has always been clamped; the explicit setter was + not, so a caller could express the one thing the clamp exists to make + impossible. */ + const t = new StreamTimeline({ timestampBits: 16, reorderWindowTicks: 100000 }); + expect(t.reorderWindowTicks).toBe(MOD16 / 8); + + t.stamp(65000); + expect(t.stamp(100).unwrappedTicks).toBe(MOD16 + 100); + expect(t.state.wraps).toBe(1); + + // And the clamp follows the modulo, not the moment the setter was called. + const wide = new StreamTimeline({ reorderWindowTicks: 1_000_000 }); + expect(wide.reorderWindowTicks).toBe(1_000_000); // fits at 24 bits + wide.setTimestampBits(16); + expect(wide.reorderWindowTicks).toBe(MOD16 / 8); + }); + it('can be set outright, overriding the rate', () => { const t = new StreamTimeline({ samplingRateHz: 51.2, reorderWindowTicks: 0 }); expect(t.reorderWindowTicks).toBe(0); diff --git a/tests/fixtures/timestamp_unwrap.json b/tests/fixtures/timestamp_unwrap.json index 367efd9..de91de2 100644 --- a/tests/fixtures/timestamp_unwrap.json +++ b/tests/fixtures/timestamp_unwrap.json @@ -724,6 +724,31 @@ false ], "expectedFinalCycle": 1 + }, + { + "id": "reorder-onto-origin-then-earlier-packet-24bit", + "description": "A reorder that lands exactly on the counter's origin, followed by a packet from just before it. A host that keeps an unwrapped value and a cycle count rather than the previous raw value has to encode 'no sample yet' somehow, and (0, 0) is the obvious choice - but this sequence reaches (0, 0) mid stream, so that host reads the third packet as a first sample and places it a whole modulo late. The state has to be distinguishable from the value.", + "timestampBits": 24, + "modulo": 16777216, + "samplingRateHz": 504.12307692307695, + "ticksPerSample": 65.0, + "reorderWindowTicks": 520, + "raw": [ + 520, + 0, + 16777200 + ], + "expectedUnwrapped": [ + 520, + 0, + -16 + ], + "expectedRejected": [ + false, + false, + false + ], + "expectedFinalCycle": -1 } ] }