From 4740013d24d8b94892797b23d6be687a18237982 Mon Sep 17 00:00:00 2001 From: Matthew Runo <74583+inktomi@users.noreply.github.com> Date: Mon, 18 May 2026 11:05:10 -0700 Subject: [PATCH] fix(add_mounting_hole): set FPID and restrict NPTH pad to mask layers (#154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BoardOutlineCommands.add_mounting_hole` produced footprints with an empty library:name id (`(footprint "" ...)` in the .kicad_pcb), which KiCad's GUI Move tool refuses to select — users couldn't drag the resulting MHs in the editor. It also emitted the pad with the default `*.Cu` + `*.Mask` LSET on NPTHs; with `padDiameter > diameter` that creates phantom copper annular rings on every Cu layer and trips clearance DRC against neighbouring nets. Repro: call `add_mounting_hole` with `position={x:117,y:84.5,unit:"mm"}, diameter:3.2, padDiameter:3.5`. The resulting MH is unselectable and DRC reports phantom F.Cu pad shorts to neighbouring component pads. Changes: - Set a real FPID via `module.SetFPID(pcbnew.LIB_ID(lib, name))`. Default is `MountingHole:MountingHole_mm` (e.g. 3.2 → 3.2mm); a new optional `footprintLibId` parameter overrides. - For NPTH (the default `plated:false`), restrict the pad's LSET to `F.Mask` + `B.Mask` only. PTH path is unchanged — the default Cu+Mask LSET is correct there. - Update the schema in `tool_schemas.py`: previously advertised `x`/`y`/`diameter` at the top level, but the impl reads `position={x,y,unit}`, `padDiameter`, `plated`. Schema now matches the implementation and exposes the new `footprintLibId` param. - New `tests/test_add_mounting_hole.py` regression suite (7 tests) asserting SetFPID is invoked with non-empty lib:name (default + override forms), NPTH SetLayerSet excludes any Cu layer, and PTH does not call SetLayerSet (preserves default Cu+Mask). --- python/commands/board/outline.py | 27 +++++ python/schemas/tool_schemas.py | 37 +++++- tests/test_add_mounting_hole.py | 190 +++++++++++++++++++++++++++++++ 3 files changed, 249 insertions(+), 5 deletions(-) create mode 100644 tests/test_add_mounting_hole.py diff --git a/python/commands/board/outline.py b/python/commands/board/outline.py index c0e9642..db2e138 100644 --- a/python/commands/board/outline.py +++ b/python/commands/board/outline.py @@ -216,6 +216,7 @@ class BoardOutlineCommands: diameter = params.get("diameter") pad_diameter = params.get("padDiameter") plated = params.get("plated", False) + footprint_lib_id = params.get("footprintLibId") if not position or not diameter: return { @@ -247,6 +248,19 @@ class BoardOutlineCommands: module.SetReference(f"MH{next_num}") module.SetValue(f"MountingHole_{diameter}mm") + # Set a real library:name FPID. Without this, the footprint is + # written as `(footprint "" ...)` and KiCad's GUI Move tool refuses + # to select it (no library link → not draggable in the editor). + if not footprint_lib_id: + # Strip trailing zeros so 3.2 → "3.2" not "3.20" + footprint_lib_id = f"MountingHole:MountingHole_{diameter:g}mm" + if ":" in footprint_lib_id: + lib_name, fp_name = footprint_lib_id.split(":", 1) + else: + lib_name = "MountingHole" + fp_name = footprint_lib_id + module.SetFPID(pcbnew.LIB_ID(lib_name, fp_name)) + # Create the pad for the hole pad = pcbnew.PAD(module) pad.SetNumber(1) @@ -255,6 +269,18 @@ class BoardOutlineCommands: pad.SetSize(pcbnew.VECTOR2I(pad_diameter_nm, pad_diameter_nm)) pad.SetDrillSize(pcbnew.VECTOR2I(diameter_nm, diameter_nm)) pad.SetPosition(pcbnew.VECTOR2I(0, 0)) # Position relative to module + + if not plated: + # NPTH must not include *.Cu in pad layers. The default LSET + # for a circular pad is *.Cu + *.Mask; on a NPTH with + # padDiameter > diameter that produces phantom copper annular + # rings on every Cu layer, which trip clearance DRC against + # neighbouring nets. + mask_only = pcbnew.LSET() + mask_only.AddLayer(pcbnew.F_Mask) + mask_only.AddLayer(pcbnew.B_Mask) + pad.SetLayerSet(mask_only) + module.Add(pad) # Position the mounting hole @@ -271,6 +297,7 @@ class BoardOutlineCommands: "diameter": diameter, "padDiameter": pad_diameter or diameter + 1, "plated": plated, + "footprintLibId": f"{lib_name}:{fp_name}", }, } diff --git a/python/schemas/tool_schemas.py b/python/schemas/tool_schemas.py index 4049359..8e1675a 100644 --- a/python/schemas/tool_schemas.py +++ b/python/schemas/tool_schemas.py @@ -266,19 +266,46 @@ BOARD_TOOLS = [ { "name": "add_mounting_hole", "title": "Add Mounting Hole", - "description": "Adds a mounting hole (non-plated through hole) at the specified position with given diameter.", + "description": "Adds a mounting hole at the specified position with given diameter. Defaults to non-plated (NPTH) with mask-only pad layers; set plated=true for a PTH with copper pad.", "inputSchema": { "type": "object", "properties": { - "x": {"type": "number", "description": "X coordinate in millimeters"}, - "y": {"type": "number", "description": "Y coordinate in millimeters"}, + "position": { + "type": "object", + "description": "Position of the mounting hole", + "properties": { + "x": {"type": "number", "description": "X coordinate"}, + "y": {"type": "number", "description": "Y coordinate"}, + "unit": { + "type": "string", + "enum": ["mm", "inch"], + "default": "mm", + "description": "Unit for x/y (default mm)", + }, + }, + "required": ["x", "y"], + }, "diameter": { "type": "number", - "description": "Hole diameter in millimeters", + "description": "Hole (drill) diameter in millimeters", "minimum": 0.1, }, + "padDiameter": { + "type": "number", + "description": "Pad diameter in millimeters (defaults to diameter + 1mm). For NPTH this only affects the solder-mask opening, not copper.", + "minimum": 0.1, + }, + "plated": { + "type": "boolean", + "default": False, + "description": "True for plated through-hole (PTH) with copper pad; false (default) for NPTH (mask only).", + }, + "footprintLibId": { + "type": "string", + "description": "Optional library:name FPID (e.g. 'MountingHole:MountingHole_3.2mm'). Defaults to MountingHole:MountingHole_mm. A non-empty FPID is required for the footprint to be selectable in KiCad's GUI Move tool.", + }, }, - "required": ["x", "y", "diameter"], + "required": ["position", "diameter"], }, }, { diff --git a/tests/test_add_mounting_hole.py b/tests/test_add_mounting_hole.py new file mode 100644 index 0000000..6bbcaa9 --- /dev/null +++ b/tests/test_add_mounting_hole.py @@ -0,0 +1,190 @@ +""" +Regression tests for BoardOutlineCommands.add_mounting_hole. + +Covers two prior bugs: + +1. Empty FPID + The footprint was created with no library:name id, producing + `(footprint "" ...)` in the .kicad_pcb. KiCad's GUI Move tool refuses to + select footprints with no library link, so users couldn't drag the + resulting MHs in the editor. + +2. NPTH pad on copper layers + The pad was emitted with the default LSET (`*.Cu` + `*.Mask`) even when + `plated:false`. With `padDiameter > diameter` that produces phantom + copper annular rings on every Cu layer, which trigger DRC clearance + errors against neighbouring nets. +""" + +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent / "python")) + +import pcbnew # noqa: E402 — pcbnew is stubbed by conftest + + +@pytest.fixture +def fresh_pcbnew_mock(monkeypatch): + """ + The conftest pcbnew is a long-lived MagicMock. Reset its call history + before each test so we can make precise assertions about what + add_mounting_hole calls on the pcbnew API. + """ + pcbnew.reset_mock() + # PAD_ATTRIB constants must compare unequal so the conditional in the + # implementation picks the right branch. + pcbnew.PAD_ATTRIB_NPTH = "NPTH" + pcbnew.PAD_ATTRIB_PTH = "PTH" + pcbnew.PAD_SHAPE_CIRCLE = "circle" + pcbnew.F_Mask = "F.Mask" + pcbnew.B_Mask = "B.Mask" + return pcbnew + + +@pytest.fixture +def cmds(fresh_pcbnew_mock): + from commands.board.outline import BoardOutlineCommands + + board = MagicMock(name="board") + board.GetFootprints.return_value = [] # no existing MHs + return BoardOutlineCommands(board=board) + + +def _captured_module(pcbnew_mock): + """Return the FOOTPRINT mock instance created by the call under test.""" + return pcbnew_mock.FOOTPRINT.return_value + + +def _captured_pad(pcbnew_mock): + """Return the PAD mock instance created by the call under test.""" + return pcbnew_mock.PAD.return_value + + +# --------------------------------------------------------------------------- +# Bug #1: empty FPID +# --------------------------------------------------------------------------- + + +class TestFootprintLibIdSet: + def test_default_fpid_uses_diameter(self, cmds, fresh_pcbnew_mock): + result = cmds.add_mounting_hole( + { + "position": {"x": 117, "y": 84.5, "unit": "mm"}, + "diameter": 3.2, + "padDiameter": 3.5, + } + ) + + assert result["success"] is True + + # LIB_ID was constructed with a non-empty library and footprint name + fresh_pcbnew_mock.LIB_ID.assert_called_once_with("MountingHole", "MountingHole_3.2mm") + # And the FOOTPRINT had its FPID set + _captured_module(fresh_pcbnew_mock).SetFPID.assert_called_once_with( + fresh_pcbnew_mock.LIB_ID.return_value + ) + # The response surfaces the lib id used + assert result["mountingHole"]["footprintLibId"] == "MountingHole:MountingHole_3.2mm" + + def test_default_fpid_strips_trailing_zeros(self, cmds, fresh_pcbnew_mock): + cmds.add_mounting_hole( + { + "position": {"x": 0, "y": 0, "unit": "mm"}, + "diameter": 3.0, # would become "3.0mm" with %f, "3" with %g + } + ) + + # %g formatting: 3.0 → "3" + fresh_pcbnew_mock.LIB_ID.assert_called_once_with("MountingHole", "MountingHole_3mm") + + def test_explicit_fpid_override(self, cmds, fresh_pcbnew_mock): + cmds.add_mounting_hole( + { + "position": {"x": 50, "y": 50, "unit": "mm"}, + "diameter": 3.2, + "footprintLibId": "MountingHole:MountingHole_3.2mm_M3", + } + ) + + fresh_pcbnew_mock.LIB_ID.assert_called_once_with("MountingHole", "MountingHole_3.2mm_M3") + + def test_explicit_fpid_without_colon_falls_back_to_mountinghole_lib( + self, cmds, fresh_pcbnew_mock + ): + cmds.add_mounting_hole( + { + "position": {"x": 0, "y": 0, "unit": "mm"}, + "diameter": 2.5, + "footprintLibId": "MyCustomHole", + } + ) + + fresh_pcbnew_mock.LIB_ID.assert_called_once_with("MountingHole", "MyCustomHole") + + +# --------------------------------------------------------------------------- +# Bug #2: NPTH pad layers +# --------------------------------------------------------------------------- + + +class TestNpthPadLayers: + def test_npth_pad_layers_are_mask_only(self, cmds, fresh_pcbnew_mock): + cmds.add_mounting_hole( + { + "position": {"x": 117, "y": 84.5, "unit": "mm"}, + "diameter": 3.2, + "padDiameter": 3.5, + "plated": False, + } + ) + + pad = _captured_pad(fresh_pcbnew_mock) + + # The pad must have been set to NPTH attr + pad.SetAttribute.assert_called_once_with("NPTH") + + # SetLayerSet was called exactly once with an LSET that has + # F_Mask and B_Mask added — and nothing on Cu layers. + pad.SetLayerSet.assert_called_once() + lset_arg = pad.SetLayerSet.call_args.args[0] + + added_layers = [c.args[0] for c in lset_arg.AddLayer.call_args_list] + assert "F.Mask" in added_layers + assert "B.Mask" in added_layers + assert all( + "Cu" not in str(layer) for layer in added_layers + ), f"NPTH pad must not include any Cu layers, got: {added_layers}" + + def test_npth_is_default(self, cmds, fresh_pcbnew_mock): + # Omit `plated` entirely; default must be NPTH. + cmds.add_mounting_hole( + { + "position": {"x": 0, "y": 0, "unit": "mm"}, + "diameter": 3.2, + } + ) + + pad = _captured_pad(fresh_pcbnew_mock) + pad.SetAttribute.assert_called_once_with("NPTH") + pad.SetLayerSet.assert_called_once() + + def test_pth_keeps_default_layers(self, cmds, fresh_pcbnew_mock): + cmds.add_mounting_hole( + { + "position": {"x": 0, "y": 0, "unit": "mm"}, + "diameter": 3.2, + "padDiameter": 3.5, + "plated": True, + } + ) + + pad = _captured_pad(fresh_pcbnew_mock) + pad.SetAttribute.assert_called_once_with("PTH") + + # For PTH, the default LSET (*.Cu + *.Mask) is correct, so we must + # NOT override it via SetLayerSet. + pad.SetLayerSet.assert_not_called()