fix(uci): bounded uci_wait_idle + connect short-read detection + Labels.from_file parser convergence - #38
Merged
Conversation
5 UCI test scripts had bespoke `_load_labels()` parsers that called
`parts[1].split(":", 1)`, assuming every line in build/labels.txt has
the form `al C:XXXX .name`. The Makefile sed only rewrites 4-digit
addresses with leading zeros; raw 6-digit ld65 emissions (e.g.
`al 022100 .REU_OVERLAY_P256`) survive verbatim and crash the parser
with `ValueError: not enough values to unpack`.
The harness's `Labels.from_file()` already handles both forms via
`r"al\s+(?:C:)?([0-9a-fA-F]+)\s+\.(\S+)"` and is used by the other
two UCI scripts (`test_https_local`, `bench_ecdsa_u64e`) — converge
on it as the single source of truth.
Affected: phase3_tcp_echo (crashed), test_http_local (crashed),
test_http_live, _analyze_ecdsa_trace, phase2_check (latent — would
have crashed when any new ≥0x10000 symbol landed). phase2_check
preserves its custom `KeyError` message via an explicit guard.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Two related defects on the UCI/U64E TCP path that together turned a non-deterministic firmware OPEN_TCP failure into a 600s sentinel hang in tools/uci/test_https_local. Triaged in c64-test-harness#90. #36 — net_tcp_connect masked OPEN_TCP short-read as success Read the firmware response into uci_socket_id but unconditionally set net_tcp_state = UCI_TCP_CONNECTED regardless of how many bytes came back. A short/empty response left uci_socket_id = 0, the C64 proceeded to the TLS layer, and the ClientHello pushed bytes via SOCKET_WRITE on a phantom socket the firmware never opened. Fix: pre-zero uci_socket_id before the read so a short-read leaves a known sentinel; validate uci_resp_count != 0 AND uci_socket_id != 0 before committing CONNECTED. On failure: net_last_error = UCI_ERR_NO_SOCKET, net_tcp_state = UCI_TCP_CONNECT_FAIL, return C=1. All three existing call sites (src/http.s:87, :680; src/boot.s:465) already check carry, no caller updates needed. #37 — uci_wait_idle was an unbounded spin Spun forever waiting for UCI_STATUS to clear, so a wedged FPGA or the phantom-socket retry loop above became a 600s harness sentinel timeout instead of a clean error. Fix: wall-clock-bounded via CIA1 TOD ($DC08-$DC0B) per the CLAUDE.md "bounded timeouts must use wall-clock time" design note. TOD ticks at 10 Hz independent of CPU turbo, which is why a cycle-counted budget breaks at 48 MHz (cf. abandoned feat/net-drain-abi). 5-second budget; on exhaustion: net_last_error = UCI_ERR_WAIT_TIMEOUT, C=1. Read order is HOUR (latch) → MIN → SEC → TENTHS (unlatch); the SMC state bytes inside the routine match the file's no-ZP convention. All 4 callers (net_dhcp_acquire, net_tcp_connect, net_tcp_send, net_tcp_close) gained `bcs` bails to surface the timeout. Verified on U64E @ 10.43.23.81: test_https_local PASS 79.9s (full TLS handshake, HTTP 200, body OK — happy path uci_socket_id=0x07); test_https_print_body PASS 84.9s; phase2_check PASS; phase3_tcp_echo PASS. Both new error paths surface as immediate C=1 with a populated net_last_error byte. Fixes#36Fixes#37 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promote the "bounded timeouts must use wall-clock time" design note from "future work" to "implemented for uci_wait_idle, copy this pattern" — TOD read order (HOUR latch → MIN → SEC → TENTHS unlatch), 5 s budget, C=1 + UCI_ERR_WAIT_TIMEOUT on bail. Note that uci_push_wait and uci_end_cmd are still unbounded and should follow the same template when a wedge is observed. Add a new "UCI error codes" subsection enumerating the values surfaced via net_last_error and net_tcp_state — UCI_ERR_NO_SOCKET (short-read OPEN_TCP / phantom socket), UCI_ERR_WAIT_TIMEOUT, UCI_TCP_CONNECT_FAIL — with cross-refs to issues #36 and #37. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Three independent improvements uncovered while triaging widespread test-suite failures (c64-test-harness#90 → routed here as #36/#37):
fix(uci)— bounduci_wait_idleto a 5 s wall-clock budget via CIA1 TOD, and detectnet_tcp_connectshort-read on the OPEN_TCP response so a phantom socket is rejected withC=1instead of being silently committed to the TLS layer. Together these convert a non-deterministic firmware OPEN_TCP failure from a 600 s sentinel hang into an immediate, well-typed error.chore(tools/uci)— converge 5 UCI test scripts onto the harness'sLabels.from_file()instead of bespoke parsers that crash on raw 6-digit ld65 labels (al 022100 .REU_OVERLAY_P256and friends). Two were crashing today; three were latent.docs(uci)— promote the "wall-clock timeouts only" design note from "future work" to "implemented foruci_wait_idle, copy this template" and document the newUCI_ERR_*/UCI_TCP_CONNECT_FAILcodes.Fixes#36
Fixes#37
Detail
#36 —
net_tcp_connectunconditional CONNECTED on short-readsrc/net/uci/net.spreviously setnet_tcp_state = UCI_TCP_CONNECTED (0x01)regardless of whetheruci_read_resp_bytesreturned a real socket-id byte. A short response leftuci_socket_id = 0, the C64 proceeded to TLS, and the ClientHello was pushed via SOCKET_WRITE on a phantom socket.Fix: pre-zero
uci_socket_id, validateuci_resp_count != 0 AND uci_socket_id != 0post-read, on failure setnet_last_error = UCI_ERR_NO_SOCKET+net_tcp_state = UCI_TCP_CONNECT_FAILand returnC=1. All three existing call sites (src/http.s:87,src/http.s:680,src/boot.s:465) already check carry — no caller updates needed.#37 —
uci_wait_idleunbounded spinThe spin-wait at
src/net/uci/uci_cmd.s:69-76had no bound, so a wedged FPGA or the phantom-socket retry loop above became a 600 s harness sentinel timeout instead of a clean error.Fix: wall-clock-bounded via CIA1 TOD (
$DC08-$DC0B) per the CLAUDE.md design note. TOD ticks at 10 Hz independent of CPU turbo, which is why a cycle-counted budget breaks at 48 MHz (cf. abandonedfeat/net-drain-abibranch). Read order is HOUR (latch) → MIN → SEC → TENTHS (unlatch); state lives in two SMC bytes inside the routine to match the file's no-ZP convention. 5 s budget; on exhaustion:net_last_error = UCI_ERR_WAIT_TIMEOUT,C=1. All four callers (net_dhcp_acquire,net_tcp_connect,net_tcp_send,net_tcp_close) gainedbcsbails to surface the timeout.Labels-parser convergence
5 UCI scripts had bespoke
_load_labels()doingparts[1].split(":", 1), assuming every line inbuild/labels.txtwasal C:XXXX .name. The Makefile sed rewrites only 4-digit-with-leading-zeros addresses; raw 6-digit ld65 emissions (REU overlay symbols at0x22100,0x24100) survive verbatim and crash the parser. The harness'sLabels.from_file()regexr"al\s+(?:C:)?([0-9a-fA-F]+)\s+\.(\S+)"handles both — adopt it as the single source of truth.phase2_check.load_label(name)preserves its customKeyErrormessage via an explicit guard.Test plan
make BACKEND=uci— clean buildtools/uci/test_https_local.py— PASS in 79.9 s (full TLS 1.3 handshake, HTTP 200, bodyHELLO FROM TLS SERVER, happy-pathuci_socket_id=0x07). First-ever pass for this test.tools/uci/test_https_print_body.py— PASS in 84.9 s (no regression on the previously-passing sibling)tools/uci/phase2_check.py— PASS (DHCP/IP readback)tools/uci/phase3_tcp_echo.py— PASS (echo roundtrip)tools/uci/test_http_local.py— PASS (was crashing on labels parser pre-fix)tools/run_all_tests.py(full VICE suite) — 264/264 PASS (verified separately; no UCI changes touch the ip65 path)UCI suite: was 2/6 → now 6/6. VICE suite: 264/264. Both new error paths (NO_SOCKET, WAIT_TIMEOUT) verified to surface as immediate
C=1with populatednet_last_error.Risk
uci_wait_idlecallers innet.snow have newbcsbail paths that didn't exist before. Lint of those error paths in the assembly is the main review surface.uci_push_waitanduci_end_cmdare still unbounded — same wedge pattern would still trip there. They're left for a follow-up because no wedge has been observed there yet.