瀏覽代碼

spec: discover VIA channels instead of assuming three zones

The tool drives hardcoded zones mapped to channels 2, 3 and 4, so any
other keyboard is written to at with the Impact 80's channels, effect
names and firmware transforms. Channel presence is queryable: QMK
answers an uncompiled channel with 0xFF, and channels 2, 3 and 4 answer
on this board while 1 and 5 do not. The design probes channels 1 to 15,
names them by QMK subsystem name, takes display names from keyboards.json
and keeps the effect catalogs in code with their provenance recorded.

The spec records the rejected alternatives, the byte-identical-output
acceptance criterion for the board we have, and the transient protocol
desync the probe is more exposed to.
Paul Klumpp 1 周之前
父節點
當前提交
0f141964ca
共有 1 個文件被更改,包括 320 次插入 和 0 次删除
  1. 320 0
      docs/superpowers/specs/2026-09-27-channel-discovery-design.md

+ 320 - 0
docs/superpowers/specs/2026-09-27-channel-discovery-design.md

@@ -0,0 +1,320 @@
+# Channel Discovery Design
+
+Date: 2026-09-27
+Status: approved in conversation, awaiting written review
+
+## Problem
+
+The tool drives three hardcoded zones. `internal/rgb/impact80.go` names them
+`logo`, `backlight` and `side`, and `Zone.Channel()` maps them to VIA channels
+2, 3 and 4. Every command therefore assumes that any connected keyboard exposes
+exactly those three channels, with the Impact 80's effect catalog and the
+Impact 80's per-channel firmware transforms.
+
+That assumption is wrong for any other board. A keyboard with only
+`rgb_matrix` would receive writes to channels it does not have, and a keyboard
+with a different firmware build would be offered effect names that mean
+something else there. `keyboards.json` does not help: it supplies names only,
+and `README.md` says so.
+
+The question this design answers: how much of the zone model can be derived
+from the keyboard instead of assumed?
+
+## What the hardware and protocol actually offer
+
+Measured on the Impact 80 by reading the HID reports directly, and read from
+QMK upstream.
+
+**Channel presence is queryable.** QMK's `quantum/via.h` defines the channel
+space:
+
+```c
+enum via_channel_id {
+    id_custom_channel = 0,
+    id_qmk_backlight_channel = 1,
+    id_qmk_rgblight_channel = 2,
+    id_qmk_rgb_matrix_channel = 3,
+    id_qmk_audio_channel = 4,
+    id_qmk_led_matrix_channel = 5,
+};
+```
+
+A `CustomGetValue` for a channel the firmware does not compile in is answered
+with `id_unhandled` (`0xFF`); a channel it does have answers with `0x08`. Probe
+results on the Impact 80, asking for brightness (value ID `0x01`):
+
+| Channel | Answer | Verdict |
+|---:|---|---|
+| `0x00`, `0x01` | `0xFF` | not present |
+| `0x02` | `0x08`, value 160 | present, `rgblight` |
+| `0x03` | `0x08`, value 255 | present, `rgb_matrix` |
+| `0x04` | `0x08`, value 160 | present, `audio` |
+| `0x05`–`0x08` | `0xFF` | not present |
+
+The `0xFF` response is the discriminator. An unknown *value ID* on a known
+channel behaves differently: the firmware mirrors the request and answers with a
+zero payload. That asymmetry is what makes the probe unambiguous.
+
+**Value IDs are uniform across subsystems.** QMK defines brightness `1`, effect
+`2`, effect speed `3` and color `4` identically for `rgblight` and
+`rgb_matrix`. There is no per-subsystem value handling to write, which is the
+main reason this design is small.
+
+**Effect catalogs are not queryable.** Stock VIA has no enumeration command;
+`GetValue(channel, 0x02)` returns only the running effect. The catalog is a
+compile-time constant in the firmware. Vial would answer it through its own RGB
+commands (`GetInfo` `0x40`, `GetSupportedOrDirectFastSet` `0x42`), but the
+Impact 80 runs stock VIA. Whether brute-force probing would recover the valid ID
+range is unknown and untested; the design does not depend on the answer.
+
+**Color is two bytes.** Value ID `4` carries hue and saturation only. The
+keyboard has no value register, and a VIA handler preserves the current value
+when the color is set. `color` therefore reads back two bytes, and its
+`hsv:<h>,<s>,<v>` notation writes `v` to the brightness register of the same
+channels, which is documented in README.md.
+
+## Decisions
+
+Decided in conversation, with the rejected alternatives recorded.
+
+1. **Probe channel presence; do not guess.** Rejected: keep the hardcoded three
+   zones. It is the bug being fixed.
+2. **Name channels with the QMK subsystem names.** `--zone rgblight`,
+   `--zone rgb_matrix`, `--zone audio`. Rejected: physical names from the board,
+   which are not in the protocol; and numbers only, which lose readability.
+3. **The canonical vocabulary is the QMK subsystem name.** A board may add
+   display names in `keyboards.json`, and those are accepted too. The physical
+   names are not baked into the code: `logo` and `side` are names this board
+   supplies through the file, which is why they keep working here and would not
+   work on a board with no entry. Rejected: keeping the old names as aliases in
+   code, which would make them work everywhere and put a board's layout in the
+   binary. `--zone backlight` on the Impact 80 resolves to channel 3 because the
+   file says so; on a board that has a channel 1, the same name is the QMK
+   meaning and the file entry is rejected at load rather than retargeting the
+   command.
+4. **Store only a display name per channel in `keyboards.json`.** The subsystem
+   name follows from the channel number, so it is not stored.
+5. **Keep the effect catalogs in code, keyed by VID/PID.** Rejected: catalogs in
+   `keyboards.json` (a transcription of an external document would then live in
+   the file the README describes as a name lookup), and a catalog per subsystem
+   (provably wrong for `rgblight` and `audio`, whose names on this board are the
+   vendor's 7-entry scheme, not QMK's).
+6. **A board without a catalog gets raw IDs only.** `effect <name>` fails and
+   names `mode <index>` as the way. `effect --list` reports an empty catalog.
+7. **Do not open the device in `keyboard info`.** Channel detection therefore
+   runs in the commands that already open a keyboard, and `info` reports them.
+
+## Design
+
+### Detection
+
+A probe over channels 1 to 15, one read each, asking for brightness:
+
+```
+for channel := 1; channel <= 15; channel++ {
+    response := GetValue(channel, 0x01)
+    0xFF -> absent
+    0x08 -> present
+}
+```
+
+Brightness is the probe value because it is a single byte and valid on every
+lighting subsystem, so one request finds every kind of channel. The range goes
+to 15 rather than stopping at 5 so a future QMK that claims channel 6 is found
+without a code change; unassigned channels answer `0xFF`, which is measured.
+
+New API, in `internal/via` because the channel space is a protocol concept:
+
+```go
+type Channel uint8
+
+const (
+    ChannelBacklight Channel = 1
+    ChannelRgblight  Channel = 2
+    ChannelRgbMatrix Channel = 3
+    ChannelAudio     Channel = 4
+    ChannelLedMatrix Channel = 5
+)
+
+func (c Channel) Subsystem() string  // "backlight", "rgblight", "rgb_matrix", "audio", "led_matrix"
+
+func (p *Protocol) DetectChannels() ([]Channel, error)
+```
+
+`internal/rgb.Zone` and `Zone.Channel()` are removed; commands address
+`via.Channel` directly.
+
+### Naming
+
+`keyboards.json` gains an optional per-board display name per channel:
+
+```json
+{
+  "name": "Wobkey Impact 80",
+  "vendorId": 14000,
+  "productId": 12447,
+  "channels": { "2": "logo", "3": "backlight", "4": "side" }
+}
+```
+
+`--zone` accepts, in resolution order:
+
+1. a display name, when the board is known, the channel is present, and the
+   name does not shadow the subsystem name of a *different* present channel
+2. a subsystem name, which always works for a present channel
+
+A display name that shadows another present channel's subsystem name is
+rejected when the file is loaded, with an error naming the conflict. This is the
+case that would otherwise retarget a command silently.
+
+The Impact 80's display names are `logo`, `backlight` and `side`, and it has no
+channel 1, so `backlight` is unambiguous there. That is a property of this board
+to be verified, not assumed: a board with both channel 1 and a channel 3 called
+`backlight` must fail to load.
+
+Without an entry in `keyboards.json`, channels are named by subsystem only.
+
+Resolution lives in `cmd/qmk-rgb-tool/zones.go`, which already owns
+`--zone` parsing; the display names come from `internal/device`, which already
+loads the file.
+
+### Effect catalogs
+
+`internal/rgb` keeps the Impact 80 tables and gains a lookup:
+
+```go
+// The 46 backlight names are QMK's rgb_matrix_effects.inc, verified against
+// the live register. The 7 logo and side names are the vendor's VIA
+// definition for this board, whose dropdowns read "fixed wave" and "breathe";
+// the tool spells them fixed_wave and breathing and adds the compatibility
+// aliases. The brightness and speed transforms were measured on the unit.
+// The generic Effect enum in effects.go is dead and is removed.
+func CatalogFor(vendorID, productID uint16) (*Catalog, bool)
+
+// Catalog is one board's effect names, keyed by the channel they apply to.
+type Catalog struct {
+	effects map[via.Channel][]string
+}
+```
+
+`internal/rgb` imports `internal/via` for the `Channel` type. That direction is
+acyclic already: `internal/via` imports `internal/device` and `internal/hid`,
+never `internal/rgb`.
+
+Unknown board, no catalog:
+
+- `effect <name>` returns an error naming the board and pointing at
+  `mode <index>`
+- `effect --list` prints an empty `zones` array
+- `mode <index>`, `brightness`, `speed`, `color` and `info` all work, because
+  they need no catalog
+
+### Command surface and JSON
+
+`effect --list` is extended additively so existing readers keep working. For a
+known board, `zone`, `effect` and `id` are unchanged and three fields appear:
+
+```json
+{
+  "catalog": "impact80",
+  "zones": [
+    {"zone": "logo", "channel": 2, "subsystem": "rgblight", "effect": "none", "id": 0}
+  ]
+}
+```
+
+For a board without a catalog, `"catalog": ""` and `"zones": []`.
+
+`info` keeps its shape. Its `zone` field carries the resolved name, and its
+existing `channel` field becomes the authoritative identity. On the Impact 80
+both are byte-identical to today's output.
+
+Default targeting without `--zone` is every detected channel in ascending
+channel order, which for the Impact 80 is 2, 3, 4 — the current order.
+
+### Profiles
+
+Profiles are keyed by zone name, so every key must resolve to a channel. On
+load, each key resolves through the same order as `--zone`: display name first,
+subsystem name second. A key that resolves to nothing is skipped with a warning,
+as today.
+
+The Impact 80's display names are the strings its existing profiles already use,
+so those profiles keep loading. Renaming a channel in `keyboards.json`
+invalidates the profiles that used the old name, and README.md will say so.
+
+## Constraints
+
+- `keyboard info` must not open a device, so it cannot report channels.
+- Read-back reporting stays as it is for the parameters that report values today,
+  `brightness`, `speed` and `color`. `effect` and `mode` still write without
+  reading back; closing that gap is a separate change, deliberately not folded
+  into this one.
+- A silent retarget is a defect. Where a name could mean two channels, the tool
+  refuses.
+- No Vial support, and no editing of effect catalogs outside code.
+
+## Acceptance criteria
+
+1. `effect --list` and `info` produce byte-identical output on the Impact 80
+   before and after the change. The board we have must not change behaviour.
+2. `qmk-rgb-tool info` names the Impact 80's channels `logo`, `backlight` and
+   `side`, and `qmk-rgb-tool --zone rgb_matrix brightness 160` reaches the same
+   zone as the old `--zone backlight`.
+3. `--zone logo` reaches channel 2 on the Impact 80, because `keyboards.json`
+   names it. On a board with no entry, `--zone logo` is rejected with an error
+   naming the accepted forms, and the exit code is non-zero.
+4. `keyboards.json` with a display name that shadows a present channel's
+   subsystem name is rejected at load, naming the conflict.
+5. On a board with no catalog entry, `effect --list` prints an empty `zones`
+   array and `effect anything` fails with a message that names `mode`.
+
+## Testing
+
+Unit tests against the existing protocol fakes, no device:
+
+- `DetectChannels` returns exactly the channels that answer, treats `0xFF` as
+  absence, and propagates a read error instead of reporting a short list
+- no channel present produces a clear error, not an empty zone list
+- name resolution for a known board, an unknown board, and a display name that
+  shadows a subsystem name
+- catalog gating: known board resolves names, unknown board does not
+- `mode` still works on a board without a catalog
+- profile load with display-name keys and with subsystem-name keys, and the
+  warning for a key that resolves to nothing
+- removal of the dead `Effect` enum and `State` type, together with their tests
+
+Live verification on the Impact 80, by hand, comparing before and after:
+`effect --list`, `info`, one `brightness`, one `speed`, one `color`, and a
+profile save and load round trip.
+
+## Documentation
+
+README.md:
+
+- `--zone` vocabulary is the QMK subsystem names, with display names per board
+- the protocol section names the channel enum and its QMK source
+- the effect catalog is per board; an unknown board has raw IDs only
+- renaming a channel invalidates the profiles that used the old name
+
+AGENTS.md:
+
+- Device Selection no longer claims the three zones are fixed, and no longer
+  claims `--zone` takes `logo`, `backlight` or `side`
+- the provenance note from the catalog section, so nobody re-derives the names
+  or edits them on a hunch
+
+## Risks
+
+- **A board that answers on a channel it should not.** If a firmware responds
+  to a channel it does not implement, the probe reports a phantom zone. Nothing
+  in the measurement shows that, and a read-back after every write would catch
+  it, but the detection itself would still be wrong.
+- **The transient protocol desync.** A `save` during this session failed once
+  with `Interrupted system call` and two value mismatches, which is a
+  request/response stream falling out of step. A probe is 15 sequential
+  round trips and is therefore more exposed to it than a single read. If it
+  happens, the probe must fail loudly rather than report a short channel list
+  as complete. This is unfixed and tracked separately.
+- **Profile names become board metadata.** A board whose display names change
+  silently stops loading its old profiles. Documented, and a warning is printed.