Address all critical and important findings from safety review
C1: Port count mismatch now emits ERROR to stderr and marks
unconnected ports as __UNCONNECTED_<name>__ (never silent)
C2: Single-module pin mapping built in lockstep — cable_wires
always matches header_pins length, warns on unmappable nets
C3: VSS removed from is_power_net (it's ground per CMOS convention),
dead _POWER_PATTERN regex replaced with _KNOWN_NET_PATTERN
C4: apply_filters dead computation removed, docstring clarifies
that net-level filtering is a mapper concern
I3: WireViz render catches (ValueError, TypeError, OSError) with
diagnostic context instead of bare Exception
I5: Invalid --format flags now warn and error instead of silently
falling through to defaults
I6: Model/value heuristic warns on stderr when it triggers, since
signal names like ALERT or DATA could be misidentified
New tests: VSS classification, port count mismatch (C1), model/value
heuristic warning (I6), duplicate refs (S3), empty subcircuit (S2),
full pipeline determinism across 3 runs (S1)
115 tests pass, ruff clean
This commit is contained in:
parent
eb3ad60bd1
commit
b9154e851b
7 changed files with 254 additions and 46 deletions
|
|
@ -288,11 +288,31 @@ def _render_wireviz(
|
|||
sys.exit(1)
|
||||
|
||||
# Map format flags to WireViz output_formats
|
||||
format_map = {"h": "html", "p": "png", "s": "svg", "g": "gv", "c": "csv", "t": "tsv"}
|
||||
formats = tuple(format_map[f] for f in format_flags.lower() if f in format_map)
|
||||
format_map = {
|
||||
"h": "html", "p": "png", "s": "svg",
|
||||
"g": "gv", "c": "csv", "t": "tsv",
|
||||
}
|
||||
|
||||
# Warn about unrecognized format characters
|
||||
invalid_chars = [f for f in format_flags.lower() if f not in format_map]
|
||||
if invalid_chars:
|
||||
click.echo(
|
||||
f"Warning: unrecognized format flags: {''.join(invalid_chars)} "
|
||||
f"(valid: {''.join(format_map.keys())})",
|
||||
err=True,
|
||||
)
|
||||
|
||||
formats = tuple(
|
||||
format_map[f] for f in format_flags.lower() if f in format_map
|
||||
)
|
||||
|
||||
if not formats:
|
||||
formats = ("html", "png", "svg")
|
||||
click.echo(
|
||||
"Error: no valid format flags provided. "
|
||||
f"Valid flags: {''.join(format_map.keys())}",
|
||||
err=True,
|
||||
)
|
||||
sys.exit(1)
|
||||
|
||||
output_dir = output.parent if output else Path.cwd()
|
||||
output_name = output.stem if output else "spice2wireviz_output"
|
||||
|
|
@ -308,6 +328,12 @@ def _render_wireviz(
|
|||
f"Rendered: {', '.join(f'{output_name}.{fmt}' for fmt in formats)}",
|
||||
err=True,
|
||||
)
|
||||
except Exception as exc:
|
||||
click.echo(f"WireViz render error: {exc}", err=True)
|
||||
except (ValueError, TypeError, OSError) as exc:
|
||||
click.echo(
|
||||
f"WireViz render error ({type(exc).__name__}): {exc}\n"
|
||||
f" Output dir: {output_dir}\n"
|
||||
f" Output name: {output_name}\n"
|
||||
f" Formats: {formats}",
|
||||
err=True,
|
||||
)
|
||||
sys.exit(1)
|
||||
|
|
|
|||
|
|
@ -124,7 +124,12 @@ def filter_net(net: str, config: FilterConfig, netlist: ParsedNetlist) -> bool:
|
|||
|
||||
|
||||
def apply_filters(netlist: ParsedNetlist, config: FilterConfig) -> ParsedNetlist:
|
||||
"""Return a new ParsedNetlist with filters applied.
|
||||
"""Return a new ParsedNetlist with component/instance-level filters applied.
|
||||
|
||||
Filters components by prefix/ref and instances by prefix/ref/subcircuit.
|
||||
Net-level filtering (ground, power, glob patterns) is handled by the
|
||||
mappers during connection tracing, not here — because net filtering
|
||||
affects pin lists and cable generation which are mapper concerns.
|
||||
|
||||
Produces warnings on stderr for significant exclusions.
|
||||
"""
|
||||
|
|
@ -138,17 +143,13 @@ def apply_filters(netlist: ParsedNetlist, config: FilterConfig) -> ParsedNetlist
|
|||
inst for inst in netlist.instances if filter_instance(inst, config, netlist)
|
||||
]
|
||||
|
||||
# Filter nets within surviving components/instances
|
||||
excluded_net_count = 0
|
||||
for comp in filtered_components:
|
||||
original_count = len(comp.nodes)
|
||||
comp = comp.model_copy()
|
||||
new_nodes = [n for n in comp.nodes if filter_net(n, config, netlist)]
|
||||
excluded_net_count += original_count - len(new_nodes)
|
||||
excluded_comp_count = len(netlist.top_level_components) - len(filtered_components)
|
||||
excluded_inst_count = len(netlist.instances) - len(filtered_instances)
|
||||
|
||||
if excluded_net_count > 0:
|
||||
if excluded_comp_count > 0 or excluded_inst_count > 0:
|
||||
print(
|
||||
f"Note: {excluded_net_count} net connections hidden by filters",
|
||||
f"Note: filters excluded {excluded_comp_count} components "
|
||||
f"and {excluded_inst_count} instances",
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ This shows the external interface of a single board/module.
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
from typing import Any
|
||||
|
||||
from ..emitter.yaml_emitter import (
|
||||
|
|
@ -103,31 +104,43 @@ def map_single_module(
|
|||
shared_nets = _find_shared_nets(subckt, comp, config, netlist)
|
||||
|
||||
if shared_nets and header_name in connectors:
|
||||
cable_name = f"W_{comp_name}"
|
||||
wire_colors = [_net_wire_color(net, netlist) for net in shared_nets]
|
||||
wire_labels = list(shared_nets)
|
||||
# Build pin mappings in lockstep — every entry must resolve
|
||||
mapped_header: list[int] = []
|
||||
mapped_comp: list[int] = []
|
||||
mapped_nets: list[str] = []
|
||||
|
||||
cables[cable_name] = build_cable(
|
||||
cable_name,
|
||||
wirecount=len(shared_nets),
|
||||
colors=wire_colors if any(wire_colors) else None,
|
||||
wirelabels=wire_labels,
|
||||
category="bundle",
|
||||
notes=f"Nets: {', '.join(shared_nets)}",
|
||||
)
|
||||
for net in shared_nets:
|
||||
if net in port_labels and net in comp.nodes:
|
||||
mapped_header.append(port_labels.index(net) + 1)
|
||||
mapped_comp.append(comp.nodes.index(net) + 1)
|
||||
mapped_nets.append(net)
|
||||
else:
|
||||
print(
|
||||
f"Warning: net '{net}' shared between "
|
||||
f"{header_name} and {comp_name} but not "
|
||||
f"resolvable in both pin lists",
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
# Map pin indices for connection
|
||||
header_pins = [port_labels.index(net) + 1 for net in shared_nets if net in port_labels]
|
||||
comp_pins = [
|
||||
comp.nodes.index(net) + 1 for net in shared_nets if net in comp.nodes
|
||||
]
|
||||
cable_wires = list(range(1, len(shared_nets) + 1))
|
||||
if mapped_nets:
|
||||
cable_name = f"W_{comp_name}"
|
||||
wire_colors = [
|
||||
_net_wire_color(net, netlist) for net in mapped_nets
|
||||
]
|
||||
cables[cable_name] = build_cable(
|
||||
cable_name,
|
||||
wirecount=len(mapped_nets),
|
||||
colors=wire_colors if any(wire_colors) else None,
|
||||
wirelabels=mapped_nets,
|
||||
category="bundle",
|
||||
notes=f"Nets: {', '.join(mapped_nets)}",
|
||||
)
|
||||
|
||||
if header_pins and comp_pins and len(header_pins) == len(comp_pins):
|
||||
cable_wires = list(range(1, len(mapped_nets) + 1))
|
||||
connections.append(build_connection(
|
||||
header_name, header_pins,
|
||||
header_name, mapped_header,
|
||||
cable_name, cable_wires,
|
||||
comp_name, comp_pins,
|
||||
comp_name, mapped_comp,
|
||||
))
|
||||
|
||||
return assemble_wireviz_doc(connectors, cables, connections, metadata)
|
||||
|
|
|
|||
|
|
@ -96,8 +96,10 @@ class ParsedNetlist(BaseModel):
|
|||
|
||||
def is_power_net(self, net: str) -> bool:
|
||||
upper = net.upper()
|
||||
return upper in {"VCC", "VDD", "V+", "V-", "VEE", "VSS", "AVCC", "AVDD", "DVCC", "DVDD"}
|
||||
# VSS is ground in CMOS conventions, not power — see is_ground_net
|
||||
return upper in {"VCC", "VDD", "V+", "V-", "VEE", "AVCC", "AVDD", "DVCC", "DVDD"}
|
||||
|
||||
def is_ground_net(self, net: str) -> bool:
|
||||
upper = net.upper()
|
||||
# VSS is ground (CMOS convention: VSS = negative supply = ground)
|
||||
return upper in {"GND", "AGND", "DGND", "GND!", "EARTH", "VSS"} or net == "0"
|
||||
|
|
|
|||
|
|
@ -36,13 +36,17 @@ BOUNDARY_PREFIXES = {"J", "TP", "P"}
|
|||
# Prefixes we recognize as external-facing components
|
||||
RECOGNIZED_PREFIXES = {"J", "TP", "P", "X"}
|
||||
|
||||
# Known power net patterns
|
||||
_POWER_PATTERN = re.compile(
|
||||
r"^(V(CC|DD|EE|SS|[+-])|A?V(CC|DD)|D?V(CC|DD))$", re.IGNORECASE
|
||||
)
|
||||
# Known ground net patterns (used in value/model heuristic)
|
||||
_GROUND_PATTERN = re.compile(r"^(GND!?|AGND|DGND|EARTH|0)$", re.IGNORECASE)
|
||||
|
||||
# Known ground net patterns
|
||||
_GROUND_PATTERN = re.compile(r"^(GND!?|AGND|DGND|EARTH|VSS|0)$", re.IGNORECASE)
|
||||
# Known power net patterns (used in value/model heuristic)
|
||||
_POWER_NET_NAMES = {"VCC", "VDD", "V+", "V-", "VEE", "AVCC", "AVDD", "DVCC", "DVDD"}
|
||||
|
||||
# Combined: nets the heuristic should never misidentify as model/value names
|
||||
_KNOWN_NET_PATTERN = re.compile(
|
||||
r"^(V(CC|DD|EE|[+-])|AV(CC|DD)|DV(CC|DD)|GND!?|AGND|DGND|EARTH|VSS|0)$",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
|
||||
|
||||
def _strip_comment(line: str) -> str:
|
||||
|
|
@ -289,9 +293,30 @@ def _parse_x_instance(
|
|||
port_to_net: dict[str, str] = {}
|
||||
subckt_def = known_subcircuits.get(subckt_name)
|
||||
if subckt_def:
|
||||
expected = len(subckt_def.port_names)
|
||||
actual = len(nodes)
|
||||
if expected != actual:
|
||||
print(
|
||||
f"ERROR: port count mismatch for {ref}: "
|
||||
f"subcircuit '{subckt_name}' defines {expected} ports "
|
||||
f"({', '.join(subckt_def.port_names)}) but instance "
|
||||
f"provides {actual} nodes ({', '.join(nodes)})",
|
||||
file=sys.stderr,
|
||||
)
|
||||
for i, port_name in enumerate(subckt_def.port_names):
|
||||
if i < len(nodes):
|
||||
port_to_net[port_name] = nodes[i]
|
||||
else:
|
||||
# Mark unconnected ports visibly
|
||||
port_to_net[port_name] = f"__UNCONNECTED_{port_name}__"
|
||||
# Warn about extra nodes beyond the definition
|
||||
if len(nodes) > expected:
|
||||
extras = nodes[expected:]
|
||||
print(
|
||||
f"Warning: {ref} has {len(extras)} extra nodes "
|
||||
f"beyond subcircuit definition: {', '.join(extras)}",
|
||||
file=sys.stderr,
|
||||
)
|
||||
else:
|
||||
# No definition available; use positional indices as port names
|
||||
for i, node in enumerate(nodes):
|
||||
|
|
@ -326,18 +351,27 @@ def _parse_boundary_component(tokens: list[str], prefix: str) -> SpiceComponent
|
|||
positional.append(token)
|
||||
|
||||
# Heuristic: if last positional looks like a model name (contains only
|
||||
# alphanumeric/underscore and doesn't match net patterns), treat it as value
|
||||
# alphanumeric/underscore and doesn't match net patterns), treat it as value.
|
||||
# WARNING: This can misidentify signal net names (e.g., ALERT, ENABLE, DATA)
|
||||
# as model names. We warn when the heuristic triggers.
|
||||
value = ""
|
||||
nodes = positional
|
||||
|
||||
if len(positional) >= 2:
|
||||
last = positional[-1]
|
||||
# If it's not purely numeric and not a known net pattern, it's likely a value/model
|
||||
if re.match(r"^[A-Za-z_]\w*$", last) and not _POWER_PATTERN.match(
|
||||
last
|
||||
) and not _GROUND_PATTERN.match(last):
|
||||
if (
|
||||
re.match(r"^[A-Za-z_]\w*$", last)
|
||||
and not _KNOWN_NET_PATTERN.match(last)
|
||||
):
|
||||
value = last
|
||||
nodes = positional[:-1]
|
||||
print(
|
||||
f"Note: treating '{last}' as model/value for {ref} "
|
||||
f"(not as net name). If this is wrong, the net "
|
||||
f"connection to '{last}' will be missing.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
pins = [
|
||||
SpicePin(
|
||||
|
|
|
|||
|
|
@ -146,6 +146,13 @@ class TestNetClassification:
|
|||
assert not netlist.is_power_net("GND")
|
||||
assert not netlist.is_power_net("SIGNAL")
|
||||
|
||||
def test_vss_is_ground_not_power(self):
|
||||
"""VSS should be classified as ground (CMOS convention), not power."""
|
||||
text = ".subckt test VSS VDD\n.ends test\n"
|
||||
netlist = parse_netlist(text)
|
||||
assert netlist.is_ground_net("VSS")
|
||||
assert not netlist.is_power_net("VSS")
|
||||
|
||||
|
||||
class TestEdgeCases:
|
||||
def test_nonexistent_file(self):
|
||||
|
|
@ -169,3 +176,57 @@ X1 A B C undefined_subckt
|
|||
# Without definition, uses positional port names
|
||||
assert "port1" in inst.port_to_net
|
||||
assert inst.port_to_net["port1"] == "NET1"
|
||||
|
||||
def test_port_count_mismatch_fewer_nodes(self, capsys):
|
||||
"""C1: When instance has fewer nodes than subcircuit ports, warn and mark unconnected."""
|
||||
text = """\
|
||||
.subckt amp VIN GND VOUT ENABLE
|
||||
.ends amp
|
||||
X1 NET1 NET2 amp
|
||||
"""
|
||||
netlist = parse_netlist(text)
|
||||
inst = netlist.instances[0]
|
||||
captured = capsys.readouterr()
|
||||
assert "ERROR: port count mismatch" in captured.err
|
||||
assert "4 ports" in captured.err
|
||||
assert "2 nodes" in captured.err
|
||||
# Unconnected ports should be marked
|
||||
assert inst.port_to_net["VIN"] == "NET1"
|
||||
assert inst.port_to_net["GND"] == "NET2"
|
||||
assert "__UNCONNECTED_VOUT__" in inst.port_to_net["VOUT"]
|
||||
assert "__UNCONNECTED_ENABLE__" in inst.port_to_net["ENABLE"]
|
||||
|
||||
def test_port_count_mismatch_more_nodes(self, capsys):
|
||||
"""C1: When instance has more nodes than subcircuit ports, warn about extras."""
|
||||
text = """\
|
||||
.subckt small A B
|
||||
.ends small
|
||||
X1 NET1 NET2 NET3 NET4 small
|
||||
"""
|
||||
parse_netlist(text)
|
||||
captured = capsys.readouterr()
|
||||
assert "ERROR: port count mismatch" in captured.err
|
||||
assert "extra nodes" in captured.err
|
||||
|
||||
def test_model_value_heuristic_warning(self, capsys):
|
||||
"""I6: The model/value heuristic should warn when it triggers."""
|
||||
text = """\
|
||||
.subckt test A B
|
||||
J1 A B SOME_MODEL
|
||||
.ends test
|
||||
"""
|
||||
parse_netlist(text)
|
||||
captured = capsys.readouterr()
|
||||
assert "SOME_MODEL" in captured.err
|
||||
assert "model/value" in captured.err
|
||||
|
||||
def test_duplicate_refs_both_parsed(self):
|
||||
"""S3: Duplicate reference designators should both be parsed."""
|
||||
text = """\
|
||||
J1 NET_A NET_B CONN_TYPE
|
||||
J1 NET_C NET_D CONN_TYPE
|
||||
"""
|
||||
netlist = parse_netlist(text)
|
||||
# Both should appear (parser doesn't deduplicate)
|
||||
j1_refs = [c for c in netlist.top_level_components if c.reference == "J1"]
|
||||
assert len(j1_refs) == 2
|
||||
|
|
|
|||
|
|
@ -97,6 +97,77 @@ class TestYamlValidity:
|
|||
assert isinstance(entry, dict), f"Connection set {i} entry is not a dict"
|
||||
|
||||
|
||||
class TestPipelineDeterminism:
|
||||
"""S1: Verify the full pipeline produces byte-identical output on repeated runs."""
|
||||
|
||||
def test_inter_module_deterministic(self):
|
||||
"""Parse the same file multiple times; output must be identical."""
|
||||
outputs = []
|
||||
for _ in range(3):
|
||||
netlist = parse_netlist(FIXTURES / "hierarchical.net")
|
||||
result = map_inter_module(netlist)
|
||||
yaml_str = emit_yaml(result)
|
||||
outputs.append(yaml_str)
|
||||
assert outputs[0] == outputs[1] == outputs[2]
|
||||
|
||||
def test_single_module_deterministic(self):
|
||||
outputs = []
|
||||
for _ in range(3):
|
||||
netlist = parse_netlist(FIXTURES / "simple_board.net")
|
||||
result = map_single_module(netlist, "amplifier_board")
|
||||
yaml_str = emit_yaml(result)
|
||||
outputs.append(yaml_str)
|
||||
assert outputs[0] == outputs[1] == outputs[2]
|
||||
|
||||
def test_filtered_deterministic(self):
|
||||
config = FilterConfig(show_ground=False, show_power=False)
|
||||
outputs = []
|
||||
for _ in range(3):
|
||||
netlist = parse_netlist(FIXTURES / "multi_board.net")
|
||||
result = map_inter_module(netlist, config)
|
||||
yaml_str = emit_yaml(result)
|
||||
outputs.append(yaml_str)
|
||||
assert outputs[0] == outputs[1] == outputs[2]
|
||||
|
||||
|
||||
class TestEmptySubcircuit:
|
||||
"""S2: Subcircuit with ports but no boundary components."""
|
||||
|
||||
def test_empty_subcircuit_produces_header_only(self):
|
||||
text = """\
|
||||
.subckt bare_module A B C
|
||||
* No J*/TP*/P* inside — just passives
|
||||
R1 A B 10k
|
||||
.ends bare_module
|
||||
"""
|
||||
netlist = parse_netlist(text)
|
||||
result = map_single_module(netlist, "bare_module")
|
||||
# Should produce a header connector but no cables or connections
|
||||
assert "bare_module" in result["connectors"]
|
||||
assert len(result["cables"]) == 0
|
||||
assert len(result["connections"]) == 0
|
||||
|
||||
|
||||
class TestDuplicateRefInMapper:
|
||||
"""S3: Duplicate reference designators in the mapper."""
|
||||
|
||||
def test_duplicate_ref_last_wins_in_inter_module(self):
|
||||
"""When two top-level components share a reference, the mapper
|
||||
builds connectors from both but dict key collision means last wins.
|
||||
This is a known limitation that should at least not crash."""
|
||||
text = """\
|
||||
.subckt mod A B
|
||||
.ends mod
|
||||
X1 NET1 NET2 mod
|
||||
J1 NET1 NET2 CONN_A
|
||||
J1 NET3 NET4 CONN_B
|
||||
"""
|
||||
netlist = parse_netlist(text)
|
||||
result = map_inter_module(netlist)
|
||||
# Should not crash; J1 connector exists (last definition wins)
|
||||
assert "J1" in result["connectors"]
|
||||
|
||||
|
||||
@pytest.mark.skipif(not _has_wireviz(), reason="WireViz not installed")
|
||||
class TestWireVizRoundtrip:
|
||||
"""Feed generated YAML to WireViz's parse() to verify full compatibility."""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue