Conversation
Shimmer firmware stamps a packet when its sample tick starts it and does not
write out a packet it never stamped, so a timestamp field of exactly zero marks
an invalid record rather than the counter reaching its origin. LogAndStream
v1.00.x-v1.01.003 could produce one under SD write back-pressure.
unwrap() read every backward step as an overflow, so one such record added a
whole modulo - 512 seconds on the 3-byte counter - to every later sample in the
recording. A 9 minute 30 second trial holding four of them read as 43 minutes
38. An out-of-order or duplicated record cost the same.
Each sample is now classified by its modular forward distance from the one
before it: a repeat holds the timeline, a sample no further back than the
reorder window is placed where it was taken, a 3-byte counter reading exactly
zero from mid-range is marked invalid, and everything else is forward motion -
an overflow when the raw value fell. Forward is the default, which is what keeps
an overflow preceded by a long dropout classified as an overflow.
Three things in that are load-bearing, each verified by mutation:
- The comparison is on modular distance. Comparing unwrapped values misses a
packet arriving late from BEFORE an overflow boundary: it looks like forward
motion of nearly a modulo, so it is accepted, and the next real sample is
read as a second overflow.
- reorder_window_ticks() is sized in sample periods, not as a fraction of the
modulo. A reorder swaps adjacent packets; a dropout spanning the overflow
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 overflow is silently
lost, and a 1.75 s gap is ordinary.
- An unknown period gives a window of zero, never infinity. An infinite window
reads every backward step as a reorder and loses every overflow, which is
worse than no reorder detection at all.
find_invalid_timestamps() takes the window too, because reorder is tested first
and a zero can be either; and it judges a zero against the last NON-ZERO sample,
so a run of consecutive invalid records is measured from the last record that
carried a timestamp rather than from the zero before it.
ShimmerReader passes the header divider down as period_ticks. It is the tick
count between samples directly, and the reader is the only thing that knows it.
The new parameters are keyword-optional throughout, so an existing caller and
any subclass of HardwareRevision keep working unchanged.
Two behaviour notes worth stating:
- unwrap() no longer modifies its input in place. It also casts to int64
first, so a 32-bit input array can no longer overflow after enough overflows
on a 64-bit platform.
- A negative difference has one modulo added rather than being reduced
outright, which leaves a value at or above the modulo - not something a real
counter emits, but something a caller can pass - as plain forward motion.
The cases are the conformance vectors the Shimmer host APIs share, in
test/resources/timestamp_unwrap.json, copied byte-identically from the firmware
repository where the rule is specified. The Java, C# and TypeScript APIs run the
same file. Four implementations of one wire format drifted apart once already -
the same defect sat in all four - so if these disagree, one of them is wrong.
83 tests pass, including the reader suite against the real binary fixtures.
Co-Authored-By: Mas Azalya <43565312+MAzalya@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marknolan
force-pushed
the
DEV-1023_reject_isolated_zero_timestamp
branch
from
September 17, 2026 11:46
6f82237 to
35f3956
Compare
Collaborator
|
Wow, thank you for the PR. It will probably take a bit of time to review this, but I will try to get on it asap. |
Author
|
@lumagi we're in the middle of reviewing the same update to our other APIs (e.g., below) so if it might be best to hold off on reviewing until they are in and I will update this PR should anything change.: |
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. A reorder that lands exactly on the counter's origin reaches that state mid array, so the next sample is read as a first sample and a packet from just before the origin is placed a whole modulo late. unwrap() keeps the previous raw value and was already right here, but the sequence is now part of the shared set - [520, 0, 2**24 - 16] - so this suite runs it. It is also the first vector whose final cycle is negative. The conformance test looped over whatever the file happened to contain, so a vector quietly disappearing upstream would have gone unnoticed. The ids are now written out, as they are in the other host suites. 83 tests pass; black clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Shimmer firmware stamps a packet when its sample tick starts it and does not write out a
packet it never stamped, so a timestamp field of exactly zero marks an invalid record
rather than the counter reaching its origin. LogAndStream v1.00.x–v1.01.003 could produce
one under SD write back-pressure.
unwrap()reads every backward step as an overflow, so one such record adds a wholemodulo — 512 seconds on the 3-byte counter — to every later sample:
Those four samples span 455 ticks, about 14 ms. A 9 minute 30 second trial containing four
such records was reported as 43 minutes 38.
An out-of-order or duplicated record costs exactly the same, which is the wider half of
the problem — nothing about the current rule distinguishes a reordered packet from an
overflow.
The change
Each sample is classified by its modular forward distance from the one before it:
Forward is the default, which is what keeps an overflow preceded by a long dropout
classified as an overflow.
reorder_window_ticks(period_ticks, modulo)sizes the window. Everything new iskeyword-optional, so existing callers and any
HardwareRevisionsubclass keep workingunchanged;
ShimmerReaderpasses the header divider down, since it is the tick countbetween samples directly and the reader is the only thing that knows it.
Three things that are easy to get wrong
Compare modular distances, not unwrapped values. A packet arriving late from before
an overflow boundary looks like forward motion of nearly a whole modulo, so it is accepted
— and the next real sample is then read as a second overflow. Two modulos from one
out-of-order packet.
Size the window in sample periods, not as a fraction of the modulo. A reorder swaps
adjacent packets; a dropout spanning the overflow point is most of a modulo. At
modulo / 8on the 2-byte counter, every dropout between 1.75 s and 2.0 s reads as areorder and the overflow is silently lost — and a 1.75 s gap is ordinary. Eight periods
shrinks that band to about 16 ms.
An unknown period means no window, not an infinite one. An infinite window reads every
backward step as a reorder and loses every overflow, which is worse than no reorder
detection at all.
Two behaviour notes
unwrap()no longer modifies its input in place. It also casts toint64first, soa 32-bit input array can no longer overflow after enough wraps.
value at or above the modulo — not something a real counter emits, but something a
caller can pass, and something
test_unwrap_device_timestampsdoes — stays plainforward motion.
find_invalid_timestamps()judges a zero against the last non-zero sample, so a run ofconsecutive invalid records is measured from the last record that carried a timestamp
rather than from the zero before it.
Tests
83 pass, including the reader suite against the real binary fixtures — so the change is
exercised end to end on actual recordings, not only on synthetic arrays.
test/resources/timestamp_unwrap.jsonholds 26 conformance vectors and 11 windowderivations, covering every case above. They are copied byte-identically from the Shimmer
firmware repository, where the rule is specified and a reference implementation regenerates
and re-checks them in CI. The Java, C# and TypeScript Shimmer APIs run the same file —
four implementations of one wire format drifted apart once already, the same defect in all
four, so if they disagree now one of them is wrong.
Five mutations were checked rather than assumed, each caught: ignoring the reorder window,
sizing it as
modulo / 8, letting an unknown period become infinite, judging a zeroagainst the element before it rather than the last non-zero, and dropping the period on the
reader call. The last of those found a genuine gap — nothing had covered the reader
actually threading the period through — and a test was added for it.
blackclean.Happy to adjust naming, placement, or the vector file location to suit the project.