fix(add_mounting_hole): set FPID and restrict NPTH pad to mask layers (#154)

`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_<diameter:g>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

@@ -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}",
                 },
             }
 

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"},
+                "position": {
-                "y": {"type": "number", "description": "Y coordinate in millimeters"},
+                    "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,
                 },
-            "required": ["x", "y", "diameter"],
+                "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_<diameter>mm. A non-empty FPID is required for the footprint to be selectable in KiCad's GUI Move tool.",
+                },
+            },
+            "required": ["position", "diameter"],
         },
     },
     {

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()