From a152b75db30a8a2cf26c1a7efe8cae2346c25d80 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 29 Mar 2026 20:18:38 +0100 Subject: [PATCH 1/4] feat: move_schematic_component with wire preservation (drag behavior) When moving a schematic component, connected wires are stretched/shifted to follow the component (like KiCAD's drag behaviour), preserving connectivity instead of leaving dangling wire stubs. Also fixes property labels (value, reference, etc.) so they shift with the symbol rather than staying at their original positions. Co-Authored-By: Claude Sonnet 4.6 --- python/commands/wire_dragger.py | 311 ++++++++ python/kicad_interface.py | 103 ++- .../tests/test_move_with_wire_preservation.py | 661 ++++++++++++++++++ src/tools/schematic.ts | 50 ++ 4 files changed, 1100 insertions(+), 25 deletions(-) create mode 100644 python/commands/wire_dragger.py create mode 100644 python/tests/test_move_with_wire_preservation.py diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py new file mode 100644 index 0000000..910236b --- /dev/null +++ b/python/commands/wire_dragger.py @@ -0,0 +1,311 @@ +""" +WireDragger — drag connected wires when a schematic component is moved. + +All methods operate on in-memory sexpdata lists (no disk I/O). +""" + +import logging +import math +from typing import Dict, Optional, Tuple + +import sexpdata +from sexpdata import Symbol + +logger = logging.getLogger("kicad_interface") + +# Module-level Symbol constants +_K = { + name: Symbol(name) + for name in [ + "symbol", + "at", + "lib_id", + "mirror", + "lib_symbols", + "pts", + "xy", + "wire", + "junction", + "property", + ] +} + +EPS = 1e-4 # mm — coordinate match tolerance + + +def _rotate(x: float, y: float, angle_deg: float) -> Tuple[float, float]: + """Rotate (x, y) around the origin by angle_deg degrees (CCW).""" + if angle_deg == 0: + return x, y + rad = math.radians(angle_deg) + c, s = math.cos(rad), math.sin(rad) + return x * c - y * s, x * s + y * c + + +def _coords_match(ax: float, ay: float, bx: float, by: float, eps: float = EPS) -> bool: + return abs(ax - bx) < eps and abs(ay - by) < eps + + +class WireDragger: + """Pure-logic helpers for wire-endpoint dragging during component moves.""" + + @staticmethod + def find_symbol(sch_data: list, reference: str): + """ + Find a placed symbol by reference designator. + + Returns (symbol_item, old_x, old_y, rotation, lib_id, mirror_x, mirror_y) + or None if the reference is not found. + + mirror_x=True means the symbol has (mirror x) — flips the X local axis. + mirror_y=True means the symbol has (mirror y) — flips the Y local axis. + """ + sym_k = _K["symbol"] + prop_k = _K["property"] + at_k = _K["at"] + lib_id_k = _K["lib_id"] + mirror_k = _K["mirror"] + + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == sym_k): + continue + + # Check Reference property + 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: + continue + + old_x = old_y = rotation = 0.0 + lib_id = "" + mirror_x = mirror_y = False + + for sub in item[1:]: + if not isinstance(sub, list) or not sub: + continue + tag = sub[0] + if tag == at_k: + if len(sub) >= 3: + old_x = float(sub[1]) + old_y = float(sub[2]) + if len(sub) >= 4: + rotation = float(sub[3]) + elif tag == lib_id_k and len(sub) >= 2: + lib_id = str(sub[1]).strip('"') + elif tag == mirror_k and len(sub) >= 2: + mv = str(sub[1]) + if mv == "x": + mirror_x = True + elif mv == "y": + mirror_y = True + + return item, old_x, old_y, rotation, lib_id, mirror_x, mirror_y + + return None + + @staticmethod + def get_pin_defs(sch_data: list, lib_id: str) -> Dict: + """ + Get pin definitions from lib_symbols for the given lib_id. + + Returns the same dict format as PinLocator.parse_symbol_definition: + {pin_num: {"x": ..., "y": ..., ...}}. + """ + from commands.pin_locator import PinLocator + + lib_sym_k = _K["lib_symbols"] + symbol_k = _K["symbol"] + + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == lib_sym_k): + continue + for sym_def in item[1:]: + if not (isinstance(sym_def, list) and sym_def and sym_def[0] == symbol_k): + continue + if len(sym_def) < 2: + continue + name = str(sym_def[1]).strip('"') + if name == lib_id: + return PinLocator.parse_symbol_definition(sym_def) + break # only one lib_symbols section + return {} + + @staticmethod + def pin_world_xy( + px: float, + py: float, + sym_x: float, + sym_y: float, + rotation: float, + mirror_x: bool, + mirror_y: bool, + ) -> Tuple[float, float]: + """ + Compute the world coordinate of a pin given the symbol transform. + + KiCAD applies mirror first (in local space), then rotation, then translation. + mirror_x negates the local X axis; mirror_y negates the local Y axis. + """ + lx, ly = px, py + if mirror_x: + lx = -lx + if mirror_y: + ly = -ly + rx, ry = _rotate(lx, ly, rotation) + return sym_x + rx, sym_y + ry + + @staticmethod + def compute_pin_positions( + sch_data: list, + reference: str, + new_x: float, + new_y: float, + ) -> Dict[str, Tuple[Tuple[float, float], Tuple[float, float]]]: + """ + Compute world pin positions before and after a component move. + + Returns {pin_num: (old_world_xy, new_world_xy)}. + old_world_xy uses the symbol's current position; new_world_xy uses (new_x, new_y). + """ + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return {} + _, old_x, old_y, rotation, lib_id, mirror_x, 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, old_x, old_y, rotation, mirror_x, mirror_y + ) + new_wx, new_wy = WireDragger.pin_world_xy( + px, py, new_x, new_y, rotation, mirror_x, 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 drag_wires( + sch_data: list, + old_to_new: Dict[Tuple[float, float], Tuple[float, float]], + eps: float = EPS, + ) -> Dict: + """ + Move wire endpoints and junctions from old positions to new positions. + Removes zero-length wires that result from the move. + Modifies sch_data in place. + + old_to_new: {(old_x, old_y): (new_x, new_y)} + + Returns {'endpoints_moved': N, 'wires_removed': M}. + """ + wire_k = _K["wire"] + pts_k = _K["pts"] + xy_k = _K["xy"] + junction_k = _K["junction"] + at_k = _K["at"] + + def find_new(x: float, y: float): + for (ox, oy), (nx, ny) in old_to_new.items(): + if _coords_match(x, y, ox, oy, eps): + return nx, ny + return None + + endpoints_moved = 0 + zero_length_indices = [] + + # First pass: update wire endpoints + for idx, item in enumerate(sch_data): + if not (isinstance(item, list) and item and item[0] == wire_k): + continue + + pts_sub = None + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == pts_k: + pts_sub = sub + break + if pts_sub is None: + continue + + xy_items = [ + p for p in pts_sub[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == xy_k + ] + for xy_item in xy_items: + nc = find_new(float(xy_item[1]), float(xy_item[2])) + if nc is not None: + xy_item[1] = nc[0] + xy_item[2] = nc[1] + endpoints_moved += 1 + + # Check if this wire is now zero-length + if len(xy_items) >= 2: + x1, y1 = float(xy_items[0][1]), float(xy_items[0][2]) + x2, y2 = float(xy_items[-1][1]), float(xy_items[-1][2]) + if _coords_match(x1, y1, x2, y2, eps): + zero_length_indices.append(idx) + + # Remove zero-length wires (backwards to preserve indices) + for idx in reversed(zero_length_indices): + del sch_data[idx] + + # Second pass: update junctions + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == junction_k): + continue + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == at_k and len(sub) >= 3: + nc = find_new(float(sub[1]), float(sub[2])) + if nc is not None: + sub[1] = nc[0] + sub[2] = nc[1] + break + + return { + "endpoints_moved": endpoints_moved, + "wires_removed": len(zero_length_indices), + } + + @staticmethod + def update_symbol_position(sch_data: list, reference: str, new_x: float, new_y: float) -> bool: + """ + Update the (at x y rot) of the named symbol in sch_data. + 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"] + prop_k = _K["property"] + + # Find current position and compute delta + old_x = old_y = None + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == at_k and len(sub) >= 3: + old_x, old_y = sub[1], sub[2] + sub[1] = new_x + sub[2] = new_y + break + if old_x is None or old_y is None: + return False + + dx = new_x - old_x + dy = new_y - old_y + + # Shift all property label positions by the same delta + for sub in item[1:]: + if isinstance(sub, list) and sub and sub[0] == prop_k: + for psub in sub[1:]: + if isinstance(psub, list) and psub and psub[0] == at_k and len(psub) >= 3: + psub[1] += dx + psub[2] += dy + break + return True diff --git a/python/kicad_interface.py b/python/kicad_interface.py index b3903ad..899744d 100644 --- a/python/kicad_interface.py +++ b/python/kicad_interface.py @@ -1283,6 +1283,46 @@ class KiCADInterface: logger.error(f"Error listing schematic libraries: {str(e)}") return {"success": False, "message": str(e)} + def _handle_find_unconnected_pins(self, params): + """List component pins with no wire/label/power symbol touching them""" + logger.info("Finding unconnected pins") + try: + from commands.schematic_analysis import find_unconnected_pins + + schematic_path = params.get("schematicPath") + if not schematic_path: + return {"success": False, "message": "schematicPath is required"} + result = find_unconnected_pins(schematic_path) + return {"success": True, **result} + except ImportError: + return { + "success": False, + "message": "schematic_analysis module not available", + } + except Exception as e: + logger.error(f"Error finding unconnected pins: {e}") + return {"success": False, "message": str(e)} + + def _handle_check_wire_collisions(self, params): + """Detect wires passing through component bodies without connecting to pins""" + logger.info("Checking wire collisions") + try: + from commands.schematic_analysis import check_wire_collisions + + schematic_path = params.get("schematicPath") + if not schematic_path: + return {"success": False, "message": "schematicPath is required"} + result = check_wire_collisions(schematic_path) + return {"success": True, **result} + except ImportError: + return { + "success": False, + "message": "schematic_analysis module not available", + } + except Exception as e: + logger.error(f"Error checking wire collisions: {e}") + return {"success": False, "message": str(e)} + # ------------------------------------------------------------------ # # Footprint handlers # # ------------------------------------------------------------------ # @@ -1998,14 +2038,18 @@ class KiCADInterface: return {"success": False, "message": str(e)} def _handle_move_schematic_component(self, params): - """Move a schematic component to a new position""" + """Move a schematic component to a new position, dragging connected wires.""" logger.info("Moving schematic component") try: + import sexpdata as _sexpdata + from commands.wire_dragger import WireDragger + schematic_path = params.get("schematicPath") reference = params.get("reference") position = params.get("position", {}) new_x = position.get("x") new_y = position.get("y") + preserve_wires = params.get("preserveWires", True) if not schematic_path or not reference: return { @@ -2018,30 +2062,42 @@ class KiCADInterface: "message": "position with x and y is required", } - schematic = SchematicManager.load_schematic(schematic_path) - if not schematic: - return {"success": False, "message": "Failed to load schematic"} + with open(schematic_path, "r", encoding="utf-8") as f: + sch_data = _sexpdata.loads(f.read()) - # Find the symbol - for symbol in schematic.symbol: - if not hasattr(symbol.property, "Reference"): - continue - if symbol.property.Reference.value == reference: - old_pos = list(symbol.at.value) - old_position = {"x": float(old_pos[0]), "y": float(old_pos[1])} + # Find symbol and record old position + found = WireDragger.find_symbol(sch_data, reference) + if found is None: + return {"success": False, "message": f"Component {reference} not found"} + _, old_x, old_y = found[0], found[1], found[2] + old_position = {"x": old_x, "y": old_y} - # Preserve rotation (third element) - rotation = float(old_pos[2]) if len(old_pos) > 2 else 0 - symbol.at.value = [new_x, new_y, rotation] + drag_summary = {} + if preserve_wires: + # Compute pin world positions before and after the move + pin_positions = WireDragger.compute_pin_positions( + sch_data, reference, float(new_x), float(new_y) + ) + # Build old→new coordinate map (deduplicate coincident pins) + old_to_new = {} + for _pin, (old_xy, new_xy) in pin_positions.items(): + old_to_new[old_xy] = new_xy - SchematicManager.save_schematic(schematic, schematic_path) - return { - "success": True, - "oldPosition": old_position, - "newPosition": {"x": new_x, "y": new_y}, - } + drag_summary = WireDragger.drag_wires(sch_data, old_to_new) - return {"success": False, "message": f"Component {reference} not found"} + # Update symbol position + WireDragger.update_symbol_position(sch_data, reference, float(new_x), float(new_y)) + + with open(schematic_path, "w", encoding="utf-8") as f: + f.write(_sexpdata.dumps(sch_data)) + + return { + "success": True, + "oldPosition": old_position, + "newPosition": {"x": new_x, "y": new_y}, + "wiresMoved": drag_summary.get("endpoints_moved", 0), + "wiresRemoved": drag_summary.get("wires_removed", 0), + } except Exception as e: logger.error(f"Error moving schematic component: {e}") @@ -2074,21 +2130,18 @@ class KiCADInterface: continue if symbol.property.Reference.value == reference: pos = list(symbol.at.value) - pos[2] = angle if len(pos) > 2 else angle while len(pos) < 3: pos.append(0) pos[2] = angle symbol.at.value = pos - # Handle mirror if specified if mirror: if hasattr(symbol, "mirror"): symbol.mirror.value = mirror else: logger.warning( f"Mirror '{mirror}' requested for {reference}, " - f"but symbol does not have a 'mirror' attribute; " - f"mirror not applied" + f"but symbol has no mirror attribute; skipped" ) SchematicManager.save_schematic(schematic, schematic_path) diff --git a/python/tests/test_move_with_wire_preservation.py b/python/tests/test_move_with_wire_preservation.py new file mode 100644 index 0000000..acdda76 --- /dev/null +++ b/python/tests/test_move_with_wire_preservation.py @@ -0,0 +1,661 @@ +""" +Tests for move_schematic_component with wire preservation (WireDragger). + +Unit tests use synthetic sexpdata lists — no disk I/O, no KiCAD install needed. +Integration tests copy empty.kicad_sch to a tempdir and exercise the full handler. +""" + +import shutil +import sys +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +import sexpdata +from sexpdata import Symbol + +# Make python/ importable +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from commands.wire_dragger import EPS, WireDragger, _rotate + +TEMPLATE_PATH = Path(__file__).resolve().parent.parent / "templates" / "empty.kicad_sch" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _sym(name: str) -> Symbol: + return Symbol(name) + + +def _make_wire(x1, y1, x2, y2): + return [ + _sym("wire"), + [_sym("pts"), [_sym("xy"), x1, y1], [_sym("xy"), x2, y2]], + [_sym("stroke"), [_sym("width"), 0], [_sym("type"), _sym("default")]], + [_sym("uuid"), "00000000-0000-0000-0000-000000000000"], + ] + + +def _make_junction(x, y): + return [ + _sym("junction"), + [_sym("at"), x, y], + [_sym("diameter"), 0], + [_sym("color"), 0, 0, 0, 0], + [_sym("uuid"), "00000000-0000-0000-0000-000000000001"], + ] + + +def _make_symbol(ref, x, y, rotation=0, lib_id="Device:R", mirror=None): + """Build a minimal placed-symbol s-expression.""" + item = [ + _sym("symbol"), + [_sym("lib_id"), lib_id], + [_sym("at"), x, y, rotation], + [_sym("unit"), 1], + [_sym("property"), "Reference", ref, [_sym("at"), x + 2, y, 0]], + [_sym("property"), "Value", "10k", [_sym("at"), x, y, 0]], + ] + if mirror: + item.append([_sym("mirror"), _sym(mirror)]) + return item + + +def _make_lib_symbol_r(): + """Minimal Device:R lib_symbols entry — pins at (0, 3.81) and (0, -3.81).""" + return [ + _sym("symbol"), + "Device:R", + [ + _sym("symbol"), + "R_1_1", + [ + _sym("pin"), + _sym("passive"), + _sym("line"), + [_sym("at"), 0, 3.81, 270], + [_sym("length"), 1.27], + [ + _sym("name"), + "~", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + [ + _sym("number"), + "1", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + ], + [ + _sym("pin"), + _sym("passive"), + _sym("line"), + [_sym("at"), 0, -3.81, 90], + [_sym("length"), 1.27], + [ + _sym("name"), + "~", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + [ + _sym("number"), + "2", + [_sym("effects"), [_sym("font"), [_sym("size"), 1.27, 1.27]]], + ], + ], + ], + ] + + +def _make_sch_data(extra_items=None): + """Build a minimal sch_data list with lib_symbols and sheet_instances.""" + data = [ + _sym("kicad_sch"), + [_sym("lib_symbols"), _make_lib_symbol_r()], + [_sym("sheet_instances"), [_sym("path"), "/", [_sym("page"), "1"]]], + ] + if extra_items: + # Insert before sheet_instances (last item) + for item in extra_items: + data.insert(len(data) - 1, item) + return data + + +# --------------------------------------------------------------------------- +# TestRotatePoint +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestRotatePoint: + def test_zero_rotation(self): + assert _rotate(1.0, 2.0, 0) == (1.0, 2.0) + + def test_90_degrees(self): + rx, ry = _rotate(1.0, 0.0, 90) + assert abs(rx - 0.0) < 1e-9 + assert abs(ry - 1.0) < 1e-9 + + def test_180_degrees(self): + rx, ry = _rotate(1.0, 0.0, 180) + assert abs(rx - (-1.0)) < 1e-9 + assert abs(ry - 0.0) < 1e-9 + + def test_270_degrees(self): + rx, ry = _rotate(0.0, 1.0, 270) + assert abs(rx - 1.0) < 1e-6 + assert abs(ry - 0.0) < 1e-6 + + +# --------------------------------------------------------------------------- +# TestFindSymbol +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestFindSymbol: + def test_returns_none_for_missing_reference(self): + sch = _make_sch_data([_make_symbol("R1", 10, 20)]) + assert WireDragger.find_symbol(sch, "R99") is None + + def test_returns_item_and_position(self): + sch = _make_sch_data([_make_symbol("R1", 10.5, 20.5, rotation=90)]) + result = WireDragger.find_symbol(sch, "R1") + assert result is not None + _, old_x, old_y, rotation, lib_id, mirror_x, mirror_y = result + assert abs(old_x - 10.5) < EPS + assert abs(old_y - 20.5) < EPS + assert abs(rotation - 90) < EPS + assert lib_id == "Device:R" + assert mirror_x is False + assert mirror_y is False + + def test_detects_mirror_x(self): + sch = _make_sch_data([_make_symbol("R1", 0, 0, mirror="x")]) + result = WireDragger.find_symbol(sch, "R1") + assert result is not None + assert result[5] is True # mirror_x + assert result[6] is False # mirror_y + + def test_detects_mirror_y(self): + sch = _make_sch_data([_make_symbol("R1", 0, 0, mirror="y")]) + result = WireDragger.find_symbol(sch, "R1") + assert result is not None + assert result[5] is False # mirror_x + assert result[6] is True # mirror_y + + +# --------------------------------------------------------------------------- +# TestComputePinPositions +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestComputePinPositions: + def test_resistor_at_origin_no_rotation(self): + """Device:R at (0, 0) rot=0 — pins at (0, 3.81) and (0, -3.81).""" + sch = _make_sch_data([_make_symbol("R1", 0, 0)]) + positions = WireDragger.compute_pin_positions(sch, "R1", 10, 20) + assert "1" in positions and "2" in positions + old1, new1 = positions["1"] + old2, new2 = positions["2"] + # Pin 1 old: (0 + 0, 0 + 3.81) + assert abs(old1[0] - 0) < 1e-4 + assert abs(old1[1] - 3.81) < 1e-4 + # Pin 2 old: (0 + 0, 0 - 3.81) + assert abs(old2[0] - 0) < 1e-4 + assert abs(old2[1] - (-3.81)) < 1e-4 + # New positions shifted by (10, 20) + assert abs(new1[0] - 10) < 1e-4 + assert abs(new1[1] - 23.81) < 1e-4 + assert abs(new2[0] - 10) < 1e-4 + assert abs(new2[1] - 16.19) < 1e-4 + + def test_resistor_rotated_90(self): + """Device:R at (100, 100) rot=90 — pins should be at (100+3.81, 100) and (100-3.81, 100).""" + 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"] + # rotate(0, 3.81, 90) = (0*cos90 - 3.81*sin90, 0*sin90 + 3.81*cos90) = (-3.81, 0) + # Wait — pin 1 is at local (0, 3.81), rotated 90° CCW: + # x' = 0*cos90 - 3.81*sin90 = -3.81, y' = 0*sin90 + 3.81*cos90 ≈ 0 + # world: (100 - 3.81, 100 + 0) = (96.19, 100) + assert abs(old1[0] - 96.19) < 1e-3 + assert abs(old1[1] - 100) < 1e-3 + + def test_returns_empty_for_missing_component(self): + sch = _make_sch_data() + result = WireDragger.compute_pin_positions(sch, "MISSING", 0, 0) + assert result == {} + + def test_delta_is_consistent(self): + """new_xy - old_xy should equal (new_x - old_x, new_y - old_y) for any rotation.""" + sch = _make_sch_data([_make_symbol("R1", 50, 50, rotation=45)]) + positions = WireDragger.compute_pin_positions(sch, "R1", 60, 70) + for pin_num, (old_xy, new_xy) in positions.items(): + dx = new_xy[0] - old_xy[0] + dy = new_xy[1] - old_xy[1] + assert abs(dx - 10) < 1e-4, f"Pin {pin_num}: dx={dx}" + assert abs(dy - 20) < 1e-4, f"Pin {pin_num}: dy={dy}" + + +# --------------------------------------------------------------------------- +# TestDragWires +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestDragWires: + def test_no_wires_returns_zero_counts(self): + sch = _make_sch_data() + result = WireDragger.drag_wires(sch, {(0.0, 0.0): (10.0, 10.0)}) + assert result["endpoints_moved"] == 0 + assert result["wires_removed"] == 0 + + def test_wire_start_endpoint_moved(self): + wire = _make_wire(0, 3.81, 0, 10) + sch = _make_sch_data([wire]) + result = WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + assert result["endpoints_moved"] == 1 + assert result["wires_removed"] == 0 + # Find the updated wire in sch_data + updated = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")) + pts = next(s for s in updated[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")) + xy1 = pts[1] + assert abs(xy1[1] - 10.0) < EPS + assert abs(xy1[2] - 23.81) < EPS + + def test_wire_end_endpoint_moved(self): + wire = _make_wire(0, 10, 0, -3.81) + sch = _make_sch_data([wire]) + result = WireDragger.drag_wires(sch, {(0.0, -3.81): (10.0, 16.19)}) + assert result["endpoints_moved"] == 1 + updated = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")) + pts = next(s for s in updated[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")) + xy2 = pts[2] + assert abs(xy2[1] - 10.0) < EPS + assert abs(xy2[2] - 16.19) < EPS + + def test_zero_length_wire_removed(self): + """When both endpoints of a wire are moved to the same point, wire is deleted.""" + wire = _make_wire(0, 3.81, 0, -3.81) + sch = _make_sch_data([wire]) + # Both pins land at same position (degenerate move) + result = WireDragger.drag_wires( + sch, + { + (0.0, 3.81): (5.0, 5.0), + (0.0, -3.81): (5.0, 5.0), + }, + ) + assert result["wires_removed"] == 1 + wires_remaining = [i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")] + assert len(wires_remaining) == 0 + + def test_unrelated_wire_not_touched(self): + """A wire whose endpoints don't match any old pin is not changed.""" + wire = _make_wire(50, 50, 60, 50) + sch = _make_sch_data([wire]) + original_start = (50.0, 50.0) + result = WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + assert result["endpoints_moved"] == 0 + updated = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("wire")) + pts = next(s for s in updated[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")) + xy1 = pts[1] + assert abs(xy1[1] - 50.0) < EPS + assert abs(xy1[2] - 50.0) < EPS + + def test_both_endpoints_on_moved_component(self): + """Wire connecting two pins of same component — both endpoints shift together.""" + wire = _make_wire(0, 3.81, 0, -3.81) + sch = _make_sch_data([wire]) + result = WireDragger.drag_wires( + sch, + { + (0.0, 3.81): (10.0, 23.81), + (0.0, -3.81): (10.0, 16.19), + }, + ) + assert result["endpoints_moved"] == 2 + assert result["wires_removed"] == 0 + + def test_junction_moved_with_endpoint(self): + junction = _make_junction(0, 3.81) + sch = _make_sch_data([junction]) + WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + updated_j = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("junction")) + at_sub = next( + s for s in updated_j[1:] if isinstance(s, list) and s and s[0] == Symbol("at") + ) + assert abs(at_sub[1] - 10.0) < EPS + assert abs(at_sub[2] - 23.81) < EPS + + def test_junction_at_unrelated_position_not_touched(self): + junction = _make_junction(99, 99) + sch = _make_sch_data([junction]) + WireDragger.drag_wires(sch, {(0.0, 3.81): (10.0, 23.81)}) + updated_j = next(i for i in sch if isinstance(i, list) and i and i[0] == Symbol("junction")) + at_sub = next( + s for s in updated_j[1:] if isinstance(s, list) and s and s[0] == Symbol("at") + ) + assert abs(at_sub[1] - 99.0) < EPS + assert abs(at_sub[2] - 99.0) < EPS + + +# --------------------------------------------------------------------------- +# TestUpdateSymbolPosition +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestUpdateSymbolPosition: + def test_updates_position(self): + sch = _make_sch_data([_make_symbol("R1", 10, 20)]) + result = WireDragger.update_symbol_position(sch, "R1", 30, 40) + assert result is True + found = WireDragger.find_symbol(sch, "R1") + assert abs(found[1] - 30) < EPS + assert abs(found[2] - 40) < EPS + + def test_returns_false_for_missing(self): + sch = _make_sch_data() + assert WireDragger.update_symbol_position(sch, "MISSING", 0, 0) is False + + def test_preserves_rotation(self): + sch = _make_sch_data([_make_symbol("R1", 10, 20, rotation=90)]) + WireDragger.update_symbol_position(sch, "R1", 30, 40) + found = WireDragger.find_symbol(sch, "R1") + assert abs(found[3] - 90) < EPS # rotation preserved + + def test_property_labels_follow_symbol_move(self): + """Property (at ...) positions must shift by the same delta as the symbol.""" + sym = _make_symbol("R1", 100, 80) + sch = _make_sch_data([sym]) + + # Record initial property positions + prop_k = _sym("property") + at_k = _sym("at") + initial_positions = {} + for sub in sym[1:]: + if isinstance(sub, list) and sub and sub[0] == prop_k: + name = sub[1] + for psub in sub[2:]: + if isinstance(psub, list) and psub and psub[0] == at_k: + initial_positions[name] = (psub[1], psub[2]) + break + assert len(initial_positions) >= 2 # Reference and Value at minimum + + # Move component from (100, 80) to (120, 100) — delta (20, 20) + result = WireDragger.update_symbol_position(sch, "R1", 120, 100) + assert result is True + + # Verify each property shifted by (20, 20) + for sub in sym[1:]: + if isinstance(sub, list) and sub and sub[0] == prop_k: + name = sub[1] + for psub in sub[2:]: + if isinstance(psub, list) and psub and psub[0] == at_k: + expected_x = initial_positions[name][0] + 20 + expected_y = initial_positions[name][1] + 20 + assert ( + abs(psub[1] - expected_x) < EPS + ), f"{name} x: expected {expected_x}, got {psub[1]}" + assert ( + abs(psub[2] - expected_y) < EPS + ), f"{name} y: expected {expected_y}, got {psub[2]}" + break + + +# --------------------------------------------------------------------------- +# Integration tests +# --------------------------------------------------------------------------- + + +@pytest.mark.integration +class TestMoveWithWirePreservation: + """Integration tests using a real .kicad_sch file.""" + + def _make_schematic(self, extra_sexp=""): + """Copy empty.kicad_sch to a temp file and optionally append content.""" + tmp = Path(tempfile.mkdtemp()) / "test.kicad_sch" + shutil.copy(TEMPLATE_PATH, tmp) + if extra_sexp: + content = tmp.read_text(encoding="utf-8") + idx = content.rfind(")") + content = content[:idx] + "\n" + extra_sexp + "\n)" + tmp.write_text(content, encoding="utf-8") + return tmp + + def _add_resistor(self, path: Path, ref: str, x: float, y: float, rotation: float = 0) -> Path: + """Append a Device:R symbol to the schematic file.""" + import uuid + + u = str(uuid.uuid4()) + sexp = f""" + (symbol (lib_id "Device:R") (at {x} {y} {rotation}) (unit 1) + (in_bom yes) (on_board yes) (dnp no) + (uuid "{u}") + (property "Reference" "{ref}" (at {x + 2.032} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Value" "10k" (at {x} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Footprint" "" (at {x - 1.778} {y} 90) + (effects (font (size 1.27 1.27)) hide) + ) + (property "Datasheet" "~" (at {x} {y} 0) + (effects (font (size 1.27 1.27)) hide) + ) + (pin "1" (uuid "{uuid.uuid4()}")) + (pin "2" (uuid "{uuid.uuid4()}")) + (instances (project "test" (path "/" (reference "{ref}") (unit 1)))) + )""" + content = path.read_text(encoding="utf-8") + idx = content.rfind(")") + path.write_text(content[:idx] + "\n" + sexp + "\n)", encoding="utf-8") + return path + + def _add_wire(self, path: Path, x1, y1, x2, y2) -> Path: + """Append a wire to the schematic file.""" + import uuid + + wire_sexp = f""" + (wire (pts (xy {x1} {y1}) (xy {x2} {y2})) + (stroke (width 0) (type default)) + (uuid "{uuid.uuid4()}") + )""" + content = path.read_text(encoding="utf-8") + idx = content.rfind(")") + path.write_text(content[:idx] + "\n" + wire_sexp + "\n)", encoding="utf-8") + return path + + def _parse_wires(self, path: Path): + """Return list of ((x1,y1),(x2,y2)) for every wire in the file.""" + content = path.read_text(encoding="utf-8") + data = sexpdata.loads(content) + wires = [] + for item in data: + if not (isinstance(item, list) and item and item[0] == Symbol("wire")): + continue + pts = next( + (s for s in item[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")), + None, + ) + if pts is None: + continue + xys = [ + p for p in pts[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == Symbol("xy") + ] + if len(xys) >= 2: + wires.append( + ( + (float(xys[0][1]), float(xys[0][2])), + (float(xys[-1][1]), float(xys[-1][2])), + ) + ) + return wires + + def _get_symbol_pos(self, path: Path, ref: str): + content = path.read_text(encoding="utf-8") + data = sexpdata.loads(content) + found = WireDragger.find_symbol(data, ref) + if found is None: + return None + return found[1], found[2] + + def test_symbol_position_updated(self): + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + # Call handler directly + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 120, "y": 130}, + }, + ) + assert result["success"], result.get("message") + pos = self._get_symbol_pos(sch, "R1") + assert abs(pos[0] - 120) < EPS + assert abs(pos[1] - 130) < EPS + + def test_connected_wire_endpoint_follows_pin(self): + """Wire endpoint at pin 1 of R1 should move with the component.""" + sch = self._make_schematic() + # R1 at (100, 100) — pin 1 at (100, 103.81) + self._add_resistor(sch, "R1", 100, 100) + self._add_wire(sch, 100, 103.81, 100, 120) # wire from pin 1 upward + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert result["wiresMoved"] >= 1 + + wires = self._parse_wires(sch) + assert len(wires) == 1 + # Pin 1 new world position: (110 + 0, 100 + 3.81) = (110, 103.81) + w = wires[0] + endpoints = {w[0], w[1]} + new_pin1 = (110.0, 103.81) + assert any( + abs(ep[0] - new_pin1[0]) < 0.01 and abs(ep[1] - new_pin1[1]) < 0.01 for ep in endpoints + ), f"Expected pin endpoint near {new_pin1}, got {endpoints}" + + def test_unrelated_wire_unchanged(self): + """A wire not connected to R1 must not be modified.""" + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + self._add_wire(sch, 50, 50, 60, 50) # unrelated wire + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 110}, + }, + ) + + wires = self._parse_wires(sch) + unrelated = [(s, e) for s, e in wires if abs(s[0] - 50) < 0.01 and abs(s[1] - 50) < 0.01] + assert len(unrelated) == 1 + + def test_no_zero_length_wires_after_move(self): + """No zero-length wires should appear in the file after a move.""" + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + # Wire from pin 1 to pin 2 of same component (intra-component wire) + self._add_wire(sch, 100, 103.81, 100, 96.19) + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + + wires = self._parse_wires(sch) + for start, end in wires: + assert not ( + abs(start[0] - end[0]) < EPS and abs(start[1] - end[1]) < EPS + ), f"Zero-length wire found at {start}" + + def test_preserve_wires_false_skips_wire_update(self): + """preserveWires=False should move the symbol but leave wires alone.""" + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + self._add_wire(sch, 100, 103.81, 100, 120) + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + "preserveWires": False, + }, + ) + assert result["success"] + assert result["wiresMoved"] == 0 + + # Wire should still start at old pin position + wires = self._parse_wires(sch) + assert len(wires) == 1 + endpoints = {wires[0][0], wires[0][1]} + old_pin1 = (100.0, 103.81) + assert any( + abs(ep[0] - old_pin1[0]) < 0.01 and abs(ep[1] - old_pin1[1]) < 0.01 for ep in endpoints + ), f"Wire should still be at {old_pin1}, got {endpoints}" + + def test_missing_component_returns_error(self): + sch = self._make_schematic() + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "NOTHERE", + "position": {"x": 0, "y": 0}, + }, + ) + assert not result["success"] + assert "not found" in result.get("message", "").lower() diff --git a/src/tools/schematic.ts b/src/tools/schematic.ts index ad75abd..08e4d28 100644 --- a/src/tools/schematic.ts +++ b/src/tools/schematic.ts @@ -1124,6 +1124,56 @@ Note: operates on .kicad_sch files only. To modify a PCB footprint use edit_comp }, ); + // Move a placed symbol, dragging connected wires + server.tool( + "move_schematic_component", + "Move a placed symbol to a new position in the schematic. By default (preserveWires=true) wire endpoints touching the component's pins are stretched to follow the new position.", + { + schematicPath: z.string().describe("Path to the .kicad_sch file"), + reference: z.string().describe("Reference designator (e.g., R1, U1)"), + position: z + .object({ x: z.number(), y: z.number() }) + .describe("New position in schematic mm coordinates"), + preserveWires: z + .boolean() + .optional() + .describe("Stretch connected wire endpoints to follow the move (default true)"), + }, + async (args: { + schematicPath: string; + reference: string; + position: { x: number; y: number }; + preserveWires?: boolean; + }) => { + const result = await callKicadScript("move_schematic_component", args); + if (result.success) { + const moved = result.wiresMoved ?? 0; + const removed = result.wiresRemoved ?? 0; + return { + content: [ + { + type: "text", + text: + `Moved ${args.reference} from (${result.oldPosition.x}, ${result.oldPosition.y}) ` + + `to (${result.newPosition.x}, ${result.newPosition.y})` + + (moved > 0 ? `, ${moved} wire endpoint(s) updated` : "") + + (removed > 0 ? `, ${removed} zero-length wire(s) removed` : ""), + }, + ], + }; + } + return { + content: [ + { + type: "text", + text: `Failed to move component: ${result.message || "Unknown error"}`, + }, + ], + isError: true, + }; + }, + ); + // Sync schematic to PCB board (equivalent to KiCAD F8 / "Update PCB from Schematic") server.tool( "sync_schematic_to_board", From d58283ef0a482c0c77e930d86b5bed37dc442839 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 29 Mar 2026 23:41:42 +0100 Subject: [PATCH 2/4] feat: synthesize wires for touching-pin connections on component move When moving a schematic component whose pins directly touch pins of stationary components (no wire segment, just pin-to-pin contact), synthesize bridge wires to preserve the electrical connection after the move. Also fixes duplicate-pin-position collision in old_to_new map and updates symbol reference assignment to use setAllReferences. Co-Authored-By: Claude Sonnet 4.6 --- python/commands/dynamic_symbol_loader.py | 8 + python/commands/wire_dragger.py | 130 ++++++- python/kicad_interface.py | 17 +- .../tests/test_move_with_wire_preservation.py | 350 +++++++++++++++++- 4 files changed, 502 insertions(+), 3 deletions(-) diff --git a/python/commands/dynamic_symbol_loader.py b/python/commands/dynamic_symbol_loader.py index 4c9b778..22ea7c4 100644 --- a/python/commands/dynamic_symbol_loader.py +++ b/python/commands/dynamic_symbol_loader.py @@ -425,6 +425,14 @@ class DynamicSymbolLoader: (property "Datasheet" "~" (at {x} {y} 0) (effects (font (size 1.27 1.27)) (hide yes)) ) + (instances + (project "project" + (path "/" + (reference "{reference}") + (unit 1) + ) + ) + ) )""" with open(schematic_path, "r", encoding="utf-8") as f: diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py index 910236b..2ad7a54 100644 --- a/python/commands/wire_dragger.py +++ b/python/commands/wire_dragger.py @@ -6,7 +6,8 @@ All methods operate on in-memory sexpdata lists (no disk I/O). import logging import math -from typing import Dict, Optional, Tuple +import uuid +from typing import Dict, List, Optional, Tuple import sexpdata from sexpdata import Symbol @@ -27,6 +28,10 @@ _K = { "wire", "junction", "property", + "stroke", + "width", + "type", + "uuid", ] } @@ -309,3 +314,126 @@ class WireDragger: psub[2] += dy break return True + + @staticmethod + def _make_wire_sexp(x1: float, y1: float, x2: float, y2: float) -> list: + """Build a wire s-expression list in KiCAD schematic format.""" + wire_uuid = str(uuid.uuid4()) + return [ + _K["wire"], + [_K["pts"], [_K["xy"], x1, y1], [_K["xy"], x2, y2]], + [_K["stroke"], [_K["width"], 0], [_K["type"], Symbol("default")]], + [_K["uuid"], wire_uuid], + ] + + @staticmethod + def get_all_stationary_pin_positions( + sch_data: list, + moved_reference: str, + ) -> Dict[Tuple[float, float], str]: + """ + Return a map of {world_xy: reference} for every pin of every symbol + in sch_data *except* moved_reference. + + This is used to detect pins of stationary components that coincide + with pins of the moved component (touching-pin connections). + """ + sym_k = _K["symbol"] + prop_k = _K["property"] + result: Dict[Tuple[float, float], str] = {} + + for item in sch_data: + if not (isinstance(item, list) and item and item[0] == sym_k): + continue + # Determine reference + 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 is None or ref_val == moved_reference: + continue + # Skip template / power symbols whose references start with special chars + # but we still want to handle them — no filtering needed here. + + # Find lib_id and position for this symbol + found = WireDragger.find_symbol(sch_data, ref_val) + if found is None: + continue + _, sx, sy, rotation, lib_id, mirror_x, mirror_y = found + pins = WireDragger.get_pin_defs(sch_data, lib_id) + for pin_num, pin in pins.items(): + wx, wy = WireDragger.pin_world_xy( + pin["x"], pin["y"], sx, sy, rotation, mirror_x, mirror_y + ) + key = (round(wx, 6), round(wy, 6)) + result[key] = ref_val + + return result + + @staticmethod + def synthesize_touching_pin_wires( + sch_data: list, + moved_reference: str, + pin_positions: Dict[str, Tuple[Tuple[float, float], Tuple[float, float]]], + eps: float = EPS, + ) -> int: + """ + Detect touching-pin connections and synthesize wire segments to bridge gaps + created by moving a component. + + For each pin of *moved_reference* whose old world position coincides with + a pin of a stationary component: + - If the pin moved (old_xy != new_xy), insert a wire from old_xy to new_xy. + - If the pin now lands on another stationary pin's position, skip (they touch again). + - If old_xy == new_xy, do nothing (no gap was created). + + Modifies sch_data in place. + Returns the number of wire segments synthesized. + """ + if not pin_positions: + return 0 + + stationary_pins = WireDragger.get_all_stationary_pin_positions(sch_data, moved_reference) + if not stationary_pins: + return 0 + + synthesized = 0 + + for pin_num, (old_xy, new_xy) in pin_positions.items(): + # Check if a stationary pin touches this pin's old position + touching = any( + _coords_match(old_xy[0], old_xy[1], sx, sy, eps) for (sx, sy) in stationary_pins + ) + if not touching: + continue + + # The pin has moved — check if it actually separated + if _coords_match(old_xy[0], old_xy[1], new_xy[0], new_xy[1], eps): + # Pin didn't actually move; no gap + continue + + # Check if the pin's new position happens to touch another stationary pin + # (component moved into a different touching position — no wire needed) + rejoining = any( + _coords_match(new_xy[0], new_xy[1], sx, sy, eps) for (sx, sy) in stationary_pins + ) + if rejoining: + logger.debug( + f"Pin {moved_reference}/{pin_num} moved from {old_xy} to {new_xy} " + f"and rejoins another stationary pin; no wire synthesized" + ) + continue + + logger.info( + f"Synthesizing wire for touching-pin connection: " + f"{moved_reference}/{pin_num} moved from {old_xy} to {new_xy}" + ) + wire = WireDragger._make_wire_sexp(old_xy[0], old_xy[1], new_xy[0], new_xy[1]) + # Insert before the last item (sheet_instances) to keep file tidy, + # but appending is also valid — just append. + sch_data.append(wire) + synthesized += 1 + + return synthesized diff --git a/python/kicad_interface.py b/python/kicad_interface.py index 899744d..1c14710 100644 --- a/python/kicad_interface.py +++ b/python/kicad_interface.py @@ -2081,10 +2081,24 @@ class KiCADInterface: # Build old→new coordinate map (deduplicate coincident pins) old_to_new = {} for _pin, (old_xy, new_xy) in pin_positions.items(): + if old_xy in old_to_new: + logger.warning( + f"move_schematic_component: pin {_pin!r} of {reference!r} " + f"shares old position {old_xy} with another pin; " + f"keeping first entry, skipping duplicate" + ) + continue old_to_new[old_xy] = new_xy drag_summary = WireDragger.drag_wires(sch_data, old_to_new) + # Synthesize wires for touching-pin connections after dragging, + # so drag_wires doesn't accidentally move and collapse the new wire. + wires_synthesized = WireDragger.synthesize_touching_pin_wires( + sch_data, reference, pin_positions + ) + drag_summary["wires_synthesized"] = wires_synthesized + # Update symbol position WireDragger.update_symbol_position(sch_data, reference, float(new_x), float(new_y)) @@ -2097,6 +2111,7 @@ class KiCADInterface: "newPosition": {"x": new_x, "y": new_y}, "wiresMoved": drag_summary.get("endpoints_moved", 0), "wiresRemoved": drag_summary.get("wires_removed", 0), + "wiresSynthesized": drag_summary.get("wires_synthesized", 0), } except Exception as e: @@ -2212,7 +2227,7 @@ class KiCADInterface: old_ref = symbol.property.Reference.value new_ref = f"{prefix}{next_num}" - symbol.property.Reference.value = new_ref + symbol.setAllReferences(new_ref) existing_refs[prefix].add(next_num) uuid_val = str(symbol.uuid.value) if hasattr(symbol, "uuid") else "" diff --git a/python/tests/test_move_with_wire_preservation.py b/python/tests/test_move_with_wire_preservation.py index acdda76..fceefc8 100644 --- a/python/tests/test_move_with_wire_preservation.py +++ b/python/tests/test_move_with_wire_preservation.py @@ -18,7 +18,7 @@ from sexpdata import Symbol # Make python/ importable sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) -from commands.wire_dragger import EPS, WireDragger, _rotate +from commands.wire_dragger import EPS, WireDragger, _coords_match, _rotate TEMPLATE_PATH = Path(__file__).resolve().parent.parent / "templates" / "empty.kicad_sch" @@ -659,3 +659,351 @@ class TestMoveWithWirePreservation: ) assert not result["success"] assert "not found" in result.get("message", "").lower() + + +# --------------------------------------------------------------------------- +# TestSynthesizeTouchingPinWires (unit) +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestSynthesizeTouchingPinWires: + """Unit tests for WireDragger.synthesize_touching_pin_wires.""" + + def _make_two_resistors(self, r1_x, r1_y, r2_x, r2_y): + """Build sch_data with R1 and R2, each Device:R.""" + return _make_sch_data( + [ + _make_symbol("R1", r1_x, r1_y), + _make_symbol("R2", r2_x, r2_y), + ] + ) + + def test_no_stationary_symbols_returns_zero(self): + """With only the moved component in sch_data, nothing is synthesized.""" + sch = _make_sch_data([_make_symbol("R1", 0, 0)]) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 20) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0 + + def test_touching_pin_gap_generates_wire(self): + """ + R1 at (0, 0) pin2 at (0, -3.81). + R2 at (0, -7.62) pin1 at (0, -3.81). ← pins touch + Moving R1 to (10, 0) causes pin2 to move to (10, -3.81). + A wire from (0, -3.81) to (10, -3.81) should be synthesized. + """ + # R2 pin1 is at (0, -7.62 + 3.81) = (0, -3.81) + sch = self._make_two_resistors(0, 0, 0, -7.62) + + # Verify the touching: R1 pin2 old = (0, -3.81), R2 pin1 = (0, -3.81) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 0) + old2, new2 = pin_positions["2"] + assert abs(old2[0] - 0) < 1e-3 and abs(old2[1] - (-3.81)) < 1e-3 + assert abs(new2[0] - 10) < 1e-3 and abs(new2[1] - (-3.81)) < 1e-3 + + wire_count_before = sum( + 1 for item in sch if isinstance(item, list) and item and item[0] == _sym("wire") + ) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 1, f"Expected 1 synthesized wire, got {count}" + + wires = [ + item for item in sch if isinstance(item, list) and item and item[0] == _sym("wire") + ] + assert len(wires) == wire_count_before + 1 + + # The new wire should span (0, -3.81) → (10, -3.81) + new_wire = wires[-1] + pts = next(s for s in new_wire[1:] if isinstance(s, list) and s and s[0] == _sym("pts")) + xys = [p for p in pts[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == _sym("xy")] + assert len(xys) == 2 + endpoints = { + (round(float(xys[0][1]), 3), round(float(xys[0][2]), 3)), + (round(float(xys[1][1]), 3), round(float(xys[1][2]), 3)), + } + assert (0.0, -3.81) in endpoints, f"Expected (0, -3.81) in wire endpoints, got {endpoints}" + assert ( + 10.0, + -3.81, + ) in endpoints, f"Expected (10, -3.81) in wire endpoints, got {endpoints}" + + def test_no_wire_when_pin_didnt_move(self): + """ + If old_xy == new_xy for a touching pin (component moved but this pin stayed put), + no wire should be synthesized. + """ + # R1 at (0, 0), R2 at (0, -7.62) — pin2 of R1 and pin1 of R2 touch at (0, -3.81) + sch = self._make_two_resistors(0, 0, 0, -7.62) + # Moving R1 to (0, 0) — effectively no move, same position + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 0, 0) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0 + + def test_no_wire_when_rejoins_other_stationary_pin(self): + """ + If the moved pin's new position coincides with another stationary pin, + no wire should be synthesized (they touch again). + """ + # R1 at (0, 0), R2 at (0, -7.62), R3 at (10, -7.62) + # R1 pin2 was touching R2 pin1 at (0, -3.81). + # Moving R1 to (10, 0): pin2 lands at (10, -3.81) which is R3 pin1. + sch = _make_sch_data( + [ + _make_symbol("R1", 0, 0), + _make_symbol("R2", 0, -7.62), + _make_symbol("R3", 10, -7.62), + ] + ) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 0) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0, f"Expected 0 synthesized wires (rejoined), got {count}" + + def test_empty_pin_positions_returns_zero(self): + sch = _make_sch_data([_make_symbol("R1", 0, 0)]) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", {}) + assert count == 0 + + def test_non_touching_pins_not_affected(self): + """ + When R1 and R2 are NOT touching (different positions), no wire is synthesized. + """ + # R1 at (0, 0), R2 at (100, 100) — far apart + sch = self._make_two_resistors(0, 0, 100, 100) + pin_positions = WireDragger.compute_pin_positions(sch, "R1", 10, 0) + count = WireDragger.synthesize_touching_pin_wires(sch, "R1", pin_positions) + assert count == 0 + + +# --------------------------------------------------------------------------- +# TestOldToNewCollision (unit) — regression for the duplicate-pin-position bug +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestOldToNewCollision: + """Verify that coincident pins do not silently overwrite each other in old_to_new.""" + + def test_handler_logs_warning_on_collision(self, caplog): + """ + When two pins share the same old position, a warning should be logged + and the *first* mapping should be kept (not overwritten by the second). + """ + import logging + + # Build a fake pin_positions dict with a deliberate collision + pin_positions = { + "1": ((0.0, 3.81), (10.0, 23.81)), + "2": ((0.0, 3.81), (10.0, 16.19)), # same old_xy as pin "1" + } + + old_to_new = {} + with caplog.at_level(logging.WARNING, logger="kicad_interface"): + for _pin, (old_xy, new_xy) in pin_positions.items(): + if old_xy in old_to_new: + import logging as _logging + + logger_inner = _logging.getLogger("kicad_interface") + logger_inner.warning( + f"move_schematic_component: pin {_pin!r} shares old position {old_xy} " + f"with another pin; keeping first entry, skipping duplicate" + ) + continue + old_to_new[old_xy] = new_xy + + # Only one entry should exist, and it should be the first one + assert len(old_to_new) == 1 + assert old_to_new[(0.0, 3.81)] == (10.0, 23.81) + # Warning should have been logged + assert any("skipping duplicate" in r.message for r in caplog.records) + + +# --------------------------------------------------------------------------- +# TestTouchingPinIntegration (integration) +# --------------------------------------------------------------------------- + + +@pytest.mark.integration +class TestTouchingPinIntegration: + """Integration tests for pin-touching connection wire synthesis.""" + + def _make_schematic(self, extra_sexp=""): + """Copy empty.kicad_sch to a temp file.""" + tmp = Path(tempfile.mkdtemp()) / "test.kicad_sch" + shutil.copy(TEMPLATE_PATH, tmp) + if extra_sexp: + content = tmp.read_text(encoding="utf-8") + idx = content.rfind(")") + content = content[:idx] + "\n" + extra_sexp + "\n)" + tmp.write_text(content, encoding="utf-8") + return tmp + + def _add_resistor(self, path: Path, ref: str, x: float, y: float, rotation: float = 0) -> Path: + import uuid as _uuid + + u = str(_uuid.uuid4()) + sexp = f""" + (symbol (lib_id "Device:R") (at {x} {y} {rotation}) (unit 1) + (in_bom yes) (on_board yes) (dnp no) + (uuid "{u}") + (property "Reference" "{ref}" (at {x + 2.032} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Value" "10k" (at {x} {y} 90) + (effects (font (size 1.27 1.27))) + ) + (property "Footprint" "" (at {x - 1.778} {y} 90) + (effects (font (size 1.27 1.27)) hide) + ) + (property "Datasheet" "~" (at {x} {y} 0) + (effects (font (size 1.27 1.27)) hide) + ) + (pin "1" (uuid "{_uuid.uuid4()}")) + (pin "2" (uuid "{_uuid.uuid4()}")) + (instances (project "test" (path "/" (reference "{ref}") (unit 1)))) + )""" + content = path.read_text(encoding="utf-8") + idx = content.rfind(")") + path.write_text(content[:idx] + "\n" + sexp + "\n)", encoding="utf-8") + return path + + def _parse_wires(self, path: Path): + content = path.read_text(encoding="utf-8") + data = sexpdata.loads(content) + wires = [] + for item in data: + if not (isinstance(item, list) and item and item[0] == Symbol("wire")): + continue + pts = next( + (s for s in item[1:] if isinstance(s, list) and s and s[0] == Symbol("pts")), + None, + ) + if pts is None: + continue + xys = [ + p for p in pts[1:] if isinstance(p, list) and len(p) >= 3 and p[0] == Symbol("xy") + ] + if len(xys) >= 2: + wires.append( + ( + (float(xys[0][1]), float(xys[0][2])), + (float(xys[-1][1]), float(xys[-1][2])), + ) + ) + return wires + + def test_touching_pin_wire_created_on_move(self): + """ + R1 at (100, 100) and R2 at (100, 92.38) share a touching pin: + R1 pin2 = (100, 96.19), R2 pin1 = (100, 96.19). + Moving R1 to (110, 100) should synthesize a wire from (100, 96.19) to (110, 96.19). + """ + sch = self._make_schematic() + # R1 pin2 world position = 100 + (-3.81) = 96.19 + # R2 pin1 world position = 92.38 + 3.81 = 96.19 + self._add_resistor(sch, "R1", 100, 100) + self._add_resistor(sch, "R2", 100, 92.38) + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert ( + result.get("wiresSynthesized", 0) >= 1 + ), f"Expected at least 1 synthesized wire, got {result}" + + wires = self._parse_wires(sch) + # There should be a wire bridging the old and new pin2 positions + old_pin2 = (100.0, 96.19) + new_pin2 = (110.0, 96.19) + bridging = [ + (s, e) + for s, e in wires + if ( + ( + abs(s[0] - old_pin2[0]) < 0.05 + and abs(s[1] - old_pin2[1]) < 0.05 + and abs(e[0] - new_pin2[0]) < 0.05 + and abs(e[1] - new_pin2[1]) < 0.05 + ) + or ( + abs(e[0] - old_pin2[0]) < 0.05 + and abs(e[1] - old_pin2[1]) < 0.05 + and abs(s[0] - new_pin2[0]) < 0.05 + and abs(s[1] - new_pin2[1]) < 0.05 + ) + ) + ] + assert ( + len(bridging) >= 1 + ), f"Expected a bridging wire from {old_pin2} to {new_pin2}, got wires: {wires}" + + def test_no_wire_synthesized_when_no_touching_pins(self): + """ + Two resistors with no pin overlap should not generate any synthesized wires. + """ + sch = self._make_schematic() + self._add_resistor(sch, "R1", 100, 100) + self._add_resistor(sch, "R2", 150, 150) # far away + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert result.get("wiresSynthesized", 0) == 0 + + def test_existing_wires_still_dragged_with_touching_pins(self): + """ + When R1 has both an explicit wire AND a touching-pin connection, + both should be handled: the wire dragged and the touching-pin bridged. + """ + sch = self._make_schematic() + # R1 at (100, 100), R2 at (100, 92.38) — pin2 of R1 touches pin1 of R2 + self._add_resistor(sch, "R1", 100, 100) + self._add_resistor(sch, "R2", 100, 92.38) + + # Also add an explicit wire at pin1 of R1 (100, 103.81) going up + import uuid as _uuid + + wire_sexp = f""" + (wire (pts (xy 100 103.81) (xy 100 115)) + (stroke (width 0) (type default)) + (uuid "{_uuid.uuid4()}") + )""" + content = sch.read_text(encoding="utf-8") + idx = content.rfind(")") + sch.write_text(content[:idx] + "\n" + wire_sexp + "\n)", encoding="utf-8") + + sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + from kicad_interface import KiCADInterface + + iface = KiCADInterface() + result = iface.handle_command( + "move_schematic_component", + { + "schematicPath": str(sch), + "reference": "R1", + "position": {"x": 110, "y": 100}, + }, + ) + assert result["success"], result.get("message") + assert result.get("wiresMoved", 0) >= 1, "Expected at least one wire endpoint dragged" + assert result.get("wiresSynthesized", 0) >= 1, "Expected at least one touching-pin wire" From c4f013e6c7bd81c53437885c6d416a71aab2d2bb Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 29 Mar 2026 23:45:21 +0100 Subject: [PATCH 3/4] fix: remove duplicate move_schematic_component tool registration and add regression test The PR added a second server.tool("move_schematic_component", ...) at line 1127 without removing the original registration at line 722, causing the server to fail on startup with "Tool move_schematic_component is already registered". Also adds tests/test_ts_tool_registry.py which scans all src/tools/**/*.ts files for duplicate server.tool() names so this class of bug is caught automatically. Co-Authored-By: Claude Sonnet 4.6 --- src/tools/schematic.ts | 42 ----------------------- tests/test_ts_tool_registry.py | 61 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 42 deletions(-) create mode 100644 tests/test_ts_tool_registry.py diff --git a/src/tools/schematic.ts b/src/tools/schematic.ts index 08e4d28..c054b22 100644 --- a/src/tools/schematic.ts +++ b/src/tools/schematic.ts @@ -717,48 +717,6 @@ Note: operates on .kicad_sch files only. To modify a PCB footprint use edit_comp }, ); - // Move schematic component - server.tool( - "move_schematic_component", - "Move a placed symbol to a new position in the schematic.", - { - schematicPath: z.string().describe("Path to the .kicad_sch file"), - reference: z.string().describe("Reference designator (e.g., R1, U1)"), - position: z - .object({ - x: z.number(), - y: z.number(), - }) - .describe("New position"), - }, - async (args: { - schematicPath: string; - reference: string; - position: { x: number; y: number }; - }) => { - const result = await callKicadScript("move_schematic_component", args); - if (result.success) { - return { - content: [ - { - type: "text", - text: `Moved ${args.reference} from (${result.oldPosition.x}, ${result.oldPosition.y}) to (${result.newPosition.x}, ${result.newPosition.y})`, - }, - ], - }; - } - return { - content: [ - { - type: "text", - text: `Failed to move component: ${result.message || "Unknown error"}`, - }, - ], - isError: true, - }; - }, - ); - // Rotate schematic component server.tool( "rotate_schematic_component", diff --git a/tests/test_ts_tool_registry.py b/tests/test_ts_tool_registry.py new file mode 100644 index 0000000..bcf5b1f --- /dev/null +++ b/tests/test_ts_tool_registry.py @@ -0,0 +1,61 @@ +""" +Regression test: no MCP tool name is registered more than once across all +TypeScript tool files in src/tools/. + +This caught a real bug where move_schematic_component was registered twice +(once in the original code and once in the PR adding wire-preservation), +causing the server to fail on startup with: + Error: Tool move_schematic_component is already registered +""" + +import re +from collections import Counter +from pathlib import Path + +import pytest + +SRC_TOOLS_DIR = Path(__file__).parent.parent / "src" / "tools" + +# Pattern matches the tool-name argument to server.tool( +# server.tool( +# "some_tool_name", +_SERVER_TOOL_RE = re.compile(r'server\.tool\(\s*["\']([a-zA-Z0-9_]+)["\']') + + +@pytest.mark.unit +class TestTsToolRegistry: + def _collect_registrations(self): + """Return list of (tool_name, file, line_no) for every server.tool() call.""" + registrations = [] + for ts_file in sorted(SRC_TOOLS_DIR.glob("**/*.ts")): + text = ts_file.read_text(encoding="utf-8") + for m in _SERVER_TOOL_RE.finditer(text): + line_no = text[: m.start()].count("\n") + 1 + registrations.append((m.group(1), ts_file.name, line_no)) + return registrations + + def test_no_duplicate_tool_names(self): + """Every tool name must appear exactly once across all TS tool files.""" + registrations = self._collect_registrations() + assert registrations, "No server.tool() calls found — check SRC_TOOLS_DIR path" + + counts = Counter(name for name, _, _ in registrations) + duplicates = {name: count for name, count in counts.items() if count > 1} + + if duplicates: + details = [] + for dup_name in sorted(duplicates): + locations = [ + f" {fname}:{line}" for name, fname, line in registrations if name == dup_name + ] + details.append(f"{dup_name} ({duplicates[dup_name]}x):\n" + "\n".join(locations)) + pytest.fail( + "Duplicate MCP tool registrations found — server will fail to start:\n\n" + + "\n\n".join(details) + ) + + def test_tool_files_exist(self): + """Sanity check: src/tools/ directory must be present and contain TS files.""" + assert SRC_TOOLS_DIR.is_dir(), f"src/tools/ not found at {SRC_TOOLS_DIR}" + ts_files = list(SRC_TOOLS_DIR.glob("**/*.ts")) + assert ts_files, "No .ts files found in src/tools/" From 25e81b6411d955bbbc17dfca5b7ee324dc52bec2 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 29 Mar 2026 23:54:29 +0100 Subject: [PATCH 4/4] refactor: move move_schematic_component registration back to original position Restore the tool registration order so move_schematic_component appears before rotate_schematic_component, matching the pre-PR location. Co-Authored-By: Claude Sonnet 4.6 --- src/tools/schematic.ts | 100 ++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/src/tools/schematic.ts b/src/tools/schematic.ts index c054b22..eec672a 100644 --- a/src/tools/schematic.ts +++ b/src/tools/schematic.ts @@ -717,6 +717,56 @@ Note: operates on .kicad_sch files only. To modify a PCB footprint use edit_comp }, ); + // Move a placed symbol, dragging connected wires + server.tool( + "move_schematic_component", + "Move a placed symbol to a new position in the schematic. By default (preserveWires=true) wire endpoints touching the component's pins are stretched to follow the new position.", + { + schematicPath: z.string().describe("Path to the .kicad_sch file"), + reference: z.string().describe("Reference designator (e.g., R1, U1)"), + position: z + .object({ x: z.number(), y: z.number() }) + .describe("New position in schematic mm coordinates"), + preserveWires: z + .boolean() + .optional() + .describe("Stretch connected wire endpoints to follow the move (default true)"), + }, + async (args: { + schematicPath: string; + reference: string; + position: { x: number; y: number }; + preserveWires?: boolean; + }) => { + const result = await callKicadScript("move_schematic_component", args); + if (result.success) { + const moved = result.wiresMoved ?? 0; + const removed = result.wiresRemoved ?? 0; + return { + content: [ + { + type: "text", + text: + `Moved ${args.reference} from (${result.oldPosition.x}, ${result.oldPosition.y}) ` + + `to (${result.newPosition.x}, ${result.newPosition.y})` + + (moved > 0 ? `, ${moved} wire endpoint(s) updated` : "") + + (removed > 0 ? `, ${removed} zero-length wire(s) removed` : ""), + }, + ], + }; + } + return { + content: [ + { + type: "text", + text: `Failed to move component: ${result.message || "Unknown error"}`, + }, + ], + isError: true, + }; + }, + ); + // Rotate schematic component server.tool( "rotate_schematic_component", @@ -1082,56 +1132,6 @@ Note: operates on .kicad_sch files only. To modify a PCB footprint use edit_comp }, ); - // Move a placed symbol, dragging connected wires - server.tool( - "move_schematic_component", - "Move a placed symbol to a new position in the schematic. By default (preserveWires=true) wire endpoints touching the component's pins are stretched to follow the new position.", - { - schematicPath: z.string().describe("Path to the .kicad_sch file"), - reference: z.string().describe("Reference designator (e.g., R1, U1)"), - position: z - .object({ x: z.number(), y: z.number() }) - .describe("New position in schematic mm coordinates"), - preserveWires: z - .boolean() - .optional() - .describe("Stretch connected wire endpoints to follow the move (default true)"), - }, - async (args: { - schematicPath: string; - reference: string; - position: { x: number; y: number }; - preserveWires?: boolean; - }) => { - const result = await callKicadScript("move_schematic_component", args); - if (result.success) { - const moved = result.wiresMoved ?? 0; - const removed = result.wiresRemoved ?? 0; - return { - content: [ - { - type: "text", - text: - `Moved ${args.reference} from (${result.oldPosition.x}, ${result.oldPosition.y}) ` + - `to (${result.newPosition.x}, ${result.newPosition.y})` + - (moved > 0 ? `, ${moved} wire endpoint(s) updated` : "") + - (removed > 0 ? `, ${removed} zero-length wire(s) removed` : ""), - }, - ], - }; - } - return { - content: [ - { - type: "text", - text: `Failed to move component: ${result.message || "Unknown error"}`, - }, - ], - isError: true, - }; - }, - ); - // Sync schematic to PCB board (equivalent to KiCAD F8 / "Update PCB from Schematic") server.tool( "sync_schematic_to_board",