diff --git a/python/commands/pin_locator.py b/python/commands/pin_locator.py index 5b664b4..e49289b 100644 --- a/python/commands/pin_locator.py +++ b/python/commands/pin_locator.py @@ -279,15 +279,26 @@ class PinLocator: pin_def_angle = pins[pin_number].get("angle", 0) - # Mirror flips the angle before applying symbol rotation. - # mirror_x flips the X component of local vectors → reflects across Y axis → 180 - angle. - # mirror_y flips the Y component of local vectors → reflects across X axis → negate angle. - if mirror_x: - pin_def_angle = (180 - pin_def_angle) % 360 - if mirror_y: - pin_def_angle = (-pin_def_angle) % 360 + # Mirror this exactly the way WireDragger.pin_world_xy does, in the + # same order: Y-flip (lib Y-up → screen Y-down) → mirror → rotate. + # + # Y-flip on an angle: negate it (reflects across X axis). + pin_def_angle = (-pin_def_angle) % 360 - absolute_angle = (pin_def_angle + symbol_rotation) % 360 + # eeschema (symbol.h:43-44): + # (mirror x) = SYM_MIRROR_X = TRANSFORM(1,0,0,-1) → negates Y → + # reflect angle across X axis → -angle. + # (mirror y) = SYM_MIRROR_Y = TRANSFORM(-1,0,0,1) → negates X → + # reflect angle across Y axis → 180 - angle. + if mirror_x: + pin_def_angle = (-pin_def_angle) % 360 + if mirror_y: + pin_def_angle = (180 - pin_def_angle) % 360 + + # eeschema's rotation TRANSFORM is screen-CCW in Y-down, which is + # math-CW in standard atan2 convention — so subtract the rotation + # to match `pin_world_xy`'s `_rotate(..., -rotation)` call. + absolute_angle = (pin_def_angle - symbol_rotation) % 360 return absolute_angle except Exception: diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py index 793457e..7c8e110 100644 --- a/python/commands/wire_dragger.py +++ b/python/commands/wire_dragger.py @@ -75,14 +75,17 @@ class WireDragger: if not (isinstance(item, list) and item and item[0] == sym_k): continue - # Check Reference property + # Check Reference property. + # kicad-skip may write a trailing "_" on references (e.g. "R1_") when + # cloning symbols; strip it so callers passing the canonical "R1" + # still find the symbol. Mirrors the rstrip in PinLocator.get_pin_location. ref_val = None for sub in item[1:]: if isinstance(sub, list) and len(sub) >= 3 and sub[0] == prop_k: if str(sub[1]).strip('"') == "Reference": ref_val = str(sub[2]).strip('"') break - if ref_val != reference: + if ref_val is None or ref_val.rstrip("_") != reference: continue old_x = old_y = rotation = 0.0 @@ -153,15 +156,22 @@ class WireDragger: Compute the world coordinate of a pin given the symbol transform. Library pins are stored Y-up; the schematic is Y-down. Order matches - eeschema: mirror in lib space → Y-flip to screen → rotate → translate. - Without the Y-flip, polarized parts get pin 1/pin 2 silently swapped. + eeschema: Y-flip to screen → mirror → rotate (screen-CCW) → translate. + + eeschema's TRANSFORM matrix for rotation 90 is (0, 1, -1, 0) — + i.e. screen-CCW in Y-down: (x, y) → (y, -x). Our `_rotate` helper is + standard math (Y-up CCW), so we negate the rotation angle to convert. + + Mirror axis semantics match eeschema's symbol.h: + (mirror x) = SYM_MIRROR_X = TRANSFORM(1, 0, 0, -1) → negates Y. + (mirror y) = SYM_MIRROR_Y = TRANSFORM(-1, 0, 0, 1) → negates X. """ - lx, ly = px, py + lx, ly = px, -py # Y-flip: lib Y-up → screen Y-down if mirror_x: - lx = -lx + ly = -ly # SYM_MIRROR_X negates screen-Y if mirror_y: - ly = -ly - rx, ry = _rotate(lx, -ly, rotation) + lx = -lx # SYM_MIRROR_Y negates screen-X + rx, ry = _rotate(lx, ly, -rotation) # negate angle: math-CCW → screen-CCW return sym_x + rx, sym_y + ry @staticmethod diff --git a/tests/test_add_schematic_component.py b/tests/test_add_schematic_component.py index dff0227..92f1d1f 100644 --- a/tests/test_add_schematic_component.py +++ b/tests/test_add_schematic_component.py @@ -19,9 +19,7 @@ EMPTY_SCH = TEMPLATES_DIR / "empty.kicad_sch" def _write_temp_sch(content: str) -> Path: - tmp = tempfile.NamedTemporaryFile( - suffix=".kicad_sch", delete=False, mode="w", encoding="utf-8" - ) + tmp = tempfile.NamedTemporaryFile(suffix=".kicad_sch", delete=False, mode="w", encoding="utf-8") tmp.write(content) tmp.close() return Path(tmp.name) @@ -36,7 +34,10 @@ def _unit_values_in_file(path: Path) -> list[int]: """Return all (unit N) values written for symbol instances in the schematic.""" content = path.read_text() # Match top-level symbol instances: (symbol (lib_id ...) (at ...) (unit N) ...) - return [int(n) for n in re.findall(r"\(symbol \(lib_id [^)]+\) \(at [^)]+\) \(unit (\d+)\)", content)] + return [ + int(n) + for n in re.findall(r"\(symbol \(lib_id [^)]+\) \(at [^)]+\) \(unit (\d+)\)", content) + ] # --------------------------------------------------------------------------- @@ -155,18 +156,20 @@ class TestHandlerAddSchematicComponent: def test_unit_defaults_to_1_in_handler(self, tmp_path: Any) -> None: sch = tmp_path / "test.kicad_sch" shutil.copy(EMPTY_SCH, sch) - result = self._call_handler({ - "schematicPath": str(sch), - "component": { - "library": "Device", - "type": "R", - "reference": "R99", - "value": "1k", - "x": 10, - "y": 10, - # no "unit" key — should default to 1 - }, - }) + result = self._call_handler( + { + "schematicPath": str(sch), + "component": { + "library": "Device", + "type": "R", + "reference": "R99", + "value": "1k", + "x": 10, + "y": 10, + # no "unit" key — should default to 1 + }, + } + ) assert result["success"] is True units = _unit_values_in_file(sch) assert 1 in units @@ -174,18 +177,82 @@ class TestHandlerAddSchematicComponent: def test_unit_2_passed_through_handler(self, tmp_path: Any) -> None: sch = tmp_path / "test.kicad_sch" shutil.copy(EMPTY_SCH, sch) - result = self._call_handler({ - "schematicPath": str(sch), - "component": { - "library": "Device", - "type": "R", - "reference": "U10", - "value": "TLP291-4", - "x": 25, - "y": 35, - "unit": 2, - }, - }) + result = self._call_handler( + { + "schematicPath": str(sch), + "component": { + "library": "Device", + "type": "R", + "reference": "U10", + "value": "TLP291-4", + "x": 25, + "y": 35, + "unit": 2, + }, + } + ) assert result["success"] is True units = _unit_values_in_file(sch) assert 2 in units + + +# --------------------------------------------------------------------------- +# Mirror parameter — known gap +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestAddComponentMirrorParam: + """ComponentManager.add_component does NOT honor a 'mirror' kwarg today. + + The MCP add_schematic_component tool schema also doesn't expose mirror. + A mirror is currently only applicable post-add via rotate_schematic_component. + + These tests pin down the silent-drop behavior so a fixture that passes + 'mirror': 'x' and then asserts something against the resulting schematic + cannot accidentally pass for the wrong reason (the symbol ends up + unmirrored). If/when add_component grows real mirror support, update both + tests together — the second test then becomes the positive assertion.""" + + def setup_method(self) -> None: + from commands.component_schematic import ComponentManager + from commands.schematic import SchematicManager + + self.ComponentManager = ComponentManager + self.SchematicManager = SchematicManager + + def _add(self, sch_path: Path, mirror_value: Any) -> None: + sch = self.SchematicManager.load_schematic(str(sch_path)) + params = { + "type": "R", + "reference": "R1", + "value": "10k", + "x": 100.0, + "y": 100.0, + "rotation": 0, + } + if mirror_value is not None: + params["mirror"] = mirror_value + self.ComponentManager.add_component(sch, params, sch_path) + self.SchematicManager.save_schematic(sch, str(sch_path)) + + def test_mirror_x_arg_is_silently_dropped(self, tmp_path: Any) -> None: + sch = tmp_path / "mirror_x.kicad_sch" + shutil.copy(EMPTY_SCH, sch) + self._add(sch, "x") + text = sch.read_text() + assert "(mirror x)" not in text, ( + "ComponentManager.add_component now appears to honor mirror='x'. " + "Update _build_mirror_case in test_pin_world_xy_eeschema_truth.py " + "to drop the post-add mirror application and remove this test." + ) + + def test_mirror_y_arg_is_silently_dropped(self, tmp_path: Any) -> None: + sch = tmp_path / "mirror_y.kicad_sch" + shutil.copy(EMPTY_SCH, sch) + self._add(sch, "y") + text = sch.read_text() + assert "(mirror y)" not in text, ( + "ComponentManager.add_component now appears to honor mirror='y'. " + "See sibling test_mirror_x_arg_is_silently_dropped." + ) diff --git a/tests/test_hierarchical_pad_net_map.py b/tests/test_hierarchical_pad_net_map.py index 922edbb..6ff8a0f 100644 --- a/tests/test_hierarchical_pad_net_map.py +++ b/tests/test_hierarchical_pad_net_map.py @@ -36,8 +36,7 @@ sys.path.insert(0, str(Path(__file__).parent.parent / "python")) # pin "2" at ( 1.27, 0) → wire connects on the right # --------------------------------------------------------------------------- -_SCH_WITH_TESTLIB_R = textwrap.dedent("""\ - (kicad_sch (version 20231120) +_LIB_SYMBOLS_BLOCK = textwrap.dedent("""\ (lib_symbols (symbol "TestLib:R" (symbol "TestLib:R_1_1" @@ -52,12 +51,40 @@ _SCH_WITH_TESTLIB_R = textwrap.dedent("""\ ) ) ) - ) """) +_SCH_WITH_TESTLIB_R = "(kicad_sch (version 20231120)\n" + _LIB_SYMBOLS_BLOCK + ")\n" + _SCH_EMPTY = "(kicad_sch (version 20231120))" +def _build_sch_with_instances( + instances: "list[tuple[str, str, float, float, float]] | None" = None, +) -> str: + """Build a .kicad_sch text with TestLib:R lib_symbols + the given symbol instances. + + Each instance tuple: (reference, lib_id, x, y, rotation). + + Required because PinLocator._get_symbol_transform reads the symbol position, + rotation, and lib_id directly from disk via sexpdata (it does not consult + the kicad-skip cache that the tests mock for labels/wires). + """ + parts = ["(kicad_sch (version 20231120)", _LIB_SYMBOLS_BLOCK] + for i, (ref, lib_id, x, y, rot) in enumerate(instances or [], start=1): + uuid = f"00000000-0000-0000-0000-{i:012d}" + parts.append( + f' (symbol (lib_id "{lib_id}") (at {x} {y} {rot}) (unit 1)\n' + f" (uuid {uuid})\n" + f' (property "Reference" "{ref}" (at {x} {y - 2.54} 0))\n' + f' (property "Value" "~" (at {x} {y + 2.54} 0))\n' + f' (pin "1" (uuid {uuid[:-1]}a))\n' + f' (pin "2" (uuid {uuid[:-1]}b))\n' + f" )\n" + ) + parts.append(")\n") + return "".join(parts) + + # --------------------------------------------------------------------------- # Mock helpers # --------------------------------------------------------------------------- @@ -176,7 +203,7 @@ class TestLabelAtPin: def test_global_label_pin1(self, iface, tmp_path): """Global label at pin-1 position → (R1, 1) in map.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # R1 at (10, 10); pin 1 abs = (10 − 1.27, 10) = (8.73, 10) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -189,7 +216,7 @@ class TestLabelAtPin: def test_global_label_pin2(self, iface, tmp_path): """Global label at pin-2 position → (R1, 2) in map.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # pin 2 abs = (10 + 1.27, 10) = (11.27, 10) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -202,7 +229,7 @@ class TestLabelAtPin: def test_both_pins_mapped(self, iface, tmp_path): """Labels at both pin positions → both (ref, pin) keys present.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], global_labels=[ @@ -217,7 +244,7 @@ class TestLabelAtPin: def test_local_label_also_works(self, iface, tmp_path): """Local (net) labels are treated identically to global labels.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], labels=[_lbl_mock("LOCAL_NET", 8.73, 10.0)], @@ -228,7 +255,7 @@ class TestLabelAtPin: def test_hierarchical_label_also_works(self, iface, tmp_path): """Hierarchical labels are treated identically to global labels.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], hier_labels=[_lbl_mock("HIER_NET", 8.73, 10.0)], @@ -249,7 +276,7 @@ class TestLabelViaWire: def test_label_one_hop_away(self, iface, tmp_path): """Label at wire start, wire end at pin → net assigned.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # pin 1 at (8.73, 10); label at (5.0, 10); wire (5.0,10)→(8.73,10) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -262,7 +289,7 @@ class TestLabelViaWire: def test_label_two_hops_away(self, iface, tmp_path): """Net propagates through two chained wire segments.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # label at (3.0, 10); wire1: 3→6; wire2: 6→8.73 → pin 1 mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -330,7 +357,7 @@ class TestMultipleSubsheets: sub_dir = tmp_path / "sheets" sub_dir.mkdir() sub = sub_dir / "component_sheet.kicad_sch" - sub.write_text(_SCH_WITH_TESTLIB_R) + sub.write_text(_build_sch_with_instances([("R2", "TestLib:R", 10.0, 10.0, 0)])) top_mock = _sch_mock() # top sheet has no components sub_mock = _sch_mock( @@ -340,6 +367,7 @@ class TestMultipleSubsheets: def _factory(path: str) -> MagicMock: from pathlib import Path as _P + return sub_mock if _P(path).name == "component_sheet.kicad_sch" else top_mock with ( @@ -354,11 +382,11 @@ class TestMultipleSubsheets: def test_top_and_sub_components_merged(self, iface, tmp_path): """Components from both top-level and sub-sheet appear in the same map.""" top = tmp_path / "top.kicad_sch" - top.write_text(_SCH_WITH_TESTLIB_R) + top.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) sub_dir = tmp_path / "sheets" sub_dir.mkdir() sub = sub_dir / "component_sheet.kicad_sch" - sub.write_text(_SCH_WITH_TESTLIB_R) + sub.write_text(_build_sch_with_instances([("R2", "TestLib:R", 10.0, 10.0, 0)])) top_mock = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -371,6 +399,7 @@ class TestMultipleSubsheets: def _factory(path: str) -> MagicMock: from pathlib import Path as _P + return sub_mock if _P(path).name == "component_sheet.kicad_sch" else top_mock with ( @@ -391,17 +420,19 @@ class TestMultipleSubsheets: @pytest.mark.unit class TestRotatedSymbol: def test_90_degree_rotation(self, iface, tmp_path): - """A 90° CCW rotation maps pin (-1.27, 0) → (0, -1.27) in local coords.""" + """eeschema rot=90 is CCW in screen Y-down: TRANSFORM(0,1,-1,0). + Pin 1 lib (-1.27, 0) → internal (-1.27, 0) → (0,1,-1,0) applied = (0, 1.27) + → world (10.0, 11.27). + Pin 2 lib ( 1.27, 0) → internal ( 1.27, 0) → (0,1,-1,0) applied = (0, -1.27) + → world (10.0, 8.73). + Verified vs kicad-cli netlist on a Device:D rotated 90.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) - # R1 at (10, 10), rotation=90° - # pin 1 (-1.27, 0) rotated 90° CCW → (0, -1.27) → abs (10.0, 8.73) - # pin 2 ( 1.27, 0) rotated 90° CCW → (0, 1.27) → abs (10.0, 11.27) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 90)])) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0, rotation=90)], global_labels=[ - _lbl_mock("UP_NET", 10.0, 8.73), - _lbl_mock("DN_NET", 10.0, 11.27), + _lbl_mock("UP_NET", 10.0, 11.27), + _lbl_mock("DN_NET", 10.0, 8.73), ], ) pad_net_map, _ = _call(iface, sch, mock_sch) diff --git a/tests/test_move_with_wire_preservation.py b/tests/test_move_with_wire_preservation.py index 65579cb..4edbd4b 100644 --- a/tests/test_move_with_wire_preservation.py +++ b/tests/test_move_with_wire_preservation.py @@ -222,13 +222,14 @@ class TestComputePinPositions: assert abs(new2[1] - 23.81) < 1e-4 def test_resistor_rotated_90(self) -> None: - """Device:R at (100, 100) rot=90. Pin 1 lib (0, +3.81) → y-flipped to - (0, -3.81) → rotated 90° → (3.81, 0) → world (103.81, 100).""" + """Device:R at (100, 100) rot=90. Pin 1 lib (0, +3.81), Y-flip → (0, -3.81), + eeschema rot=90 TRANSFORM(0,1,-1,0): (0*0+1*-3.81, -1*0+0*-3.81) = (-3.81, 0). + World (96.19, 100). Verified vs kicad-cli netlist.""" sch = _make_sch_data([_make_symbol("R1", 100, 100, rotation=90)]) positions = WireDragger.compute_pin_positions(sch, "R1", 100, 100) old1, _ = positions["1"] old2, _ = positions["2"] - assert abs(old1[0] - 103.81) < 1e-3 + assert abs(old1[0] - 96.19) < 1e-3 assert abs(old1[1] - 100) < 1e-3 def test_returns_empty_for_missing_component(self) -> None: diff --git a/tests/test_pin_locator_and_component.py b/tests/test_pin_locator_and_component.py index 462b8f3..02e8eb7 100644 --- a/tests/test_pin_locator_and_component.py +++ b/tests/test_pin_locator_and_component.py @@ -1,17 +1,21 @@ """ -Regression tests for three bugs fixed in PR #103: +Regression tests for bugs originally fixed in PR #103 and updated for PR #145. 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 + 2. pin_locator.py: reference comparison must tolerate trailing "_" from kicad-skip + (this also covers WireDragger.find_symbol, used by _get_symbol_transform) + +The pre-PR-145 y-axis-negation tests were removed: their assertions encoded the +correct post-PR-145 convention, but their MagicMock setup bypassed +_get_symbol_transform (which reads the .kicad_sch file directly via sexpdata). +The y-flip behaviour is now covered end-to-end against eeschema in +tests/test_pin_locator_y_flip.py — duplicating it with mocks added no value. """ import shutil import sys import tempfile -import types from pathlib import Path -from unittest.mock import MagicMock, patch import pytest @@ -26,15 +30,6 @@ sys.path.insert(0, str(PYTHON_DIR)) _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() # =========================================================================== @@ -71,118 +66,78 @@ class TestAddComponentNoTrailingUnderscore: # =========================================================================== -# 2. pin_locator — y-axis sign (lib y-up → schematic y-down) +# 2. pin_locator — .rstrip("_") tolerance in reference lookup # =========================================================================== -@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 +@pytest.mark.integration class TestPinLocatorReferenceRstrip: - """kicad-skip may write 'R1_' — lookups must still find 'R1'.""" + """ + kicad-skip may write 'R1_' on disk after a clone; lookups for 'R1' must + still resolve. This must hold for *both* lookup paths inside + get_pin_location: the kicad-skip Schematic scan AND the sexpdata-based + _get_symbol_transform (via WireDragger.find_symbol). + """ - @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 + def _write_sch_with_underscored_ref(self, sch_path: Path) -> None: + """Add R1, then mangle the on-disk reference to 'R1_' to simulate the kicad-skip artifact.""" + from commands.component_schematic import ComponentManager + from commands.schematic import SchematicManager - self.locator = PinLocator() + shutil.copy(_TEMPLATE_SCH, sch_path) + sch = SchematicManager.load_schematic(str(sch_path)) + ComponentManager.add_component( + sch, + {"type": "R", "reference": "R1", "value": "10k", "x": 100.0, "y": 100.0, "rotation": 0}, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + # Rewrite the saved file, replacing the Reference "R1" with "R1_" + text = sch_path.read_text(encoding="utf-8") + text = text.replace('(property "Reference" "R1"', '(property "Reference" "R1_"', 1) + sch_path.write_text(text, encoding="utf-8") 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") + from commands.pin_locator import PinLocator + + with tempfile.TemporaryDirectory() as tmp: + sch_path = Path(tmp) / "sch.kicad_sch" + self._write_sch_with_underscored_ref(sch_path) + + locator = PinLocator() + # Caller uses clean reference 'R1'; should still resolve through both + # the kicad-skip path and the sexpdata _get_symbol_transform path. + result = locator.get_pin_location(sch_path, "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") + from commands.component_schematic import ComponentManager + from commands.pin_locator import PinLocator + from commands.schematic import SchematicManager + + with tempfile.TemporaryDirectory() as tmp: + sch_path = Path(tmp) / "sch.kicad_sch" + shutil.copy(_TEMPLATE_SCH, sch_path) + sch = SchematicManager.load_schematic(str(sch_path)) + ComponentManager.add_component( + sch, + { + "type": "R", + "reference": "R2", + "value": "1k", + "x": 50.0, + "y": 50.0, + "rotation": 0, + }, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + locator = PinLocator() + result = locator.get_pin_location(sch_path, "R1", "1") + assert result is None diff --git a/tests/test_pin_locator_y_flip.py b/tests/test_pin_locator_y_flip.py index ec83f0d..e734171 100644 --- a/tests/test_pin_locator_y_flip.py +++ b/tests/test_pin_locator_y_flip.py @@ -105,8 +105,9 @@ def test_rotated_capacitor_pin_x_matches_render_convention(): p1 = locator.get_pin_location(sch_path, "C1", "1") assert p1 is not None - # Device:C pin 1 is at symbol (0, +3.81). After y-negate → (0, -3.81). - # Rotated 90° CCW in screen coords: (0, -3.81) → (3.81, 0). - # Absolute: (150+3.81, 100+0) = (153.81, 100). - assert p1[0] == pytest.approx(153.81), f"rotated pin 1 X wrong: {p1[0]}" + # Device:C pin 1 lib (0, +3.81). parseXY(invertY=true) → internal (0, -3.81). + # Rotation 90 in eeschema is CCW in screen Y-down: TRANSFORM(0,1,-1,0). + # Apply: (0*0 + 1*(-3.81), -1*0 + 0*(-3.81)) = (-3.81, 0). + # World: (150-3.81, 100) = (146.19, 100). Verified vs kicad-cli netlist. + assert p1[0] == pytest.approx(146.19), f"rotated pin 1 X wrong: {p1[0]}" assert p1[1] == pytest.approx(100.0), f"rotated pin 1 Y wrong: {p1[1]}" diff --git a/tests/test_pin_world_xy_eeschema_truth.py b/tests/test_pin_world_xy_eeschema_truth.py new file mode 100644 index 0000000..06d6848 --- /dev/null +++ b/tests/test_pin_world_xy_eeschema_truth.py @@ -0,0 +1,230 @@ +""" +Ground-truth regression for WireDragger.pin_world_xy + label snap. + +The oracle is eeschema itself, accessed via `kicad-cli sch export netlist`. +For each (rotation, mirror) corner case, we: + 1. Build a schematic with a polarized component (Device:D, K=pin1, A=pin2). + 2. Apply the transform via the MCP rotate handler. + 3. Snap a label "_K" to pin 1 and "_A" to pin 2 via PinLocator coords. + 4. Run kicad-cli to extract the netlist. + 5. Assert each label's net binds to the *named* pin in the netlist — + i.e. label "_K" must end up on pin 1 (K), not pin 2 (A). + +If our pin coords agree with eeschema's render, the labels land on the +intended pins. If they disagree, the netlist swaps them, exposing the bug. + +Skips if kicad-cli or the system Device library aren't available. +""" + +import os +import shutil +import subprocess +import sys +import tempfile +import xml.etree.ElementTree as ET +from pathlib import Path + +import pytest + +PYTHON_DIR = Path(__file__).parent.parent / "python" +sys.path.insert(0, str(PYTHON_DIR)) + +from commands.component_schematic import ComponentManager # noqa: E402 +from commands.pin_locator import PinLocator # noqa: E402 +from commands.schematic import SchematicManager # noqa: E402 +from commands.wire_dragger import WireDragger # noqa: E402 + +_KICAD_CLI = shutil.which("kicad-cli") +_DEVICE_LIB = Path("/usr/share/kicad/symbols/Device.kicad_sym") + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif(_KICAD_CLI is None, reason="kicad-cli not on PATH"), + pytest.mark.skipif(not _DEVICE_LIB.exists(), reason="Device lib not installed"), +] + + +def _add_labels_to_file(sch_path: Path, labels: list[tuple[str, float, float]]) -> None: + """Inject (label ...) tokens before the closing ')' of a .kicad_sch file.""" + text = sch_path.read_text() + block = "\n" + for name, x, y in labels: + block += ( + f' (label "{name}"\n' + f" (at {x} {y} 0)\n" + f" (effects (font (size 1.27 1.27)) (justify left bottom))\n" + f' (uuid "00000000-0000-0000-0000-{abs(hash(name)) % 10**12:012d}")\n' + f" )\n" + ) + last = text.rstrip().rfind(")") + sch_path.write_text(text[:last] + block + text[last:]) + + +def _extract_pin_to_net(netlist_xml: Path) -> dict: + """Return {(ref, pin_num): net_name} from a kicad XML netlist.""" + tree = ET.parse(netlist_xml) + root = tree.getroot() + out = {} + for net in root.findall(".//net"): + net_name = net.attrib.get("name", "").lstrip("/") + for node in net.findall("node"): + ref = node.attrib.get("ref") + pin = node.attrib.get("pin") + if ref and pin: + out[(ref, pin)] = net_name + return out + + +def _build_diode_case(tmp: Path, rotation: int) -> tuple[Path, dict]: + """Place a Device:D, rotate, snap labels, save. Returns (sch_path, expected_map).""" + sch_path = tmp / f"diode_rot{rotation}.kicad_sch" + template = PYTHON_DIR / "templates" / "template_with_symbols.kicad_sch" + shutil.copy(template, sch_path) + + sch = SchematicManager.load_schematic(str(sch_path)) + ComponentManager.add_component( + sch, + { + "type": "D", + "reference": "D1", + "value": "1N4148", + "x": 100.0, + "y": 100.0, + "rotation": rotation, + }, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + locator = PinLocator() + p_k = locator.get_pin_location(sch_path, "D1", "1") + p_a = locator.get_pin_location(sch_path, "D1", "2") + assert p_k is not None and p_a is not None + + _add_labels_to_file(sch_path, [("D1_K", p_k[0], p_k[1]), ("D1_A", p_a[0], p_a[1])]) + + return sch_path, {("D1", "1"): "D1_K", ("D1", "2"): "D1_A"} + + +def _apply_mirror_to_file(sch_path: Path, reference: str, axis: str) -> None: + """Apply (mirror x|y) to a placed symbol via direct sexpr mutation. + + ComponentManager.add_component silently drops a 'mirror' kwarg, so this + fixture goes around it via the same low-level helper rotate_schematic_component + uses (WireDragger.update_symbol_rotation_mirror).""" + import sexpdata + + sch_data = sexpdata.loads(sch_path.read_text()) + if not WireDragger.update_symbol_rotation_mirror(sch_data, reference, 0, axis): + raise RuntimeError(f"Failed to apply mirror={axis} to {reference}") + sch_path.write_text(sexpdata.dumps(sch_data)) + + +def _build_mirror_case(tmp: Path, axis: str) -> tuple[Path, dict]: + sch_path = tmp / f"resistor_mirror_{axis}.kicad_sch" + template = PYTHON_DIR / "templates" / "template_with_symbols.kicad_sch" + shutil.copy(template, sch_path) + + sch = SchematicManager.load_schematic(str(sch_path)) + ComponentManager.add_component( + sch, + {"type": "R", "reference": "R1", "value": "10k", "x": 100.0, "y": 100.0, "rotation": 0}, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + _apply_mirror_to_file(sch_path, "R1", axis) + if f"(mirror {axis})" not in sch_path.read_text(): + raise RuntimeError( + f"Fixture failed to write (mirror {axis}) — the kicad-cli oracle would " + f"silently match our pin coords for an unmirrored symbol." + ) + + locator = PinLocator() + p1 = locator.get_pin_location(sch_path, "R1", "1") + p2 = locator.get_pin_location(sch_path, "R1", "2") + if p1 is None or p2 is None: + raise RuntimeError(f"PinLocator returned None for R1 mirror={axis}") + + _add_labels_to_file(sch_path, [("R1_PIN1", p1[0], p1[1]), ("R1_PIN2", p2[0], p2[1])]) + + return sch_path, {("R1", "1"): "R1_PIN1", ("R1", "2"): "R1_PIN2"} + + +def _run_netlist(sch_path: Path) -> dict: + out = sch_path.with_suffix(".net") + env = {**os.environ, "KICAD_SYMBOL_DIR": "/usr/share/kicad/symbols"} + subprocess.run( + [ + _KICAD_CLI, + "sch", + "export", + "netlist", + "--format", + "kicadxml", + "-o", + str(out), + str(sch_path), + ], + check=True, + capture_output=True, + env=env, + ) + return _extract_pin_to_net(out) + + +@pytest.mark.parametrize("rotation", [0, 90, 180, 270]) +def test_diode_label_polarity_through_eeschema(rotation): + """Snap-labelled K must show up on pin 1 in the kicad-cli netlist.""" + with tempfile.TemporaryDirectory() as td: + sch_path, expected = _build_diode_case(Path(td), rotation) + actual = _run_netlist(sch_path) + for (ref, pin), net in expected.items(): + assert actual.get((ref, pin)) == net, ( + f"rotation={rotation}: D1.{pin} label landed on wrong pin. " + f"Expected net={net}, got {actual.get((ref, pin))}. " + f"Full mapping: {actual}" + ) + + +@pytest.mark.parametrize("axis", ["x", "y"]) +def test_mirrored_resistor_label_through_eeschema(axis): + """Snap-labelled pin 1 must show up on pin 1 after (mirror x) / (mirror y).""" + with tempfile.TemporaryDirectory() as td: + sch_path, expected = _build_mirror_case(Path(td), axis) + actual = _run_netlist(sch_path) + for (ref, pin), net in expected.items(): + assert actual.get((ref, pin)) == net, ( + f"mirror={axis}: R1.{pin} label landed on wrong pin. " + f"Expected net={net}, got {actual.get((ref, pin))}. " + f"Full mapping: {actual}" + ) + + +def test_pin_world_xy_rot90_matches_eeschema_transform(): + """Pure-math regression: pin_world_xy for Device:R rot=90 must match + eeschema's TRANSFORM(0,1,-1,0) applied to internal Y-flipped pin.""" + # Device:R pin 1: lib (0, +3.81). parseXY(invertY=true) → internal (0, -3.81). + # TRANSFORM(0,1,-1,0) applied: (0*0 + 1*-3.81, -1*0 + 0*-3.81) = (-3.81, 0). + # Symbol at (100, 100) → world (96.19, 100). + wx, wy = WireDragger.pin_world_xy(0.0, 3.81, 100.0, 100.0, 90, False, False) + assert wx == pytest.approx(96.19), f"rot=90 X wrong: {wx} (expected 96.19)" + assert wy == pytest.approx(100.0), f"rot=90 Y wrong: {wy} (expected 100.0)" + + +def test_pin_world_xy_mirror_x_matches_eeschema(): + """(mirror x) = SYM_MIRROR_X = TRANSFORM(1,0,0,-1) → negates internal Y. + Device:R pin 1 lib (0, +3.81) → internal (0, -3.81) → mirror_x → (0, 3.81) + → symbol (100, 100) → world (100, 103.81). Pin should NOT be at (100, 96.19).""" + wx, wy = WireDragger.pin_world_xy(0.0, 3.81, 100.0, 100.0, 0, True, False) + assert wx == pytest.approx(100.0) + assert wy == pytest.approx(103.81), f"mirror_x Y wrong: {wy} (expected 103.81)" + + +def test_pin_world_xy_mirror_y_matches_eeschema(): + """(mirror y) = SYM_MIRROR_Y = TRANSFORM(-1,0,0,1) → negates internal X. + Device:R pin 1 lib (0, +3.81) → internal (0, -3.81) → mirror_y → (0, -3.81) + → world (100, 96.19). Y of pin 1 is unchanged by mirror across Y axis.""" + wx, wy = WireDragger.pin_world_xy(0.0, 3.81, 100.0, 100.0, 0, False, True) + assert wx == pytest.approx(100.0) + assert wy == pytest.approx(96.19), f"mirror_y Y wrong: {wy} (expected 96.19)" diff --git a/tests/test_rotate_schematic_mirror.py b/tests/test_rotate_schematic_mirror.py index dba2c03..eb67545 100644 --- a/tests/test_rotate_schematic_mirror.py +++ b/tests/test_rotate_schematic_mirror.py @@ -6,13 +6,13 @@ Tests are split into two layers: 2. Handler integration smoke test — patches SchematicManager away. """ +import importlib.util +import math import os import sys -import math -import textwrap import tempfile -import importlib.util -from unittest.mock import patch, MagicMock +import textwrap +from unittest.mock import MagicMock, patch import sexpdata from sexpdata import Symbol @@ -40,6 +40,7 @@ WireDragger = _wd_mod.WireDragger # Helpers # --------------------------------------------------------------------------- + def _parse(text: str) -> list: return sexpdata.loads(text) @@ -80,6 +81,7 @@ def _make_sch(sym_extra: str = "", wires: str = "") -> list: # 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) @@ -130,6 +132,7 @@ def test_update_unknown_reference_returns_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() @@ -160,25 +163,28 @@ def test_pin_positions_unchanged_at_same_transform(): assert old_xy == new_xy -def test_pin_positions_mirror_x_flips_x(): - """mirror_x should negate the local X coordinate before rotation.""" +def test_pin_positions_mirror_x_flips_y(): + """mirror_x = SYM_MIRROR_X = TRANSFORM(1,0,0,-1) negates the screen-Y + coordinate (eeschema symbol.h:43-44), not X. With the lib→screen Y-flip + applied first, this means the pin's screen Y is reflected back to lib Y.""" sch = _make_sch() # at (75, 105, 0), no mirror - fake_pins = {"1": {"x": 2.0, "y": 0.0}} + fake_pins = {"1": {"x": 0.0, "y": 2.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 + # old: pin at lib (0, 2). Y-flip → (0, -2). No mirror. World = (75, 105-2) = (75, 103). + assert abs(old_xy[1] - 103.0) < 1e-4 + # new: mirror_x → negate screen-Y → (0, 2). World = (75, 105+2) = (75, 107). + assert abs(new_xy[1] - 107.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.""" # Ensure python/ is on sys.path so commands.* imports resolve @@ -186,26 +192,44 @@ def test_rotate_handler_no_crash(tmp_path): 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", - "annotations"): - 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"), + # Stub heavy imports before loading kicad_interface. Save and restore + # sys.modules state so we don't pollute already-imported real modules + # (notably schemas.tool_schemas, whose TOOL_SCHEMAS dict is shared across + # the test session). + _stub_modnames = ( + "pcbnew", + "skip", + "resources", + "schemas", + "resources.resource_definitions", + "schemas.tool_schemas", + "annotations", ) - ki_mod = importlib.util.module_from_spec(ki_spec) - ki_spec.loader.exec_module(ki_mod) - KiCADInterface = ki_mod.KiCADInterface + _saved_modules = {n: sys.modules.get(n) for n in _stub_modnames} + try: + for modname in _stub_modnames: + sys.modules[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 + finally: + for modname, mod in _saved_modules.items(): + if mod is None: + sys.modules.pop(modname, None) + else: + sys.modules[modname] = mod # Write a minimal schematic file sch_path = str(tmp_path / "test.kicad_sch") @@ -233,11 +257,13 @@ def test_rotate_handler_no_crash(tmp_path): f.write(sch_content) iface = KiCADInterface.__new__(KiCADInterface) - result = iface._handle_rotate_schematic_component({ - "schematicPath": sch_path, - "reference": "R1", - "angle": 90, - }) + result = iface._handle_rotate_schematic_component( + { + "schematicPath": sch_path, + "reference": "R1", + "angle": 90, + } + ) assert result["success"] is True assert result["angle"] == 90