experiment: define keeps every setting an arm names

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 |

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

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.
 //

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)
+	}
+}

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]

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