net_tcp_connect (4 exits) and net_tcp_close (1) leave the UCI interface mid-transaction on a timeout, bricking it until reboot
Summary
Sibling of #194, deliberately scoped out of its fix. #194 closes three net_tcp_send timeout exits that returned without clearing the command interface. The same defect exists, unfixed, at five more sites:
net_tcp_connect — four structurally identical dirty exits: the entry uci_wait_idle bail, the uci_push_wait bail, @tc_err_drain_to, and @tc_ok_drain_to.net_tcp_close — @cl_drain_to.
All in src/net/uci/net.s.
Why these were left out of #194, and why that was right
#194 was scoped to the $86 mechanism, and for that purpose the scope is exactly correct — verified during its adversarial review, against the firmware source rather than inference:
- Only
net_poll reaches PUSH_CMD via uci_wait_not_busy (mask $01). net_tcp_send, net_tcp_connect and net_tcp_close all open with uci_wait_idle (mask $31), so a left-open data phase blocks them rather than producing a rejected push.
- Only a
net_tcp_send bail leaves net_tcp_state at NET_TCP_CONNECTED, so polling continues afterwards and meets the dirty interface.
So connect/close cannot produce $86. That is why they are not part of #194 and should not be retro-fitted into it.
Why they still matter
They can brick the interface until reboot. A @tc_ok_drain_to bail leaves the interface in state "10" with the data phase open. Every subsequent net_tcp_connect then burns its full 5 s in uci_wait_idle and returns CONNECT_FAIL — permanently, for the rest of the session. Before #194, uci_abort had exactly one call site in the entire codebase (net_init), so the only thing that ever cleared a mid-transaction bail was the next boot.
The user-visible shape is "the machine connects fine, then stops being able to connect at all, and a reset fixes it" — which is easy to misfile as a device wedge. This project has a documented history of misdiagnosing interface-level faults as firmware corruption, so a mechanism that produces exactly that signature is worth closing.
Suggested fix
@sb_bail from #194 is the ready-made remedy: write ABORT, then a bounded uci_wait_idle, carry discarded (the expiring wait has already set net_last_error = $89). Route all five exits to an equivalent shared bail.
Notes carried over from #194's review, so they are not re-derived:
- ABORT, not DATA_ACC. DATA_ACC is gated on
state(1) = '1' in command_protocol.vhd:163, so it is a no-op on a push-wait exit (state "01"). The abort bit sets handshake_in(2) unconditionally (:168-170); the FreeRTOS task calls target->abort() and writes HANDSHAKE_RESET (0x87, command_intf.h:45), whose bit 7 hits the state <= "00" arm (:227-232).
- ABORT does not close sockets.
NetworkTarget::abort() (network_target.cc:494) only calls discard_read_reply(); close_all_sockets() is reached solely from c64_reset(). No socket desync, fw #814's socket table unaffected.
HANDSHAKE_RESET also resets command_pointer, which DATA_ACC does not.uci_abort's own 32-iteration settle (~160 cycles) is far shorter than one uci_fence (~5,450 cycles at OUTER=5 × INNER=217), so it cannot cover a FreeRTOS round trip on its own — the bounded uci_wait_idle after it is load-bearing.
Known limitation the fix will inherit
From #194's review: the abort is serviced by the same FreeRTOS "UCI Server" task that services commands (command_intf.cc:136-143). If that task is blocked in lwip_send — the likeliest reason the wait expired in the first place — the ABORT cannot be actioned either, the follow-up uci_wait_idle also expires, and the interface is still dirty with ~5 s more spent. This class of fix helps transient slowness only. Worth stating in the fix rather than discovering later.
Also inherited: each patched exit adds up to one extra bounded 5 s wait to a failure path. In net_tcp_send that was measured as one extra wait per call (the bail exits the routine, not the chunk loop), with no caller retrying. Confirm the same holds for connect/close before assuming it.
Cost estimate
#194 cost +1 byte of UCI_CODE for three exits sharing one bail, with every other segment byte-identical and all four ip65 PRGs unchanged. Five more exits sharing the same bail should be comparably cheap, but measure it: ip65 is unaffected (nothing outside src/net/uci/ moves), while UCI NET_CODE tail free was 365 B after #194.
Testing
tools/test_uci_timeout_recovery.py (added by #194) runs the shipped 6502 bytes against a model of $DF1B-$DF1F built from command_protocol.vhd and command_intf.cc, with a delayed-response firmware and a running CIA1 TOD, so a recovery that aborts without waiting does not pass by accident. Extend it rather than writing a second harness. Note its documented limits: the model's "5 s" budget is really ~50 loop iterations, and it proves the code waits at all rather than that a real abort round-trip fits in the bound.
These paths are not reachable on demand on hardware, so a deterministic test is the primary evidence and any hardware run is happy-path confirmation only.
net_tcp_connect(4 exits) andnet_tcp_close(1) leave the UCI interface mid-transaction on a timeout, bricking it until rebootSummary
Sibling of #194, deliberately scoped out of its fix. #194 closes three
net_tcp_sendtimeout exits that returned without clearing the command interface. The same defect exists, unfixed, at five more sites:net_tcp_connect— four structurally identical dirty exits: the entryuci_wait_idlebail, theuci_push_waitbail,@tc_err_drain_to, and@tc_ok_drain_to.net_tcp_close—@cl_drain_to.All in
src/net/uci/net.s.Why these were left out of #194, and why that was right
#194 was scoped to the
$86mechanism, and for that purpose the scope is exactly correct — verified during its adversarial review, against the firmware source rather than inference:net_pollreachesPUSH_CMDviauci_wait_not_busy(mask$01).net_tcp_send,net_tcp_connectandnet_tcp_closeall open withuci_wait_idle(mask$31), so a left-open data phase blocks them rather than producing a rejected push.net_tcp_sendbail leavesnet_tcp_stateatNET_TCP_CONNECTED, so polling continues afterwards and meets the dirty interface.So connect/close cannot produce
$86. That is why they are not part of #194 and should not be retro-fitted into it.Why they still matter
They can brick the interface until reboot. A
@tc_ok_drain_tobail leaves the interface in state"10"with the data phase open. Every subsequentnet_tcp_connectthen burns its full 5 s inuci_wait_idleand returnsCONNECT_FAIL— permanently, for the rest of the session. Before #194,uci_aborthad exactly one call site in the entire codebase (net_init), so the only thing that ever cleared a mid-transaction bail was the next boot.The user-visible shape is "the machine connects fine, then stops being able to connect at all, and a reset fixes it" — which is easy to misfile as a device wedge. This project has a documented history of misdiagnosing interface-level faults as firmware corruption, so a mechanism that produces exactly that signature is worth closing.
Suggested fix
@sb_bailfrom #194 is the ready-made remedy: write ABORT, then a boundeduci_wait_idle, carry discarded (the expiring wait has already setnet_last_error = $89). Route all five exits to an equivalent shared bail.Notes carried over from #194's review, so they are not re-derived:
state(1) = '1'incommand_protocol.vhd:163, so it is a no-op on a push-wait exit (state"01"). The abort bit setshandshake_in(2)unconditionally (:168-170); the FreeRTOS task callstarget->abort()and writesHANDSHAKE_RESET(0x87,command_intf.h:45), whose bit 7 hits thestate <= "00"arm (:227-232).NetworkTarget::abort()(network_target.cc:494) only callsdiscard_read_reply();close_all_sockets()is reached solely fromc64_reset(). No socket desync, fw #814's socket table unaffected.HANDSHAKE_RESETalso resetscommand_pointer, which DATA_ACC does not.uci_abort's own 32-iteration settle (~160 cycles) is far shorter than oneuci_fence(~5,450 cycles at OUTER=5 × INNER=217), so it cannot cover a FreeRTOS round trip on its own — the boundeduci_wait_idleafter it is load-bearing.Known limitation the fix will inherit
From #194's review: the abort is serviced by the same FreeRTOS "UCI Server" task that services commands (
command_intf.cc:136-143). If that task is blocked inlwip_send— the likeliest reason the wait expired in the first place — the ABORT cannot be actioned either, the follow-upuci_wait_idlealso expires, and the interface is still dirty with ~5 s more spent. This class of fix helps transient slowness only. Worth stating in the fix rather than discovering later.Also inherited: each patched exit adds up to one extra bounded 5 s wait to a failure path. In
net_tcp_sendthat was measured as one extra wait per call (the bail exits the routine, not the chunk loop), with no caller retrying. Confirm the same holds for connect/close before assuming it.Cost estimate
#194 cost +1 byte of
UCI_CODEfor three exits sharing one bail, with every other segment byte-identical and all four ip65 PRGs unchanged. Five more exits sharing the same bail should be comparably cheap, but measure it: ip65 is unaffected (nothing outsidesrc/net/uci/moves), while UCINET_CODEtail free was 365 B after #194.Testing
tools/test_uci_timeout_recovery.py(added by #194) runs the shipped 6502 bytes against a model of$DF1B-$DF1Fbuilt fromcommand_protocol.vhdandcommand_intf.cc, with a delayed-response firmware and a running CIA1 TOD, so a recovery that aborts without waiting does not pass by accident. Extend it rather than writing a second harness. Note its documented limits: the model's "5 s" budget is really ~50 loop iterations, and it proves the code waits at all rather than that a real abort round-trip fits in the bound.These paths are not reachable on demand on hardware, so a deterministic test is the primary evidence and any hardware run is happy-path confirmation only.