diff options
Diffstat (limited to 'docs/superpowers/followup')
6 files changed, 277 insertions, 361 deletions
diff --git a/docs/superpowers/followup/pr3803-hil-blindness-reporting.md b/docs/superpowers/followup/pr3803-hil-blindness-reporting.md deleted file mode 100644 index 374ee62c7..000000000 --- a/docs/superpowers/followup/pr3803-hil-blindness-reporting.md +++ /dev/null @@ -1,186 +0,0 @@ -# Blindness Reporting Gaps Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make a HIL worker's sysfs blindness reach the report in the two cases where it -currently does not — an untested producer, and a board that raises. - -**Architecture:** A worker returns `hil_util.sysfs_blind()` as the last field of its result -tuple; `_blind_note()` turns that into a report banner. Two holes: nothing tests the -producer, and a board that raises returns no tuple at all, so its blindness is lost. - -**Tech Stack:** Python 3.13 stdlib, multiprocessing Pool with `maxtasksperchild=1`. - -## Global Constraints - -- A blind worker answers `SYSFS_UNKNOWN` for every attribute, so its "device not found" - means "could not tell". The report must say so or a red cell reads as a broken board. -- `maxtasksperchild=1`: one worker per board, so the flag is per-board and must not be - smeared across boards. -- Tests: `cd test/hil && python3 test/test_hil_bounded.py`. - -## What is already established - -- `hil_test.test_board` returns `(..., hil_util.sysfs_blind(), stray)`; `_blind_note(mret)` - renders the banner; wired into all three report paths. -- **The producer is provably untested**: replacing `hil_util.sysfs_blind()` with `False` in - the return leaves all tests green. Nothing drives `test_board` — it needs a board dict, a - real flock, a flasher and `test_example` per test. -- Blindness fired for real on ci.lan: four workers went blind in one run, and cells failed - *because* of it (`Printer device not found ... (this worker is blind)`). - -**Why this is a separate PR:** closing it means making `test_board` testable, which is a -refactor of the harness's orchestration layer — a different scope from the containment -work, and the reason the gap was accepted rather than papered over. - -## File Structure - -- `test/hil/hil_test.py` — extract the result-tuple assembly from `test_board` so it can be - built and asserted without running a board; carry blindness out of the raise path. -- `test/hil/test/test_hil_bounded.py` — tests for both. - ---- - -### Task 1: Make the result tuple assembly testable - -**Files:** -- Modify: `test/hil/hil_test.py` (`test_board`, the `return (name, err_count, ...)` at the - end of the try block) -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Produces: `_board_result(name, err_count, failed_tests, rows, t_total, board_wide_fail)` - returning the 7-tuple `(name, err_count, failed, rows, t_total, blind, stray)`, reading - `hil_util.sysfs_blind()` and `hil_health.kill_own_children()` itself. - -- [ ] **Step 1: Write the failing test** - -```python -class BoardResultCarriesBlindness(unittest.TestCase): - def test_a_blind_worker_reports_it(self): - from helper import hil_util, hil_health - self.addCleanup(setattr, hil_util, 'sysfs_blind', hil_util.sysfs_blind) - self.addCleanup(setattr, hil_health, 'kill_own_children', hil_health.kill_own_children) - hil_util.sysfs_blind = lambda: True - hil_health.kill_own_children = lambda: 0 - row = hil_test._board_result('b', 0, [], [], 1.0, False) - self.assertTrue(row[5], 'blindness did not reach the result tuple') - self.assertIn('b', hil_test._blind_note([row])) - - def test_a_sighted_worker_does_not(self): - from helper import hil_util, hil_health - self.addCleanup(setattr, hil_util, 'sysfs_blind', hil_util.sysfs_blind) - self.addCleanup(setattr, hil_health, 'kill_own_children', hil_health.kill_own_children) - hil_util.sysfs_blind = lambda: False - hil_health.kill_own_children = lambda: 0 - row = hil_test._board_result('b', 0, [], [], 1.0, False) - self.assertFalse(row[5]) - self.assertEqual(hil_test._blind_note([row]), '') -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cd test/hil && python3 test/test_hil_bounded.py BoardResultCarriesBlindness -v` -Expected: FAIL — `module 'hil_test' has no attribute '_board_result'` - -- [ ] **Step 3: Write minimal implementation** - -```python -def _board_result(name, err_count, failed_tests, rows, t_total, board_wide_fail): - """Assemble a worker's result tuple. Separate from test_board so the two fields only - the WORKER can answer -- its process-global blindness latch and what it could not kill - -- are testable without running a board.""" - stray = hil_health.kill_own_children() - return (name, err_count, [] if board_wide_fail else sorted(set(failed_tests)), - rows, t_total, hil_util.sysfs_blind(), stray) -``` - -Replace the tail of `test_board` with: - -```python - return _board_result(name, err_count, failed_tests, rows, t_total, board_wide_fail) -``` - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cd test/hil && python3 test/test_hil_bounded.py -v` -Expected: PASS, and the existing `BlindWorkerReachesTheReport` tests still pass. - -- [ ] **Step 5: Verify the mutation is now caught** - -Replace `hil_util.sysfs_blind()` with `False` inside `_board_result` and re-run; the suite -MUST fail. Restore it. - -- [ ] **Step 6: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "test/hil: make the worker result tuple testable, covering blindness" -``` - ---- - -### Task 2: Carry blindness out of the worker-raise path - -**Files:** -- Modify: `test/hil/hil_test.py` (`test_board`'s except/finally, and `main`'s worker-raise - handler that builds synthetic rows) -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Consumes: `_board_result` from Task 1. -- Produces: a board that raises still contributes a row whose blindness field is accurate. - -- [ ] **Step 1: Write the failing test** - -```python - def test_a_board_that_raises_still_reports_blindness(self): - """The result tuple is returned inside a try whose finally only releases the lock, - so a board that dies by exception contributed nothing -- and its blindness, the - thing that most explains its failure, was lost with it.""" - from helper import hil_util - self.addCleanup(setattr, hil_util, 'sysfs_blind', hil_util.sysfs_blind) - hil_util.sysfs_blind = lambda: True - row = hil_test._board_result_on_error('b', RuntimeError('boom')) - self.assertTrue(row[5]) - self.assertIn('b', hil_test._blind_note([row])) -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cd test/hil && python3 test/test_hil_bounded.py BoardResultCarriesBlindness -v` -Expected: FAIL — no `_board_result_on_error` - -- [ ] **Step 3: Write minimal implementation** - -```python -def _board_result_on_error(name, exc): - """A row for a board that died by exception. err_count 1, no per-test detail, but the - blindness and stray fields are still accurate -- they explain the failure more often - than the exception text does.""" - rows = [(name, {hil_report.BOUNDARY_CELL: - f'{hil_report.REPORT_CELL["fail"]} {type(exc).__name__}'}, None)] - return _board_result(name, 1, [], rows, 0.0, True) -``` - -Wrap the body of `test_board` so the exception path returns it instead of propagating. - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cd test/hil && python3 test/test_hil_bounded.py -v` -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "test/hil: keep a raising board's blindness in the report" -``` - ---- - -## Caution - -`test_board`'s `finally` releases the board flock. Any restructuring MUST keep that -release on every path, including the new error path — a leaked flock locks the board until -the host reboots. diff --git a/docs/superpowers/followup/pr3803-usbtest-recovery-reserve.md b/docs/superpowers/followup/pr3803-usbtest-recovery-reserve.md deleted file mode 100644 index eb8959520..000000000 --- a/docs/superpowers/followup/pr3803-usbtest-recovery-reserve.md +++ /dev/null @@ -1,175 +0,0 @@ -# usbtest Recovery Reserve Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make the post-hang recovery reserve a derived, asserted property instead of an -accident of four independently-set constants. - -**Architecture:** `hil_test` passes `--budget` and `--outer-timeout` to `usbtest.py`, which -decides at runtime whether a recovery still fits. Today the reserve survives only because -the four numbers happen to line up; nothing ties them together or fails when they stop. - -**Tech Stack:** Python 3.13 stdlib. - -## Global Constraints - -- `usbtest.py`: `RECOVER_FLASH_TIMEOUT = 90`, `RECOVER_RESET_TIMEOUT = 30`. -- `hil_test.py`: `USBTEST_BATTERY_BUDGET = 260`, `USBTEST_RECOVERY_BUDGET = 250`, - `USBTEST_OVERSHOOT = 120`; `outer = BATTERY_BUDGET + (RECOVERY_BUDGET if recovery else - OVERSHOOT)`, used for both the child's `--outer-timeout` and the parent's `run_cmd` bound. -- All five are env-overridable via `hil_util.pos_int_env`, so a rig can change them. -- Tests: `cd test/hil && python3 test/test_hil_health.py` and `test_hil_bounded.py`. - -## What is already established - -The reserve holds at the shipped values, checked by hand: - -- The battery checks its budget BEFORE dispatching a case, so it can overshoot by one - case — worst case `260 + 60 + 5 = 325 s`. -- Recovery is gated on `_time_left() >= RECOVER_RESET_TIMEOUT`, where - `_time_left() = outer_timeout - elapsed - 35`; with `outer = 510` that allows recovery - until `elapsed = 445 s`, and the reflash until `385 s`. -- So ~60 s of margin survives, and recovery does fire. - -**The defect is structural, not arithmetic:** lower `--outer-timeout`, raise `--timeout`, or -raise `USBTEST_BATTERY_BUDGET` via the env and the reserve silently disappears. The failure -mode is a skipped reflash that leaves the D-state holder for the next job — the exact thing -the containment exists to prevent — with no error anywhere. - -**Why this is a separate PR:** it changes the timing contract between `hil_test` and -`usbtest.py`, which affects every board's run duration, so it wants its own review and a -full rig run. - -## File Structure - -- `test/hil/usbtest.py` — a `reserve_ok()` predicate plus a startup assertion. -- `test/hil/hil_test.py` — derive the battery budget from the outer bound rather than - setting both independently. -- `test/hil/test/test_hil_health.py` — tests. - ---- - -### Task 1: Assert the reserve at startup - -**Files:** -- Modify: `test/hil/usbtest.py` (constants block, and `main()` after argparse) -- Test: `test/hil/test/test_hil_health.py` - -**Interfaces:** -- Produces: `usbtest.reserve_ok(budget, outer, case_timeout)` returning bool. - -- [ ] **Step 1: Write the failing test** - -```python -class RecoveryReserveIsChecked(unittest.TestCase): - """The battery may overshoot its budget by ONE already-started case, so the outer bound - must leave room for that overshoot AND a bounded recovery afterwards.""" - - def setUp(self): - import usbtest - self.u = usbtest - - def test_the_shipped_numbers_leave_room(self): - self.assertTrue(self.u.reserve_ok(budget=260, outer=510, case_timeout=60)) - - def test_a_tighter_outer_bound_is_rejected(self): - self.assertFalse(self.u.reserve_ok(budget=260, outer=380, case_timeout=60)) - - def test_a_longer_case_timeout_is_rejected(self): - self.assertFalse(self.u.reserve_ok(budget=260, outer=510, case_timeout=200)) -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cd test/hil && python3 test/test_hil_health.py RecoveryReserveIsChecked -v` -Expected: FAIL — `module 'usbtest' has no attribute 'reserve_ok'` - -- [ ] **Step 3: Write minimal implementation** - -```python -def reserve_ok(budget: int, outer: int, case_timeout: int) -> bool: - """Does `outer` leave room for the battery's worst case AND a bounded recovery? - - The budget is checked BEFORE dispatch, so the battery can run to - `budget + case_timeout + 5` (the +5 is run_case's reap). _time_left() subtracts a - further 35 s of fixed tail. A reflash needs RECOVER_FLASH_TIMEOUT beyond that. - """ - worst_case_end = budget + case_timeout + 5 - return outer - worst_case_end - 35 >= RECOVER_FLASH_TIMEOUT -``` - -In `main()`, after parsing args: - -```python - if args.budget and args.outer_timeout and not reserve_ok( - args.budget, args.outer_timeout, args.timeout): - print(f'warning: --outer-timeout {args.outer_timeout} leaves no room for a bounded ' - f'recovery after a --budget {args.budget} battery with --timeout ' - f'{args.timeout} cases; a HUNG board will be left wedged', file=sys.stderr) -``` - -Warn, do not exit: a caller that deliberately runs without recovery is legitimate. - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cd test/hil && python3 test/test_hil_health.py RecoveryReserveIsChecked -v` -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/usbtest.py test/hil/test/test_hil_health.py -git commit -m "usbtest: check the recovery reserve instead of assuming it" -``` - ---- - -### Task 2: Derive the outer bound from one place - -**Files:** -- Modify: `test/hil/hil_test.py` (constants block ~line 227, and `test_device_usbtest`) -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Consumes: `usbtest.reserve_ok` semantics (duplicate the arithmetic, do not import - usbtest — `hil_test` must not import it). -- Produces: an assertion at module import that the shipped constants satisfy the reserve. - -- [ ] **Step 1: Write the failing test** - -```python - def test_the_shipped_constants_satisfy_the_reserve(self): - """Whatever the env overrides, the pair hil_test computes must leave recovery room: - outer - (budget + case_timeout + 5) - 35 >= 90.""" - outer = hil_test.USBTEST_BATTERY_BUDGET + hil_test.USBTEST_RECOVERY_BUDGET - self.assertGreaterEqual(outer - (hil_test.USBTEST_BATTERY_BUDGET + 60 + 5) - 35, 90) -``` - -- [ ] **Step 2: Run test to verify it fails** - -Temporarily set `HIL_USBTEST_RECOVERY_BUDGET=100` and run; expect FAIL. Unset. - -- [ ] **Step 3: Add the guard** - -```python -# The recovery reserve is a PROPERTY of these two, not a coincidence: the battery may -# overshoot its budget by one already-started case (checked before dispatch), and a bounded -# reflash needs 90 s after a 35 s fixed tail. Env overrides make this checkable at import -# rather than discoverable when a wedge is left unrecovered. -if USBTEST_RECOVERY_BUDGET - 60 - 5 - 35 < 90: - print(f'warning: HIL_USBTEST_RECOVERY_BUDGET={USBTEST_RECOVERY_BUDGET} leaves no room ' - f'for a bounded reflash after a one-case overshoot; HUNG boards will stay wedged', - file=sys.stderr) -``` - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `cd test/hil && python3 test/test_hil_bounded.py -v` -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "hil: warn when the timeout constants leave no recovery reserve" -``` diff --git a/docs/superpowers/followup/pr3840-mret-board-result.md b/docs/superpowers/followup/pr3840-mret-board-result.md index 7b8da7b9c..77b76b605 100644 --- a/docs/superpowers/followup/pr3840-mret-board-result.md +++ b/docs/superpowers/followup/pr3840-mret-board-result.md @@ -3,6 +3,15 @@ **Origin:** split out of PR #3840 (making `hil_report.md` a rendering of `hil_report.json`). Delete this file when its own PR lands. +> **SUPERSEDED IN PART (2026-08-26).** Written against a 7-field tuple whose index 5 was +> `blind`. The sysfs blindness subsystem is gone: `test_board` now returns **6** fields with +> `stray` at index 5, and its board-locked early return is 5 wide. The problem described +> below is unchanged and still worth fixing — three producers, three widths, and +> `len(r) > 5 and r[5]` reads a WRONG SLOT rather than raising. But drop the `blind` field +> from the proposed NamedTuple and re-derive every index from `hil_test.test_board` before +> executing, or `_stray_note` starts reading a duration as a stray count. +> `StrayNoteSurvivesTheTupleWidth` pins the current shape. + ## What is established `test_board()` returns a bare tuple that three producers build and fourteen call sites read diff --git a/docs/superpowers/followup/pr3851-msc-host-tur-retry.md b/docs/superpowers/followup/pr3851-msc-host-tur-retry.md new file mode 100644 index 000000000..462b90fdf --- /dev/null +++ b/docs/superpowers/followup/pr3851-msc-host-tur-retry.md @@ -0,0 +1,149 @@ +# MSC host: bound the Test Unit Ready retry loop and act on sense data + +> Split out of PR #3851 (`etmtrace-rp2350`, rp2350 ETM trace + stock clocks): +> a host-stack MSC bug with no relation to that branch's scope. + +**Goal:** stop `msch_open`'s enumeration retry from spinning forever when a +device answers Test Unit Ready with CHECK CONDITION, and use the sense data the +driver already fetches to decide whether to keep waiting, give up, or report. + +--- + +## What is already established + +### The loop is unbounded, and the source says so + +`src/class/msc/msc_host.c:445-472` is a two-function cycle with no counter: + +```c +static bool config_test_unit_ready_complete(...) { + if (csw->status == 0) { + ... tuh_msc_read_capacity(...); // ready -> proceed to mount + } else { + // Note: During enumeration, some device fails Test Unit Ready and require a few retries + // with Request Sense to start working !! + // TODO limit number of retries <-- :459, pre-existing + TU_LOG_DRV("SCSI Request Sense\r\n"); + TU_ASSERT(tuh_msc_request_sense(dev_addr, cbw->lun, enum_buf, + config_request_sense_complete, 0)); + } + return true; +} + +static bool config_request_sense_complete(...) { + TU_ASSERT(csw->status == 0); + TU_ASSERT(tuh_msc_test_unit_ready(dev_addr, cbw->lun, + config_test_unit_ready_complete, 0)); // :472 + return true; +} +``` + +Two defects, independent of each other: + +1. **No bound.** TUR fail -> Request Sense -> TUR -> ... forever. `tuh_msc_mount_cb()` + is never called and the application is never told anything; the device sits + enumerated-but-unmounted indefinitely. +2. **Sense data is fetched and discarded.** `config_request_sense_complete` + checks only the CSW status. `enum_buf` holds a `scsi_sense_fixed_resp_t` + whose `sense_key` / ASC / ASCQ distinguish "Not Ready — becoming ready" + (retry is correct) from "Not Ready — medium not present" (a card reader with + no card; retrying can never succeed) from a hard error. The driver cannot + currently tell these apart because it never looks. + +### Measured on hardware (2026-08-25) + +Rig: `raspberry_pi_pico` (RP2040) + Pico-PIO-USB host on GP20/21, probe +`E6614103E719612F`, console over the probe's CDC. Build: +`-DCFG_TUH_RPI_PIO_USB=1 -DLOG=2`. + +- `examples/host/msc_file_explorer` never mounts. Debug log over ~25 s: + **1× `SCSI Test Unit Ready`, 350× `SCSI Request Sense`**, zero + `SCSI Read Capacity`, zero mount callbacks. `dd` reports + `no MSC device mounted`. +- **The transfers themselves all succeed** — every CBW/CSW pair logs `OK` + (`Queue EP 02 with 31 bytes ... OK`, `Queue EP 81 with 13 bytes ... OK`), so + this is a SCSI-state-machine problem, not a bulk-transfer or PIO-USB timing + problem. +- Reproduced with **two different drives** (`24a9:1802` "STORAGE DEVICE" and the + drive swapped in after it), so it is not one device's quirk. +- Control transfers on the same target are fine: `examples/host/device_info` + reads full descriptors from the same drive on the same board + (`bcdUSB 0210`, `bMaxPacketSize0 64`, i.e. full-speed). +- **The very same drive mounts and sustains I/O on RP2350** + (`pico2_etm_trace` carrier): `msc_file_explorer` + `dd` returns + `dd: 524288 bytes in 8448 ms = 62 KB/s`. Confirmed by the maintainer at the + bench, so the device is healthy and the "not ready" answer is provoked by + something specific to the RP2040 setup. +- **Bumping Pico-PIO-USB does not fix it.** Retested with upstream HEAD + `5a37a66` (10 commits ahead of the pinned `675543b`, including + `512d3a2` "Place calc_usb_crc16 in RAM like calc_usb_crc5 and the CRC + tables", which looked like a promising RP2040 timing fix, and `cbf055d` + transaction-length clamp) via `-DPICO_PIO_USB_PATH=<clone>`: identical + failure, no mount. +- Clock is **not** a factor: identical failure at 120 MHz, 133 MHz and + 156 MHz on RP2040 (and on RP2350 all of 120/125/126/138/150/156/162/174/186/240 MHz + behave identically). + +### What is NOT established + +- The actual sense key/ASC/ASCQ the failing drives return — the driver never + logs it. **Task 1 below exists to capture it**, and its answer decides whether + a bounded retry is sufficient or a "medium not present" path is also needed. +- **Why the RP2040 setup provokes the not-ready state.** Leading suspect is + VBUS quality rather than firmware: the RP2350 carrier feeds J5 through a + proper load switch, while the RP2040 rig is a bare Pico whose GP22 "VBUS + enable" drives nothing (no load switch on a bare Pico), so the drive is fed + directly off the VBUS pin through hookup wire. A bus-powered drive that + cannot spin up answers exactly this "not ready" forever. Measure VBUS at the + device under load, or retest with a powered hub / self-powered device, + BEFORE attributing the stall to the host stack. +- The actual sense key (Task 1) — still the gate for any policy change. + +--- + +## What remains + +### Task 1: Log the sense response (diagnostic, ship-able on its own) + +**Files:** `src/class/msc/msc_host.c` (`config_request_sense_complete`, ~:467) + +Add a `TU_LOG_DRV` of `sense_key`, `add_sense_code`, `add_sense_qualifier` from +the fixed-format response in `usbh_get_enum_buf()`. `scsi_sense_fixed_resp_t` is +already declared in `src/class/msc/msc.h`. + +Verify on the rig above: rebuild `msc_file_explorer` with `-DLOG=2`, flash, read +the probe CDC, and record the triple. Expected candidates: +`0x02/0x04/0x01` (becoming ready) or `0x02/0x3A/0x00` (medium not present). + +### Task 2: Bound the retry + +**Files:** `src/class/msc/msc_host.c`, `msch_interface_t` (add a retry counter), +`src/class/msc/msc_host.h` (a `CFG_TUH_MSC_TUR_RETRY_COUNT`-style knob with a +sane default; follow the existing `CFG_TUH_MSC_*` naming in +`src/tusb_option.h`). + +On exhaustion, stop the cycle and surface the failure rather than silently +looping — the application currently has no way to learn the device is stuck. + +### Task 3: Decide behaviour per sense key + +Gated on Task 1's measurement. At minimum: keep retrying on "becoming ready", +stop immediately on "medium not present". Do not invent policy for sense keys +that were not observed. + +### Task 4: Regression coverage + +`test/unit-test/` has no MSC host suite today; adding one means mocking +`tuh_msc_*` completions. Confirm with the maintainer whether a unit test or a +HIL case on a known not-ready device (an empty card reader is the cheap +reproducer) is the wanted evidence before building either. + +--- + +## Why it was split out + +Found while sweeping PIO-USB clocks on the `etmtrace-rp2350` branch, which +touches only rp2040/rp2350 clock pinning and ETM trace config. This bug is in +the class-driver layer, affects every MCU running the MSC host, and predates +that branch (the `// TODO limit number of retries` is already in master). It +deserves its own PR and its own hardware evidence. diff --git a/docs/superpowers/followup/pr3853-board-putchar-logger.md b/docs/superpowers/followup/pr3853-board-putchar-logger.md new file mode 100644 index 000000000..46a4417bd --- /dev/null +++ b/docs/superpowers/followup/pr3853-board-putchar-logger.md @@ -0,0 +1,57 @@ +# `board_putchar` is not LOGGER-aware + +**Origin:** surfaced while validating the RTT console in PR #3853 (the `rtt` skill +promotion), which is harness-only scope. This is a src-level fix to `hw/bsp/board.c` +that touches every board/logger combination, so it needs its own build sweep rather +than a drive-by. Delete this file when its own PR lands. + +## Established (with evidence) + +`hw/bsp/board.c` retargets stdio through `sys_write`/`sys_read`, which are compiled +per logger: `SEGGER_RTT_Write`/`SEGGER_RTT_Read` under `LOGGER_RTT`, ITM under +`LOGGER_SWO`, `board_uart_write`/`board_uart_read` by default. The two board-level +character helpers do not agree: + +```c +168: int board_getchar(void) { +169: char c; +170: return (sys_read(0, &c, 1) > 0) ? (int) c : (-1); +171: } +172: +173: int board_putchar(int c) { +174: if (board_uart_write((const char *)&c, 1) > 0) { +``` + +`board_getchar` follows the logger; `board_putchar` always goes to the UART. So with +`LOGGER=rtt` console input arrives over RTT while the echo goes out the UART. + +Measured on ea4088_quickstart (`LOGGER=rtt`, `board_uart_write` is a `-1` stub on +lpc40): the `board_test` echo vanishes entirely while a `printf` echo — same console, +same keystroke — comes back byte-for-byte. `LOGGER=swo` has the same asymmetry by +construction (ITM out of `sys_write`, UART out of `board_putchar`), unverified on +hardware. + +## What remains + +Candidate fix: route `board_putchar` through `sys_write(0, ...)` for symmetry with +`board_getchar`. Two things to settle while doing it: + +- `board_putchar` currently passes `&c` of an `int` to a `const char*` — it writes + the low byte only on little-endian. Narrow to a `char` local as part of the change. +- The default (UART) path must keep its current return contract: `board_uart_write` + returns negative when the UART is a stub, and the default `sys_write` breaks out of + its retry loop on that, returning a short count — so `board_putchar` still has to + map "wrote nothing" to `-1`. + +## Validation + +Build sweep across loggers and families — at minimum one UART board, one +`LOGGER=rtt` board and one `LOGGER=swo` board — plus a hardware check that the +`board_test` echo comes back on an RTT board (ea4088_quickstart reproduces the bug +today) and that a plain UART board's echo is unchanged. + +## Why it was split out + +PR #3853 promotes a debug-tooling skill and touches `test/hil/*.py` and +`tools/rtt.py`. A `hw/bsp/board.c` change lands in every example on every board and +belongs in a review that carries the build evidence for it. diff --git a/docs/superpowers/followup/pr3853-rtt-harness-adoption.md b/docs/superpowers/followup/pr3853-rtt-harness-adoption.md new file mode 100644 index 000000000..8f3eae16b --- /dev/null +++ b/docs/superpowers/followup/pr3853-rtt-harness-adoption.md @@ -0,0 +1,62 @@ +# Follow-up: finish RTT-console adoption in the HIL harness + +Split out of the `rtt` skill-promotion PR #3853. That PR deliberately ships the skill + CLI and leaves the harness's remaining +VCOM assumptions in place — converting them is separate test-infra scope that +deserves its own review and HIL runs. Scope here is `test/hil/*.py` only; the +src-level `board_putchar` asymmetry this work surfaced has its own handoff +(`pr3853-board-putchar-logger.md`). + +## Established (with evidence) + +- `hil_util.JlinkRtt` + `open_board_console()` work end-to-end: + ea4088_quickstart runs its host suite over RTT (16 passed / 0 failed / 3 + skipped, the 'hil: read the host console over RTT when the probe has no VCOM' commit), and the `rtt` skill's boards.md carries the + validated matrix. +- `test_host_device_info` honors `"logger": "rtt"` (hil_test.py, `test_host_device_info`; the eof fail-fast assert sits in its read loop): + in RTT mode it resets via the flasher BEFORE opening the console (which + then owns the probe; Commander delivers the buffered boot burst) and its + read loop fails fast on `JlinkRtt.eof` instead of blaming the board. + +## Remaining gaps + +1. **`test_host_cdc_msc_hid` and `test_host_msc_file_explorer` (hil_test.py) still call `hil_util.get_serial_dev(flasher["uid"], ...)` + directly** — on a `logger: rtt` board with `is_cdc`/`is_msc` fixtures they + would fail with the same "No serial device found" the console work fixed + for device_info (an interim load-time gate in `hil_test.py` now rejects + that combination up front; delete the gate when this lands). Fix: route + both through `open_board_console(board)` — but design the conversion + reset-aware rather than hand-copying device_info's dual branch: hoist a + `reset=` parameter into `open_board_console` that does the per-console + ordering itself (RTT: reset via flasher BEFORE opening — the console owns + the probe; VCOM: reset after open to catch the banner), and REMOVE the + existing post-open `# reset device to catch mount messages` blocks in both + tests (grep the marker — line numbers churn) — kept as-is on an RTT board they reset + while the console holds the probe. `JlinkRtt` carries input for their + menus and implements the `reset_input_buffer()` those tests call. +2. **`hil_pool_check.check_host_serial` carries its own inline RTT branch** + (reset → `JlinkRtt` → poll through `hil_util.strip_banner`) — RTT boards + ARE health-checkable today, but the console-opening logic now lives in + two places (`open_board_console` in hil_test.py and this branch), each + with its own reset-ordering. Fix: hoist `open_board_console()` into + `hil_util.py` with the `reset=` parameter from item 1 and collapse + pool_check's branch onto it; keep the `do_reset` flush semantics for the + VCOM path intact. +3. **OpenOCD console backend in the harness**: the skill's CLI + (`tools/rtt.py --backend openocd`, class + `OpenocdRtt` in the same module) is built, deduplicated behind a shared + base class next to `JlinkRtt` in `tools/rtt.py`, re-exported by + `hil_util`, and hardware-validated (all 20 rig boards through the CLI on + both backends, incl. the 8 native-probe ones). What remains is only the + `open_board_console` plumbing: choosing `OpenocdRtt` for a + `"logger": "rtt"` board with an openocd/stlink flasher needs the per-test + flashed-ELF path (for the control-block address) and, for stlink + flashers, an openocd target-cfg mapping the roster doesn't carry — until + then the config-load gate keeps rejecting non-jlink rtt boards. + +## Validation for this follow-up + +Run the ea4088 local host suite (a board with a `is_cdc`+`is_msc` capable +device attached to J3, or the rig's frdm_k64f/mimxrt1064 with a temporary +`logger: rtt` entry) so cdc_msc_hid and msc_file_explorer actually execute +over RTT; then a `hil_pool_check.py` pass on a no-VCOM board. Delete this doc +when the follow-up PR lands. |