Skip to content

fix(schematic): report each placement's mirror from pin-location tools - #871

Open
pauliuszaleckas wants to merge 1 commit into
mixelpixx:mainfrom
pauliuszaleckas:fix/855-pin-location-mirror
Open

pauliuszaleckas wants to merge 1 commit into
mixelpixx:mainfrom
pauliuszaleckas:fix/855-pin-location-mirror

Conversation

@pauliuszaleckas

Copy link
Copy Markdown
Contributor

Summary

get_schematic_pin_locations and batch_get_schematic_pin_locations reported
each placement's rotation but not its mirror. A mirrored unit and a plain one
at the same rotation could be told apart only by which way their pins pointed.

Both tools now return mirror_x and mirror_y on the top-level anchor (the
lowest unit, like x, y and rotation there) and on each units[] entry.
They are the names get_schematic_component already uses. Pin positions are
unchanged.

Closes #855

Approach

This follows the issue's suggested direction. pin_locations_for_reference already
holds each SymbolInstance, which carries the parsed mirror_x/mirror_y
that instance.pin_transform() applies to the pins. It now emits them
beside rotation. The tool descriptions and tool-directory.md name the new
fields.

Architectural fit

Same fields, same names, same source as the sibling readers
(get_schematic_component, list_schematic_components, and sch_batch.rs). No new
helper. A shared placement-JSON builder across the five emitters was considered
and left out: they read two different models (cse::Symbol's mirror string and
SymbolInstance's bools), and folding them together would touch five public
responses for an additive fix.

Branch and dependencies

Base: main @ 9488e5f. One commit. No dependencies.

Compatibility and safety

Additive response fields only; no argument, error or write-path change. The new
docs/API_MIGRATIONS.md entry sits at the top, above main's entries. The
file's diff against main only adds lines.

Risk: a client that validates the response against a closed schema would see
two new keys. Rollback: revert the commit.

Validation

Head 135f7de05a9a45a153a2733594729135f74b3483.

Fixture. pin_location_mirror_kicad10.kicad_sch was built through Konnect
(create_schematic, four add_schematic_component with unit and mirror)
against stock Amplifier_Operational:LM358, then force-resaved by KiCad 10.0.6
(kicad-cli sch upgrade --force). It places two LM358s whose units disagree:

Ref Unit Mirror
U1 1 —
U1 2 y
U2 1 x
U2 2 —

A tool that copies the anchor's mirror onto every unit, or reports none, fails
on one unit of each.

Oracle. The flags are the (mirror …) tokens eeschema wrote. Each pin
position is where kicad-cli sch erc --severity-all reports that unconnected
pin. All 12 pins are tabled in the fixture README and restated as literals in
the test.

Tests (sch_components.rs::pin_location_mirror_tests, both through
McpHandler::handle_message tools/call):

Test Proves
get_schematic_pin_locations_reports_each_units_mirror anchor and per-unit mirror_x/mirror_y match eeschema's tokens; every pin is at KiCad's ERC position
batch_get_schematic_pin_locations_reports_each_units_mirror the same for both references in one batch call

Negative controls, each applied alone. Count is from cargo test -p konnect-core --lib --no-fail-fast (1692 tests):

Control Fails
Unit flags forced to false 2 (both tests)
Anchor flags forced to false 2
Units report the anchor's mirror 2
Unit mirror_x/mirror_y swapped 2
Anchor mirror_x/mirror_y swapped 2

End to end, the installed build (current main plus open PRs) against this
head's debug build over stdio, batch_get_schematic_pin_locations on the
fixture:

Installed This head
U1 anchor / unit 1 / unit 2 no mirror keys false,false / false,false / false,true
U2 anchor / unit 1 / unit 2 no mirror keys true,false / true,false / false,false

Gate on this head:

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --locked --all-targets -- -D warnings: clean
  • cargo test -p konnect-core --locked reliability_contract: 2 passed
  • cargo test --workspace --locked --lib --tests: 2391 passed, 0 failed, 45 ignored
  • cargo test --workspace --locked --doc: 9 passed
  • cargo xtask fix-doc-counts --check: current

Not run: the live-KiCad suite, since these tools read the file only.

Changed tool behavior

Behavior Contract and evidence for this change
Accepted inputs and declared defaults Unchanged: schematic plus reference / references.
Invalid/unsupported inputs and structured errors Unchanged.
Target, data source and prerequisite state Unchanged: the saved .kicad_sch, read once. The new flags come from the same parsed SymbolInstance the pin transform uses.
Observed changes and preserved unrelated objects N/A: read-only tools. The response gains mirror_x/mirror_y on the anchor and each unit; every existing field is unchanged.
Failure before/after mutation, including applied work N/A: read-only, no mutation.
Recovery from partial/uncertain results without repeating applied work N/A: read-only, safe to retry.

Code review

/simplify changes, all in the tests: ServerConfig::default(), a const
oracle table, one handler per test, and a shorter migration note.

Rejected from /code-review, all outside this additive change:

  • The anchor is arbitrary when two placements share a unit number (a duplicated
    designator). That is pre-existing for x/y/rotation too, and refusing it
    would change the tool's errors.
  • Lookup by the Reference property rather than the per-instance path in reused
    sheets: pre-existing and unchanged here.
  • The flags are KiCad's raw tokens, so (mirror x) at 180° and (mirror y) at
    0° draw alike. That is how get_schematic_component reports them too, and
    rotation is reported raw as well.
  • orientation_degrees can print -0.0, and the tests do not check it: this PR
    leaves orientation_degrees alone, and ERC gives no direction oracle.
  • The served tests log to the user's calls.jsonl, and served repeats a
    closure elsewhere: both match the file's existing served tests.
  • Batch re-extracts instances per reference, and flat pins duplicates
    units[].pins: pre-existing, and unchanged here.
  • The bundled kicad-schematic skill names get_schematic_pin_locations only
    for label orientation, which this does not change.

🤖 Generated with Claude Code

get_schematic_pin_locations and its batch form reported rotation but not
mirror, so a mirrored unit looked like a plain one except for which way its
pins pointed. Add mirror_x and mirror_y to the anchor and each unit, named as
get_schematic_component names them.

Closes mixelpixx#855

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pin-location tools don't report whether a symbol is mirrored

1 participant