fix(udpscope): stop a wrong hrtDt from displacing the trace permanently
On the hrt branch the derived period is not just a spacing: it is the burst width ClockOffset latches against, so a wrong one shifts the whole trace by an amount that is usually too small for kRecalibThresholdS to ever heal. Three routes to a wrong period were open. Packet loss. elapsed spans every packet since the last one seen, but it was divided by prevAccCount alone, so a lost datagram scaled the period by the whole counter gap. Since a burst is anchored on its LAST element, too wide means it ends in the FUTURE: +22.5 ms for one loss, +225 ms for ten, at 10 samples per 25 ms packet, mis-spacing 2.7% of all samples at 1% loss. The declared branch already reads the counter for exactly this; the hrt branch now does too. Producer restart and reorder. Both leave elapsed at zero, so no period can be measured -- and the restart packet is also the one that re-latches after offset.reset(). Falling back to kDefaultDt is only right at 1 kHz; measured standing displacement was +13.5 ms at 10 samples per 25 ms and -89 ms at 100 per 10 ms. Remember the last measured period instead. A stray hrt == 0 packet re-enters the warm-up branch, which spans from packetBurst's lastPacketWall -- a field the hrt branch never wrote, so it still held the start of the session. After 153 packets that emitted a burst 3.8 s in the past, worse the longer the scope had run. Also: rule 2 with no declared rate stacked every element of the array on one instant (as UDPSourceSession.cpp:522 does, harmlessly, for a host-local consumer). Spread it from consecutive time-signal anchors, which measure the burst on the producer's own clock. Reverts the previous commit's wallElapsed <= 0 change: it was measurably inert -- the step floor two lines below already yields the same number -- and its comment claimed a divergence it did not stop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Martino Ferrari
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
3add2c42b9
commit
f97fd825c4
@@ -107,6 +107,49 @@ TEST(FrameDecoder, FirstSampleAnchorsElementZeroAndCountsForward) {
|
||||
EXPECT_NEAR(ts[3], 2000.003, 1e-9);
|
||||
}
|
||||
|
||||
// With no declared rate there is nothing to spread the array by, and
|
||||
// UDPSourceSession.cpp:522 leaves the step at zero — every element of the array
|
||||
// on one instant. A host-local consumer only stores them; this scope's ring,
|
||||
// decimator and trigger all require increasing stamps, and N points at one X is
|
||||
// not a trace. Consecutive time-signal anchors carry the burst duration on the
|
||||
// PRODUCER'S clock, so the spread is recoverable without a rate.
|
||||
TEST(FrameDecoder, FirstSampleWithNoRateSpreadsFromConsecutiveAnchors) {
|
||||
FrameDecoder dec;
|
||||
dec.setSignals({burst("Sine", kTimeFirstSample, 0.0, 4, 1),
|
||||
timeSignal("Time", 1)});
|
||||
|
||||
/* 4 samples per packet, anchors 4 ms apart: a 1 ms period. Arrivals are
|
||||
* jittered so a spread accidentally taken from arrival would be visible. */
|
||||
const double jitter[4] = {0.0, 0.0021, -0.0017, 0.0};
|
||||
std::vector<double> ts;
|
||||
for (int p = 0; p < 5; p++) {
|
||||
FrameBuilder fb;
|
||||
fb.addSignal({1.0, 2.0, 3.0, 4.0});
|
||||
fb.addSignal({7.0e9 + p * 4.0e6}); /* ns, +4 ms per packet */
|
||||
const FrameView& f = fb.build(0, 2000.0 + p * 0.004 + jitter[p % 4], 4,
|
||||
static_cast<uint32_t>(p + 1));
|
||||
dec.beginFrame(f);
|
||||
ASSERT_TRUE(dec.timestamps(f, 0, ts));
|
||||
ASSERT_EQ(ts.size(), 4u);
|
||||
for (size_t i = 1; i < ts.size(); i++) {
|
||||
/* The first packet has no predecessor to measure against and legally
|
||||
* stacks; from the second on the array must be spread. */
|
||||
if (p > 0) { ASSERT_GT(ts[i], ts[i - 1]) << "packet " << p; }
|
||||
}
|
||||
if (p > 0) { EXPECT_NEAR(ts[1] - ts[0], 0.001, 1e-9) << "packet " << p; }
|
||||
}
|
||||
|
||||
/* A lost datagram doubles the anchor difference; without reading the counter
|
||||
* the recovery packet would be spread twice as wide. */
|
||||
FrameBuilder fb;
|
||||
fb.addSignal({1.0, 2.0, 3.0, 4.0});
|
||||
fb.addSignal({7.0e9 + 5 * 4.0e6 + 4.0e6}); /* packet 6 arrives, 5 lost */
|
||||
const FrameView& f = fb.build(0, 2000.024, 4, 7u);
|
||||
dec.beginFrame(f);
|
||||
ASSERT_TRUE(dec.timestamps(f, 0, ts));
|
||||
EXPECT_NEAR(ts[1] - ts[0], 0.001, 1e-9) << "loss stretched the array";
|
||||
}
|
||||
|
||||
TEST(FrameDecoder, LastSampleAnchorsTheFinalElementAndCountsBackward) {
|
||||
FrameDecoder dec;
|
||||
dec.setSignals({burst("Sine", kTimeLastSample, 1000.0, 4, 1),
|
||||
@@ -795,10 +838,13 @@ TEST(FrameDecoder, UndeclaredAccumulatedScalarEndsItsBurstOnArrival) {
|
||||
|
||||
const double ticks = 1.0e9;
|
||||
const uint64_t bootHrt = static_cast<uint64_t>(86400.0 * ticks);
|
||||
/* 10 ms per packet of 10 samples, so the derived period is 1 ms — equal to
|
||||
* the fallback the very first hrt-branch packet has to use, which is what
|
||||
* ClockOffset latches against. Any other period would bake that one packet's
|
||||
* fallback into the offset and blur the convention this test is pinning. */
|
||||
/* 10 ms per packet of 10 samples. The cadence used to matter — the first
|
||||
* hrt-branch packet had no measurable interval, latched ClockOffset using
|
||||
* kDefaultDt, and only a 1 ms derived period made that harmless — but the
|
||||
* warm-up now hands over a real tick reference, so this assertion holds at
|
||||
* every cadence. See UndeclaredAccumulatedScalarCrossesTheHrtHandoverCleanly,
|
||||
* which is the test that pins that down; this one only fixes the convention
|
||||
* that a burst ends, rather than starts, on arrival. */
|
||||
double lastArrival = 0.0;
|
||||
std::vector<double> last;
|
||||
for (int p = 0; p < 60; p++) {
|
||||
@@ -822,10 +868,14 @@ TEST(FrameDecoder, UndeclaredAccumulatedScalarEndsItsBurstOnArrival) {
|
||||
// packetBurst spans arrival gaps until HrtRateFit has collected enough packets,
|
||||
// then the hrt branch takes over. They place a burst differently — packetBurst
|
||||
// ends it at wallNow, the hrt branch at wallNow - (nElems-1)*hrtDt — so the
|
||||
// handover is where a discontinuity hides. It is invisible at 10 samples per
|
||||
// 10 ms packet, the one cadence where the derived period equals the kDefaultDt
|
||||
// fallback, which is exactly why the other tests here could not see it. Sweep
|
||||
// cadences either side of that coincidence.
|
||||
// handover is where a discontinuity hides, and it took two separate blind spots
|
||||
// for the other tests to miss it. UndeclaredAccumulatedScalarEndsItsBurstOnArrival
|
||||
// runs at 10 samples per 10 ms, the one cadence where the derived period equals
|
||||
// the kDefaultDt fallback, so nothing was wrong to see. The two long-run tests
|
||||
// run at 10 samples per 25 ms, where the fallback burst is 9 ms against a 25 ms
|
||||
// packet interval — too narrow to invert, so their monotonicity assertions held
|
||||
// while the trace sat 13.5 ms off the wall clock, which neither of them measures.
|
||||
// So sweep cadences either side of the coincidence AND assert absolute position.
|
||||
TEST(FrameDecoder, UndeclaredAccumulatedScalarCrossesTheHrtHandoverCleanly) {
|
||||
struct Case { uint32_t nElems; double packetSec; };
|
||||
const Case cases[] = {
|
||||
@@ -883,6 +933,146 @@ TEST(FrameDecoder, UndeclaredAccumulatedScalarCrossesTheHrtHandoverCleanly) {
|
||||
}
|
||||
}
|
||||
|
||||
// Lost datagrams widen the hrt tick gap without widening the sample count that
|
||||
// gap is divided by, so a recovery burst is drawn as many times too wide as the
|
||||
// counter gap — and because a burst is anchored on its LAST element, too wide
|
||||
// means it ends in the FUTURE. The declared branch reads the counter to
|
||||
// reinstate the hole exactly; this pins the hrt branch to the same standard.
|
||||
// Assert POSITION, not just spacing: a burst can be correctly spaced and still
|
||||
// be drawn across the wrong stretch of the axis.
|
||||
TEST(FrameDecoder, UndeclaredAccumulatedScalarKeepsItsSpacingThroughPacketLoss) {
|
||||
FrameDecoder dec;
|
||||
dec.setSignals({undeclaredAcc()});
|
||||
|
||||
const double ticks = 1.0e9;
|
||||
const uint64_t bootHrt = static_cast<uint64_t>(86400.0 * ticks);
|
||||
const double packetSec = 0.025;
|
||||
const double sampleDt = 0.0025;
|
||||
/* Runs of 1, 4 and 10 consecutive losses, well clear of each other and of
|
||||
* the fit warm-up. Ten losses is the interesting one: it used to stretch the
|
||||
* recovery burst 11x and date its last sample 225 ms into the future. */
|
||||
const int dropFrom[3] = {120, 200, 300};
|
||||
const int dropLen[3] = {1, 4, 10};
|
||||
|
||||
double worstFuture = 0.0;
|
||||
double last = 0.0;
|
||||
bool seen = false;
|
||||
for (int p = 0; p < 500; p++) {
|
||||
bool dropped = false;
|
||||
for (int k = 0; k < 3; k++) {
|
||||
if (p >= dropFrom[k] && p < dropFrom[k] + dropLen[k]) { dropped = true; }
|
||||
}
|
||||
if (dropped) { continue; }
|
||||
|
||||
FrameBuilder fb;
|
||||
fb.addSignal(std::vector<double>(10, 1.0));
|
||||
const uint64_t hrt = bootHrt + static_cast<uint64_t>(p * packetSec * ticks);
|
||||
const double arrival = 700.0 + p * packetSec;
|
||||
const FrameView& f =
|
||||
fb.build(hrt, arrival, 10, static_cast<uint32_t>(p + 1));
|
||||
dec.beginFrame(f);
|
||||
std::vector<double> ts;
|
||||
if (!dec.timestamps(f, 0, ts)) { continue; }
|
||||
|
||||
for (double t : ts) {
|
||||
if (seen) { ASSERT_GT(t, last) << "backwards at packet " << p; }
|
||||
last = t;
|
||||
seen = true;
|
||||
}
|
||||
if (p > 100) {
|
||||
/* The samples were acquired BEFORE the packet carrying them landed,
|
||||
* so none of them may be stamped after its arrival. */
|
||||
const double future = ts.back() - arrival;
|
||||
if (future > worstFuture) { worstFuture = future; }
|
||||
EXPECT_NEAR(ts[1] - ts[0], sampleDt, sampleDt * 1e-3)
|
||||
<< "spacing stretched at packet " << p;
|
||||
}
|
||||
}
|
||||
EXPECT_LT(worstFuture, 1e-6)
|
||||
<< "a recovery burst ended " << worstFuture << " s in the future";
|
||||
}
|
||||
|
||||
// A restart is the other way kDefaultDt gets latched: hrt goes backwards, so the
|
||||
// restart packet measures no interval of its own, and whatever burst width it
|
||||
// falls back on is baked into ClockOffset. The displacement that leaves — 13.5 ms
|
||||
// at this cadence — is below ClockOffset::kRecalibThresholdS, so it never heals.
|
||||
// AccumulatedScalarSurvivesAProducerRestart asserts only order and spacing and
|
||||
// passes right through it; this asserts absolute position.
|
||||
TEST(FrameDecoder, UndeclaredAccumulatedScalarReturnsToTheWallClockAfterARestart) {
|
||||
FrameDecoder dec;
|
||||
dec.setSignals({undeclaredAcc()});
|
||||
|
||||
const double ticks = 1.0e9;
|
||||
const uint64_t bootHrt = static_cast<uint64_t>(86400.0 * ticks);
|
||||
const double packetSec = 0.025;
|
||||
std::vector<double> lastTs;
|
||||
double lastArrival = 0.0;
|
||||
|
||||
for (int p = 0; p < 400; p++) {
|
||||
FrameBuilder fb;
|
||||
fb.addSignal(std::vector<double>(10, 1.0));
|
||||
/* Packet 200 restarts the producer: hrt returns to a fresh boot and the
|
||||
* counter to 1. The wall clock does not restart. */
|
||||
const bool after = (p >= 200);
|
||||
const uint64_t hrt = after
|
||||
? static_cast<uint64_t>((p - 200) * packetSec * ticks)
|
||||
: bootHrt + static_cast<uint64_t>(p * packetSec * ticks);
|
||||
const uint32_t counter = after ? static_cast<uint32_t>(p - 199)
|
||||
: static_cast<uint32_t>(p + 1);
|
||||
const double arrival = 700.0 + p * packetSec;
|
||||
const FrameView& f = fb.build(hrt, arrival, 10, counter);
|
||||
dec.beginFrame(f);
|
||||
std::vector<double> ts;
|
||||
if (dec.timestamps(f, 0, ts)) { lastTs = ts; lastArrival = arrival; }
|
||||
}
|
||||
|
||||
ASSERT_EQ(lastTs.size(), 10u);
|
||||
EXPECT_NEAR(lastTs.back(), lastArrival, 1e-6)
|
||||
<< "still displaced from the wall clock 200 packets after the restart";
|
||||
EXPECT_NEAR(lastTs[1] - lastTs[0], 0.0025, 2.5e-6);
|
||||
}
|
||||
|
||||
// hrt == 0 sends the packet back to the warm-up branch, which spans from
|
||||
// packetBurst's own lastPacketWall. The hrt branch does not otherwise touch that
|
||||
// field, so it would be left at whenever this signal last took the warm-up
|
||||
// branch — the start of the session — and one stray packet would emit a burst
|
||||
// starting seconds in the past, worse the longer the scope has been running.
|
||||
TEST(FrameDecoder, UndeclaredAccumulatedScalarSurvivesAStrayZeroHrtPacket) {
|
||||
FrameDecoder dec;
|
||||
dec.setSignals({undeclaredAcc()});
|
||||
|
||||
const double ticks = 1.0e9;
|
||||
const uint64_t bootHrt = static_cast<uint64_t>(86400.0 * ticks);
|
||||
const double packetSec = 0.025;
|
||||
double last = 0.0;
|
||||
bool seen = false;
|
||||
|
||||
for (int p = 0; p < 200; p++) {
|
||||
FrameBuilder fb;
|
||||
fb.addSignal(std::vector<double>(10, 1.0));
|
||||
const uint64_t hrt = (p == 153)
|
||||
? 0u
|
||||
: bootHrt + static_cast<uint64_t>(p * packetSec * ticks);
|
||||
const double arrival = 700.0 + p * packetSec;
|
||||
const FrameView& f =
|
||||
fb.build(hrt, arrival, 10, static_cast<uint32_t>(p + 1));
|
||||
dec.beginFrame(f);
|
||||
std::vector<double> ts;
|
||||
if (!dec.timestamps(f, 0, ts)) { continue; }
|
||||
for (double t : ts) {
|
||||
if (seen) {
|
||||
ASSERT_GT(t, last) << "stray zero-hrt packet stepped back "
|
||||
<< (last - t) << " s at packet " << p;
|
||||
}
|
||||
last = t;
|
||||
seen = true;
|
||||
}
|
||||
/* And it must not land far from where the stream already is: spanning
|
||||
* from a session-old reference put the burst 3.8 s in the past. */
|
||||
if (p > 100) { EXPECT_NEAR(ts.back(), arrival, 0.05) << "at packet " << p; }
|
||||
}
|
||||
}
|
||||
|
||||
// The same double delivery that the declared branch guards against — a host
|
||||
// joined on two interfaces receives every unfragmented update twice — reaches an
|
||||
// undeclared-rate signal identically. The guard can only fire if this branch
|
||||
|
||||
Reference in New Issue
Block a user