From 9cf2392c4ba9cba2665f5f0d5a19a10f08842216 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Mon, 13 Jul 2026 11:08:02 -0600 Subject: [PATCH] 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. --- src/freeroute/route/room_router.py | 16 +- tests/dsn/fixtures/rooms_multipin_desync.dsn | 72 +++++++++ .../fixtures/rooms_multipin_desync_crash.dsn | 63 ++++++++ tests/route/test_room_shove.py | 138 ++++++++++++++++-- 4 files changed, 276 insertions(+), 13 deletions(-) create mode 100644 tests/dsn/fixtures/rooms_multipin_desync.dsn create mode 100644 tests/dsn/fixtures/rooms_multipin_desync_crash.dsn diff --git a/src/freeroute/route/room_router.py b/src/freeroute/route/room_router.py index 882adcd..2d57b3c 100644 --- a/src/freeroute/route/room_router.py +++ b/src/freeroute/route/room_router.py @@ -742,12 +742,20 @@ def _shove_recover_room( return 0 owners = force_place_owners(net_no, layer, corners, tree, owner, half_width) - acc = _Accepted( - wires=result.wires.setdefault(net_no, []), vias=result.vias.setdefault(net_no, []) - ) + # 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( + wires=result.wires.setdefault(net_no, []), vias=result.vias.setdefault(net_no, []) + ) + accepted[net_no] = acc result.add_wire(net_no, layer, corners) acc.wire_owners.append(owners) - accepted[net_no] = acc return len(journal) + 1 diff --git a/tests/dsn/fixtures/rooms_multipin_desync.dsn b/tests/dsn/fixtures/rooms_multipin_desync.dsn new file mode 100644 index 0000000..cfc85c0 --- /dev/null +++ b/tests/dsn/fixtures/rooms_multipin_desync.dsn @@ -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 + ) +) diff --git a/tests/dsn/fixtures/rooms_multipin_desync_crash.dsn b/tests/dsn/fixtures/rooms_multipin_desync_crash.dsn new file mode 100644 index 0000000..8ee0737 --- /dev/null +++ b/tests/dsn/fixtures/rooms_multipin_desync_crash.dsn @@ -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 + ) +) diff --git a/tests/route/test_room_shove.py b/tests/route/test_room_shove.py index 03b7283..dce9c82 100644 --- a/tests/route/test_room_shove.py +++ b/tests/route/test_room_shove.py @@ -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 the projection provably reduces to the old clamp where nothing else crosses the 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: - when a net would be dropped, the committed trace blocking it is nudged aside - with the exact shove primitive so *both* fit through the same channel. The - headline fixture ``rooms_shove_channel`` drops a net without it and routes both - with it, DRC-clean. +* **Shove-in-rooms** nudges a committed trace aside with the exact shove + primitive when a net would otherwise be dropped, so *both* fit through the same + channel. ``rooms_shove_channel`` drops a net under the default greedy net order + and routes both with shove, DRC-clean -- a recovery from greedy ordering (the + 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 @@ -24,10 +27,35 @@ import pytest from freeroute.board import build_board from freeroute.dsn import parse_dsn 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.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)) FIXTURES = Path(__file__).resolve().parent.parent / "dsn" / "fixtures" @@ -37,6 +65,8 @@ KICAD = FIXTURES / "kicad_routable.dsn" NARROW = FIXTURES / "narrow_channel.dsn" SHOVE_CHANNEL = FIXTURES / "rooms_shove_channel.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] @@ -61,15 +91,15 @@ def _pins_by_net(fixture): 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()) off, _, _ = route_dsn_board_rooms(dsn, shove=False) 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.drc_clean # 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 +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(): on, _, _ = route_dsn_board_rooms(parse_dsn(SHOVE_CHANNEL.read_text()), shove=True) 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 # no forbidden band -> the whole span, clamped to the ideal (old clamp) 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) == []