Refuse PyUmiPacket merge when the LEN field would overflow

[eastagiletracker]

This PR proposes refusing a PyUmiPacket merge when the combined LEN would overflow the eight-bit LEN field, fixes #262. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/400. You can sign in with your GitHub ID to claim ownership of the project.

What goes wrong today

PyUmiPacket::merge writes the merged length back with set_umi_len(&cmd, len + umi_len(other.cmd) + 1). LEN is an eight-bit field (DECL_UMI_SETTER(len, 8, 8) in switchboard/cpp/umilib.h) and set_umi_bits masks the value to that width, so a merge whose combined length exceeds 256 words wraps around silently and still returns true. By that point the packet has already been resized and the incoming data copied in, so the caller is handed a transaction whose command describes far less data than the packet actually holds.

Reproducing on main at 4287d19 — pip install .[test], then:

import numpy as np
from switchboard import PyUmiPacket, UmiCmd, umi_pack, umi_len

def make(len_field, addr):
    cmd = umi_pack(UmiCmd.UMI_REQ_WRITE, 0, 0, len_field, 0, 0)
    return PyUmiPacket(cmd, addr, addr, np.arange(len_field + 1, dtype=np.uint8))

p = make(254, 0)    # LEN=254, i.e. 255 words
q = make(5, 255)    # LEN=5,   i.e. 6 words

print('merge returned:', p.merge(q))
print('LEN after merge:', umi_len(p.cmd), '->', umi_len(p.cmd) + 1, 'words')
print('bytes actually held:', p.data.size)

Output on main:

merge returned: True
LEN after merge: 4 -> 5 words
bytes actually held: 261

That is the case @azaidy described in #262: (254 + 1) + (5 + 1) rolls back to a LEN of 4, and the merge reports success.

The change

merge now returns false when len + umi_len(other.cmd) + 1 would exceed the largest value LEN can represent, and it checks this before resize, so a refused merge leaves the packet untouched instead of half-modified. The limit is named UMI_MAX_LEN in switchboard/cpp/umilib.h, beside the LEN accessors it belongs with.

Nothing that could legitimately merge before stops merging: 256 words is the most a single UMI transaction can describe, so the only merges now refused are the ones that were producing incorrect packets. umi_loopback already treats a false return as "start a new transaction", and it applies the same rule on the TX and the RX side, so a stream that crosses the limit is split at the same point on both — I checked that a 512-word stream partitions identically either way.

Verification

Same script, with the change applied:

merge returned: False
LEN after merge: 254 -> 255 words
bytes actually held: 255

examples/test_umi_packet_merge.py adds three cases in the style of examples/test_bit_vector.py: the boundary merge that fills LEN exactly (LEN 254 plus LEN 0, giving LEN 255) still succeeds, the overflowing merge is refused and leaves the packet unchanged, and an ordinary short merge still concatenates its data as before. Run against an unpatched build, the middle case fails on assert not p.merge(q) while the other two pass, so the test is red without the change and green with it.

I ran pytest in examples/ before and after the change and got the same set of failures both times — the ten test_make cases, which need Verilator or Icarus and the umi package, none of which I have locally — with the three new cases passing on top. make in tests/ prints PASS with the modified header, including torture.c, which compiles it as C rather than C++. flake8 is clean.

How this was managed

This work was tracked as Fix merging of PyUmiPacket on a board at https://eastagiletracker.com/projects/400 that was built from this repository's own issues and pull requests, 315 stories in all.

If you'd rather not receive contributions like this, reply no-more-prs on this pull request and we won't open any further ones on your repositories.

---

Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com