Skip to content

fix(import): split collapsed unconnected stacked-pad nets using PCB layout names - #1204

Open
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-import-split-collapsed-unconnected-stacked-pad-7730e1
Open

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-import-split-collapsed-unconnected-stacked-pad-7730e1

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Bug

pcb import of a KiCad 9+ project could silently short two unconnected pads into one net. KiCad's schematic netlist collapses an unconnected stacked pin (e.g. [5,6]) into a single unconnected-(REF-Pad5) net whose block carries two distinct <node> entries (pin "5" and pin "6"), giving ImportNetData { ports.len() == 2 }. The PCB layout, by contrast, keeps the two pads disambiguated as <base> and <base>_1 — names extract_kicad_layout_data already harvests onto ImportLayoutPad.net_names.

extract_ir never reconciled the collapsed net against those per-pad names, so the 2-port unconnected-(...) net flowed into ImportIr.nets unchanged. Downstream build_not_connected_nets only classifies unconnected-( nets with ports.len() == 1, so the collapsed net was treated as a real net and both pins were bound to one generated Net(...), creating an unintended short. The in-repo verify_physical_partitions couldn't catch it because both the source and built partitions carried the same collapsed {{REF.5},{REF.6}} set. This violates the README invariant: "A source pin with no connection may remain isolated on its own generated net. It must not share that net with another endpoint."

Fix

After extract_kicad_layout_data on the project-import branch, call a new restore_unconnected_net_suffixes_from_layout(&mut netlist.nets, &netlist.components)? (crates/pcbc/src/import/extract.rs). It splits any unconnected-( net with ports.len() > 1 into one net per distinct per-pad layout name already collected on ImportLayoutPad.net_names:

  • A companion is_disambiguated_unconnected_name predicate accepts only the exact <base> form and <base>_<all-digits> (e.g. unconnected-(U1-Pad5) and unconnected-(U1-Pad5)_1), so unrelated nets that merely share a string prefix are never stolen.
  • A pad with no recorded layout name falls back to the collapsed base name — endpoints are never silently dropped.
  • A pad carrying multiple matching names is a hard error (with the pad path, pin, collapsed net, and candidates in the message) rather than a silent pick.

The standalone-schematic branch is untouched (the restore call is gated on ImportSourceKind::Project). Added one CHANGELOG.md bullet under Unreleased > Fixed, per AGENTS.md.

Testing

  • Routine checks: cargo check --workspace --locked, cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo test --doc -p pcbc all pass clean (zero warnings).
  • Unit tests (committed): added 6 tests to extract.rs covering the split of the collapsed 2-port net, the no-op pass-through of single-port unconnected nets (KiCad 7/8 case), real named multi-port nets being left intact, the multi-component split with the no-layout-name fallback, the hard-error path for an ambiguous pad, and the predicate's accept/reject edge cases. cargo nextest run -p pcbc import::extract → 17/17 pass.
  • Importer suite: cargo nextest run -p pcbc import → 97/98 pass after installing kicad-cli 9.0.9. The one failure (missing_footprint_assignment_preserves_unset_marker_and_still_imports) is a pre-existing pcb-component-gen footprint-rendering fixture mismatch, confirmed identical on the clean baseline (verified by git stash + rerun), and unrelated to net extraction.
  • End-to-end with a stacked pin: I built a minimal KiCad 9 schematic with an unconnected [5,6] pin and ran pcbc import against it (release binary) — exit 0, generated U(name="U1", P5_6=NotConnected()), a single isolated no-connect with no short.
  • Trigger-reproduction caveat: the natural collapse the bug report describes (a Pad5 net name from GetSmallestStackedPadNumber, expanded into two <node> entries) does not reproduce on the installed kicad-cli 9.0.9 release — its string_utils.cpp lacks ExpandStackedPinNotation entirely, so the pin stays as [5,6] single-node. I corroborated the fix against the bug report's quoted master netlist via a parser-level E2E: feeding the 2-port unconnected-(U1-Pad5) net block (nodes pin "5"/pin "6") through the real parse_kicad_sexpr_netlist yields ports.len() == 2, and restore_unconnected_net_suffixes_from_layout then splits it into two 1-port nets. (That throwaway test was reverted before commit.)
  • Regression of existing importer: the kicad-bom Project-branch integration test (project_import_preserves_sources_and_existing_archive_behavior) passes both at baseline and with the fix.
  • Full workspace regression: cargo nextest run --workspace --profile ci --locked → 2390/2408 pass with the fix vs 2384/2402 on the clean baseline. The same 18 pre-existing environmental failures appear in both (14 in pcb-layout, which this PR doesn't touch; 4 in pcbc::integration requiring board tooling/network/fixture-format) — zero new regressions, 6 new passing tests.

Automatic Fixes PRs can be configured here.


Note

Medium Risk
Changes net identity during KiCad project import and could mis-split nets if layout names are ambiguous or malformed, though scope is limited to multi-port unconnected-( nets with strict suffix rules.

Overview
Fixes a KiCad 9+ project import bug where schematic netlists can merge stacked unconnected pins (e.g. [5,6]) into one multi-port unconnected-(...) net while the PCB keeps separate names like unconnected-(U1-Pad5) and unconnected-(U1-Pad5)_1. That collapse could make pcb import treat the net as a real connection and short two pads that should stay isolated.

After layout data is extracted for project imports, the importer now runs restore_unconnected_net_suffixes_from_layout, which re-splits any multi-port unconnected-( net using per-pad layout net names from ImportLayoutPad.net_names. Matching is limited to the base name and numeric _N suffixes via is_disambiguated_unconnected_name; ambiguous pads error, and pads without layout names keep the collapsed base so nothing is dropped. Standalone schematic import is unchanged.

CHANGELOG documents the fix under 0.4.50 Fixed. Six unit tests cover split behavior, pass-through, real nets, cross-component cases, errors, and suffix matching.

Reviewed by Cursor Bugbot for commit 0fb8284. Bugbot is set up for automated code reviews on this repo. Configure here.

@detail-app
detail-app Bot requested a review from LK September 6, 2026 02:24
@detail-app detail-app Bot assigned LK Sep 6, 2026
@detail-app detail-app Bot added the detail label Sep 6, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Devin Review

Comment on lines +662 to +665
let layout_name = matching_layout_names
.into_iter()
.next()
.unwrap_or_else(|| collapsed_name.clone());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Missing layout names preserve shorts

When a collapsed endpoint lacks a layout assignment, unwrap_or_else places it on the shared base net. Any endpoint already using that base joins it, so the import recreates the unintended short.

Prompt for agents
In crates/pcbc/src/import/extract.rs, restore_unconnected_net_suffixes_from_layout groups every port without a matching layout net name under collapsed_name. This collides with a port whose layout name is the exact base and also groups multiple unnamed ports together, preserving the short the function is intended to remove. Assign each unresolved endpoint a deterministic unique net identity while preserving an available layout base name for at most one endpoint. Avoid collisions with existing net names and add tests for base-plus-missing and multiple-missing cases.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigation for akhilles: the fallback does retain a shared partition for base-plus-missing or multiple-missing endpoints. However, assigning unique nets to those endpoints is not justified by missing PCB evidence. KiCad master exports expanded stacked pad numbers as nodes in one schematic net (eeschema/netlist_exporters/netlist_exporter_xml.cpp, makeListOfNets), then PCBNew independently suffixes valid no-connect pad names (pcbnew/netlist_reader/board_netlist_updater.cpp, updateFootprintNets). Missing PCB assignments also occur when no matching netlist endpoint exists; they do not establish isolation. The stacked-notation support was introduced after KiCad 9.0, so the PR’s KiCad 9+ premise is also too broad. This comment remains unresolved pending the source-of-truth decision: whether import may replace schematic connectivity with PCB-derived no-connect partitions. No synthetic endpoint identities or defensive reconstruction added; green CI will not clear the existing behavior hold.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Left a non-blocking comment and did not approve; no reviewers were assigned. Bugbot and Security Agent completed with no findings, but this is a functional pcb import connectivity change rather than a small fixup, so it still needs human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@akhilles
akhilles force-pushed the detail/bug-fix/fix-import-split-collapsed-unconnected-stacked-pad-7730e1 branch from ced5fd0 to 5014766 Compare September 7, 2026 15:21

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Left a non-blocking comment and did not approve; no reviewers were assigned. Bugbot completed with no findings and Security Agent was not running, but this is a functional pcb import connectivity change rather than a small fixup, so it still needs human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@akhilles
akhilles force-pushed the detail/bug-fix/fix-import-split-collapsed-unconnected-stacked-pad-7730e1 branch from 5014766 to 0fb8284 Compare September 7, 2026 16:10

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a non-blocking comment and did not approve; no reviewers were assigned. Bugbot completed with no findings and Security Agent was not running, but this is a functional pcb import connectivity change rather than a small fixup, so it still needs human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants