diff --git a/bcm.go b/bcm.go index 22172c2..734b2bf 100644 --- a/bcm.go +++ b/bcm.go @@ -45,8 +45,9 @@ const ( // Covers the handler's documented 2 s freeze during 10GBASE-T training. bcmStatusTimeout = 3 * time.Second - ecdPoll = 200 * time.Millisecond - ecdDeadline = 50 * time.Second + ecdPoll = 200 * time.Millisecond + ecdArmDeadline = 2 * time.Second + ecdDeadline = 50 * time.Second ) type bcm struct { @@ -272,6 +273,24 @@ func (b *bcm) cableDiag() (res ecdResult, err error) { if err = b.mdioWrite(bcmMMDVendor, bcmRegECDCtrl, ctrl&^0xF400|0x8400); err != nil { return } + // A diag takes seconds against a 200 ms poll, so busy must be seen set + // before clear means finished; accepting an early clear reads the + // result of a diag that never armed, which is an instant all-zero + // "success" while restartAN still tears the link down. + arm := time.Now().Add(ecdArmDeadline) + for { + if ctrl, err = b.mdioRead(bcmMMDVendor, bcmRegECDCtrl); err != nil { + return + } + if ctrl&0x0800 != 0 { + break + } + if time.Now().After(arm) { + err = fmt.Errorf("%s: cable diag never armed within %s", b.ifname, ecdArmDeadline) + return + } + time.Sleep(ecdPoll) + } deadline := time.Now().Add(ecdDeadline) for { if ctrl, err = b.mdioRead(bcmMMDVendor, bcmRegECDCtrl); err != nil { diff --git a/main.go b/main.go index c0bf168..9807a2c 100644 --- a/main.go +++ b/main.go @@ -56,8 +56,8 @@ func resetAll(dirs []*direction, mods []*phyModule) time.Time { // Nothing to hold or revert here: failures during a measure were never // counted, so the view is always the counters as they stand. func phyView(diag *cableDiag, modules []*phyModule) (phyDisplay, bool) { - info, rng, measuring := diag.snapshot() - return phyDisplayFrom(info, rng, measuring, modules[0].view(), modules[1].view()), measuring + info, lens, measuring := diag.snapshot() + return phyDisplayFrom(info, lens, measuring, modules[0].view(), modules[1].view()), measuring } var intervalCols = []colSpec{ @@ -346,7 +346,7 @@ func run(aName, bName string) (err error) { // hide or revert afterwards. kickMeasure := func() { if diag.kick(&done) { - diag.resetRange() + diag.resetLengths() start = resetAll(dirs, modules) hist.reset() } diff --git a/phy.go b/phy.go index 095f1a1..c6353e9 100644 --- a/phy.go +++ b/phy.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "slices" "sync" "sync/atomic" "time" @@ -155,6 +156,14 @@ func (m *phyModule) poll() error { return err } + // Setting busy is a measure's first act, so busy still clear here proves + // every read above finished before the measure disturbed anything; a + // measure that began mid-read makes the samples ambiguous, and forgive() + // re-baselines after it anyway. + if m.busy.Load() { + return nil + } + m.mu.Lock() if m.link && !link { m.notes = append(m.notes, @@ -267,18 +276,22 @@ type cableInfo struct { haveMaps [2]bool } +// The median across the pairs rather than the mean: the pairs run the same +// cable, so one pair's skewed TDR estimate is an outlier, not signal. Always +// a value some pair actually measured — the lower middle when the count is +// even, never an average. func (c cableInfo) metresValue() (int, bool) { - sum, n := 0, 0 + var ok []int for i, v := range c.ecd.verdicts { if v == pairOK { - sum += c.ecd.metres[i] - n++ + ok = append(ok, c.ecd.metres[i]) } } - if n == 0 { + if len(ok) == 0 { return 0, false } - return (sum + n/2) / n, true + slices.Sort(ok) + return ok[(len(ok)-1)/2], true } func (c cableInfo) metresString() string { @@ -289,26 +302,24 @@ func (c cableInfo) metresString() string { return fmt.Sprintf("%d", m) } -type cableRange struct { - have bool - min, max int +// One value per completed healthy measure; the running median steadies as +// they accumulate, and like every median here it is always a value that was +// actually measured. +type cableLengths struct { + vals []int } -func (r *cableRange) add(m int) { - if !r.have || m < r.min { - r.min = m - } - if !r.have || m > r.max { - r.max = m - } - r.have = true +func (l *cableLengths) add(m int) { + l.vals = append(l.vals, m) } -func (r cableRange) String() string { - if r.min == r.max { - return fmt.Sprintf("%d", r.min) +func (l cableLengths) median() (int, bool) { + if len(l.vals) == 0 { + return 0, false } - return fmt.Sprintf("%d-%d", r.min, r.max) + s := slices.Clone(l.vals) + slices.Sort(s) + return s[(len(s)-1)/2], true } const ( @@ -378,7 +389,7 @@ func cableSummary(cable cableInfo, measuring bool) (string, int) { // The margin is the worst pair across the ends that measure SNR (the Wiitek's // IEEE 1.133–136), gated on the whole pair being fresh and linked. -func phyDisplayFrom(cable cableInfo, rng cableRange, measuring bool, a, b phyModView) phyDisplay { +func phyDisplayFrom(cable cableInfo, lens cableLengths, measuring bool, a, b phyModView) phyDisplay { d := phyDisplay{ haveSNR: a.fresh && b.fresh && a.link && b.link && (a.haveSNR || b.haveSNR), corrected: a.blocks + a.ber + a.retrain + b.blocks + b.ber + b.retrain, @@ -399,8 +410,11 @@ func phyDisplayFrom(cable cableInfo, rng cableRange, measuring bool, a, b phyMod } } d.metres, d.metresClass = cableSummary(cable, measuring) - if d.metresClass == clsGood && rng.have { - d.metres = rng.String() + // The running median outlives moments without a fresh verdict — a + // running measure, or one that returned no data — and only an actual + // fault verdict replaces it. + if m, ok := lens.median(); ok && (d.metresClass == clsGood || d.metresClass == clsNone) { + d.metres, d.metresClass = fmt.Sprintf("%d", m), clsGood } return d } @@ -444,14 +458,16 @@ func measureCable(mods []*phyModule, done *atomic.Bool) (cableInfo, error) { } // A pair the diag just called faulted is not going to relink; waiting the // full carrier timeout would hold the verdict hostage for 25 s on an - // unplugged cable. - healthy := true + // unplugged cable. A verdict of no data is not a fault: the restart above + // took a working link down, and returning before carrier drops the + // suppression while it is still retraining. + faulted := false for _, v := range c.ecd.verdicts { - if v != pairOK { - healthy = false + if v != 0 && v != pairOK { + faulted = true } } - if healthy { + if !faulted { waitCarrier([2]string{mods[0].dev.name(), mods[1].dev.name()}, done) } for i, m := range mods { @@ -475,24 +491,24 @@ type cableDiag struct { // hidden and reverted. measuring atomic.Bool - mu sync.Mutex - info cableInfo - rng cableRange + mu sync.Mutex + info cableInfo + lengths cableLengths } func newCableDiag(mods []*phyModule, info cableInfo) *cableDiag { return &cableDiag{mods: mods, info: info} } -func (c *cableDiag) snapshot() (cableInfo, cableRange, bool) { +func (c *cableDiag) snapshot() (cableInfo, cableLengths, bool) { c.mu.Lock() defer c.mu.Unlock() - return c.info, c.rng, c.measuring.Load() + return c.info, c.lengths, c.measuring.Load() } -func (c *cableDiag) resetRange() { +func (c *cableDiag) resetLengths() { c.mu.Lock() - c.rng = cableRange{} + c.lengths = cableLengths{} c.mu.Unlock() } @@ -513,7 +529,7 @@ func (c *cableDiag) kick(done *atomic.Bool) bool { c.info = info if _, cls := cableSummary(info, false); cls == clsGood { if m, ok := info.metresValue(); ok { - c.rng.add(m) + c.lengths.add(m) } } c.mu.Unlock() diff --git a/phy_test.go b/phy_test.go index 6e4f1cd..972b407 100644 --- a/phy_test.go +++ b/phy_test.go @@ -14,7 +14,7 @@ func TestPhyDisplayWorstMarginAndFault(t *testing.T) { } a := freshMod([4]float64{5, 5, 2, 0.5}) b := freshMod([4]float64{4, 5, 5, 5}) - d := phyDisplayFrom(cable, cableRange{}, false, a, b) + d := phyDisplayFrom(cable, cableLengths{}, false, a, b) if !d.haveSNR || d.worstMargin != 0.5 { t.Errorf("worstMargin = %v (have %v), want 0.5", d.worstMargin, d.haveSNR) @@ -27,15 +27,15 @@ func TestPhyDisplayWorstMarginAndFault(t *testing.T) { func TestPhyDisplaySNREndOwnsMargin(t *testing.T) { wiitek := freshMod([4]float64{9.2, 7.2, 7.0, 8.7}) fs := phyModView{fresh: true, link: true} - d := phyDisplayFrom(cableInfo{}, cableRange{}, false, wiitek, fs) + d := phyDisplayFrom(cableInfo{}, cableLengths{}, false, wiitek, fs) if !d.haveSNR || d.worstMargin != 7.0 { t.Errorf("worstMargin = %v (have %v), want the SNR end's 7.0", d.worstMargin, d.haveSNR) } - d = phyDisplayFrom(cableInfo{}, cableRange{}, false, fs, wiitek) + d = phyDisplayFrom(cableInfo{}, cableLengths{}, false, fs, wiitek) if !d.haveSNR || d.worstMargin != 7.0 { t.Errorf("swapped ends: worstMargin = %v, want 7.0", d.worstMargin) } - d = phyDisplayFrom(cableInfo{}, cableRange{}, false, fs, fs) + d = phyDisplayFrom(cableInfo{}, cableLengths{}, false, fs, fs) if d.haveSNR { t.Error("a pair with no SNR end should withhold snr") } @@ -50,9 +50,9 @@ func TestCableSummary(t *testing.T) { want string class int }{ - {"clean", cableInfo{ecd: healthy, maps: [2]byte{pairIdentityMap, pairIdentityMap}, haveMaps: [2]bool{true, true}}, false, "49", clsGood}, + {"clean", cableInfo{ecd: healthy, maps: [2]byte{pairIdentityMap, pairIdentityMap}, haveMaps: [2]bool{true, true}}, false, "48", clsGood}, {"far-end swap", cableInfo{ecd: healthy, maps: [2]byte{pairIdentityMap, 0xE1}, haveMaps: [2]bool{true, true}}, false, "xover", clsWarn}, - {"one-end map only", cableInfo{ecd: healthy, maps: [2]byte{pairIdentityMap, 0}, haveMaps: [2]bool{true, false}}, false, "49", clsGood}, + {"one-end map only", cableInfo{ecd: healthy, maps: [2]byte{pairIdentityMap, 0}, haveMaps: [2]bool{true, false}}, false, "48", clsGood}, {"fault", cableInfo{ecd: ecdResult{verdicts: [4]int{pairOK, pairShort, pairXtalk, pairOK}, metres: [4]int{50, 12, 30, 51}}}, false, "SHORT", clsBad}, {"fault beats swap", cableInfo{ecd: ecdResult{verdicts: [4]int{pairOpen, pairOK, pairOK, pairOK}}, maps: [2]byte{pairIdentityMap, 0xE1}, haveMaps: [2]bool{true, true}}, false, "OPEN", clsBad}, {"no diag yet", cableInfo{}, false, "-", clsNone}, @@ -71,11 +71,11 @@ func TestPhyDisplayStaleGoesDim(t *testing.T) { maps: [2]byte{pairIdentityMap, pairIdentityMap}, haveMaps: [2]bool{true, true}, } - d := phyDisplayFrom(cable, cableRange{}, false, freshMod([4]float64{5, 5, 5, 5}), phyModView{}) + d := phyDisplayFrom(cable, cableLengths{}, false, freshMod([4]float64{5, 5, 5, 5}), phyModView{}) if d.haveSNR { t.Error("one silent module should withhold snr") } - if d.metres != "44" || d.metresClass != clsGood { + if d.metres != "45" || d.metresClass != clsGood { t.Errorf("metres = %q class %d, want the diag verdict kept", d.metres, d.metresClass) } } @@ -85,26 +85,26 @@ func TestCableMetresString(t *testing.T) { verdicts: [4]int{pairOK, pairOpen, pairOK, pairOK}, metres: [4]int{45, 3, 41, 46}, }} - if got := c.metresString(); got != "44" { - t.Errorf("metres = %q, want 44", got) + if got := c.metresString(); got != "45" { + t.Errorf("metres = %q, want the healthy pairs' median 45", got) } if got := (cableInfo{}).metresString(); got != "-" { t.Errorf("no diag = %q, want -", got) } } -func TestCableRange(t *testing.T) { - var r cableRange +func TestCableLengths(t *testing.T) { + var l cableLengths for _, m := range []int{42, 40, 44, 41} { - r.add(m) + l.add(m) } - if r.String() != "40-44" { - t.Errorf("range = %q, want 40-44", r.String()) + if m, ok := l.median(); !ok || m != 41 { + t.Errorf("median = %d (%v), want the lower middle 41", m, ok) } - one := cableRange{} + one := cableLengths{} one.add(42) - if one.String() != "42" { - t.Errorf("single measure = %q, want 42", one.String()) + if m, ok := one.median(); !ok || m != 42 { + t.Errorf("single measure = %d (%v), want 42", m, ok) } cable := cableInfo{ @@ -112,14 +112,23 @@ func TestCableRange(t *testing.T) { maps: [2]byte{pairIdentityMap, pairIdentityMap}, haveMaps: [2]bool{true, true}, } - d := phyDisplayFrom(cable, r, false, freshMod([4]float64{5, 5, 5, 5}), freshMod([4]float64{5, 5, 5, 5})) - if d.metres != "40-44" || d.metresClass != clsGood { - t.Errorf("metres = %q class %d, want the range shown good", d.metres, d.metresClass) + d := phyDisplayFrom(cable, l, false, freshMod([4]float64{5, 5, 5, 5}), freshMod([4]float64{5, 5, 5, 5})) + if d.metres != "41" || d.metresClass != clsGood { + t.Errorf("metres = %q class %d, want the running median shown good", d.metres, d.metresClass) } faulted := cableInfo{ecd: ecdResult{verdicts: [4]int{pairOK, pairOpen, pairOK, pairOK}}} - d = phyDisplayFrom(faulted, r, false, freshMod([4]float64{5, 5, 5, 5}), freshMod([4]float64{5, 5, 5, 5})) + d = phyDisplayFrom(faulted, l, false, freshMod([4]float64{5, 5, 5, 5}), freshMod([4]float64{5, 5, 5, 5})) if d.metres != "OPEN" { - t.Errorf("fault shows %q, want the verdict to beat the range", d.metres) + t.Errorf("fault shows %q, want the verdict to beat the median", d.metres) + } + + d = phyDisplayFrom(cable, l, true, freshMod([4]float64{5, 5, 5, 5}), freshMod([4]float64{5, 5, 5, 5})) + if d.metres != "41" || d.metresClass != clsGood { + t.Errorf("measuring shows %q class %d, want the median held", d.metres, d.metresClass) + } + d = phyDisplayFrom(cableInfo{}, l, false, freshMod([4]float64{5, 5, 5, 5}), freshMod([4]float64{5, 5, 5, 5})) + if d.metres != "41" || d.metresClass != clsGood { + t.Errorf("no data shows %q class %d, want the median held", d.metres, d.metresClass) } }