diff --git a/ac-rs/ZMQ.md b/ac-rs/ZMQ.md index 47080a80..c1ae7521 100644 --- a/ac-rs/ZMQ.md +++ b/ac-rs/ZMQ.md @@ -1551,7 +1551,7 @@ reading either. "vrms_at_0dbfs_in": | null, // post-scale, projected to 0 dBFS "out_state": "measured" | "unchanged" | "absent", "in_state": "measured" | "unchanged" | "absent", - "tau_state": "measured" | "not_measured_no_loopback" | "error" + "tau_state": "measured" | "not_measured_low_snr" | "error" | "disagree_period_shift" | "disagree_other" | "refused_xrun", "tau_s": | null, // interface round-trip delay, seconds; only non-null when tau_state == "measured" "tau_sample_rate": , // condition τ was measured/attempted under @@ -1564,6 +1564,8 @@ reading either. "tau_delta_samples": , // #347: round((reading2 - reading1) * sample_rate) — present only on disagree_* "tau_periods": , // #347: signed period count — present only on tau_state == "disagree_period_shift" "tau_error": "", // present when tau_state is "error", "disagree_period_shift", or "disagree_other" + "tau_pre_impulse_snr_db": , // #368: the (worse-of-two, when both ran) peak's pre-impulse SNR — present on measured / not_measured_low_snr / disagree_*, absent on error + "tau_snr_threshold_db": , // #368: the threshold that SNR was judged against — present alongside tau_pre_impulse_snr_db "error": "", // only present on partial failure (voltage-cal save) "input_port": "", // #370: resolved server-side, e.g. "system:capture_2" — not the client's copy of the request "output_port": "" // ditto, e.g. "system:playback_5" @@ -1587,28 +1589,38 @@ not only what this run measured, and the `*_state` word says which: | `unchanged` | the prompt was skipped; the previously stored value stands | | `absent` | the field holds no value — never set, or just cleared | -**τ (interface latency, #281/#347)** is not prompt-driven — it piggybacks -on the loopback state `cal_prompt` step 2 already established, so there is -no third interactive step and no `unchanged` state (skipping a voltage +**τ (interface latency, #281/#347)** is not prompt-driven — it is not a +third interactive step and has no `unchanged` state (skipping a voltage prompt does not affect it). #347: a single reading is not a measurement of τ on this stack — round-trip latency for a fixed path can vary by exactly one period between client lifetimes, invisible within any one lifetime (stable to 0.001 frames). `calibrate` therefore always runs τ as **two** independent client lifecycles (fresh `start`/`stop` each) and compares -them before storing anything: +them before storing anything. + +**#368**: τ used to run only when `cal_prompt` step 2's `loopback` flag +was `true` — a captured-level proxy for "is this cable patched" that a +loopback 3 dB hot or 4 dB low both failed even though both carried a real, +measurable arrival, and that a loud but uncorrelated interferer could +still pass. τ is now attempted unconditionally; the gate lives inside the +measurement itself, on the deconvolved peak's own pre-impulse SNR, which +is the quantity that actually distinguishes "patched" from "not patched." +`cal_prompt` step 2's own `loopback` flag is unchanged and keeps gating +only whether the DMM prompt pre-fills the output reading — a separate, +still-unity-keyed decision. | `tau_state` | meaning | |-------------|---------| -| `measured` | loopback detected this run; two independent readings agreed to the whole sample and their average was appended to `tau_history` | -| `not_measured_no_loopback` | loopback not detected this run — nothing to measure τ against | -| `error` | loopback was detected but a lifecycle's own measurement failed (`tau_error` names why, including which reading); the voltage-cal legs above are unaffected | +| `measured` | two independent readings agreed to the whole sample and their average was appended to `tau_history` | +| `not_measured_low_snr` | a lifecycle's deconvolved peak was below `tau_snr_threshold_db` pre-impulse SNR — not distinguishable from noise, so nothing was measured | +| `error` | a lifecycle's own measurement failed for a reason other than low SNR (`tau_error` names why, including which reading); the voltage-cal legs above are unaffected | | `disagree_period_shift` | the two readings disagreed by an exact multiple of `tau_period_size` samples — a graph-buffering shift (software), not hardware drift. Nothing is stored. | | `disagree_other` | the two readings disagreed, but not by a period multiple — a different fault class. Nothing is stored. | -| `refused_xrun` | either lifecycle's own `AudioEngine::xruns()` delta was nonzero (#369) — checked *before* the two readings are compared, so this fires even when they would otherwise have agreed, closing the corroboration hole a doubly-corrupted agreeing pair would leave in the `measured` path. Nothing is stored. | +| `refused_xrun` | either lifecycle's own `AudioEngine::xruns()` delta was nonzero (#369) — checked *before* the two readings are compared, so this fires even when they would otherwise have agreed, closing the corroboration hole a doubly-corrupted agreeing pair would leave in the `measured` path. Also takes precedence over `not_measured_low_snr` (#368/#369 merge decision): a lifecycle that crosses an xrun skips its own SNR gate entirely, so a capture an xrun corrupted is never reported as merely low-SNR — a contaminated capture's SNR figure is not a meaningful "no arrival" reading. Nothing is stored. | `tau_sample_rate` / `tau_period_size` are the conditions the attempt ran under (present regardless of `tau_state`, including `error`), so a -`not_measured_no_loopback` or `error` result is still legible against +`not_measured_low_snr` or `error` result is still legible against `cal.json` history without a second round trip. `tau_period_size: null` does not mean unknown — see `AudioEngine::period_size` in `ac-daemon/src/audio/mod.rs`: some backends cannot report a period size @@ -1616,6 +1628,24 @@ at all, which is a documented backend limitation, distinct from a period size that simply wasn't queried (and means `disagree_period_shift` can never fire on that backend — any disagreement there is `disagree_other`). +`tau_pre_impulse_snr_db` / `tau_snr_threshold_db` (#368) are present on +every state where at least one lifecycle reached deconvolution +(`measured`, `not_measured_low_snr`, `disagree_*`), absent on `error` +(which can fail before a peak was ever located), and **also absent on +`refused_xrun`** (#369): an xrun-crossed lifecycle's SNR gate never runs +(see the `refused_xrun` row above), so there is no SNR figure to report — +the state name itself names the cause, and no number is offered that +could be misread as a scored noise floor. On `measured` and `disagree_*`, +the SNR reported is the worse (lower) of the two lifecycles' — both +necessarily cleared the threshold, since a lifecycle that didn't, and +carried no xrun, would have produced `not_measured_low_snr` instead, and +a lifecycle that did carry one would have diverted the whole run to +`refused_xrun` before either `measured` or `disagree_*` could be reached +— so this is a diagnostic figure alongside the result rather than a +second gate. `tau_snr_threshold_db` is a derived constant (see +`ac-daemon/src/handlers/calibrate/tau/measure.rs`'s `TAU_SNR_THRESHOLD_DB` doc +comment for its provenance), not measured on this exact sweep. + On either disagreement state, `tau_reading1_s` / `tau_reading2_s` are the raw seconds values from the two lifecycles, shown verbatim rather than compressed to a delta — the fractional part staying identical across a diff --git a/ac-rs/crates/ac-cli/src/commands/calibrate.rs b/ac-rs/crates/ac-cli/src/commands/calibrate.rs index abc6514d..ccee3c79 100644 --- a/ac-rs/crates/ac-cli/src/commands/calibrate.rs +++ b/ac-rs/crates/ac-cli/src/commands/calibrate.rs @@ -270,15 +270,33 @@ fn render_tau_leg(data: &serde_json::Value) -> Vec { "disagree_period_shift" | "disagree_other" => { render_tau_disagreement_leg(state, data, sample_rate) } + // #368: the peak's own SNR fell short of the threshold it was + // judged against — both are what the daemon actually measured, so + // print them rather than an inferred wiring conclusion. + "not_measured_low_snr" => { + match ( + data.get("tau_pre_impulse_snr_db").and_then(|v| v.as_f64()), + data.get("tau_snr_threshold_db").and_then(|v| v.as_f64()), + ) { + (Some(snr), Some(threshold)) => vec![format!( + " {:<8}not measured (peak SNR {snr:.2} dB, need {threshold:.2} dB, \ + threshold derived)", + "Delay:" + )], + // Fields absent (older daemon claiming this state without + // them): fall through to the raw-state rendering below + // rather than assert numbers the daemon never sent. + _ => vec![format!(" {:<8}not measured (state: {state})", "Delay:")], + } + } "refused_xrun" => render_tau_xrun_leg(data), - // "not_measured_no_loopback" and anything unrecognised (older - // daemon without this field): state the observation, not an - // inferred cause — `is_loopback` is what the daemon saw, not a - // claim about physical wiring the instrument cannot verify. - _ => vec![format!( - " {:<8}not measured (loopback not detected this run)", - "Delay:" - )], + // Anything unrecognised (older daemon, or a future state this + // client doesn't know): state the raw wire value, not an inferred + // cause the instrument cannot verify. Also covers the retired + // `"not_measured_no_loopback"` state — an old daemon predating + // #368 that still sends it renders here, on the raw value, rather + // than asserting the wiring conclusion #368 removed. + _ => vec![format!(" {:<8}not measured (state: {state})", "Delay:")], } } diff --git a/ac-rs/crates/ac-core/src/measurement/report/ir_stats.rs b/ac-rs/crates/ac-core/src/measurement/report/ir_stats.rs index 417bfa87..46ad6c52 100644 --- a/ac-rs/crates/ac-core/src/measurement/report/ir_stats.rs +++ b/ac-rs/crates/ac-core/src/measurement/report/ir_stats.rs @@ -62,7 +62,10 @@ impl MeasurementReport { let arrival_s = delay_samples as f64 / *sample_rate_hz as f64; let pre_region = pre_impulse_region(linear_ir, peak_index); - let pre_impulse_snr_db = pre_impulse_snr_db(pre_region, peak_magnitude); + // Same formula `ac-daemon`'s τ gate calls (#368) — one definition + // of "pre-impulse SNR", not two that can drift. + let pre_impulse_snr_db = + crate::measurement::sweep::pre_impulse_snr_db(linear_ir, peak_index); let (gate_window_s, gate_f_low_hz, gate_window_kind) = resolve_gate(payload.gate.as_ref(), window_len, *sample_rate_hz); let verdict = ir_verdict(peak_magnitude, pre_region, pre_impulse_snr_db); @@ -104,25 +107,13 @@ pub(super) fn ir_peak(linear_ir: &[f64]) -> (usize, f64) { /// floor estimate upward. Empty when the guard band consumes the whole /// pre-peak window — which [`ir_verdict`] treats as a failure, not as a /// clean floor. +/// +/// The guard arithmetic itself lives in `measurement::sweep` (#368), so +/// `ac-daemon`'s τ gate and this read-out cannot drift apart on what +/// "pre-impulse" means; this only turns the length into the slice +/// [`ir_verdict`] needs for its empty check. pub(super) fn pre_impulse_region(linear_ir: &[f64], peak_index: usize) -> &[f64] { - let guard = (linear_ir.len() / 32).max(8); - &linear_ir[..peak_index.saturating_sub(guard)] -} - -/// `20·log10(peak / rms(pre_region))`. `+inf` for an empty region (nothing -/// to measure) and for a true-silent one (`rms == 0.0`); [`ir_verdict`] is -/// what separates those two cases, since only the first is a failure. -pub(super) fn pre_impulse_snr_db(pre_region: &[f64], peak_magnitude: f64) -> f64 { - if pre_region.is_empty() { - return f64::INFINITY; - } - let mean_sq = pre_region.iter().map(|v| v * v).sum::() / pre_region.len() as f64; - let rms = mean_sq.sqrt(); - if rms > 0.0 { - 20.0 * (peak_magnitude / rms).log10() - } else { - f64::INFINITY - } + &linear_ir[..crate::measurement::sweep::pre_impulse_region_len(linear_ir.len(), peak_index)] } /// Gate duration, low-frequency limit and window shape for an IR payload. diff --git a/ac-rs/crates/ac-core/src/measurement/sweep/harmonics.rs b/ac-rs/crates/ac-core/src/measurement/sweep/harmonics.rs index daf9e5e7..da8d2e63 100644 --- a/ac-rs/crates/ac-core/src/measurement/sweep/harmonics.rs +++ b/ac-rs/crates/ac-core/src/measurement/sweep/harmonics.rs @@ -211,6 +211,44 @@ pub fn extract_irs( }) } +/// Number of samples [`pre_impulse_snr_db`] measures its noise floor over: +/// everything strictly before `peak_index`, minus the guard band. Zero when +/// the guard band consumes the whole pre-peak window, which is the case +/// `pre_impulse_snr_db` answers with `f64::INFINITY` — a caller that must +/// distinguish "infinite because the floor is silent" from "infinite +/// because there was no floor to measure" (`report::ir_stats`, #376) +/// checks this rather than keeping its own copy of the guard arithmetic. +pub fn pre_impulse_region_len(ir_len: usize, peak_index: usize) -> usize { + let guard = (ir_len / 32).max(8); + peak_index.saturating_sub(guard).min(ir_len) +} + +/// Pre-impulse SNR of a linear impulse response, in dB: the located peak's +/// magnitude over the RMS of everything strictly before it, minus a small +/// guard band (`(ir.len() / 32).max(8)` samples) so the peak's own skirt +/// doesn't bias the floor estimate upward. `f64::INFINITY` when the +/// pre-peak region is empty or measures true silence (zero RMS). +/// +/// Lifted out of `report::ir_stats` (#368) so `ac-daemon`'s τ gate can call +/// the same formula on the same quantity — "does the deconvolution find a +/// peak with adequate SNR" — rather than keeping two copies of one +/// calculation that could drift apart. `peak_index` is the caller's own +/// argmax over `ir`; this does not recompute it. +pub fn pre_impulse_snr_db(ir: &[f64], peak_index: usize) -> f64 { + let pre_region = &ir[..pre_impulse_region_len(ir.len(), peak_index)]; + if pre_region.is_empty() { + return f64::INFINITY; + } + let peak_magnitude = ir.get(peak_index).map(|v| v.abs()).unwrap_or(0.0); + let mean_sq = pre_region.iter().map(|v| v * v).sum::() / pre_region.len() as f64; + let rms = mean_sq.sqrt(); + if rms > 0.0 { + 20.0 * (peak_magnitude / rms).log10() + } else { + f64::INFINITY + } +} + /// Copy `len` samples of `buf` starting at signed index `start`, padding /// with zeros wherever the request falls outside the buffer, and scale /// sample `i` by `weight(i)`. diff --git a/ac-rs/crates/ac-core/src/measurement/sweep/mod.rs b/ac-rs/crates/ac-core/src/measurement/sweep/mod.rs index 2ac3b747..877f4e4a 100644 --- a/ac-rs/crates/ac-core/src/measurement/sweep/mod.rs +++ b/ac-rs/crates/ac-core/src/measurement/sweep/mod.rs @@ -45,7 +45,9 @@ mod tail_decay; pub use deconv::{deconvolve_full, inverse_sweep, log_sweep}; pub use gated::{gated_frequency_response, tukey_window, GatedResponsePoint}; -pub use harmonics::{extract_irs, DeconvolvedIrs, HarmonicIr}; +pub use harmonics::{ + extract_irs, pre_impulse_region_len, pre_impulse_snr_db, DeconvolvedIrs, HarmonicIr, +}; pub use tail_decay::{check_tail_decay, TailDecayCheck}; /// Parameters for a Farina log sweep. diff --git a/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs b/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs index 859fc76c..cfde9f7b 100644 --- a/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs +++ b/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs @@ -58,6 +58,47 @@ pub(super) fn period_size_override() -> Option { }) } +/// Opt-in, fake-only test hooks (#368): let an external integration test +/// simulate a low/no-SNR capture — the muted-route rig case #368's AC3 +/// needs reachable under `--fake-audio`, which by default always returns a +/// clean, noiseless delayed copy of the played signal (the loopback shape +/// every other τ test relies on). +/// +/// `AC_FAKE_TAU_GAIN_OVERRIDE`: models the loopback cable's own gain, so it +/// scales both `play_and_capture`'s played-signal copy (the τ ESS) and +/// `capture_block`'s tone synthesis (`calibrate` step 2's captured level, +/// via `capture_rms`) — the same cable, read by two different captures. +/// `1.0` (unset) keeps the existing unity loopback on both paths; `0.0` +/// simulates a fully muted route. Before PR #384's codex-qa finding this +/// scaled only `play_and_capture`, so an off-unity gain never reached step +/// 2's `captured_dbfs`/`loopback` fields. +/// `AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE`: peak amplitude of broadband +/// dither added to every sample of `play_and_capture`'s output. `0.0` +/// (unset) is byte-identical to pre-#368 behaviour — with the gain also at +/// its default, `out[j] = 0.0 + s * 1.0 == s`. Combined with a `0.0` gain, +/// the deconvolved IR then contains only the dither at every position, so +/// the peak the daemon finds is indistinguishable from its own noise +/// floor, matching a real muted route's low pre-impulse SNR. +pub(super) fn tau_gain_override() -> f32 { + static OVERRIDE: std::sync::OnceLock = std::sync::OnceLock::new(); + *OVERRIDE.get_or_init(|| { + std::env::var("AC_FAKE_TAU_GAIN_OVERRIDE") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(1.0) + }) +} + +pub(super) fn tau_noise_amplitude_override() -> f32 { + static OVERRIDE: std::sync::OnceLock = std::sync::OnceLock::new(); + *OVERRIDE.get_or_init(|| { + std::env::var("AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(0.0) + }) +} + /// Opt-in, fake-only test hook (#369): lets a test drive one or both of /// `measure_tau_twice`'s two lifecycles across a nonzero xrun count. /// Without this, `FakeEngine::xruns()` never leaves the 0 it is diff --git a/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs b/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs index 4f1d568f..75910512 100644 --- a/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs +++ b/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs @@ -38,7 +38,10 @@ mod stimulus; use anyhow::Result; use std::time::Duration; -use self::hooks::{next_loopback_delay_samples, next_xruns_delta, period_size_override}; +use self::hooks::{ + next_loopback_delay_samples, next_xruns_delta, period_size_override, tau_gain_override, + tau_noise_amplitude_override, +}; use self::ring_mode::{FakeRings, RingDrain}; use self::stimulus::{Stimulus, StimulusGen, Synth}; use super::AudioEngine; @@ -218,6 +221,14 @@ impl AudioEngine for FakeEngine { self.gen.set_correlated_pair(gain, delay_samples); } + /// #368 codex-qa finding on PR #384: `AC_FAKE_TAU_GAIN_OVERRIDE` models + /// the loopback cable's own gain, and `calibrate`'s step-2 captured + /// level (read through this path via `capture_rms`) is that same cable + /// — so the override has to reach it, not just `play_and_capture`'s τ + /// ESS. Before this it was applied only there, so a test driving an + /// off-unity gain through this hook could never actually see step 2 + /// report the off-unity `captured_dbfs`/`loopback` it claimed to + /// exercise. Unset (`1.0`) multiplies by 1.0, i.e. unchanged. fn capture_block(&mut self, duration: f64) -> Result> { let n = self.samples_in(duration); if let Some(out) = self.ring_capture(n, duration, RingDrain::Block) { @@ -225,7 +236,12 @@ impl AudioEngine for FakeEngine { } std::thread::sleep(Duration::from_secs_f64(duration)); let port = self.input_port.clone(); - Ok(self.synth().block(port.as_deref(), duration, 0)) + let gain = tau_gain_override(); + let mut block = self.synth().block(port.as_deref(), duration, 0); + for v in block.iter_mut() { + *v *= gain; + } + Ok(block) } /// Non-clearing drain. In ring mode this is the *contiguous* control arm: @@ -247,14 +263,29 @@ impl AudioEngine for FakeEngine { /// peaks at the expected offset. fn play_and_capture(&mut self, samples: &[f32], tail_s: f64) -> Result> { let delay_samples = next_loopback_delay_samples(); + let gain = tau_gain_override(); + let noise_amp = tau_noise_amplitude_override(); self.xruns += next_xruns_delta(); let tail = (tail_s * self.sample_rate as f64).round() as usize; let total = samples.len() + tail; let mut out = vec![0.0f32; total]; + if noise_amp > 0.0 { + // Deterministic LCG (same constants as `Stimulus::Noise`), + // seeded from the delay so distinct fake sessions get distinct + // dither rather than sharing one repeated sequence. + let mut state: u64 = 0x9E37_79B9_7F4A_7C15 ^ (delay_samples as u64); + for v in out.iter_mut() { + state = state + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let u = ((state >> 40) as f64 / (1u64 << 24) as f64) * 2.0 - 1.0; + *v = (noise_amp as f64 * u) as f32; + } + } for (i, &s) in samples.iter().enumerate() { let j = i + delay_samples; if j < total { - out[j] = s; + out[j] += s * gain; } } Ok(out) diff --git a/ac-rs/crates/ac-daemon/src/handlers/calibrate/mod.rs b/ac-rs/crates/ac-daemon/src/handlers/calibrate/mod.rs index 9f56802a..2bf75dd0 100644 --- a/ac-rs/crates/ac-daemon/src/handlers/calibrate/mod.rs +++ b/ac-rs/crates/ac-daemon/src/handlers/calibrate/mod.rs @@ -283,7 +283,7 @@ pub fn calibrate(state: &ServerState, cmd: &Value) -> Value { } // Fallback conditions for the `cal_done` wire frame when τ isn't - // measured this run (no-loopback, or a lifecycle error before any + // measured this run (low SNR, or a lifecycle error before any // conditions were captured) — ZMQ.md requires `tau_sample_rate` / // `tau_period_size` present regardless of `tau_state`. let fallback_sample_rate = eng.sample_rate(); @@ -292,18 +292,20 @@ pub fn calibrate(state: &ServerState, cmd: &Value) -> Value { eng.set_silence(); eng.stop(); - // τ (interface latency, #281/#347) — not prompt-driven, so it - // piggybacks on the loopback state established above rather than - // adding a third interactive step. Measured whenever a loopback was - // detected this run, regardless of whether either voltage prompt - // was answered or skipped — the cheap-refresh path (#279: both - // prompts skipped) still refreshes τ. #347: a single reading is not + // τ (interface latency, #281/#347) — not prompt-driven, so it does + // not add a third interactive step. Always attempted regardless of + // the loopback state step 2 established (#368: τ used to be gated + // on that captured-level proxy; it is now gated on its own + // deconvolved peak's SNR instead, inside `measure_tau` itself) and + // regardless of whether either voltage prompt was answered or + // skipped — the cheap-refresh path (#279: both prompts skipped) + // still refreshes τ. #347: a single reading is not // a measurement of τ on this stack, so this now runs two // independent client lifecycles (`measure_tau_twice`), decoupled // from the voltage-cal `eng` above (already stopped) — see that // function's doc for why the lifecycle boundary matters. let ref_amp = ac_core::shared::generator::dbfs_to_amplitude(ref_dbfs); - let tau_outcome = tau_result(is_loopback, || { + let tau_outcome = tau_result(|| { measure_tau_twice( fake, cfg.backend.as_deref(), diff --git a/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/measure.rs b/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/measure.rs index b1837904..404a8d2e 100644 --- a/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/measure.rs +++ b/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/measure.rs @@ -9,7 +9,7 @@ //! the parent module. use ac_core::measurement::sweep::{ - deconvolve_full, extract_irs, inverse_sweep, log_sweep, SweepParams, + deconvolve_full, extract_irs, inverse_sweep, log_sweep, pre_impulse_snr_db, SweepParams, }; use crate::audio::AudioEngine; @@ -47,6 +47,39 @@ const TAU_MIN_HALF_WINDOW_S: f64 = 0.05; /// once measured against real noise floors. const TAU_EDGE_MARGIN_FRAC: f64 = 0.10; +/// Minimum pre-impulse SNR (dB) a τ lifecycle's deconvolved peak must clear +/// before the reading is trusted at all (#368). This replaces the old +/// pre-attempt `is_loopback` gate, which keyed on a *captured level* +/// against a unity-gain expectation — a proxy that a hot cable (3.01 dB +/// over unity) or a low-gain cable (4.19 dB under) both fail even though +/// both carry a perfectly real, measurable arrival, and that a loud but +/// uncorrelated interferer could still pass. This checks the quantity that +/// actually distinguishes "patched" from "not patched": whether the +/// deconvolution the τ sweep produced finds a peak that stands clear of its +/// own pre-impulse noise floor, measured under the exact drive and gain +/// conditions τ was measured under. +/// +/// Provenance: derived, not measured on this exact sweep. Two rig sessions +/// anchor it from different contexts — +/// `work/rig/rig-2026-08-22-tau-window-350-results.md` measured real +/// electrical-loopback τ SNR at 33.8–83.5 dB (the low end a JACK-startup- +/// transient artefact on the first reading after engine start, not a true +/// floor); #376's rig session measured a deconvolution noise cliff at +/// ~16 dB pre-impulse SNR on an unrelated (long-ESS, acoustic) path. 24 dB +/// splits that gap, rounded toward the reject side rather than the +/// midpoint — a false accept (a spurious peak silently stored in +/// `tau_history`) is more expensive than a false refuse (operator sees +/// "not measured" and re-runs). Wired through the same `tau-window- +/// override` env-override mechanism as `TAU_EDGE_MARGIN_FRAC` so a rig +/// session can correct it without a rebuild. +/// +/// Not to be confused with `report::ir_stats`'s `PRE_IMPULSE_SNR_MIN_DB` +/// (18.0 dB, #376): that one gates a long-ESS *acoustic* capture's IR +/// read-out, this one gates a short-ESS *electrical* τ lifecycle. Same +/// quantity (`sweep::pre_impulse_snr_db`), different path, different +/// evidence — which is why they are two constants and not one. +pub(super) const TAU_SNR_THRESHOLD_DB: f64 = 24.0; + /// Rig-instrument overrides for the two τ window constants (#350). /// /// Compiled in only under the `tau-window-override` feature, which is off @@ -95,12 +128,25 @@ fn tau_edge_margin_frac() -> f64 { TAU_EDGE_MARGIN_FRAC } -/// Per-reading τ diagnostic (#350). `measure_tau` reports only the peak -/// position, so nothing on this path has ever recorded the SNR the peak -/// was located against — which is the quantity #350 exists to measure. -/// `floor` is defined exactly as `it_loopback_ir` and `ir_probe` define -/// it (max |x| over the leading eighth of the window) so the numbers -/// compare directly against #277's record. +#[cfg(feature = "tau-window-override")] +pub(super) fn tau_snr_threshold_db() -> f64 { + tau_env_f64("AC_TAU_SNR_THRESHOLD_DB", TAU_SNR_THRESHOLD_DB) +} + +#[cfg(not(feature = "tau-window-override"))] +pub(super) fn tau_snr_threshold_db() -> f64 { + TAU_SNR_THRESHOLD_DB +} + +/// Per-reading τ diagnostic (#350). `snr_db` is the real gate value — +/// `sweep::pre_impulse_snr_db` on this same peak, computed once by the +/// caller and passed in rather than recomputed here (#368: this used to +/// carry its own separate, leading-eighth-window SNR calculation, which +/// became a second implementation of "is this peak real" once the actual +/// gate needed the same number). `floor`/`far_end` below are a distinct, +/// unrelated diagnostic — max |x| over the leading eighth of the window, +/// defined exactly as `it_loopback_ir` and `ir_probe` define it, so those +/// numbers still compare directly against #277's record. #[cfg(feature = "tau-window-override")] fn tau_probe_log( ir: &[f64], @@ -109,13 +155,13 @@ fn tau_probe_log( window_len: usize, half: usize, sr: u32, + snr_db: f64, ) { let far_end = (ir.len() / 8).max(1); let floor = ir[..far_end] .iter() .map(|v| v.abs()) .fold(0.0_f64, f64::max); - let snr_db = 20.0 * (peak_abs / floor.max(1e-15)).log10(); let margin_frac = tau_edge_margin_frac(); let margin = (margin_frac * half as f64).round() as usize; let dist_from_end = window_len.saturating_sub(1).saturating_sub(peak_idx); @@ -141,6 +187,55 @@ fn tau_probe_log( eprintln!("------------------------"); } +/// Distinguishes a τ lifecycle's low-SNR refusal ([`check_peak_snr`]) from +/// a genuine measurement failure (#368), so `measure_tau_twice` can report +/// a distinct `cal_done.tau_state` (`"not_measured_low_snr"`) instead of +/// folding it into the generic `"error"` state a real engine/deconvolution +/// failure produces. Carried as a typed `anyhow::Error` payload, +/// downcast-recovered by `measure_tau_twice`, rather than a string match on +/// the message — a message wording change must not silently break the +/// state split. +#[derive(Debug, Clone, Copy)] +pub(super) struct LowSnrRefusal { + pub(super) snr_db: f64, + pub(super) threshold_db: f64, +} + +impl std::fmt::Display for LowSnrRefusal { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "\u{3c4} peak pre-impulse SNR {:.2} dB is below the {:.2} dB threshold \u{2014} the \ + deconvolution did not find a peak distinguishable from noise, so no value is \ + reported", + self.snr_db, self.threshold_db + ) + } +} + +impl std::error::Error for LowSnrRefusal {} + +/// Refuse a τ lifecycle whose deconvolved peak sits below `threshold_db` +/// pre-impulse SNR — the peak cannot be trusted as a real arrival rather +/// than noise (#368, replacing the old pre-attempt `is_loopback` level +/// gate). Modeled on [`check_peak_within_window`]'s shape — a small pure +/// function over already-computed values, unit-testable without an +/// `AudioEngine` — and called before it in `measure_tau`, since a peak +/// that isn't real shouldn't be judged against the edge margin at all. +/// +/// Skipped by `measure_tau` entirely when this lifecycle's own capture +/// crossed an xrun (#368/#369 merge precedence) — see its doc comment. +fn check_peak_snr(snr_db: f64, threshold_db: f64) -> anyhow::Result<()> { + if snr_db < threshold_db { + return Err(LowSnrRefusal { + snr_db, + threshold_db, + } + .into()); + } + Ok(()) +} + /// Refuse a peak sitting within `margin_frac` of the half-window of /// either edge of a `window_len`-sample gate. Pulled out of `measure_tau` /// so the edge case can be driven directly in tests without an @@ -165,12 +260,26 @@ fn check_peak_within_window( } /// Play a short ESS, deconvolve it, and return the interface round-trip -/// delay in seconds (peak of the linear IR, converted from samples). +/// delay in seconds (peak of the linear IR, converted from samples) +/// alongside that peak's pre-impulse SNR in dB (#368) and the xrun count +/// `AudioEngine::xruns()` reported across the `play_and_capture` call +/// specifically (#369) — the caller needs both even on success, since +/// `cal_done` reports the SNR on every state that reached deconvolution +/// (not only a refusal) and the xrun count on every state where both +/// lifecycles ran. +/// +/// A capture that crossed an xrun skips the SNR gate entirely +/// (#368/#369 merge precedence): its SNR figure is meaningless — the +/// contamination, not the noise floor it produced, is what `tau_result` +/// reports (`refused_xrun`) once both lifecycles are in. A lifecycle +/// with no xrun keeps the original order (SNR gate before the edge-margin +/// check, unchanged from #368: a peak that isn't distinguishable from +/// noise shouldn't be judged against the window edge at all). /// /// Reuses the Farina machinery from `ac_core::measurement::sweep` exactly /// as `plot_ir` does — see `handlers/audio/plot.rs` for the longer-form /// version of the same technique. -pub(super) fn measure_tau(eng: &mut dyn AudioEngine, amp: f64) -> anyhow::Result { +pub(super) fn measure_tau(eng: &mut dyn AudioEngine, amp: f64) -> anyhow::Result<(f64, f64, u32)> { let sr = eng.sample_rate(); let f2_hz = (sr as f64 * 0.45).min(20_000.0); let params = SweepParams { @@ -182,7 +291,9 @@ pub(super) fn measure_tau(eng: &mut dyn AudioEngine, amp: f64) -> anyhow::Result let sweep = log_sweep(¶ms)?; let amp = amp as f32; let scaled: Vec = sweep.iter().map(|&s| s * amp).collect(); + let xruns_before = eng.xruns(); let captured = eng.play_and_capture(&scaled, TAU_TAIL_S)?; + let xruns = eng.xruns().saturating_sub(xruns_before); let inv = inverse_sweep(¶ms)?; let full = deconvolve_full(&captured, &inv); let half_window_s = tau_half_window_s(); @@ -203,13 +314,31 @@ pub(super) fn measure_tau(eng: &mut dyn AudioEngine, amp: f64) -> anyhow::Result .map(|(i, v)| (i, *v)) .max_by(|a, b| a.1.abs().partial_cmp(&b.1.abs()).unwrap()) .ok_or_else(|| anyhow::anyhow!("empty IR from τ sweep"))?; + let snr_db = pre_impulse_snr_db(&irs.linear, peak_idx); #[cfg(feature = "tau-window-override")] - tau_probe_log(&irs.linear, peak_idx, peak_val.abs(), window_len, half, sr); + tau_probe_log( + &irs.linear, + peak_idx, + peak_val.abs(), + window_len, + half, + sr, + snr_db, + ); #[cfg(not(feature = "tau-window-override"))] let _ = peak_val; + // #368/#369 merge precedence: a lifecycle that crossed an xrun skips + // its own SNR gate — that reading's SNR is not evaluated at all, and + // `tau_result` reports `refused_xrun` for the run once both lifecycles + // are in, regardless of what this figure would have said. A clean + // lifecycle keeps the original #368 order: SNR gate before the + // edge-margin check. + if xruns == 0 { + check_peak_snr(snr_db, tau_snr_threshold_db())?; + } check_peak_within_window(peak_idx, window_len, tau_edge_margin_frac())?; let offset_samples = peak_idx as i64 - half as i64; - Ok(offset_samples as f64 / sr as f64) + Ok((offset_samples as f64 / sr as f64, snr_db, xruns)) } #[cfg(test)] @@ -355,4 +484,40 @@ mod tests { ); } } + + /// #368: `check_peak_snr` mirrors `check_peak_within_window`'s shape — + /// pin its boundary the same way (refuses strictly below, accepts at + /// and above). + #[test] + fn check_peak_snr_refuses_below_threshold() { + assert!(check_peak_snr(23.99, 24.0).is_err()); + } + + #[test] + fn check_peak_snr_accepts_at_and_above_threshold() { + assert!(check_peak_snr(24.0, 24.0).is_ok()); + assert!(check_peak_snr(83.5, 24.0).is_ok()); + } + + /// The rig's own measured muted-route reading (#368 triage: drive + /// -30 dBFS, captured -83.8 dBFS) — a concrete refusal, not just a + /// boundary probe. + #[test] + fn check_peak_snr_refuses_the_rigs_measured_muted_route() { + assert!(check_peak_snr(-3.45, TAU_SNR_THRESHOLD_DB).is_err()); + } + + /// The refusal must stay recoverable *by type* — `measure_tau_twice` + /// downcasts to split `not_measured_low_snr` from the generic `error` + /// state, so an `anyhow!`-flavoured rewording of the message here must + /// not quietly collapse the two. + #[test] + fn check_peak_snr_refusal_is_downcastable_to_its_own_type() { + let err = check_peak_snr(-3.45, TAU_SNR_THRESHOLD_DB).expect_err("refused"); + let refusal = err + .downcast_ref::() + .expect("refusal carries its typed payload, not just a message"); + assert_eq!(refusal.snr_db, -3.45); + assert_eq!(refusal.threshold_db, TAU_SNR_THRESHOLD_DB); + } } diff --git a/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/mod.rs b/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/mod.rs index 58468863..03e629c9 100644 --- a/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/mod.rs +++ b/ac-rs/crates/ac-daemon/src/handlers/calibrate/tau/mod.rs @@ -15,7 +15,7 @@ use ac_core::shared::calibration::{compare_tau_readings, TauComparison, TauCondi use crate::audio::make_engine; -use measure::measure_tau; +use measure::{measure_tau, tau_snr_threshold_db, LowSnrRefusal}; /// Method tag stored on every [`TauEntry`] this handler produces. Bumped /// to `_v2` by #340: the window-sizing change below means a τ captured @@ -26,9 +26,10 @@ use measure::measure_tau; /// method tag is the only thing that can invalidate it. pub(super) const TAU_METHOD: &str = "farina_short_ess_v2"; -/// Outcome of one independent τ lifecycle attempt (#347): either both -/// readings were taken and compared, or a lifecycle itself failed (engine -/// start / measurement error) before a comparison was possible. +/// Outcome of one independent τ lifecycle attempt (#347): both readings +/// were taken and compared, one lifecycle's peak was below the SNR +/// threshold (#368), or a lifecycle itself failed (engine start / +/// measurement error) before either could happen. pub(super) enum TauAttempt { Compared { conditions: TauConditions, @@ -40,6 +41,27 @@ pub(super) enum TauAttempt { reading1_xruns: u32, reading2_xruns: u32, comparison: TauComparison, + /// The worse (lower) of the two lifecycles' pre-impulse SNR. Both + /// cleared the τ SNR threshold — a lifecycle that didn't, and + /// carried no xrun, would have produced [`TauAttempt::LowSnr`] + /// instead — with one exception (#368/#369 merge precedence): a + /// lifecycle that crossed an xrun skips the SNR gate entirely, so + /// this figure can be sub-threshold when `reading1_xruns > 0 || + /// reading2_xruns > 0`. `tau_result` routes that case to + /// `TauOutcome::RefusedXrun` before either the comparison or this + /// field is consulted, so `Measured`/`Disagree` — the only + /// `TauOutcome`s this field survives into — still only ever see a + /// value that genuinely cleared the threshold. + pre_impulse_snr_db: f64, + }, + /// A lifecycle's peak sits below the SNR threshold (#368). Short- + /// circuits the same way [`TauAttempt::Error`] does: the second + /// lifecycle does not run once the first has already refused, and + /// `conditions` is `Some` only when the refusal happened on the second + /// lifecycle (mirroring `Error`'s own short-circuit shape below). + LowSnr { + conditions: Option, + pre_impulse_snr_db: f64, }, Error { conditions: Option, @@ -63,13 +85,16 @@ pub(super) fn measure_tau_twice( in_port: &str, amp: f64, ) -> TauAttempt { - // (t, conditions, xruns) — the xrun count is `eng.xruns()` sampled - // immediately before and after the `measure_tau` call, i.e. scoped to - // the sweep-plus-tail I/O call and nothing else in the lifecycle (#369 - // architect note: this is the only I/O call `measure_tau`'s body makes, - // so bracketing the function call already excludes `start`'s JACK - // client registration from the count). - let run_once = || -> anyhow::Result<(f64, TauConditions, u32)> { + // ((tau_s, snr_db), conditions, xruns) — `measure_tau` scopes the xrun + // count to its own `play_and_capture` I/O call (#369 architect note: + // the only I/O call its body makes, so this already excludes `start`'s + // JACK client registration) and, per the #368/#369 merge precedence + // documented on its own doc comment, skips its internal SNR gate + // entirely on a lifecycle that crossed one — that lifecycle still + // needs its own `(tau_s, snr_db)` to reach `TauAttempt::Compared` + // below, where `reading{1,2}_xruns` decides `refused_xrun` regardless + // of what either SNR figure says. + let run_once = || -> anyhow::Result<((f64, f64), TauConditions, u32)> { let mut eng = make_engine(fake, required)?; eng.start(std::slice::from_ref(&out_port.to_string()), Some(in_port))?; let conditions = TauConditions { @@ -80,29 +105,42 @@ pub(super) fn measure_tau_twice( output_port: out_port.to_string(), input_port: in_port.to_string(), }; - let xruns_before = eng.xruns(); let reading = measure_tau(&mut *eng, amp); - let xruns = eng.xruns().saturating_sub(xruns_before); eng.set_silence(); eng.stop(); - reading.map(|t| (t, conditions, xruns)) + reading.map(|(offset_s, snr_db, xruns)| ((offset_s, snr_db), conditions, xruns)) }; - let (reading1_s, conditions, reading1_xruns) = match run_once() { + // #368: a low-SNR refusal is recovered from the error by type, not by + // matching the message — it is a distinct `tau_state`, and a reworded + // message must not silently collapse it back into `error`. + let ((reading1_s, snr1_db), conditions, reading1_xruns) = match run_once() { Ok(r) => r, Err(e) => { - return TauAttempt::Error { - conditions: None, - message: format!("\u{3c4} measurement failed (reading 1 of 2): {e}"), + return match e.downcast_ref::() { + Some(refusal) => TauAttempt::LowSnr { + conditions: None, + pre_impulse_snr_db: refusal.snr_db, + }, + None => TauAttempt::Error { + conditions: None, + message: format!("\u{3c4} measurement failed (reading 1 of 2): {e}"), + }, } } }; - let (reading2_s, conditions2, reading2_xruns) = match run_once() { + let ((reading2_s, snr2_db), conditions2, reading2_xruns) = match run_once() { Ok(r) => r, Err(e) => { - return TauAttempt::Error { - conditions: Some(conditions), - message: format!("\u{3c4} measurement failed (reading 2 of 2): {e}"), + return match e.downcast_ref::() { + Some(refusal) => TauAttempt::LowSnr { + conditions: Some(conditions), + pre_impulse_snr_db: refusal.snr_db, + }, + None => TauAttempt::Error { + conditions: Some(conditions), + message: format!("\u{3c4} measurement failed (reading 2 of 2): {e}"), + }, } } }; @@ -119,6 +157,7 @@ pub(super) fn measure_tau_twice( reading1_xruns, reading2_xruns, comparison, + pre_impulse_snr_db: snr1_db.min(snr2_db), } } @@ -135,8 +174,17 @@ pub(super) fn measure_tau_twice( /// `Option` fields and three `.expect("… when tau_state is measured")` /// at the one call site that stored an entry. pub(super) enum TauOutcome { - /// No loopback detected this run, so no sweep was played at all. - NotMeasuredNoLoopback, + /// #368: a lifecycle's deconvolved peak sat below the τ SNR threshold, + /// so the sweep ran but found nothing distinguishable from noise. This + /// replaces the old `NotMeasuredNoLoopback`, which reported on a + /// *captured level* measured before the sweep — see [`tau_result`]. + /// `conditions` is `None` when the refusal came from the first + /// lifecycle, which short-circuits before any were captured. + NotMeasuredLowSnr { + conditions: Option, + pre_impulse_snr_db: f64, + snr_threshold_db: f64, + }, /// Two independent lifecycles agreed to the whole sample. Measured { conditions: TauConditions, @@ -146,6 +194,12 @@ pub(super) enum TauOutcome { agreement_count: u32, reading1_s: f64, reading2_s: f64, + /// #368: the worse of the two lifecycles' pre-impulse SNR, and the + /// threshold it cleared. Reported on every state that reached a + /// deconvolution, not only on the refusal, so an operator can see + /// how much margin a *passing* run actually had. + pre_impulse_snr_db: f64, + snr_threshold_db: f64, /// #369: always 0 on this variant — a nonzero count on either /// lifecycle diverts to [`TauOutcome::RefusedXrun`] before the /// comparison is consulted. Carried anyway so the wire frame's @@ -174,6 +228,10 @@ pub(super) enum TauOutcome { conditions: TauConditions, reading1_s: f64, reading2_s: f64, + /// #368: as on [`TauOutcome::Measured`] — both lifecycles cleared + /// the threshold, they just did not agree with each other. + pre_impulse_snr_db: f64, + snr_threshold_db: f64, /// #369: always 0 here for the same reason as on /// [`TauOutcome::Measured`] — the xrun check runs first. reading1_xruns: u32, @@ -195,7 +253,7 @@ impl TauOutcome { /// The `tau_state` wire value. See ZMQ.md's `cal_done` table. pub(super) fn state(&self) -> &'static str { match self { - Self::NotMeasuredNoLoopback => "not_measured_no_loopback", + Self::NotMeasuredLowSnr { .. } => "not_measured_low_snr", Self::Measured { .. } => "measured", Self::RefusedXrun { .. } => "refused_xrun", Self::Disagree { periods, .. } => { @@ -215,11 +273,12 @@ impl TauOutcome { /// `tau_sample_rate` / `tau_period_size` on every `cal_done`. pub(super) fn conditions(&self) -> Option<&TauConditions> { match self { - Self::NotMeasuredNoLoopback => None, Self::Measured { conditions, .. } | Self::RefusedXrun { conditions, .. } | Self::Disagree { conditions, .. } => Some(conditions), - Self::Error { conditions, .. } => conditions.as_ref(), + Self::NotMeasuredLowSnr { conditions, .. } | Self::Error { conditions, .. } => { + conditions.as_ref() + } } } @@ -262,8 +321,31 @@ impl TauOutcome { } => json!(agreement_count), _ => json!(0), }; + // #368: present on every state that reached a deconvolution at + // least once — `measured`, `not_measured_low_snr`, `disagree_*` — + // and absent on `error`, which can fail before a peak was ever + // located. + if let Self::Measured { + pre_impulse_snr_db, + snr_threshold_db, + .. + } + | Self::NotMeasuredLowSnr { + pre_impulse_snr_db, + snr_threshold_db, + .. + } + | Self::Disagree { + pre_impulse_snr_db, + snr_threshold_db, + .. + } = self + { + frame["tau_pre_impulse_snr_db"] = json!(pre_impulse_snr_db); + frame["tau_snr_threshold_db"] = json!(snr_threshold_db); + } match self { - Self::NotMeasuredNoLoopback => {} + Self::NotMeasuredLowSnr { .. } => {} Self::Measured { reading1_s, reading2_s, @@ -310,16 +392,18 @@ impl TauOutcome { } } -/// Turn the loopback flag established at step 2 into the [`TauOutcome`] -/// `calibrate` reports — the exact decision #281 QA flagged as untestable -/// because it was inlined in the worker closure, reachable only through a -/// full daemon spawn. `attempt` is only called when `is_loopback`, matching -/// the worker's original behaviour of never running the τ sweep on a run -/// with no loopback detected. -pub(super) fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt) -> TauOutcome { - if !is_loopback { - return TauOutcome::NotMeasuredNoLoopback; - } +/// Turn a τ attempt into the [`TauOutcome`] `calibrate` reports — the exact +/// decision #281 QA flagged as untestable because it was inlined in the +/// worker closure, reachable only through a full daemon spawn. +/// +/// `attempt` always runs (#368). τ used to be gated on the `is_loopback` +/// flag established at step 2 — a captured-level proxy that a hot (+3.01 dB) +/// or low-gain (−4.19 dB) but genuinely patched loopback both fail, and +/// that a loud uncorrelated interferer could pass. The gate now lives +/// inside `measure_tau` itself, on the deconvolved peak's own pre-impulse +/// SNR, so it applies regardless of what step 2 observed and answers the +/// question that actually matters: did this sweep find a real arrival. +pub(super) fn tau_result(attempt: impl FnOnce() -> TauAttempt) -> TauOutcome { match attempt() { TauAttempt::Error { conditions, @@ -328,10 +412,28 @@ pub(super) fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt conditions, message, }, - // #369: dispatch is xrun-first — a lifecycle that crossed an xrun - // is refused without the comparison being consulted at all, which - // is what catches the doubly-corrupted pair that would otherwise - // have *agreed* its way into `measured`. + TauAttempt::LowSnr { + conditions, + pre_impulse_snr_db, + } => TauOutcome::NotMeasuredLowSnr { + conditions, + pre_impulse_snr_db, + snr_threshold_db: tau_snr_threshold_db(), + }, + // #368/#369 precedence: an xrun-crossed lifecycle is refused + // (`refused_xrun`) even when its own SNR would also have been + // below threshold — a contaminated capture's SNR figure is + // meaningless, so there is nothing to gain by reporting it. This + // only matters when a lifecycle both completes far enough to + // produce a `Compared` attempt (i.e. its own SNR gate already + // passed — `measure_tau` checks SNR before returning) *and* + // crossed an xrun; `LowSnr`, produced entirely inside a single + // lifecycle before xruns for that lifecycle are even read here, + // never competes with `refused_xrun` for the same reading. Dispatch + // is xrun-first among the `Compared` arms below: a lifecycle that + // crossed an xrun is refused without the comparison being + // consulted at all, which is what catches the doubly-corrupted + // pair that would otherwise have *agreed* its way into `measured`. TauAttempt::Compared { conditions, reading1_s, @@ -353,12 +455,15 @@ pub(super) fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt reading1_xruns, reading2_xruns, comparison: TauComparison::Agree, + pre_impulse_snr_db, } => TauOutcome::Measured { conditions, tau_s: (reading1_s + reading2_s) / 2.0, agreement_count: 2, reading1_s, reading2_s, + pre_impulse_snr_db, + snr_threshold_db: tau_snr_threshold_db(), reading1_xruns, reading2_xruns, }, @@ -369,10 +474,13 @@ pub(super) fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt reading1_xruns, reading2_xruns, comparison: TauComparison::Disagree(d), + pre_impulse_snr_db, } => TauOutcome::Disagree { conditions, reading1_s, reading2_s, + pre_impulse_snr_db, + snr_threshold_db: tau_snr_threshold_db(), reading1_xruns, reading2_xruns, delta_samples: d.delta_samples, @@ -383,6 +491,7 @@ pub(super) fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt } #[cfg(test)] mod tests { + use super::measure::TAU_SNR_THRESHOLD_DB; use super::*; fn dummy_conditions() -> TauConditions { @@ -406,35 +515,37 @@ mod tests { frame } - /// #281 QA correctness issue 3: the no-loopback path is hard to drive - /// end-to-end under `--fake-audio` (the fake backend's step-2 capture - /// always reads as loopback-shaped), so pin the decision down directly - /// instead. `attempt` must not run at all when there's no loopback. + /// #368: replaces `tau_result_no_loopback_short_circuits_without_ + /// measuring` — the pre-attempt `is_loopback` gate that test pinned + /// down is gone, `attempt` now always runs, and a low-SNR peak is + /// refused *inside* the attempt instead. This is the "measured because + /// the gate was deleted" guard AC8 of #368 asks for at the + /// `tau_result` level: even though `attempt` ran and returned a real + /// conditions/SNR pair, a `LowSnr` outcome must still surface as + /// `not_measured_low_snr` rather than being folded into `measured` or + /// into the generic `error` state. #[test] - fn tau_result_no_loopback_short_circuits_without_measuring() { - let mut called = false; - let outcome = tau_result(false, || { - called = true; - TauAttempt::Compared { - conditions: dummy_conditions(), - reading1_s: 0.001, - reading2_s: 0.001, - reading1_xruns: 0, - reading2_xruns: 0, - comparison: TauComparison::Agree, - } + fn tau_result_low_snr_reports_new_state_and_fields() { + let outcome = tau_result(|| TauAttempt::LowSnr { + conditions: Some(dummy_conditions()), + pre_impulse_snr_db: -3.45, }); - assert_eq!(outcome.state(), "not_measured_no_loopback"); + assert_eq!(outcome.state(), "not_measured_low_snr"); + assert!(outcome.conditions().is_some()); + // Refused, so nothing reaches `tau_history`. assert!(outcome.stored_entry("m").is_none()); let f = frame_for(&outcome); assert_eq!(f["tau_s"], Value::Null); assert_eq!(f["tau_agreement_count"], json!(0)); + assert_eq!(f["tau_pre_impulse_snr_db"].as_f64(), Some(-3.45)); + assert_eq!( + f["tau_snr_threshold_db"].as_f64(), + Some(TAU_SNR_THRESHOLD_DB) + ); + // A refusal is not an error, and carries no readings — neither + // lifecycle produced one. assert!(f.get("tau_error").is_none(), "{f}"); assert!(f.get("tau_reading1_s").is_none(), "{f}"); - assert!( - !called, - "attempt must not run when no loopback was detected" - ); } /// #347: two independent readings agreeing is what "measured" means @@ -442,13 +553,14 @@ mod tests { /// `tau_agreement_count` must always be 2 alongside it. #[test] fn tau_result_agreeing_readings_reports_measured_with_agreement_count() { - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.000_667, reading2_s: 0.000_667, reading1_xruns: 0, reading2_xruns: 0, comparison: TauComparison::Agree, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state(), "measured"); assert!(outcome.conditions().is_some()); @@ -469,6 +581,13 @@ mod tests { // #348 correctness 1). assert!(f.get("tau_delta_samples").is_none(), "{f}"); assert!(f.get("tau_periods").is_none(), "{f}"); + // #368: present on every state that reached deconvolution, so a + // passing run shows how much margin it actually had. + assert_eq!(f["tau_pre_impulse_snr_db"].as_f64(), Some(40.0)); + assert_eq!( + f["tau_snr_threshold_db"].as_f64(), + Some(TAU_SNR_THRESHOLD_DB) + ); // #369: presence tracks "both readings were taken", so a clean run // still carries concrete 0s, not an absent field — a consumer must // never have to read absence as zero. @@ -478,13 +597,14 @@ mod tests { #[test] fn tau_result_averages_two_agreeing_readings() { - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.001_000_00, reading2_s: 0.001_000_02, reading1_xruns: 0, reading2_xruns: 0, comparison: TauComparison::Agree, + pre_impulse_snr_db: 40.0, }); let tau_s = outcome.stored_entry("m").expect("measured").tau_s; assert!((tau_s - 0.001_000_01).abs() < 1e-9); @@ -498,13 +618,14 @@ mod tests { fn tau_result_period_shift_disagreement_refuses_and_names_the_period() { let comparison = compare_tau_readings(4262.064 / 96_000.0, 5286.064 / 96_000.0, 96_000, Some(1024)); - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 4262.064 / 96_000.0, reading2_s: 5286.064 / 96_000.0, reading1_xruns: 0, reading2_xruns: 0, comparison, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state(), "disagree_period_shift"); assert!( @@ -528,13 +649,14 @@ mod tests { #[test] fn tau_result_non_period_disagreement_is_a_different_state() { let comparison = compare_tau_readings(0.0, 0.000_5, 48_000, Some(1024)); - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.0, reading2_s: 0.000_5, reading1_xruns: 0, reading2_xruns: 0, comparison, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state(), "disagree_other"); assert!(outcome.stored_entry("m").is_none()); @@ -552,7 +674,7 @@ mod tests { #[test] fn tau_result_loopback_err_reports_error_state_and_message() { - let outcome = tau_result(true, || TauAttempt::Error { + let outcome = tau_result(|| TauAttempt::Error { conditions: None, message: "\u{3c4} measurement failed (reading 1 of 2): timeout".to_string(), }); @@ -571,6 +693,10 @@ mod tests { msg.contains("timeout"), "error message should name the failure: {msg}" ); + // #368: absent on error — a lifecycle can fail before a peak was + // ever located, so there is no SNR to report. + assert!(f.get("tau_pre_impulse_snr_db").is_none(), "{f}"); + assert!(f.get("tau_snr_threshold_db").is_none(), "{f}"); } /// #369 acceptance criterion: an xrun crossing either lifecycle refuses @@ -579,13 +705,14 @@ mod tests { /// case the two-lifetime rule alone cannot catch. #[test] fn tau_result_xrun_on_one_reading_refuses_even_when_readings_agree() { - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.000_667, reading2_s: 0.000_667, reading1_xruns: 0, reading2_xruns: 1, comparison: TauComparison::Agree, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state(), "refused_xrun"); // Refused, never stored — the corroboration hole this closes is @@ -609,13 +736,14 @@ mod tests { /// Symmetric with the above: reading 1 dirty, reading 2 clean. #[test] fn tau_result_xrun_on_reading1_is_attributed_to_reading1() { - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.000_667, reading2_s: 0.000_667, reading1_xruns: 3, reading2_xruns: 0, comparison: TauComparison::Agree, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state(), "refused_xrun"); let f = frame_for(&outcome); @@ -626,13 +754,14 @@ mod tests { /// Both lifecycles dirty — both counts carried, not summed into one. #[test] fn tau_result_xrun_on_both_readings_carries_both_counts() { - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.000_667, reading2_s: 0.000_667, reading1_xruns: 2, reading2_xruns: 1, comparison: TauComparison::Agree, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state(), "refused_xrun"); let f = frame_for(&outcome); @@ -645,13 +774,14 @@ mod tests { #[test] fn tau_result_xrun_takes_priority_over_disagreement() { let comparison = compare_tau_readings(0.0, 0.000_5, 48_000, Some(1024)); - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.0, reading2_s: 0.000_5, reading1_xruns: 1, reading2_xruns: 0, comparison, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state(), "refused_xrun"); let f = frame_for(&outcome); diff --git a/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs b/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs index 7d0257c1..18d2432d 100644 --- a/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs +++ b/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs @@ -22,8 +22,9 @@ fn calibrate_measures_tau_against_fake_loopback_delay() { "output_channel": 0, "input_channel": 0})); assert_eq!(r["ok"], json!(true)); - // Both prompts skipped — τ must still be measured (it keys only on - // `is_loopback`, established at step 2, independent of the replies). + // Both prompts skipped — τ must still be measured (#368: it is not + // gated on the step-2 loopback flag at all, and never was gated on + // either voltage reply). for step in 1..=2 { expect_prompt(&c, step); reply_vrms(&c, None); @@ -47,6 +48,16 @@ fn calibrate_measures_tau_against_fake_loopback_delay() { // ZMQ.md: tau_delta_samples is present only on disagree_* — an Agree // outcome must not serialize a stray Some(0) (QA #348 correctness 1). assert!(done.get("tau_delta_samples").is_none(), "frame: {done}"); + // #368: present whenever a lifecycle reached deconvolution, including + // a clean "measured" run — not only on the refusal leg. + assert!( + done["tau_pre_impulse_snr_db"].as_f64().is_some(), + "frame: {done}" + ); + assert!( + done["tau_snr_threshold_db"].as_f64().is_some(), + "frame: {done}" + ); // #369 clean-path regression: a run with no xruns still carries the // fields, present-as-zero rather than absent (per ZMQ.md's presence // rule), and does not take the refused_xrun path. @@ -54,6 +65,77 @@ fn calibrate_measures_tau_against_fake_loopback_delay() { assert_eq!(done["tau_reading2_xruns"], json!(0), "frame: {done}"); } +/// #368: the pre-attempt `is_loopback` level gate is gone — τ is refused +/// only when the deconvolved peak itself sits below +/// `tau_snr_threshold_db` pre-impulse SNR. This drives that refusal +/// end-to-end through `--fake-audio`'s low-SNR test hooks +/// (`AC_FAKE_TAU_GAIN_OVERRIDE` / `AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE`, +/// `audio/fake/hooks.rs`): a muted route (gain 0, dither only) must come +/// back `not_measured_low_snr`, not the plausible-looking `measured` a +/// deleted gate would still produce, since the fake backend's default +/// loopback shape has no other codepath capable of returning anything but +/// a clean peak. Pairs with `calibrate_measures_tau_against_fake_loopback_delay` +/// above (a passing, high-SNR loopback) to distinguish "measured because +/// SNR is genuinely adequate" from "measured because the gate was +/// deleted." +#[test] +fn calibrate_reports_not_measured_low_snr_on_muted_fake_loopback() { + let d = Daemon::spawn_with_env(&[ + // Deterministic dither seeded from the loopback delay (see + // `audio/fake/hooks.rs`'s doc comment); the default 32-sample + // delay happens to land this noise-only IR's peak within the + // edge margin, refusing via `check_peak_within_window` instead + // of the SNR gate this test means to exercise — 800 lands well + // clear of either edge (empirically probed, not derived). + ("AC_FAKE_TAU_DELAY_SAMPLES_OVERRIDE", "800,800"), + ("AC_FAKE_TAU_GAIN_OVERRIDE", "0.0"), + ("AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE", "0.01"), + ]); + let cal_path = d.home.join(".config").join("ac").join("cal.json"); + let c = Client::new(&d); + + let r = c.call(json!({"cmd": "calibrate", "ref_dbfs": -10.0, + "output_channel": 0, "input_channel": 0})); + assert_eq!(r["ok"], json!(true)); + + for step in 1..=2 { + expect_prompt(&c, step); + reply_vrms(&c, None); + } + let done = expect_cal_done(&c); + + assert_eq!( + done["tau_state"], + json!("not_measured_low_snr"), + "frame: {done}" + ); + assert_eq!( + done["tau_s"], + json!(null), + "a low-SNR refusal must not report a τ: {done}" + ); + let snr = done["tau_pre_impulse_snr_db"] + .as_f64() + .expect("tau_pre_impulse_snr_db present on a refusal that reached deconvolution"); + let threshold = done["tau_snr_threshold_db"] + .as_f64() + .expect("tau_snr_threshold_db present alongside it"); + assert!( + snr < threshold, + "refused SNR {snr} should be below the {threshold} dB threshold: {done}" + ); + + // Refused, not stored — no entry in tau_history at all. + let after = read_cal_entry(&cal_path); + assert!( + after.get("tau_history").is_none() + || after["tau_history"] + .as_array() + .is_some_and(|a| a.is_empty()), + "a low-SNR refusal must not append to tau_history: {after}" + ); +} + /// #369: a lifecycle that crosses an xrun refuses the reading end-to-end, /// through the real `measure_tau_twice` → `tau_result` → `cal_done` path — /// not just the hand-constructed `TauAttempt::Compared` unit tests. Uses @@ -104,6 +186,144 @@ fn calibrate_reports_refused_xrun_end_to_end() { ); } +/// #368/#369 merge precedence: when a lifecycle both crosses an xrun and +/// would independently have failed the SNR gate (a muted/noise-only +/// route — `AC_FAKE_TAU_GAIN_OVERRIDE`/`AC_FAKE_TAU_NOISE_AMPLITUDE_ +/// OVERRIDE` apply to both lifecycles here, since those two hooks are not +/// call-indexed), the run is reported `refused_xrun`, not +/// `not_measured_low_snr` — a contaminated capture's SNR figure is +/// meaningless, so the xrun is what gets named, not the noise floor it +/// produced. Both lifecycles carry an xrun (`AC_FAKE_XRUNS_OVERRIDE` +/// `"1,1"`) so that both reach `TauAttempt::Compared` at all: if only one +/// did, the *other* (clean-of-xruns, still muted) lifecycle would fail its +/// own SNR gate on its own account and short-circuit into +/// `not_measured_low_snr` before the dirty lifecycle's xrun ever entered +/// the picture — a different mechanism than the one this test exists to +/// pin down. +/// +/// Swap the precedence (restore the SNR gate to run unconditionally +/// inside `measure_tau`, i.e. revert `calibrate/tau/mod.rs`'s `run_once` +/// to call `check_peak_snr` regardless of `xruns`) and this goes red: the +/// first lifecycle would refuse via `not_measured_low_snr` before its own +/// xrun count is ever consulted, and the run never reaches +/// `refused_xrun` at all. +#[test] +fn calibrate_reports_refused_xrun_over_low_snr_when_both_conditions_hold() { + let d = Daemon::spawn_with_env(&[ + ("AC_FAKE_XRUNS_OVERRIDE", "1,1"), + // See the sibling low-SNR test above for why 800 (not the + // default 32): it keeps this noise-only peak clear of the + // edge-margin refusal so the SNR-vs-xrun precedence is what + // this test actually exercises. + ("AC_FAKE_TAU_DELAY_SAMPLES_OVERRIDE", "800,800"), + ("AC_FAKE_TAU_GAIN_OVERRIDE", "0.0"), + ("AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE", "0.01"), + ]); + let cal_path = d.home.join(".config").join("ac").join("cal.json"); + let c = Client::new(&d); + + let r = c.call(json!({"cmd": "calibrate", "ref_dbfs": -10.0, + "output_channel": 0, "input_channel": 0})); + assert_eq!(r["ok"], json!(true)); + + for step in 1..=2 { + expect_prompt(&c, step); + reply_vrms(&c, None); + } + let done = expect_cal_done(&c); + + assert_eq!( + done["tau_state"], + json!("refused_xrun"), + "both lifecycles crossed an xrun *and* are muted (low SNR) — xrun \ + must be the reported cause: {done}" + ); + assert_eq!( + done["tau_s"], + json!(null), + "a refused reading must not report a τ: {done}" + ); + assert_eq!(done["tau_reading1_xruns"], json!(1), "frame: {done}"); + assert_eq!(done["tau_reading2_xruns"], json!(1), "frame: {done}"); + // Not the low-SNR fields' job to report on this path — the state name + // itself is the assertion that xrun, not SNR, was named as the cause. + assert!(done["tau_reading1_s"].as_f64().is_some(), "frame: {done}"); + assert!(done["tau_reading2_s"].as_f64().is_some(), "frame: {done}"); + + // Refused, not stored — no entry in tau_history at all. + let after = read_cal_entry(&cal_path); + assert!( + after.get("tau_history").is_none() + || after["tau_history"] + .as_array() + .is_some_and(|a| a.is_empty()), + "a refused-xrun reading must not append to tau_history: {after}" + ); +} + +/// #368 AC8 (QA request-changes on PR #384; codex-qa finding on the first +/// attempt at this test — see below). `calibrate_measures_tau_against_ +/// fake_loopback_delay` above passes at the fake backend's default unity +/// gain — exactly the one case the old `is_loopback` ±2 dB gate already +/// handled correctly, so it cannot tell "measured because SNR is genuinely +/// adequate" apart from "measured because the gate was deleted" for any +/// off-unity level. This drives the +3.01 dB hot loopback from the issue's +/// own rig case (drive -30 dBFS, captured -30.0 dBFS) through +/// `AC_FAKE_TAU_GAIN_OVERRIDE` and asserts `measured` — a regression that +/// reintroduced any captured-level check keyed near unity would fail this +/// without touching the muted-route test. +/// +/// codex-qa on PR #384 caught that the first version of this test asserted +/// only the final `tau_state`, never the off-unity level it claimed to +/// drive: `AC_FAKE_TAU_GAIN_OVERRIDE` at the time scaled only +/// `play_and_capture` (the τ ESS), not the step-2 tone capture +/// `capture_rms` reads — so step 2 still saw the unity-loopback level and +/// `measured` proved nothing about the off-unity path. Fixed at the +/// source (`audio/fake/mod.rs::capture_block` now applies the same +/// override) and pinned here: step 2's own `captured_dbfs`/`loopback` +/// fields are asserted before the final `tau_state` check, so a regression +/// in either the fake model or a reintroduced level gate fails this test. +#[test] +fn calibrate_measures_tau_on_hot_off_unity_fake_loopback() { + let d = Daemon::spawn_with_env(&[ + ("AC_FAKE_TAU_GAIN_OVERRIDE", "1.4142135623730951"), // +3.01 dB + ]); + let c = Client::new(&d); + + let r = c.call(json!({"cmd": "calibrate", "ref_dbfs": -30.0, + "output_channel": 0, "input_channel": 0})); + assert_eq!(r["ok"], json!(true)); + + expect_prompt(&c, 1); + reply_vrms(&c, None); + let step2 = expect_prompt(&c, 2); + // Unity loopback at ref_dbfs -30.0 would capture at -33.01 dBFS + // (the sine peak/RMS factor); the +3.01 dB override must land step 2 + // at -30.0, matching the issue's own hot-loopback rig case, and take + // it outside the old ±2 dB `is_loopback` window. + let captured_dbfs = step2["captured_dbfs"] + .as_f64() + .expect("captured_dbfs present on step 2"); + assert!( + (captured_dbfs - (-30.0)).abs() < 0.1, + "step 2 must see the +3.01 dB hot level (#368 AC1), not unity loopback: {step2}" + ); + assert_eq!( + step2["loopback"], + json!(false), + "3.01 dB off unity must fall outside the ±2 dB is_loopback window: {step2}" + ); + reply_vrms(&c, None); + let done = expect_cal_done(&c); + + assert_eq!( + done["tau_state"], + json!("measured"), + "3.01 dB hot must not be refused (#368 AC1): {done}" + ); + assert!(done["tau_s"].as_f64().is_some(), "frame: {done}"); +} + /// QA #348 test-coverage gap: every other disagreement test drives /// `compare_tau_readings` or `tau_result` as a pure function, never /// `measure_tau_twice` itself — the function that actually spins up two @@ -166,9 +386,9 @@ fn calibrate_reports_disagree_period_shift_end_to_end() { /// voltage prompts skipped still refreshes stored state cheaply) is an /// explicit issue acceptance criterion for τ too — a skipped-both-prompts /// run must still append a fresh `tau_history` entry, not just leave the -/// voltage legs alone. Previously asserted only by reading the code (τ's -/// branch is keyed on `is_loopback`, not on either reply); this test pins -/// it down on the wire and on disk. +/// voltage legs alone. Previously asserted only by reading the code (τ is +/// never keyed on either voltage reply, and since #368 not on the step-2 +/// loopback flag either); this test pins it down on the wire and on disk. #[test] fn calibrate_cheap_refresh_still_measures_tau() { let d = Daemon::spawn();