martino/MARTe-Integrated-Components
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(udpscope): keep the packet counter in lockstep with the tick reference

Browse Source 
The counter is the denominator of the very period lastAccHrt is the
numerator of, so any packet that cannot move the tick reference must not
move the counter either. Two paths were violating that: a reordered
datagram rolled the counter back while the reference correctly held
(next burst drawn 0.048x too narrow at distance 20, 83.3% worst spacing
error under 2% sustained reordering), and a stray hrt == 0 packet
advanced the counter from the warm-up branch without a tick to match
(+22.5 ms of future-dating per stray packet).

Rules 1 and 2 now record a counter too. The duplicate-datagram guard is
keyed on one, so an array rule that recorded none was exempt and plotted
every doubly-delivered update twice.

Also: rule 2 divides by the count the anchor actually spans and falls
back to the last period it derived; the counter-gap test is wrap-safe so
2^32 rollover reads as no information rather than 2e9 lost packets.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Martino Ferrari
2026-08-28 06:41:47 +02:00
co-authored by Claude Opus 4.6
parent f97fd825c4
commit 1c61e814c0
4 changed files with 644 additions and 144 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Client/udpscope/FrameDecoder.cpp
+110 -34
View File
@@ -139,8 +139,12 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
* The C client only de-duplicates fragments, so an unfragmented update
* reaches us intact both times; emitting it again would double the values
* and advance the timeline by a burst that never existed. Counter zero is
* excluded because a producer that never sets one leaves it there. */
if (st.lastEmittedValid && f.counter != 0u && f.counter == st.lastCounter) {
* excluded because a producer that never sets one leaves it there.
*
* Keyed on counterValid, not lastEmittedValid: rules 1 and 2 are exposed to
* the same double delivery and would otherwise plot every array twice, since
* neither of them ever joins rule 3's emitted chain. */
if (st.counterValid && f.counter != 0u && f.counter == st.lastCounter) {
return false;
}
@@ -166,6 +170,10 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
for (uint32_t e = 0; e < nElems; e++) {
tsOut[e] = base + tv[e] * tScale;
}
/* Only so the duplicate-datagram guard above has something to compare
* against; nothing in this rule reads it back. */
st.lastCounter = f.counter;
st.counterValid = true;
return true;
}
@@ -189,18 +197,48 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
* on arrival — immune to the bursty delivery that corrupts everything
* arrival-derived. Divide by the counter gap for the same reason rule 3
* does: a lost datagram widens the anchor difference without widening
* the array. Until a second packet arrives there is nothing to measure
* and the elements do stack; that is one packet, not the whole run. */
if (!(dt > 0.0) && nElems > 1u && st.prevAnchorValid &&
prodSec > st.prevAnchorProdSec) {
const uint32_t gap = (f.counter != 0u && f.counter > st.lastCounter)
? (f.counter - st.lastCounter) : 1u;
dt = (prodSec - st.prevAnchorProdSec) /
(static_cast<double>(nElems) * static_cast<double>(gap));
* the array.
*
* Which array, though, is a question of which end is anchored. For
* LAST_SAMPLE the anchors bracket THIS packet's elements, so the divisor
* is nElems; for FIRST_SAMPLE they bracket the PREVIOUS packet's, so it
* is that packet's count. Accumulate mode flushes on a timer, so the
* count really does vary between packets — using the wrong one against a
* 10,10,2,10,20 pattern gave 5x, 0.2x and 0.5x the true period and one
* backward step of 3 ms.
*
* Two packets cannot always be measured. The first has no predecessor,
* and a reordered one has an anchor behind its predecessor's; rather
* than stack the whole array on one instant — the very defect this
* paragraph exists to remove — reuse the last period actually measured,
* exactly as the hrt branch reuses lastHrtDt. Only the genuine first
* packet of a run stacks, and only until the second arrives. */
if (!(dt > 0.0) && nElems > 1u) {
const uint32_t divisor = (d.timeMode == kTimeLastSample)
? nElems : st.prevAnchorCount;
const uint32_t rawGap = f.counter - st.lastCounter;
const bool fwdGap = (f.counter != 0u) && st.counterValid &&
(rawGap != 0u) && (rawGap < 0x80000000u);
if (st.prevAnchorValid && prodSec > st.prevAnchorProdSec &&
divisor > 0u) {
dt = (prodSec - st.prevAnchorProdSec) /
(static_cast<double>(divisor) *
static_cast<double>(fwdGap ? rawGap : 1u));
st.prevAnchorDt = dt;
} else if (st.prevAnchorDt > 0.0) {
dt = st.prevAnchorDt;
}
}
/* All four move together or not at all: a reordered packet must not
* leave a newer anchor and an older counter behind for the next one to
* divide one by the other. */
if (!st.prevAnchorValid || prodSec > st.prevAnchorProdSec) {
st.prevAnchorProdSec = prodSec;
st.prevAnchorCount = nElems;
st.prevAnchorValid = true;
st.lastCounter = f.counter;
st.counterValid = true;
}
st.prevAnchorProdSec = prodSec;
st.prevAnchorValid = true;
st.lastCounter = f.counter;
tsOut.resize(nElems);
for (uint32_t e = 0; e < nElems; e++) {
@@ -386,6 +424,7 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
st.lastEmittedEnd = tsOut[nElems - 1u];
st.lastEmittedWall = wallNow;
st.lastCounter = f.counter;
st.counterValid = true;
st.prevAccCount = nElems;
st.lastEmittedValid = true;
return true;
@@ -417,11 +456,23 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
st.lastAccHrt = f.hrt;
st.lastAccValid = true;
}
/* Same lockstep rule as the hrt branch below. A packet with hrt == 0
* lands here mid-stream and cannot move the tick reference, so it
* must not move the counter either: advancing the counter alone
* makes the next packet's elapsed span two intervals while its gap
* reports one, drawing that burst twice too wide and ending it
* 22.5 ms in the future at 10 samples per 25 ms packet (6x and
* +112 ms after five such packets). Before any tick reference exists
* nothing is keyed to the counter, so it is free to advance and arm
* the duplicate guard for a producer that never sets hrt at all. */
if (f.hrt != 0u || !st.lastAccValid) {
st.lastCounter = f.counter;
st.counterValid = true;
}
st.prevAccCount = nElems;
if (!ok) { return false; }
st.lastEmittedEnd = tsOut[nElems - 1u];
st.lastEmittedWall = wallNow;
st.lastCounter = f.counter;
st.lastEmittedValid = true;
return true;
}
@@ -495,25 +546,43 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
* 11x for ten — which draws the recovery burst that many times too wide
* and, because the burst is anchored on its LAST element, ends it in the
* FUTURE (measured: +22.5 ms for one loss, +225 ms for ten, at 10
* samples per 25 ms packet). At 1% loss that mis-spaced 2.7% of all
* samples per 25 ms packet). At 1% loss that mis-spaced 4.0% of all
* samples. The declared branch already reads the counter for exactly
* this purpose (`lost`, above); the hrt branch must too.
*
* Only a FORWARD gap counts. A backward or repeated counter is the
* reorder case handled above, where elapsed is zero anyway. */
const uint32_t accGap = (f.counter != 0u && st.lastEmittedValid &&
f.counter > st.lastCounter)
? (f.counter - st.lastCounter) : 1u;
* The gap and `elapsed` must be measured from the SAME packet, or the
* division mixes references. That is why lastCounter is written under
* takeHrt below, in lockstep with lastAccHrt: a reordered datagram that
* rolled lastCounter back while leaving lastAccHrt alone would give the
* next in-order packet a gap of d+1 against an elapsed spanning one
* interval, dividing its period by d+1 — measured 0.5x the true spacing
* for a swap of neighbours, 0.048x for a distance of twenty.
*
* Unsigned subtraction wraps, which is what makes this right across the
* counter's own 2^32 rollover. A gap in the top half of the range is not
* a forward gap at all but a backward one seen through the wrap, so it
* is treated as no information rather than as 2 billion lost packets. */
const uint32_t rawGap = f.counter - st.lastCounter;
const bool fwdGap = (f.counter != 0u) && st.counterValid &&
(rawGap != 0u) && (rawGap < 0x80000000u);
const double cycles = static_cast<double>(st.prevAccCount) *
static_cast<double>(accGap);
static_cast<double>(fwdGap ? rawGap : 1u);
/* Falling back to kDefaultDt is a last resort, not a default: see
* SigState::lastHrtDt. The fallback is reached on the first hrt packet
* of a producer restart (elapsed is zero because hrt went backwards) and
* on a reordered datagram, and in both cases the wrong burst width is
* latched into ClockOffset permanently — measured 13.5 ms of standing
* SigState::lastHrtDt. What makes the fallback matter is the producer
* restart, because that is the one path where elapsed is zero AND
* offset.reset() has just forced a re-latch, so the burst width used
* here is the one calibrated against — measured 13.5 ms of standing
* displacement at 10 samples per 25 ms packet, 89 ms at 100 per 10 ms,
* both below kRecalibThresholdS and so never corrected. */
* both below kRecalibThresholdS and so never corrected. A reorder also
* reaches the fallback but does not re-latch, and its measured standing
* error is 0.013 ms, i.e. nothing.
*
* This is better than kDefaultDt at every cadence except one: a producer
* that restarts having CHANGED its cycle time is remembered wrongly, and
* a tenfold change measured -20 ms against kDefaultDt's -6.8 ms. Both
* are bounded and neither is correct; the remembered period wins the
* case that actually happens. */
double hrtDt;
if (elapsed > 0.0 && cycles > 0.0) {
hrtDt = elapsed / cycles;
@@ -574,16 +643,23 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
/* Keep packetBurst's reference current even though this branch does not
* use it. A single packet with hrt == 0 re-enters the warm-up branch
* above, and packetBurst would otherwise span from whenever this signal
* last took that branch — the whole session. Measured: after 153 hrt
* packets, one zero-hrt packet emitted a burst starting 3.8 s in the
* past, growing without bound with session length. */
* last took that branch — the whole session. Measured: 153 packets into
* a 25 ms stream, one zero-hrt packet emitted a burst starting 2.74 s
* behind the trace, and that figure grows with session length. */
st.lastPacketWall = wallNow;
st.lastPacketValid = true;
/* Same duplicate-datagram exposure as the declared branch: a host joined
* on two interfaces receives every unfragmented update twice, and the
* guard at the top of timestamps() can only fire if this branch leaves a
* counter behind for it to compare against. */
st.lastCounter = f.counter;
/* In lockstep with lastAccHrt, and for the same reason: these two are
* the numerator and the denominator of the next packet's period. Only a
* packet that defines the new front of producer time may move either.
* Advancing the counter alone on a reordered datagram halves the next
* packet's spacing; see the gap comment above. Leaving a counter behind
* at all is what lets the duplicate-datagram guard at the top of
* timestamps() fire — a host joined on two interfaces receives every
* unfragmented update twice, and the original always sets takeHrt. */
if (takeHrt) {
st.lastCounter = f.counter;
st.counterValid = true;
}
st.lastEmittedValid = true;
return true;
}