Skip to content

DEV-1023: one timestamp-unwrap rule — invalid zeros, reorders, duplicates and wraps - #300

Merged
MAzalya merged 5 commits into
masterfrom
DEV-1023_reject_isolated_zero_timestamp
Sep 18, 2026
Merged

MAzalya merged 5 commits into
masterfrom
DEV-1023_reject_isolated_zero_timestamp

Conversation

@marknolan

@marknolan marknolan commented Sep 16, 2026

Copy link
Copy Markdown
Member

Supersedes #301, whose reorder and duplicate detection is adopted here with credit on
the commit. Depends on log-and-stream-common#136,
which specifies the rule and ships the conformance vectors this runs.

The fault

Firmware stamps a packet when its sample tick starts it and does not publish a packet it
never stamped, so 0x000000 in the timestamp field means the record is invalid — not that
the counter reached its origin. LogAndStream v1.00.x–v1.01.003 could emit one under SD
write back-pressure.

unwrapTimeStamp() read any backward step as a roll-over, and a zero is always below its
predecessor, so each such record added 2²⁴ ticks = 512.002 s to every later sample,
permanently
. A 9 minute 30 second trial holding four of them imported as 43 minutes 38.

Across six devices in one session, every reported duration was an integer multiple of
512 s above the true one, against a single clean device as the control — the evidence
table on DEV-1030 that made the mechanism unambiguous.

The rule

Each sample is classified by its modular forward distance from the previous one, with
forward motion as the default:

  if forward == 0:            hold                          // duplicate
  elif backwards <= W:        lastUnwrapped - backwards     // reordered packet
  elif modulo == 2^24 and raw == 0 and lastRaw < modulo - 32768:
                              reject, state untouched       // never stamped
  else:                       lastUnwrapped + forward       // forward; a wrap iff raw < lastRaw

  W = 0 when the rate is unknown; else min(8 * 32768 / rateHz, modulo / 8)

Three things in it are load-bearing, and all three were verified by mutation rather than
assumed:

why
Modular distance, not unwrapped values Asking "is the candidate below the last one?" misses a packet arriving late from before a wrap boundary — its candidate sits nearly a modulo ahead, so it is accepted and the next real sample reads as a second wrap. 16777206, 5, 16777206, 70 costs 512 s twice
Forward motion is the default A roll-over preceded by a long dropout is still a roll-over, however much was lost
An unknown rate gives zero, never infinity 32768.0 / 0.0 is POSITIVE_INFINITY in Java; an infinite window makes every backward step a reorder and loses every wrap — worse than the naive rule

The window is eight sample periods, not a fraction of the modulo: a reorder swaps adjacent
packets, a dropout spanning the wrap point is most of a modulo, and modulo / 8 confuses
them — on the 2-byte counter every dropout between 1.75 s and 2.0 s would read as a reorder
and lose the wrap, which is an ordinary Bluetooth gap.

It is derived from getRtcClockFreq() (32768 Hz), not getSamplingClockFreq(), which
is 312500 or 255765.625 Hz on a TCXO board and would widen the window about nine and a half
times.

Scope and behaviour notes

  • Shimmer2/2R get a window of zero. Their tick domain is unsettled — this class divides
    their 16-bit counter by 32768 while the C# API divides by 1024 — so a rate-derived window
    would be wrong in one of the two. They keep today's behaviour; the invalid-zero rule
    never applied to a 2-byte counter anyway. Follow-up ticket to settle the clock.
  • The window is derived per sample, so a rate written mid-session is picked up by the
    next one and there is no cached window to reset.
  • The first sample of a stream is passed through rather than measured against the reset
    state, which would otherwise let a first raw value near the top of the range read as a
    packet reordered across a boundary and place a whole recording one modulo early.
  • Reordered output is deliberately not monotonic — a late packet belongs at the time it
    was sampled. Checked against the SD-import consumers (ParserLoggedDataToCSV,
    …ToDatabase, …ToObjectCluster, DockManager, UtilDock): none sorts, diffs or throws
    on a backward step, and none derives duration from the timestamp span. In a file it
    should not fire at all — the firmware writes records sequentially.
  • Two limits, stated rather than hidden: a packet more than eight sample periods late is
    indistinguishable from a roll-over, and a gap longer than a whole modulo cannot be
    recovered from the counter at all.
  • isLastTimestampRejected() lets the SD importer drop the record (the Advance-API PR);
    a live stream carries the previous timestamp for one packet. calculateTrialPacketLoss()
    is skipped for a rejected sample, and resetCalibratedTimeStamp() clears the flag.

Tests

22 tests, 0 failures.

API_00010_TimestampUnwrapVectorsTest runs the 26 shared conformance vectors from
log-and-stream-common, byte-identical at revision 1 / commit 267f693. The C# API,
pyshimmer and the web SDK run the same cases — if the four disagree, one is wrong, which is
the thing nobody could see last time. It asserts the file's revision and its full id list
too, so a vector dropped upstream fails here instead of quietly reducing coverage, plus the
window derivations and the values JSON cannot spell (infinity, NaN, negative).

API_00009_TimestampUnwrapTest keeps covering the rule in a form meant to be read, and
gains cases for the three properties above.

Four mutations were checked, each caught by two or three tests:

mutation caught by
compare unwrapped values instead of modular distances 2
size the window as modulo / 8 3
let an unknown rate become an infinite window 3
drop the first-sample sentinel 2

Running these locally needs the Gradle 8.14.3 wrapper and Guava 33.4.0-jre — master's
Gradle 6.1 cannot resolve Guava's dual-capability metadata (DEV-1013, DEV-1020). Both were
applied temporarily and reverted; the only build change here is the test-only Gson
dependency, which is a plain POM and resolves on 6.1.

Landing

Shimmer-Advance-API #472 drops the rejected rows on SD import and depends on this; its CI
is red until this merges, because it builds against the driver's default branch.

🤖 Generated with Claude Code

marknolan and others added 2 commits September 16, 2026 10:24
The packet tick counter is 3 bytes at 32768 Hz, so it returns to zero every 512
seconds and a host adds a modulo back each time it does. The rule both of the
driver's timestamp paths used - if this sample reads lower than the last, a wrap
happened - is wrong for exactly one input: a record whose timestamp field is
zero.

Firmware stamps a packet when its sample tick starts it and does not publish one
it never stamped, so 00 00 00 in that field means the record is invalid, not
that the counter reached its origin. LogAndStream v1.00.x-v1.01.003 could emit
one under SD write back-pressure. Read as a wrap, a single such record makes
every later sample in the recording 512 seconds late. A customer's 9m30s trial
imported as 43m38s off four of them, and their 2h04m patient recording as 2h38m.

New TimestampUnwrap holds the rule once, pure and static, and both
ShimmerObject and SensorShimmerClock call it instead of keeping a copy each. A
backward step is a roll-over unless the counter is the 3-byte one, the new value
is exactly zero, and the previous value was more than a second below the top of
the range - then the sample is rejected: the caller gets the previous timestamp
back and the wrap count is untouched.

The exemption is deliberately narrow, because the rule it is carving out of is
right the rest of the time:

- a genuine wrap onto zero has a predecessor at the very top of the range, so it
  is still counted as a wrap;
- a 2-byte counter is never rejected, since its whole range is 2 seconds and a
  stall really can cross it;
- any other backward step, including to a value of 1, is still a wrap;
- the first sample of a recording may legitimately be zero.

Rejection cannot cascade. The retained previous value is what the next sample is
compared against, and it reads above it, so the timeline resumes at its true
spacing. That also means a live Bluetooth stream degrades gracefully: one packet
carries the previous timestamp and the rest are unaffected. A file has a better
option - drop the record - and that follows in Shimmer-Advance-API, which is
where SD parsing lives.

isLastTimestampRejected() exposes the outcome for callers that can act on it,
and packet-loss estimation now skips a rejected sample rather than being handed
a gap of zero that never happened.

API_00009_TimestampUnwrapTest covers the ten cases above, including the exact
four-record sequence recovered from the customer's file: 455 ticks, 14 ms, where
it used to be 512 seconds.

Verified: 61 tests pass across ShimmerDriver. Note that running them needs both
DEV-1013 and DEV-1020 - master's Gradle 6.1 wrapper cannot resolve Guava's
dual-capability metadata, so `./gradlew test` fails on master today regardless
of this change. Both were applied locally to run the suite and reverted; the
test class itself also runs standalone against junit 4.12 with no other
dependency, since TimestampUnwrap has none.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resetCalibratedTimeStamp() puts back the two fields the unwrap works from -
the last unwrapped tick value and the wrap count - but left
mLastTimestampRejected set. The flag describes the last sample unwrapped
against that state, so it has to go back with them.

Not reachable as a fault today: every reader of the flag unwraps a sample
immediately before reading it. It is a stale-state trap for the first caller
that does not, which is reason enough to close it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@marknolan
marknolan requested a review from JongChern September 16, 2026 15:32
@marknolan marknolan self-assigned this Sep 16, 2026
@marknolan
marknolan requested a review from MAzalya September 17, 2026 07:32
marknolan and others added 2 commits September 17, 2026 11:55
… zeros

The exact-zero rejection this branch already carried is right for the firmware
fault it was written for and wrong for everything else that makes the counter
read backwards. A corrupted non-zero value, a duplicated packet and a reordered
one all still added 2^24 ticks - testNonZeroBackwardStepIsStillAWrap asserted
exactly that.

PR #301 found the gap and fixed the missing half: it detects reordered and
duplicated packets by comparing the backward step against a window derived from
the sampling rate. That idea is adopted here. Its polarity is not: #301 makes
"corrupt" the default and calls something a wrap only when it clears
maxTicks - 10 periods, so a roll-over preceded by more than ten lost samples is
misread as corruption. Forward motion has to be the default.

Each sample is now classified by its modular forward distance from the last:
duplicate, reordered, invalid zero, or forward - and forward, which is a wrap
when the raw value fell, is what everything else falls through to.

Three things in that are load-bearing, each verified by mutation:

  - The comparison is modular, not on unwrapped values. Asking whether the new
    candidate is below the last one misses a packet arriving late from BEFORE a
    wrap boundary: its candidate sits nearly a modulo ahead, so it is accepted,
    and the next real sample is read as a second wrap. 16777206, 5, 16777206,
    70 costs 512 s twice under the old shape.

  - The window is eight sample periods, not a fraction of the modulo. A reorder
    swaps adjacent packets; a dropout spanning the wrap point is most of a
    modulo. At modulo/8 on the 2-byte counter every dropout between 1.75 s and
    2.0 s reads as a reorder and the wrap is silently lost - and that is an
    ordinary Bluetooth gap.

  - An unknown rate gives a window of zero, never infinity. 32768/0 is
    POSITIVE_INFINITY in Java, and an infinite window makes every backward step
    a reorder and loses every wrap - worse than the naive rule being replaced.
    #301 derives the window without that guard, so a device whose rate has not
    been read yet silently reverts to the original defect.

Callers derive the window per sample through getReorderWindowTicks(), so a rate
written mid-session is picked up by the next sample and there is no cached
window to reset. It comes from getSamplingRateShimmer(), which is populated on
both paths before the first sample is parsed - seeded at construction, set from
the SD header before the first record, and set during the Bluetooth connect.

Shimmer2 and Shimmer2R get a window of zero. Their tick domain is unsettled -
this class divides their 16-bit counter by 32768 while the C# API divides by
1024 - so a rate-derived window would be wrong in one of the two. They keep the
behaviour they have always had.

The first sample of a stream is passed through rather than measured against the
reset state. Under a modular rule a first raw value near the top of the range
would otherwise read as a packet reordered across a boundary and place a whole
recording one modulo early.

Rule and vectors: log-and-stream-common, docs/SHIMMER3_STREAMING_DATA_FORMAT.md
section 2.1 and Test/conformance/timestamp_unwrap.json.

Co-Authored-By: Mas Azalya <43565312+MAzalya@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four host APIs unwrap this counter and all four had the same defect, because
nothing checked them against each other. log-and-stream-common now specifies the
rule once and ships machine-readable vectors for it; this runs them.

src/test/resources/timestamp_unwrap.json is a byte-identical copy of
Test/conformance/timestamp_unwrap.json at revision 1, commit 267f693. The C#
API, pyshimmer and the web SDK run the same 26 cases. If this test and its
counterparts disagree, one of the four is wrong - which is the whole point, and
is what nobody could see last time.

API_00010 asserts the file's revision and its full list of vector ids as well as
the vectors themselves, so a case dropped upstream fails here rather than
quietly reducing what is covered. It also covers the window derivations, and
natively the values JSON cannot spell: infinity, NaN and a negative rate all
have to give a window of zero.

API_00009 keeps covering the same rule in a form meant to be read, and gains
cases for the three properties the vectors exist to pin: a reordered packet
placed where it was taken, a packet arriving late from before a wrap boundary,
and a 1.8 s dropout across the 2-byte counter that must not read as a reorder.

Gson is a test-only dependency, and a plain POM rather than Gradle module
metadata, so it resolves under the Gradle version this project is pinned to.

22 tests, 0 failures. Four mutations were checked rather than assumed: reading
unwrapped values instead of modular distances, sizing the window as modulo/8,
letting an unknown rate become an infinite window, and dropping the first-sample
sentinel. Each is caught by two or three of these tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This driver keeps an unwrapped value and a cycle count rather than the previous
raw value, so it has to encode "no sample yet" somehow, and (0, 0) was the
encoding. That state is also reachable: a reordered packet landing exactly on
the counter's origin leaves mLastReceivedTimeStampTicksUnwrapped and
mCurrentTimeStampCycle both at zero in the middle of a stream. The next packet
is then read as a first sample and passed through, so one arriving from just
before the origin is placed a whole modulo late rather than sixteen ticks
behind.

Found by running the two formulations of the rule against each other rather
than by reading them. [520, 0, 16777200] gives -16 where the previous raw value
is kept - the web SDK and pyshimmer - and 16777200 here. A 512 second
disagreement between host APIs that are meant to be identical.

So: a six-argument unwrap that is told outright. The five-argument overload
stays, still inferring it from (0, 0), because getLastReceivedTimeStampTicks-
Unwrapped is public and callers chaining legacy SD files across a trial seed it;
a test states what that overload does with this sequence so the compatibility
boundary is deliberate rather than discovered.

mHasPreviousTimeStamp is set by setLastReceivedTimeStampTicksUnwrapped - being
told the previous sample's value is precisely what a caller seeding state across
files is doing - and cleared by resetCalibratedTimeStamp afterwards, which is
the one place that means a stream start.

The sequence is now the shared conformance vector
reorder-onto-origin-then-earlier-packet-24bit, so the other four host APIs are
held to the same answer. It is the first vector whose final cycle is negative,
which is a real state here: the raw value is derived back out of it.

24 tests pass across API_00009 and API_00010. Reverting the wiring in the
vectors test reproduces the defect as a failure -
"unwrapped expected:<-16.0> but was:<1.67772E7>" - rather than as an argument.

Co-Authored-By: Mas Azalya <43565312+MAzalya@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@marknolan

Copy link
Copy Markdown
Member Author

Follow-up commit: an adversarial review of this branch found that "no sample
yet" was not distinguishable from a real sample
.

This API keeps an unwrapped value and a cycle count rather than the previous raw
value, so the reset state had to be encoded somehow, and (0, 0) was the
encoding. That state is also reachable: a reordered packet landing exactly on the
counter's origin leaves both at zero in the middle of a stream. The next packet
is then read as a first sample and passed through, so one arriving from just
before the origin is placed a whole modulo late rather than sixteen ticks behind.

Found by running the two formulations of the rule against each other rather than
by reading them:

[520, 0, 16777200]
keeps lastRaw — web SDK, pyshimmer [520, 0, -16]
(0, 0) as the reset state — here, before [520, 0, 16777200]

512 seconds apart on the same input, between APIs that are meant to be
identical, and no vector covered it.

The unwrap is now told outright. The existing overload stays, still inferring it
from (0, 0), for callers written before the distinction existed — with a test
stating what it does with this sequence, so the compatibility boundary is
deliberate rather than discovered.

The sequence is now the shared conformance vector
reorder-onto-origin-then-earlier-packet-24bit
(ShimmerResearch/log-and-stream-common#136), so all five host APIs are held to
the same answer. It is the first vector whose final cycle is negative — a real
state here, since the raw value is derived back out of it.

Probability in the field is negligible: it needs a reorder to land exactly on the
origin, within one window of it. The reason to fix it rather than note it is that
the class of problem is the one this whole workstream exists to stop — two
families of implementation that cannot represent the same state, with nothing
testing the difference.

@MAzalya MAzalya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok tested, looks good

@MAzalya
MAzalya merged commit d791a91 into master Sep 18, 2026
1 check passed
@MAzalya
MAzalya deleted the DEV-1023_reject_isolated_zero_timestamp branch September 18, 2026 05:16
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