diff --git a/src/wireviz/DataClasses.py b/src/wireviz/DataClasses.py index c62c073..11e4cf7 100644 --- a/src/wireviz/DataClasses.py +++ b/src/wireviz/DataClasses.py @@ -213,6 +213,11 @@ class Connector: resolved.append(pin) # Make sure loop connected pins are not hidden. self.activate_pin(pin, None) + if resolved[0] == resolved[1]: + raise Exception( + f'Loop in connector "{self.name}" connects pin ' + f'"{resolved[0]}" to itself.' + ) self.loops[i] = resolved for i, item in enumerate(self.additional_components): @@ -229,7 +234,13 @@ class Connector: in_labels = pin in self.pinlabels if self.pinlabels else False if in_pins and in_labels: - # present in both lists — check for ambiguity + # present in both lists — check for duplicate labels first + if self.pinlabels.count(pin) > 1: + raise Exception( + f'Pin label "{pin}" in connector "{self.name}" ' + f"is defined more than once in pinlabels." + ) + # then check for positional ambiguity if self.pins.index(pin) != self.pinlabels.index(pin): raise Exception( f'"{pin}" in connector "{self.name}" exists in both ' @@ -252,7 +263,7 @@ class Connector: f'Unknown pin "{pin}" for connector "{self.name}"!' ) - def activate_pin(self, pin: Pin, side: Side) -> None: + def activate_pin(self, pin: Pin, side: Optional[Side]) -> None: self.visible_pins[pin] = True if side == Side.LEFT: self.ports_left = True diff --git a/src/wireviz/Harness.py b/src/wireviz/Harness.py index c4af236..e7c0ffa 100644 --- a/src/wireviz/Harness.py +++ b/src/wireviz/Harness.py @@ -105,28 +105,17 @@ class Harness: to_name: str, to_pin: (int, str), ) -> None: - # check from and to connectors - for name, pin in zip([from_name, to_name], [from_pin, to_pin]): + # resolve pin labels to pin numbers via Connector.resolve_pin() + for name, pin, is_from in [ + (from_name, from_pin, True), + (to_name, to_pin, False), + ]: if name is not None and name in self.connectors: - connector = self.connectors[name] - # check if provided name is ambiguous - if pin in connector.pins and pin in connector.pinlabels: - if connector.pins.index(pin) != connector.pinlabels.index(pin): - raise Exception( - f"{name}:{pin} is defined both in pinlabels and pins, for different pins." - ) - # TODO: Maybe issue a warning if present in both lists but referencing the same pin? - if pin in connector.pinlabels: - if connector.pinlabels.count(pin) > 1: - raise Exception(f"{name}:{pin} is defined more than once.") - index = connector.pinlabels.index(pin) - pin = connector.pins[index] # map pin name to pin number - if name == from_name: - from_pin = pin - if name == to_name: - to_pin = pin - if not pin in connector.pins: - raise Exception(f"{name}:{pin} not found.") + resolved = self.connectors[name].resolve_pin(pin) + if is_from: + from_pin = resolved + else: + to_pin = resolved # check via cable if via_name in self.cables: @@ -280,9 +269,14 @@ class Harness: else: raise Exception("No side for loops") for loop in connector.loops: + # Convert pin numbers to 1-based port indices. + # Ports are named p{index+1} in the connector table, + # not p{pin_number} — these differ for non-sequential pins. + idx0 = connector.pins.index(loop[0]) + 1 + idx1 = connector.pins.index(loop[1]) + 1 dot.edge( - f"{connector.name}:p{loop[0]}{loop_side}:{loop_dir}", - f"{connector.name}:p{loop[1]}{loop_side}:{loop_dir}", + f"{connector.name}:p{idx0}{loop_side}:{loop_dir}", + f"{connector.name}:p{idx1}{loop_side}:{loop_dir}", label=" ", # Work-around to avoid over-sized loops. ) diff --git a/tests/test_resolve_pin.py b/tests/test_resolve_pin.py new file mode 100644 index 0000000..81eedcd --- /dev/null +++ b/tests/test_resolve_pin.py @@ -0,0 +1,297 @@ +# -*- coding: utf-8 -*- + +"""Tests for Connector.resolve_pin() and loop pin resolution. + +Covers the fix for https://github.com/wireviz/WireViz/issues/432 +and related safety-critical pin resolution correctness. +""" + +import pytest + +from wireviz.DataClasses import Connector + + +def make_connector(pins=None, pinlabels=None, loops=None, **kwargs): + """Helper to build a Connector with minimal required fields.""" + args = {"name": "X1"} + if pins is not None: + args["pins"] = pins + if pinlabels is not None: + args["pinlabels"] = pinlabels + if loops is not None: + args["loops"] = loops + args.update(kwargs) + return Connector(**args) + + +# --- resolve_pin() happy paths --- + + +class TestResolvePinHappyPaths: + def test_pin_number_passthrough(self): + """Pin number that exists in self.pins returns unchanged.""" + c = make_connector(pins=[1, 2, 3]) + assert c.resolve_pin(2) == 2 + + def test_label_resolves_to_pin_number(self): + """Pin label resolves to the corresponding pin number.""" + c = make_connector(pins=[1, 2, 3], pinlabels=["VCC", "GND", "SIG"]) + assert c.resolve_pin("GND") == 2 + + def test_label_with_non_sequential_pins(self): + """Labels work correctly even when pin numbers are non-sequential.""" + c = make_connector(pins=[10, 20, 30], pinlabels=["A", "B", "C"]) + assert c.resolve_pin("B") == 20 + + def test_value_in_both_lists_same_position(self): + """Value that exists in both pins and pinlabels at the same index.""" + c = make_connector(pins=["A", "B", "C"], pinlabels=["A", "X", "Y"]) + assert c.resolve_pin("A") == "A" + + def test_empty_pinlabels_falls_through(self): + """When pinlabels is empty, pin numbers still resolve.""" + c = make_connector(pins=[1, 2, 3], pinlabels=[]) + assert c.resolve_pin(3) == 3 + + +# --- resolve_pin() error paths --- + + +class TestResolvePinErrors: + def test_unknown_pin_raises(self): + """Resolving a pin that doesn't exist in either list raises.""" + c = make_connector(pins=[1, 2, 3], pinlabels=["A", "B", "C"]) + with pytest.raises(Exception, match="Unknown pin"): + c.resolve_pin("NONEXISTENT") + + def test_unknown_number_raises(self): + """Resolving a pin number not in self.pins raises.""" + c = make_connector(pins=[1, 2, 3]) + with pytest.raises(Exception, match="Unknown pin"): + c.resolve_pin(99) + + def test_ambiguous_pin_different_positions(self): + """Value in both pins and pinlabels at different positions raises.""" + c = make_connector(pins=["A", "B", "C"], pinlabels=["X", "A", "Y"]) + with pytest.raises(Exception, match="exists in both"): + c.resolve_pin("A") + + def test_duplicate_label_only_in_labels(self): + """Duplicate label in pinlabels (label-only path) raises.""" + c = make_connector(pins=[1, 2, 3], pinlabels=["A", "B", "A"]) + with pytest.raises(Exception, match="defined more than once"): + c.resolve_pin("A") + + def test_duplicate_label_in_both_lists(self): + """Duplicate label detected even when value also exists in pins (C-2 fix).""" + c = make_connector(pins=["A", "B", "C"], pinlabels=["A", "X", "A"]) + with pytest.raises(Exception, match="defined more than once"): + c.resolve_pin("A") + + +# --- Loop resolution --- + + +class TestLoopResolution: + def test_loop_with_labels(self): + """Loops specified with pin labels resolve to pin numbers.""" + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["VCC", "GND"]], + ) + assert c.loops == [[1, 2]] + + def test_loop_with_mixed_number_and_label(self): + """Loop with one pin number and one label resolves correctly.""" + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[[1, "GND"]], + ) + assert c.loops == [[1, 2]] + + def test_loop_with_non_sequential_pins(self): + """Loops resolve correctly with non-sequential pin numbering.""" + c = make_connector( + pins=[10, 20, 30, 40], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["VCC", "GND"]], + ) + assert c.loops == [[10, 20]] + + def test_loop_self_reference_raises(self): + """A loop from a pin to itself raises (I-2 fix).""" + with pytest.raises(Exception, match="to itself"): + make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + loops=[["A", "A"]], + ) + + def test_loop_self_reference_via_label_and_number(self): + """Self-loop detected even when specified as label + number.""" + with pytest.raises(Exception, match="to itself"): + make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + loops=[[2, "B"]], + ) + + def test_loop_activates_pins(self): + """Loop-connected pins are marked visible (for hide_disconnected_pins).""" + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["TX", "RX"]], + ) + assert c.visible_pins.get(3) is True + assert c.visible_pins.get(4) is True + # Unconnected pins should not be in visible_pins + assert 1 not in c.visible_pins + + def test_multiple_loops(self): + """Multiple loops on the same connector all resolve correctly.""" + c = make_connector( + pins=[1, 2, 3, 4, 5, 6], + pinlabels=["A", "B", "C", "D", "E", "F"], + loops=[["A", "B"], ["E", "F"]], + ) + assert c.loops == [[1, 2], [5, 6]] + + +# --- Loop rendering: non-sequential pins (C-1 fix) --- + + +class TestLoopRendering: + """Verify that the GraphViz output uses port indices, not pin numbers. + + This is the critical C-1 fix: Harness.py must convert resolved pin + numbers to 1-based position indices for GraphViz port references. + """ + + def _render_gv(self, yaml_str): + """Parse a YAML string through WireViz and return the GV source.""" + import yaml + + from wireviz.DataClasses import Metadata, Options, Tweak + from wireviz.Harness import Harness + + data = yaml.safe_load(yaml_str) + harness = Harness( + metadata=Metadata(data.get("metadata", {})), + options=Options(**data.get("options", {})), + tweak=Tweak(**data.get("tweak", {})), + ) + for name, conn_data in data.get("connectors", {}).items(): + harness.add_connector(name, **conn_data) + for name, cable_data in data.get("cables", {}).items(): + harness.add_cable(name, **cable_data) + + graph = harness.create_graph() + return graph.source + + def test_non_sequential_pins_use_indices(self): + """C-1 regression test: loops must use port indices, not pin numbers. + + With pins: [10, 20, 30, 40], a loop between pins 10 and 20 + must produce port references p1 and p2 (1-based indices), + NOT p10 and p20 (pin numbers). + """ + yaml_str = """ +connectors: + X1: + pins: [10, 20, 30, 40] + pinlabels: [VCC, GND, TX, RX] + loops: + - [VCC, GND] +cables: + W1: + wirecount: 2 + colors: [BK, RD] +connections: + - + - X1: [TX, RX] + - W1: [1-2] +""" + gv = self._render_gv(yaml_str) + # The loop should reference p1 and p2 (positions), not p10 and p20. + # Look specifically for the loop edge pattern with port names. + import re + + loop_edges = re.findall(r"X1:p(\d+)\w:\w -- X1:p(\d+)\w:\w", gv) + assert len(loop_edges) == 1, f"Expected 1 loop edge, found {len(loop_edges)}" + idx_a, idx_b = loop_edges[0] + # Pin 10 is at index 1, pin 20 is at index 2 + assert idx_a == "1" and idx_b == "2", ( + f"Loop ports should be p1/p2 (indices), got p{idx_a}/p{idx_b}" + ) + + def test_sequential_pins_still_work(self): + """Sequential pins (the common case) continue to work correctly.""" + yaml_str = """ +connectors: + X1: + pinlabels: [VCC, GND, TX, RX] + loops: + - [VCC, GND] +cables: + W1: + wirecount: 2 + colors: [BK, RD] +connections: + - + - X1: [TX, RX] + - W1: [1-2] +""" + gv = self._render_gv(yaml_str) + # With sequential pins [1,2,3,4], p1/p2 are both the index and number + assert ":p1" in gv + assert ":p2" in gv + + +# --- Harness.connect() delegation (I-1 fix) --- + + +class TestHarnessConnectDelegation: + """Verify Harness.connect() now delegates to Connector.resolve_pin().""" + + def test_connect_resolves_labels(self): + """Wire connections using pin labels resolve correctly.""" + from wireviz.DataClasses import Cable, Metadata, Options, Tweak + from wireviz.Harness import Harness + + harness = Harness( + metadata=Metadata({}), + options=Options(), + tweak=Tweak(), + ) + harness.add_connector( + "X1", pins=[1, 2, 3], pinlabels=["VCC", "GND", "SIG"] + ) + harness.add_connector("X2", pins=[1, 2, 3]) + harness.add_cable("W1", wirecount=1, colors=["BK"]) + + # Connect using a label on the from side + harness.connect("X1", "SIG", "W1", 1, "X2", 1) + + # The connection should store the resolved pin number (3), not the label + conn = harness.cables["W1"].connections[0] + assert conn.from_pin == 3 + + def test_connect_rejects_unknown_pin(self): + """Wire connections with unknown pins raise via resolve_pin().""" + from wireviz.DataClasses import Metadata, Options, Tweak + from wireviz.Harness import Harness + + harness = Harness( + metadata=Metadata({}), + options=Options(), + tweak=Tweak(), + ) + harness.add_connector("X1", pins=[1, 2, 3]) + harness.add_connector("X2", pins=[1, 2, 3]) + harness.add_cable("W1", wirecount=1, colors=["BK"]) + + with pytest.raises(Exception, match="Unknown pin"): + harness.connect("X1", 99, "W1", 1, "X2", 1)