From 00ffe92ed94a91537f14b3005692a4128a75830e Mon Sep 17 00:00:00 2001 From: R0ck Date: Sat, 17 Oct 2026 08:24:42 +0100 Subject: [PATCH 1/2] experiment: define keeps every setting an arm names experiment.define read an arm's label and its two firmware versions and dropped everything else, so an arm defined with cad on ran with cad as the scenario had it, under a label saying otherwise. Arms are now decoded whole, held to the values experiment.vary accepts, and a field an arm does not have is refused with bad_params, leaving the arms already defined in place. --- docs/verb-reference.md | 2 +- internal/app/session/experiment/arm.go | 56 +++++++++++++ .../app/session/experiment/define_test.go | 84 +++++++++++++++++++ internal/app/session/experiment/matrix.go | 16 ++-- .../app/session/experiment/matrix.verbs.json | 2 +- 5 files changed, 147 insertions(+), 13 deletions(-) create mode 100644 internal/app/session/experiment/define_test.go diff --git a/docs/verb-reference.md b/docs/verb-reference.md index 48de1e69..15f0459c 100644 --- a/docs/verb-reference.md +++ b/docs/verb-reference.md @@ -3538,7 +3538,7 @@ State a whole matrix in one call - the arms, the seeds, the senders and the burs | parameter | type | | what | |---|---|---|---| -| `arms` | array | optional | one object per arm, carrying `label`, `repeater_version` and `companion_version`; an arm with no label takes its repeater version as one, and an absent or empty list leaves the arms alone | +| `arms` | array | optional | one object per arm, carrying `label`, `repeater_version` and `companion_version`, and optionally any setting experiment.vary crosses on: `path_hash_mode`, `rep_path_hash`, `loop_detect`, `cad`, `spread_ms` and `set`, a map of firmware settings as the CLI names them; a field an arm does not have, or a value its setting cannot take, is refused and the arms already defined are kept; an arm with no label takes its repeater version as one, and an absent or empty list leaves the arms alone | | `seeds` | array | optional | the seeds each arm is repeated over, as numbers; anything that is not a number is dropped, and an absent or empty list leaves the seeds alone | | `senders` | array | optional | the nodes that originate the burst, by name; unlike the others an empty list is obeyed and clears them, which leaves an experiment experiment.start will refuse | | `run_for_ms` | number | optional | how long each cell runs, in simulated milliseconds; zero or less is ignored and the current length kept | diff --git a/internal/app/session/experiment/arm.go b/internal/app/session/experiment/arm.go index a0d01b6b..8026b208 100644 --- a/internal/app/session/experiment/arm.go +++ b/internal/app/session/experiment/arm.go @@ -8,6 +8,8 @@ package experiment import ( + "bytes" + "encoding/json" "fmt" "strconv" "strings" @@ -95,6 +97,60 @@ func varied(base session.ExpArm, param, v string) (session.ExpArm, string, error param, strings.Join(have, ", ")) } +// armsFrom reads experiment.define's arms, every field an arm has. +// +// It used to read three and drop the rest without a word, so an arm defined +// with cad on ran with cad as the scenario had it, under a label saying +// otherwise, and the row compared two identical configurations. A field it does +// not know is refused for the same reason: a misspelt setting is the same +// silent control arm. +func armsFrom(xs []any) ([]session.ExpArm, error) { + raw, err := json.Marshal(xs) + if err != nil { + return nil, err + } + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.DisallowUnknownFields() + var arms []session.ExpArm + if err := dec.Decode(&arms); err != nil { + return nil, fmt.Errorf("experiment.define cannot read its arms: %w", err) + } + for i := range arms { + if err := checkArm(arms[i]); err != nil { + return nil, fmt.Errorf("arm %d: %w", i+1, err) + } + if arms[i].Label == "" { + arms[i].Label = arms[i].RepeaterVersion + } + } + return arms, nil +} + +// checkArm holds a defined arm to what experiment.vary accepts for the same +// settings, through the same function, so the two verbs cannot disagree about +// what a setting may be. +func checkArm(a session.ExpArm) error { + named := [][2]string{{"loop_detect", a.LoopDetect}, {"cad", a.CAD}} + if a.PathHashMode != nil { + named = append(named, [2]string{"path_hash_mode", strconv.Itoa(*a.PathHashMode)}) + } + if a.RepPathHash != nil { + named = append(named, [2]string{"rep_path_hash", strconv.Itoa(*a.RepPathHash)}) + } + for _, n := range named { + if n[1] == "" { + continue + } + if _, _, err := varied(a, n[0], n[1]); err != nil { + return err + } + } + if a.SpreadMs != nil && *a.SpreadMs < 0 { + return fmt.Errorf("spread_ms cannot be negative; got %d", *a.SpreadMs) + } + return nil +} + // cellText is what one cell floods: its own arm and seed, at a width every // cell of the matrix shares. // diff --git a/internal/app/session/experiment/define_test.go b/internal/app/session/experiment/define_test.go new file mode 100644 index 00000000..9c54bc0c --- /dev/null +++ b/internal/app/session/experiment/define_test.go @@ -0,0 +1,84 @@ +// What experiment.define keeps of an arm, and what it refuses. +package experiment + +import ( + "testing" + + "github.com/MeshBench/meshbench/internal/app/control" + "github.com/MeshBench/meshbench/internal/app/session" +) + +// An arm defined with cad on used to run with cad off: define read the label +// and the two firmware versions and dropped everything else, so the arm was a +// second control arm under a label saying otherwise. +func TestADefinedArmKeepsEverySettingItNames(t *testing.T) { + st, s := aNetwork(t) + _, err := st.Do(t.Context(), "experiment.define", map[string]any{ + "arms": []any{ + map[string]any{"label": "base", "repeater_version": "v1.18.0"}, + map[string]any{ + "label": "listens", "repeater_version": "v1.18.0", "cad": "on", + "loop_detect": "strict", "path_hash_mode": float64(0), + "spread_ms": float64(5000), "set": map[string]any{"agc.reset.interval": "4"}, + }, + }, + }) + if err != nil { + t.Fatal(err) + } + arms := matrixOf(s).Arms + if len(arms) != 2 { + t.Fatalf("defined %d arms, want 2", len(arms)) + } + a := arms[1] + if a.CAD != "on" || a.LoopDetect != "strict" || a.Set["agc.reset.interval"] != "4" { + t.Fatalf("the arm lost its settings: %+v", a) + } + if a.PathHashMode == nil || *a.PathHashMode != 0 { + t.Fatalf("path hash mode 0 did not survive: %v", a.PathHashMode) + } + if a.SpreadMs == nil || *a.SpreadMs != 5000 { + t.Fatalf("spread did not survive: %v", a.SpreadMs) + } + + var p session.Provisioning + a.ApplyOver(&p) + if p.CadMode != "on" { + t.Errorf("provisioning for the arm has cad %q", p.CadMode) + } + if arms[0].CAD != "" { + t.Errorf("the control arm picked up cad %q", arms[0].CAD) + } +} + +// A misspelt field, or a value the setting cannot take, is the same silent +// control arm, so both are refused and the arms already defined are kept. +func TestADefinedArmWithAFieldItCannotUseIsRefused(t *testing.T) { + st, s := aNetwork(t) + if _, err := st.Do(t.Context(), "experiment.define", map[string]any{ + "arms": []any{map[string]any{"label": "kept"}}, + }); err != nil { + t.Fatal(err) + } + for _, arm := range []map[string]any{ + {"label": "x", "cad_mode": "on"}, + {"label": "x", "cad": "maybe"}, + {"label": "x", "path_hash_mode": float64(7)}, + {"label": "x", "spread_ms": float64(-1)}, + } { + _, err := st.Do(t.Context(), "experiment.define", map[string]any{"arms": []any{arm}}) + if err == nil { + t.Errorf("define accepted %v", arm) + continue + } + if code := control.CodeOf(err); code != control.BadParams { + t.Errorf("define refused %v as %q, want %q", arm, code, control.BadParams) + } + } + mentions(t, refuses(t, st, "experiment.define", map[string]any{ + "arms": []any{map[string]any{"cad_mode": "on"}}, + }), "cad_mode") + if arms := matrixOf(s).Arms; len(arms) != 1 || arms[0].Label != "kept" { + t.Errorf("a refused define replaced the arms with %+v", arms) + } +} diff --git a/internal/app/session/experiment/matrix.go b/internal/app/session/experiment/matrix.go index bcbbe476..22222983 100644 --- a/internal/app/session/experiment/matrix.go +++ b/internal/app/session/experiment/matrix.go @@ -23,6 +23,7 @@ import ( "fmt" "sync" + "github.com/MeshBench/meshbench/internal/app/control" "github.com/MeshBench/meshbench/internal/app/session" "github.com/MeshBench/meshbench/internal/app/state" ) @@ -177,18 +178,11 @@ func registerExperiment(st *state.Store, s *session.Sim, e *experiment) { defer func() { e.publish(w) }() if m, ok := p.(map[string]any); ok { if xs, ok := m["arms"].([]any); ok && len(xs) > 0 { - e.Arms = e.Arms[:0] - for _, x := range xs { - am, _ := x.(map[string]any) - arm := session.ExpArm{} - arm.Label, _ = am["label"].(string) - arm.RepeaterVersion, _ = am["repeater_version"].(string) - arm.CompanionVersion, _ = am["companion_version"].(string) - if arm.Label == "" { - arm.Label = arm.RepeaterVersion - } - e.Arms = append(e.Arms, arm) + arms, err := armsFrom(xs) + if err != nil { + return nil, control.WithCode(control.BadParams, err) } + e.Arms = arms } if xs, ok := m["seeds"].([]any); ok && len(xs) > 0 { e.Seeds = e.Seeds[:0] diff --git a/internal/app/session/experiment/matrix.verbs.json b/internal/app/session/experiment/matrix.verbs.json index 4e837c02..0cf833e3 100644 --- a/internal/app/session/experiment/matrix.verbs.json +++ b/internal/app/session/experiment/matrix.verbs.json @@ -41,7 +41,7 @@ { "name": "arms", "type": "array", - "what": "one object per arm, carrying `label`, `repeater_version` and `companion_version`; an arm with no label takes its repeater version as one, and an absent or empty list leaves the arms alone" + "what": "one object per arm, carrying `label`, `repeater_version` and `companion_version`, and optionally any setting experiment.vary crosses on: `path_hash_mode`, `rep_path_hash`, `loop_detect`, `cad`, `spread_ms` and `set`, a map of firmware settings as the CLI names them; a field an arm does not have, or a value its setting cannot take, is refused and the arms already defined are kept; an arm with no label takes its repeater version as one, and an absent or empty list leaves the arms alone" }, { "name": "seeds", From 9221a72418df8d8a375d0885d6a7cf7443abfb40 Mon Sep 17 00:00:00 2001 From: R0ck Date: Sat, 24 Oct 2026 11:26:40 +0100 Subject: [PATCH 2/2] docs: regenerate the verb manifest --- docs/verbs.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/verbs.json b/docs/verbs.json index 38e15670..8b6216fa 100644 --- a/docs/verbs.json +++ b/docs/verbs.json @@ -1369,7 +1369,7 @@ { "name": "arms", "type": "array", - "what": "one object per arm, carrying `label`, `repeater_version` and `companion_version`; an arm with no label takes its repeater version as one, and an absent or empty list leaves the arms alone" + "what": "one object per arm, carrying `label`, `repeater_version` and `companion_version`, and optionally any setting experiment.vary crosses on: `path_hash_mode`, `rep_path_hash`, `loop_detect`, `cad`, `spread_ms` and `set`, a map of firmware settings as the CLI names them; a field an arm does not have, or a value its setting cannot take, is refused and the arms already defined are kept; an arm with no label takes its repeater version as one, and an absent or empty list leaves the arms alone" }, { "name": "seeds",