diff --git a/harness/mame/run-vidstream.sh b/harness/mame/run-vidstream.sh index 4fd7e7d..48e7c19 100755 --- a/harness/mame/run-vidstream.sh +++ b/harness/mame/run-vidstream.sh @@ -57,6 +57,14 @@ if [ -f "$ENVFILE" ]; then fi mkdir -p "$LOGDIR" +# Ruling R15: draw-mode evidence must come from THIS run. The Lua removes its +# own dump files at startup; removing them here as well means a run that dies +# before the script starts (or before it dumps) leaves no previous screen +# behind for vidstream_check.py to bless. +if [ "$MODE" = "draw" ]; then + rm -f "$LOGDIR/vram-draw.bin" "$LOGDIR/frame-draw.bin" \ + "$LOGDIR/draw-status.txt" +fi # Absolute: the Lua runs with MAME's cwd, so a relative default only resolves # when the caller happens to be at the repo root. VID_PAYLOAD="$ROOT/harness/udpstream/build/vidstream.bin" diff --git a/harness/mame/vidstream.lua b/harness/mame/vidstream.lua index 86bb40c..5e12a70 100644 --- a/harness/mame/vidstream.lua +++ b/harness/mame/vidstream.lua @@ -19,7 +19,7 @@ -- of POOL_FREE (before) marks the fallback, i.e. the payload keeps running. -- -- Mode "rx" (Task 4) is "alloc" plus the receive path: it writes a VALID frame --- into slot A and the same frame with one payload byte flipped into slot B, +-- into slot A and the same frame with one header byte flipped into slot B, -- raises each slot's ready flag (the role the LANCE receive interrupt plays on -- the real transport), and asserts RX_OK == 1 && RX_BAD == 1, that RX_LAST_LEN -- is the accepted frame's payload_len, that RX_SLOT names slot A, and that the @@ -55,6 +55,9 @@ -- VID_SENTINEL_CLOBBER=1 draw mode only: overwrite the VRAM canary word -- before the frame is announced, so the payload must count -- an OS touch. The negative control for OS_TOUCH. +-- VID_DRAW_WAIT draw mode only: emulated frames to wait for the payload to +-- accept the frame (default 300). Set it to 1 to exercise +-- the timeout path. -- VID_PAYLOAD raw .text image (default harness/udpstream/build/vidstream.bin) -- VID_ORIGIN payload load (poke) bit address (default 0x018C0000) -- VID_ENTRY_PTR _start bit address from vidstream.env (default VID_ORIGIN) @@ -146,7 +149,8 @@ end -- CCITT-FALSE (polynomial 0x1021, init 0xFFFF, not reflected, no final xor). -- frame.py, frame_parse.c and this loop are three independent readings of the -- same spec; the harness signs its own frames rather than reusing a value the --- payload computed, so the C parser accepting them is a real check. +-- payload computed, so the C parser accepting them is a real check. Since +-- ruling R14 the CRC input is the 12 header bytes only. local function crc_add(crc, bytes, first, count) for i = first, first + count - 1 do crc = (crc ~ (bytes[i] << 8)) & 0xFFFF @@ -186,7 +190,9 @@ local function make_frame(kind, frame_no, rows, stretch, plen, fill) end end local crc = crc_add(0xFFFF, b, 0, 12) - crc = crc_add(crc, b, 16, plen) + -- Header bytes 0..11 only (ruling R14): the payload is protected by each + -- datagram's Ethernet FCS, not by this CRC. frame.py, frame_parse.c and + -- this loop are three independent readings of the same rule. b[12] = crc & 0xFF b[13] = (crc >> 8) & 0xFF b[14] = 0 @@ -304,7 +310,11 @@ local function inject() readback_a = verify(a, fa, RX_FRAME) local fb = make_frame(1, 0x0022, RX_ROWS, RX_STRETCH, RX_PLEN) - fb[16 + 100] = fb[16 + 100] ~ 0x01 -- payload byte flipped, CRC left alone + -- Corrupt a HEADER byte, inside the 12 bytes the CRC covers. Since ruling + -- R14 a flipped PAYLOAD byte is ACCEPTED by fp_parse (the Ethernet FCS is + -- the chip's job), so a payload flip would no longer exercise the reject + -- path; the payload-flip case is pinned by the host tests instead. + fb[4] = fb[4] ~ 0x01 -- frame_no low byte, CRC left alone poke(b, fb, RX_FRAME) readback_b = verify(b, fb, RX_FRAME) @@ -313,7 +323,7 @@ local function inject() prog:write_u32(RX_READY_A, 1) prog:write_u32(RX_READY_B, 1) - print(string.format("VID: frame %d: injected slot A (%s, %d B, valid) and slot B (%s, %d B, one payload byte flipped)", + print(string.format("VID: frame %d: injected slot A (%s, %d B, valid) and slot B (%s, %d B, one header byte flipped)", frames, hex(a), RX_FRAME, hex(b), RX_FRAME)) return true end @@ -325,6 +335,28 @@ end -- compares against what was actually delivered rather than re-deriving it. local draw_at = nil local draw_bytes = nil +local rx_ok_at = nil + +-- How long to wait for the payload to accept the frame, in EMULATED FRAMES. +-- This hook only runs once a frame, so that is the resolution of the +-- measurement below as well as the timeout. VID_DRAW_WAIT exists so the +-- timeout path can be exercised on purpose (set it to 1). +local DRAW_WAIT = tonumber(os.getenv("VID_DRAW_WAIT") or "300") + +-- Evidence freshness (ruling R15): remove this mode's dump files before the +-- run, so a failed run cannot be checked against the previous run's screen. +local DRAW_FILES = { "vram-draw.bin", "frame-draw.bin", "draw-status.txt" } + +local function clear_draw_files() + if MODE ~= "draw" then + return + end + for _, name in ipairs(DRAW_FILES) do + os.remove(DUMP_DIR .. "/" .. name) + end +end + +clear_draw_files() local function draw_row_byte(i) if (i // RX_ROW_BYTES) % 3 == 2 then @@ -394,10 +426,15 @@ local function write_file(path, s) end -- Dump the screen and the payload's record for vidstream_check.py, then end --- the run. The pass/fail for pixels belongs to the checker; here we only --- report what the payload itself says (one accepted frame, no rejects, no --- repaint by the OS). -local function dump_draw() +-- the run. The pass/fail for pixels belongs to the checker; here we report +-- what the payload itself says (one accepted frame, no rejects, no repaint by +-- the OS) plus the measured parse cost. +-- +-- `timed_out` matters for what the checker is allowed to say: if the payload +-- never accepted the frame, the screen shows the previous picture and calling +-- that a pixel mismatch would blame the blit for a transport failure. The +-- status file carries TIMEOUT=1 and the checker stops there. +local function dump_draw(timed_out) local vram = read_vram() local chars = {} for i = 0, DRAW_FRAME - 1 do @@ -416,23 +453,36 @@ local function dump_draw() local pcv = cpu.state['PC'].value local sent = prog:read_u16(VRAM_BASE + (VRAM_ROWS - 1) * VRAM_ROW_BITS + 1280) + local parse_frames = -1 local ok = okv and okf + if rx_ok_at ~= nil then + parse_frames = rx_ok_at - draw_at + end + print(string.format("VID: heartbeat %d, PC=%s (%s)", alive, hex(pcv), in_payload(pcv) and "inside the payload" or "OUTSIDE the payload")) print(string.format("VID: POOL_PTR=%s RX_SLOT_A=%s (an unchanged pointer means " .. "the payload did not re-enter main)", hex(ptr), hex(slota))) write_file(DUMP_DIR .. "/draw-status.txt", string.format( - "RX_OK=%d\nRX_BAD=%d\nOS_TOUCH=%d\nRX_READY_A=%d\nSENTINEL=%d\n", - rx_ok, rx_bad, os_touch, ready, sent)) + "RX_OK=%d\nRX_BAD=%d\nOS_TOUCH=%d\nRX_READY_A=%d\nSENTINEL=%d\n" + .. "TIMEOUT=%d\nPARSE_FRAMES=%d\n", + rx_ok, rx_bad, os_touch, ready, sent, timed_out and 1 or 0, + parse_frames)) print(string.format("VID: frame %d: screen dumped to %s/vram-draw.bin (%d B), frame to %s/frame-draw.bin (%d B)", frames, DUMP_DIR, #vram, DUMP_DIR, DRAW_FRAME)) print(string.format("VID: RX_OK=%d RX_BAD=%d OS_TOUCH=%d RX_READY_A=%d SENTINEL=%04X", rx_ok, rx_bad, os_touch, ready, sent)) + if parse_frames >= 0 then + print(string.format("VID: parse cost: RX_READY_A written at frame %d, RX_OK first read at frame %d = %d emulated frames", + draw_at, rx_ok_at, parse_frames)) + else + print("VID: parse cost: no RX_OK observed (the frame was never accepted)") + end - if rx_ok ~= 1 or rx_bad ~= 0 or ready ~= 0 or os_touch ~= 0 then + if timed_out or rx_ok ~= 1 or rx_bad ~= 0 or ready ~= 0 or os_touch ~= 0 then ok = false end print(ok and "VID: RESULT PASS" or "VID: RESULT FAIL") @@ -617,23 +667,35 @@ emu.register_frame_done(function() end end - -- Draw mode: inject once, then wait for the payload to consume the slot - -- (it clears RX_READY_A) and give it a few frames of slack before reading - -- the screen back. The timeout is generous on purpose: fp_parse() runs a - -- BITWISE CRC, and a 20 KB frame costs it ~40 emulated frames (measured), - -- so an unlucky 10-frame wait would read the screen before the blit. - -- A payload that never takes the frame still gets its screen dumped, so - -- the checker reports the truth instead of the run hanging. + -- Draw mode: inject once, then wait for the payload to accept the frame. + -- Two separate observations, because they answer different questions: + -- + -- * the first frame at which RX_OK reads >= 1 is the PARSE COST + -- (RX_READY_A write -> first RX_OK read, in emulated frames); + -- * RX_READY_A == 0 is the payload done with the slot, i.e. after + -- paint_frame() returned -- that is the screen to dump. + -- + -- R15: the wait is bounded, and hitting the bound is reported as a + -- timeout, never as a pixel mismatch. if entered and MODE == "draw" and draw_at == nil then if inject_draw() then draw_at = frames end end - if MODE == "draw" and draw_at ~= nil and frames > draw_at + 1 then - if (prog:read_u32(RX_READY_A) == 0 and frames >= draw_at + 5) - or frames >= draw_at + 300 then - dump_draw() + if MODE == "draw" and draw_at ~= nil then + if rx_ok_at == nil and prog:read_u32(RX_OK) >= 1 then + rx_ok_at = frames + end + if rx_ok_at ~= nil and prog:read_u32(RX_READY_A) == 0 + and frames > rx_ok_at then + dump_draw(false) + return + end + if frames >= draw_at + DRAW_WAIT then + print(string.format("VID: timed out waiting for the frame: announced at frame %d, RX_OK=%d after %d emulated frames", + draw_at, prog:read_u32(RX_OK), DRAW_WAIT)) + dump_draw(true) return end end diff --git a/harness/mame/vidstream_check.py b/harness/mame/vidstream_check.py index ca3e588..ed087cd 100755 --- a/harness/mame/vidstream_check.py +++ b/harness/mame/vidstream_check.py @@ -7,7 +7,13 @@ and blit it, then dumps three files: vram-draw.bin the screen as memory: 1024 rows of 256 bytes from bit 0x04000000 frame-draw.bin the bytes actually poked into the slot (16-byte header + payload) - draw-status.txt key=value lines (RX_OK, RX_BAD, OS_TOUCH, ...) + draw-status.txt key=value lines (RX_OK, RX_BAD, OS_TOUCH, SENTINEL, TIMEOUT, ...) + +A run whose wait for the payload expired writes TIMEOUT=1, and that is reported +as a timeout -- not as a screen full of wrong pixels, which would blame the +blit for a frame the payload never accepted (ruling R15). The canary word is +also asserted (0xA5C3), so a payload that stopped arming it is a failure rather +than a silently missing check. This script is the independent reading of the geometry, not a copy of the code under test. The frame is 1280 pixels wide, 1 bpp, 160 bytes a row; the screen @@ -38,6 +44,7 @@ SCREEN_ROWS = 1024 ZERO_ROW_MOD = 3 # rows with r % 3 == 2 are the all-zero rows ZERO_ROW_REM = 2 FILL_BYTE = 0xF0 # rows that are not zero are 0xF0F0 words -> 0xF0 bytes +CANARY = 0xA5C3 # SENT_VALUE in vidstream.c, at row 1023 / column 1280 failures = 0 @@ -96,6 +103,14 @@ def main(): print("vidstream_check: vram %s (%d B) frame %s (%d B) status %s" % (args.vram, len(vram), args.frame, len(frame), args.status)) + # --- a timed-out wait is not a pixel verdict (ruling R15) ------------- + if status.get("TIMEOUT", 0) != 0: + print("FAIL: timed out waiting for the frame (RX_OK=%r, " + "RX_READY_A=%r: the payload did not finish with the slot)" + % (status.get("RX_OK"), status.get("RX_READY_A"))) + print("vidstream_check: FAIL (timeout)") + return 1 + # --- the frame that was delivered ------------------------------------ rows, stretch = args.rows, args.stretch if not check(stretch > 0, "stretch must be positive, got %d" % stretch): @@ -163,13 +178,21 @@ def main(): rx_ok = status.get("RX_OK") rx_bad = status.get("RX_BAD") os_touch = status.get("OS_TOUCH") + canary = status.get("SENTINEL") check(rx_ok == 1, "RX_OK=%r, expected 1 accepted frame" % (rx_ok,)) check(rx_bad == 0, "RX_BAD=%r, expected 0 rejected frames" % (rx_bad,)) check(os_touch == 0, "OS_TOUCH=%r: something else wrote to the sentinel corner of VRAM" % (os_touch,)) - print("vidstream_check: RX_OK=%r RX_BAD=%r OS_TOUCH=%r" - % (rx_ok, rx_bad, os_touch)) + # A missing canary (a payload that never armed it, or a status file from + # an older payload) must fail loudly: 0 was the firmware's RAM pattern, so + # "not written" and "correct" would otherwise look alike. + check(canary == CANARY, + "canary word is %r, expected 0x%04X (SENT_VALUE in vidstream.c)" + % (canary, CANARY)) + print("vidstream_check: RX_OK=%r RX_BAD=%r OS_TOUCH=%r canary=%s" + % (rx_ok, rx_bad, os_touch, + ("0x%04X" % canary) if isinstance(canary, int) else repr(canary))) if failures: print("vidstream_check: FAIL (%d checks)" % failures) diff --git a/harness/udpstream/frame.py b/harness/udpstream/frame.py index 6de19ae..9bf9df0 100644 --- a/harness/udpstream/frame.py +++ b/harness/udpstream/frame.py @@ -18,13 +18,19 @@ byte, so the layout is spelled out here and must be mirrored by the payload: 14 2 padding to HEADER_SIZE, zero The CRC is CCITT-FALSE -- polynomial 0x1021, initial value 0xFFFF, no input or -output reflection, no final XOR -- and it covers header bytes **0..11 plus the -payload**, never the CRC field itself. +output reflection, no final XOR -- and it covers **header bytes 0..11 only**, +never the payload and never the CRC field itself (ruling R14). -``unpack_header(buf)`` with no payload therefore checks a bare header (the CRC -of an empty payload). ``unpack_header(buf, payload)`` checks a whole frame, -which is what the receiving side does. A payload that is not passed is simply -not covered, so a header sealed over a payload will *not* validate without it. +That is a deliberate narrowing of an earlier contract. A payload-wide CRC cost +the terminal ~139 instructions per payload byte; on the 34010 that made the +parser, not the wire or the blitter, the frame-rate ceiling. Nothing is lost by +dropping it: every datagram already carries an Ethernet FCS that the LANCE +validates before the bytes reach a slot, and ``payload_len`` plus the delivered +length catches truncation. What the header CRC still buys is a check that the +geometry a frame was *sent* with is the geometry the terminal acts on. + +So a corrupted **payload** byte does not invalidate a frame here -- that is the +chip's job -- while a corrupted **header** byte does. """ import struct @@ -44,7 +50,7 @@ MAGIC = b"VS" #: Wire format version. Bump when the layout changes. VERSION = 0x01 -#: The CRC covers header bytes [0, CRC_OFFSET) plus the payload. +#: The CRC covers header bytes [0, CRC_OFFSET) and nothing else. CRC_OFFSET = 12 #: A strip of packed 1-bpp source rows. @@ -64,8 +70,9 @@ def _make_crc_table(): Keeping the loop here means the polynomial, the init value and the shift direction are stated exactly once, in the form ``vidstream.c`` repeats. - The table is only how the host gets through a 12 KB frame in ~2 ms instead - of ~28 ms, which a 30 fps sender cannot afford. + The table is only how the host gets through a long buffer quickly; since + ruling R14 the frame CRC itself runs over 12 bytes, but ``crc16`` is still + a general function and the tests cross-check it on long inputs. """ table = [] for byte in range(256): @@ -106,20 +113,18 @@ def pack_header(kind: int, frame_no: int, payload_len: int, rows: int, stretch: int, payload: bytes | None = None) -> bytes: """Build the 16-byte header for a frame. - The CRC is computed over header bytes 0..11 plus ``payload``. Pass the - payload here to seal a complete frame in one call -- and then it must be - exactly ``payload_len`` bytes, or a frame the CRC covers but the length - field denies would go on the wire. Omit ``payload`` to build the header - before the body exists and seal it later with :func:`seal_header`. + The CRC covers header bytes 0..11 only (ruling R14), so the header can be + built before the body exists. ``payload`` is kept as an **advisory** + consistency check: when it is passed it must be exactly ``payload_len`` + bytes, which catches a header that describes a body the caller does not + have. Its *contents* are not covered -- the Ethernet FCS is. """ kind = _checked("kind", kind, 0xFF) frame_no = _checked("frame_no", frame_no, 0xFFFF) payload_len = _checked("payload_len", payload_len, 0xFFFF) rows = _checked("rows", rows, 0xFF) stretch = _checked("stretch", stretch, 0xFF) - if payload is None: - body = b"" - else: + if payload is not None: body = bytes(payload) if len(body) != payload_len: raise ValueError("payload is %d bytes but payload_len says %d" @@ -135,35 +140,45 @@ def pack_header(kind: int, frame_no: int, payload_len: int, rows: int, 0, # reserved ) assert len(prefix) == CRC_OFFSET - crc = crc16(prefix + body) + crc = crc16(prefix) return prefix + _TAIL.pack(crc, 0) -def seal_header(header: bytes, payload: bytes = b"") -> bytes: - """Return ``header`` with its CRC field recomputed over ``payload``. +def seal_header(header: bytes, payload: bytes | None = None) -> bytes: + """Return ``header`` with its CRC field recomputed over header bytes 0..11. ``header`` must be exactly :data:`HEADER_SIZE` bytes: a longer buffer is rejected rather than silently truncated, so a caller cannot pass - ``header + payload`` here and lose the payload from the CRC. + ``header + payload`` here and believe the payload was sealed. - Byte-for-byte identical to ``pack_header(..., payload=payload)`` for a - header that came from :func:`pack_header`; kept separate so a caller that - assembles a frame in pieces (header, then payload) can still seal it. + ``payload`` is advisory, exactly as in :func:`pack_header`: when it is + passed it must match the header's ``payload_len``, and its bytes are not + covered by the CRC. Sealing is therefore idempotent and order-independent + -- header first or body first, the bytes are the same. """ if len(header) != HEADER_SIZE: raise ValueError("header must be exactly %d bytes, got %d" % (HEADER_SIZE, len(header))) - crc = crc16(bytes(header[:CRC_OFFSET]) + bytes(payload)) + if payload is not None: + want = struct.unpack_from(" dict: +def unpack_header(buf: bytes, payload: bytes | None = None) -> dict: """Parse a frame header and check its CRC. - ``payload`` is the frame body that follows the header; the CRC is computed - over ``header[0:12] + payload``. The returned ``crc_ok`` is purely the CRC - result -- callers also check ``magic`` and ``version`` (the C receiver - checks magic and the length bound before it bothers with the CRC). + ``crc_ok`` covers header bytes 0..11 only (ruling R14). ``payload`` is + advisory: when it is passed, its length must equal ``payload_len`` (a + truncated or over-long body is a length error, not a CRC error), but the + bytes themselves are never hashed here. + + The returned ``crc_ok`` is purely the CRC result -- callers also check + ``magic`` and ``version`` (the C receiver checks magic, the version and the + length bound before it bothers with the CRC). """ if len(buf) < HEADER_SIZE: raise ValueError("buffer shorter than a %d-byte header: %d" @@ -171,6 +186,9 @@ def unpack_header(buf: bytes, payload: bytes = b"") -> dict: prefix = _PREFIX.unpack_from(buf, 0) magic, version, kind, frame_no = prefix[:4] payload_len, rows, stretch, _reserved = prefix[4:] + if payload is not None and len(payload) != payload_len: + raise ValueError("payload is %d bytes but the header says %d" + % (len(payload), payload_len)) stored_crc, _pad = _TAIL.unpack_from(buf, CRC_OFFSET) return { "magic": magic, @@ -180,5 +198,5 @@ def unpack_header(buf: bytes, payload: bytes = b"") -> dict: "payload_len": payload_len, "rows": rows, "stretch": stretch, - "crc_ok": crc16(bytes(buf[:CRC_OFFSET]) + bytes(payload)) == stored_crc, + "crc_ok": crc16(bytes(buf[:CRC_OFFSET])) == stored_crc, } diff --git a/harness/udpstream/frame_parse.c b/harness/udpstream/frame_parse.c index df07933..982d9a5 100644 --- a/harness/udpstream/frame_parse.c +++ b/harness/udpstream/frame_parse.c @@ -4,13 +4,18 @@ * fp_parse() answers one question: is the block of bytes the transport just * delivered a complete, uncorrupted frame of the format frame.py writes? It * checks, in this order, the length bound, the magic and version, and finally - * the CRC -- the CRC is the expensive part, so a truncated or foreign frame is - * rejected before it is computed. + * the CRC over the 12 header bytes. * * The CRC is CCITT-FALSE (polynomial 0x1021, initial value 0xFFFF, no input or * output reflection, no final xor), the same one frame.py implements with a - * 256-entry table. It covers header bytes 0..11 followed by the payload; the - * stored crc16 at offset 12 is never part of its own input. + * 256-entry table. It covers header bytes 0..11 ONLY (ruling R14): the bytes + * at offset 12 are the CRC itself and the payload is never walked. A + * payload-wide CRC cost ~139 instructions per byte on this CPU and made the + * parser the frame-rate ceiling; every datagram already carries an Ethernet + * FCS the LANCE validates, and the length bound below catches truncation. + * + * So a corrupted PAYLOAD byte does not make fp_parse() reject a frame -- the + * chip's FCS is the check for that -- while a corrupted HEADER byte does. * * This file is compiled twice: by the 1987 TI C compiler inside the payload * build (build-stream.sh stages it as FRMPSR.C) and by the host cc for @@ -62,6 +67,9 @@ unsigned int len; * See frame_parse.h for the contract. A NULL buffer or header is a rejection * rather than a crash: the payload calls this with a pointer it published * itself, and a zero there means "no slot yet". + * + * The CRC covers header bytes 0..11 only, so the body is never touched: a + * 20 KB frame costs the same as a 1 KB one (ruling R14). */ int fp_parse(buf, len, out) unsigned char *buf; @@ -96,7 +104,6 @@ fp_header *out; stored = ((unsigned int) buf[FP_CRC_OFF + 1] << 8) | (unsigned int) buf[FP_CRC_OFF]; crc = fp_crc_add(0xFFFF, buf, FP_PREFIX_SIZE); - crc = fp_crc_add(crc, buf + FP_HEADER_SIZE, plen); if (crc != stored) { return 0; } diff --git a/harness/udpstream/frame_parse.h b/harness/udpstream/frame_parse.h index 3945a44..1cd0f54 100644 --- a/harness/udpstream/frame_parse.h +++ b/harness/udpstream/frame_parse.h @@ -21,8 +21,12 @@ * 14 2 padding, zero * * The CRC is CCITT-FALSE (polynomial 0x1021, initial value 0xFFFF, no input or - * output reflection, no final xor) over header bytes 0..11 followed by the - * payload -- never the CRC field itself. + * output reflection, no final xor) over header bytes 0..11 ONLY (ruling R14); + * the payload is never part of it. A corrupted payload byte is therefore + * accepted -- the Ethernet FCS, checked by the LANCE per datagram, is the + * check for those bytes -- while a corrupted header byte is rejected. What + * catches a truncated frame is the `len >= FP_HEADER_SIZE + payload_len` bound + * in fp_parse()'s contract at the end of this file. * * --------------------------------------------------------------------------- * Marker page: bit 0x01810000, byte 0x00302000. A cell below is one 32-bit @@ -129,9 +133,9 @@ typedef struct { * 1 and *out filled when buf[0..len) is a well-formed frame, 0 otherwise. * A frame is well formed when len is at least FP_HEADER_SIZE, the magic and * version match, len is at least FP_HEADER_SIZE + payload_len, and the stored - * crc16 equals the CCITT-FALSE CRC over buf[0..11] and the payload. Bytes - * beyond the frame are ignored: payload_len is authoritative (the transport - * hands us exactly what arrived, and a tail is not part of the frame). + * crc16 equals the CCITT-FALSE CRC over buf[0..11]. Bytes beyond the frame + * are ignored: payload_len is authoritative (the transport hands us exactly + * what arrived, and a tail is not part of the frame). */ int fp_parse(); diff --git a/harness/udpstream/tests/test_frame.py b/harness/udpstream/tests/test_frame.py index 837e70b..b63434d 100644 --- a/harness/udpstream/tests/test_frame.py +++ b/harness/udpstream/tests/test_frame.py @@ -2,8 +2,14 @@ The plan's two tests come first, verbatim. The rest pin down the parts of the format the plan text is ambiguous about, because both ends must agree on them: -the CCITT-FALSE polynomial, the CRC covering header[0:12] *and* the payload, and -the exact byte offsets the C side will mirror. +the CCITT-FALSE polynomial, the CRC covering header[0:12] **only** (ruling R14), +and the exact byte offsets the C side will mirror. + +Ruling R14 changed the CRC's coverage, and the consequence is stated here on +purpose: a corrupted **payload** byte leaves a frame ACCEPTED (the Ethernet FCS +of each datagram is the check for those bytes), while a corrupted **header** +byte is REJECTED, and a truncated frame (``len < 16 + payload_len``) is rejected +by the length bound that ``fp_parse`` applies before the CRC. """ import struct @@ -51,18 +57,48 @@ def test_crc16_matches_an_independent_implementation(): assert crc16(data) == binascii.crc_hqx(data, 0xFFFF), n -def test_crc16_spans_the_payload(): +def test_crc_covers_the_header_not_the_payload(): + """Ruling R14: payload bytes are the chip's job, header bytes are ours. + + A corrupted payload byte must leave ``crc_ok`` True (the LANCE has already + validated each datagram's Ethernet FCS) and a corrupted header byte must + make it False. The payload argument is advisory: it must match + ``payload_len`` in length, and its bytes are not hashed. + """ payload = bytes(range(256)) * 2 h = pack_header(KIND_STRIP, frame_no=1, payload_len=len(payload), rows=8, stretch=128, payload=payload) assert unpack_header(h, payload)["crc_ok"] is True - # The same header checked without its payload must not validate: that is the - # difference between "header covers the payload" and "header only". - assert unpack_header(h)["crc_ok"] is False - # And a payload bit flip is caught. + # Same result without the body at all: the body is not part of the CRC. + assert unpack_header(h)["crc_ok"] is True + + # A payload bit flip is ACCEPTED here: the Ethernet FCS is the chip's job. bad = bytearray(payload) bad[100] ^= 0x80 - assert unpack_header(h, bytes(bad))["crc_ok"] is False + assert unpack_header(h, bytes(bad))["crc_ok"] is True + + # A header bit flip (inside bytes 0..11) is REJECTED. + hh = bytearray(h) + hh[8] ^= 0x01 + assert unpack_header(bytes(hh))["crc_ok"] is False + + # The payload argument still has to be the length the header claims: a + # truncated body is a length error, not a CRC error (fp_parse's job). + with pytest.raises(ValueError): + unpack_header(h, payload[:-1]) + with pytest.raises(ValueError): + unpack_header(h, payload + b"\x00") + + +def test_payload_bytes_do_not_change_the_sealed_header(): + """One header, two different bodies: the header bytes are identical.""" + a = b"\x00" * 160 + b = b"\xFF" * 160 + ha = pack_header(KIND_STRIP, 5, len(a), 1, 8, payload=a) + hb = pack_header(KIND_STRIP, 5, len(b), 1, 8, payload=b) + assert ha == hb + assert unpack_header(ha, a)["crc_ok"] is True + assert unpack_header(hb, b)["crc_ok"] is True def test_seal_header_after_the_payload_is_known(): @@ -94,7 +130,7 @@ def test_seal_header_rejects_over_length_input(): def test_pack_header_rejects_payload_length_mismatch(): - """A CRC sealed over N bytes must not be sent under a different payload_len.""" + """The payload argument is advisory: it must match payload_len when given.""" payload = b"\xAA" * 10 pack_header(KIND_STRIP, 1, 10, 8, 128, payload=payload) # exact: fine with pytest.raises(ValueError): @@ -105,6 +141,12 @@ def test_pack_header_rejects_payload_length_mismatch(): pack_header(KIND_STRIP, 1, 10, 8, 128, payload=b"") # explicit empty # Omitting the payload is the header-only path: no length to disagree with. assert unpack_header(pack_header(KIND_STRIP, 1, 10, 8, 128))["crc_ok"] is True + # seal_header applies the same advisory rule. + h = pack_header(KIND_STRIP, 1, 10, 8, 128) + assert seal_header(h, b"\x00" * 10) == h + assert seal_header(h) == h # not given: no check + with pytest.raises(ValueError): + seal_header(h, b"\x00" * 9) # --- the byte layout the C guest will mirror ----------------------------- diff --git a/harness/udpstream/tests/test_frame_parse.c b/harness/udpstream/tests/test_frame_parse.c index 2c7cd62..cc05aaf 100644 --- a/harness/udpstream/tests/test_frame_parse.c +++ b/harness/udpstream/tests/test_frame_parse.c @@ -4,22 +4,37 @@ * cc -std=c89 -Wall -o /tmp/tfp harness/udpstream/tests/test_frame_parse.c \ * harness/udpstream/frame_parse.c && /tmp/tfp * - * The frame under test is a GOLDEN VECTOR, not something this test or - * frame_parse.c computed: the bytes in `golden` came from frame.py, the - * sender's implementation of the same spec. That is what makes the test a - * cross-check of the C parser rather than a tautology: if the C CRC, the - * field offsets or the coverage (header bytes 0..11 plus the payload, never - * the CRC field) were wrong, the golden frame would be rejected. + * The frames under test are GOLDEN VECTORS, not something this test or + * frame_parse.c computed: the bytes in `golden` and `golden_short` came from + * frame.py, the sender's implementation of the same spec. That is what makes + * the test a cross-check of the C parser rather than a tautology: if the C CRC, + * the field offsets, the CRC's coverage or the length rule were wrong, these + * frames would not be judged as frame.py judged them. * - * Regenerate the vector with: + * THE CRC COVERS HEADER BYTES 0..11 ONLY (ruling R14). That has a consequence + * this test states explicitly rather than leaving implicit: + * + * * a corrupted PAYLOAD byte is ACCEPTED -- the bytes between the header and + * the end of the frame are not hashed here; each datagram's Ethernet FCS, + * validated by the LANCE, is what protects them; + * * a corrupted HEADER byte (inside bytes 0..11) is REJECTED; + * * a TRUNCATED frame (len < 16 + payload_len) is REJECTED by the length + * bound, which is the check that has to carry truncation now. + * + * Regenerate both vectors with: * * cd harness/udpstream && python3 -c ' * import frame * pl = bytes((i*7+3) & 0xFF for i in range(80)) * d = frame.pack_header(1, 0x1234, len(pl), 1, 8, payload=pl) + pl - * print(len(d)); print(", ".join("0x%02X" % b for b in d))' + * print("golden:", len(d)); print("\t" + "".join("0x%02X, " % b for b in d)) + * pl = bytes((i*7+3) & 0xFF for i in range(81)) + * d = frame.pack_header(1, 0x1235, len(pl), 1, 8, payload=pl) + pl + * print("golden_short:", len(d)); print("\t" + "".join("0x%02X, " % b for b in d))' * - * (kind 1 = KIND_STRIP, frame_no 0x1234, rows 1, stretch 8, payload_len 80.) + * `golden_short` claims 81 payload bytes and is delivered with only 80 of them, + * so its header (and its header CRC) is valid while its length is one short -- + * only the length bound can reject it. (kind 1 = KIND_STRIP.) */ #include @@ -41,7 +56,7 @@ static void check(int cond, const char *what) static const unsigned char golden[] = { 0x56, 0x53, 0x01, 0x01, 0x34, 0x12, 0x50, 0x00, - 0x01, 0x08, 0x00, 0x00, 0xB3, 0x3C, 0x00, 0x00, + 0x01, 0x08, 0x00, 0x00, 0x0B, 0x39, 0x00, 0x00, 0x03, 0x0A, 0x11, 0x18, 0x1F, 0x26, 0x2D, 0x34, 0x3B, 0x42, 0x49, 0x50, 0x57, 0x5E, 0x65, 0x6C, 0x73, 0x7A, 0x81, 0x88, 0x8F, 0x96, 0x9D, 0xA4, @@ -54,6 +69,23 @@ static const unsigned char golden[] = { 0xFB, 0x02, 0x09, 0x10, 0x17, 0x1E, 0x25, 0x2C }; +/* The same shape, one payload byte longer than what is delivered. */ +static const unsigned char golden_short[] = { + 0x56, 0x53, 0x01, 0x01, 0x35, 0x12, 0x51, 0x00, + 0x01, 0x08, 0x00, 0x00, 0x78, 0x3B, 0x00, 0x00, + 0x03, 0x0A, 0x11, 0x18, 0x1F, 0x26, 0x2D, 0x34, + 0x3B, 0x42, 0x49, 0x50, 0x57, 0x5E, 0x65, 0x6C, + 0x73, 0x7A, 0x81, 0x88, 0x8F, 0x96, 0x9D, 0xA4, + 0xAB, 0xB2, 0xB9, 0xC0, 0xC7, 0xCE, 0xD5, 0xDC, + 0xE3, 0xEA, 0xF1, 0xF8, 0xFF, 0x06, 0x0D, 0x14, + 0x1B, 0x22, 0x29, 0x30, 0x37, 0x3E, 0x45, 0x4C, + 0x53, 0x5A, 0x61, 0x68, 0x6F, 0x76, 0x7D, 0x84, + 0x8B, 0x92, 0x99, 0xA0, 0xA7, 0xAE, 0xB5, 0xBC, + 0xC3, 0xCA, 0xD1, 0xD8, 0xDF, 0xE6, 0xED, 0xF4, + 0xFB, 0x02, 0x09, 0x10, 0x17, 0x1E, 0x25, 0x2C, + 0x33 +}; + #define GOLDEN_LEN ((unsigned int) sizeof golden) #define GOLDEN_PLEN 80 @@ -84,15 +116,40 @@ int main(void) check(h.payload_len == GOLDEN_PLEN, "payload_len == 80"); check(h.payload_off == FP_HEADER_SIZE, "payload_off == 16"); - /* ---- a flipped payload byte fails the CRC ---- */ + /* ---- a flipped PAYLOAD byte is ACCEPTED (ruling R14) ---- + * + * The CRC is over the header only, so the body is not hashed here. The + * Ethernet FCS of each datagram, validated by the LANCE, is what + * protects those bytes; this parser must not pretend otherwise. */ len = load(); buf[FP_HEADER_SIZE + 40] = (unsigned char) (buf[FP_HEADER_SIZE + 40] ^ 0x01); - check(fp_parse(buf, len, &h) == 0, "flipped payload byte rejected"); + check(fp_parse(buf, len, &h) == 1, + "flipped payload byte ACCEPTED (header-only CRC; FCS is the chip's job)"); - /* ---- a flipped header byte inside the CRC prefix fails too ---- */ + /* ---- a flipped header byte inside the CRC prefix is rejected ---- */ len = load(); buf[8] = (unsigned char) (buf[8] + 1); check(fp_parse(buf, len, &h) == 0, "flipped header byte rejected"); + len = load(); + buf[3] = (unsigned char) (buf[3] ^ 0x80); /* kind, inside the prefix */ + check(fp_parse(buf, len, &h) == 0, "flipped kind byte rejected"); + + /* ---- a valid header whose payload_len exceeds what arrived ---- + * + * `golden_short` claims 81 payload bytes. Delivered whole it is a valid + * frame (so the vector is not simply broken); one byte short it must be + * rejected by the LENGTH BOUND alone, because its header CRC is right. */ + { + unsigned int n = (unsigned int) sizeof golden_short; + + memcpy(buf, golden_short, sizeof golden_short); + check(fp_parse(buf, n, &h) == 1, + "golden_short delivered whole is accepted"); + check(h.payload_len == 81, "golden_short payload_len == 81"); + memcpy(buf, golden_short, sizeof golden_short); + check(fp_parse(buf, n - 1, &h) == 0, + "length bound rejects len < 16 + payload_len (header CRC valid)"); + } /* ---- short buffers: one byte short of the frame, and below the header ---- */ len = load(); @@ -126,11 +183,11 @@ int main(void) buf[FP_CRC_OFF + 1] = (unsigned char) (buf[FP_CRC_OFF + 1] + 1); check(fp_parse(buf, len, &h) == 0, "corrupt crc high byte rejected"); - /* ---- a header that understates payload_len cannot hide bad bytes ---- */ + /* ---- editing payload_len is a header edit: the header CRC catches it ---- */ len = load(); buf[6] = 0x40; /* payload_len 0x40 instead of 0x50 */ check(fp_parse(buf, len, &h) == 0, - "payload_len shorter than the sealed payload rejected"); + "header payload_len edit (CRC not recomputed) rejected"); /* ---- payload_len is authoritative: a tail in the slot is not the frame ---- */ len = load(); diff --git a/harness/udpstream/tests/test_vidsrc.py b/harness/udpstream/tests/test_vidsrc.py index ec48ab6..94a5584 100644 --- a/harness/udpstream/tests/test_vidsrc.py +++ b/harness/udpstream/tests/test_vidsrc.py @@ -5,9 +5,10 @@ text leaves implicit but the terminal depends on: the frame is split at 1450 bytes with only the last packet short, and a payload whose length disagrees with ``rows`` is rejected here instead of on the wire. -Note the third plan test checks ``crc_ok`` *with the payload*: the header CRC -covers header[0:12] + payload, so a header-only seal would fail every frame on -the receiving side. +Note the third plan test checks ``crc_ok`` *with the payload*: since ruling R14 +the header CRC covers header[0:12] only, so the same result must hold with or +without the body -- the payload argument is an advisory length check, not part +of the CRC. """ import pytest @@ -105,8 +106,9 @@ def test_every_packet_carries_the_same_valid_header(): assert head["payload_len"] == 8 * 160 assert head["rows"] == 8 and head["stretch"] == 128 assert unpack_header(pkts[0][:HEADER_SIZE], b"".join(pkts)[HEADER_SIZE:])["crc_ok"] is True - # A header-only check must not validate: that would mean the CRC missed the body. - assert head["crc_ok"] is False + # The header-only check gives the same answer (ruling R14): the body is + # covered by the Ethernet FCS, not by this CRC. + assert head["crc_ok"] is True def test_encode_rejects_a_payload_that_is_not_rows_times_160(): diff --git a/harness/udpstream/vidsrc.py b/harness/udpstream/vidsrc.py index 863ccd6..5575d65 100644 --- a/harness/udpstream/vidsrc.py +++ b/harness/udpstream/vidsrc.py @@ -15,10 +15,11 @@ header against the whole payload, exactly as the C receiver does: [ header | body ... ][ body ... ][ body ] -The header CRC covers header[0:12] **plus the payload**, so ``pack_header`` is -called with ``payload=rows_1bpp``. A header-only CRC would satisfy every local -test that only unpacks the header and would still be rejected by the terminal -for every single frame. +The header CRC covers header[0:12] **only** (ruling R14): the body bytes are +covered by each datagram's Ethernet FCS, and a payload-wide CRC is far too slow +on the terminal's CPU. ``pack_header`` is still called with +``payload=rows_1bpp`` as an advisory check that the body is exactly +``payload_len`` bytes -- it does not change the CRC. Pure standard library; no dependency on any host-side capture stack. """