burn: let terminal mode carry keystrokes, not just serial output - #136
Merged
Conversation
`-t` advertised a U-Boot console and delivered a viewer. The terminal-mode loop read the port and wrote the screen, and no code path in the CLI read stdin at all -- on any platform, not just Windows. README.md said "raw terminal passthrough -- type commands directly", which was not true. It cost the reporter in OpenIPC/firmware#2381 an evening. They had a bricked Hi3516CV300, reached a live `OpenIPC #` prompt over the bootrom -- exactly the position from which the flash can be rewritten -- and the prompt answered nothing they typed. The flood of `<INTERRUPT>` on their screen made it look like Ctrl-C spam was eating the keystrokes, which it was not: the break loop is bounded and had finished, and that output is the banner deliberately replayed from post_burn_buffer. The keys were never sent, so there was nothing to eat them. Keys are now polled between serial reads. No thread, no executor, and nothing that can stall the loop -- which during a recovery is also holding the serial link to the board. cbreak keeps ISIG on, so Ctrl-C still exits the terminal as the banner has always promised, and disables local echo, because a serial console echoes what it received and doing both shows every character twice. Windows drops the marker-plus-scan-code pairs a special key produces, which mean nothing to U-Boot and type garbage at the prompt. Where there is no terminal to configure -- Windows, a pipe, stdin captured under pytest -- raw_terminal is a no-op and reading still works, so automation that feeds stdin keeps working and callers need no platform branch.
PR Summary by QodoEnable two-way keyboard input in burn terminal mode
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
test_real_netsh_yields_at_least_one_adapter failed all three windows-latest jobs here, on code identical to what it passed on in #135. It asserted that netsh lists at least one adapter, which is a property of the runner rather than of anything we wrote -- so it passed once, failed the next time, and told us nothing about the parser either way. What it now asserts is ours: netsh is callable, its output parses, and nothing that is not an adapter name comes back. A header row surviving the shape test is the failure mode that would matter, and that shows up regardless of what the host has plugged in. The deterministic tests against a captured table still pin the parsing itself. The failure did expose something real. The synchronous path ignored netsh's exit status, unlike the async twin added in the same PR, so a netsh that failed to run left us parsing empty stdout and reporting "no adapters" -- a command that did not run, presented as a host without a network. It checks the exit status now, and a test pins that.
All three windows-latest jobs failed on TestPosixReading::test_what_was_typed_comes_back with `assert b'' == b'sf probe 0\n'`. Windows `select()` accepts sockets only, so the pipe those tests type into is rejected, the read comes back empty, and the assertion fails. Nothing is wrong: `read_available_keys` dispatches on the platform and `_read_posix` is never reached on Windows, so the tests were forcing a code path that does not run there and failing for the reason it does not. Skipped on Windows, where `_read_windows` and its fake msvcrt already cover the reader that does run. This is the failure the previous commit's netsh flake was hiding: only one test can be the first to fail, and fixing that one let this one surface.
Third Windows failure on this branch, and the third one of mine: UnicodeDecodeError: 'charmap' codec can't decode byte 0x90 in position 29005: character maps to <undefined> `Path.read_text()` with no encoding uses the locale codec, which on the Windows runners is cp1252. src/defib/cli/app.py is UTF-8 and has bytes cp1252 cannot represent, so a test that reads it to check what the code does could not even open it. Reproduced locally with `read_text(encoding="cp1252")`, which gives the identical error. Fixed everywhere it appears rather than only where CI stopped: both of the new doc-vs-code tests, the Makefile the agent-map test reads, and one pre-existing line in test_profiles_usb_recovery.py with the same latent failure. The two in src/defib/profiles/loader.py are a real defect rather than a test artefact. Chip profiles are JSON, which is UTF-8 by definition, so decoding one with the host's locale codec is wrong wherever the locale is not UTF-8 -- today's profiles are ASCII, so nothing has broken yet, and a single non-ASCII byte in one would have broken it only on Windows.
…lence Two findings from the review of this branch. The reader drained stdin for as long as it stayed readable. Someone typing makes it return at once, but a pipe that keeps producing never stops being readable -- and the poll runs inline before each serial read, so the board's output, the stop flag and the transport cleanup would all wait for the producer instead. One poll now collects at most 4 KB, which no one can type and any flood exceeds. raw_terminal discarded every exception from its one attempt to restore the terminal. cbreak leaves the shell with no echo and no line editing, so a failed restore that says nothing hands the operator a terminal that looks dead and no reason for it. TCSADRAIN waits for pending output and so is the half that can fail; it stays the first choice because it does not truncate what the board was printing, TCSANOW is tried next, and if both fail we say so on stderr with the command that fixes it. Still never raises -- the body may already be unwinding with the error that actually matters. The flood test lowers the cap rather than enlarging the write: filling a pipe past its buffer would block on whichever platform has the smallest one, and pipe capacity is not what is under test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
defib burn -tadvertised a U-Boot console and delivered a viewer.The terminal-mode loop read the serial port and wrote the screen, and no code path in the CLI read stdin at all — no
add_reader, notermios, nomsvcrt, on any platform.README.mdsaidraw terminal passthrough — type commands directly, which was not true of the normal U-Boot branch. (The separate download-command branch, thedefib>prompt, does take input, which is probably how it went unnoticed.)It cost the reporter in OpenIPC/firmware#2381 an evening. They had a bricked Hi3516CV300, got a live prompt over the bootrom — exactly the position from which the flash can be rewritten — and it answered nothing they typed:
Worth recording because it misleads: the flood of
OpenIPC # <INTERRUPT>on their screen looks like Ctrl-C spam swallowing the keystrokes. It is not.RecoverySession.runbounds that loop and had already finished, and the output is the banner deliberately replayed frompost_burn_bufferso-b -tdoes not hide it. The keys were never sent, so nothing ate them.The fix
Keys are polled between serial reads, in the loop that was already there:
No thread and no executor — during a recovery that loop is also holding the serial link to the board, and I had just been pulled up in #135 for putting a blocking call on it. Both readers are non-blocking (
selectwith a zero timeout;msvcrt.kbhit).Details that matter:
ISIGstays on, so Ctrl-C still exits the terminal as the banner has always promised. Sending Ctrl-C to U-Boot is therefore still not possible; that is a deliberate trade for not changing the documented exit key, and rarely needed once you are at a prompt.raw_terminalis a no-op and reading still works, so anything feeding stdin keeps working and callers need no platform branch.README.mdnow describes what the code does.Testing
New
tests/test_terminal_keyboard.py, 13 tests: real pipes for the POSIX reader, a fakemsvcrtfor the Windows one, termios save/restore including on an exception, and two tests that pin the regression itself — that the terminal block reads the keyboard and writes it to the transport, and that the README does not promise what the code cannot do.Not verified against a camera — I have no Hi3516CV300 in front of me. The reporter is mid-recovery and has a workaround in the meantime (
burn -bwithout-t, then PuTTY on the same port; the RAM-loaded U-Boot stays at its prompt), so they can confirm this independently.