diff --git a/AGENTS.md b/AGENTS.md index 4027ca9..5eb7cad 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,15 +89,11 @@ reported `IDR=0x0000` for several rounds because its regex did not match gdb's output format at all. The register was fine; the reader was broken. Cross-check with `tools/gpiob_dump.sh`, which uses a different path. -## The keypad: two separate problems, one fixed +## The keypad works: it was always the hold time The old note here said "keys reach the firmware but the UI does not react" and -pointed at the machine model. The model was not the problem. There were two -independent causes, and only the first is fixed. - -### Fixed: key.py held keys far too long - -`tools/key.py` was holding every key for 2500 ms. +pointed at the machine model. The model was never the problem, and there was only +ever one cause: `tools/key.py` held every key for 2500 ms. The two SysTick mechanisms are separate, and conflating them caused this: @@ -155,34 +151,87 @@ lands on 3 rather than 30. Pre-positioning `gMenuCursor` with gdb, in one attach right after opening the menu, is the reliable way to reach a distant entry. Verified this way: menu opens, DOWN/UP move the list, MENU enters a submenu, and -a digit selects a value. Screenshots confirmed BatSav at 30/79 showing OFF. +a digit selects a value. Screenshots confirmed Step at 01/79, RxDCS at 03/79 +after two DOWN presses, and BatSav at 30/79 showing OFF. -### Known limitation: power save stops the keypad scan +### row_out must stay volatile or GCC deletes the keypad -The keypad works, but only while the radio is awake. Measured on a fresh boot: -`gCurrentFunction` is 0 (`FUNCTION_FOREGROUND`) until about 6 s, then becomes 5 -(`FUNCTION_POWER_SAVE`) and the scan stops: +`UVK5KeypadState::row_out` is declared `qemu_irq volatile`. Drop the `volatile` +and the keypad stops working entirely: no press reaches the UI, awake or in power +save, and nothing warns you. `tools/keypad_test.py` covers it. - gRxIdleMode=1 gCurrentFunction=5 +The reason is visible in the object code. `qdev_init_gpio_out_named()` is +inlinable and only records the array; the lines are filled in later by +`qdev_connect_gpio_out_named()` from the board, which GCC cannot see. Left plain, +GCC at -O2 proves every element is still NULL, sees that `qemu_set_irq()` returns +immediately on a NULL irq, and deletes the body of `keypad_update_rows()` along +with **all five calls to it**: -From then on `KEYBOARD_Poll` returns `KEY_INVALID` however long or often a key is -held — eight consecutive `key.py MENU` presses left `gKeyReading0` at 19 and -`gScreenToDisplay` at 0. A real radio wakes on a keypress, so this is a gap in -the model rather than firmware behaviour. The suspect is the sleep/wake path in -`HandlePowerSave`, which calls `BK4819_Sleep()` and spins on `BK4819_REG_0C` -bit 0 over the bit-banged bus. Note that bus shares GPIOB with the keypad (bus on -PB8/PB9, columns PB3-PB6, rows PB12-PB15), and the model idles PB9 low as a -deliberate hack so those reads return zero — worth a look when someone picks this -up. + callers reaching keypad_update_rows + plain {} <- none; the calls are gone + volatile {keypad_key_changed, keypad_col_changed, keypad_set_press, + keypad_reset, uvk5_machine_init} -Working around it is easy and does not need the gap closed: any keypad activity -keeps the radio awake, and being in the menu blocks power save outright -(`gScreenToDisplay != DISPLAY_MAIN` guards the entry at `app/app.c:1377`). So -open the menu within the first few seconds of boot and a session stays usable -indefinitely. Turning BatSav off through the UI works too, though the setting -does not survive a restart — see below. +`keypad_col_changed` compiles to a store and a `ret` with no call at all. With +`volatile` it ends in `jmp keypad_update_rows`. So no row line is ever driven, +the firmware's scan reads all-high, and the model looks broken. -Two dead ends, both confirmed by experiment, so nobody repeats them: +Getting here took three wrong diagnoses, all worth knowing about: + +1. **"Power save stops the keypad scan."** Written up here as a model gap. It was + not: the breakage was present awake too. +2. **"It needs settling time."** Three `fprintf(stderr, "TRACE ...")` probes had + been removed as cleanup, and restoring the one in `keypad_update_rows` fixed + it, as did a busy loop in the same place. That looked like a timing + dependency. It was not — the fprintf and the loop were just side effects GCC + could not discard, which kept the loop alive. +3. **"It is a compiler ordering problem."** A zero-cost + `__asm__ __volatile__("" ::: "memory")` also fixed it, 8/8. Same reason: a + barrier is an unknown side effect, so the loop survives. + +What settled it was comparing the two object files instead of the behaviour. The +standalone `keypad_update_rows` symbol is instruction-identical either way, which +is why an early diff of just that function found nothing — the function is +inlined into its callers, and the difference is there. + +Measurements, 3+ trials each, no debugger near the press: + +| variant | result | +| --- | --- | +| plain `row_out` | 0/12 | +| `(void)r;` added — inert, no side effect | 0/6 | +| identical rebuild (stability control) | 0/6 | +| busy loop, 1 to 4000 iterations | 3/3 | +| `__asm__ ... "memory"` barrier | 12/12 | +| **`volatile row_out`** (the actual fix) | **10/10** | + +Scope, checked rather than assumed: the other out-GPIO array in this file, +`PY32GpioState::out`, is **not** affected. Marking it volatile as well produces a +byte-identical object file, because the function that drives those lines +(`py32_gpio_write`) is only reachable through a `MemoryRegionOps` function-pointer +table, so GCC cannot do the whole-function reasoning that killed the keypad path. +Leave it plain. + +The general shape to watch for: a device whose out-GPIO lines are only ever +connected from board code, driven from a function GCC can see all callers of. If a +model's outputs mysteriously do nothing, check the object code for the call before +assuming the logic is wrong: + + objdump -dr build/libqemu-arm-softmmu.fa.p/hw_arm_py32f071.c.o \ + | grep -c qemu_set_irq + +Two measurement mistakes made this much harder than it needed to be, both worth +avoiding: + +- **Reading key state after releasing the key.** `gKeyReading0` is always + `KEY_INVALID` once the key is up, so it "proves" the press was never seen. Read + mid-hold instead. +- **Trusting a gdb breakpoint on `KEYBOARD_Poll`.** With the guest stopped the + scan's delays cost no guest time, so `Poll` returns `KEY_MENU` under a + breakpoint on a build where it returns `KEY_INVALID` when running free. That + single observation sent this in the wrong direction for a long time. + +Two related facts, both confirmed by experiment, so nobody spends time on them: - **Patching battery save in `assets/flash.img` does nothing.** `SETTINGS_InitEEPROM` compares a version string at flash `0x00A160`, finds a @@ -191,19 +240,22 @@ Two dead ends, both confirmed by experiment, so nobody repeats them: byte planted at `0x00A00B` is gone before the read at `settings.c:169` sees it. - **Guest-side settings changes do not persist.** The emulated PY25Q16 loads the image into RAM at realize time and never writes back, so anything the firmware - saves is lost on restart. Adding a flush would be the real fix if persistent - settings are ever wanted. + saves is lost on restart. Adding a flush would be the fix if persistent + settings are ever wanted. Nothing needs it today. Useful here: `tools/scan_trace.sh` (what the scan reads), `tools/key_result.sh` (what Poll returns), `tools/trace_run.sh` (the TRACE points). The three `fprintf(stderr, "TRACE ...")` probes that used to sit in `qemu/py32f071.c` are gone -- they fired on every keypad poll and buried the -console. If you need them back while chasing the power save gap, they went in -`py32_gpio_set_input`, `keypad_update_rows` and `keypad_col_changed`; `git log -p --- qemu/py32f071.c` has the exact lines. `grep -c 'keypad row0 -> 0'` on the -captured stderr was how the matrix got confirmed working, and it is still the -quickest check that a press reaches the model. +console. They went in `py32_gpio_set_input`, `keypad_update_rows` and +`keypad_col_changed`; `git log -p -- qemu/py32f071.c` has the exact lines, and +they are still the quickest way to see whether a press reaches the model +(`grep -c 'keypad row0 -> 0'` on the captured stderr). + +Redirect that stderr to a file rather than a pipe, and be aware that the +`keypad_update_rows` one changes timing enough to matter -- see the settle-loop +note above. Note the ELF at `uvk5-sat/build/CW/nr7y.cw.elf` carries no DWARF, so gdb reports `'gEeprom' has unknown type`. Scalars work if you cast through their address diff --git a/README.md b/README.md index 7b742d5..dbbcab1 100644 --- a/README.md +++ b/README.md @@ -9,12 +9,12 @@ is not modelled. | Main screen | Menu | Navigated with keys | | --- | --- | --- | -| ![main VFO screen](docs/screenshots/main-vfo.png) | ![menu at Step](docs/screenshots/menu-step.png) | ![menu at BatSav](docs/screenshots/menu-batsav.png) | +| ![main VFO screen](docs/screenshots/main-vfo.png) | ![menu at Step](docs/screenshots/menu-step.png) | ![menu at RxDCS](docs/screenshots/menu-navigated.png) | Real captures, not mock-ups: `tools/screenshot.py` reads the firmware's `gFrameBuffer` out of guest memory and renders it, so these are the pixels the LCD driver actually wrote. Left to right: the dual-watch main screen, the menu -opened with `key.py MENU`, and entry 30/79 reached with keypresses. +opened with `key.py MENU` (entry 01/79, Step), and 03/79 after `key.py DOWN DOWN`. ## What it is for @@ -37,7 +37,7 @@ has no public datasheet, so its driver is the only specification available. | Boot to main loop | works, ~5 s | | LCD contents | readable via `tools/screenshot.py` | | SPI flash, settings, calibration | works | -| Keypad and menu navigation | works while the radio is awake; power save stops the scan, see below | +| Keypad and menu navigation | works, including waking from power save | | Timing accuracy | deliberately wrong, see [Timing](#timing) | | Radio/RF behaviour | not modelled | @@ -46,15 +46,19 @@ a submenu, and typing a menu number jumps straight to that entry. Press duration decides short versus held, which the firmware treats as different events -- see [Timing](#timing). -The limitation is power save. Around six seconds after boot the firmware enters -it (`gCurrentFunction` becomes `FUNCTION_POWER_SAVE`) and stops scanning the -keypad, so keys are ignored from then on. A real radio wakes on a keypress, so -this is a gap in the machine model rather than firmware behaviour. +Press duration is the thing to get right. A hold of 400 ms or more is a *long* +press, and handlers act on it differently: `MAIN_Key_MENU` opens the menu on a +short release and does nothing on the hold path. If a key seems ignored, shorten +the press rather than lengthening it. Waking from power save needs nothing +special -- one 200 ms press both wakes the radio and opens the menu, verified +after 45 s of idle. -In practice it is not much of an obstacle: keypad activity keeps the radio awake, -and being in the menu blocks power save entirely. Open the menu within the first -few seconds of boot and the session stays usable. `AGENTS.md` has the details, -including two approaches that look like fixes and are not. +`tools/keypad_test.py` checks all of this against a throwaway QEMU instance. It +exists because the keypad has one non-obvious trap: the keypad model's `row_out` +array must stay `volatile`, or GCC at -O2 proves the lines are still NULL and +deletes every call to `keypad_update_rows()`, so no row is ever driven and +keypresses silently stop working. Run the test after touching that code; +`AGENTS.md` has the object-code evidence. ## Layout diff --git a/docs/screenshots/menu-batsav.png b/docs/screenshots/menu-batsav.png deleted file mode 100644 index 870bd48..0000000 Binary files a/docs/screenshots/menu-batsav.png and /dev/null differ diff --git a/docs/screenshots/menu-navigated.png b/docs/screenshots/menu-navigated.png new file mode 100644 index 0000000..346f158 Binary files /dev/null and b/docs/screenshots/menu-navigated.png differ diff --git a/qemu/py32f071.c b/qemu/py32f071.c index f336c37..2e4b948 100644 --- a/qemu/py32f071.c +++ b/qemu/py32f071.c @@ -414,7 +414,21 @@ struct UVK5KeypadState { bool pressed[KEYPAD_COLS][KEYPAD_ROWS]; bool col_high[KEYPAD_COLS]; - qemu_irq row_out[KEYPAD_ROWS]; + /* + * volatile is required, not decorative. qdev_init_gpio_out_named() is + * inlinable and only records this array; the lines are filled in later by + * qdev_connect_gpio_out_named() from the board, which GCC cannot see. Left + * plain, GCC at -O2 proves every element is still NULL, notices that + * qemu_set_irq() returns immediately on a NULL irq, and deletes the whole + * body of keypad_update_rows() along with all five calls to it -- so no row + * line is ever driven and the firmware's keypad scan reads nothing. That + * failure is silent and looks exactly like a broken keypad model. + * + * Verified from the object code: without volatile, keypad_col_changed + * compiles to a store and a ret with no call at all; with it, the call is + * emitted. See AGENTS.md. + */ + qemu_irq volatile row_out[KEYPAD_ROWS]; }; /* @@ -491,7 +505,14 @@ static void keypad_init(Object *obj) qdev_init_gpio_in_named(dev, keypad_col_changed, "col", KEYPAD_COLS); qdev_init_gpio_in_named(dev, keypad_key_changed, "key", KEYPAD_COLS * KEYPAD_ROWS); - qdev_init_gpio_out_named(dev, s->row_out, "row", KEYPAD_ROWS); + /* + * Cast away volatile for the registration call only. row_out is declared + * volatile so GCC cannot conclude the lines stay NULL and delete + * keypad_update_rows() -- see the comment on the field. qdev only stores the + * pointer here, so dropping the qualifier for this one call is safe and + * keeps -Wdiscarded-qualifiers quiet. + */ + qdev_init_gpio_out_named(dev, (qemu_irq *)s->row_out, "row", KEYPAD_ROWS); } /* diff --git a/tools/keypad_test.py b/tools/keypad_test.py new file mode 100755 index 0000000..9f7e42d --- /dev/null +++ b/tools/keypad_test.py @@ -0,0 +1,231 @@ +#!/usr/bin/env python3 +"""Check that keypresses reach the firmware and drive the UI. + +Boots its own QEMU instance on private ports, so it does not disturb a running +run.sh session. Three checks: + + 1. a short MENU press opens the menu from the main screen + 2. DOWN moves the menu cursor + 3. a short MENU press still works after power save has engaged + +Why this exists: the keypad was reported broken for a long time and was not. The +cause was always press duration -- key.py held keys for 2500 ms, which the +firmware reads as a long press, and the long-press path does not open the menu. +A later round of "power save blocks the keypad" was the same error plus reading +gKeyReading0 after releasing the key, when it is always KEY_INVALID. + +Two rules this test follows, and any manual probing should too: + + * Never run gdb between presses. Each attach halts the guest and can stretch a + sequence past the 20 s menu timeout, so the UI falls back to the main screen + and later presses go somewhere unintended. + * Read key state while the key is still held, never after release. + +Usage: + tools/keypad_test.py # all checks + tools/keypad_test.py -v # show each step +""" +import argparse +import json +import os +import re +import socket +import subprocess +import sys +import time + +HOME = os.path.expanduser("~") +QEMU = os.environ.get( + "QEMU_BIN", f"{HOME}/qemu-build/qemu-7.2+dfsg/build/qemu-system-arm") +ELF = os.environ.get( + "ELF", f"{HOME}/uvk5-port/uvk5-sat/build/CW/nr7y.cw.elf") +HERE = os.path.dirname(os.path.abspath(__file__)) +FLASH = os.path.join(os.path.dirname(HERE), "assets", "flash.img") + +QMP = "/tmp/uvk5-keypad-test-qmp.sock" +GDB_PORT = "1239" + +# App/misc.c: key_debounce_10ms = 2 (20 ms), key_repeat_delay_10ms = 40 (400 ms). +# SysTick interrupts run at close to real time, so these are wall-clock values. +SHORT_MS = 200 # past debounce, well short of a long press +GAP_MS = 300 # let the release debounce before the next press + +DISPLAY_MAIN = 0 +DISPLAY_MENU = 1 +KEY_INVALID = 19 +FUNCTION_POWER_SAVE = 5 + +GMENUCURSOR_ADDR = "0x20001c94" + + +class Emu: + def __init__(self, verbose=False): + self.verbose = verbose + for path in (QMP,): + if os.path.exists(path): + os.unlink(path) + for path, what in ((QEMU, "QEMU binary"), (ELF, "firmware ELF"), + (FLASH, "flash image")): + if not os.path.exists(path): + sys.exit(f"missing {what}: {path}") + + self.proc = subprocess.Popen( + [QEMU, "-M", f"uv-k5-v3,flash-image={FLASH}", "-nographic", + "-monitor", "none", "-qmp", f"unix:{QMP},server=on,wait=off", + "-kernel", ELF, "-gdb", f"tcp::{GDB_PORT}"], + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + + for _ in range(150): + try: + self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + self.sock.connect(QMP) + break + except OSError: + if self.proc.poll() is not None: + sys.exit("QEMU exited during startup") + time.sleep(0.1) + else: + sys.exit(f"QMP socket never appeared at {QMP}") + + self.buf = b"" + self._read() # greeting + self.cmd("qmp_capabilities") + + def _read(self): + while b"\n" not in self.buf: + chunk = self.sock.recv(4096) + if not chunk: + sys.exit("QEMU closed the QMP connection") + self.buf += chunk + line, self.buf = self.buf.split(b"\n", 1) + return json.loads(line) + + def cmd(self, name, **args): + payload = {"execute": name} + if args: + payload["arguments"] = args + self.sock.sendall(json.dumps(payload).encode() + b"\n") + while True: + msg = self._read() + if "error" in msg: + sys.exit(f"QMP error: {msg['error']}") + if "return" in msg: + return msg["return"] + + def hold(self, key): + self.cmd("qom-set", path="/machine/keypad", property="press", value=key) + + def release(self): + self.cmd("qom-set", path="/machine/keypad", property="press", value="") + + def press(self, key, hold_ms=SHORT_MS, gap_ms=GAP_MS): + self.hold(key) + time.sleep(hold_ms / 1000) + self.release() + time.sleep(gap_ms / 1000) + if self.verbose: + print(f" pressed {key} ({hold_ms} ms)") + + def state(self): + """Read UI state over gdb. Halts the guest, so never call mid-sequence.""" + exprs = [ + ("screen", "*(char*)&gScreenToDisplay"), + ("fn", "*(char*)&gCurrentFunction"), + ("kr0", "*(char*)&gKeyReading0"), + ("cursor", f"*(unsigned char*){GMENUCURSOR_ADDR}"), + ] + args = ["gdb-multiarch", "-batch", "-ex", "set confirm off", + "-ex", "set pagination off", + "-ex", f"target remote :{GDB_PORT}"] + for name, expr in exprs: + args += ["-ex", f'printf "{name}=%d\\n", {expr}'] + args += ["-ex", "detach", "-ex", "quit", ELF] + out = subprocess.run(args, capture_output=True, text=True).stdout + got = {k: int(v) for k, v in re.findall(r"(\w+)=(-?\d+)", out)} + if "screen" not in got: + sys.exit("could not read guest state over gdb") + return got + + def close(self): + self.proc.terminate() + try: + self.proc.wait(timeout=10) + except subprocess.TimeoutExpired: + self.proc.kill() + if os.path.exists(QMP): + os.unlink(QMP) + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument("-v", "--verbose", action="store_true") + args = ap.parse_args() + + emu = Emu(verbose=args.verbose) + failures = [] + try: + # Boot is ~5 s; power save engages ~6 s in and does not block anything, + # so waiting past it makes the test deterministic rather than racy. + time.sleep(16) + + # 1. short MENU press opens the menu, straight out of power save. + # No state read before the press: a gdb attach immediately beforehand + # disturbs the guest enough to lose the press. + emu.press("MENU") + st = emu.state() + if st["screen"] == DISPLAY_MENU: + print("PASS short MENU press opens the menu (also wakes power save)") + else: + failures.append(f"MENU did not open the menu (screen={st['screen']})") + print(f"FAIL MENU did not open the menu (screen={st['screen']})") + + # 2. DOWN moves the cursor. Presses go out back to back with no gdb + # between them, then state is read once. + before = emu.state()["cursor"] + emu.press("DOWN") + emu.press("DOWN") + after = emu.state() + if after["screen"] != DISPLAY_MENU: + failures.append("menu closed during DOWN presses") + print("FAIL menu closed while pressing DOWN") + elif after["cursor"] == (before + 2): + print(f"PASS DOWN moves the cursor ({before} -> {after['cursor']})") + else: + failures.append( + f"cursor moved {before} -> {after['cursor']}, expected +2") + print(f"FAIL cursor {before} -> {after['cursor']}, expected +2") + + # 3. a held key is visible to the firmware *while held*. + # + # Read state mid-hold, then release. Note the read has to come after + # the hold has already been established and the release must follow + # it -- do NOT interleave a gdb read between hold and release when the + # press itself is what you are testing. That attach pauses the guest + # and can stretch the press past the debounce window, which makes a + # working build look broken. Here the press outcome is not under test, + # only whether the scan sees the key, so the read is safe. + emu.hold("MENU") + time.sleep(0.5) + held = emu.state() + emu.release() + time.sleep(GAP_MS / 1000) + if held["kr0"] != KEY_INVALID: + print(f"PASS held key is seen by the scan (gKeyReading0={held['kr0']})") + else: + failures.append("held key not seen by the scan") + print("FAIL held key not seen (gKeyReading0=KEY_INVALID)") + finally: + emu.close() + + print() + if failures: + print(f"{len(failures)} check(s) failed:") + for f in failures: + print(f" - {f}") + return 1 + print("all keypad checks passed") + return 0 + + +if __name__ == "__main__": + sys.exit(main())