Fix multi-pin wire/owner desync in shove-in-rooms recovery

_shove_recover_room clobbered a net's _Accepted with a fresh one whose
wire_owners held only the just-recovered wire, while its wires list (the
shared result.wires[net_no]) already held earlier committed connections.
That left wire_owners shorter than the parallel wires list, so a later
net's shove either removed the wrong wire's copper from the search tree
(a silent different-net short: copper stayed in the emitted SES but
vanished from the tree, and has_violation falsely certified it clean) or
raised IndexError and aborted the whole route.

Reuse the net's existing _Accepted on recovery so wire_owners stays
parallel to wires. Fixes both the silent DRC violation and the crash
sibling without touching the DRC-clean or determinism invariants.

Also correct the overstated "density win" framing of the shove_channel
fixture: its plain-routing net drop is an artifact of greedy net order,
not a density limit (the same board routes both nets with zero shoves
under a reorder or with the cosmetic seals removed). Reframed the test
and docstring; the genuine sub-cell win remains the wide_door occupancy
packing (shoves == 0).

Regression tests assert the invariant against the emitted output
directly (reconstructed copper tiles), not just the tree that the bug
fooled, plus determinism, no-crash, and the ordering-artifact bound.
This commit is contained in:
Ryan Malloy 2026-07-13 11:08:02 -06:00
parent a06adbee13
commit 9cf2392c4b
4 changed files with 276 additions and 13 deletions

View File

@ -742,12 +742,20 @@ def _shove_recover_room(
return 0 return 0
owners = force_place_owners(net_no, layer, corners, tree, owner, half_width) owners = force_place_owners(net_no, layer, corners, tree, owner, half_width)
# Reuse this net's existing registry if it already committed connections, so
# ``wire_owners`` stays parallel to ``wires``. Clobbering with a fresh
# ``_Accepted`` here would leave ``wire_owners`` shorter than ``wires`` (its
# ``result.wires[net_no]`` list already holds the earlier connections),
# desyncing the two lists and making a later shove remove the wrong wire's
# copper from the tree while it stays in the emitted route.
acc = accepted.get(net_no) if accepted is not None else None
if acc is None:
acc = _Accepted( acc = _Accepted(
wires=result.wires.setdefault(net_no, []), vias=result.vias.setdefault(net_no, []) wires=result.wires.setdefault(net_no, []), vias=result.vias.setdefault(net_no, [])
) )
accepted[net_no] = acc
result.add_wire(net_no, layer, corners) result.add_wire(net_no, layer, corners)
acc.wire_owners.append(owners) acc.wire_owners.append(owners)
accepted[net_no] = acc
return len(journal) + 1 return len(journal) + 1

View File

@ -0,0 +1,72 @@
(pcb "rooms_multipin_desync"
(parser
(string_quote ")
(space_in_quoted_tokens on)
(host_cad "freeroute-test")
(host_version "1.0")
)
(resolution um 10)
(unit um)
(structure
(layer F.Cu (type signal) (property (index 0)))
(layer B.Cu (type power) (property (index 1)))
(boundary
(path pcb 0 0 0 300000 0 300000 -100000 0 -100000 0 0)
)
(keepout "seal_left" (rect F.Cu 0 -100000 2500 0))
(keepout "seal_right" (rect F.Cu 297500 -100000 300000 0))
(rule (width 2000) (clearance 2000))
)
(library
(padstack Rect_Pad
(shape (rect F.Cu -1000 -1000 1000 1000))
(shape (rect B.Cu -1000 -1000 1000 1000))
(attach off)
)
(padstack Via
(shape (circle F.Cu 1200))
(shape (circle B.Cu 1200))
(attach off)
)
(image PAD
(pin Rect_Pad 1 0 0)
)
)
(placement
(component PAD
(place B1 10000 -50000 front 0)
(place B2 290000 -50000 front 0)
(place Wt1 10000 -30000 front 0)
(place Wt2 290000 -30000 front 0)
(place M0 90000 -34000 front 0)
(place M1 90000 -46000 front 0)
(place M2 90000 -70000 front 0)
(place X1 40000 -40000 front 0)
(place X2 140000 -40000 front 0)
(place Yp1 40000 -60000 front 0)
(place Yp2 140000 -60000 front 0)
)
)
(network
(net NB
(pins B1-1 B2-1)
)
(net NWtop
(pins Wt1-1 Wt2-1)
)
(net NM
(pins M0-1 M1-1 M2-1)
)
(net NX
(pins X1-1 X2-1)
)
(net NY
(pins Yp1-1 Yp2-1)
)
(class default
(rule (width 2000) (clearance 2000))
)
)
(wiring
)
)

View File

@ -0,0 +1,63 @@
(pcb "rooms_multipin_desync_crash"
(parser
(string_quote ")
(space_in_quoted_tokens on)
(host_cad "freeroute-test")
(host_version "1.0")
)
(resolution um 10)
(unit um)
(structure
(layer F.Cu (type signal) (property (index 0)))
(layer B.Cu (type power) (property (index 1)))
(boundary
(path pcb 0 0 0 300000 0 300000 -100000 0 -100000 0 0)
)
(keepout "seal_left" (rect F.Cu 0 -100000 2500 0))
(keepout "seal_right" (rect F.Cu 297500 -100000 300000 0))
(keepout "KD" (rect F.Cu 30000 -100000 150000 -75000))
(rule (width 2000) (clearance 2000))
)
(library
(padstack Rect_Pad
(shape (rect F.Cu -1000 -1000 1000 1000))
(shape (rect B.Cu -1000 -1000 1000 1000))
(attach off)
)
(padstack Via
(shape (circle F.Cu 1200))
(shape (circle B.Cu 1200))
(attach off)
)
(image PAD
(pin Rect_Pad 1 0 0)
)
)
(placement
(component PAD
(place B1 10000 -50000 front 0)
(place B2 290000 -50000 front 0)
(place M0 30000 -20000 front 0)
(place M1 90000 -20000 front 0)
(place M2 90000 -70000 front 0)
(place X1 40000 -65000 front 0)
(place X2 140000 -65000 front 0)
)
)
(network
(net NB
(pins B1-1 B2-1)
)
(net NM
(pins M0-1 M1-1 M2-1)
)
(net NX
(pins X1-1 X2-1)
)
(class default
(rule (width 2000) (clearance 2000))
)
)
(wiring
)
)

View File

@ -7,11 +7,14 @@ Two opt-in (``shove=True``) upgrades to the continuous expansion-room track:
decomposition already keeps every door clearance-clear of committed copper, so decomposition already keeps every door clearance-clear of committed copper, so
the projection provably reduces to the old clamp where nothing else crosses the the projection provably reduces to the old clamp where nothing else crosses the
door the four foundation fixtures stay byte-for-byte identical. door the four foundation fixtures stay byte-for-byte identical.
* **Shove-in-rooms** is the real continuous, sub-cell win the grid could not do: * **Shove-in-rooms** nudges a committed trace aside with the exact shove
when a net would be dropped, the committed trace blocking it is nudged aside primitive when a net would otherwise be dropped, so *both* fit through the same
with the exact shove primitive so *both* fit through the same channel. The channel. ``rooms_shove_channel`` drops a net under the default greedy net order
headline fixture ``rooms_shove_channel`` drops a net without it and routes both and routes both with shove, DRC-clean -- a recovery from greedy ordering (the
with it, DRC-clean. same board is plainly routable under a reorder or without its cosmetic seals,
see ``test_room_shove_channel_drop_is_ordering_artifact_not_density``), not a
genuine density limit. The true sub-cell win is occupancy packing, which the
``rooms_wide_door`` fixture exercises with ``shoves == 0``.
""" """
from __future__ import annotations from __future__ import annotations
@ -24,10 +27,35 @@ import pytest
from freeroute.board import build_board from freeroute.board import build_board
from freeroute.dsn import parse_dsn from freeroute.dsn import parse_dsn
from freeroute.dsn.sexp import parse from freeroute.dsn.sexp import parse
from freeroute.geometry import IntBox, IntPoint from freeroute.geometry import IntBox, IntPoint, Polyline, PolylineShape
from freeroute.route import route, route_dsn_board_exact, route_dsn_board_rooms from freeroute.route import route, route_dsn_board_exact, route_dsn_board_rooms
from freeroute.route.pipeline import _rule_clearance_dsn from freeroute.route.pipeline import _rule_clearance_dsn
def _emitted_drc_violations(result, clearance):
"""Independent DRC over the *emitted* route: reconstruct every wire's copper
tiles and return the different-net, same-layer pairs that come closer than
``clearance``. This does NOT consult the search tree, so it catches copper
that is present in the output but (via a bug) absent from the tree -- exactly
the failure mode the wire/owner desync produced."""
items = [] # (net_no, layer, tile)
for net_no, segments in result.wires.items():
for layer, corners in segments:
for box in PolylineShape(Polyline(corners), result.half_width).tiles():
items.append((net_no, layer, box))
bad = []
for i in range(len(items)):
n1, l1, b1 = items[i]
expanded = b1.offset(clearance)
for j in range(i + 1, len(items)):
n2, l2, b2 = items[j]
if n1 == n2 or l1 != l2:
continue
if b2.overlaps(expanded):
bad.append((n1, n2))
return bad
sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
FIXTURES = Path(__file__).resolve().parent.parent / "dsn" / "fixtures" FIXTURES = Path(__file__).resolve().parent.parent / "dsn" / "fixtures"
@ -37,6 +65,8 @@ KICAD = FIXTURES / "kicad_routable.dsn"
NARROW = FIXTURES / "narrow_channel.dsn" NARROW = FIXTURES / "narrow_channel.dsn"
SHOVE_CHANNEL = FIXTURES / "rooms_shove_channel.dsn" SHOVE_CHANNEL = FIXTURES / "rooms_shove_channel.dsn"
WIDE_DOOR = FIXTURES / "rooms_wide_door.dsn" WIDE_DOOR = FIXTURES / "rooms_wide_door.dsn"
MULTIPIN_DESYNC = FIXTURES / "rooms_multipin_desync.dsn"
MULTIPIN_DESYNC_CRASH = FIXTURES / "rooms_multipin_desync_crash.dsn"
FOUNDATION = [SIMPLE, CROSSING, KICAD, NARROW] FOUNDATION = [SIMPLE, CROSSING, KICAD, NARROW]
@ -61,15 +91,15 @@ def _pins_by_net(fixture):
return out return out
# --- the headline density win ----------------------------------------------- # --- shove recovers a greedy-ordering drop ----------------------------------
def test_room_shove_channel_density_win(): def test_room_shove_channel_recovers_greedy_ordering_drop():
dsn = parse_dsn(SHOVE_CHANNEL.read_text()) dsn = parse_dsn(SHOVE_CHANNEL.read_text())
off, _, _ = route_dsn_board_rooms(dsn, shove=False) off, _, _ = route_dsn_board_rooms(dsn, shove=False)
on, scale, _ = route_dsn_board_rooms(dsn, shove=True) on, scale, _ = route_dsn_board_rooms(dsn, shove=True)
# without shove NET_A's sealed wall leaves no top<->bottom door -> NET_C drops # under the default greedy net order NET_A (net 1) walls the channel -> NET_C drops
assert off.routed_net_numbers == {1} assert off.routed_net_numbers == {1}
assert off.drc_clean assert off.drc_clean
# with shove NET_A is nudged aside and both nets fit through the same channel # with shove NET_A is nudged aside and both nets fit through the same channel
@ -79,6 +109,42 @@ def test_room_shove_channel_density_win():
assert on.tree.has_violation(_clearance(dsn, scale)) is None assert on.tree.has_violation(_clearance(dsn, scale)) is None
def test_room_shove_channel_drop_is_ordering_artifact_not_density():
"""Honest bound on the shove_channel claim: the plain drop is an artifact of
the greedy net order, not a genuine density limit. The identical plain engine
routes BOTH nets with zero shoves if NET_C is listed first, or if the two
cosmetic edge seals are removed -- so shove here recovers a self-inflicted
ordering drop rather than overcoming an unroutable density. (The genuine
sub-cell win is occupancy packing, exercised by the wide_door fixture with
shoves == 0.)"""
orig = SHOVE_CHANNEL.read_text()
# (1) reorder so NET_C becomes net 1 -> plain routing solves it, no shove
swapped = orig.replace(
" (net NET_A\n (pins A1-1 A2-1)\n )\n"
" (net NET_C\n (pins C1-1 C2-1)\n )",
" (net NET_C\n (pins C1-1 C2-1)\n )\n"
" (net NET_A\n (pins A1-1 A2-1)\n )",
)
assert swapped != orig
reordered, _, _ = route_dsn_board_rooms(parse_dsn(swapped), shove=False)
assert reordered.routed_net_numbers == {1, 2}
assert reordered.shoves == 0
assert reordered.drc_clean
# (2) drop the seal keepouts -> plain routing solves it, no shove
noseal = orig.replace(
' (keepout "seal_left"\n (rect F.Cu 0 -30000 2500 0)\n )\n'
' (keepout "seal_right"\n (rect F.Cu 197500 -30000 200000 0)\n )\n',
"",
)
assert noseal != orig
unsealed, _, _ = route_dsn_board_rooms(parse_dsn(noseal), shove=False)
assert unsealed.routed_net_numbers == {1, 2}
assert unsealed.shoves == 0
assert unsealed.drc_clean
def test_room_shove_channel_moves_blocker_but_keeps_its_pads(): def test_room_shove_channel_moves_blocker_but_keeps_its_pads():
on, _, _ = route_dsn_board_rooms(parse_dsn(SHOVE_CHANNEL.read_text()), shove=True) on, _, _ = route_dsn_board_rooms(parse_dsn(SHOVE_CHANNEL.read_text()), shove=True)
pins = _pins_by_net(SHOVE_CHANNEL) pins = _pins_by_net(SHOVE_CHANNEL)
@ -226,3 +292,57 @@ def test_free_intervals_splits_around_forbidden_band():
assert _best_free_point(0, 200, [(87, 113)], ideal=100) == 87 assert _best_free_point(0, 200, [(87, 113)], ideal=100) == 87
# no forbidden band -> the whole span, clamped to the ideal (old clamp) # no forbidden band -> the whole span, clamped to the ideal (old clamp)
assert _best_free_point(0, 200, [], ideal=100) == 100 assert _best_free_point(0, 200, [], ideal=100) == 100
# --- regression: multi-pin wire/owner desync in shove recovery ---------------
def test_room_shove_multipin_recovery_keeps_output_drc_clean():
"""Regression for the wire/owner desync in ``_shove_recover_room``.
A 3-pin net commits one connection via ``_commit`` and has a second straight
connection *recovered* by the shove path. The recovery used to clobber the
net's ``_Accepted`` with a fresh one whose ``wire_owners`` held only the
just-recovered wire, leaving it SHORTER than the parallel ``wires`` list. A
later net's shove then removed the WRONG wire's copper from the search tree;
that copper stayed in the emitted route but vanished from the tree, so
``has_violation`` falsely certified clean while a following net was placed on
top of it -- a hard different-net short in the SES.
Assert the invariant against the *emitted* output directly, not just the tree
(the tree was the thing being fooled)."""
dsn = parse_dsn(MULTIPIN_DESYNC.read_text())
on, scale, _ = route_dsn_board_rooms(dsn, shove=True)
clearance = _clearance(dsn, scale)
# the tree self-check and the independent emitted-copper check must agree
assert on.drc_clean
assert on.tree.has_violation(clearance) is None
assert _emitted_drc_violations(on.result, clearance) == []
# the shove path actually engaged and did not silently drop the multi-pin net
assert on.shoves >= 1
assert 3 in on.routed_net_numbers
def test_room_shove_multipin_recovery_is_deterministic():
a, _, _ = route_dsn_board_rooms(parse_dsn(MULTIPIN_DESYNC.read_text()), shove=True)
b, _, _ = route_dsn_board_rooms(parse_dsn(MULTIPIN_DESYNC.read_text()), shove=True)
assert _wire_key(a.result) == _wire_key(b.result)
assert a.routed_net_numbers == b.routed_net_numbers
def test_room_shove_multipin_recovery_does_not_crash():
"""Sibling of the desync: when the conflicting wire index exceeds the
(formerly truncated) ``wire_owners`` length, the recovery raised
``IndexError`` and aborted the whole public route. Keeping ``wire_owners``
parallel to ``wires`` fixes both faces; this board must route to completion,
DRC-clean, through the public ``route(engine="room", shove=True)`` API."""
ses = route(MULTIPIN_DESYNC_CRASH.read_text(), engine="room", shove=True)
top = parse(ses)
assert top.head == "session"
# and the library path stays DRC-clean on the emitted output
dsn = parse_dsn(MULTIPIN_DESYNC_CRASH.read_text())
on, scale, _ = route_dsn_board_rooms(dsn, shove=True)
clearance = _clearance(dsn, scale)
assert on.drc_clean
assert _emitted_drc_violations(on.result, clearance) == []