nearxos/kicad-mcp-server
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(pin_locator): keep outer pin when a symbol defines a number twice (#179)

Browse Source 
Some community-generated KiCad symbol libraries (e.g.
PCM_Diode_Schottky_AKL:MBRS130) define each pin number twice — once as a
visible "real" pin with non-zero length whose ``at`` coordinate is the
wire-connection endpoint, and once as an inner zero-length "ghost" pin
used as an internal graphic-anchoring join. Both definitions live inside
the same ``lib_symbols`` block.

``parse_symbol_definition`` stored pins via ``pins[number] = pin_data``
— a plain assignment. Each duplicate-numbered pin encountered during the
recursive walk overwrote the previous one. The recursion order put the
ghost pins last for MBRS130, so the ghost won and ``get_pin_location``
returned a coordinate that did not match any wire/label.

Downstream this caused ``get_connections_for_net`` to silently miss diode
pins on the rails they were wired to — on a real schematic, querying
``+BATT`` returned 8 of 9 expected nodes (D1/1 absent) and ``+3V3``
returned 44 of 46 (D1/2 and D2/2 absent), because the BFS could not find
the diode's pin endpoint at the labelled position.

Fix: when the same pin number is defined more than once, keep the entry
with the greater ``length``. The outer real pin has length > 0; the
inner ghost has length == 0. Strict-greater comparison resolves ties to
first-encountered, so legitimate same-length duplicates (e.g., per-unit
repetitions in multi-unit symbols) keep stable existing behaviour.

Tests: four unit tests in ``tests/test_pin_locator_duplicate_pin_defs.py``
cover (a) outer-then-ghost order (the real bug), (b) ghost-then-outer
order (length-not-order heuristic), (c) no-duplicate baseline regression,
and (d) equal-length tie keeps first-encountered. Two of the four fail
on main, all four pass on this branch.

Full suite: 671 passed, 11 skipped, 0 regressions (modulo the pre-existing
tests/test_get_pin_angle.py collection error which is unrelated to
pin_locator).

End-to-end on the real schematic that triggered the report: after the
fix ``get_connections_for_net('+BATT')`` returns all 9 expected nodes
matching ``kicad-cli sch export netlist`` exactly. The companion fix in
PR #177 (wire_connectivity pwr-flag bridge) closes the orthogonal
over-merge bug; together they bring net membership to full parity with
kicad-cli on schematics that use both ``PWR_FLAG`` markers and
diodes from community libraries.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
blinksoft
2026-05-18 13:53:37 -04:00
committed by GitHub
parent 68b8bbf2fb
commit ff6ae63b8c
2 changed files with 167 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
python/commands/pin_locator.py
View File

@@ -80,9 +80,21 @@ class PinLocator:
elif item[0] == Symbol("number") and len(item) >= 2:
pin_data["number"] = str(item[1]).strip('"')
# Store by pin number
# Store by pin number. When the same pin number is defined
# more than once in a single symbol — which happens in some
# community-generated symbols (e.g.,
# ``PCM_Diode_Schottky_AKL:MBRS130``) where an inner
# zero-length "ghost" pin overlaps the real outer pin — keep
# the definition with the greater ``length``. That is the pin
# with a visible stub; its ``at`` coordinate is the wire-
# connection endpoint that matches where labels and wires
# are actually placed. Ties resolve to first-encountered, so
# legitimate same-length duplicates (e.g., per-unit
# repetitions in multi-unit symbols) retain stable ordering.
if pin_data["number"]:
pins[pin_data["number"]] = pin_data
existing = pins.get(pin_data["number"])
if existing is None or pin_data["length"] > existing["length"]:
pins[pin_data["number"]] = pin_data
# Recurse into sublists
for item in sexp:

153
tests/test_pin_locator_duplicate_pin_defs.py Normal file
View File

@@ -0,0 +1,153 @@
"""
Regression tests for ``PinLocator.parse_symbol_definition`` when a symbol
defines the same pin number more than once.
Background
----------
Some community-generated symbol libraries — for example
``PCM_Diode_Schottky_AKL:MBRS130`` — include both an outer "real" pin
with a visible stub (non-zero ``length``) and an inner zero-length "ghost"
pin at a different ``at`` coordinate. Both definitions share the same pin
number. Conceptually the ghost is an internal join used for symbol
graphic anchoring; the real outer pin is where wires and labels are
placed by the schematic author.
Before this fix, ``parse_symbol_definition`` stored pins as
``pins[pin_data["number"]] = pin_data`` — a plain assignment. Each
duplicate-numbered definition encountered during recursion overwrote the
previous one. The recursion order put the ghost pins last for the MBRS130
symbol, so the ghost won and ``get_pin_location`` returned a coordinate
that did not match any wire/label. As a knock-on effect,
``get_connections_for_net`` failed to discover diode pins on the rails
they were wired to (e.g. ``D1/1`` on ``+BATT``).
The fix: when storing a duplicate-numbered pin, keep the entry with the
greater ``length``. The outer "real" pin has length > 0 (a visible stub
out to the wire-attach point); the inner ghost has length == 0. Ties
resolve to first-encountered, so legitimate same-length duplicates
(e.g., per-unit repetitions in multi-unit symbols) keep stable ordering
and existing behaviour.
"""
import sys
from pathlib import Path
import pytest
from sexpdata import Symbol
sys.path.insert(0, str(Path(__file__).parent.parent / "python"))
from commands.pin_locator import PinLocator # noqa: E402
# ---------------------------------------------------------------------------
# Helper: build a (symbol …) sexp matching the structure KiCad writes
# ---------------------------------------------------------------------------
def _pin_sexp(number: str, name: str, x: float, y: float, angle: int, length: float):
"""Return an s-expression list that mimics a KiCad ``(pin …)`` definition."""
return [
Symbol("pin"),
Symbol("passive"),
Symbol("line"),
[Symbol("at"), x, y, angle],
[Symbol("length"), length],
[Symbol("name"), f'"{name}"'],
[Symbol("number"), f'"{number}"'],
]
# ---------------------------------------------------------------------------
# Tests
# ---------------------------------------------------------------------------
@pytest.mark.unit
class TestParseSymbolDefinitionDuplicatePinNumbers:
"""``parse_symbol_definition`` must pick the outer (length>0) pin when a
symbol defines the same pin number twice — once with a visible stub and
once as a zero-length ghost."""
def test_outer_pin_wins_when_ghost_appears_after(self) -> None:
"""Mirrors the real bug: outer pin defined first, ghost defined
later. The ghost must not clobber the outer pin in the result."""
symbol_def = [
Symbol("symbol"),
"MBRS130",
# Outer "real" pins — the ones with a visible stub.
_pin_sexp("2", "A", -3.81, 0, 0, 2.54),
_pin_sexp("1", "K", 3.81, 0, 180, 2.54),
# Inner ghost pins — zero-length, would have clobbered before.
_pin_sexp("2", "A", -2.54, -2.54, 0, 0),
_pin_sexp("1", "K", 2.54, 2.54, 180, 0),
]
pins = PinLocator.parse_symbol_definition(symbol_def)
# Pin 1 = outer K at (3.81, 0), length 2.54 — not the (2.54, 2.54) ghost.
assert pins["1"]["x"] == 3.81, pins
assert pins["1"]["y"] == 0.0, pins
assert pins["1"]["length"] == 2.54, pins
# Pin 2 = outer A at (-3.81, 0), length 2.54 — not the (-2.54, -2.54) ghost.
assert pins["2"]["x"] == -3.81, pins
assert pins["2"]["y"] == 0.0, pins
assert pins["2"]["length"] == 2.54, pins
def test_outer_pin_wins_when_ghost_appears_first(self) -> None:
"""Symmetry check: ghost defined first, outer pin defined later. The
outer pin must overwrite the ghost (the heuristic is length-based,
not order-based)."""
symbol_def = [
Symbol("symbol"),
"MBRS130_alt_ordering",
_pin_sexp("1", "K", 2.54, 2.54, 180, 0),
_pin_sexp("2", "A", -2.54, -2.54, 0, 0),
_pin_sexp("2", "A", -3.81, 0, 0, 2.54),
_pin_sexp("1", "K", 3.81, 0, 180, 2.54),
]
pins = PinLocator.parse_symbol_definition(symbol_def)
assert pins["1"]["x"] == 3.81, pins
assert pins["1"]["length"] == 2.54, pins
assert pins["2"]["x"] == -3.81, pins
assert pins["2"]["length"] == 2.54, pins
def test_no_duplicates_unaffected(self) -> None:
"""Regression: a normal symbol with unique pin numbers stores the
same data it always did. Behaviour for the common case is
unchanged."""
symbol_def = [
Symbol("symbol"),
"Device:R",
_pin_sexp("1", "~", 0, 3.81, 270, 1.27),
_pin_sexp("2", "~", 0, -3.81, 90, 1.27),
]
pins = PinLocator.parse_symbol_definition(symbol_def)
assert pins["1"]["x"] == 0.0
assert pins["1"]["y"] == 3.81
assert pins["1"]["length"] == 1.27
assert pins["2"]["x"] == 0.0
assert pins["2"]["y"] == -3.81
assert pins["2"]["length"] == 1.27
def test_equal_length_duplicates_keep_first_encountered(self) -> None:
"""When two definitions of the same pin number have equal length
(e.g. per-unit repetitions in a multi-unit symbol), the first one
encountered wins. The fix's length-strict-greater comparison keeps
this case stable and matches pre-fix behaviour for the only case
that pre-fix code handled correctly."""
symbol_def = [
Symbol("symbol"),
"MultiUnit_Example",
_pin_sexp("1", "VCC", 0, 5.0, 270, 1.27),
_pin_sexp("1", "VCC", 0, 10.0, 270, 1.27), # same length, different y
]
pins = PinLocator.parse_symbol_definition(symbol_def)
# First definition (y=5.0) wins.
assert pins["1"]["y"] == 5.0, pins