Repository navigation
fix(schematic): report each placement's mirror from pin-location tools - #871
Open
pauliuszaleckas wants to merge 1 commit into
Open
pauliuszaleckas wants to merge 1 commit into
pauliuszaleckas wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_schematic_pin_locationsandbatch_get_schematic_pin_locationsreportedeach placement's
rotationbut not its mirror. A mirrored unit and a plain oneat the same rotation could be told apart only by which way their pins pointed.
Both tools now return
mirror_xandmirror_yon the top-level anchor (thelowest unit, like
x,yandrotationthere) and on eachunits[]entry.They are the names
get_schematic_componentalready uses. Pin positions areunchanged.
Closes #855
Approach
This follows the issue's suggested direction.
pin_locations_for_referencealreadyholds each
SymbolInstance, which carries the parsedmirror_x/mirror_ythat
instance.pin_transform()applies to the pins. It now emits thembeside
rotation. The tool descriptions andtool-directory.mdname the newfields.
Architectural fit
Same fields, same names, same source as the sibling readers
(
get_schematic_component,list_schematic_components, andsch_batch.rs). No newhelper. A shared placement-JSON builder across the five emitters was considered
and left out: they read two different models (
cse::Symbol's mirror string andSymbolInstance's bools), and folding them together would touch five publicresponses 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.mdentry sits at the top, above main's entries. Thefile'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_schwas built through Konnect(
create_schematic, fouradd_schematic_componentwithunitandmirror)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:yxA 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 pinposition is where
kicad-cli sch erc --severity-allreports that unconnectedpin. 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 throughMcpHandler::handle_messagetools/call):get_schematic_pin_locations_reports_each_units_mirrormirror_x/mirror_ymatch eeschema's tokens; every pin is at KiCad's ERC positionbatch_get_schematic_pin_locations_reports_each_units_mirrorNegative controls, each applied alone. Count is from
cargo test -p konnect-core --lib --no-fail-fast(1692 tests):falsefalsemirror_x/mirror_yswappedmirror_x/mirror_yswappedEnd to end, the installed build (current
mainplus open PRs) against thishead's debug build over stdio,
batch_get_schematic_pin_locationson thefixture:
false,false/false,false/false,truetrue,false/true,false/false,falseGate on this head:
cargo fmt --all -- --check: cleancargo clippy --workspace --locked --all-targets -- -D warnings: cleancargo test -p konnect-core --locked reliability_contract: 2 passedcargo test --workspace --locked --lib --tests: 2391 passed, 0 failed, 45 ignoredcargo test --workspace --locked --doc: 9 passedcargo xtask fix-doc-counts --check: currentNot run: the live-KiCad suite, since these tools read the file only.
Changed tool behavior
schematicplusreference/references..kicad_sch, read once. The new flags come from the same parsedSymbolInstancethe pin transform uses.mirror_x/mirror_yon the anchor and each unit; every existing field is unchanged.Code review
/simplifychanges, all in the tests:ServerConfig::default(), a constoracle table, one handler per test, and a shorter migration note.
Rejected from
/code-review, all outside this additive change:designator). That is pre-existing for
x/y/rotationtoo, and refusing itwould change the tool's errors.
Referenceproperty rather than the per-instance path in reusedsheets: pre-existing and unchanged here.
(mirror x)at 180° and(mirror y)at0° draw alike. That is how
get_schematic_componentreports them too, androtationis reported raw as well.orientation_degreescan print-0.0, and the tests do not check it: this PRleaves
orientation_degreesalone, and ERC gives no direction oracle.calls.jsonl, andservedrepeats aclosure elsewhere: both match the file's existing served tests.
pinsduplicatesunits[].pins: pre-existing, and unchanged here.kicad-schematicskill namesget_schematic_pin_locationsonlyfor label orientation, which this does not change.
🤖 Generated with Claude Code