Skip to content

fix(library): support oval drills in footprint tools - #621

Open
korkin25 wants to merge 1 commit into
mixelpixx:mainfrom
korkin25:fix/oval-footprint-drills
Open

korkin25 wants to merge 1 commit into
mixelpixx:mainfrom
korkin25:fix/oval-footprint-drills

Conversation

@korkin25

@korkin25 korkin25 commented Sep 17, 2026

Copy link
Copy Markdown

Summary

Part of #296.

create_footprint and edit_footprint_pad can only describe a circular hole:
drill is a scalar diameter. KiCad supports (drill oval width height), so
plated slots — connector shields, USB-C mechanical pads, keyed headers — cannot
be authored through the library tools today. Saved oval drills are already
readable through get_footprint_info(include_pads=true); this PR adds the
matching write path so a slot can be created and edited, not only inspected.

Approach

Extend the existing drill argument instead of adding a tool: it accepts either
the current positive scalar diameter or an explicit object,
{"shape":"oval","width":0.6,"height":1.6}, in pad-local millimetres. Pad
rotation rotates the hole, so no separate drill-rotation concept is introduced.
One helper (footprint_drill_sexp) owns schema, validation and serialization for
both tools, so creation and editing cannot drift apart.

Editing now replaces the complete direct-child (drill …) block found by the
S-expression parser rather than truncating at the first ). A drill that carries
a nested (offset …) is refused with an explicit error instead of silently
dropping the offset; creating or clearing offsets stays out of scope and is
tracked as the separate follow-up under #296.

Architectural fit

Extends existing library tools and the shared KiCad S-expression handling. No new
tool, transport, service, dependency, or workaround — tool names and counts are
unchanged, so no registry or doc-count update applies.

Branch and dependencies

  • Base branch: main at 9d33cf0c239257f41a178cd26a2513d6e568d1c0.
  • Depends on: nothing; no series position.
  • Unique commits owned by this PR: one, db08e2670eb894206532a4d3994464b9ca03a092.
  • Acceptance criteria owned: oval drill schema, shared validation, serialization
    and generic regression coverage for create_footprint / edit_footprint_pad.
  • Next PR to promote: none.

This does not close #296; the remaining generation controls stay outside its scope.

Compatibility and safety

Public schema change, backward compatible: drill becomes a oneOf of the
existing positive number and the new closed oval object. Every call that works
today keeps working and keeps serializing as (drill 0.8); no migration needed.

The oval object accepts exactly shape, width, height. Dimensions must be
numeric, finite, strictly positive, and fit inside the pad size. A drill is only
accepted on thru_hole / np_thru_hole pads. Validation runs for every pad
before anything is written, so an invalid request never leaves a partially
written footprint — a rejected create_footprint does not create the file at
all, and a rejected edit_footprint_pad leaves the target byte-identical.
Unrelated footprint content is preserved, verified against a KiCad-authored
fixture already in the repository. No IPC or live-board path is touched.

Validation

Changed tool behavior

Behavior Contract and evidence for this change
Accepted inputs and declared defaults Positive scalar diameter (unchanged) or {shape:"oval",width,height} in pad-local mm; omitted drill keeps current behavior. Schema asserted against the live tool definitions and validated with jsonschema.
Invalid/unsupported inputs and structured errors Wrong type, zero/negative/non-finite dimensions, missing or extra keys, non-oval shape, holes larger than the pad, and drills on SMD pads are refused with invalid_argument naming the field (pads[i].drill).
Target, data source and prerequisite state Explicit library footprint path, saved-file operations only. Readback in the tests uses get_footprint_info(include_pads=true), source=file. No live PCB/IPC state required or claimed.
Observed changes and preserved unrelated objects Round-trip test asserts (drill oval 0.6 1.6), (drill 0.8), pad rotation (at 0 0 90) and layers; typed readback reports shape:"oval", size:{x:0.6,y:1.6}. Editing a KiCad-authored fixture changes exactly one substring and nothing else.
Failure before/after mutation, including applied work All validation precedes the write. Invalid-payload test asserts unchanged file bytes for edits and that no file is created for refused creations.
Recovery from partial/uncertain results without repeating applied work A refusal proves nothing was applied, so the caller can correct the request and retry. Editing a drill that has an (offset …) fails explicitly rather than producing an uncertain result.

Commands run locally on this exact head (db08e267), Linux, Rust 1.96.0:

  • cargo fmt --all -- --check — clean.
  • cargo test --workspace --locked --lib --tests (what CI runs) — 1910 passed, 0 failed, 31 ignored (pre-existing environment-dependent tests).
  • cargo test --workspace --locked --doc — 8 passed, 0 failed, 2 ignored.
  • cargo clippy --workspace --locked --all-targets -- -D warnings — clean, no warnings.
  • cargo test -p konnect-core --locked oval_drill — five focused tests: scalar/oval schema acceptance, create+edit+typed readback round-trip with rotation, invalid payloads leaving files unchanged, offset-drill refusal, and preservation of a KiCad-authored footprint.
  • Relevant viewer, plugin, packaging checks — covered by hosted CI: all ten checks passed on this head, including Check & Test on ubuntu/macOS/Windows, Schematic viewer, Plugin (Python), PCM packaging validation, Nix flake and Dependency licenses.
  • Real-KiCad observation — not run. No KiCad GUI, macOS/Windows host, or live IPC session is available in my environment. This change only writes saved library files, and the tests exercise a KiCad-authored fixture already in the repository; an independent GUI load of a generated slot would still be useful confirmation if a maintainer or the original reporter can run one.

Limitations: creating, editing or clearing a drill (offset …) remains
unsupported — an existing offset is preserved on unrelated edits and refused on a
drill edit. Arbitrary non-oval cutouts stay out of scope and belong to the
Edge.Cuts work.

Review checklist

  • The diff is focused and contains no generated output, personal data, or unrelated cleanup — one file, crates/konnect-core/src/tools/library.rs.
  • The branch includes current upstream/main, has no merge conflicts, and CI passed on this exact head — all ten hosted checks green on db08e267 (run 35266667047), including Check & Test on ubuntu, macOS and Windows.
  • The branch was based on latest upstream/main, not a release tag.
  • The PR shows only its unique commit and diff; there are no dependencies.
  • Every review conversation is resolved — none open yet.
  • New names follow docs/NAMING_CONVENTIONS.md; the drill field keeps its name and gains a compatible alternative form, reusing the existing shape/width/height vocabulary.
  • New behavior and failure paths have regression coverage.
  • File mutations are atomic and preserve unrelated content.
  • IPC mutations verify the requested board — N/A, this change does not touch the IPC path.
  • If tools were added/removed — N/A, no tool added or removed; counts unchanged.

Marked ready for review after all ten hosted checks passed on this exact head.
That is the author's readiness only — review and the status:* workflow label
remain the maintainer's decision. No merge or auto-merge is requested, and no
status:ready-to-merge is claimed.

🤖 Generated with Claude Code

@korkin25
korkin25 marked this pull request as ready for review September 17, 2026 19:52
@korkin25
korkin25 requested a review from mixelpixx as a code owner September 17, 2026 19:52
@neusse

neusse commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Please correct these items before final review:

  • Remove the accidental closing reference to tracker Advanced symbol and footprint generation controls #296. The phrase “does not close Advanced symbol and footprint generation controls #296” is still parsed by GitHub as a closing keyword; closingIssuesReferences currently contains Advanced symbol and footprint generation controls #296. Rephrase it as “leaves Advanced symbol and footprint generation controls #296 open,” then verify the closing reference is gone.
  • Attribute drill failures to drill, not shape. handle_edit_footprint_pad currently maps every block-edit error to invalid_library_argument("shape", ...), so oversized, SMD, and offset-drill refusals name the wrong field.
  • Add served tools/call coverage for legacy scalar and oval-object acceptance plus malformed/semantic refusal with no write. Direct handler/helper tests do not satisfy the reliability contract by themselves.
  • Correct the compatibility claim. This head newly rejects legacy scalar drills on SMD pads, non-positive drills, and drills larger than the pad, while the previous handler serialized any number. Either preserve the prior scalar contract outside the new oval form or explicitly document and test this safety-driven narrowing in docs/API_MIGRATIONS.md.
  • Record this focused child in Advanced symbol and footprint generation controls #296's accounting, including the still-open offset-drill follow-up and the remaining tracker items. This PR is partial and must not close the tracker.

The missing independent KiCad load is explicit validation debt rather than a merge blocker for this focused saved-library change: hosted CI is green, the syntax/readback tests use a KiCad-authored fixture, and no live board is mutated. The items above are deterministic blockers on the current head.

@neusse neusse added enhancement New feature or request P2 Wanted, not urgent area:pcb Board editing, export, manufacturing status:waiting-on-author Next actor: the PR author — one checklist, 14-day target labels Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:pcb Board editing, export, manufacturing enhancement New feature or request P2 Wanted, not urgent status:waiting-on-author Next actor: the PR author — one checklist, 14-day target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Advanced symbol and footprint generation controls

2 participants