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] 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}")