take hue, saturation and value on the color command

color accepts hsv:<h>,<s>,<v> in the keyboard's own 0-255 units next to
the existing hex notations, and reads every notation back so the line
reports what the keyboard holds. The keyboard has no value register for a
color, so the value of an hsv: notation is written to the brightness of
the same zones, where the per-channel firmware transform applies and is
reported rather than assumed.

README.md

@@ -68,6 +68,7 @@ re-run the command whenever commands or flags change.
 ./qmk-rgb-tool brightness 160
 ./qmk-rgb-tool speed 2
 ./qmk-rgb-tool color 00ff00
+./qmk-rgb-tool color hsv:85,255,255
 # Raw mode IDs are zone-specific; ID 17 is Backlight-only
 ./qmk-rgb-tool mode 17 --zone backlight
 ./qmk-rgb-tool enable
@@ -116,6 +117,40 @@ HID path, which the operating system reassigns on reboot, and most keyboards
 report no serial number. Re-run `keyboard info` after reconnecting a keyboard
 rather than storing the number.
 
+## Color Notations
+
+`color` accepts three notations for one operation:
+
+```bash
+qmk-rgb-tool color ff0000            # six digit hex
+qmk-rgb-tool color rgb:ff0000        # the same, written out
+qmk-rgb-tool color hsv:0,255,255     # hue, saturation, value, each 0-255
+```
+
+`hsv:` takes the numbers the keyboard itself computes in, so a color can be
+addressed directly instead of being derived from hex. The hex notations say
+nothing about brightness and leave it alone.
+
+The keyboard stores hue and saturation only; it has no value register, so the
+value component of a hex color has nowhere to go and is dropped. The value of an
+`hsv:` notation therefore goes to the **brightness** of the same zones, where the
+per-channel firmware transform applies — `hsv:0,255,200` leaves `logo` and `side`
+at 160 while `backlight` reaches 255. See "Brightness and Speed Are Not Applied
+Verbatim" for the table.
+
+Every notation is read back, so the line reports what the keyboard holds:
+
+```console
+$ qmk-rgb-tool color 00ff00 --zone backlight
+Color set to hue 85 sat 255
+$ qmk-rgb-tool color hsv:0,255,200
+Color logo hue 0 sat 255 brightness 160 backlight hue 0 sat 255 brightness 255 (requested hue 0 sat 255 brightness 200)
+```
+
+Hex is the wider of the two: at full brightness it reaches 195,841 colors, a pair
+of 8-bit hue and saturation values 56,654. They cover the same colors, and `hsv:`
+trades some of that range for direct addressing.
+
 ## Brightness and Speed Are Not Applied Verbatim
 
 The keyboard's firmware transforms these values per channel, so the accepted
@@ -327,6 +362,8 @@ Two models are listed in `keyboards.json`:
 | `qmk-rgb-tool brightness <val>`       | Set brightness (0–255) on selected zones, verified by read-back |
 | `qmk-rgb-tool speed <val>`            | Set effect speed (0–255) on selected zones, verified by read-back |
 | `qmk-rgb-tool color <hex>`            | Set color (e.g. `ff0000`) on selected zones |
+| `qmk-rgb-tool color rgb:<hex>`        | The same hex color, written out |
+| `qmk-rgb-tool color hsv:<h>,<s>,<v>`  | Set hue and saturation (0–255) and write `v` to the brightness of the same zones |
 | `qmk-rgb-tool mode <index>`           | Set a raw zone-specific effect ID         |
 | `qmk-rgb-tool --zone <zone> ...`      | Target `logo`, `backlight`, or `side`     |
 | `qmk-rgb-tool --device <n> ...`      | Target keyboard by number (see `keyboard info`) |

cmd/qmk-rgb-tool/color.go

@@ -2,45 +2,102 @@ package main
 
 import (
 	"fmt"
-	"os"
+	"strings"
 
 	"github.com/spf13/cobra"
-	"netdome.biz/paul/qmk-rgb/internal/rgb"
+	intrgb "netdome.biz/paul/qmk-rgb/internal/rgb"
 )
 
 func NewColorCmd() *cobra.Command {
 	return &cobra.Command{
-		Use:   "color <hex>",
+		Use:   "color <hex>|rgb:<hex>|hsv:<h>,<s>,<v>",
 		Short: "Set RGB color",
-		Long:  "Set the RGB color using a 6-digit hex code (e.g. \"ff0000\" for red).",
-		Args:  cobra.ExactArgs(1),
-		Run: func(cmd *cobra.Command, args []string) {
-			c, err := rgb.ParseHexColor(args[0])
+		Long: "Set the color of the selected zones from one of three notations:\n" +
+			"  ff0000          six digit hex, hue and saturation only\n" +
+			"  rgb:ff0000      the same, written out\n" +
+			"  hsv:0,255,255   hue, saturation and value, each 0-255\n" +
+			"\n" +
+			"The hex notations say nothing about brightness, so they leave it alone.\n" +
+			"The keyboard has no value register for a color: the value of an hsv:\n" +
+			"notation is written to the brightness of the same zones, and the\n" +
+			"brightness the keyboard ended up holding is reported.\n" +
+			"\n" +
+			"Every notation is read back, so a color the keyboard rounded or refused\n" +
+			"is reported instead of being claimed as set.",
+		Args: cobra.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			spec, err := intrgb.ParseColorSpec(args[0])
 			if err != nil {
-				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-				os.Exit(1)
+				return err
 			}
 
 			zones, err := selectedZones()
 			if err != nil {
-				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-				os.Exit(1)
+				return err
 			}
 
-			proto, err := OpenDevice()
+			proto, err := openRGBProtocol()
 			if err != nil {
-				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-				os.Exit(1)
+				return err
 			}
 			defer proto.Close()
 
-			h, s, _ := c.HSV()
-			if err := setColorOnZones(proto, zones, h, s); err != nil {
-				fmt.Fprintf(os.Stderr, "Error setting color: %v\n", err)
-				os.Exit(1)
+			colors, err := setColorVerified(proto, zones, spec.H, spec.S)
+			if err != nil {
+				return fmt.Errorf("set color: %w", err)
+			}
+
+			if !spec.HasValue() {
+				printColorResult(cmd, colors, nil)
+				return nil
+			}
+
+			brightness, err := setBrightnessVerified(proto, zones, spec.V)
+			if err != nil {
+				return fmt.Errorf("set brightness from %s: %w", args[0], err)
 			}
 
-			fmt.Printf("Color set to %s\n", args[0])
+			printColorResult(cmd, colors, brightness)
+			return nil
 		},
 	}
 }
+
+// printColorResult states the color every zone actually holds. The brightness
+// results are nil for a notation that did not write one, and then no brightness
+// is mentioned: the command must not imply it set something it left alone.
+func printColorResult(cmd *cobra.Command, colors []colorResult, brightness []zoneResult) {
+	request := ""
+	if len(colors) > 0 {
+		request = fmt.Sprintf("hue %d sat %d", colors[0].RequestedHue, colors[0].RequestedSaturation)
+		if len(brightness) > 0 {
+			request += fmt.Sprintf(" brightness %d", brightness[0].Requested)
+		}
+	}
+
+	if !anyColorMismatch(colors) && !anyMismatch(brightness) {
+		fmt.Fprintf(cmd.OutOrStdout(), "Color set to %s\n", request)
+		return
+	}
+
+	var b strings.Builder
+	b.WriteString("Color")
+	for i, c := range colors {
+		fmt.Fprintf(&b, " %s hue %d sat %d", c.Zone, c.Hue, c.Saturation)
+		if i < len(brightness) {
+			fmt.Fprintf(&b, " brightness %d", brightness[i].Applied)
+		}
+	}
+	fmt.Fprintf(&b, " (requested %s)", request)
+	fmt.Fprintln(cmd.OutOrStdout(), b.String())
+}
+
+// anyColorMismatch reports whether any zone stored another color.
+func anyColorMismatch(results []colorResult) bool {
+	for _, r := range results {
+		if r.Mismatch() {
+			return true
+		}
+	}
+	return false
+}

cmd/qmk-rgb-tool/color_verify_test.go

@@ -0,0 +1,285 @@
+package main
+
+import (
+	"bytes"
+	"errors"
+	"strings"
+	"testing"
+
+	intrgb "netdome.biz/paul/qmk-rgb/internal/rgb"
+	"netdome.biz/paul/qmk-rgb/internal/via"
+)
+
+// colorProtocol reports the two byte color the keyboard holds for the color
+// value ID, and the single byte brightness for the brightness value ID, so a
+// test can describe a keyboard that applied something other than the request.
+type colorProtocol struct {
+	colorWrites      []colorWrite
+	brightnessWrites []uint8
+	appliedColor     map[via.LEDType][2]uint8
+	appliedBright    map[via.LEDType]uint8
+	shortColorRead   bool
+	setColorErr      error
+	getErr           error
+}
+
+type colorWrite struct {
+	channel via.LEDType
+	hue     uint8
+	sat     uint8
+}
+
+func (p *colorProtocol) SetValue(_ via.LEDType, param, value uint8) error {
+	if param == uint8(intrgb.Brightness) {
+		p.brightnessWrites = append(p.brightnessWrites, value)
+	}
+	return nil
+}
+
+func (p *colorProtocol) SetColor(channel via.LEDType, hue, sat uint8) error {
+	if p.setColorErr != nil {
+		return p.setColorErr
+	}
+	p.colorWrites = append(p.colorWrites, colorWrite{channel: channel, hue: hue, sat: sat})
+	return nil
+}
+
+func (p *colorProtocol) GetValue(channel via.LEDType, param uint8) ([]byte, error) {
+	if p.getErr != nil {
+		return nil, p.getErr
+	}
+	if param == uint8(intrgb.ColorValue) {
+		if p.shortColorRead {
+			return []byte{0}, nil
+		}
+		c := p.appliedColor[channel]
+		return []byte{c[0], c[1]}, nil
+	}
+	return []byte{p.appliedBright[channel]}, nil
+}
+
+func (p *colorProtocol) Close() error { return nil }
+
+// A color is two bytes on the wire, so the read-back has to look at both. A
+// one byte answer would leave the saturation unverified.
+func TestSetColorVerifiedReadsBackHueAndSaturation(t *testing.T) {
+	proto := &colorProtocol{
+		appliedColor: map[via.LEDType][2]uint8{via.RGBLight: {99, 255}},
+	}
+
+	results, err := setColorVerified(proto, []intrgb.Zone{intrgb.ZoneLogo}, 85, 255)
+	if err != nil {
+		t.Fatalf("setColorVerified() error = %v", err)
+	}
+	if len(results) != 1 {
+		t.Fatalf("results = %d entries, want 1", len(results))
+	}
+
+	got := results[0]
+	if got.Zone != intrgb.ZoneLogo {
+		t.Errorf("results[0].Zone = %q, want %q", got.Zone, intrgb.ZoneLogo)
+	}
+	if got.RequestedHue != 85 || got.RequestedSaturation != 255 {
+		t.Errorf("requested = hue %d sat %d, want hue 85 sat 255", got.RequestedHue, got.RequestedSaturation)
+	}
+	if got.Hue != 99 || got.Saturation != 255 {
+		t.Errorf("applied = hue %d sat %d, want hue 99 sat 255", got.Hue, got.Saturation)
+	}
+	if !got.Mismatch() {
+		t.Error("Mismatch() = false, want true when the keyboard stored another hue")
+	}
+}
+
+func TestSetColorVerifiedReportsNoMismatchWhenKeyboardAppliesTheRequest(t *testing.T) {
+	proto := &colorProtocol{
+		appliedColor: map[via.LEDType][2]uint8{via.RGBLight: {0, 255}},
+	}
+
+	results, err := setColorVerified(proto, []intrgb.Zone{intrgb.ZoneLogo}, 0, 255)
+	if err != nil {
+		t.Fatalf("setColorVerified() error = %v", err)
+	}
+	if results[0].Mismatch() {
+		t.Error("Mismatch() = true, want false when the keyboard applied the request")
+	}
+}
+
+// A truncated answer means the keyboard did not report both components, so the
+// command must not claim it knows the applied color.
+func TestSetColorVerifiedRejectsATruncatedColorRead(t *testing.T) {
+	proto := &colorProtocol{shortColorRead: true}
+
+	_, err := setColorVerified(proto, []intrgb.Zone{intrgb.ZoneLogo}, 0, 255)
+	if err == nil {
+		t.Fatal("setColorVerified() expected an error, got nil")
+	}
+	if !strings.Contains(err.Error(), "logo") {
+		t.Errorf("error = %q, want it to name the zone whose read back was unusable", err)
+	}
+}
+
+func TestSetColorVerifiedPropagatesWriteErrors(t *testing.T) {
+	proto := &colorProtocol{setColorErr: errors.New("write refused")}
+
+	_, err := setColorVerified(proto, []intrgb.Zone{intrgb.ZoneLogo}, 0, 255)
+	if err == nil {
+		t.Fatal("setColorVerified() expected an error, got nil")
+	}
+	if !strings.Contains(err.Error(), "write refused") {
+		t.Errorf("error = %q, want the keyboard's own reason", err)
+	}
+}
+
+func runColor(t *testing.T, proto *colorProtocol, zoneFlag string, args ...string) (stdout, stderr string, err error) {
+	t.Helper()
+
+	originalOpen := openRGBProtocol
+	originalZone := targetZone
+	t.Cleanup(func() {
+		openRGBProtocol = originalOpen
+		targetZone = originalZone
+	})
+	openRGBProtocol = func() (rgbProtocol, error) { return proto, nil }
+	targetZone = zoneFlag
+
+	var out, errOut bytes.Buffer
+	cmd := NewColorCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+	cmd.SetArgs(args)
+
+	err = cmd.Execute()
+	return out.String(), errOut.String(), err
+}
+
+func allZonesAppliedColor(hue, sat uint8) map[via.LEDType][2]uint8 {
+	return map[via.LEDType][2]uint8{
+		via.RGBLight:  {hue, sat},
+		via.RGBMatrix: {hue, sat},
+		via.SideLight: {hue, sat},
+	}
+}
+
+func TestColorReportsTheAppliedHueAndSaturation(t *testing.T) {
+	// 00ff00 is hue 85 at full saturation.
+	proto := &colorProtocol{appliedColor: allZonesAppliedColor(85, 255)}
+
+	stdout, stderr, err := runColor(t, proto, "logo", "00ff00")
+	if err != nil {
+		t.Fatalf("color returned error: %v", err)
+	}
+	if strings.TrimSpace(stdout) != "Color set to hue 85 sat 255" {
+		t.Errorf("stdout = %q, want the exact success message", stdout)
+	}
+	if stderr != "" {
+		t.Errorf("stderr = %q, want nothing on stderr when the request was met", stderr)
+	}
+}
+
+// The hex notations carry no brightness, so the command must not write one and
+// must not report one.
+func TestColorHexNotationLeavesBrightnessAlone(t *testing.T) {
+	proto := &colorProtocol{appliedColor: allZonesAppliedColor(0, 255)}
+
+	stdout, _, err := runColor(t, proto, "", "rgb:ff0000")
+	if err != nil {
+		t.Fatalf("color returned error: %v", err)
+	}
+	if len(proto.brightnessWrites) != 0 {
+		t.Errorf("brightness writes = %v, want none for a hex notation", proto.brightnessWrites)
+	}
+	if strings.Contains(stdout, "brightness") {
+		t.Errorf("stdout = %q, must not report a brightness the command did not set", stdout)
+	}
+}
+
+// hsv: carries a value, and the keyboard has no value register for it, so the
+// value goes to the brightness of the same zones.
+func TestColorHSVNotationWritesTheValueAsBrightness(t *testing.T) {
+	proto := &colorProtocol{
+		appliedColor:  allZonesAppliedColor(85, 255),
+		appliedBright: map[via.LEDType]uint8{via.RGBLight: 200, via.RGBMatrix: 200, via.SideLight: 200},
+	}
+
+	stdout, _, err := runColor(t, proto, "", "hsv:85,255,200")
+	if err != nil {
+		t.Fatalf("color returned error: %v", err)
+	}
+	if len(proto.brightnessWrites) != 3 {
+		t.Fatalf("brightness writes = %v, want one per selected zone", proto.brightnessWrites)
+	}
+	for _, v := range proto.brightnessWrites {
+		if v != 200 {
+			t.Errorf("brightness write = %d, want 200", v)
+		}
+	}
+	if strings.TrimSpace(stdout) != "Color set to hue 85 sat 255 brightness 200" {
+		t.Errorf("stdout = %q, want hue, saturation and brightness", stdout)
+	}
+}
+
+// The Impact 80 clamps brightness at 160 on logo and side and scales it up on
+// the backlight channel, so an hsv: value is reported the way brightness is.
+func TestColorHSVSummarisesTheBrightnessTheKeyboardApplied(t *testing.T) {
+	proto := &colorProtocol{
+		appliedColor:  allZonesAppliedColor(0, 255),
+		appliedBright: map[via.LEDType]uint8{via.RGBLight: 160, via.RGBMatrix: 255, via.SideLight: 160},
+	}
+
+	stdout, _, err := runColor(t, proto, "", "hsv:0,255,200")
+	if err != nil {
+		t.Fatalf("color returned error: %v", err)
+	}
+
+	for _, want := range []string{
+		"logo hue 0 sat 255 brightness 160",
+		"backlight hue 0 sat 255 brightness 255",
+		"side hue 0 sat 255 brightness 160",
+		"requested hue 0 sat 255 brightness 200",
+	} {
+		if !strings.Contains(stdout, want) {
+			t.Errorf("stdout = %q, want it to contain %q", stdout, want)
+		}
+	}
+	if strings.Contains(stdout, "set to") {
+		t.Errorf("stdout = %q, must not claim the request was met when a zone clamped it", stdout)
+	}
+	if strings.Count(strings.TrimSpace(stdout), "\n") != 0 {
+		t.Errorf("stdout = %q, want exactly one line", stdout)
+	}
+}
+
+// A notation the tool cannot parse must fail before the keyboard is opened.
+func TestColorRejectsAnUnknownNotationWithoutOpeningTheDevice(t *testing.T) {
+	proto := &colorProtocol{}
+
+	opened := false
+	originalOpen := openRGBProtocol
+	originalZone := targetZone
+	t.Cleanup(func() {
+		openRGBProtocol = originalOpen
+		targetZone = originalZone
+	})
+	openRGBProtocol = func() (rgbProtocol, error) {
+		opened = true
+		return proto, nil
+	}
+	targetZone = ""
+
+	var out bytes.Buffer
+	cmd := NewColorCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&out)
+	cmd.SetArgs([]string{"xyz:1"})
+
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("color expected an error, got nil")
+	}
+	if opened {
+		t.Error("the keyboard was opened, want the notation rejected first")
+	}
+	if len(proto.colorWrites) != 0 {
+		t.Errorf("color writes = %v, want none", proto.colorWrites)
+	}
+}

cmd/qmk-rgb-tool/rgb.go

@@ -117,6 +117,52 @@ func setSpeedVerified(proto rgbProtocol, zones []intrgb.Zone, value uint8) ([]zo
 	return setValueVerified(proto, zones, uint8(intrgb.Speed), value)
 }
 
+// colorResult records the hue and saturation one zone holds after a color was
+// written to it. The color value ID carries two bytes, so both components are
+// read back: a keyboard that stored another saturation must not be reported as
+// having taken the requested color.
+type colorResult struct {
+	Zone                intrgb.Zone
+	RequestedHue        uint8
+	RequestedSaturation uint8
+	Hue                 uint8
+	Saturation          uint8
+}
+
+// Mismatch reports whether the keyboard stored something other than the
+// requested color.
+func (r colorResult) Mismatch() bool {
+	return r.Hue != r.RequestedHue || r.Saturation != r.RequestedSaturation
+}
+
+// setColorVerified writes one hue and saturation to every zone and reads each
+// back, for the same reason setValueVerified exists: the command must report
+// what the keyboard holds, not what it was asked for.
+func setColorVerified(proto rgbProtocol, zones []intrgb.Zone, hue, saturation uint8) ([]colorResult, error) {
+	if err := setColorOnZones(proto, zones, hue, saturation); err != nil {
+		return nil, err
+	}
+
+	results := make([]colorResult, 0, len(zones))
+	for _, zone := range zones {
+		raw, err := proto.GetValue(via.LEDType(zone.Channel()), uint8(intrgb.ColorValue))
+		if err != nil {
+			return nil, fmt.Errorf("read back color for %s: %w", zone, err)
+		}
+		if len(raw) < 2 {
+			return nil, fmt.Errorf("read back color for %s: got %d bytes, want hue and saturation", zone, len(raw))
+		}
+		results = append(results, colorResult{
+			Zone:                zone,
+			RequestedHue:        hue,
+			RequestedSaturation: saturation,
+			Hue:                 raw[0],
+			Saturation:          raw[1],
+		})
+	}
+	return results, nil
+}
+
 func setSpeedOnZones(proto zoneProtocol, zones []intrgb.Zone, value uint8) error {
 	return setValueOnZones(proto, zones, uint8(intrgb.Speed), value)
 }

internal/rgb/colorspec.go

@@ -0,0 +1,84 @@
+package rgb
+
+import (
+	"fmt"
+	"strconv"
+	"strings"
+)
+
+// HSV is a color in the units the keyboard uses: hue, saturation and value,
+// each 0-255. The keyboard stores hue and saturation only; Value is the
+// brightness register, which is why it is tracked apart from the color.
+type HSV struct {
+	H, S, V uint8
+
+	// hasValue separates "the notation carried no value" from "the notation
+	// asked for value 0". The hex forms mean the first, and leave V at zero.
+	hasValue bool
+}
+
+// HasValue reports whether the notation carried a value component, that is,
+// whether the caller asked for a brightness as well as a color.
+func (c HSV) HasValue() bool { return c.hasValue }
+
+// ParseColorSpec parses one of the three documented color notations: a bare or
+// rgb:-prefixed six digit hex value, or hsv:<h>,<s>,<v> in the firmware's own
+// 0-255 units. A hex value contributes hue and saturation; the value component
+// of a hex color is not a brightness request and is dropped.
+func ParseColorSpec(spec string) (HSV, error) {
+	prefix, body, found := strings.Cut(spec, ":")
+	if !found {
+		if prefix == "hsv" || prefix == "rgb" {
+			return HSV{}, unknownNotation(spec)
+		}
+		return hexToHSV(spec)
+	}
+
+	switch prefix {
+	case "hsv":
+		return parseHSVSpec(body)
+	case "rgb":
+		return hexToHSV(body)
+	default:
+		return HSV{}, unknownNotation(spec)
+	}
+}
+
+// hexToHSV converts a hex color to the hue and saturation the keyboard stores.
+// The value component of the hex color is not carried over: only the hsv
+// notation asks for a brightness, and HasValue keeps that distinction visible.
+func hexToHSV(hex string) (HSV, error) {
+	c, err := ParseHexColor(hex)
+	if err != nil {
+		return HSV{}, err
+	}
+	h, s, _ := c.HSV()
+	return HSV{H: h, S: s}, nil
+}
+
+// parseHSVSpec parses the body of an hsv: notation. Three components are
+// required: a two component form would have to mean "leave the brightness
+// alone", which no other notation does.
+func parseHSVSpec(body string) (HSV, error) {
+	parts := strings.Split(body, ",")
+	if len(parts) != 3 {
+		return HSV{}, fmt.Errorf("want hsv:<h>,<s>,<v> with three components in 0-255, got %q", body)
+	}
+
+	var components [3]uint8
+	for i, part := range parts {
+		n, err := strconv.ParseUint(part, 10, 8)
+		if err != nil {
+			return HSV{}, fmt.Errorf("invalid hsv component %q: want 0-255", part)
+		}
+		components[i] = uint8(n)
+	}
+
+	return HSV{H: components[0], S: components[1], V: components[2], hasValue: true}, nil
+}
+
+// unknownNotation names the accepted forms, because a rejected notation is
+// usually a typo of one of them.
+func unknownNotation(spec string) error {
+	return fmt.Errorf("unknown color notation %q: use ff0000, rgb:ff0000 or hsv:0,255,255", spec)
+}

internal/rgb/colorspec_test.go

@@ -0,0 +1,101 @@
+package rgb
+
+import (
+	"strings"
+	"testing"
+)
+
+// The color notation is documented in README.md: a bare or rgb:-prefixed hex
+// value, and hsv:<h>,<s>,<v> in the firmware's own 0-255 units. Only the HSV
+// form carries a value component, and it is not a color component: the keyboard
+// has no value register, so the caller must be able to tell "no value given"
+// from "value 255".
+func TestParseColorSpec(t *testing.T) {
+	tests := []struct {
+		name string
+		spec string
+		want HSV
+		// hasValue is false for the hex forms: nothing in the notation asks
+		// for a brightness, so the caller must not write one.
+		hasValue bool
+	}{
+		{"bare hex red", "ff0000", HSV{H: 0, S: 255}, false},
+		{"bare hex green", "00ff00", HSV{H: 85, S: 255}, false},
+		{"bare hex gray", "d0d0d0", HSV{H: 0, S: 0}, false},
+		{"bare hex black", "000000", HSV{H: 0, S: 0}, false},
+		{"bare hex white", "ffffff", HSV{H: 0, S: 0}, false},
+		{"rgb prefixed", "rgb:00ff00", HSV{H: 85, S: 255}, false},
+		{"rgb prefixed red", "rgb:ff0000", HSV{H: 0, S: 255}, false},
+		{"hsv full", "hsv:0,255,255", HSV{H: 0, S: 255, V: 255}, true},
+		{"hsv partial brightness", "hsv:85,255,128", HSV{H: 85, S: 255, V: 128}, true},
+		{"hsv zero value", "hsv:85,255,0", HSV{H: 85, S: 255, V: 0}, true},
+		{"hsv zero saturation", "hsv:120,0,200", HSV{H: 120, S: 0, V: 200}, true},
+		{"hsv hue 254", "hsv:254,255,255", HSV{H: 254, S: 255, V: 255}, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParseColorSpec(tt.spec)
+			if err != nil {
+				t.Fatalf("ParseColorSpec(%q) unexpected error: %v", tt.spec, err)
+			}
+			// Compare the components the notation can carry. V is only
+			// meaningful together with HasValue, so it is checked here
+			// rather than through struct equality.
+			if got.H != tt.want.H || got.S != tt.want.S || got.V != tt.want.V {
+				t.Errorf("ParseColorSpec(%q) = hue %d sat %d value %d, want hue %d sat %d value %d",
+					tt.spec, got.H, got.S, got.V, tt.want.H, tt.want.S, tt.want.V)
+			}
+			if got.HasValue() != tt.hasValue {
+				t.Errorf("ParseColorSpec(%q).HasValue() = %t, want %t", tt.spec, got.HasValue(), tt.hasValue)
+			}
+		})
+	}
+}
+
+func TestParseColorSpecRejectsInvalidInput(t *testing.T) {
+	tests := []struct {
+		name string
+		spec string
+		want string
+	}{
+		{"unknown prefix", "xyz:ff0000", "unknown color notation"},
+		{"hsv with two components", "hsv:0,255", "want hsv:<h>,<s>,<v>"},
+		{"hsv with four components", "hsv:0,255,255,255", "want hsv:<h>,<s>,<v>"},
+		{"hsv without components", "hsv:", "want hsv:<h>,<s>,<v>"},
+		{"hsv bare", "hsv", "unknown color notation"},
+		{"hsv component above range", "hsv:0,256,255", "invalid hsv component"},
+		{"hsv negative component", "hsv:-1,255,255", "invalid hsv component"},
+		{"hsv non numeric component", "hsv:0,ff,255", "invalid hsv component"},
+		{"hsv empty component", "hsv:0,,255", "invalid hsv component"},
+		{"hex too short", "ff00", "invalid hex color"},
+		{"hex with hash", "#ff0000", "invalid hex color"},
+		{"empty", "", "invalid hex color"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := ParseColorSpec(tt.spec)
+			if err == nil {
+				t.Fatalf("ParseColorSpec(%q) expected error, got nil", tt.spec)
+			}
+			if !strings.Contains(err.Error(), tt.want) {
+				t.Errorf("ParseColorSpec(%q) error = %q, want it to contain %q", tt.spec, err, tt.want)
+			}
+		})
+	}
+}
+
+// The error message is the only place a caller learns which notations exist, so
+// it has to name them.
+func TestUnknownNotationErrorNamesTheThreeForms(t *testing.T) {
+	_, err := ParseColorSpec("nope:1")
+	if err == nil {
+		t.Fatal("ParseColorSpec(\"nope:1\") expected error, got nil")
+	}
+	for _, form := range []string{"ff0000", "rgb:", "hsv:"} {
+		if !strings.Contains(err.Error(), form) {
+			t.Errorf("error %q does not mention the %q form", err, form)
+		}
+	}
+}