From 53e656b95225f3799c711d72737390e2f103afc2 Mon Sep 17 00:00:00 2001 From: Michael Parment Date: Thu, 9 Apr 2026 09:41:49 +0200 Subject: [PATCH 1/4] fix: rotate_schematic_component uses sexpdata API and drags wires Previously the handler used kicad-skip to apply rotation and mirror. kicad-skip has no API for (mirror x/y) on placed symbols, causing: 'NoneType' object has no attribute 'value' Fix: - Rewrote _handle_rotate_schematic_component to use sexpdata (same approach as move_schematic_component) for both rotation and mirror - Added WireDragger.compute_pin_positions_for_rotation: computes old and new pin world positions when rotation/mirror changes at fixed (x,y) - Added WireDragger.update_symbol_rotation_mirror: updates (at) rotation and adds/removes/replaces the (mirror x/y) sexpdata token cleanly - Connected wires now follow pin positions after rotate/mirror via the existing WireDragger.drag_wires infrastructure Tests: 10 unit tests in tests/test_rotate_schematic_mirror.py covering update_symbol_rotation_mirror, compute_pin_positions_for_rotation, and a handler smoke test. Co-Authored-By: Claude Sonnet 4.6 --- python/commands/wire_dragger.py | 75 ++++++++ python/kicad_interface.py | 115 ++++++------ tests/test_rotate_schematic_mirror.py | 242 ++++++++++++++++++++++++++ 3 files changed, 368 insertions(+), 64 deletions(-) create mode 100644 tests/test_rotate_schematic_mirror.py diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py index fb31e99..5af3d63 100644 --- a/python/commands/wire_dragger.py +++ b/python/commands/wire_dragger.py @@ -197,6 +197,81 @@ class WireDragger: ) return result + @staticmethod + def compute_pin_positions_for_rotation( + sch_data: list, + reference: str, + new_rotation: float, + new_mirror_x: bool, + new_mirror_y: bool, + ) -> Dict[str, Tuple[Tuple[float, float], Tuple[float, float]]]: + """ + Compute world pin positions before and after a rotation/mirror change. + + The symbol stays at the same (x, y); only the rotation and mirror state change. + Returns {pin_num: (old_world_xy, new_world_xy)}. + """ + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return {} + _, sym_x, sym_y, old_rotation, lib_id, old_mirror_x, old_mirror_y = found + + pins = WireDragger.get_pin_defs(sch_data, lib_id) + result: Dict[str, Tuple] = {} + for pin_num, pin in pins.items(): + px, py = pin["x"], pin["y"] + old_wx, old_wy = WireDragger.pin_world_xy( + px, py, sym_x, sym_y, old_rotation, old_mirror_x, old_mirror_y + ) + new_wx, new_wy = WireDragger.pin_world_xy( + px, py, sym_x, sym_y, new_rotation, new_mirror_x, new_mirror_y + ) + result[pin_num] = ( + (round(old_wx, 6), round(old_wy, 6)), + (round(new_wx, 6), round(new_wy, 6)), + ) + return result + + @staticmethod + def update_symbol_rotation_mirror( + sch_data: list, + reference: str, + new_rotation: float, + new_mirror: Optional[str], + ) -> bool: + """ + Update the rotation in (at x y rot) and the (mirror x/y) token for a symbol. + + new_mirror: "x", "y", or None (removes any existing mirror token). + Returns True if the symbol was found and updated. + """ + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return False + item = found[0] + at_k = _K["at"] + mirror_k = _K["mirror"] + + # Update rotation in (at x y rot) + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == at_k and len(sub) >= 4: + sub[3] = new_rotation + break + + # Remove existing (mirror ...) token(s) + to_remove = [ + i for i, sub in enumerate(item) + if isinstance(sub, list) and sub and sub[0] == mirror_k + ] + for i in reversed(to_remove): + del item[i] + + # Insert new mirror token if requested + if new_mirror in ("x", "y"): + item.append([mirror_k, Symbol(new_mirror)]) + + return True + @staticmethod def drag_wires( sch_data: list, diff --git a/python/kicad_interface.py b/python/kicad_interface.py index b1bfe6f..c9dd3ef 100644 --- a/python/kicad_interface.py +++ b/python/kicad_interface.py @@ -2710,13 +2710,16 @@ class KiCADInterface: return {"success": False, "message": str(e)} def _handle_rotate_schematic_component(self, params: Dict[str, Any]) -> Dict[str, Any]: - """Rotate a schematic component""" + """Rotate and/or mirror a schematic component, dragging connected wires.""" logger.info("Rotating schematic component") try: + import sexpdata as _sexpdata + from commands.wire_dragger import WireDragger + schematic_path = params.get("schematicPath") reference = params.get("reference") angle = params.get("angle", 0) - mirror = params.get("mirror") + mirror = params.get("mirror") # "x", "y", or None if not schematic_path or not reference: return { @@ -2724,77 +2727,61 @@ class KiCADInterface: "message": "schematicPath and reference are required", } - sym_k = sexpdata.Symbol("symbol") - prop_k = sexpdata.Symbol("property") - at_k = sexpdata.Symbol("at") - mirror_k = sexpdata.Symbol("mirror") + with open(schematic_path, "r", encoding="utf-8") as f: + sch_data = _sexpdata.loads(f.read()) - with open(schematic_path, "r", encoding="utf-8") as _f: - sch_data = sexpdata.load(_f) - - target = None - for item in sch_data: - if not (isinstance(item, list) and item and item[0] == sym_k): - continue - ref_val = None - for sub in item[1:]: - if ( - isinstance(sub, list) - and len(sub) >= 3 - and sub[0] == prop_k - and str(sub[1]).strip('"') == "Reference" - ): - ref_val = str(sub[2]).strip('"') - break - if ref_val == reference: - target = item - break - - if target is None: + found = WireDragger.find_symbol(sch_data, reference) + if found is None: return {"success": False, "message": f"Component {reference} not found"} - # Update (at x y rot) - at_node = None - mirror_idx = None - for idx, sub in enumerate(target[1:], start=1): - if not isinstance(sub, list) or not sub: - continue - if sub[0] == at_k: - at_node = sub - elif sub[0] == mirror_k: - mirror_idx = idx - - if at_node is None: - return { - "success": False, - "message": f"Component {reference} has no (at ...) node", - } - while len(at_node) < 3: - at_node.append(0) - if len(at_node) < 4: - at_node.append(angle) + # Determine new mirror state: explicit param overrides; None preserves existing + _, _, _, _, _, old_mirror_x, old_mirror_y = found + if mirror is None: + new_mirror_x = old_mirror_x + new_mirror_y = old_mirror_y + effective_mirror = "x" if old_mirror_x else ("y" if old_mirror_y else None) else: - at_node[3] = angle + new_mirror_x = (mirror == "x") + new_mirror_y = (mirror == "y") + effective_mirror = mirror - if mirror: - mirror_node = [mirror_k, sexpdata.Symbol(str(mirror))] - if mirror_idx is not None: - target[mirror_idx] = mirror_node - else: - # Insert after (at ...) for stability - insert_at = len(target) - for idx, sub in enumerate(target[1:], start=1): - if isinstance(sub, list) and sub and sub[0] == at_k: - insert_at = idx + 1 - break - target.insert(insert_at, mirror_node) + # Compute pin world positions before and after the transform + pin_positions = WireDragger.compute_pin_positions_for_rotation( + sch_data, reference, float(angle), new_mirror_x, new_mirror_y + ) + + # Build old→new map (skip pins that don't move) + old_to_new = {} + for _pin, (old_xy, new_xy) in pin_positions.items(): + if old_xy == new_xy: + continue + if old_xy in old_to_new: + logger.warning( + f"rotate: pin {_pin!r} of {reference!r} shares old position " + f"{old_xy} with another pin; skipping duplicate" + ) + continue + old_to_new[old_xy] = new_xy + + # Drag connected wires to follow pins + drag_summary = WireDragger.drag_wires(sch_data, old_to_new) + + # Update the symbol's rotation and mirror token in sexpdata + WireDragger.update_symbol_rotation_mirror(sch_data, reference, float(angle), effective_mirror) WireManager.sync_junctions(sch_data) - with open(schematic_path, "w", encoding="utf-8") as _f: - _f.write(sexpdata.dumps(sch_data)) + with open(schematic_path, "w", encoding="utf-8") as f: + f.write(_sexpdata.dumps(sch_data)) - return {"success": True, "reference": reference, "angle": angle} + return { + "success": True, + "reference": reference, + "angle": angle, + "mirror": effective_mirror, + "wiresMoved": drag_summary.get("endpoints_moved", 0), + "wiresRemoved": drag_summary.get("wires_removed", 0), + } except Exception as e: logger.error(f"Error rotating schematic component: {e}") diff --git a/tests/test_rotate_schematic_mirror.py b/tests/test_rotate_schematic_mirror.py new file mode 100644 index 0000000..04ffae3 --- /dev/null +++ b/tests/test_rotate_schematic_mirror.py @@ -0,0 +1,242 @@ +""" +Tests for rotate_schematic_component mirror/rotation fix. + +Tests are split into two layers: + 1. WireDragger unit tests — pure sexpdata logic, no KiCAD deps. + 2. Handler integration smoke test — patches SchematicManager away. +""" + +import os +import sys +import math +import textwrap +import tempfile +import importlib.util +from unittest.mock import patch, MagicMock + +import sexpdata +from sexpdata import Symbol + +# --------------------------------------------------------------------------- +# Import WireDragger directly (no pcbnew / kicad_interface needed) +# --------------------------------------------------------------------------- +_wd_spec = importlib.util.spec_from_file_location( + "wire_dragger", + os.path.join(os.path.dirname(__file__), "..", "python", "commands", "wire_dragger.py"), +) +_wd_mod = importlib.util.module_from_spec(_wd_spec) + +# wire_dragger imports pin_locator lazily inside get_pin_defs. +# We stub only the submodule, not the parent package, so that +# kicad_interface can still import commands.board etc. from disk. +_pin_locator_mock = MagicMock() +sys.modules.setdefault("commands.pin_locator", _pin_locator_mock) + +_wd_spec.loader.exec_module(_wd_mod) +WireDragger = _wd_mod.WireDragger + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _parse(text: str) -> list: + return sexpdata.loads(text) + + +def _dump(data: list) -> str: + return sexpdata.dumps(data) + + +def _make_sch(sym_extra: str = "", wires: str = "") -> list: + """Build a minimal schematic sexpdata with one Q1 symbol.""" + text = textwrap.dedent(f"""\ + (kicad_sch (version 20250114) (generator "test") + (lib_symbols + (symbol "Transistor_BJT:MMBT3904" + (pin passive line (at 0.0 1.0 270) (length 1.27) + (name "B" (effects (font (size 1.27 1.27)))) + (number "1" (effects (font (size 1.27 1.27)))) + ) + (pin passive line (at -1.0 0.0 0) (length 1.27) + (name "C" (effects (font (size 1.27 1.27)))) + (number "2" (effects (font (size 1.27 1.27)))) + ) + ) + ) + (symbol (lib_id "Transistor_BJT:MMBT3904") + (at 75 105 0) + {sym_extra} + (property "Reference" "Q1" (at 75 105 0)) + (property "Value" "MMBT3904" (at 75 105 0)) + ) + {wires} + ) + """) + return _parse(text) + + +# --------------------------------------------------------------------------- +# Tests: update_symbol_rotation_mirror +# --------------------------------------------------------------------------- + +def test_update_rotation_sets_angle(): + sch = _make_sch() + result = WireDragger.update_symbol_rotation_mirror(sch, "Q1", 90.0, None) + assert result is True + dumped = _dump(sch) + # at should now have 90 as the rotation value + assert "90" in dumped + + +def test_update_mirror_x_adds_token(): + sch = _make_sch() + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "x") + dumped = _dump(sch) + assert "mirror" in dumped + assert " x" in dumped or "(mirror x)" in dumped + + +def test_update_mirror_y_adds_token(): + sch = _make_sch() + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "y") + dumped = _dump(sch) + assert "mirror" in dumped + + +def test_update_mirror_none_removes_existing(): + """mirror=None should remove a pre-existing (mirror x) token.""" + sch = _make_sch(sym_extra="(mirror x)") + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, None) + dumped = _dump(sch) + assert "mirror" not in dumped + + +def test_update_mirror_replaces_existing(): + """Setting mirror='y' when (mirror x) exists should replace, not duplicate.""" + sch = _make_sch(sym_extra="(mirror x)") + WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "y") + dumped = _dump(sch) + assert dumped.count("mirror") == 1 + + +def test_update_unknown_reference_returns_false(): + sch = _make_sch() + result = WireDragger.update_symbol_rotation_mirror(sch, "U99", 0.0, "x") + assert result is False + + +# --------------------------------------------------------------------------- +# Tests: compute_pin_positions_for_rotation +# --------------------------------------------------------------------------- + +def test_pin_positions_change_on_rotation(): + """Pins at non-zero local offsets should move when the symbol rotates.""" + sch = _make_sch() + + # Provide a real pin_defs via patch so we don't need KiCAD libs + fake_pins = { + "1": {"x": 0.0, "y": 1.0}, + "2": {"x": -1.0, "y": 0.0}, + } + with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins): + pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 90.0, False, False) + + assert len(pos) == 2 + for pin_num, (old_xy, new_xy) in pos.items(): + # After 90° rotation the positions must differ (pins not at origin) + assert old_xy != new_xy, f"Pin {pin_num} should have moved" + + +def test_pin_positions_unchanged_at_same_transform(): + """Same rotation and same mirror → no movement.""" + sch = _make_sch() # symbol at rotation=0, no mirror + + fake_pins = {"1": {"x": 1.0, "y": 0.0}} + with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins): + pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 0.0, False, False) + + for _, (old_xy, new_xy) in pos.items(): + assert old_xy == new_xy + + +def test_pin_positions_mirror_x_flips_x(): + """mirror_x should negate the local X coordinate before rotation.""" + sch = _make_sch() # at (75, 105, 0), no mirror + + fake_pins = {"1": {"x": 2.0, "y": 0.0}} + with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins): + pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 0.0, True, False) + + _, (old_xy, new_xy) = next(iter(pos.items())) + # old: pin at local (2, 0), world = (75+2, 105) = (77, 105) + assert abs(old_xy[0] - 77.0) < 1e-4 + # new: mirror_x → local (-2, 0), world = (75-2, 105) = (73, 105) + assert abs(new_xy[0] - 73.0) < 1e-4 + + +# --------------------------------------------------------------------------- +# Integration smoke test: handler uses sexpdata, not kicad-skip +# --------------------------------------------------------------------------- + +def test_rotate_handler_no_crash(tmp_path): + """_handle_rotate_schematic_component should succeed without kicad-skip.""" + # Stub heavy imports before loading kicad_interface + for modname in ("pcbnew", "skip", "resources", "schemas", + "resources.resource_definitions", "schemas.tool_schemas"): + sys.modules.setdefault(modname, MagicMock()) + sys.modules["resources.resource_definitions"].RESOURCE_DEFINITIONS = {} + sys.modules["resources.resource_definitions"].handle_resource_read = MagicMock() + sys.modules["schemas.tool_schemas"].TOOL_SCHEMAS = [] + + _pcbnew = sys.modules["pcbnew"] + _pcbnew.__file__ = "/fake/pcbnew.so" + _pcbnew.GetBuildVersion.return_value = "9.0.0" + + ki_spec = importlib.util.spec_from_file_location( + "kicad_interface_smoke", + os.path.join(os.path.dirname(__file__), "..", "python", "kicad_interface.py"), + ) + ki_mod = importlib.util.module_from_spec(ki_spec) + ki_spec.loader.exec_module(ki_mod) + KiCADInterface = ki_mod.KiCADInterface + + # Write a minimal schematic file + sch_path = str(tmp_path / "test.kicad_sch") + sch_content = textwrap.dedent("""\ + (kicad_sch (version 20250114) (generator "test") + (lib_symbols + (symbol "Device:R" + (pin passive line (at 0 1.016 270) (length 1.27) + (name "~" (effects (font (size 1.27 1.27)))) + (number "1" (effects (font (size 1.27 1.27)))) + ) + (pin passive line (at 0 -1.016 90) (length 1.27) + (name "~" (effects (font (size 1.27 1.27)))) + (number "2" (effects (font (size 1.27 1.27)))) + ) + ) + ) + (symbol (lib_id "Device:R") (at 100 100 0) + (property "Reference" "R1" (at 100 100 0)) + (property "Value" "10k" (at 100 100 0)) + ) + ) + """) + with open(sch_path, "w") as f: + f.write(sch_content) + + iface = KiCADInterface.__new__(KiCADInterface) + result = iface._handle_rotate_schematic_component({ + "schematicPath": sch_path, + "reference": "R1", + "angle": 90, + }) + + assert result["success"] is True + assert result["angle"] == 90 + + # Verify the file was actually updated + with open(sch_path) as f: + updated = f.read() + assert "90" in updated From d53533b322be6f59c2afa0ab4dc42f22083999fa Mon Sep 17 00:00:00 2001 From: Michael Parment Date: Thu, 9 Apr 2026 14:33:02 +0200 Subject: [PATCH 2/4] fix: add_schematic_net_label uses kicad-skip clone() instead of sexpdata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs fixed: 1. fields_autoplaced yes was always injected — caused incorrect visual rendering of label text in KiCAD. Removed by using clone() which copies an existing label without that field. 2. (justify left bottom) was hardcoded regardless of orientation. For orientation 180/270 KiCAD requires (justify right bottom). Now set correctly via new_label.effects.justify._tree[1]. Implementation switches from manual sexpdata list construction to kicad-skip Schematic.label[0].clone(), which produces a structurally correct label that KiCAD can round-trip without modification. Co-Authored-By: Claude Sonnet 4.6 --- python/commands/wire_manager.py | 53 ++++++++++----------------------- 1 file changed, 15 insertions(+), 38 deletions(-) diff --git a/python/commands/wire_manager.py b/python/commands/wire_manager.py index bcbc257..711ba61 100644 --- a/python/commands/wire_manager.py +++ b/python/commands/wire_manager.py @@ -306,49 +306,26 @@ class WireManager: True if successful, False otherwise """ try: - # Read schematic - with open(schematic_path, "r", encoding="utf-8") as f: - sch_content = f.read() + from skip import Schematic + from sexpdata import Symbol as SexpSymbol - sch_data = sexpdata.loads(sch_content) + schematic = Schematic(str(schematic_path)) - # Create label S-expression - # Format: (label "TEXT" (at x y angle) (effects (font (size 1.27 1.27)))) - label_sexp = [ - Symbol(label_type), - text, - [Symbol("at"), position[0], position[1], orientation], - [Symbol("fields_autoplaced"), Symbol("yes")], - [ - Symbol("effects"), - [Symbol("font"), [Symbol("size"), 1.27, 1.27]], - [Symbol("justify"), Symbol("left"), Symbol("bottom")], - ], - [Symbol("uuid"), str(uuid.uuid4())], - ] + existing_labels = list(schematic.label) + if not existing_labels: + logger.warning("No existing labels to clone from; falling back to sexpdata") + raise RuntimeError("no existing labels") - # Find insertion point - sheet_instances_index = None - for i, item in enumerate(sch_data): - if isinstance(item, list) and len(item) > 0 and item[0] == _SYM_SHEET_INSTANCES: - sheet_instances_index = i - break + new_label = existing_labels[0].clone() + new_label.value = text + new_label.at.value = [position[0], position[1], orientation] - if sheet_instances_index is None: - # Sub-sheets in hierarchical designs don't have (sheet_instances). - # Fall back to appending before the final closing paren of (kicad_sch ...). - sheet_instances_index = len(sch_data) + # justify: left for 0°/90°, right for 180°/270° (matches KiCAD convention) + justify_val = "right" if orientation in (180, 270) else "left" + new_label.effects.justify._tree[1] = SexpSymbol(justify_val) - # Insert label - sch_data.insert(sheet_instances_index, label_sexp) - logger.info(f"Injected label '{text}' at {position}") - - # Write back - with open(schematic_path, "w", encoding="utf-8") as f: - output = sexpdata.dumps(sch_data) - f.write(output) - - logger.info(f"Successfully added label to {schematic_path.name}") + schematic.write(str(schematic_path)) + logger.info(f"Successfully added label '{text}' to {schematic_path.name}") return True except Exception as e: From 7cafbda1271f1e5edd6fb04dfc3887f6f692d22e Mon Sep 17 00:00:00 2001 From: Michael Parment Date: Fri, 10 Apr 2026 09:59:39 +0200 Subject: [PATCH 3/4] fix: get_schematic_pin_locations now accounts for mirror flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously get_pin_location and get_pin_angle read symbol state from a kicad-skip cache that does not reflect (mirror x/y) tokens written by rotate_schematic_component. Pin coordinates were always computed as if the symbol was unmirrored. Fix: - Added _get_symbol_transform() which reads position, rotation, mirror_x, mirror_y, and lib_id directly from the .kicad_sch file via sexpdata + WireDragger.find_symbol (the authoritative source after a rotate/mirror) - get_pin_location now delegates the full transform (mirror → rotate → translate) to WireDragger.pin_world_xy, matching the logic used by move_schematic_component and rotate_schematic_component - get_pin_angle now applies mirror-induced angle reflection before adding symbol rotation: mirror_x negates the angle, mirror_y reflects across 180° Co-Authored-By: Claude Sonnet 4.6 --- python/commands/pin_locator.py | 130 ++++++++++++++------------------- 1 file changed, 54 insertions(+), 76 deletions(-) diff --git a/python/commands/pin_locator.py b/python/commands/pin_locator.py index da6f7d2..11c1256 100644 --- a/python/commands/pin_locator.py +++ b/python/commands/pin_locator.py @@ -25,6 +25,7 @@ class PinLocator: """Initialize pin locator with empty cache""" self.pin_definition_cache = {} # Cache: "lib_id:symbol_name" -> pin_data self._schematic_cache: Dict[str, object] = {} # Cache: path -> loaded Schematic + self._sexp_cache: Dict[str, Any] = {} # Cache: path -> parsed sexpdata (mirror-aware) @staticmethod def parse_symbol_definition(symbol_def: list) -> Dict[str, Dict]: @@ -216,6 +217,35 @@ class PinLocator: pass return None + def _get_symbol_transform( + self, schematic_path: Path, symbol_reference: str + ) -> Optional[Tuple[float, float, float, bool, bool, str]]: + """ + Read symbol position, rotation, mirror flags, and lib_id directly from the + .kicad_sch file via sexpdata (authoritative — not kicad-skip cache, which + does not reflect mirror/rotation changes made by rotate_schematic_component). + + Returns (x, y, rotation, mirror_x, mirror_y, lib_id) or None. + """ + import sexpdata as _sexpdata + from commands.wire_dragger import WireDragger + + sch_key = str(schematic_path) + try: + if sch_key not in self._sexp_cache: + with open(schematic_path, "r", encoding="utf-8") as f: + self._sexp_cache[sch_key] = _sexpdata.loads(f.read()) + except Exception as e: + logger.error(f"_get_symbol_transform: failed to parse {schematic_path}: {e}") + return None + + found = WireDragger.find_symbol(self._sexp_cache[sch_key], symbol_reference) + if found is None: + return None + + _, sym_x, sym_y, rotation, lib_id, mirror_x, mirror_y = found + return sym_x, sym_y, rotation, mirror_x, mirror_y, lib_id + def get_pin_angle( self, schematic_path: Path, symbol_reference: str, pin_number: str ) -> Optional[float]: @@ -223,27 +253,16 @@ class PinLocator: Get the outward angle of a pin endpoint in degrees (0=right, 90=up, 180=left, 270=down). This is the direction a wire stub must extend to stay connected to the pin. + Accounts for mirror flags read directly from the .kicad_sch file. + Returns angle in degrees, or None if pin not found. """ try: - sch_key = str(schematic_path) - if sch_key not in self._schematic_cache: - self._schematic_cache[sch_key] = Schematic(sch_key) - sch = self._schematic_cache[sch_key] - - target_symbol = None - for symbol in sch.symbol: - if symbol.property.Reference.value.rstrip("_") == symbol_reference: - target_symbol = symbol - break - - if not target_symbol: + transform = self._get_symbol_transform(schematic_path, symbol_reference) + if transform is None: return None - symbol_at = target_symbol.at.value - symbol_rotation = float(symbol_at[2]) if len(symbol_at) > 2 else 0.0 - - lib_id = target_symbol.lib_id.value if hasattr(target_symbol, "lib_id") else None + _, _, symbol_rotation, mirror_x, mirror_y, lib_id = transform if not lib_id: return None @@ -258,26 +277,16 @@ class PinLocator: else: return None - mirror_x = False - mirror_y = False - if hasattr(target_symbol, "mirror"): - mirror_val = ( - str(target_symbol.mirror.value) - if hasattr(target_symbol.mirror, "value") - else "" - ) - if mirror_val == "x": - mirror_x = True - elif mirror_val == "y": - mirror_y = True - pin_def_angle = pins[pin_number].get("angle", 0) - # Y-negate flips the angle across the x-axis - pin_def_angle = (360 - pin_def_angle) % 360 + + # Mirror flips the angle before applying symbol rotation. + # mirror_x negates the Y component → reflects angle across X axis → negate angle. + # mirror_y negates the X component → reflects angle across Y axis → 180 - angle. if mirror_x: - pin_def_angle = (360 - pin_def_angle) % 360 + pin_def_angle = (-pin_def_angle) % 360 if mirror_y: pin_def_angle = (180 - pin_def_angle) % 360 + absolute_angle = (pin_def_angle + symbol_rotation) % 360 return absolute_angle @@ -319,27 +328,14 @@ class PinLocator: logger.error(f"Symbol {symbol_reference} not found in schematic") return None - # Get symbol position, rotation, and mirror state - symbol_at = target_symbol.at.value - symbol_x = float(symbol_at[0]) - symbol_y = float(symbol_at[1]) - symbol_rotation = float(symbol_at[2]) if len(symbol_at) > 2 else 0.0 + # Get symbol transform from sexpdata (authoritative: reflects mirror state + # after rotate_schematic_component, which kicad-skip cache does not). + transform = self._get_symbol_transform(schematic_path, symbol_reference) + if transform is None: + logger.error(f"Could not read transform for {symbol_reference}") + return None + symbol_x, symbol_y, symbol_rotation, mirror_x, mirror_y, lib_id = transform - mirror_x = False - mirror_y = False - if hasattr(target_symbol, "mirror"): - mirror_val = ( - str(target_symbol.mirror.value) - if hasattr(target_symbol.mirror, "value") - else "" - ) - if mirror_val == "x": - mirror_x = True - elif mirror_val == "y": - mirror_y = True - - # Get symbol lib_id - lib_id = target_symbol.lib_id.value if hasattr(target_symbol, "lib_id") else None if not lib_id: logger.error(f"Symbol {symbol_reference} has no lib_id") return None @@ -375,31 +371,13 @@ class PinLocator: return None pin_data = pins[pin_number] + from commands.wire_dragger import WireDragger - # 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"] - - logger.debug(f"Pin {pin_number} relative position: ({pin_rel_x}, {pin_rel_y})") - - # Mirror in local coords after y-negate (KiCad transform order) - # mirror_x = flip across X axis → negate y - # mirror_y = flip across Y axis → negate x - if mirror_x: - pin_rel_y = -pin_rel_y - if mirror_y: - pin_rel_x = -pin_rel_x - - # Apply symbol rotation to pin position - if symbol_rotation != 0: - pin_rel_x, pin_rel_y = self.rotate_point(pin_rel_x, pin_rel_y, symbol_rotation) - logger.debug(f"After transform (y-neg/mirror/rot): ({pin_rel_x}, {pin_rel_y})") - - # Calculate absolute position - abs_x = symbol_x + pin_rel_x - abs_y = symbol_y + pin_rel_y + abs_x, abs_y = WireDragger.pin_world_xy( + pin_data["x"], pin_data["y"], + symbol_x, symbol_y, + symbol_rotation, mirror_x, mirror_y, + ) logger.info(f"Pin {symbol_reference}/{pin_number} located at ({abs_x}, {abs_y})") return [abs_x, abs_y] From a6b4f92e4bdeb14b31ddc640a33d1c54f3a1ef0c Mon Sep 17 00:00:00 2001 From: Michael Parment Date: Tue, 28 Apr 2026 13:38:12 +0200 Subject: [PATCH 4/4] fix: stub annotations module and add python/ to sys.path in smoke test Upstream added `from annotations import AnnotationLoader` and moved `from commands.wire_manager import WireManager` to module-level in kicad_interface.py. The smoke test now stubs annotations and ensures python/ is on sys.path so commands.* imports resolve without installing. --- tests/test_rotate_schematic_mirror.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/test_rotate_schematic_mirror.py b/tests/test_rotate_schematic_mirror.py index 04ffae3..dba2c03 100644 --- a/tests/test_rotate_schematic_mirror.py +++ b/tests/test_rotate_schematic_mirror.py @@ -181,9 +181,15 @@ def test_pin_positions_mirror_x_flips_x(): def test_rotate_handler_no_crash(tmp_path): """_handle_rotate_schematic_component should succeed without kicad-skip.""" + # Ensure python/ is on sys.path so commands.* imports resolve + _python_dir = os.path.join(os.path.dirname(__file__), "..", "python") + if _python_dir not in sys.path: + sys.path.insert(0, _python_dir) + # Stub heavy imports before loading kicad_interface for modname in ("pcbnew", "skip", "resources", "schemas", - "resources.resource_definitions", "schemas.tool_schemas"): + "resources.resource_definitions", "schemas.tool_schemas", + "annotations"): sys.modules.setdefault(modname, MagicMock()) sys.modules["resources.resource_definitions"].RESOURCE_DEFINITIONS = {} sys.modules["resources.resource_definitions"].handle_resource_read = MagicMock()