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/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", 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",