fix(router): window repair searches and commits at the same width, and merges via barrels#682
Merged
Conversation
`route_window_complete` returned bare segment lists from a search whose only inter-path guarantee was node-disjointness on the coarse routing grid. Two facts leaked past that guarantee: 1. Node-disjoint at pitch (width + clearance) does not mean legal in exact geometry. Terminals straddling a cell boundary can pinch below clearance, and a via pad is wider than a trace, so via stacks conflicted in a class the grid never modeled. Callers committed both paths and DRC found the violation afterwards. 2. Every caller re-derived per-net width and via positions from the segment list — auto.rs walked segment windows looking for layer changes, cm5_verdict.rs kept its own copy of the same logic. `CompleteOutcome::Routed` now carries `Vec<RoutedPath>` — net, segments, vias, width, legal. After the search succeeds, `mark_mutual_legality` replays the paths in order against a scratch clone of the session, probing each against the copper its predecessors committed and probing vias at the real pad radius the caller declares via the new `via_radius` parameter. A failure downgrades only that path, so the caller's rule collapses to "commit the legal paths, in order." Width becomes a fallback rather than the committed value: each path commits at `session.width_for(net, width)` — the same map that feeds DRC — and the search runs at the widest class width in the window. auto.rs (which silently had the broken behavior) now skips illegal paths and uses `path.width`; cm5_verdict.rs drops 114 lines of duplicated via and width derivation. ProvedInfeasible / BudgetExhausted are unchanged. On CM5 at the same 300k budget this trades one connection for a strictly cleaner board: 36 -> 35 routed, 4722 -> 4704 DRC violations, with Short/Clearance/MinTraceWidth counts identical. The dropped connection was a genuinely conflicting via stack; removing it cleared 18 violations and added none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Choji has used up today's reviewsYour team has reached the free plan's limit of 5 reviews per day, so Choji stepped aside on this pull request. That's a plan limit, not a reflection of the change itself. Your free reviews reset tomorrow. To have Choji review every push without waiting, upgrade to Pro for unlimited reviews. You're on the free plan · 5 reviews/day. |
…94bfe # Conflicts: # crates/vcad-ecad-pcb/examples/cm5_verdict.rs # crates/vcad-ecad-pcb/src/router/complete.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Scope changed after #681 landed. This PR originally moved mutual-legality proving and per-net class widths into
route_window_completeitself. #681 then implemented both caller-side —resolve_clusterprobes-then-commits per path in order, andvalidate_and_commitgets the same property for free via&mut RouteSession. Caller-side is the better place: the caller probes the real session, whereas a router-side replay can only probe a scratch clone that cannot see copper committed outside the window. So that work is dropped as superseded, and this PR is now just the part #681 left behind.#681 fixed the CM5 verdict driver. It did not fix
auto.rs::joint_window_repair, the other consumer of the window router, which had three gaps that each cost connections by finding a routing the commit then rejects:width, committed atsession.width_for(net, width)windows(2)layer-change scanF.Cu → In1 → In2at one point emitted two coincident vias, stacking drills at zero spacing and failing hole-to-hole against itselfpath_vias, which merges a run of transitions at one point into a single barrelViaClass { pad_diameter, drill }so the router's legality model is the commit rulepath_viasispubincomplete.rsbecause the merging rule belongs next to the emitter, not in a caller — #681 wrote a copy of it inline in the example, and that copy can now delegate.Tests
a_run_of_layer_changes_at_one_point_is_one_barrel—F.Cu → In1 → In2without moving collapses to oneF.Cu → In2barrel. This is the bug that produced self-colliding drills.separate_layer_changes_stay_separate_barrels— guards the other direction: two transitions at different points stay two vias, and a layer change whose endpoints don't meet is not a via at all.cargo test -p vcad-ecad-pcb --features gpu→ 266 passed, 0 failed.cargo fmt --all --checkclean.cargo clippy -p vcad-ecad-pcb --features gpu --lib --example cm5_verdict -- -D warningsclean.Reviewer notes
A fourth gap is left open, deliberately.
auto.rsstill passes an emptyterminalsslice, so its stubs can surface off the pad's own layer — the net-island failureTerminalLayersexists to prevent. Closing it needs the example'sterminal_layers(~45 lines, depends onpoint_on_segmentandDriver::span) lifted into the library so both callers share one implementation. That is a larger change than this fix and wants its own PR; I did not want to half-implement it, since a too-narrow layer set fails closed but silently loses routes.No CM5 before/after numbers this time. The earlier revision of this PR reported them, but that measurement was against the pre-#681 router and no longer describes this code. The change is argued from the three mismatches above and covered by unit tests rather than a board-level count.
Cargo.lockis intentionally untouched. Building against current phyz (main, 0.3.1) wants a 59-line lock update, because vcad main still pins phyz 0.3.0 while CI clones phyz's default branch. That refresh is unrelated to this fix and CI re-resolves the lock itself (no--lockedon the workspace build), so it is left for a dedicated change.🤖 Generated with Claude Code