Skip to content

UCI handshake: DATA_ACC is an accept, not a per-byte advance — three defects it causes #144

Closed

Description

@JC-000

Filing so the UCI adapters can align. c64-wireguard found and fixed three handshake defects that all trace to one misunderstanding of the Command Interface. c64-https's adapter shares the ancestry, so it likely shares some of these.

Verified against Gideon's own 6502 client, the Register API doc, and the FPGA source; the fixes are hardware-verified on a U64 Elite (fw 3.15).

The misunderstanding

We treated control bit 1 ($02, which we call UCI_CTRL_NEXT_DATA) as "advance to the next byte". It is not. It is DATA_ACC — the data-accept handshake that ends a transfer. Register API v1.1 §2.4.1, verbatim:

"DATA_ACC: Writing a '1' to this register bit tells the communication layer that all data from the Ultimate was accepted. … Writing to this bit also causes the transfer of the data/status queues to be aborted and reset. Thus, the data response and status response queues will be empty after writing this bit."

Reads advance the pointers by themselves — command_protocol.vhd, on the C64 read strobe:

when c_cif_slot_response =>  response_pointer <= response_pointer + 1;
when c_cif_slot_status   =>  status_pointer   <= status_pointer   + 1;

The canonical sequence (Gideon's software/6502/unsorted/uci_wedge.s, and every wrapper in the reference C lib) is:

push command -> read response until DATA_AV clears -> read status until STAT_AV clears -> ONE DATA_ACC

Nothing is written to $DF1C between the two reads. Both queues are live simultaneously in the data state, and the single accept releases both.

The three defects

1. Per-byte accept flushes the queue. Our drain loops read a byte then pulsed $02. Because that resets both queues, we captured exactly one byte and discarded the rest. It presented as "the firmware only sends one status character".

2. The accept is mandatory on every transaction — including writes. It returns the state machine to idle. Without it the next PUSH_CMD hits else error_busy <= '1' and is silently dropped. Worse, command-byte writes are not state-gated: a rejected command still advances command_pointer, and only the Ultimate resets it — so one missing accept corrupts every subsequent command. In our tree SOCKET_WRITE returns no response payload, so its path skipped the response drain and its only accept was hiding inside the status drain; removing the per-byte pulse silently removed it. Worth checking your write path specifically.

3. Status read after the accept reads nothing. On most of our paths the response drain ran first and its pulse destroyed the status line before anything read it. Status must be read before the accept.

What we changed

  • Removed per-byte $02 writes from both drain loops; reads auto-advance.
  • Exactly one uci_ack per transaction, on every exit path (12 paths, 11 sites).
  • Status drained (and captured) before the accept.
  • Added the missing accept to the SOCKET_WRITE path.

Hardware-verified: repeated UDP echo round-trips pass, 893-byte reads arrive whole, and the status line now reads in full — 04,DATAGRAM TRUNCATED: 1420 where before we saw a single 0.

Two things worth knowing

The comment is the trap. Ours said UCI_CTRL_NEXT_DATA = $02 ; advance response data FIFO — wrong on both counts, and it propagated. Check uci_regs.inc for the same wording.

A too-long command is silently corrupted. The command buffer has the identical end-of-buffer guard as the response buffer: writing more than 896 command bytes overwrites byte 895 repeatedly and command_length saturates, with no rejection. Relevant to any chunked SOCKET_WRITE.

Also: multi-block reads just landed upstream

GideonZ/1541ultimate#806 (merged 2026-08-27) returns UDP datagrams up to 1472 bytes by splitting the reply across Data More blocks. The gateware always supported this; only the software side was missing. If c64-https ever grows a UDP path, the read cap is no longer 893.

Cross-referenced from GideonZ/1541ultimate#802 and c64-wireguard#46.

Activity

  1. added 2 commits that reference this issue on Aug 29, 2026
  2. JC-000 commented on Sep 5, 2026

    @JC-000
    OwnerAuthor

    Re-verified against master at dc06095. All three defects are addressed in
    this tree
    , so this closes with a zero diff. Evidence per item, read out of the
    current sources rather than from the PR that claimed them.

    Item 1 — per-byte accept flushes the queue

    Fixed in be698e7 (PR #157, merged 2026-08-31). uci_drain_resp no longer
    writes $02 per byte; src/net/uci/uci_cmd.s:452-456 carries the reason at the
    site. The only UCI_CTRL_DATA_ACC write anywhere in the tree is inside
    uci_ack:

    $ grep -rn 'UCI_CTRL_DATA_ACC' src/
    src/net/uci/uci_regs.inc:52:UCI_CTRL_DATA_ACC = $02   ; accept + end transfer (resets both queues)
    src/net/uci/uci_cmd.s:369:        lda #UCI_CTRL_DATA_ACC
    

    The other three sta UCI_CONTROL sites in uci_cmd.s are ABORT (:71),
    PUSH_CMD (:291) and CLR_ERR (:330) — none is an accept.

    Pinned behaviourally by tools/test_uci_data_acc.py, which executes the
    shipped uci_drain_resp / uci_drain_status / uci_ack bytes out of the PRG
    on a 6502 interpreter against a model of the Command Interface built from the
    Register API §2.4.1 quote and command_protocol.vhd. It was recorded red
    against the pre-fix PRG (1bb5fad9…, on the "wrote DATA_ACC 1 time(s)"
    assertion). It runs today: pytest -q tools/test_uci_data_acc.py → 3 passed
    against a BACKEND=uci USE_NISTCURVES_ONCHIP=1 build
    (5f9e9fb9e70edd2a15ab2249120f161d1826262140fa6ce6f3e01410a876b474). Since
    #165/1ace6bc a missing UCI build there is a loud failure, not a silent pass,
    so that green is not vacuous.

    Item 2 — the accept is mandatory on every transaction, including writes

    net.s has 11 jsr uci_ack sites, and every one is preceded, in order, by
    uci_drain_resp then uci_drain_status:

    $ grep -n 'jsr uci_drain_resp\|jsr uci_drain_status\|jsr uci_ack' src/net/uci/net.s
    247/249/251   284/286/288   303/305/307   433/435/437   503/505/507
    529/531/533   646/648/650   669/673/675   827/829/831   846/848/850
    939/941/943
    

    The two the issue singles out are the SOCKET_WRITE pair inside net_tcp_send:
    :827-831 (the UCI_ERR_SEND_FAIL path) and :846-850 (the normal path, after
    uci_read_resp_bytes collects the 2-byte written count). So the write path has
    its own accept and never depended on one hiding inside the status drain — the
    specific way removing the per-byte pulse bites, which is why #157 could remove
    it safely.

    The exits that reach rts without an accept are the wall-clock timeout paths
    (@sb_err_drain_to, @sb_ok_drain_to, and uci_push_wait's bail) — C=1 with
    UCI_ERR_WAIT_TIMEOUT already set. Those are a wedged FPGA, not a completed
    transaction.

    Item 3 — status must be read before the accept

    Same grep: the order is drain_resp → drain_status → ack at all 11 sites,
    never ack before drain_status. uci_drain_status captures rather than
    discards (#147, c2e3bf1) into uci_status_buf / uci_status_seen, and
    uci_cmd.s:28-29 states the invariant at the top of the file:

    DATA_ACC mid-drain resets BOTH of them (issue #144). Every transaction is
    drain_resp -> drain_status -> uci_ack, in that order, exactly once.

    "The comment is the trap"

    Also closed. UCI_CTRL_NEXT_DATA ; advance response data FIFO — the exact
    wording flagged here — is gone; src/net/uci/uci_regs.inc:51-52 now reads
    UCI_CTRL_DATA_ACC = $02 ; accept + end transfer (resets both queues) with a
    note naming the old name and this issue.

    "A too-long command is silently corrupted"

    Not reachable in this tree: net_tcp_send chunks at
    UCI_DATA_QUEUE_MAX = 800 (uci_errors.inc:73, applied at net.s:737-752),
    below the 896-byte command buffer, so the saturating end-of-buffer guard is
    never approached. The read-side cap lore (893, and why never exactly 894) is
    recorded at uci_errors.inc:80-95.

    One residual, deliberately not held against this issue

    #157 halved the inter-access gap in the drain loop (it used to carry two
    uci_fences between the response read and the next status read; it now carries
    one, matching the hardware-verified uci_drain_status). That is a register
    timing
    question, not a queue-semantics one — it is none of this issue's three
    items, and the FPGA model in test_uci_data_acc.py cannot speak to it. It is
    documented in #157's own body along with the pass/fail oracle for a hardware
    run; if it needs tracking it belongs in a fresh issue against the fence floor,
    not here.

    Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions