Commit 6f46eea
fix(hl2): linearize the SWR detector and re-derive its noise gate — Principle VIII. (#5521)
## Summary
`Refs #4578`. The SWR published from the HL2's directional coupler is
computed from raw ADC counts that were never taken back through the
detector's curve, and it is admitted from a floor that was a guess. This
linearizes the ratio and re-derives the floor — the second from a
hardware measurement, not from arithmetic.
Four commits: a red one that adds the assertions against the unchanged
arithmetic, a green one that linearizes, a third that corrects the gate
against bench data, and a fourth that brings the prose in line with the
third (`docs/HERMES.md` §17.5 still described both halves as underived
from hardware after one of them had been measured).
### Half A — it reads optimistically low, always
`swrFromRaw()` computed `(fwd + rev) / (fwd - rev)` on raw counts. The
standing defence — repeated in `Hl2Backend::publishTelemetry`, in
`Hl2Backend::healthSnapshot` and in `docs/HERMES.md` §17.5 — was *"a
ratio of two readings from the same converter, so the unknown scale
cancels."*
The scale cancels. **The curve does not.** Writing a count as `c =
k(c)·V`,
rho_shown / rho_true = k(c_rev) / k(c_fwd)
and `k = counts / sqrt(watts)` from the shipped table rises
monotonically across the whole region of interest (512 at 26 counts to
1516 at 4953). Since `c_rev < c_fwd` always, the shown reflection
coefficient is **unconditionally** low and the shown SWR
**unconditionally** optimistic — never conservative, on a meter whose
entire job is to warn about a mismatch. At 265 forward counts a true
2.0:1 displays **1.44**.
`swrFromRaw()` now computes `rho = detectorVolts(rev) /
detectorVolts(fwd)`. Everything else is untouched: the `forwardRaw <= 0
→ nullopt` case, the `rev >= fwd` clamp, and the voltage form `(1 + rho)
/ (1 - rho)` with no square root. The existing comment survives verbatim
— its voltage-proportionality reasoning is correct and is the part
people usually get wrong; the nonlinearity caveat is appended below it,
not substituted for it.
`detectorVolts()` is new: `sqrt(directionalWatts(raw))`, the inverse
curve in arbitrary voltage units since only the ratio is ever used.
Deliberately *not* a second table — taking `sqrt()` of the existing
function makes the interpolation scheme the same one by construction, so
the two cannot drift apart when a per-unit calibration replaces the
points.
### Half B — the noise gate, and why 320 is a measurement
Linearization does **not** fix the low-end saturation and must not be
read as fixing it. Two counts one LSB apart are two nearly-equal numbers
on either side of the curve, and the knee's slope amplifies the reverse
channel relative to the forward one: `20/19` counts goes from a raw
ratio of 39.0 to a linearized **78.0**. It runs away *harder* after the
fix. The gate needed its own raise on its own grounds.
The criterion is unchanged from the original derivation: **one count of
quantisation on either channel must not move the reported SWR by more
than 0.25** — half the finest distinction anything downstream makes (1.5
against 2.0 against 2.5, and the 3.0 at which a TX Cal sweep aborts) —
for every true SWR from 1.0 to 3.0.
An earlier revision of this branch set the gate to 96 on that criterion
swept against the reference curve, and flagged in its own comment that
it assumed a **one-count** channel disagreement that nobody had put an
instrument on. **Bench run D89 put an instrument on it, and 96 was still
too low.** The gate is 320 because it was measured, not because it was
derived:
- **Offset, which is not noise at all.** Fitting the reverse channel
against the forward one across 16 legs spanning 1.3 to 822 forward
counts gives **`rev = 3.41 + 0.00097·fwd`**, residual sd 0.21 counts.
With **no reflected power** the reverse channel still reads ~3.4 counts.
Averaging does not remove a bias and neither does a gate — a gate only
shrinks its weight against a growing forward reading. The intercept is
stable to 0.05 counts over forty minutes and is identical keyed and
unkeyed, so it is the converter and not the PA.
- **Noise.** With RF in the load the reverse channel carries **2.73
counts of standard deviation** and a 0..12 count range (2311 settled
samples, 15 drive levels). It does not shrink at low drive because it
does not come from the signal: keyed with the drive register at zero the
same channel reads 0.67, and unkeyed 0.68 over 6418 samples.
- **The two channels are independent** (Pearson −0.135 to +0.308 across
15 RF legs; sd(fwd−rev) measures 1.052 against 1.002 predicted for
independent channels), **so the noise does not partly cancel in the
ratio** the way common-mode noise would. The one-count model had no term
for any of this.
Re-running the *same* criterion against the measured distributions
instead of the assumed one count:
forward counts 16 32 64 96 160 256 320
p95 error, measured 6.35 1.95 0.91 0.65 0.38 0.26 0.20
p95 error, 1-count 1.57 0.50 0.30 0.20 — — —
The two rows agree exactly where they overlap, which is what makes this
a correction rather than a second opinion. **320 is the smallest gridded
count whose 95th-percentile error stays within 0.25 everywhere above
it** (256 misses by 0.008). At 96 the linearized form publishes 1.40 on
a load the same instrument reads at 1.03–1.06 where it is trustworthy.
Cost: SWR absent below ~74 mW instead of ~12 mW, 18 dB below the rated 5
W.
The conclusion does not rest on the noise figure alone. If the reverse
noise at 96 counts were the quiet no-RF value of 0.67, the **offset
alone** still requires 164 counts. Every route lands above 160; none
near 96.
`kMeasuredReverseFloorCounts = 3.41` is added beside the gate so the
test can **run** the offset criterion rather than restate it, as it
already runs the quantisation one.
### What did not reproduce, and it is the issue's headline
**nigelfenton's 256.00 saturation case did not reproduce on this
radio.** Of 3726 keyed samples, none with forward ≥ 16 produced a
linearized SWR above 10. The mechanism checks out arithmetically against
the measured noise (67% of resampled readings exceed SWR 2.0 at 16
forward counts), but this radio **cannot hold a forward reading that
low**: its gateware decodes only the top nibble of the drive register,
so the smallest steady non-zero output is already ~165 counts. **That is
a bound on the reproduction, not a confirmation of it**, and it should
not be read as one.
Relatedly, the issue's own suggested fix — raising the gate to ~1200 —
does not work, and gating harder is not a substitute for linearizing. At
1197 forward counts a true 2.0 still displays 1.73 on the raw ratio.
Gating on the **forward** channel cannot lift the **reverse** channel
out of the knee: at a true 1.5 the reverse sits a factor of five below
forward in voltage, so putting it above 1200 counts needs about 16 W
forward — past the top of the table and past what an HL2 makes.
### Where the calibration table lives — a decision the triage left open
Triage flagged *"whether the shared calibration table lives in
`MetisProtocol` or stays in `Hl2Backend` with a seam"* as needing a
maintainer. It had to be decided to write the code. **Chosen: the table
moves down.** `Hl2Backend::directionalWatts()` becomes
`AetherSDR::hl2::directionalWatts()` in `MetisProtocol`, with
`detectorVolts()` and the two constants beside it. `Hl2Backend.h`
already includes `MetisProtocol.h` and every call site is inside
`namespace AetherSDR::hl2`, so the existing calls resolve unqualified
and are **textually unchanged** — the only line the diff removes that
names the function is its old definition. No new dependency edge, no
duplicated table, no seam.
**The cost, stated because a maintainer may weigh it differently:**
`MetisProtocol` now carries a calibration concern as well as a
wire-format one. That is a real widening of its remit. If it is refused,
the fallback that preserves the most is an injected-curve seam — it
keeps the layering and keeps the test able to see the arithmetic. **This
is cheap to overrule; say the word and it moves.**
### A small licensing note, offered rather than argued
Noticed while moving the code, and entirely for a maintainer to judge.
The calibration table this PR relocates is Quisk's
`power_meter_std_calibrations['HL2FilterE3']`, reproduced verbatim with
attribution in a source comment. **Quisk does not appear in
`THIRD_PARTY_LICENSES`**, while piHPSDR, openHPSDR, OpenWebRX and WDSP
all do — including entries the file itself marks as *"consulted as a
behavioral reference only"*.
This PR does not introduce the dependency: `MetisProtocol.h` already
reproduces Quisk's `Hermes_BandDict` verbatim for the N2ADR filter-board
masks, so the question predates it and is broader than this change. It
may well be a non-issue — a table of measured count/watt pairs is
plausibly fact rather than expression, and facts are not copyrightable.
Flagging it only because it was in front of me and the file's existing
entries suggest the project likes to record consulted sources even when
nothing is incorporated. Happy to add an entry, or to leave it alone, on
a maintainer's call.
## Tests
`tests/hl2_metis_protocol_test.cpp`, the SWR block.
**Rewritten.** `swrFromRaw(3000, 1000) == 2.0` asserted a **raw-count
ratio** of 1/3 and called it SWR 2.0 — it asserted that the detector is
linear, which it is not, so it locked in the bug and any correct
implementation had to fail it. It is replaced with counts that are a
known true SWR **through the curve** (1000 forward → 0.5046 W → V
0.7103; a true 2.0 → rho 1/3 → 277.9 reverse counts), so
`swrFromRaw(1000, 278)` must return 2.0. What the original assertion was
really protecting — the voltage form against the power form, which would
give 2.37 from the same rho — is still protected, and the comment says
so.
**Added:** the knee (`swrFromRaw(265, 48)`, a true 2.0:1 through the
same curve — raw gives 1.442, linearized 1.992, asserted both within
0.05 of 2.0 *and* explicitly not optimistic); convergence above the knee
(`swrFromRaw(4953, 1623)` within 0.01 of 2.0 and within 0.05 of the raw
ratio's own 1.975, so the change is provably a low-end correction and
not a rescaling of every reading in anyone's log); half B
(`swrFromRaw(20, 19)` still exceeds 30 after linearization, and 20 is
below the gate); and both criteria **run** rather than restated — the
quantisation sweep and the measured-offset check, each asserting the
shipped constant satisfies it.
**Kept:** the `nullopt`, flat-match and clamp assertions, all passing.
**Red before green, observed.** The red commit moves the table and adds
every assertion with the arithmetic and the gate unchanged: 8 assertions
fail, including `the publish gate is at or above the count the
quantisation criterion requires — gate is 16, criterion needs 1201`. The
green commit passes every check in that file. The gate commit shows the
same shape at the old value: `gate is 96; a matched load with the
measured reverse floor 3.41 counts reads 1.371, off by 0.371`, then
passes at 320.
Two of the new assertions — convergence, and "linearization does not
subsume the gate" — **cannot** fail against the unfixed tree, because
what they guard is the fixed implementation. Rather than leave them
unexamined, each was given observed failure evidence by perturbing the
*fixed* code: a wrong exponent in `detectorVolts()` fires the
convergence assertion, and a cap inside `swrFromRaw()` fires the half-B
one.
## What a reviewer should push back on
- **The 0.25 tolerance is chosen, not measured.** Nothing in the code
depends on it being right; it is the number the criterion is stated
with, so that it can be disagreed with. Reading the same measured data
at the median rather than the 95th percentile gives 160 instead of 320.
Both are tabulated in the record so either can be taken without
re-running the bench.
- **Measured on ONE radio.** The 3.41-count offset is a per-unit
property of a diode detector. What generalises is that it is **not
zero**, not its value. A per-unit calibration would replace this
constant along with the curve.
- **Half A has no hardware behind it and cannot get any here.** It needs
a real mismatch at a known true SWR; this station is on a dummy load.
That half rests on the two reporters' evidence and on the algebra.
- **The publish gate is not tested where it is applied.**
`publishTelemetry` is private and reachable only through a fake radio on
a socket. Nothing here asserts that the meter and the Radio Health
snapshot still agree — which was true by reading once before, while they
in fact disagreed. Worth having; not in this PR.
- **The table's new home** — flagged above, not defended to the death.
## Constitution principle honored
**Principle VIII — Evidence Over Assertion.** The bias claim is carried
by a red-before-green pair with the observed failure output, and the two
assertions that could not fail against the unfixed tree were given
failure evidence by deliberate perturbation instead of being left to
look verified. The gate value replaces an assumption with a measurement
and says so, including where the measurement *failed* to reproduce the
issue's headline symptom.
Principle XI is deliberately **not** cited. Its demonstration is CI on
the squash-merge commit, maintainer reproduction, or reporter
confirmation, and it explicitly excludes agent self-grading. None of
those has happened, and the reporter's own case did not reproduce here.
## Test plan
- [x] Local build passes (`cmake --build build`) — **0 `FAILED:` edges**
over the full unpiped log.
- [x] Behavior verified on a real radio if applicable — **partially, and
only the gate.** Bench run D89 read `fwd_pwr`/`rev_pwr` out of a
Hermes-Lite 2's response registers over UDP with AetherSDR not in the
measurement path: 3726 keyed samples across 18 legs of ≤10 s into a
**dummy load**, plus 8978 unkeyed. That is what 320 rests on. **Half A
was not verified on hardware and cannot be at this station**, and the
reporter's saturation case did not reproduce. Ticked because real
hardware genuinely carried part of this; the limits are stated rather
than hidden behind the tick.
- [x] Existing tests pass (CI) — full `ctest`, not a filtered `-R`
subset: **370 tests registered, 370 passed, 0 failed, 5 skipped**, ctest
exit 0, 229 s, on the rebased tree. `hl2_metis_protocol_test` passes
(`all checks passed`). The five skips are the standing environmental set
on this machine (`crdv_quarantined_test`,
`app_settings_safety_explicit-profile-path-isolation`,
`weather_radar_texture_gl_test`, `range_slider_a11y_test`,
`relay_bar_a11y_test`). `vkamp_connection_test`, the known under-load
flake, **passed** in 10.4 s and needed no isolated re-run — note this
supersedes the third commit's own message, which recorded 360/361 with
that test failing; on the rebased tree at this base the suite is fully
green, which is the stronger and the current result. CI has not run yet;
that is the maintainer's gate.
- [x] Reproduction steps documented if user-reported bug — the
arithmetic for half A is reproducible from the shipped table with no
radio; the bench procedure for the gate is written up in full. Half B's
reproduction is documented as a **bound**, not a success.
## Checklist
- [x] Commits are signed — all four SSH-signed; verified with `git
cat-file commit <sha> | grep '^gpgsig'`.
- [x] No new flat-key `AppSettings` calls — no settings touched.
- [x] Code is clean-room — nothing decompiled or reverse-engineered from
a proprietary binary. The calibration table is reproduced from Quisk, an
open-source client, with attribution in the source comment; see the
licensing note above.
- [x] All meter UI uses `MeterSmoother` — no meter UI changed; this is
the value feeding one.
- [x] Documentation updated if user-visible behavior changed —
`docs/HERMES.md` §17.5 updated twice: once because the standing "the
scale cancels" justification was wrong, and again because it went on
describing the gate as underived from hardware after D89 had measured
it. `CHANGELOG.md` deliberately not touched.
- [x] Security-sensitive changes reference a GHSA if applicable — not
security-sensitive. It is *safety*-adjacent (an SWR meter that
under-reads), which is why the correction is in the conservative
direction.
**On the template's self-assignment step:** `on8st` has pull-only
access, so `gh issue edit 4578 --add-assignee on8st` fails with *"on8st
does not have the correct permissions to execute
`ReplaceActorsForAssignable`"*. Recorded rather than left silently
unticked.
**Why `Refs` and not `Fixes`:** this corrects the arithmetic and the
floor, but the issue's headline saturation case did not reproduce here,
half A has no hardware confirmation, and a per-unit calibration is the
real end state for both constants. A maintainer or the reporters are
better placed than I am to say whether #4578 is finished.
**Base:** rebased onto `main` at `8a358c5f`; four commits, clean rebase,
no conflicts.
**On the `maintainer-review` label:** the originating issue carries it
("Requires maintainer review before any action is taken"). This PR is
offered as a proposal for that review, not as a way around it — nothing
here has been merged or acted on upstream, and if the label means the
issue should not have been worked at all, say so and I will close this
without argument.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent 623a97a commit 6f46eea
6 files changed
Lines changed: 525 additions & 99 deletions
File tree
- docs
- src/core/backends/hl2
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2184 | 2184 | | |
2185 | 2185 | | |
2186 | 2186 | | |
2187 | | - | |
| 2187 | + | |
| 2188 | + | |
| 2189 | + | |
| 2190 | + | |
| 2191 | + | |
| 2192 | + | |
| 2193 | + | |
| 2194 | + | |
| 2195 | + | |
| 2196 | + | |
| 2197 | + | |
| 2198 | + | |
| 2199 | + | |
| 2200 | + | |
| 2201 | + | |
| 2202 | + | |
| 2203 | + | |
| 2204 | + | |
| 2205 | + | |
| 2206 | + | |
| 2207 | + | |
| 2208 | + | |
| 2209 | + | |
| 2210 | + | |
| 2211 | + | |
| 2212 | + | |
| 2213 | + | |
| 2214 | + | |
| 2215 | + | |
2188 | 2216 | | |
2189 | 2217 | | |
2190 | 2218 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
96 | 96 | | |
97 | 97 | | |
98 | 98 | | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | | - | |
111 | | - | |
112 | | - | |
113 | | - | |
114 | | - | |
115 | 99 | | |
116 | 100 | | |
117 | 101 | | |
| |||
4482 | 4466 | | |
4483 | 4467 | | |
4484 | 4468 | | |
4485 | | - | |
4486 | | - | |
4487 | | - | |
| 4469 | + | |
| 4470 | + | |
| 4471 | + | |
| 4472 | + | |
4488 | 4473 | | |
4489 | 4474 | | |
4490 | 4475 | | |
| |||
5246 | 5231 | | |
5247 | 5232 | | |
5248 | 5233 | | |
5249 | | - | |
5250 | | - | |
| 5234 | + | |
| 5235 | + | |
| 5236 | + | |
| 5237 | + | |
| 5238 | + | |
5251 | 5239 | | |
5252 | 5240 | | |
5253 | 5241 | | |
| |||
5257 | 5245 | | |
5258 | 5246 | | |
5259 | 5247 | | |
5260 | | - | |
5261 | | - | |
| 5248 | + | |
| 5249 | + | |
| 5250 | + | |
5262 | 5251 | | |
5263 | 5252 | | |
5264 | 5253 | | |
| |||
5379 | 5368 | | |
5380 | 5369 | | |
5381 | 5370 | | |
5382 | | - | |
5383 | | - | |
5384 | | - | |
5385 | | - | |
5386 | | - | |
5387 | | - | |
5388 | | - | |
5389 | | - | |
5390 | | - | |
5391 | | - | |
5392 | | - | |
5393 | | - | |
5394 | | - | |
5395 | | - | |
5396 | | - | |
5397 | | - | |
5398 | | - | |
5399 | | - | |
5400 | | - | |
5401 | | - | |
5402 | | - | |
5403 | | - | |
5404 | | - | |
5405 | | - | |
5406 | | - | |
5407 | | - | |
5408 | | - | |
5409 | | - | |
5410 | | - | |
5411 | | - | |
5412 | | - | |
5413 | | - | |
5414 | | - | |
5415 | | - | |
5416 | | - | |
5417 | | - | |
5418 | | - | |
5419 | | - | |
5420 | | - | |
5421 | | - | |
5422 | | - | |
5423 | | - | |
5424 | | - | |
5425 | | - | |
5426 | | - | |
5427 | | - | |
5428 | | - | |
5429 | | - | |
5430 | | - | |
5431 | | - | |
5432 | | - | |
5433 | | - | |
5434 | | - | |
5435 | | - | |
5436 | | - | |
5437 | | - | |
5438 | | - | |
5439 | | - | |
5440 | | - | |
5441 | | - | |
5442 | 5371 | | |
5443 | 5372 | | |
5444 | 5373 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
261 | 261 | | |
262 | 262 | | |
263 | 263 | | |
264 | | - | |
265 | | - | |
266 | | - | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
267 | 268 | | |
268 | 269 | | |
269 | 270 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
305 | 305 | | |
306 | 306 | | |
307 | 307 | | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
308 | 387 | | |
309 | 388 | | |
310 | 389 | | |
| |||
322 | 401 | | |
323 | 402 | | |
324 | 403 | | |
325 | | - | |
326 | | - | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
327 | 449 | | |
328 | 450 | | |
329 | 451 | | |
330 | 452 | | |
331 | | - | |
| 453 | + | |
| 454 | + | |
332 | 455 | | |
333 | 456 | | |
334 | 457 | | |
| |||
0 commit comments