Repository navigation
feat(schematic): place add_power_symbol on a named pin - #782
pauliuszaleckas wants to merge 1 commit into
Conversation
Copying a pin's endpoint into x/y and picking a rotation by hand is where mistakes slip in (mixelpixx#727). add_power_symbol now also takes reference + pin_number: the symbol lands on the pin's endpoint, unsnapped, and faces away from the body unless rotation is given. Mixed or half-given selectors, missing pins and pins two symbols share are refused before writing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3882e3e to
f6cbf63
Compare
neusse
left a comment
There was a problem hiding this comment.
Substantive exact-head review: f6cbf63. Spec: no findings against the narrowed #727 acceptance: alternative pin selector, no-write mixed/incomplete/missing/ambiguous refusal, exact pin endpoint, derived omitted rotation, explicit rotation and legacy coordinate behavior preserved; stubs/batches deferred. Standards: no blocking findings; shared pin geometry, revision-checked single-source edit, observed readback, migration/tool/guidance updates and KiCad-authored served-call fixtures are coherent. The PR records real KiCad netlist/ERC/SVG observations and guard negative controls; I reviewed those records, not a fresh live run. One unique commit, Closes #727 registered, no unresolved threads, all ten checks green on this head, but it does not include current main 9488e5f. Next actor: maintainer integration/current-main readiness verification when its schematic lane reaches the front, followed by exact-head user authorization because this is a contributor PR. No deep-queue branch refresh was performed. Keep status:waiting-on-review specifically for that integration/readiness step; do not ask the author for a routine base refresh.
Summary
add_power_symbolonly tookx/y. To put a rail on a pin, a caller had to copy the pin's endpoint and choose the rotation by hand. It now also takesreference+pin_number, the wayconnect_to_netdoes. The symbol lands on the pin's endpoint and, whenrotationis omitted, faces away from the component body.The scope is the narrowed contract from the maintainer's comment.
stub_lengthand a batch form are left for follow-ups.Closes #727
Approach
Selector. The call gives exactly one of
reference+pin_numberorx+y. Mixing them, or giving half of one, is refused withinvalid_argumentnaming the field, before the file is read.Pin lookup (
resolve_power_pin, built on the sharedplaced_pins_by_reference). It is stricter thanresolve_placed_pin, which takes the first match. The following are refused, and nothing is written:stale_target, with the reason saying which of the two is missing.ambiguous_target, with candidates named by unit, UUID and position.Stacked drawings of one pin within one placed unit count as a single point. KiCad's De Morgan symbols draw every pin twice.
Position. The symbol is placed so that its own pin lands on the target pin's endpoint. That point is not snapped, so an off-grid pin is still reached. Coordinate calls still snap.
Rotation. The new
konnect_sexp::schematic::rotation_facingturns the power symbol so its body side, which is its pin's own angle, faces the target pin's outward direction. An explicitrotationstill wins.Single read. The pin lookup and the edit read the file once, so the pin cannot come from a different revision than the one the write is checked against.
Departures from the issue's suggested direction: none in scope. I used
stale_targetfor a reference or pin that isn't on the sheet. That followscomponent_target_from_source("component X is not present") anddelete_trace, which report a missing target the same way.Architectural fit
The change reuses
placed_pins_by_reference,pin_endpoint,pin_outward_direction, the placement readback and the existing error kinds. The rotation rule lives inkonnect-sexpnext topin_outward_direction, andcse::library::embedded_lib_symbolis nowpubinstead of being copied.resolve_power_pinsits beside the first-matchresolve_placed_pinthatconnect_to_net,connect_pinsand their batch forms still use. I filed that as #781, with a reproduction on this PR's fixture:connect_to_neton a duplicatedR4wires the first copy and reports success. Switching those tools to the strict resolver changes their results, so it belongs in its own PR.Branch and dependencies
main@ bf7bfd6. No dependencies; one commit.Compatibility and safety
xandyare no longer listed as required.referenceandpin_numberare added.rotationdefault of0is removed, because it now applies only to coordinates.docs/API_MIGRATIONS.mdhas a new entry at the top. Nothing else in the file moved.overwrite()as before. Every refusal above happens before the write.add_power_symboldoesn't check what net a pin is already on. The coordinate form never did either. A GND named on a pin that is already +3V3 merges the two, just as a GND placed at that point always has. Refusing that would be new behaviour, outside this scope.Validation
Fixtures and oracles
crates/konnect-core/tests/fixtures/power_on_pin_kicad10.README.mdhas the full tables.power_on_pin_kicad10.kicad_schDevice,Amplifier_Operational,74xxandpower, then force-resaved by KiCad 10.0.6.R4.+3V3= R1.1, U1.1, U1.3, U1.8 andGND= R1.2, R2.1, R2.2, R3.1, U1.4, U2.3. R3.2 stays unconnected, which a mirror-blind implementation would get wrong.kicad-cli sch export svgshows every rail facing away from its symbol.power_on_off_grid_pin.kicad_schhv_converterdemo, byte for byte.(149.352, 84.328), off-grid on both axes./d4intoGNDin KiCad's netlist. Placed by coordinates at the same point, it snaps to(149.86, 83.82)and/d4is unchanged.powersymbols have their pin at the origin, angled into the body: GND270with its body below, +3V390with its body above.rotationstill wins.End to end, same call over stdio
add_power_symbol {power_net: GND, reference: R2, pin_number: 2}on the fixture:invalid_argumentonx("missing"), file unchanged#PWR05at(134.62, 80.01), rotation90, R2.2 onGNDin KiCad's netlistTests
All new tests go through the served
tools/calldispatch, except the one that has to get past the schema gate. Every refusal also asserts the file is byte-identical afterwards.a_named_pin_takes_the_symbol_to_kicads_endpoint_facing_awayan_explicit_rotation_wins_over_the_pinrotation: 180on a right-pointing pin lands at 180coordinates_still_snap_and_default_to_no_rotationx/yunchanged: snapped, rotation 0an_off_grid_pin_is_reached_where_it_is(149.352, 84.328), unsnappedbad_selectors_are_refused_without_writinginvalid_argumentand field. R99 and R1 pin 9:stale_target, target and reasona_pin_two_symbols_share_is_ambiguous_and_nothing_is_writtenambiguous_target,R4 pin 1, both candidates at KiCad's positionsa_copy_without_a_definition_is_not_skippedlib_idrenamed in memory: refused, not placed on the other copya_malformed_selector_keeps_its_own_reasonpin_number: 1: the helper's own reason, not "give either…"rotation_facing_turns_the_body_side_onto_the_direction(konnect-sexp)transform_directionNegative controls
Each guard was neutered on 3882e3e, and
cargo test -p konnect-core -p konnect-sexp --lib --no-fail-fastwas run. The file was then restored and checked withcmp.bad_selectors_are_refused_without_writinga_pin_two_symbols_share_is_ambiguous_and_nothing_is_writtena_named_pin_takes…,an_off_grid_pin…an_explicit_rotation_wins_over_the_pinrotation_facingsign flippedrotation_facing_turns…,a_named_pin_takes…,an_off_grid_pin…an_off_grid_pin_is_reached_where_it_isa_copy_without_a_definition_is_not_skippeda_named_pin_takes…(U2.3, De Morgan)bad_selectors_are_refused_without_writing(reason)a_malformed_selector_keeps_its_own_reasonC11 fails nothing, and on purpose. All 101 stock power symbols have their pin at the origin, so no KiCad-authored fixture can tell the difference. The subtraction stays because
power:resolves through the user's library table, and an overriding library can put the pin elsewhere. Without it, that symbol would miss the pin.Changed tool behavior
reference+pin_number(strings) orx+y.rotationis optional: when omitted it is derived from the pin for the pin form and is 0 for coordinates. Tests:a_named_pin_takes…,an_explicit_rotation_wins…,coordinates_still_snap…invalid_argumentnaming the field. A malformed present value keeps the helper's reason. A power symbol without exactly one pin:invalid_argumentonpower_net. Tests:bad_selectors…,a_malformed_selector….kicad_sch, read once, and the pin resolved from that same read. Missing reference or pin:stale_target. A pin on more than one placed symbol:ambiguous_targetwith candidates. A copy with no definition:stale_target. Existing sheet-instance preflight unchangedplaced_component_readback), and tests compare the committed symbol's point and rotationplaced_component_readbackchecks the committed file and returns its structured error when the placement does not matchx+yfor the candidate they mean. A post-write readback error is handled as before: reload and inspect the file before retryingGate (local, on 3882e3e, Linux)
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: 2306 passed, 0 failedcargo test --workspace --locked --doc: 9 passedcargo xtask fix-doc-counts --check: current, with no tool added or removedRisk and rollback
The risk is confined to the new pin form: coordinate calls go through the same snap, readback and write as before. To roll back, revert the one commit. Documents written with the pin form are ordinary KiCad files.
🤖 Generated with Claude Code