From ab2500fb8d099f11082efa59a3d08e75b3c75967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Noah=20Piqu=C3=A9?= Date: Wed, 15 Apr 2026 16:20:38 +0200 Subject: [PATCH 1/3] fix: correct pin location calculation and symbol reference dedup in kicad-skip Three bugs fixed in the schematic component and pin locator pipeline: 1. component_schematic: remove redundant symbol.append() after clone() kicad-skip's clone() already inserts the raw element into the schematic tree. The subsequent NamedCollection.append() detects the reference as already registered (from the elementRename triggered by setting property.Reference.value) and renames it "R1_" with a trailing underscore, causing all subsequent pin lookups to fail. 2. pin_locator: negate lib y coordinate before rotation lib_symbols in .kicad_sch use library y-up convention; schematic coordinates use y-down. get_pin_location now negates pin_rel_y before applying rotation, matching KiCad's own transform order (same approach as _transform_local_point in schematic_analysis.py). 3. pin_locator: add .rstrip("_") guard in all symbol reference lookups Defensive guard against any residual cases where kicad-skip writes a trailing underscore to the Reference property value. Also fixes the self-test script to use template_with_symbols.kicad_sch (which contains placed _TEMPLATE_* symbols) rather than the expanded template (which only contains lib_symbols definitions and has no cloneable instances). Co-Authored-By: Claude Sonnet 4.6 --- package-lock.json | 6 +++--- python/commands/component_schematic.py | 5 +++-- python/commands/pin_locator.py | 21 +++++++++++---------- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3344ab8..f05631b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -848,9 +848,9 @@ } }, "node_modules/brace-expansion": { - "version": "1.1.14", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.14.tgz", - "integrity": "sha512-MWPGfDxnyzKU7rNOW9SP/c50vi3xrmrua/+6hfPbCS2ABNWfx24vPidzvC7krjU/RTo235sV776ymlsMtGKj8g==", + "version": "1.1.13", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.13.tgz", + "integrity": "sha512-9ZLprWS6EENmhEOpjCYW2c8VkmOvckIJZfkr7rBW6dObmfgJ/L1GpSYW5Hpo9lDz4D1+n0Ckz8rU7FwHDQiG/w==", "dev": true, "license": "MIT", "dependencies": { diff --git a/python/commands/component_schematic.py b/python/commands/component_schematic.py index 6236906..0c49a7b 100644 --- a/python/commands/component_schematic.py +++ b/python/commands/component_schematic.py @@ -255,8 +255,9 @@ class ComponentManager: # Generate new UUID new_symbol.uuid.value = str(uuid.uuid4()) - # Append to schematic - schematic.symbol.append(new_symbol) + # NOTE: clone() already inserts the raw element into the schematic tree. + # Calling schematic.symbol.append() again causes NamedCollection to detect + # the reference as "taken" and rename it to "R1_" (trailing underscore). logger.info(f"Successfully added component {reference} to schematic") return new_symbol diff --git a/python/commands/pin_locator.py b/python/commands/pin_locator.py index bfddca1..d7ede25 100644 --- a/python/commands/pin_locator.py +++ b/python/commands/pin_locator.py @@ -180,7 +180,7 @@ class PinLocator: self._schematic_cache[sch_key] = Schematic(sch_key) sch = self._schematic_cache[sch_key] for symbol in sch.symbol: - if symbol.property.Reference.value == symbol_reference: + if symbol.property.Reference.value.rstrip("_") == symbol_reference: return symbol.lib_id.value if hasattr(symbol, "lib_id") else None except Exception: pass @@ -203,7 +203,7 @@ class PinLocator: target_symbol = None for symbol in sch.symbol: - if symbol.property.Reference.value == symbol_reference: + if symbol.property.Reference.value.rstrip("_") == symbol_reference: target_symbol = symbol break @@ -258,10 +258,11 @@ class PinLocator: self._schematic_cache[sch_key] = Schematic(sch_key) sch = self._schematic_cache[sch_key] - # Find the symbol instance + # Find the symbol instance. + # skip may write references with a trailing "_" (e.g. "R1_") — strip it when comparing. target_symbol = None for symbol in sch.symbol: - ref = symbol.property.Reference.value + ref = symbol.property.Reference.value.rstrip("_") if ref == symbol_reference: target_symbol = symbol break @@ -313,9 +314,11 @@ class PinLocator: pin_data = pins[pin_number] - # Get pin position relative to symbol origin + # Get pin position relative to symbol origin. + # lib_symbols uses library y-up convention; schematic uses y-down. + # Negate y here before rotation, matching KiCad's transform order. pin_rel_x = pin_data["x"] - pin_rel_y = pin_data["y"] + pin_rel_y = -pin_data["y"] logger.debug(f"Pin {pin_number} relative position: ({pin_rel_x}, {pin_rel_y})") @@ -361,7 +364,7 @@ class PinLocator: # Find symbol target_symbol = None for symbol in sch.symbol: - if symbol.property.Reference.value == symbol_reference: + if symbol.property.Reference.value.rstrip("_") == symbol_reference: target_symbol = symbol break @@ -412,9 +415,7 @@ if __name__ == "__main__": # Create test schematic with components (cross-platform temp directory) test_path = Path(tempfile.gettempdir()) / "test_pin_locator.kicad_sch" - template_path = ( - Path(__file__).parent.parent / "templates" / "template_with_symbols_expanded.kicad_sch" - ) + template_path = Path(__file__).parent.parent / "templates" / "template_with_symbols.kicad_sch" shutil.copy(template_path, test_path) print(f"\n✓ Created test schematic: {test_path}") From e164f12ffae86361dc6df91d306edc3bc850f4d7 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sat, 18 Apr 2026 23:42:08 +0100 Subject: [PATCH 2/3] tests: add regression tests for pin location y-axis, reference trailing underscore, and clone dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers the three bugs fixed in the previous commit: - TestAddComponentNoTrailingUnderscore (integration): verifies clone() + no extra append() - TestPinLocatorYAxisNegation (unit): rotation=0 and 90° cases with y-negation - TestPinLocatorReferenceRstrip (unit): lookup tolerates 'R1_' artifact from kicad-skip Co-Authored-By: Claude Sonnet 4.6 --- tests/test_pin_locator_and_component.py | 188 ++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 tests/test_pin_locator_and_component.py diff --git a/tests/test_pin_locator_and_component.py b/tests/test_pin_locator_and_component.py new file mode 100644 index 0000000..462b8f3 --- /dev/null +++ b/tests/test_pin_locator_and_component.py @@ -0,0 +1,188 @@ +""" +Regression tests for three bugs fixed in PR #103: + + 1. component_schematic.py: clone() + redundant append() causes trailing "_" on reference + 2. pin_locator.py: pin_rel_y must be negated (lib y-up → schematic y-down) + 3. pin_locator.py: reference comparison must tolerate trailing "_" from kicad-skip +""" + +import shutil +import sys +import tempfile +import types +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +PYTHON_DIR = Path(__file__).parent.parent / "python" +TEMPLATES_DIR = PYTHON_DIR / "templates" +sys.path.insert(0, str(PYTHON_DIR)) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_TEMPLATE_SCH = TEMPLATES_DIR / "template_with_symbols.kicad_sch" + + +def _stub_symbol(ref: str, at: list, lib_id: str = "Device:R") -> MagicMock: + """Build a minimal kicad-skip symbol stub.""" + sym = MagicMock() + sym.property.Reference.value = ref + sym.at.value = at + sym.lib_id.value = lib_id + return sym + + +# =========================================================================== +# 1. component_schematic — no trailing underscore after clone() +# =========================================================================== + + +@pytest.mark.integration +class TestAddComponentNoTrailingUnderscore: + """clone() already inserts the symbol; a second append() renamed the ref to 'R1_'.""" + + def test_added_component_reference_has_no_trailing_underscore(self): + from skip import Schematic + + with tempfile.TemporaryDirectory() as tmp: + sch_path = Path(tmp) / "test.kicad_sch" + shutil.copy(_TEMPLATE_SCH, sch_path) + + from commands.component_schematic import ComponentManager + + schematic = Schematic(str(sch_path)) + component_def = { + "type": "R", + "reference": "R1", + "value": "10k", + "x": 100, + "y": 100, + "rotation": 0, + } + new_sym = ComponentManager.add_component(schematic, component_def, sch_path) + ref = new_sym.property.Reference.value + assert not ref.endswith( + "_" + ), f"Reference '{ref}' has trailing underscore — redundant append() was re-introduced" + assert ref == "R1", f"Expected 'R1', got '{ref}'" + + +# =========================================================================== +# 2. pin_locator — y-axis sign (lib y-up → schematic y-down) +# =========================================================================== + + +@pytest.mark.unit +class TestPinLocatorYAxisNegation: + """ + Device:R pin 1 is at library y=+3.81 (y-up). + For a symbol centred at (100, 100) with rotation=0, the schematic absolute y + must be 100 - 3.81 = 96.19, NOT 100 + 3.81 = 103.81. + """ + + @pytest.fixture(autouse=True) + def setup(self): + # Stub sexpdata and skip so the module can be imported without them installed + for mod_name in ("sexpdata", "skip"): + sys.modules.setdefault(mod_name, types.ModuleType(mod_name)) + from commands.pin_locator import PinLocator + + self.locator = PinLocator() + + def test_pin1_y_above_center_for_rotation_0(self): + """Pin at lib y=+3.81 should appear *above* the symbol centre (lower y value).""" + sym = _stub_symbol("R1", at=[100.0, 100.0, 0.0]) + self.locator._schematic_cache["test.kicad_sch"] = MagicMock(symbol=[sym]) + # Patch get_symbol_pins to return a Device:R-like pin definition + with patch.object( + self.locator, + "get_symbol_pins", + return_value={"1": {"x": 0.0, "y": 3.81, "angle": 270, "name": "~"}}, + ): + result = self.locator.get_pin_location(Path("test.kicad_sch"), "R1", "1") + + assert result is not None + x, y = result + assert abs(x - 100.0) < 1e-6, f"x should be 100.0, got {x}" + assert abs(y - 96.19) < 1e-4, ( + f"y should be ~96.19 (above centre), got {y}. " + "y was not negated — library y-up convention mismatch." + ) + + def test_pin2_y_below_center_for_rotation_0(self): + """Pin at lib y=-3.81 should appear *below* the symbol centre (higher y value).""" + sym = _stub_symbol("R1", at=[100.0, 100.0, 0.0]) + self.locator._schematic_cache["test.kicad_sch"] = MagicMock(symbol=[sym]) + with patch.object( + self.locator, + "get_symbol_pins", + return_value={"2": {"x": 0.0, "y": -3.81, "angle": 90, "name": "~"}}, + ): + result = self.locator.get_pin_location(Path("test.kicad_sch"), "R1", "2") + + assert result is not None + _, y = result + assert abs(y - 103.81) < 1e-4, f"y should be ~103.81 (below centre), got {y}." + + def test_pin1_rotated_90(self): + """ + Symbol rotated 90°. Pin at lib (x=0, y=+3.81). + After y-negation: (0, -3.81). After 90° CCW rotation: (x=3.81, y=0). + Absolute: (100+3.81, 100+0) = (103.81, 100). + """ + sym = _stub_symbol("C1", at=[100.0, 100.0, 90.0]) + self.locator._schematic_cache["test.kicad_sch"] = MagicMock(symbol=[sym]) + with patch.object( + self.locator, + "get_symbol_pins", + return_value={"1": {"x": 0.0, "y": 3.81, "angle": 270, "name": "~"}}, + ): + result = self.locator.get_pin_location(Path("test.kicad_sch"), "C1", "1") + + assert result is not None + x, y = result + assert abs(x - 103.81) < 1e-4, f"x should be ~103.81, got {x}" + assert abs(y - 100.0) < 1e-4, f"y should be ~100.0, got {y}" + + +# =========================================================================== +# 3. pin_locator — .rstrip("_") tolerance in reference lookup +# =========================================================================== + + +@pytest.mark.unit +class TestPinLocatorReferenceRstrip: + """kicad-skip may write 'R1_' — lookups must still find 'R1'.""" + + @pytest.fixture(autouse=True) + def setup(self): + for mod_name in ("sexpdata", "skip"): + sys.modules.setdefault(mod_name, types.ModuleType(mod_name)) + from commands.pin_locator import PinLocator + + self.locator = PinLocator() + + def test_get_pin_location_finds_symbol_with_trailing_underscore(self): + # Symbol stored in schematic with reference 'R1_' (kicad-skip artifact) + sym = _stub_symbol("R1_", at=[50.0, 50.0, 0.0]) + self.locator._schematic_cache["sch.kicad_sch"] = MagicMock(symbol=[sym]) + with patch.object( + self.locator, + "get_symbol_pins", + return_value={"1": {"x": 0.0, "y": 3.81, "angle": 270, "name": "~"}}, + ): + # Caller uses clean reference 'R1'; should still resolve + result = self.locator.get_pin_location(Path("sch.kicad_sch"), "R1", "1") + + assert ( + result is not None + ), "get_pin_location returned None for reference 'R1' when schematic stores 'R1_'" + + def test_get_pin_location_returns_none_for_genuinely_missing_symbol(self): + sym = _stub_symbol("R2", at=[50.0, 50.0, 0.0]) + self.locator._schematic_cache["sch.kicad_sch"] = MagicMock(symbol=[sym]) + result = self.locator.get_pin_location(Path("sch.kicad_sch"), "R1", "1") + assert result is None From ef42eb60bbd74980b857582edbf73da33ba6fd4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Noah=20Piqu=C3=A9?= Date: Wed, 15 Apr 2026 16:20:38 +0200 Subject: [PATCH 3/3] fix: correct pin location calculation and symbol reference dedup in kicad-skip Three bugs fixed in the schematic component and pin locator pipeline: 1. component_schematic: remove redundant symbol.append() after clone() kicad-skip's clone() already inserts the raw element into the schematic tree. The subsequent NamedCollection.append() detects the reference as already registered (from the elementRename triggered by setting property.Reference.value) and renames it "R1_" with a trailing underscore, causing all subsequent pin lookups to fail. 2. pin_locator: negate lib y coordinate before rotation lib_symbols in .kicad_sch use library y-up convention; schematic coordinates use y-down. get_pin_location now negates pin_rel_y before applying rotation, matching KiCad's own transform order (same approach as _transform_local_point in schematic_analysis.py). 3. pin_locator: add .rstrip("_") guard in all symbol reference lookups Defensive guard against any residual cases where kicad-skip writes a trailing underscore to the Reference property value. Also fixes the self-test script to use template_with_symbols.kicad_sch (which contains placed _TEMPLATE_* symbols) rather than the expanded template (which only contains lib_symbols definitions and has no cloneable instances). Co-Authored-By: Claude Sonnet 4.6 --- python/commands/pin_locator.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/python/commands/pin_locator.py b/python/commands/pin_locator.py index d7ede25..204f593 100644 --- a/python/commands/pin_locator.py +++ b/python/commands/pin_locator.py @@ -167,6 +167,10 @@ class PinLocator: cos_a = math.cos(angle_rad) sin_a = math.sin(angle_rad) + # Standard counter-clockwise rotation (math convention, Y-up). + # Callers are responsible for any y-axis negation required to convert + # library coordinates (y-up) to schematic coordinates (y-down) before + # passing values here — see get_pin_location and _transform_local_point. rotated_x = x * cos_a - y * sin_a rotated_y = x * sin_a + y * cos_a