use a Vial keyboard's own effect list, and say it is experimental

Wires the Vial path into `keyboard definitions generate` and marks it
experimental in the help, on stdout, in the README and in AGENTS.md. It
is written from Vial's source and tested against a scripted keyboard;
nobody has run it against a Vial keyboard, and the marker is what says
so rather than leaving an unverified answer looking like a measurement.

A channel now says where its slots came from, "measured by clamping" or
"reported by the firmware", because the two are not the same kind of
answer and a reader of the file has to be able to tell which one they
are looking at. Vial is asked first and only for rgb_matrix, since
VialRGB is an rgb_matrix extension, and a Vial keyboard built without
VIALRGB_ENABLE answers nothing and falls back to the clamp.

Reading Vial's own numbers turned up something that reaches past this
command, so it is in the README rather than in a comment. The IDs a Vial
board sends are VIALRGB_EFFECT_* and the translation to QMK's never
crosses the wire, so the generated slots are the firmware's numbering
and the note does not list QMK's spellings beside them — it says why.
And with VIALRGB_ENABLE set, QMK does not compile
VIA_QMK_RGB_MATRIX_ENABLE, so VIA's id_qmk_rgb_matrix_effect is not
served at all: an effect set by number on such a board addresses Vial's
field. That is stated as unverified rather than worked around.

README.md

@@ -562,6 +562,40 @@ option with an empty name is a slot without a name, so the channel reports
 board was already in. Open the file, write a name into each `options` entry, and
 the channel resolves from then on.
 
+### A Keyboard Running Vial
+
+**Experimental: this path has not been run against a Vial keyboard.** It is
+written from [`vial-kb/vial-qmk`](https://github.com/vial-kb/vial-qmk) and tested
+against a scripted one, which is what makes it reviewable rather than what makes
+it right.
+
+Vial is a QMK fork that keeps VIA's command set unchanged, so a keyboard on Vial
+firmware already answers everything else in this tool. Two things it has that VIA
+does not matter here. It can list the effect IDs it has compiled in, and it can
+answer without the keyboard being written to — where reading the range by clamping
+puts 255 in a register and takes it back out. And `keyboard definitions generate`
+asks it first, for `rgb_matrix` only, because VialRGB is an rgb_matrix extension.
+Every channel it writes says where its slots came from, `measured by clamping` or
+`reported by the firmware`, because the two are not the same kind of answer.
+
+Telling the two firmwares apart is a discriminator rather than a guess. Stock
+QMK's `raw_hid_receive` ends its switch with `default: { *command_id =
+id_unhandled; }`, so a command it does not know comes back as `0xFF`; Vial's
+default case defers to the keyboard instead and answers the keyboard-ID route
+with its protocol version, which is never zero. A keyboard that echoes the
+request, or that implements `raw_hid_receive_kb` for Vial's prefix, is the case
+the source cannot rule out.
+
+**A Vial board numbers its effects in its own namespace, and that is not QMK's.**
+The IDs it sends are `VIALRGB_EFFECT_*`; the translation to QMK's stays inside the
+firmware and never crosses the wire. So the generated slots are the firmware's
+numbers, and the `.spotted.txt` note does **not** list QMK's spellings beside them
+— it says why instead. One consequence reaches past this command: with
+`VIALRGB_ENABLE=yes`, QMK does not compile `VIA_QMK_RGB_MATRIX_ENABLE`, so
+VIA's `id_qmk_rgb_matrix_effect` is not served at all and an effect set by number
+on such a board addresses Vial's field. Reading an effect by number there is
+therefore unverified, and this tool does not claim otherwise.
+
 **It refuses to run when there is something to lose.** A definition already in the
 per-user directory is left alone, because that is the file you would have run this
 command to write. So is a definition the binary carries, and that one matters more

cmd/qmk-rgb-tool/definition_generate.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 
 	"github.com/spf13/cobra"
@@ -13,16 +14,41 @@ import (
 	"netdome.biz/paul/qmk-rgb/internal/via"
 )
 
-// effectRangeProbe is what `keyboard definitions generate` needs from a
-// keyboard: the highest effect ID each channel takes, and a way to close it.
+// effectRangeProbe is what `keyboard definitions generate` needs from a keyboard:
+// the effect IDs each channel takes, a way to close it, and the two Vial calls
+// that may answer the question without writing to it.
+//
 // It is deliberately not a method on rgbProtocol, because that interface is what
 // every other command's test stub implements and a probe is not something
 // brightness and colour have to know about.
 type effectRangeProbe interface {
 	EffectTop(via.Channel) (int, error)
+	VialVersion() (uint32, bool, error)
+	VialEffectIDs() ([]uint16, error)
 	Close() error
 }
 
+// slotsFrom is where a channel's effect slots came from. It is reported per
+// channel because the two answers are not the same kind of thing: one is the
+// firmware listing what it compiled in, the other is the firmware's answer to a
+// value written above the top.
+type slotsFrom string
+
+const (
+	// slotsMeasured means the range was found by writing above the top and
+	// reading back what the firmware clamped to.
+	slotsMeasured slotsFrom = "measured by clamping"
+	// slotsReported means the firmware listed them itself, which is Vial's
+	// vialrgb_get_supported and nothing else.
+	slotsReported slotsFrom = "reported by the firmware"
+)
+
+// experimentalVial marks the Vial path. It is written from Vial's source and
+// tested against a scripted keyboard, and nobody has run it against a Vial
+// keyboard, so every command that uses it says so rather than presenting an
+// unverified answer as a measurement. Remove the word when a real one has.
+const experimentalVial = "experimental"
+
 // generateForce replaces a definition that is already stored. It is the same
 // rule `keyboard fetch` follows, for the same reason: the file may be one the
 // user has written themselves, and this command is the one they would have run
@@ -53,7 +79,8 @@ func NewKeyboardDefinitionsGenerateCmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "generate",
 		Short: "Write a definition for the connected keyboard, with the effect names left to you",
-		Long: "Ask the keyboard how many effect IDs each of its lighting channels takes, then\n" +
+		Long: experimentalVial + ": the Vial path below has not been run against a Vial keyboard.\n\n" +
+			"Ask the keyboard how many effect IDs each of its lighting channels takes, then\n" +
 			"write a definition file with one unnamed option per ID, and a note beside it\n" +
 			"saying which spellings other keyboards' definitions use for the same numbers.\n\n" +
 			"The names are yours to write: open the file and put one in each options entry,\n" +
@@ -62,7 +89,10 @@ func NewKeyboardDefinitionsGenerateCmd() *cobra.Command {
 			"manufacturer's, and a wrong name is worse than none: the tool reports an effect\n" +
 			"as unknown and set by number until you fill it in.\n\n" +
 			"A definition already stored for this keyboard is not replaced, because it is\n" +
-			"probably the one you wrote. Edit it, or pass --force.",
+			"probably the one you wrote. Edit it, or pass --force.\n\n" +
+			"A keyboard running Vial firmware is asked for its rgb_matrix effect IDs instead of\n" +
+			"being written to, which is the better answer, and it numbers them in its own\n" +
+			"namespace rather than QMK's. Every channel says where its slots came from.",
 		Args: cobra.NoArgs,
 		RunE: runKeyboardDefinitionsGenerate,
 	}
@@ -122,18 +152,34 @@ func runKeyboardDefinitionsGenerate(cmd *cobra.Command, args []string) error {
 		ProductID: fmt.Sprintf("0x%04X", target.Device.ProductID),
 	}
 	notes := &strings.Builder{}
-	tops := map[via.Channel]int{}
+
+	// Vial can list the rgb_matrix effect IDs without the keyboard being written
+	// to. It is asked first because its answer is the better one, and only where
+	// the firmware serves it: VIALRGB_ENABLE is a build option, and a Vial
+	// keyboard without it answers nothing.
+	version, isVial, err := probe.VialVersion()
+	if err != nil {
+		return err
+	}
+	if isVial {
+		fmt.Fprintf(cmd.OutOrStdout(), "This keyboard answers as Vial firmware, protocol version %d (%s: not run against real hardware)\n",
+			version, experimentalVial)
+	}
+
+	sources := map[via.Channel]slotsFrom{}
+	slots := map[via.Channel][]uint16{}
 
 	for _, ch := range channels {
-		top, err := probe.EffectTop(ch)
+		ids, from, err := slotsFor(probe, ch, isVial)
 		if err != nil {
 			return err
 		}
-		if menu := scaffoldMenu(ch, top); len(menu.Content) > 0 {
+		if menu := scaffoldMenu(ch, ids); len(menu.Content) > 0 {
 			doc.Menus = append(doc.Menus, menu)
 		}
-		tops[ch] = top
-		writeCandidates(notes, ch, top)
+		sources[ch] = from
+		slots[ch] = ids
+		writeCandidates(notes, ch, ids, from)
 	}
 
 	dir, err := ensureDefinitionsDir()
@@ -170,10 +216,12 @@ func runKeyboardDefinitionsGenerate(cmd *cobra.Command, args []string) error {
 		verb, doc.Name, doc.VendorID, doc.ProductID, describeDataDir(path))
 	fmt.Fprintf(cmd.OutOrStdout(), "Every effect is an unnamed option. Open the file and write a name into each one:\n")
 	for _, ch := range channels {
-		if _, ok := intrgb.EffectValueKey(ch); !ok {
+		ids, ok := slots[ch]
+		if !ok {
 			continue
 		}
-		fmt.Fprintf(cmd.OutOrStdout(), "  %-10s %d slots, IDs 0 to %d\n", ch.Subsystem(), tops[ch]+1, tops[ch])
+		fmt.Fprintf(cmd.OutOrStdout(), "  %-10s %d slots, IDs %s  (%s)\n",
+			ch.Subsystem(), len(ids), describeSlotRange(ids), sources[ch])
 	}
 	fmt.Fprintf(cmd.OutOrStdout(), "\nWhat other keyboards call the same numbers is in %s\n", describeDataDir(notePath))
 	return nil
@@ -219,11 +267,70 @@ type scaffoldEntry struct {
 	Options any    `json:"options,omitempty"`
 }
 
+// slotsFor returns the effect IDs a channel has and where that came from.
+//
+// Vial is asked only for rgb_matrix, because VialRGB is an rgb_matrix extension:
+// there is no Vial list for backlight, rgblight, audio or led_matrix, and those
+// keep the clamping answer. A Vial keyboard without VIALRGB_ENABLE answers
+// nothing, which is an error rather than an empty list, and it falls back to the
+// measurement because a board that cannot list its effects can still be asked.
+func slotsFor(probe effectRangeProbe, ch via.Channel, isVial bool) ([]uint16, slotsFrom, error) {
+	if isVial && ch == via.ChannelRgbMatrix {
+		ids, err := probe.VialEffectIDs()
+		if err == nil && len(ids) > 0 {
+			return ids, slotsReported, nil
+		}
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "vialrgb effect list on %s unavailable (%v), falling back to the clamp\n", ch.Subsystem(), err)
+		}
+	}
+	top, err := probe.EffectTop(ch)
+	if err != nil {
+		return nil, "", err
+	}
+	ids := make([]uint16, 0, top+1)
+	for id := 0; id <= top; id++ {
+		ids = append(ids, uint16(id))
+	}
+	return ids, slotsMeasured, nil
+}
+
+// describeSlotRange writes a list of IDs as a range when it is one, because
+// "0 to 45" says in six characters what a list of forty-six does not.
+func describeSlotRange(ids []uint16) string {
+	if len(ids) == 0 {
+		return "none"
+	}
+	runs := true
+	for i := 1; i < len(ids); i++ {
+		if ids[i] != ids[i-1]+1 {
+			runs = false
+			break
+		}
+	}
+	if runs {
+		return fmt.Sprintf("%d to %d", ids[0], ids[len(ids)-1])
+	}
+	shown := ids
+	if len(shown) > 8 {
+		shown = append(append([]uint16{}, ids[:8]...), 0xFFFF)
+	}
+	parts := make([]string, 0, len(shown))
+	for _, id := range shown {
+		if id == 0xFFFF {
+			parts = append(parts, "...")
+			continue
+		}
+		parts = append(parts, strconv.Itoa(int(id)))
+	}
+	return strings.Join(parts, ", ")
+}
+
 // scaffoldMenu is one lighting channel as a VIA menu: the three controls the
 // value IDs name, with the effect list holding one unnamed option per slot. The
 // name is the channel's own, so the file names it the way the tool does rather
 // than inventing a label — a board the user fills in can rename it.
-func scaffoldMenu(ch via.Channel, top int) scaffoldMenuT {
+func scaffoldMenu(ch via.Channel, ids []uint16) scaffoldMenuT {
 	valueKey, ok := intrgb.EffectValueKey(ch)
 	if !ok {
 		// A channel with no registered value key has no list this tool reads, so
@@ -237,8 +344,8 @@ func scaffoldMenu(ch via.Channel, top int) scaffoldMenuT {
 	// `id_qmk_rgb_matrix_brightness` rather than one the parser will not find.
 	prefix := strings.TrimSuffix(valueKey, "_effect")
 
-	options := make([][]any, 0, top+1)
-	for id := 0; id <= top; id++ {
+	options := make([][]any, 0, len(ids))
+	for _, id := range ids {
 		// The empty name is the point: it states a slot without naming it, and the
 		// parser skips it, so the channel reports no effects until a name is
 		// written here.
@@ -273,15 +380,27 @@ func scaffoldMenu(ch via.Channel, top int) scaffoldMenuT {
 // writeCandidates records what other keyboards' definitions were seen to call
 // each ID, with the counts and the manufacturer, because a name without those is
 // a guess wearing a number.
-func writeCandidates(out *strings.Builder, ch via.Channel, top int) {
+func writeCandidates(out *strings.Builder, ch via.Channel, ids []uint16, from slotsFrom) {
 	valueKey, ok := intrgb.EffectValueKey(ch)
 	if !ok {
 		return
 	}
-	fmt.Fprintf(out, "\n%s (channel %d), IDs 0 to %d\n", ch.Subsystem(), ch, top)
+	fmt.Fprintf(out, "\n%s (channel %d), %d slots, IDs %s  (%s)\n",
+		ch.Subsystem(), ch, len(ids), describeSlotRange(ids), from)
+
+	if from == slotsReported {
+		// The firmware that listed these numbers numbers them in its own
+		// namespace, and the measurement below is QMK's. Saying "here is what
+		// other boards call ID 7" next to a slot that is not QMK's ID 7 is the one
+		// thing this note must not do.
+		fmt.Fprintf(out, "  These IDs are the firmware's own numbering, not QMK's: the list the firmware sends\n")
+		fmt.Fprintf(out, "  is VIALRGB_EFFECT_* and the translation to QMK's stays inside the firmware. The\n")
+		fmt.Fprintf(out, "  spellings other keyboards use are indexed by QMK's numbers, so they are not listed here.\n")
+		return
+	}
 	seen := 0
-	for id := 0; id <= top; id++ {
-		obs, ok := spotted.Candidates(valueKey, id)
+	for _, id := range ids {
+		obs, ok := spotted.Candidates(valueKey, int(id))
 		if !ok {
 			continue
 		}

cmd/qmk-rgb-tool/definition_generate_test.go

@@ -13,9 +13,28 @@ import (
 )
 
 // generateStub is a keyboard that answers an effect range, so the generated
-// scaffold can be checked without hardware.
+// scaffold can be checked without hardware. The Vial fields are what a keyboard
+// running Vial firmware would answer; zero versions read as a keyboard that is
+// not Vial.
 type generateStub struct {
-	tops map[intvia.Channel]int
+	tops    map[intvia.Channel]int
+	vial    bool
+	vialIDs []uint16
+	vialErr error
+}
+
+func (g generateStub) VialVersion() (uint32, bool, error) {
+	if !g.vial {
+		return 0, false, nil
+	}
+	return 6, true, nil
+}
+
+func (g generateStub) VialEffectIDs() ([]uint16, error) {
+	if g.vialErr != nil {
+		return nil, g.vialErr
+	}
+	return g.vialIDs, nil
 }
 
 func (g generateStub) EffectTop(ch intvia.Channel) (int, error) {
@@ -194,8 +213,11 @@ func TestGeneratedNoteCarriesTheSpellingsAndWhoWroteThem(t *testing.T) {
 	}
 
 	note := mustRead(t, strings.TrimSuffix(onlyDefinition(t, dir), ".json")+spottedNoteSuffix)
-	if !strings.Contains(note, "rgb_matrix (channel 3), IDs 0 to 45") {
-		t.Errorf("note = %q, want it to name the channel and the ID range", note)
+	if !strings.Contains(note, "rgb_matrix (channel 3), 46 slots, IDs 0 to 45") {
+		t.Errorf("note = %q, want it to name the channel, the count and the ID range", note)
+	}
+	if !strings.Contains(note, string(slotsMeasured)) {
+		t.Errorf("note = %q, want it to say where the slots came from", note)
 	}
 	// The measurement behind the names: a spelling, how many boards wrote it, and
 	// which manufacturer most of them were.
@@ -277,3 +299,91 @@ func TestGenerateDoesNotShadowABuiltInDefinition(t *testing.T) {
 		t.Errorf("definitions dir = %v, want nothing written", entries)
 	}
 }
+
+// A Vial keyboard lists its own rgb_matrix effect IDs, and the tool must use
+// those rather than writing above the top: the list is the firmware answering
+// without being touched.
+func TestGenerateUsesVialsOwnListWhenTheFirmwareIsVial(t *testing.T) {
+	dir := t.TempDir()
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceGenerateRestore(t))
+
+	original := openTarget
+	t.Cleanup(func() { openTarget = original })
+	openTarget = func(string) (rgbProtocol, targetDeviceData, []intvia.Channel, error) {
+		stub := generateStub{
+			tops:    map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45, intvia.ChannelRgblight: 6},
+			vial:    true,
+			vialIDs: []uint16{0, 1, 2, 40, 45},
+		}
+		return stub, stubTargetData(0x1234, 0x5678), []intvia.Channel{intvia.ChannelRgbMatrix, intvia.ChannelRgblight}, nil
+	}
+
+	cmd := NewKeyboardDefinitionsGenerateCmd()
+	var out, errOut strings.Builder
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("definitions generate error = %v (stderr %q)", err, errOut.String())
+	}
+
+	if !strings.Contains(out.String(), "Vial firmware") {
+		t.Errorf("stdout = %q, want it to say the keyboard is Vial", out.String())
+	}
+	if !strings.Contains(out.String(), experimentalVial) {
+		t.Errorf("stdout = %q, want the Vial path marked %s", out.String(), experimentalVial)
+	}
+	// rgb_matrix comes from Vial and keeps the firmware's own numbering, gaps and
+	// all; rgblight has no Vial list and falls back to the clamp.
+	if !strings.Contains(out.String(), "0, 1, 2, 40, 45") {
+		t.Errorf("stdout = %q, want the rgb_matrix slots in the firmware's numbering", out.String())
+	}
+	if !strings.Contains(out.String(), string(slotsReported)) || !strings.Contains(out.String(), string(slotsMeasured)) {
+		t.Errorf("stdout = %q, want both sources named per channel", out.String())
+	}
+
+	// The note must not put QMK's spellings next to numbers that are not QMK's.
+	note := mustRead(t, strings.TrimSuffix(onlyDefinition(t, dir), ".json")+spottedNoteSuffix)
+	if strings.Contains(note, "rainbow_moving_chevron") {
+		t.Errorf("note = %q, want no QMK spellings beside a Vial numbering", note)
+	}
+	if !strings.Contains(note, "VIALRGB_EFFECT_*") {
+		t.Errorf("note = %q, want it to say the numbering is the firmware's own", note)
+	}
+}
+
+// A Vial keyboard built without VIALRGB_ENABLE answers nothing, and a board that
+// cannot list its effects can still be asked by writing above the top.
+func TestGenerateFallsBackToTheClampWhenVialListsNothing(t *testing.T) {
+	dir := t.TempDir()
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceGenerateRestore(t))
+
+	original := openTarget
+	t.Cleanup(func() { openTarget = original })
+	openTarget = func(string) (rgbProtocol, targetDeviceData, []intvia.Channel, error) {
+		stub := generateStub{
+			tops:    map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45},
+			vial:    true,
+			vialIDs: nil,
+		}
+		return stub, stubTargetData(0x1234, 0x5678), []intvia.Channel{intvia.ChannelRgbMatrix}, nil
+	}
+
+	cmd := NewKeyboardDefinitionsGenerateCmd()
+	var out, errOut strings.Builder
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("definitions generate error = %v", err)
+	}
+	if !strings.Contains(out.String(), string(slotsMeasured)) {
+		t.Errorf("stdout = %q, want the clamp used when Vial lists nothing", out.String())
+	}
+	note := mustRead(t, strings.TrimSuffix(onlyDefinition(t, dir), ".json")+spottedNoteSuffix)
+	if !strings.Contains(note, "rainbow_moving_chevron") {
+		t.Errorf("note = %q, want the QMK spellings again, which is what the clamp gives", note)
+	}
+}