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"