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

fix(udpscope): make rule 3 converge in both branches and survive reordering

Browse Source 
Eight review findings on FrameDecoder's accumulated-scalar rule.

The squeeze that pulls a leading timeline back was expressed as a fraction of
the NOMINAL burst width, which cannot converge: inside one timestamps() call
the wall clock is frozen, so any positive step raises the lead measured at that
instant, and the lead only falls because the wall advances between packets. At
the kMinBleedFactor floor the timeline still gained 0.05 * nominal per packet,
so a declared SamplingRate of 30 against a producer really flushing 10 samples
at 1 kHz ran away without bound (667 s of lead after 1000 s of stream). Cap the
burst's total advance at half the wall time really elapsed since this signal's
previous burst instead, and the lead strictly falls for any declared rate.
SigState gained lastEmittedWall for that reference; lastPacketWall could not be
reused because it belongs to packetBurst.

The hrt branch contributed zero elapsed for a late datagram but still wrote the
hrt reference back to it, so the next packet's delta spanned two intervals and
fabricated a whole extra packet of producer time — permanently, since the
monotonic clamp discards the correction ClockOffset would have made. Reordering
is reachable in production: udps_client.c only counts counter gaps. Simply
never regressing the reference is not the fix either, because a producer restart
would then freeze the signal forever, so the two are now separated by the size
of the backward jump.

The hrt branch's clamp was also one-directional, reintroducing on that branch
exactly the defect the declared branch's squeeze exists to prevent: a backward
wall step (NTP, suspend/resume) left a permanent lead. It now shares the same
wall-elapsed cap.

Also: anchor an hrt-branch burst's LAST element on arrival, matching the
declared branch, so two accumulated scalars in one scope do not sit a burst
apart on the shared X axis; treat a non-finite samplingRate off the wire as
undeclared, since +inf produced a 0.0/0.0 factor the floor could not catch and
turned every stamp NaN; write lastCounter on the hrt branch so duplicate
datagrams are dropped there too; and correct two comments that argued for the
current code with claims that are false (a counter-gap clamp reaches the
opposite outcome, not the same one earlier, and the squeeze's steady state is a
sawtooth, not a fixed offset).

Seven new tests, each proven non-vacuous by sabotage; 54 pass. Plan document
Task 4 re-synced and its stale test count and "agree on the same stream" claim
corrected.
This commit is contained in:
Martino Ferrari
2026-08-27 22:43:27 +02:00
parent a2efc142c3
commit 440b805afd
4 changed files with 1317 additions and 166 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Client/udpscope/FrameDecoder.cpp
+199 -28
View File
@@ -30,6 +30,59 @@ static constexpr double kBurstResyncThresholdS = 0.5;
*/
static constexpr double kMinBleedFactor = 0.05;
/**
* Largest share of the wall time elapsed since a signal's previous burst that
* that signal's next burst may advance its own timeline by, while a lead is
* being pulled back.
*
* This, and not kMinBleedFactor, is what makes a lead converge. A fraction of
* the NOMINAL burst width cannot: the floor still advances the timeline by
* kMinBleedFactor * nominal per packet while the wall advances one packet
* interval, so it diverges outright whenever nominal exceeds
* (1 / kMinBleedFactor) packet intervals — a declared SamplingRate of 30 against
* a producer really flushing 10 samples at 1 kHz put the trace 667 s ahead after
* 1000 s of stream. Measuring the allowance against elapsed WALL time instead
* bounds the advance below the wall's own advance for any declared rate, so the
* lead strictly falls whatever the config says. Any fraction under 1 converges;
* a half both converges quickly and leaves the burst visibly compressed rather
* than frozen.
*/
static constexpr double kWallBleedFraction = 0.5;
/**
* A backward jump in producer hrt larger than this is a producer RESTART; a
* smaller one is a reordered datagram.
*
* The two need opposite handling — a reorder must leave the hrt reference
* untouched (its interval was already counted by the packet that overtook it),
* a restart must rebase onto the new epoch or the signal never advances again —
* and nothing but the size of the jump distinguishes them. A second of producer
* time is orders of magnitude more than any reordering window a UDP path can
* produce (a few packet intervals) and orders of magnitude less than a restart,
* which drops hrt from the producer's whole uptime back to near zero.
*
* Deliberately NOT kBurstResyncThresholdS: that one asks how far a WALL-clock
* prediction may sit from arrival, a different quantity in a different clock
* that happens to be tuned for delivery jitter. Sharing the number would couple
* two unrelated tunings.
*/
static constexpr double kProducerRestartS = 1.0;
/**
* The declared sampling rate, or 0 when there is none to trust.
*
* samplingRate arrives unvalidated from a signal descriptor on the wire. A
* malformed +inf reaches the reciprocal as dt == 0, which makes a burst's
* nominal width zero and the proportional squeeze compute 0.0/0.0 — and a NaN
* factor is not caught by the floor, since every comparison against NaN is
* false, so the whole burst is emitted as NaN. Rejecting it at the boundary
* costs one test and removes the entire class.
*/
static double DeclaredRate(double samplingRate) {
return (std::isfinite(samplingRate) && samplingRate > 0.0) ? samplingRate
: 0.0;
}
void FrameDecoder::setSignals(const std::vector<SignalMeta>& signals) {
signals_ = signals;
state_.assign(signals_.size(), SigState{});
@@ -120,7 +173,8 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
if ((d.timeMode == kTimeFirstSample || d.timeMode == kTimeLastSample) &&
hasTimeSig && f.counts[tIdx] >= 1u && f.values[tIdx] != nullptr) {
const double anchor = st.offset.map(f.values[tIdx][0] * tScale, wallNow);
const double dt = (d.samplingRate > 0.0) ? (1.0 / d.samplingRate) : 0.0;
const double rate = DeclaredRate(d.samplingRate);
const double dt = (rate > 0.0) ? (1.0 / rate) : 0.0;
tsOut.resize(nElems);
for (uint32_t e = 0; e < nElems; e++) {
tsOut[e] = (d.timeMode == kTimeFirstSample)
@@ -151,11 +205,12 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
* datagram and reinstated a hole that never existed. A signal that has never
* burst is a genuine scalar and is left to rule 5. */
if (d.numElements() == 1u && (nElems > 1u || st.lastEmittedValid)) {
const double dt = (d.samplingRate > 0.0)
? (1.0 / d.samplingRate)
: 0.0;
/* A rate that is not a finite positive number is no rate at all; see
* DeclaredRate(). Such a signal takes the hrt branch below. */
const double declared = DeclaredRate(d.samplingRate);
const double dt = (declared > 0.0) ? (1.0 / declared) : 0.0;
if (d.samplingRate > 0.0) {
if (declared > 0.0) {
/* Where arrival time says this burst begins: its last element was
* acquired just before the packet landed. */
const double arrivalAnchor =
@@ -181,9 +236,23 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
* A producer restart or a reordered datagram makes the wrapped
* gap enormous, and this deliberately does NOT special-case
* that: an absurd gap yields an absurd prediction, which the
* arrival backstop below then rejects on its own. Clamping the
* gap first would only decide the same question earlier, by a
* second rule that no stream can distinguish from this one. */
* arrival backstop below then rejects on its own.
*
* Clamping the gap first is not a harmless earlier version of
* the same decision — it reaches the OPPOSITE answer. A clamp
* that treats gap > kMaxCounterGap as unknowable has to fall
* back to lost == 0, so the prediction becomes
* lastEmittedEnd + dt: one sample period after the last burst,
* which is exactly the shape of a healthy chain and therefore
* lands INSIDE the arrival backstop, is accepted, and silently
* closes an outage of arbitrary length. Letting the absurd gap
* through produces an absurd prediction that the backstop
* catches, and the burst re-anchors on arrival — which is the
* right answer, and what
* AccumulatedScalarSurvivesAProducerRestart pins down. The
* arithmetic cannot overflow: gap and prevAccCount are both
* bounded by 2^32-1, so lost is at most ~1.8e19, finite, and
* always rejected. */
const uint32_t gap = f.counter - st.lastCounter;
const double lost = (gap > 1u)
? static_cast<double>(gap - 1u) *
@@ -223,24 +292,52 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
* dated later than the moment this packet landed, so
* there is no room to spread into and no burst can end
* on arrival without starting before it. Squeeze this
* one by exactly the excess instead. That lands its end
* one nominal burst ahead of arrival — the closest a
* forward-only timeline can legally get — and the excess
* settles at (nominal width - true burst period), a
* couple of hundred microseconds for the ppm-scale
* crystal mismatch that causes this.
* one instead, by the excess and by the wall time that
* has really elapsed since this signal's last burst.
*
* The floor keeps the step positive when the excess is
* larger than a whole burst (a declared rate that is
* wrong by a factor, not by ppm). It only slows the
* recovery: each burst then advances by almost nothing
* while arrival keeps advancing, so the excess still
* falls to zero, just over several packets. */
* Both terms are needed, and only the second one
* converges. Within this call the wall clock is frozen
* at wallNow, so ANY positive step increases the lead
* measured at this instant; the lead falls only because
* the wall advances BETWEEN packets. A step expressed
* purely as a fraction of the nominal burst width
* therefore diverges as soon as the nominal width
* outruns the packet interval — with the floor alone, a
* declared 30 Hz against a producer really flushing 10
* samples at 1 kHz gained ~0.67 s of lead per second of
* stream, without bound. Capping the burst's total
* advance at kWallBleedFraction of the elapsed wall time
* makes it advance strictly slower than the wall for any
* declared rate, so the lead strictly falls.
*
* The proportional term still does the fine work: when
* the excess is smaller than a burst it removes it in
* one packet. kMinBleedFactor only keeps that term
* positive when the excess exceeds a whole burst.
*
* Steady state is a sawtooth, not a fixed offset: the
* squeeze pulls the lead down, ordinary chaining resumes
* on the very next packet and pushes it back up until
* the prediction misses arrival by more than
* kBurstResyncThresholdS. So the lead cycles between a
* fraction of a millisecond and roughly that threshold —
* bounded, which is what matters, but not zero. */
const double nominal = static_cast<double>(nElems) * dt;
const double excess = st.lastEmittedEnd - wallNow;
double factor = 1.0 - excess / nominal;
if (factor < kMinBleedFactor) { factor = kMinBleedFactor; }
step = dt * factor;
double advance = nominal * factor;
const double wallElapsed = wallNow - st.lastEmittedWall;
if (wallElapsed > 0.0) {
const double cap = kWallBleedFraction * wallElapsed;
if (cap < advance) { advance = cap; }
}
step = advance / static_cast<double>(nElems);
/* Unreachable with a finite positive dt — kept because
* downstream monotonicity must not depend on that
* argument holding for every value off the wire. */
if (!(step > 0.0)) { step = dt * kMinBleedFactor; }
base = st.lastEmittedEnd + step;
}
}
@@ -250,6 +347,7 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
tsOut[e] = base + static_cast<double>(e) * step;
}
st.lastEmittedEnd = tsOut[nElems - 1u];
st.lastEmittedWall = wallNow;
st.lastCounter = f.counter;
st.prevAccCount = nElems;
st.lastEmittedValid = true;
@@ -279,11 +377,47 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
* arbitrary epoch that leaves behind, exactly as it would have latched
* the producer's boot epoch. */
double elapsed = 0.0;
if (st.lastAccValid && f.hrt > st.lastAccHrt) {
elapsed = static_cast<double>(f.hrt - st.lastAccHrt) / rate;
/* Whether lastAccHrt should take this packet's value. Only a packet
* that legitimately defines the new front of producer time may move it;
* see the backward case below. */
bool takeHrt = true;
if (st.lastAccValid) {
if (f.hrt > st.lastAccHrt) {
elapsed = static_cast<double>(f.hrt - st.lastAccHrt) / rate;
} else {
/* hrt went backwards. Two entirely different events look like
* this and only the SIZE of the jump separates them.
*
* A small one is a reordered datagram: the packet that overtook
* it already counted the interval it covers, so it must
* contribute nothing — and must also leave lastAccHrt alone.
* Letting it write lastAccHrt anyway (which is what this code
* used to do unconditionally) rolls the reference back one
* interval, so the NEXT packet's delta spans two and fabricates
* a whole extra packet of producer time. It never heals:
* ClockOffset would correct it, but the monotonic clamp below
* discards every backward correction. A hundred swaps on a
* 25 ms stream left the trace 3.5 s ahead, permanently. The C
* client does not reorder for us — udps_client.c only COUNTS
* counter gaps — so this is reachable on any real network.
*
* A large one is a producer restart: hrt drops from the
* producer's whole uptime to near zero. Here the unconditional
* write was the right behaviour and must be kept, because
* refusing to regress would leave every subsequent packet below
* lastAccHrt forever, elapsed permanently zero and the signal
* frozen. Rebase, and reset the offset so it re-latches against
* the new epoch instead of being dragged there by recalibration. */
const double backward =
static_cast<double>(st.lastAccHrt - f.hrt) / rate;
if (backward > kProducerRestartS) {
st.offset.reset();
} else {
takeHrt = false;
}
}
}
st.accProdSec += elapsed;
double base = st.offset.map(st.accProdSec, wallNow);
/* The flushes carry contiguous RT cycles, so the gap divided by the
* previous packet's sample count is exactly one cycle period. */
@@ -291,21 +425,58 @@ bool FrameDecoder::timestamps(const FrameView& f, uint32_t idx,
? (elapsed / static_cast<double>(st.prevAccCount))
: kDefaultDt;
/* Anchor the burst's LAST element on arrival, not its first. The
* packet's hrt is the tick count of sample 0 (UDPSourceSession.cpp:574),
* so stepping forward from it is right — but ClockOffset latches
* offset = wall - producerSec on its first call, and passing the raw
* arrival would put sample 0 at the instant the packet carrying the
* whole burst LANDED, dating every sample in it late by a burst. The
* declared-rate branch above already anchors on the burst end
* (arrivalAnchor), and two accumulated scalars in one scope, one with a
* declared rate and one without, would otherwise sit a burst apart on a
* shared X axis — 9 ms for 10 samples at 1 kHz, plain to see at a 200 ms
* window. Since map() latches once, this is a constant shift applied at
* latch and recalibration only; it changes no spacing. */
double base = st.offset.map(
st.accProdSec,
wallNow - static_cast<double>(nElems - 1u) * hrtDt);
double step = hrtDt;
/* ClockOffset recalibrates once true drift passes its threshold, and a
* recalibration can land behind where this signal already is.
* Downstream requires increasing stamps, so step forward minimally. */
* Downstream requires increasing stamps, so step forward minimally —
* but a bare forward step is one-directional, exactly the defect the
* declared branch's squeeze exists to avoid. A backward wall step (an
* NTP correction, a suspend/resume) would otherwise leave this signal
* permanently ahead of the wall clock, since the recalibrated base is
* behind lastEmittedEnd on every later packet too and the clamp keeps
* discarding it. So cap the burst's total advance against the wall time
* elapsed since this signal's previous burst, for the reason spelled out
* at kWallBleedFraction: only that makes the lead bleed off. */
if (st.lastEmittedValid && base <= st.lastEmittedEnd) {
base = st.lastEmittedEnd + hrtDt;
const double wallElapsed = wallNow - st.lastEmittedWall;
if (wallElapsed > 0.0) {
const double cap = kWallBleedFraction * wallElapsed /
static_cast<double>(nElems);
if (cap < step) { step = cap; }
}
base = st.lastEmittedEnd + step;
}
tsOut.resize(nElems);
for (uint32_t e = 0; e < nElems; e++) {
tsOut[e] = base + static_cast<double>(e) * hrtDt;
tsOut[e] = base + static_cast<double>(e) * step;
}
st.lastAccHrt = f.hrt;
if (takeHrt) { st.lastAccHrt = f.hrt; }
st.lastAccValid = true;
st.prevAccCount = nElems;
st.lastEmittedEnd = tsOut[nElems - 1u];
st.lastEmittedWall = wallNow;
/* 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;
st.lastEmittedValid = true;
return true;
}