take an effect ID on the effect command, and drop the mode command

A keyboard holds numbers and an ID is the one thing it always has, so asking for
one should not need a second command. `effect` now takes either: a number is an
effect ID, anything else is a name. A name is never written as digits, so the two
cannot be confused, and the rule is one line rather than a convention to remember.

The gain is a board with no names is no longer a partly supported board. Before,
`effect` refused and the workaround was the other command; now the same command
does it, without a definition file at all, which is what the tests hold. A number
too large to be an ID says so instead of being looked up as a name, and a name
nobody wrote points at the ID form, because "unknown effect: 46" reads like a
missing name rather than the number it is.

`mode` is removed: it was the raw-ID spelling of what `effect` now does, with its
own read-back helper and its own arity entry, and two commands for one job is how
a tool ends up with a user manual nobody reads. The divergence output keeps the
shape each request had — a name is reported as a name, an ID as numbers — because a
board without a definition has no name to put in its place.

Also fixed while in here: a section of README about the definition files had gone
missing under an earlier edit, which is the part a new user reads first, and the
errors that a command fails with named only the workaround and not the fix. All
four now say both: run `definition fetch` for the board's definition, or pass an
ID.

.claude/skills/qmk-rgb/SKILL.md

@@ -102,7 +102,7 @@ reports each channel as written while accepting the tool's spellings too.
 On the Impact 80 the `rgblight` and `audio` channels carry fewer effects (0–6)
 than `rgb_matrix` (0–45, and the board takes a 46th the file does not name). Always check `qmk-rgb-tool effect --list` to see what is
 available per channel, and note that a keyboard with no catalog has no effect
-names at all — `mode <index>` is the way to set one there. The list prints the
+names at all — `effect <index>` is the way to set one there. The list prints the
 file the names came from as its last line.
 
 ## Values the keyboard changes
@@ -126,7 +126,8 @@ user that the tool's own output did not confirm.
 
 ## Compatibility aliases
 
-- `off` → `none`
+- `off` → `none`, but on `logo` and `side` the firmware refuses effect ID 0 and
+  leaves the running effect alone; `disable` is what turns a channel off
 - `breathe` → `breathing`
 - `rainbow` → varies by zone (resolves automatically)
 - `solid` → varies by zone (resolves automatically)

AGENTS.md

@@ -107,7 +107,7 @@ animation, 1 is the slowest movement, anything above 1 becomes 4 — while
 tabulates the per-channel behaviour; the rule that follows from it is the part
 that matters when you write code here:
 
-`brightness`, `speed`, `color`, `effect` and `mode` read every selected zone
+`brightness`, `speed`, `color` and `effect` read every selected zone
 back and
 print what was actually applied; where all zones match they print the plain
 success line, and where any zone differs they print one summary line naming each
@@ -126,7 +126,8 @@ so `effect none` there does nothing to the effect and is reported as the effect
 still running. `disable` is the command that turns a channel off, because it
 also writes brightness 0. Above the top the behaviour is the opposite: the
 backlight channel does not refuse ID 47 or ID 99 but clamps it to the 46 it
-holds, so `mode` there is reported as the index the keyboard ended up with. Read
+holds, so an `effect <index>` there is reported as the index the keyboard ended
+  up with. Read
 the register back; never report the number that was asked for.
 
 There is no compiled-in effect catalog, and that is deliberate. A board's names
@@ -184,9 +185,9 @@ A board's effect names live in a VIA definition file, the same one VIA reads. Th
 tool therefore depends on those files, exactly as VIA does, and takes them in a
 fixed order: `resolveCatalog` in `cmd/qmk-rgb-tool/catalog.go` is the **only**
 place a catalog is looked up, and the order is `--definition`, then a file in
-`definitions/` whose `vendorId`/`productId` match the board, then the compiled-in
-`CatalogFor`. A command that called `CatalogFor` itself would silently ignore a
-file the user had placed, so do not add one.
+`definitions/` whose `vendorId`/`productId` match the board. There is no third
+step: `CatalogFor` is gone, and a command that reached for a catalog itself would
+silently ignore a file the user had placed, so do not add one.
 
 Two things about reading those files are easy to get wrong, and both were
 measured on VIA's own collection of 2029 definitions rather than assumed:
@@ -250,6 +251,21 @@ but keeps its device data in C++ controllers, so it settles VID/PID, not effects
 README.md carries the details; the conclusion for code here is that a catalog is
 transcribed per board and cannot be generated from a common source.
 
+## Output Shape
+
+`keyboard info`, `info`, `list`, `effect --list` and `definition list` print text,
+because that is what a person reads, and JSON only behind the persistent `--json`.
+The field names are the ones the JSON always had, so a consumer that passes the
+flag is unaffected. A command that emits structured data and neither honours
+`--json` nor says why is the bug this rule exists for. Two shapes are deliberately
+not in that list: `effect` with no argument routes to `effect --list`, and
+`definition fetch` writes a file and prints a line about it.
+
+`keyboard info` does not open the board, so it must not report the board's
+channels or an effect list from one: it reports the name and whether this tool has
+effect names for that board, and the channels come from `info` and `effect --list`,
+which open it.
+
 ## Code vs Documentation
 
 All documentation (README.md, comments, AGENTS.md) must stay in sync with the code.

cmd/qmk-rgb-tool/arity_test.go

@@ -57,7 +57,6 @@ func TestOneArgCommandsKeepTheirArity(t *testing.T) {
 		"brightness": NewBrightnessCmd(),
 		"speed":      NewSpeedCmd(),
 		"color":      NewColorCmd(),
-		"mode":       NewModeCmd(),
 	}
 
 	for name, cmd := range cmds {

cmd/qmk-rgb-tool/channels_test.go

@@ -298,8 +298,8 @@ func TestLoadSaysSoWhenTheBoardHasNoCatalog(t *testing.T) {
 	if err := cmd.Execute(); err != nil {
 		t.Fatalf("load returned error: %v", err)
 	}
-	if !strings.Contains(stderr.String(), "catalog") {
-		t.Errorf("stderr = %q, want the missing catalog named", stderr.String())
+	if !strings.Contains(stderr.String(), "no effect names") {
+		t.Errorf("stderr = %q, want the missing names named", stderr.String())
 	}
 	if strings.Contains(stderr.String(), `effect "breathing" not found`) {
 		t.Errorf("stderr = %q, must not blame the effect name for a missing catalog", stderr.String())
@@ -359,8 +359,8 @@ func TestSaveWarnsThatEffectNamesCannotBeRecorded(t *testing.T) {
 	if !strings.Contains(stderr.String(), "unknown") {
 		t.Errorf("stderr = %q, want the effect name it cannot record named", stderr.String())
 	}
-	if !strings.Contains(stderr.String(), "catalog") {
-		t.Errorf("stderr = %q, want the missing catalog named", stderr.String())
+	if !strings.Contains(stderr.String(), "no effect names") {
+		t.Errorf("stderr = %q, want the missing names named", stderr.String())
 	}
 }
 

cmd/qmk-rgb-tool/commands_test.go

@@ -166,20 +166,6 @@ func TestSelectedChannelOperations(t *testing.T) {
 				{channel: 4, param: 1, value: 160},
 			},
 		},
-		{
-			name:      "mode",
-			zones:     defaultZones,
-			sideZones: sideZones,
-			run: func(protocol zoneProtocol, zones []via.Channel) error {
-				return setModeOnChannels(protocol, zones, 17)
-			},
-			want: []commandReport{
-				{channel: 2, param: 2, value: 17},
-				{channel: 3, param: 2, value: 17},
-				{channel: 4, param: 2, value: 17},
-			},
-			wantSide: []commandReport{{channel: 4, param: 2, value: 17}},
-		},
 	}
 
 	for _, tc := range operations {

cmd/qmk-rgb-tool/effect.go

@@ -1,7 +1,9 @@
 package main
 
 import (
+	"errors"
 	"fmt"
+	"strconv"
 	"strings"
 
 	"github.com/spf13/cobra"
@@ -24,7 +26,8 @@ func NewEffectCmd() *cobra.Command {
 		Short: "Set or list RGB effects",
 		Long: "Set the RGB lighting effect on the connected keyboard. Without an argument, lists all effects.\n" +
 			"Effect names come from a per-board catalog; a keyboard without one has no names\n" +
-			"and is driven with `mode <index>` instead.",
+			"and is driven with `effect <index>` instead.\n" +
+			"A number is an effect ID; anything else is a name.",
 		Args: cobra.MaximumNArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if listEffects || len(args) == 0 {
@@ -91,6 +94,30 @@ func printEffectListJSON(cmd *cobra.Command, catalog *intrgb.Catalog, channels [
 	return encodeJSON(cmd.OutOrStdout(), list)
 }
 
+// effectArgument is what was asked for: a name to resolve against the board's
+// definition, or a raw effect ID to write.
+type effectArgument struct {
+	Name string
+	ID   uint8
+	IsID bool
+}
+
+// parseEffectArgument reads the argument of the effect command. A number is an
+// effect ID and nothing else: a name is never written as digits, so the two
+// cannot be confused, and the ID is what a board without a definition needs. A
+// number too large to be one is reported as such rather than looked up as a name.
+func parseEffectArgument(arg string) (effectArgument, error) {
+	value, err := strconv.ParseUint(arg, 10, 8)
+	if err == nil {
+		return effectArgument{ID: uint8(value), IsID: true}, nil
+	}
+	var numErr *strconv.NumError
+	if errors.As(err, &numErr) && errors.Is(numErr.Err, strconv.ErrRange) {
+		return effectArgument{}, fmt.Errorf("effect ID %s is out of range: an effect ID is 0-255", arg)
+	}
+	return effectArgument{Name: arg}, nil
+}
+
 func runEffectSet(cmd *cobra.Command, args []string) error {
 	proto, target, channels, err := openTarget()
 	if err != nil {
@@ -98,11 +125,19 @@ func runEffectSet(cmd *cobra.Command, args []string) error {
 	}
 	defer proto.Close()
 
+	want, err := parseEffectArgument(args[0])
+	if err != nil {
+		return err
+	}
+	if want.IsID {
+		return setEffectID(cmd, proto, target, channels, want.ID)
+	}
+
 	catalog, _, err := resolveCatalog(target)
 	if err != nil {
 		return err
 	}
-	targets, skipped, err := resolveEffectTargets(catalog, args[0], channels)
+	targets, skipped, err := resolveEffectTargets(catalog, want.Name, channels)
 	if err != nil {
 		return err
 	}
@@ -119,10 +154,35 @@ func runEffectSet(cmd *cobra.Command, args []string) error {
 	// contradiction this read-back exists to prevent: on the logo and side
 	// channels an effect of ID 0 leaves the previous effect running.
 	if anyEffectMismatch(results) {
-		fmt.Fprintln(cmd.OutOrStdout(), formatEffectResults(args[0], results))
+		fmt.Fprintln(cmd.OutOrStdout(), formatEffectResults(want.Name, results))
+		return nil
+	}
+
+	fmt.Fprintf(cmd.OutOrStdout(), "Effect set to %q\n", want.Name)
+	return nil
+}
+
+// setEffectID writes a raw effect ID to every selected channel. It needs no
+// catalog, which is the point: an ID is the one thing a board always has, and
+// which ID is which effect is a question the board does not answer.
+func setEffectID(cmd *cobra.Command, proto rgbProtocol, target targetDeviceData, channels []via.Channel, id uint8) error {
+	targets := make([]intrgb.EffectTarget, 0, len(channels))
+	for _, ch := range channels {
+		targets = append(targets, intrgb.EffectTarget{Channel: ch, ID: id})
+	}
+
+	results, err := setEffectVerified(proto, targets, target.Display, nil)
+	if err != nil {
+		return err
+	}
+
+	// The numbers are what the request was, so the numbers are what the summary
+	// states: a name would be one the board may not even have.
+	if anyEffectMismatch(results) {
+		fmt.Fprintln(cmd.OutOrStdout(), formatEffectIDResults(results))
 		return nil
 	}
 
-	fmt.Fprintf(cmd.OutOrStdout(), "Effect set to %q\n", args[0])
+	fmt.Fprintf(cmd.OutOrStdout(), "Effect set to index %d\n", id)
 	return nil
 }

cmd/qmk-rgb-tool/effect_id_test.go

@@ -0,0 +1,127 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// A number is an effect ID, not a name. That is the whole rule: a user typing
+// digits means the number, and a name is never written as digits, so the two
+// cannot be confused.
+func TestEffectTakesAnIDAsWellAsAName(t *testing.T) {
+	tests := []struct {
+		name       string
+		argument   string
+		wantValue  uint8
+		wantOutput string
+	}{
+		{"an ID the definition names", "13", 13, "Effect set to index 13"},
+		{"the ID past the end of the definition", "46", 46, "Effect set to index 46"},
+		{"ID 0, which two channels refuse", "0", 0, "Effect set to index 0"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			proto := &fakeZoneProtocol{failAt: -1}
+			t.Cleanup(vendoredDefinitions(t))
+
+			stdout, _, err := executeEffectCommand(t, proto, "Backlight", tt.argument)
+			if err != nil {
+				t.Fatalf("effect %s: %v", tt.argument, err)
+			}
+			if !strings.Contains(stdout, tt.wantOutput) {
+				t.Errorf("stdout = %q, want it to contain %q", stdout, tt.wantOutput)
+			}
+			if len(proto.reports) != 1 {
+				t.Fatalf("reports = %v, want one write", proto.reports)
+			}
+			if got := proto.reports[0]; got.value != tt.wantValue || got.param != 0x02 {
+				t.Errorf("wrote value %d param 0x%02x, want %d param 0x02", got.value, got.param, tt.wantValue)
+			}
+		})
+	}
+}
+
+// The point of taking IDs on the same command: a board with no names is still
+// fully drivable, because an ID is the only thing it ever needed.
+func TestEffectTakesAnIDOnAKeyboardWithNoNames(t *testing.T) {
+	proto := &fakeZoneProtocol{failAt: -1}
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, t.TempDir()))
+
+	stdout, _, err := executeEffectCommand(t, proto, "Backlight", "46")
+	if err != nil {
+		t.Fatalf("effect 46 without a definition: %v", err)
+	}
+	if len(proto.reports) != 1 || proto.reports[0].value != 46 {
+		t.Errorf("reports = %v, want one write of 46", proto.reports)
+	}
+	if !strings.Contains(stdout, "Effect set to index 46") {
+		t.Errorf("stdout = %q, want the ID form reported", stdout)
+	}
+}
+
+// A number that cannot be an effect ID is a number that cannot be used, and the
+// message says so rather than looking for a name nobody wrote.
+func TestEffectSaysWhenAnIDIsOutOfRange(t *testing.T) {
+	proto := &fakeZoneProtocol{failAt: -1}
+	t.Cleanup(vendoredDefinitions(t))
+
+	_, _, err := executeEffectCommand(t, proto, "Backlight", "300")
+	if err == nil {
+		t.Fatal("effect 300 = nil error, want a range error")
+	}
+	if !strings.Contains(err.Error(), "0-255") {
+		t.Errorf("error = %q, want it to name the range", err)
+	}
+	if len(proto.reports) != 0 {
+		t.Errorf("reports = %v, want nothing written for a value that cannot be one", proto.reports)
+	}
+}
+
+// A name still resolves by name, and still goes through the alias table.
+func TestEffectStillTakesAName(t *testing.T) {
+	proto := &fakeZoneProtocol{failAt: -1}
+	t.Cleanup(vendoredDefinitions(t))
+
+	stdout, _, err := executeEffectCommand(t, proto, "Backlight", "rainbow_moving_chevron")
+	if err != nil {
+		t.Fatalf("effect by name: %v", err)
+	}
+	if len(proto.reports) != 1 || proto.reports[0].value != 17 {
+		t.Errorf("reports = %v, want one write of 17", proto.reports)
+	}
+	if stdout != "Effect set to \"rainbow_moving_chevron\"\n" {
+		t.Errorf("stdout = %q, want the plain success line", stdout)
+	}
+}
+
+// An unknown name is still an unknown name, and the message points at the ID form
+// rather than at a command that no longer exists.
+func TestUnknownNameErrorPointsAtTheIDForm(t *testing.T) {
+	proto := &fakeZoneProtocol{failAt: -1}
+	t.Cleanup(vendoredDefinitions(t))
+
+	_, _, err := executeEffectCommand(t, proto, "Backlight", "nonsense")
+	if err == nil {
+		t.Fatal("effect nonsense = nil error, want one")
+	}
+	if strings.Contains(err.Error(), "`mode") {
+		t.Errorf("error = %q, want no reference to a removed command", err)
+	}
+	if !strings.Contains(err.Error(), "effect <index>") {
+		t.Errorf("error = %q, want it to point at the ID form", err)
+	}
+}
+
+// The mode command is gone: an ID goes through the command that also takes names.
+func TestModeCommandIsGone(t *testing.T) {
+	root := newRootCommand()
+	registerCommands(root)
+	registerFlagCompletions(root)
+
+	for _, c := range root.Commands() {
+		if c.Name() == "mode" {
+			t.Fatal("the mode command is still registered; effect takes an ID now")
+		}
+	}
+}

cmd/qmk-rgb-tool/enable.go

@@ -24,7 +24,8 @@ func NewEnableCmd() *cobra.Command {
 			}
 			for _, ch := range channels {
 				if _, ok := catalog.DefaultEffect(ch); !ok {
-					return fmt.Errorf("this keyboard has no effect catalog, so `enable` cannot choose an effect; set one with `mode <index>`")
+					return fmt.Errorf("this keyboard has no effect names, so `enable` cannot choose an effect: " +
+						"run `definition fetch` for its VIA definition, or set one with `effect <index>`")
 				}
 			}
 

cmd/qmk-rgb-tool/output.go

@@ -23,7 +23,7 @@ func addJSONFlag(cmd *cobra.Command) {
 
 // printEffectListText writes one effect per line, its ID in brackets, under a
 // heading per channel. The ID is in the line because it is what a user needs to
-// check a name against the register, and what `mode <index>` takes.
+// check a name against the register, and what `effect <index>` takes.
 func printEffectListText(out io.Writer, catalog *intrgb.Catalog, channels []via.Channel, display map[uint16]string, source string) error {
 	for _, ch := range channels {
 		effects := catalog.Effects(ch)

cmd/qmk-rgb-tool/rgb.go

@@ -246,12 +246,19 @@ func setSpeedVerified(proto rgbProtocol, channels []via.Channel, display map[uin
 	return setValueVerified(proto, channels, display, uint8(intrgb.Speed), value)
 }
 
-// setModeVerified writes a raw effect index and reads every channel back. A
-// board that does not implement the index clamps it to the highest ID it holds
-// rather than refusing it, so the index the keyboard ended up with is the only
-// honest thing to print.
-func setModeVerified(proto rgbProtocol, channels []via.Channel, display map[uint16]string, value uint8) ([]zoneResult, error) {
-	return setValueVerified(proto, channels, display, uint8(intrgb.EffectID), value)
+// formatEffectIDResults renders the same line as formatEffectResults, with the
+// numbers: the request was a number, and a board without a definition has no name
+// to put in its place.
+func formatEffectIDResults(results []effectResult) string {
+	var b strings.Builder
+	b.WriteString("Effect")
+	for _, r := range results {
+		fmt.Fprintf(&b, " %s %d", r.Name, r.AppliedID)
+	}
+	if len(results) > 0 {
+		fmt.Fprintf(&b, " (requested %d)", results[0].RequestedID)
+	}
+	return b.String()
 }
 
 // colorResult records the hue and saturation one channel holds after a color was
@@ -336,10 +343,6 @@ func enableLightingOnChannels(proto zoneProtocol, channels []via.Channel, catalo
 	})
 }
 
-func setModeOnChannels(proto zoneProtocol, channels []via.Channel, value uint8) error {
-	return setValueOnChannels(proto, channels, uint8(intrgb.EffectID), value)
-}
-
 // openTarget opens the keyboard and resolves the requested channels against the
 // ones it actually has. It is a seam because a command needs all three: the
 // handle it writes to, the names it reports with, and the channels it may touch.

internal/rgb/catalog.go

@@ -202,7 +202,11 @@ func (c *Catalog) DefaultEffect(ch via.Channel) (uint8, bool) {
 // request for it.
 func ResolveEffect(catalog *Catalog, name string, channels []via.Channel, explicit bool) ([]EffectTarget, []string, error) {
 	if catalog == nil {
-		return nil, nil, fmt.Errorf("no effect catalog for this keyboard; set an effect by number with `mode <index>`")
+		// Both ways out belong in the message: the board is still drivable by
+		// number, and the names are one command away. A user who reads only the
+		// error should not have to find either out elsewhere.
+		return nil, nil, fmt.Errorf("no effect names for this keyboard: run `definition fetch` for its VIA definition, " +
+			"or set an effect by number with `effect <index>`")
 	}
 
 	// The compatibility spelling every catalog shares, so a caller cannot resolve
@@ -222,7 +226,9 @@ func ResolveEffect(catalog *Catalog, name string, channels []via.Channel, explic
 		}
 	}
 	if !knownAnywhere {
-		return nil, nil, fmt.Errorf("unknown effect: %s", name)
+		// A name nobody wrote is either a typo or a number that belongs in the
+		// other form, and pointing at it saves the user from guessing.
+		return nil, nil, fmt.Errorf("unknown effect: %s (an effect ID from 0 to 255 is `effect <index>`)", name)
 	}
 
 	var targets []EffectTarget

internal/rgb/catalog_test.go

@@ -122,8 +122,8 @@ func TestResolveEffectWithoutACatalogRefuses(t *testing.T) {
 	if err == nil {
 		t.Fatal("ResolveEffect(nil, ...) expected an error, got nil")
 	}
-	if !strings.Contains(err.Error(), "mode") {
-		t.Errorf("error = %q, want it to point at mode", err)
+	if !strings.Contains(err.Error(), "effect <index>") {
+		t.Errorf("error = %q, want it to point at the ID form", err)
 	}
 }
 

internal/rgb/definition_test.go

@@ -3,6 +3,7 @@ package rgb
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"netdome.biz/paul/qmk-rgb/internal/via"
@@ -363,3 +364,18 @@ func TestDefaultEffectIsAbsentForAnEmptyChannel(t *testing.T) {
 		t.Errorf("DefaultEffect() = %d, true, want none: the channel has only the off entry", got)
 	}
 }
+
+// The message a command fails with is the only documentation a user meets before
+// they read anything. It has to name the way out, and there are two: fetch the
+// board's definition, or set an effect by number.
+func TestMissingCatalogErrorNamesBothWaysOut(t *testing.T) {
+	_, _, err := ResolveEffect(nil, "wave", []via.Channel{via.ChannelRgbMatrix}, false)
+	if err == nil {
+		t.Fatal("ResolveEffect(nil catalog) = nil error, want one")
+	}
+	for _, want := range []string{"`definition fetch`", "`effect <index>`"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error = %q, want it to mention %q", err, want)
+		}
+	}
+}