diff --git a/docs/verb-reference.md b/docs/verb-reference.md index db2f96e8..48de1e69 100644 --- a/docs/verb-reference.md +++ b/docs/verb-reference.md @@ -3599,7 +3599,7 @@ Read the sweep back as one row per finished cell and one summary per arm, and pu **Takes** nothing. -**Answers** `runs`, `arms`, `warning`. `runs` and `arms` are both lists, and an empty `runs` is the normal answer before anything has started. `warning` is present only where the numbers do not mean what they look like: nothing run, one seed, one arm, a cell that failed, or seeds that agree so exactly that a difference between arms has nothing to be called larger than. Each run and each arm summary carries `builds`: the firmware every role actually ran, as `{role, version, file, bytes}`, so two arms that came back identical can be told from two arms that never switched build. +**Answers** `runs`, `arms`, `warning`. `runs` and `arms` are both lists, and an empty `runs` is the normal answer before anything has started. `warning` is present only where the numbers do not mean what they look like: nothing run, one seed, one arm, a cell that failed, or seeds that agree so exactly that a difference between arms has nothing to be called larger than. Each run and each arm summary carries `builds`: the firmware every role actually ran, as `{role, version, file, bytes}`, so two arms that came back identical can be told from two arms that never switched build. Each arm summary also carries `irq_suppressed` and `busy_ms`, its chips' counts per cell averaged over the seeds that counted them, and absent where none did. **Example** - read the table so far diff --git a/docs/verbs.json b/docs/verbs.json index dafe9da9..38e15670 100644 --- a/docs/verbs.json +++ b/docs/verbs.json @@ -1472,7 +1472,7 @@ "arms", "warning" ], - "answers": "`runs` and `arms` are both lists, and an empty `runs` is the normal answer before anything has started. `warning` is present only where the numbers do not mean what they look like: nothing run, one seed, one arm, a cell that failed, or seeds that agree so exactly that a difference between arms has nothing to be called larger than. Each run and each arm summary carries `builds`: the firmware every role actually ran, as `{role, version, file, bytes}`, so two arms that came back identical can be told from two arms that never switched build.", + "answers": "`runs` and `arms` are both lists, and an empty `runs` is the normal answer before anything has started. `warning` is present only where the numbers do not mean what they look like: nothing run, one seed, one arm, a cell that failed, or seeds that agree so exactly that a difference between arms has nothing to be called larger than. Each run and each arm summary carries `builds`: the firmware every role actually ran, as `{role, version, file, bytes}`, so two arms that came back identical can be told from two arms that never switched build. Each arm summary also carries `irq_suppressed` and `busy_ms`, its chips' counts per cell averaged over the seeds that counted them, and absent where none did.", "example": { "params": {}, "what": "read the table so far", diff --git a/internal/app/session/experiment/chipstats_test.go b/internal/app/session/experiment/chipstats_test.go index a69ee84e..bee74166 100644 --- a/internal/app/session/experiment/chipstats_test.go +++ b/internal/app/session/experiment/chipstats_test.go @@ -93,3 +93,32 @@ func TestANodeThatRestartedMidCellReportsWhatItCountedSince(t *testing.T) { t.Errorf("irq_reads %d, want 15", got.IRQReads) } } + +// An arm's chip means come from the seeds whose chips counted, and are absent +// rather than zero where none did: an emulated node sends no record, and its +// zeros averaged in would read as a firmware that missed less. +func TestAnArmsChipMeansLeaveOutWhatWasNotCounted(t *testing.T) { + chip := func(supp uint64, reporting int) *ChipStats { + return &ChipStats{BusyMs: 1000, IRQSuppressed: supp, Reporting: reporting, + Nodes: []NodeChip{{Name: "A"}}} + } + row := summariseArm("1.17.0", []Result{ + {Arm: "1.17.0", Chip: chip(80, 159)}, + {Arm: "1.17.0", Chip: chip(0, 0)}, + {Arm: "1.17.0"}, + }) + if got, _ := row["irq_suppressed"].(float64); got != 80 { + t.Errorf("irq_suppressed %v, want 80 from the one seed that counted", row["irq_suppressed"]) + } + if got, _ := row["busy_ms"].(float64); got != 1000 { + t.Errorf("busy_ms %v, want 1000 from the two seeds with a chip record", row["busy_ms"]) + } + + bare := summariseArm("emulated", []Result{{Arm: "emulated", TX: 3}}) + if _, ok := bare["irq_suppressed"]; ok { + t.Error("an arm with no chip record carries a suppressed mean") + } + if _, ok := bare["busy_ms"]; ok { + t.Error("an arm with no chip record carries a busy mean") + } +} diff --git a/internal/app/session/experiment/report.go b/internal/app/session/experiment/report.go index 0d5acc22..00d9ac66 100644 --- a/internal/app/session/experiment/report.go +++ b/internal/app/session/experiment/report.go @@ -334,9 +334,38 @@ func summariseArm(name string, rs []Result) map[string]any { if at2n > 0 { row["at_risk_2db"] = at2 / at2n } + chipMeans(row, ran) return row } +// chipMeans adds the chip's counters to an arm's row, averaged over the seeds +// whose chips counted them. +// +// Over those seeds only, and absent where there were none, for the reason +// at_risk_2db is: a chip that predates the suppressed count, or an emulated +// node that sends no record, measured nothing, and a mean with its zeros in +// would read as a firmware that missed less. +func chipMeans(row map[string]any, ran []Result) { + var busy, busyN, supp, suppN float64 + for _, r := range ran { + if r.Chip == nil || len(r.Chip.Nodes) == 0 { + continue + } + busy += float64(r.Chip.BusyMs) + busyN++ + if r.Chip.Reporting > 0 { + supp += float64(r.Chip.IRQSuppressed) + suppN++ + } + } + if busyN > 0 { + row["busy_ms"] = busy / busyN + } + if suppN > 0 { + row["irq_suppressed"] = supp / suppN + } +} + // spreadOf is the range of receptions across seeds, which is what says whether // a difference between arms is bigger than the noise within one. // spreadOf is how much the seeds of one arm disagree, as a fraction of that diff --git a/internal/app/session/experiment/results.go b/internal/app/session/experiment/results.go index 71462ee6..003e998c 100644 --- a/internal/app/session/experiment/results.go +++ b/internal/app/session/experiment/results.go @@ -45,7 +45,7 @@ func registerExperimentResults(st *state.Store, s *session.Sim, e *experiment) { // store's goroutine - taking the whole application with it - for // what is at worst one arm with a missing number. arm, _ := m["arm"].(string) - w.Experiment = append(w.Experiment, state.ArmSummary{ + sum := state.ArmSummary{ Arm: arm, Runs: mapInt(m, "runs"), Failed: mapInt(m, "failed"), @@ -57,7 +57,12 @@ func registerExperimentResults(st *state.Store, s *session.Sim, e *experiment) { AirtimeMs: mapFloat(m, "airtime_ms"), RXSpread: mapFloat(m, "rx_spread"), PerSecond: e.perSecondFor(arm), - }) + } + // Present or not, rather than read as zero: an arm whose chips + // counted nothing draws a dash. + sum.IRQSuppressed, sum.HasSuppressed = m["irq_suppressed"].(float64) + sum.BusyMs, sum.HasBusy = m["busy_ms"].(float64) + w.Experiment = append(w.Experiment, sum) } w.ExperimentRuns = e.runRows() w.ExperimentWarning = warn diff --git a/internal/app/session/experiment/results.verbs.json b/internal/app/session/experiment/results.verbs.json index 049c70a2..c9768773 100644 --- a/internal/app/session/experiment/results.verbs.json +++ b/internal/app/session/experiment/results.verbs.json @@ -59,7 +59,7 @@ "arms", "warning" ], - "answers": "`runs` and `arms` are both lists, and an empty `runs` is the normal answer before anything has started. `warning` is present only where the numbers do not mean what they look like: nothing run, one seed, one arm, a cell that failed, or seeds that agree so exactly that a difference between arms has nothing to be called larger than. Each run and each arm summary carries `builds`: the firmware every role actually ran, as `{role, version, file, bytes}`, so two arms that came back identical can be told from two arms that never switched build.", + "answers": "`runs` and `arms` are both lists, and an empty `runs` is the normal answer before anything has started. `warning` is present only where the numbers do not mean what they look like: nothing run, one seed, one arm, a cell that failed, or seeds that agree so exactly that a difference between arms has nothing to be called larger than. Each run and each arm summary carries `builds`: the firmware every role actually ran, as `{role, version, file, bytes}`, so two arms that came back identical can be told from two arms that never switched build. Each arm summary also carries `irq_suppressed` and `busy_ms`, its chips' counts per cell averaged over the seeds that counted them, and absent where none did.", "example": { "params": {}, "what": "read the table so far", diff --git a/internal/app/state/results.go b/internal/app/state/results.go index c1e675cf..20646338 100644 --- a/internal/app/state/results.go +++ b/internal/app/state/results.go @@ -26,6 +26,13 @@ type ArmSummary struct { // spread - and a difference between arms cannot be called larger than a // noise nobody has measured. RXSpread float64 + // IRQSuppressed and BusyMs are what the arm's chips counted over a cell, + // summed across nodes and averaged across seeds: events the firmware's + // IrqMask kept out, and how long the detection flags were up. Two + // firmwares that deliver identically can differ only here. Each Has is + // false where no seed's chip counted it, which is a dash and not a zero. + IRQSuppressed, BusyMs float64 + HasSuppressed, HasBusy bool // PerSecond is receptions in each second after the burst, summed over this // arm's seeds. The shape of the flood rather than its total. PerSecond []int diff --git a/internal/ui/workbench/nodeview/nodewindowradio.go b/internal/ui/workbench/nodeview/nodewindowradio.go index 92c71330..01274e81 100644 --- a/internal/ui/workbench/nodeview/nodewindowradio.go +++ b/internal/ui/workbench/nodeview/nodewindowradio.go @@ -110,12 +110,16 @@ func (p *WindowPanel) radio(t *theme.Theme, gtx layout.Context, s *state.Snapsho radioRow{label: "IRQ mask", value: fmt.Sprintf("%#04x", r.IRQMask), why: "what the firmware allowed into the chip's interrupt register"}, dio1MaskRow(r), + suppressedRow(st), radioRow{label: "IRQ flags", value: fmt.Sprintf("%#04x", r.IRQFlags), why: "what is raised now; a flag that sticks is a node that stops transmitting"}, radioRow{label: "IRQ reads", value: fmt.Sprintf("%d, %d found the air busy", st.IRQReads, st.BusyReads), why: "how often the driver asked, and how often it was told to wait"}, radioRow{label: "busy time", value: fmt.Sprintf("%d ms", st.BusyMs)}, + radioRow{label: "spurious detections", value: fmt.Sprintf("%d", st.Spurious), + why: "false detections the faulty chip variants inject; none on a chip that behaves", + warn: st.Spurious > 0}, ) // Vertical, said out loud. A list left on the zero value lays its rows out @@ -165,6 +169,27 @@ func (p *WindowPanel) radio(t *theme.Theme, gtx layout.Context, s *state.Snapsho // A dash rather than 0x0000 where the node has not said: a radioserver older // than this field sends a shorter record, and zero is a legal mask for a chip // the firmware has not configured yet. "no data" is never drawn as zero. +// suppressedRow is what the IRQ mask above it cost: events the chip raised and +// the firmware never saw. +// +// "Not counted" rather than zero for a chip that predates the count, because +// zero is the answer a firmware that enabled everything gives, and the two +// must not look alike. +func suppressedRow(st state.NodeStat) radioRow { + row := radioRow{label: "IRQ suppressed", + why: "events the chip raised that the IRQ mask kept out of the register; " + + "a firmware waiting on one it never enabled looks like a quiet mesh"} + if !st.SuppressedReported { + row.value = "not counted" + row.why = "this node's chip model predates the count, which arrived in " + + "virtual-sx1262 v1.3.2" + return row + } + row.value = fmt.Sprintf("%d", st.IRQSuppressed) + row.warn = st.IRQSuppressed > 0 + return row +} + func dio1MaskRow(r state.RadioState) radioRow { if !r.DIO1Reported { return radioRow{label: "DIO1 mask", value: "-", diff --git a/internal/ui/workbench/nodeview/radiorows_test.go b/internal/ui/workbench/nodeview/radiorows_test.go new file mode 100644 index 00000000..d8e7228e --- /dev/null +++ b/internal/ui/workbench/nodeview/radiorows_test.go @@ -0,0 +1,32 @@ +package nodeview + +import ( + "testing" + + "github.com/MeshBench/meshbench/internal/app/state" +) + +// A chip that does not keep the suppressed count says so, rather than reading +// as a firmware that enabled every interrupt it needed. +func TestASuppressedCountNobodyKeptIsNotAZero(t *testing.T) { + row := suppressedRow(state.NodeStat{IRQSuppressed: 0}) + if row.value != "not counted" { + t.Errorf("a chip without the count reads %q", row.value) + } + if row.warn { + t.Error("a chip without the count is marked as worth a second look") + } +} + +// A counted figure is shown as it is, and anything above zero is worth a +// second look: an event the firmware never enabled is one it cannot act on. +func TestASuppressedCountIsShownAndMarked(t *testing.T) { + none := suppressedRow(state.NodeStat{SuppressedReported: true}) + if none.value != "0" || none.warn { + t.Errorf("a chip that kept nothing out reads %q, warn %v", none.value, none.warn) + } + some := suppressedRow(state.NodeStat{IRQSuppressed: 14, SuppressedReported: true}) + if some.value != "14" || !some.warn { + t.Errorf("fourteen suppressed events read %q, warn %v", some.value, some.warn) + } +} diff --git a/internal/ui/workbench/nodeview/radioshot_test.go b/internal/ui/workbench/nodeview/radioshot_test.go new file mode 100644 index 00000000..a05fd948 --- /dev/null +++ b/internal/ui/workbench/nodeview/radioshot_test.go @@ -0,0 +1,81 @@ +package nodeview + +import ( + "image/png" + "os" + "path/filepath" + "testing" + + "gioui.org/layout" + + "github.com/MeshBench/meshbench/internal/app/state" + "github.com/MeshBench/meshbench/internal/ui/theme" + "github.com/MeshBench/meshbench/internal/ui/uitest" +) + +// Draw the radio tab for the three answers a chip can give about its mask, and +// look at it. +// +// The figures are one repeater's from a scotland-strict cell: 1.16 enables no +// PreambleDetected for receive and 1.17 does. The third chip predates the count, +// and the row has to say so rather than read as a node that missed nothing. +func TestDrawTheRadioTab(t *testing.T) { + if os.Getenv("MESHBENCH_SHOTS") == "" { + t.Skip("set MESHBENCH_SHOTS= to write the pictures") + } + dir := os.Getenv("MESHBENCH_SHOTS") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + + radio := func(mask uint16) state.RadioState { + return state.RadioState{ + Reported: true, GainReg: 0x96, Boosted: true, TxPowerDBm: 22, + Mode: 1, SF: 8, CR: 8, FreqHz: 869618000, BandwidthHz: 62500, + PreambleSyms: 32, IRQMask: mask, IRQFlags: 0, DIO1Mask: 0x0002, + DIO1Reported: true, + } + } + cases := []struct { + name string + stat state.NodeStat + }{ + {"meshcore-1.16", state.NodeStat{ + Name: "node-1", Running: true, Radio: radio(0x0272), + IRQReads: 2566, BusyReads: 224, BusyMs: 4478, + IRQSuppressed: 14, SuppressedReported: true, + }}, + {"meshcore-1.17", state.NodeStat{ + Name: "node-1", Running: true, Radio: radio(0x0276), + IRQReads: 2501, BusyReads: 67, BusyMs: 4821, + IRQSuppressed: 0, SuppressedReported: true, + }}, + {"chip-without-the-count", state.NodeStat{ + Name: "node-1", Running: true, Radio: radio(0x0276), + IRQReads: 2501, BusyReads: 67, BusyMs: 4821, + }}, + } + + for _, c := range cases { + c := c + t.Run(c.name, func(t *testing.T) { + snap := &state.Snapshot{Stats: []state.NodeStat{c.stat}} + p := &WindowPanel{Node: "node-1", Kind: "simple_repeater"} + p.Tab = TabRadio + + img := uitest.RenderWidget(t, 900, 760, func(gtx layout.Context, th *theme.Theme) layout.Dimensions { + return p.radio(th, gtx, snap) + }) + out := filepath.Join(dir, "radio-"+c.name+".png") + f, err := os.Create(out) + if err != nil { + t.Fatal(err) + } + defer func() { _ = f.Close() }() + if err := png.Encode(f, img); err != nil { + t.Fatal(err) + } + t.Logf("wrote %s", out) + }) + } +} diff --git a/internal/ui/workbench/sweepfailedarm_test.go b/internal/ui/workbench/sweepfailedarm_test.go index 2e2d4fed..1f88a121 100644 --- a/internal/ui/workbench/sweepfailedarm_test.go +++ b/internal/ui/workbench/sweepfailedarm_test.go @@ -87,3 +87,40 @@ func TestMoreIsBetterIsPaintedTheOtherWayRound(t *testing.T) { t.Error("a change smaller than a rounding error was painted as one") } } + +// An arm whose chips counted nothing draws the chip's columns as a dash, and a +// baseline in that state leaves the arm beside it absolute rather than a delta +// against a zero nobody measured. +func TestAChipThatDidNotCountDrawsAsADash(t *testing.T) { + th := linkTestTheme() + counted := state.ArmSummary{Arm: "1.17.0", Runs: 5, RX: 192, + IRQSuppressed: 83.2, HasSuppressed: true, BusyMs: 283620, HasBusy: true} + silent := state.ArmSummary{Arm: "emulated", Runs: 5, RX: 192} + + for _, title := range []string{"IRQ suppressed", "busy time"} { + got, colour := armCell(th, colNamed(title), silent, counted, false) + if got != "-" || colour != th.P.Faint { + t.Errorf("the %s of an arm whose chips did not count reads %q", title, got) + } + got, _ = armCell(th, colNamed(title), counted, silent, false) + if got == "-" || strings.Contains(got, "%") { + t.Errorf("against a baseline that did not count, the %s reads %q, "+ + "not the arm's own figure", title, got) + } + } + if got, _ := armCell(th, colNamed("rx"), silent, counted, false); got == "-" { + t.Error("a chip that did not count took the arm's delivery numbers with it") + } +} + +// Fewer suppressed events is the better direction, so 1.17 against 1.16 is +// painted as a gain. +func TestFewerSuppressedEventsIsPaintedAsBetter(t *testing.T) { + th := linkTestTheme() + old := state.ArmSummary{Arm: "1.16.0", Runs: 5, IRQSuppressed: 550.6, HasSuppressed: true} + fixed := state.ArmSummary{Arm: "1.17.0", Runs: 5, IRQSuppressed: 83.2, HasSuppressed: true} + got, colour := armCell(th, colNamed("IRQ suppressed"), fixed, old, false) + if colour != th.P.Good { + t.Errorf("%s fewer suppressed events is not painted as better", got) + } +} diff --git a/internal/ui/workbench/sweepresults.go b/internal/ui/workbench/sweepresults.go index d4ae3f7c..70008429 100644 --- a/internal/ui/workbench/sweepresults.go +++ b/internal/ui/workbench/sweepresults.go @@ -102,9 +102,10 @@ func (p *sweepResults) matrix(t *theme.Theme, gtx layout.Context, s *state.Snaps children = append(children, layout.Rigid(layout.Spacer{Height: t.Sp.S}.Layout), layout.Rigid(comp.OneLine(t, t.Sz.Caption, t.P.Faint, - "against "+base.Arm+"; every column but rx and delivered is a cost, "+ + "against "+base.Arm+"; every column but rx, delivered and busy time is a cost, "+ "so red is worse in both directions. A seed that failed is left out "+ - "of its arm's mean, and an arm no seed got through is a dash", false))) + "of its arm's mean, and a dash is an arm that did not run or a chip "+ + "that did not count", false))) return layout.Flex{Axis: layout.Vertical}.Layout(gtx, children...) } @@ -122,12 +123,13 @@ func armCell(t *theme.Theme, c armCol, a, base state.ArmSummary, isBase bool) (s } return c.plain(a), colour } - if a.Runs == 0 { + if a.Runs == 0 || (c.has != nil && !c.has(a)) { return "-", t.P.Faint } // Absolute where there is nothing to take it away from: the baseline's own - // row, a column with no good direction, and a baseline that did not run. - if isBase || c.better == 0 || base.Runs == 0 { + // row, a column with no good direction, and a baseline that did not run or + // whose chips did not count this. + if isBase || c.better == 0 || base.Runs == 0 || (c.has != nil && !c.has(base)) { return fmt.Sprintf("%.0f%s", c.value(a), c.unit), t.P.Ink } ref := c.value(base) @@ -210,6 +212,10 @@ type armCol struct { value func(state.ArmSummary) float64 unit string better int // -1 less is better, +1 more is better, 0 no delta + // has says whether the arm measured this column at all, for the chip's + // counters, which an emulated node or an older chip does not send. Nil + // means every arm that ran measured it. + has func(state.ArmSummary) bool } var armCols = []armCol{ @@ -221,6 +227,18 @@ var armCols = []armCol{ value: func(a state.ArmSummary) float64 { return a.RX }}, {title: "delivered", width: 100, better: +1, value: func(a state.ArmSummary) float64 { return a.Delivered }}, + // What the firmware let its chip report, which is where two builds that + // flood identically differ - so beside the delivery columns rather than at + // the end, where a docked panel cuts the table off. Named as the node + // window's radio rows are, so one word means one thing across the app. + // Busy time has no good direction: more of it is a firmware seeing more of + // the traffic that is really there. + {title: "IRQ suppressed", width: 130, better: -1, + value: func(a state.ArmSummary) float64 { return a.IRQSuppressed }, + has: func(a state.ArmSummary) bool { return a.HasSuppressed }}, + {title: "busy time", width: 100, unit: " s", + value: func(a state.ArmSummary) float64 { return a.BusyMs / 1000 }, + has: func(a state.ArmSummary) bool { return a.HasBusy }}, {title: "redundant", width: 100, better: -1, value: func(a state.ArmSummary) float64 { return a.Redundant }}, {title: "collisions", width: 100, better: -1, diff --git a/internal/ui/workbench/sweepshot_test.go b/internal/ui/workbench/sweepshot_test.go index effaf3d1..0558c64e 100644 --- a/internal/ui/workbench/sweepshot_test.go +++ b/internal/ui/workbench/sweepshot_test.go @@ -66,13 +66,31 @@ func TestDrawTheSweepResults(t *testing.T) { {Arm: "9.9.9", Runs: 0, Failed: 3}, }, }}, + // Three MeshCore releases over five seeds of scotland-strict. The flood + // came back identical in every arm, so the delivery columns cannot tell + // them apart; what each firmware let its chip report is the difference. + {"chip-counters", &state.Snapshot{ + ExperimentWarning: "the seeds disagree by more than the arms do on " + + "receptions (±34.4% within an arm, 0.0% between them) - add repeats", + Experiment: []state.ArmSummary{ + {Arm: "1.16.0", Runs: 5, TX: 229.6, RX: 192, Delivered: 110.8, + Redundant: 81.2, Collided: 573.8, AirtimeMs: 233059, RXSpread: 0.344, + IRQSuppressed: 550.6, HasSuppressed: true, BusyMs: 235893, HasBusy: true}, + {Arm: "1.17.0", Runs: 5, TX: 229.6, RX: 192, Delivered: 110.8, + Redundant: 81.2, Collided: 554, AirtimeMs: 233059, RXSpread: 0.344, + IRQSuppressed: 83.2, HasSuppressed: true, BusyMs: 283620, HasBusy: true}, + {Arm: "1.17.1", Runs: 5, TX: 229.6, RX: 192, Delivered: 110.8, + Redundant: 81.2, Collided: 550.2, AirtimeMs: 233059, RXSpread: 0.344, + IRQSuppressed: 83.2, HasSuppressed: true, BusyMs: 283356, HasBusy: true}, + }, + }}, } for _, c := range cases { c := c t.Run(c.name, func(t *testing.T) { p := &sweepResults{} - img := uitest.RenderWidget(t, 1100, 260, func(gtx layout.Context, th *theme.Theme) layout.Dimensions { + img := uitest.RenderWidget(t, 1340, 260, func(gtx layout.Context, th *theme.Theme) layout.Dimensions { return p.Draw(th, gtx, c.snap) }) out := filepath.Join(dir, "sweep-"+c.name+".png")