Conversation
TimeSensor read any backward step in the counter as a roll-over:
if (LastReceivedTimeStamp > (timeStamp + (TimeStampPacketRawMaxValue * CurrentTimeStampCycle)))
{
CurrentTimeStampCycle = CurrentTimeStampCycle + 1;
}
That is right for a roll-over and wrong for the three other things that step
the counter backwards, each of which then adds a whole modulo - 512 seconds -
to every later sample for the rest of the session:
- a reordered packet, which is behind its predecessor by a sample period;
- a duplicated packet, which is behind it by nothing at all;
- a record the firmware never stamped, whose counter field is 0x000000.
Firmware stamps a packet when the sample tick starts it and does not
publish a packet it never stamped, so an exact 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; a
9 minute 30 second recording containing four of them was reported as
43 minutes 38.
It is also stated the wrong way round - on a comparison of unwrapped values
rather than on the modular forward distance. A packet arriving late from just
before a boundary has an unwrapped value ABOVE its predecessor, so the
comparison accepts it and then reads the next real sample as a second
roll-over: [2^24 - 10, 5, 2^24 - 10, 70] lands at 33554502, two modulos out,
from one out-of-order packet.
TimestampUnwrap.swift is the rule as specified in the firmware repository,
mirroring TimestampUnwrap.cs and TimestampUnwrap.java method for method.
Forward motion is the default, which is what keeps a roll-over preceded by a
long dropout classified as a roll-over however much was lost.
The reorder window is eight sample periods, derived from the rate the inquiry
reports - not a fraction of the counter's range, which is the same quantity
the old rule confused. Shimmer3Protocol sets it in interpretDataPacketFormat,
where both inquiry paths have just set CurrentSamplingRate. A rate arriving as
a division by zero is +Infinity in Swift, and reorderWindowTicks answers 0 for
it: an unknown rate must disable the branch, never widen it, because an
infinite window reads every backward step as a reorder and loses every
roll-over.
ObjectCluster gains timestampValid. A rejected record keeps its sensor values
and loses its time; both Time Stamp signals are still present so a consumer
reading by name is unaffected. The unwrap now runs whether or not calibration
is enabled - an unstamped record has no time either way, and a timeline that
only advanced while someone was watching would jump when calibration was
switched on mid-stream.
TimestampUnwrapVectorsTest.swift carries the 26 conformance vectors and 11
window derivations every Shimmer host API is checked against, generated from
the same reference implementation the Java, C#, Python and TypeScript APIs load
as a data file:
python Test/host/crosscheck_timestamp_unwrap.py --emit swift
Not built or run locally - there is no Swift toolchain on the machine this was
written on, so CI is the first execution. The rule itself was compared
statement by statement against the C# implementation, which its own suite
passes.
Co-Authored-By: Mas Azalya <43565312+MAzalya@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TimeSensor 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 LastReceivedTimeStamp and CurrentTimeStampCycle 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. hasPreviousSample is asked for outright, and asked for on both overloads rather than defaulted. The C# and Java copies keep a form that infers it from (0, 0), because they have callers older than the distinction; this file does not, so defaulting it would only be a quiet way to get the first sample of a stream wrong. The docblock says where the three differ and why. 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. Vectors regenerated with --emit swift, not hand-edited. Still not built locally - CI is the first execution, as before. Co-Authored-By: Mas Azalya <43565312+MAzalya@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 Swift API is the fifth host implementation of the same wire format, and it
carries the same timestamp-unwrap defect as the other four — in its original,
unmitigated form. This brings it onto the shared rule and the shared conformance
vectors.
What is wrong today
TimeSensor.calibrateTimeStampreads any backward step in the counter as aroll-over:
Right for a roll-over, wrong for the three other things that step the counter
backwards — and each of them then adds a whole modulo, 512 seconds, to every
later sample for the rest of the session:
0x000000The third is the reported one. Firmware stamps a packet when the sample tick
starts it and does not publish a packet it never stamped, so an exact zero in
the counter field 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; a 9 minute 30 second recording containing four of them was
reported as 43 minutes 38.
It is also stated the wrong way round — on a comparison of unwrapped values
rather than on the modular forward distance. A packet arriving late from just
before a boundary has an unwrapped value above its predecessor, so the
comparison accepts it and then reads the next real sample as a second roll-over.
[2²⁴ − 10, 5, 2²⁴ − 10, 70]lands at 33554502 — two modulos out, from oneout-of-order packet.
The rule
TimestampUnwrap.swift, mirroringTimestampUnwrap.csandTimestampUnwrap.javamethod for method:Forward motion is the default. That is what keeps a roll-over preceded by a
long dropout classified as a roll-over: however much was lost, the counter still
wrapped.
The window is eight sample periods, not a fraction of the counter's range —
the same two quantities the old rule confused. A reorder swaps packets adjacent
in time; a dropout spanning the wrap point is most of a modulo.
An unknown rate disables the branch.
32768 / 0is+Infinityin Swift, notan error, and an infinite window reads every backward step as a reorder and
loses every roll-over — the original bug restored, with no symptom until a
recording comes out short.
reorderWindowTicksanswers0for infinity, NaN,zero and negatives.
Wiring
Shimmer3Protocol.interpretDataPacketFormatsets the window. Both inquiry paths(
interpretInquiryResponseShimmer3and…Shimmer3R) setCurrentSamplingRateimmediately before calling it, so the rate is always current there.
ObjectClustergainstimestampValid. A rejected record keeps its sensor valuesand loses its time; both Time Stamp signals are still added, so a consumer
reading by signal name is unaffected, and one that needs a true time axis has a
flag to filter on.
The unwrap now runs whether or not
calibrationEnabledis set. An unstampedrecord has no time either way, so the flag has to mean something in both modes —
and a timeline that only advanced while someone was watching would jump by
however much was missed the moment calibration was switched on mid-stream.
Tests
TimestampUnwrapVectorsTest.swiftcarries 27 conformance vectors and 11 windowderivations, the same set the Java, C#, Python and TypeScript APIs run. They
are specified in the firmware repository beside the prose they encode, where a
reference implementation regenerates and re-checks them in CI. Those four APIs
load the JSON directly; a classic Xcode project would need four hand-edited
project.pbxprojentries to carry a resource nothing compiles, so this targetgets generated source instead:
Regenerating produces a byte-identical file, so a stale copy is detectable.
Plus seven tests on
TimeSensoritself: a swapped pair placed where it was taken,the same swap costing a modulo with the window off (what wiring the rate buys,
stated as the difference it makes), an unstamped record rejected without
disturbing the next sample, a genuine roll-over onto zero still accepted, a
roll-over after heavy loss still a roll-over, a reorder onto the origin not
looking like a fresh stream, and
timestampValidon theObjectCluster.Two things to be aware of
Not built or run locally. There is no Swift toolchain on the machine this was
written on, so this PR's CI run was the first execution of any of it. Before
pushing, the rule was compared statement by statement against the C#
implementation, whose own suite passes, and the generated vectors come from the
same script that produces the C# array.
CI has since run it:
** TEST SUCCEEDED **, with all 11 new cases passing(
testAllSharedVectorsAgreecovers the vectors). So it is verified now — butby the runner, not by anything that happened before the push, which is worth
knowing if you are weighing how much review the Swift-specific parts deserve.
The one-line wiring in
interpretDataPacketFormatis not covered by a test.Shimmer3Protocol.inittakes a concreteBleByteRadio, which needs a liveCBPeripheral, so no unit test can construct the protocol — and no existing testdoes. The window derivation and everything downstream of it are tested; that the
protocol passes the rate through is not.
Raised as DEV-1034: widening the three protocol initialisers to take
ByteCommunicationmakes a loopback stub possible, which is how the TypeScriptand C# APIs test their protocol layers. It needs two members added to that
protocol (
deviceNameanddelegate, both of whichBleByteRadioalreadydeclares) — not the one-word change it first looks like.
Since the first push — one review finding, fixed here
Keeping
(lastUnwrapped, cycle)rather than the previous raw value means thereset state has to be encoded somehow, and
(0, 0)was the encoding. It is alsoreachable: a reordered packet landing exactly on the counter's origin leaves both
at zero mid stream, so the next packet is read as a first sample and one from
just before the origin is placed a whole modulo late.
[520, 0, 2²⁴ − 16]lastRaw)[520, 0, -16][520, 0, 16777200]512 seconds apart, and nothing in the vector set could see it.
unwrapis nowtold outright, on both overloads rather than by default: the C# and Java copies
keep a form that infers it from
(0, 0)because they have callers older than thedistinction, and this file does not, so a default would only be a quiet way to
get the first sample of a stream wrong. The sequence is now
reorder-onto-origin-then-earlier-packet-24bitin the shared file, and it is thefirst vector whose final cycle is negative — a real state here, since the raw
value is derived back out of it.
Related
Reorder and duplicate detection came out of @MAzalya's parallel fix for this
defect on the Java driver. The polarity and the window sizing here differ, but
the observation that a backward step is not always a roll-over is hers.
🤖 Generated with Claude Code