Media keys working again

docs/daemon-plan.md

@@ -1,6 +1,6 @@
 # gKeyMapper daemon — implementation plan
 
-Status: implemented (M0–M4 done, see "Implementation status")
+Status: implemented (M0–M5 done, see "Implementation status")
 Scope: a native C/C++ background service that detects G1–G9 (and later M1–M3/MR
 and media keys) on the Logitech G910 and replays the mappings stored by the GUI.
 
@@ -38,6 +38,18 @@ and media keys) on the Logitech G910 and replays the mappings stored by the GUI.
   the daemon on demand; the unit is `Type=dbus` with `BusName=`. A second
   instance refuses to start instead of fighting over the keyboard. Verified with
   `gdbus` (methods, signals, activation) and over AT-SPI (live footer).
+- **M5 done** — hardening and GUI polish: the daemon survives a missing or
+  disappearing keyboard and reopens it without losing its bus name (hot-plug,
+  quiet retry every 2 s); G-key decoding is state-based, so a held key no longer
+  repeats and two keys held at once both fire; the media keys are decoded and
+  forwarded through the virtual keyboard, so mute / volume / play keep working
+  while the daemon runs (see §4.4 and §11); the GUI loads the saved mappings
+  at startup (a fresh window can no longer save empty rows over the file) and the
+  footer distinguishes `Device: not found` and `Configuration error`; the
+  `GtkAboutDialog` `response` handler (invalid in GTK4) is gone. Verified without
+  hardware with a synthetic decoder test and a fake device driving the real loop,
+  with a GUI attached to it over D-Bus; the bitmap decoding and the media
+  passthrough were additionally confirmed on the real keyboard (see §9, §11).
 
 ## 1. Goals and non-goals
 
@@ -146,8 +158,13 @@ alternative; decision point in §11.
   (`bmRequestType=0x21, bRequest=0x09, wValue=0x0212, wIndex=1`) and consume the
   confirmation reports; ignore the known LED-return patterns
   (`\x11\xff\x0f`, `\x11\xff\x10`, `\x11\xff\xff`).
-- Hot-plug / re-plug: on `LIBUSB_ERROR_NO_DEVICE`/`IO` re-run discovery with a
-  small backoff (reference re-inits in place).
+- Hot-plug / re-plug (as implemented in M5): a read error closes the device,
+  reports `DeviceChanged(false)` and the loop keeps looking for it every 2 s
+  without exiting. A keyboard that is unplugged — or not there yet when the
+  session starts — never costs the bus name, so the GUI keeps its connection and
+  D-Bus activation does not start a second instance. The retries are quiet (the
+  absence is logged once); `GKeyDevice::open(quiet)` suppresses only the
+  cannot-use-the-keyboard lines, libusb-level failures always log.
 - Optional (later): `libusb_hotplug_register_callback` to react to device
   add/remove without polling.
 
@@ -162,6 +179,28 @@ artifact so the runtime does zero parsing.
   `dict` + `list(...).index()` O(n) scans with O(1)/O(log n) and no allocation.
 - Categories: `macro` (G1..G9), `memory` (profiles, later), `media` (later),
   `release` (stops a held shortcut), and ignored/known-noise.
+- **As implemented (M1, reworked in M5):** one `decode_gkey_state()` instead of a
+  report lookup table. The G910 macro report is a *state* report, not a
+  single-key event: bytes 0–3 are the fixed header (`11 ff 08 00`), byte 4 is the
+  bitmap of G1–G8 and the low bit of byte 5 is G9. The decoder derives the header
+  and the bit of every key from the per-model table, returns the pressed bitmap
+  and rejects the other families by length or header (the `11 ff 09`/`11 ff 0a`
+  memory keys, the `11 ff 08 2e` command and the LED answers). The daemon fires a
+  key on a 0 → 1 transition, which is what makes a held key (repeating report)
+  fire once, makes a chord fire each key in it, and makes the release report
+  (`11 ff 08 00` + zero bitmap) a no-op.
+- **Media keys (measured and added in M5):** they are *not* a lookup table either.
+  The keyboard reports them as a 2-byte report `02 <bit>` on the same interface
+  (bit 0 `01` next, `02` previous, `04` stop, `08` play/pause, `10` volume up,
+  `20` volume down, `40` mute, `02 00` release), so `decode_media_key()` reads the
+  bit byte and the daemon replays every bit it finds through its virtual keyboard.
+  That is a passthrough, not a mapping: without it the media keys would do nothing
+  at all while the daemon runs, because they reach the kernel only through this
+  interface (see §10 and §11).
+- Consequence: exact-report matching is gone, so `match_gkey`,
+  `is_release_report` and the `releaseReport` table entry went with it. Verified
+  without hardware against synthetic reports and through a fake device running
+  the real loop.
 
 ### 4.5 Action layer (uinput)
 - Open `/dev/uinput` once; `UI_SET_EVBIT` for `EV_KEY`, register the key set used
@@ -353,8 +392,13 @@ only dispatches what the loop's read timeout already woke up for.
 - **M4 – IPC (done).** GDBus service (`Reload`, `GetStatus`, `TestMapping`,
   signals); GUI status line + Reload-on-save; D-Bus activation file; unit
   `Type=dbus` + `BusName`; single-instance guard.
-- **M5 – Polish.** Packaging/install rules, docs, translations, hardening
-  (hot-plug, debounce, error reporting to the GUI).
+- **M5 – Polish (done).** Packaging/install rules, docs, translations, hardening
+  (hot-plug, debounce, error reporting to the GUI): the daemon survives a
+  missing/disappearing keyboard and reopens it without losing the bus name;
+  decoding is state-based (repeats suppressed, chords work); the GUI loads the
+  saved mappings at startup and reports `Device: not found` / `Configuration
+  error` in the footer; `GtkAboutDialog` no longer gets a `response` handler it
+  does not have in GTK4.
 
 ## 9. Testing strategy
 
@@ -369,6 +413,31 @@ only dispatches what the loop's read timeout already woke up for.
   line is read back over AT-SPI to check the live status.
 - Permission tests: verify the udev rule grants access as a normal user.
 - Resource check: `powertop`/`perf` or simple wakeup counting while idle.
+- Decoder without hardware (used in M5): compile `daemon/device_tables.cpp`
+  standalone and feed it synthetic reports — single keys, chords, a repeated
+  report, release and the families that must be ignored (media, memory, command,
+  LED). Scratch test, not part of the repo.
+- Loop without hardware (used in M5): build the daemon with a *fake*
+  `GKeyDevice` (same header, scripted `open()`/`read_report()`, linked without
+  libusb) to exercise hot-plug (no keyboard at start, device lost, reopen), the
+  D-Bus signals and the GUI footer states — without claiming the real keyboard.
+- Hardware confirmation (done in M5, 2026-10-18): the bitmap model was checked on
+  the real keyboard. Pressing two G keys at once logs both keys from a single
+  report (`G6 -> key:` and `G7 -> key:`, same timestamp) and a key held for three
+  seconds logs exactly one line, with the release logging nothing. Use two rows
+  whose target is empty so the check injects no key into the focused window.
+- Media keys (checked in M5, 2026-10-18): they are *not* on the keyboard's usual
+  event node. With the daemon stopped they arrive on `event11`
+  (`Phys …/input1`, created when `usbhid` rebinds interface `1.1`), while
+  `event10` (interface `1.0`) only advertises them in its capability bitmap;
+  with the daemon running neither node delivers them, which is why the daemon
+  forwards them itself. Raw reports can be captured from the `hidraw` node of
+  interface `1.1` while the daemon is stopped (that is how the bit table was
+  measured), and the passthrough is checked by pressing the keys with the repo
+  daemon running: `/tmp/daemon.log` logs `media -> mute` / `media -> volume up` /
+  `media -> stop` and the desktop reacts as it does without the daemon. A released
+  key logs nothing, and a `key`/`command` mapping is unaffected (press a G key in
+  between).
 
 ## 10. Risks / open points
 
@@ -376,33 +445,33 @@ only dispatches what the loop's read timeout already woke up for.
   byte tables is real work and model-specific.
 - **Interface detach** may momentarily disturb the device; must re-attach on
   exit and handle crashes gracefully.
+- **Media keys need the passthrough (measured 2026-10-18).** The keyboard reports
+  mute/volume/play only on the interface the daemon holds, so without the
+  passthrough of §4.4 they would do nothing while it runs — measured, and the
+  reason the decode path must not be dropped as "just a media key". See §11.
 - **Permissions**: relies on `TAG+="uaccess"` + a user service; must never fall
   back to requiring root silently.
-- **`command` environment**: `DISPLAY`/`WAYLAND_DISPLAY`/`XAUTHORITY` and
-  `PATH` differ between a user service and an interactive shell; decide whether
-  to inherit the user manager environment (`systemd --user` import-environment)
-  or document `run` snippets setting `DISPLAY=:0` explicitly.
 - **Key-name vocabulary drift** between GUI (GDK names) and daemon (Linux KEY_*)
   is the most likely source of silent mismatches — that is why the shared table
   in M0 is mandatory before the daemon consumes the file.
 - **uinput device identity**: a second keyboard appears; choose stable
   name/vendor/product so the desktop does not confuse it with the real one.
-- **GUI does not load the config at startup** (found while testing M4, GUI-side,
-  not a daemon issue): `build_ui()` fills the rows empty and only
-  `Import configuration...` reads the file, so pressing `Save mappings` in a
-  freshly opened window overwrites the user's mappings with empty ones. Loading
-  `~/.config/gKeyMapper/mappings.conf` on start (or refusing to save unchanged
-  rows) should be the next GUI fix.
 
 ## 11. Decisions to make
 
-1. Bus name / app-id: replace `com.example.gkeymapper` with a real reverse-DNS id
-   (suggested `dev.rager.gkeymapper`)? Affects the `.desktop` name too.
-2. Event loop: GLib `GMainLoop` (simplest, reuses GIO) vs. raw `epoll` (leanest).
-3. IPC: D-Bus (idiomatic, GIO already present) vs. a Unix socket (no bus
-   dependency). Recommendation: D-Bus, with signals as a fallback.
-4. Scope of v1: keep `key`/`command`/`disabled` only, or also port
-   `typeout`/`media`?
+The questions this section used to list were all settled while implementing M3–M5
+— app id/bus name (`dev.rager.gkeymapper` in M4), IPC (D-Bus with GDBus, in M4)
+and the event loop (blocking loop + `DaemonService::pump()`, in M4). Their
+answers are kept under *Resolved* below. What is genuinely still open:
+
+- **Scope beyond v1.** v1 ships `key`/`command`/`disabled` plus the media-key
+  passthrough. The reference project additionally has `typeout` (type out a fixed
+  text), handles the media /memory keys itself and can *remap* them: our daemon
+  forwards the media keys unchanged (see *Resolved*) but ignores the memory keys
+  (`11 ff 09/0a`) and the command/LED answers. `typeout` would be a pure action
+  addition with no decoding involved (it needs the text typed through the active
+  layout); media-key *remapping* would turn the passthrough into mappings with
+  config entries and GUI rows. Not in v1 — decide when the need comes up.
 
 ### Resolved
 
@@ -457,3 +526,62 @@ only dispatches what the loop's read timeout already woke up for.
   `DO_NOT_QUEUE` and exits with a message when it is already taken — two
   instances would detach the kernel driver from each other and create two virtual
   keyboards. The check happens before the device is opened.
+- **GUI loads the config at startup (done in M5).** `build_ui()` reads
+  `default_config_path()` into the rows once they exist (skipped while the file
+  does not exist, so a first start stays quiet). This closes the M4 finding that
+  `Save mappings` in a fresh window overwrote the user's mappings with empty
+  rows; since the rows are filled from exactly what the file holds, saving
+  rewrites the same content.
+- **Hot-plug (done in M5).** The daemon no longer exits when the keyboard cannot
+  be opened: it reports the absence once, keeps serving D-Bus and the config
+  watch and retries every 2 s (sleeping 200 ms per iteration in between), so a
+  re-plug is picked up without a restart and without losing the bus name. A read
+  error is treated the same way (`DeviceChanged(false)` → reopen →
+  `DeviceChanged(true)`).
+- **G-key decoding (reworked in M5).** Deliberately not the report-lookup table
+  of §4.4: the macro report is a state bitmap, and a transition decoder is both
+  simpler and more correct — a held key no longer fires repeatedly and a chord
+  fires every key in it. The exact-match helpers it replaced were removed rather
+  than kept as dead code.
+- **Bitmap decoding confirmed on hardware (M5, 2026-10-18).** The bitmap model is
+  what the device really sends, not just what the reference tables suggested: one
+  report with two bits set fired both keys (G6 and G7, same second in the journal),
+  a key held for three seconds fired exactly once, and the release fired nothing.
+  `decode_gkey_state()` is still the single place to change if another model in
+  `device_tables` turns out to differ.
+- **Media keys keep working while the daemon runs (added in M5).** They are the
+  second report family on the same interface: a 2-byte `02 <bit>` report that the
+  first decoder version rejected by length. The daemon now decodes it
+  (`decode_media_key()`) and replays every bit it contains through the virtual
+  keyboard (`UinputKeyboard::tap()` via `ActionRunner::tap_key()`), so mute /
+  volume / play / next / previous / stop behave exactly as they do without the
+  daemon — the report is forwarded, not turned into a mapping, and the release
+  report carries no bit and is dropped.
+  The bit table was *measured* rather than guessed: the `hidraw` node of interface
+  `1.1` (which exists only while `usbhid` owns it) gave `02 01` next, `02 02`
+  previous, `02 04` stop, `02 08` play/pause, `02 10` volume up, `02 20` volume
+  down, `02 40` mute and `02 00` for the release, and the kernel's own input
+  device for that interface confirmed the codes (`KEY_NEXTSONG`,
+  `KEY_PREVIOUSSONG`, `KEY_STOPCD`, `KEY_PLAYPAUSE`, `KEY_VOLUMEUP`,
+  `KEY_VOLUMEDOWN`, `KEY_MUTE`). Both interfaces advertise the consumer usages in
+  their HID descriptors, so the capability bitmap alone is misleading — the raw
+  report is the only reliable source.
+  Why it has to exist at all: those keys reach the kernel only through this
+  interface, so while the daemon held it they did nothing. The README called that
+  a "may stop working" limitation until the measurement showed it was a plain
+  regression. Verified live: five volume-up and three volume-down presses moved
+  the sink by exactly two 2.5 % steps, and `media -> mute` / `media -> stop` /
+  `G6 -> key:` appeared in the daemon log.
+  Remapping the media keys instead of forwarding them stays open (§11, "Scope
+  beyond v1").
+- **`command` environment (checked in M5).** Nothing to do: the daemon runs as a
+  **user** service, and the user manager's environment already carries
+  `DISPLAY=:0`, `XAUTHORITY` and a sane `PATH` on this machine (verified with
+  `systemctl --user show-environment` and by letting a `command` mapping dump its
+  environment). If a command cannot reach the display on another setup,
+  `systemctl --user import-environment DISPLAY WAYLAND_DISPLAY XAUTHORITY` is the
+  fix; the README says so.
+- **About dialog (fixed in M5).** It must not be connected to `response`: GTK4's
+  `GtkAboutDialog` is not a `GtkDialog` subclass with buttons, so the signal does
+  not exist and connecting to it only logged `GLib-GObject-WARNING`. Closing goes
+  through `close-request`, whose default handler destroys the window.