Fix the keypad: row_out must be volatile

The previous commit removed three TRACE fprintfs from py32f071.c as
cleanup. That silently broke the keypad completely -- no press reached the
UI, and nothing warned about it.

Root cause is dead-code elimination, not the printing.
qdev_init_gpio_out_named() is inlinable and only records the row_out array;
the lines are filled in later by qdev_connect_gpio_out_named() from board
code, which GCC cannot see. At -O2 GCC therefore 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. No
row line is ever driven and the firmware's scan reads all-high.

From the object code:

  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

keypad_col_changed compiles to a store and a ret with no call at all; with
volatile it ends in jmp keypad_update_rows. Declaring row_out volatile fixes
it at the cause. 10/10 on the press test, 3/3 on keypad_test.py, no build
warnings.

Scoped rather than assumed: PY32GpioState::out is not affected. Marking it
volatile too gives a byte-identical object file, because py32_gpio_write()
is only reachable through a MemoryRegionOps function-pointer table so GCC
cannot enumerate its callers. It stays plain.

Adds tools/keypad_test.py: boots its own instance on private ports and
checks that a short MENU press opens the menu, DOWN moves the cursor, and a
held key is visible to the scan. This is what should have caught the
breakage before it was pushed.

Docs corrected. The breakage had been written up as "power save stops the
keypad scan" and called a gap in the model; it was neither. AGENTS.md now
records the mechanism, the measurements, the objdump check, and the two
measurement traps that made this hard: reading gKeyReading0 after releasing
the key (always KEY_INVALID), and trusting a gdb breakpoint on
KEYBOARD_Poll (with the guest stopped the scan's delays cost no guest time,
so Poll returns KEY_MENU on a build where it fails when running free).

README screenshots regenerated from the current build.
This commit is contained in:
mckero committed 2026-08-27 18:34:41 +01:00
1 parent 1c9a2afe55
commit 2154f80414
6 files changed
+357 -49

No files matched your search

+88 -36
View File
@@ -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
+15 -11
View File
@@ -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
Binary file not shown.

Before

Width:  |  Height:  |  Size: 1.1 KiB

Binary file not shown.

After

Width:  |  Height:  |  Size: 1.1 KiB

+23 -2
View File
@@ -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);
}
/*
+231
View File
@@ -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())