From 8994d8e743260cd03dfc3925d6d6b9ada2dc707c Mon Sep 17 00:00:00 2001 From: grabowski Date: Wed, 22 Jul 2026 16:21:18 +0700 Subject: [PATCH] Overlay push: verify deletes, clear unwritten slots, one undo step kipy's Board.remove_items discards the DeleteItemsResponse, and the proto warns the overall status may read OK even when nothing was deleted - a locked image comes back IDS_IMMUTABLE. remove_overlays went through the proto layer already for create; do the same for delete, so a locked overlay is reported instead of silently surviving while a second image is stacked on top of it. Slots that a narrower run does not write kept the previous solve's heatmap and read as current; clear them. The whole push is now one commit, so a single undo reverts it rather than one layer of it. _pad_polygons probed F.Cu before B.Cu regardless of which side the joint protrudes from, so a pad sized differently per copper layer had its solder coat measured from the wrong face; probe the solder side first. Pads on no single copper layer are now noted rather than silently skipped. _fail could report nothing at all: before the output directory exists no PNG is written, and with no GUI toolkit plots only opens saved PNGs - the broken-plugin-environment case the docstring promises to cover. Fall back to the temp dir, and never let reporting mask the fault. Frequency input now keeps its specific rejection reason, as the other numeric fields do. --- fill_resistance/board_io.py | 124 ++++++++++++++++------ fill_resistance/dialog.py | 6 +- fill_resistance/main.py | 18 +++- tests/test_board_io_api.py | 201 ++++++++++++++++++++++++++++++++++++ 4 files changed, 314 insertions(+), 35 deletions(-) create mode 100644 tests/test_board_io_api.py diff --git a/fill_resistance/board_io.py b/fill_resistance/board_io.py index a530d8c..b86ba4b 100644 --- a/fill_resistance/board_io.py +++ b/fill_resistance/board_io.py @@ -182,11 +182,18 @@ def _pad_default_contact(pad: Pad) -> str: return "all" -def _pad_polygons(board: Board, pad: Pad, contact: str) -> list[Polygon] | None: +def _pad_polygons(board: Board, pad: Pad, contact: str, + prefer: str | None = None) -> list[Polygon] | None: + """Exact pad copper. The first probed layer that has a shape wins, so + `prefer` (the solder side of a THT joint) must be tried before the + F.Cu/B.Cu fallback: KiCad allows a different pad size per copper + layer, and the solder coat is sized from this shape.""" layer_ids = [] - if contact != "all": + for name in (contact if contact != "all" else None, prefer): + if not name: + continue try: - layer_ids.append(layer_from_canonical_name(contact)) + layer_ids.append(layer_from_canonical_name(name)) except Exception: pass for name in ("F.Cu", "B.Cu"): @@ -274,8 +281,10 @@ def _to_electrode(board: Board, item, stackup: StackupInfo | None = None, raise SelectionError(f"Could not get the bounding box of {label}.") rect = _box2_to_rect(box, "pad") drill, slot_dx, slot_dy = _drill_info(pad) + prot = _tht_protrusion_side(pad, pad_map or {}) if drill > 0 else None return Electrode(rect=rect, contact=contact, - polygons=_pad_polygons(board, pad, contact), label=label, + polygons=_pad_polygons(board, pad, contact, prefer=prot), + label=label, # through-hole pad: current enters at the soldered # barrel; the joint is solder-filled + pad-coated, # with a solder cone around the protruding lead @@ -283,9 +292,7 @@ def _to_electrode(board: Board, item, stackup: StackupInfo | None = None, pad_min_nm=_padstack_pad_min_nm(pad), slot_dx_nm=slot_dx, slot_dy_nm=slot_dy, center=(pad.position.x, pad.position.y), - solder=drill > 0, - protrusion_side=(_tht_protrusion_side(pad, pad_map or {}) - if drill > 0 else None)) + solder=drill > 0, protrusion_side=prot) def _net_hint_of(items: list) -> str | None: @@ -605,6 +612,10 @@ def gather_smd_pad_copper(board: Board, net_name: str continue layer = _pad_default_contact(pad) # SMD: its own copper layer if layer == "all": + # zero or >1 copper layers (custom padstack): no single layer + # to stamp it on. Say so - a silent skip loses a real junction + print(f"note: pad {pad.number}@{net_name} sits on no single " + f"copper layer - its pad copper is not modelled") continue polys = _pad_polygons(board, pad, layer) if polys: @@ -657,20 +668,48 @@ def _create_reference_image(board: Board, ref) -> None: def remove_overlays(board: Board, layer) -> int: - """Remove every reference image on the given layer; returns count.""" + """Remove every reference image on the given layer; returns count. + + remove_items with the per-item status surfaced: kipy discards the + DeleteItemsResponse, and its own proto warns the overall status "may + return IRS_OK even if no items were deleted" - a locked image comes + back IDS_IMMUTABLE. Unchecked, the stale image survives and the new + one is stacked on top of it instead of replacing it.""" + from kipy.proto.common.commands.editor_commands_pb2 import ( + DeleteItems, DeleteItemsResponse, ItemDeletionStatus) + ours = [r for r in board.get_reference_images() if r.layer == layer] - if ours: - board.remove_items(ours) - return len(ours) + if not ours: + return 0 + + cmd = DeleteItems() + cmd.header.document.CopyFrom(board._doc) + cmd.item_ids.extend([r.id for r in ours]) + results = board._kicad.send(cmd, DeleteItemsResponse).deleted_items + + stuck = [r for r in results + if r.status not in (ItemDeletionStatus.IDS_OK, + ItemDeletionStatus.IDS_NONEXISTENT)] + if stuck: + locked = sum(1 for r in stuck + if r.status == ItemDeletionStatus.IDS_IMMUTABLE) + raise RuntimeError( + f"{len(stuck)} existing overlay image(s) could not be removed" + + (f" ({locked} locked)" if locked else "") + + " - unlock them in KiCad, or delete them by hand, then run " + "again (a new image would otherwise stack on top).") + return len(results) def push_result_overlays(board: Board, stack, result, lock: bool = False) -> None: """EXPERIMENTAL: the solved |J| of every included copper layer as an unlocked reference image on config.OVERLAY_LAYERS (stackup order, - top first; existing images there are replaced). Editor-only - - reference images never plot. Per-layer failures are reported and - skipped, never fatal to the run.""" + top first; existing images there are replaced, and slots this run + does not write are cleared so no stale heatmap is left behind). + The whole push is one commit, so a single undo reverts it. Editor- + only - reference images never plot. Per-layer failures are reported + and skipped, never fatal to the run.""" from kipy.board_types import ReferenceImage from kipy.geometry import Vector2 @@ -683,23 +722,46 @@ def push_result_overlays(board: Board, stack, result, f"{', '.join(names[len(config.OVERLAY_LAYERS):])} skipped") ny, nx = stack.shape2d w_nm, h_nm = nx * stack.h_nm, ny * stack.h_nm - for src, dest_name in pairs: - try: - dest = layer_from_canonical_name(dest_name) - png = heatmap_png(result.Jmag * 1e-6, names.index(src)) - remove_overlays(board, dest) - ref = ReferenceImage() - ref.layer = dest - ref.position = Vector2.from_xy(round(stack.x0_nm + w_nm / 2), - round(stack.y0_nm + h_nm / 2)) - ref.image_scale = w_nm / (nx * OVERLAY_PIX_NM) - ref.image_data = png - ref.locked = lock - _create_reference_image(board, ref) - print(f"overlay: |J| of {src} -> {dest_name} " - f"({len(png) / 1024:.0f} kB)") - except Exception as e: - print(f"overlay: {src} -> {dest_name} failed: {e}") + + commit = board.begin_commit() if hasattr(board, "begin_commit") else None + done = False + try: + # a narrower run than last time writes fewer slots; whatever the + # zip above left out still holds the previous solve's heatmap and + # would read as current, so clear it + for dest_name in config.OVERLAY_LAYERS[len(pairs):]: + try: + if remove_overlays(board, layer_from_canonical_name(dest_name)): + print(f"overlay: cleared stale {dest_name}") + except Exception as e: + print(f"overlay: clearing stale {dest_name} failed: {e}") + + for src, dest_name in pairs: + try: + dest = layer_from_canonical_name(dest_name) + png = heatmap_png(result.Jmag * 1e-6, names.index(src)) + remove_overlays(board, dest) + ref = ReferenceImage() + ref.layer = dest + ref.position = Vector2.from_xy(round(stack.x0_nm + w_nm / 2), + round(stack.y0_nm + h_nm / 2)) + ref.image_scale = w_nm / (nx * OVERLAY_PIX_NM) + ref.image_data = png + ref.locked = lock + _create_reference_image(board, ref) + print(f"overlay: |J| of {src} -> {dest_name} " + f"({len(png) / 1024:.0f} kB)") + except Exception as e: + print(f"overlay: {src} -> {dest_name} failed: {e}") + if commit is not None: + board.push_commit(commit, "Fill Resistance |J| overlays") + done = True + finally: + if commit is not None and not done: + try: + board.drop_commit(commit) + except Exception: + pass # --- top level ---------------------------------------------------------------- diff --git a/fill_resistance/dialog.py b/fill_resistance/dialog.py index 8d2beba..f37063f 100644 --- a/fill_resistance/dialog.py +++ b/fill_resistance/dialog.py @@ -214,7 +214,11 @@ class _Dialog(QDialog): raise ValueError("Cell size must be > 0 µm.") try: freq = skin.parse_frequency(self.freq_edit.text()) - except ValueError: + except ValueError as exc: + # as in number(): keep parse_frequency's own explanation for + # the inputs it rejects deliberately, not just "unparseable" + if any(k in str(exc) for k in ("separator", "negative")): + raise ValueError(f"Frequency: {exc}") raise ValueError( f"Frequency: cannot parse '{self.freq_edit.text()}' " f"(examples: 0, 142k, 1.5M).") diff --git a/fill_resistance/main.py b/fill_resistance/main.py index 8403086..7df40db 100644 --- a/fill_resistance/main.py +++ b/fill_resistance/main.py @@ -18,9 +18,21 @@ from .errors import CandidateError, UserFacingError def _fail(message: str, outdir) -> None: print(f"ERROR: {message}") - from . import plots - fig = plots.fig_error(message) - plots.save_and_show([(fig, "error")], outdir) + try: + if outdir is None: + # A failure before the run has an output directory (a broken + # plugin environment throws on import) would otherwise save + # no PNG - and with no GUI toolkit, plots falls back to + # opening the saved PNGs, so the figure would never be shown + # either. Exactly the case the docstring promises to cover. + import tempfile + from pathlib import Path + outdir = Path(tempfile.gettempdir()) / "fill-resistance-error" + from . import plots + fig = plots.fig_error(message) + plots.save_and_show([(fig, "error")], outdir) + except Exception: # reporting must not mask the fault + traceback.print_exc() sys.exit(1) diff --git a/tests/test_board_io_api.py b/tests/test_board_io_api.py new file mode 100644 index 0000000..e2c8f06 --- /dev/null +++ b/tests/test_board_io_api.py @@ -0,0 +1,201 @@ +"""board_io's kipy-facing paths, against a fake board. + +Real protobuf messages, a fake transport. These cover what a live KiCad +would otherwise be needed for: the overlay push (kipy's +Board.remove_items discards the DeleteItemsResponse, so board_io talks +to the proto layer directly and these pin the status handling that +depends on) and per-layer pad copper selection. +""" +from types import SimpleNamespace as NS + +import numpy as np +import pytest +from kipy.proto.common.commands.editor_commands_pb2 import ( + CreateItemsResponse, DeleteItemsResponse, ItemDeletionStatus) +from kipy.proto.common.types.base_types_pb2 import KIID +from kipy.util.board_layer import layer_from_canonical_name + +from fill_resistance import board_io, config + + +class _Ref: + """Stand-in for a reference image already on the board (kipy board + items carry a KIID message, not a bare id).""" + def __init__(self, layer_name, ident): + self.layer = layer_from_canonical_name(layer_name) + self.id = KIID(value=f"00000000-0000-0000-0000-{ident:012d}") + + +class _FakeKiCad: + def __init__(self, delete_status=ItemDeletionStatus.IDS_OK): + self.delete_status = delete_status + self.deleted = [] # layers we were asked to clear + self.created = [] # ReferenceImages we were asked to add + + def send(self, cmd, response_type): + if response_type is DeleteItemsResponse: + resp = DeleteItemsResponse() + for _ in cmd.item_ids: + resp.deleted_items.add().status = self.delete_status + self.deleted.append(len(cmd.item_ids)) + return resp + if response_type is CreateItemsResponse: + resp = CreateItemsResponse() + resp.created_items.add().status.code = 1 # ISC_OK + self.created.append(cmd) + return resp + raise AssertionError(f"unexpected command {type(cmd).__name__}") + + +class _FakeBoard: + def __init__(self, existing=(), delete_status=ItemDeletionStatus.IDS_OK): + self._kicad = _FakeKiCad(delete_status) + self._refs = list(existing) + self.commits = [] + self.pushed = [] + self.dropped = [] + + # kipy Board surface board_io actually uses + @property + def _doc(self): + from kipy.proto.common.types.base_types_pb2 import DocumentSpecifier + return DocumentSpecifier() + + def get_reference_images(self): + return list(self._refs) + + def begin_commit(self): + self.commits.append("open") + return object() + + def push_commit(self, commit, message=""): + self.pushed.append(message) + + def drop_commit(self, commit): + self.dropped.append(commit) + + +class _Stack: + layer_names = ["F.Cu", "B.Cu"] + shape2d = (12, 16) + h_nm = 100_000 + x0_nm = 0 + y0_nm = 0 + + +class _Result: + def __init__(self, nlayers=2, ny=12, nx=16): + self.Jmag = np.full((nlayers, ny, nx), 1e6) + + +def test_remove_overlays_counts_deleted(): + layer = layer_from_canonical_name("User.9") + board = _FakeBoard(existing=[_Ref("User.9", 1), _Ref("User.9", 2), + _Ref("User.10", 3)]) + assert board_io.remove_overlays(board, layer) == 2 # not the User.10 one + + +def test_remove_overlays_no_images_is_a_noop(): + board = _FakeBoard() + assert board_io.remove_overlays( + board, layer_from_canonical_name("User.9")) == 0 + assert board._kicad.deleted == [] # no DeleteItems sent at all + + +def test_locked_overlay_raises_instead_of_stacking(): + """A locked image comes back IDS_IMMUTABLE while the overall request + still reports OK. Unchecked, the caller would add a second image on + top of the one it believed it had replaced.""" + board = _FakeBoard(existing=[_Ref("User.9", 1)], + delete_status=ItemDeletionStatus.IDS_IMMUTABLE) + with pytest.raises(RuntimeError, match="could not be removed"): + board_io.remove_overlays(board, layer_from_canonical_name("User.9")) + + +def test_already_gone_overlay_is_not_an_error(): + board = _FakeBoard(existing=[_Ref("User.9", 1)], + delete_status=ItemDeletionStatus.IDS_NONEXISTENT) + assert board_io.remove_overlays( + board, layer_from_canonical_name("User.9")) == 1 + + +def test_push_clears_slots_this_run_does_not_write(monkeypatch): + """A 2-layer run after a 4-layer run must not leave the previous + solve's heatmap sitting on User.11/User.12.""" + stale = [_Ref(n, i) for i, n in enumerate(config.OVERLAY_LAYERS)] + board = _FakeBoard(existing=stale) + board_io.push_result_overlays(board, _Stack(), _Result()) + + written = {c.items[0].type_url for c in board._kicad.created} + assert len(board._kicad.created) == 2 # F.Cu, B.Cu -> 2 slots + assert written # images really created + # 2 written slots cleared + 2 unwritten slots cleared = 4 delete calls + assert len(board._kicad.deleted) == 4 + + +def test_push_is_one_undo_step(): + board = _FakeBoard() + board_io.push_result_overlays(board, _Stack(), _Result()) + assert board.commits and board.pushed and not board.dropped + + +def _square(side): + """Minimal duck-typed PolygonWithHoles: an origin square.""" + pts = [(0, 0), (side, 0), (side, side), (0, side)] + return NS(outline=NS(nodes=[NS(has_point=True, has_arc=False, + point=NS(x=x, y=y)) for x, y in pts]), + holes=[]) + + +class _PadBoard: + """F.Cu carries a small pad, B.Cu a deliberately larger one - KiCad + allows a different pad size per copper layer.""" + def __init__(self): + self.f = layer_from_canonical_name("F.Cu") + self.b = layer_from_canonical_name("B.Cu") + self.asked = [] + + def get_pad_shapes_as_polygons(self, pad, layer): + self.asked.append(layer) + return {self.f: _square(1000), self.b: _square(5000)}.get(layer) + + +def _width(polys): + xs = [p[0] for p in polys[0].outline] + return max(xs) - min(xs) + + +def test_tht_pad_copper_comes_from_the_solder_side(): + """The solder coat is sized from this shape, so a B.Cu-protruding + joint must not be measured with F.Cu's (here smaller) pad.""" + board = _PadBoard() + polys = board_io._pad_polygons(board, pad=None, contact="all", + prefer="B.Cu") + assert _width(polys) == 5000 + assert board.asked[0] == board.b # probed before the F.Cu default + + +def test_pad_copper_falls_back_when_no_side_is_known(): + board = _PadBoard() + polys = board_io._pad_polygons(board, pad=None, contact="all") + assert _width(polys) == 1000 # F.Cu, the documented fallback + + +def test_explicit_contact_layer_still_wins(): + board = _PadBoard() + polys = board_io._pad_polygons(board, pad=None, contact="B.Cu", + prefer="F.Cu") + assert _width(polys) == 5000 + + +def test_push_drops_the_commit_if_it_cannot_finish(monkeypatch): + board = _FakeBoard() + monkeypatch.setattr(board_io.config, "OVERLAY_LAYERS", ("User.9",)) + + def boom(*a, **k): + raise RuntimeError("transport died") + monkeypatch.setattr(board, "push_commit", boom) + + with pytest.raises(RuntimeError): + board_io.push_result_overlays(board, _Stack(), _Result()) + assert board.dropped