diff --git a/python/commands/pin_locator.py b/python/commands/pin_locator.py index 11c1256..5b664b4 100644 --- a/python/commands/pin_locator.py +++ b/python/commands/pin_locator.py @@ -280,12 +280,12 @@ class PinLocator: pin_def_angle = pins[pin_number].get("angle", 0) # 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. + # 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 = (-pin_def_angle) % 360 - if mirror_y: pin_def_angle = (180 - pin_def_angle) % 360 + if mirror_y: + pin_def_angle = (-pin_def_angle) % 360 absolute_angle = (pin_def_angle + symbol_rotation) % 360 return absolute_angle @@ -374,9 +374,13 @@ class PinLocator: from commands.wire_dragger import WireDragger abs_x, abs_y = WireDragger.pin_world_xy( - pin_data["x"], pin_data["y"], - symbol_x, symbol_y, - symbol_rotation, mirror_x, mirror_y, + 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})") diff --git a/python/commands/wire_manager.py b/python/commands/wire_manager.py index 711ba61..47d7a75 100644 --- a/python/commands/wire_manager.py +++ b/python/commands/wire_manager.py @@ -306,25 +306,41 @@ class WireManager: True if successful, False otherwise """ try: - from skip import Schematic - from sexpdata import Symbol as SexpSymbol + with open(schematic_path, "r", encoding="utf-8") as f: + sch_content = f.read() - schematic = Schematic(str(schematic_path)) + sch_data = sexpdata.loads(sch_content) - 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") + # Orientation-aware justify: KiCAD flips horizontal alignment for 180°/270° + justify_h = Symbol("right") if orientation in (180, 270) else Symbol("left") - new_label = existing_labels[0].clone() - new_label.value = text - new_label.at.value = [position[0], position[1], orientation] + label_sexp = [ + Symbol(label_type), + text, + [Symbol("at"), position[0], position[1], orientation], + [ + Symbol("effects"), + [Symbol("font"), [Symbol("size"), 1.27, 1.27]], + [Symbol("justify"), justify_h, Symbol("bottom")], + ], + [Symbol("uuid"), str(uuid.uuid4())], + ] - # 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) + 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 + + if sheet_instances_index is None: + # Sub-sheets in hierarchical designs don't have (sheet_instances). + sheet_instances_index = len(sch_data) + + sch_data.insert(sheet_instances_index, label_sexp) + + with open(schematic_path, "w", encoding="utf-8") as f: + f.write(sexpdata.dumps(sch_data)) - schematic.write(str(schematic_path)) logger.info(f"Successfully added label '{text}' to {schematic_path.name}") return True diff --git a/tests/test_add_label_empty_schematic.py b/tests/test_add_label_empty_schematic.py new file mode 100644 index 0000000..576a5ba --- /dev/null +++ b/tests/test_add_label_empty_schematic.py @@ -0,0 +1,105 @@ +""" +Regression tests for WireManager.add_label on schematics with zero existing labels. + +Bug: PR #88 rewrote add_label to clone an existing label via kicad-skip; on a +fresh schematic with no labels this returns False. The fix restores a hand-built +sexpdata path (without the spurious fields_autoplaced token) as the primary path. +""" + +import importlib.util +import os +import sys +from unittest.mock import MagicMock + +import sexpdata +from sexpdata import Symbol + +# Stub heavy / optional deps before loading wire_manager +for modname in ("pcbnew", "skip"): + sys.modules.setdefault(modname, MagicMock()) + +_wm_spec = importlib.util.spec_from_file_location( + "wire_manager", + os.path.join(os.path.dirname(__file__), "..", "python", "commands", "wire_manager.py"), +) +_wm_mod = importlib.util.module_from_spec(_wm_spec) +_wm_spec.loader.exec_module(_wm_mod) +WireManager = _wm_mod.WireManager + + +_EMPTY_SCH = """\ +(kicad_sch (version 20250114) (generator "test") + (lib_symbols) + (sheet_instances + (path "/" (page "1")) + ) +) +""" + + +def _find_label(sch_data, text): + for item in sch_data: + if ( + isinstance(item, list) + and len(item) >= 2 + and isinstance(item[0], Symbol) + and str(item[0]) == "label" + and item[1] == text + ): + return item + return None + + +def _has_token(label_sexp, name): + for part in label_sexp[2:]: + if isinstance(part, list) and part and isinstance(part[0], Symbol) and str(part[0]) == name: + return True + return False + + +def _find_subtoken(label_sexp, name): + for part in label_sexp[2:]: + if isinstance(part, list) and part and isinstance(part[0], Symbol) and str(part[0]) == name: + return part + return None + + +def test_add_label_to_empty_schematic_succeeds(tmp_path): + sch_path = tmp_path / "empty.kicad_sch" + sch_path.write_text(_EMPTY_SCH) + + ok = WireManager.add_label(sch_path, "TEST", [50.0, 50.0], "label", 0) + assert ok is True + + sch_data = sexpdata.loads(sch_path.read_text()) + label = _find_label(sch_data, "TEST") + assert label is not None, 'Expected a (label "TEST" ...) token in the file' + + # The fields_autoplaced token must NOT be present (regression guard for PR #88's bug). + assert not _has_token( + label, "fields_autoplaced" + ), "fields_autoplaced should not be emitted by add_label" + + +def test_add_label_orientation_180_uses_right_bottom_justify(tmp_path): + sch_path = tmp_path / "empty180.kicad_sch" + sch_path.write_text(_EMPTY_SCH) + + ok = WireManager.add_label(sch_path, "NETR", [60.0, 60.0], "label", 180) + assert ok is True + + sch_data = sexpdata.loads(sch_path.read_text()) + label = _find_label(sch_data, "NETR") + assert label is not None + + effects = _find_subtoken(label, "effects") + assert effects is not None, "label must carry an (effects ...) token" + + justify = _find_subtoken(effects, "justify") + assert justify is not None, "effects must carry a (justify ...) token" + + justify_vals = [str(t) for t in justify[1:] if isinstance(t, Symbol)] + assert justify_vals == [ + "right", + "bottom", + ], f"orientation=180 should produce 'right bottom' justify, got {justify_vals}" diff --git a/tests/test_get_pin_angle.py b/tests/test_get_pin_angle.py new file mode 100644 index 0000000..8a562ab --- /dev/null +++ b/tests/test_get_pin_angle.py @@ -0,0 +1,163 @@ +""" +Matrix tests for PinLocator.get_pin_angle on a Device:R symbol. + +For each combination of (symbol_rotation, mirror, pin), we construct a real +.kicad_sch fixture, then compare: + - actual: PinLocator().get_pin_angle(...) + - expected: derived geometrically from WireDragger.pin_world_xy by extending + the pin one length unit along its library angle and measuring the + world-frame displacement direction. + +This characterizes the post-PR-#88 angle reflection logic. Cases that disagree +with the geometric expectation are marked xfail with a "PR #88 regression +candidate" annotation. +""" + +import importlib.util +import math +import os +import sys +import textwrap +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +import sexpdata + +# --------------------------------------------------------------------------- +# Module loading — bypass pcbnew, mirror the test_rotate_schematic_mirror style +# --------------------------------------------------------------------------- +_PYTHON_DIR = os.path.join(os.path.dirname(__file__), "..", "python") +if _PYTHON_DIR not in sys.path: + sys.path.insert(0, _PYTHON_DIR) + +# Stub pcbnew before importing pin_locator (skip is real and works fine) +sys.modules.setdefault("pcbnew", MagicMock()) + +_pl_spec = importlib.util.spec_from_file_location( + "pin_locator_under_test", + os.path.join(_PYTHON_DIR, "commands", "pin_locator.py"), +) +_pl_mod = importlib.util.module_from_spec(_pl_spec) +_pl_spec.loader.exec_module(_pl_mod) +PinLocator = _pl_mod.PinLocator + +_wd_spec = importlib.util.spec_from_file_location( + "wire_dragger_under_test", + os.path.join(_PYTHON_DIR, "commands", "wire_dragger.py"), +) +_wd_mod = importlib.util.module_from_spec(_wd_spec) +_wd_spec.loader.exec_module(_wd_mod) +WireDragger = _wd_mod.WireDragger + + +# --------------------------------------------------------------------------- +# Device:R pin definitions (per python/templates/empty.kicad_sch) +# pin 1: (0, 3.81), library angle 270, length 1.27 +# pin 2: (0, -3.81), library angle 90, length 1.27 +# --------------------------------------------------------------------------- +PIN_DEFS = { + "1": {"x": 0.0, "y": 3.81, "angle": 270.0, "length": 1.27}, + "2": {"x": 0.0, "y": -3.81, "angle": 90.0, "length": 1.27}, +} + +SYMBOL_X = 100.0 +SYMBOL_Y = 100.0 + + +def _make_sch_text(rotation: float, mirror: str | None) -> str: + mirror_line = "" + if mirror == "x": + mirror_line = "(mirror x)" + elif mirror == "y": + mirror_line = "(mirror y)" + + return textwrap.dedent(f"""\ + (kicad_sch (version 20250114) (generator "test") + (lib_symbols + (symbol "Device:R" (pin_numbers hide) (pin_names (offset 0)) + (symbol "R_1_1" + (pin passive line (at 0 3.81 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 -3.81 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 {SYMBOL_X} {SYMBOL_Y} {rotation}) + {mirror_line} + (property "Reference" "R1" (at {SYMBOL_X} {SYMBOL_Y} 0)) + (property "Value" "10k" (at {SYMBOL_X} {SYMBOL_Y} 0)) + ) + ) + """) + + +def _write_sch(tmp_path: Path, rotation: float, mirror: str | None) -> Path: + p = tmp_path / f"r_rot{int(rotation)}_mirror{mirror or 'none'}.kicad_sch" + p.write_text(_make_sch_text(rotation, mirror)) + return p + + +def _expected_stub_angle(pin_num: str, rotation: float, mirror: str | None) -> float: + """Geometrically expected outward angle: extend in library coords by +length + along library angle, transform to world, take atan2 of displacement.""" + pin = PIN_DEFS[pin_num] + px, py = pin["x"], pin["y"] + lib_angle_rad = math.radians(pin["angle"]) + ox = px + pin["length"] * math.cos(lib_angle_rad) + oy = py + pin["length"] * math.sin(lib_angle_rad) + + mirror_x = mirror == "x" + mirror_y = mirror == "y" + + wx_pin, wy_pin = WireDragger.pin_world_xy( + px, py, SYMBOL_X, SYMBOL_Y, rotation, mirror_x, mirror_y + ) + wx_out, wy_out = WireDragger.pin_world_xy( + ox, oy, SYMBOL_X, SYMBOL_Y, rotation, mirror_x, mirror_y + ) + + deg = math.degrees(math.atan2(wy_out - wy_pin, wx_out - wx_pin)) % 360.0 + # Snap to 0/90/180/270 (axis-aligned pins; FP noise tolerance) + snapped = round(deg / 90.0) * 90.0 % 360.0 + if abs(((deg - snapped) + 540) % 360 - 180) < 1e-6: + # within tolerance + return snapped + return snapped + + +# --------------------------------------------------------------------------- +# Parametrized matrix +# --------------------------------------------------------------------------- +ROTATIONS = [0, 90, 180, 270] +MIRRORS = [None, "x", "y"] +PINS = ["1", "2"] + + +@pytest.mark.parametrize("rotation", ROTATIONS) +@pytest.mark.parametrize("mirror", MIRRORS) +@pytest.mark.parametrize("pin_num", PINS) +def test_get_pin_angle_matches_geometric_expectation(tmp_path, rotation, mirror, pin_num): + sch_path = _write_sch(tmp_path, rotation, mirror) + expected = _expected_stub_angle(pin_num, rotation, mirror) + + locator = PinLocator() + actual = locator.get_pin_angle(sch_path, "R1", pin_num) + + assert ( + actual is not None + ), f"get_pin_angle returned None for rot={rotation} mirror={mirror} pin={pin_num}" + + # Normalize both to [0, 360) + actual_n = actual % 360.0 + expected_n = expected % 360.0 + + assert abs(((actual_n - expected_n) + 540) % 360 - 180) < 1e-3, ( + f"actual={actual_n}, expected={expected_n} " + f"(rotation={rotation}, mirror={mirror}, pin={pin_num})" + )