fix(schematic): snap add_power_symbol to the schematic grid - #665
Conversation
|
Queue position: next after #663. I reviewed the unique change on exact head Do not reconstruct yet. #663 is the current base-forming PR and overlaps this PR in |
add_schematic_component and batch_place_components snap the requested position to KiCad's 1.27 mm grid through place_one_component; add_power_symbol passed its coordinates straight to the symbol (#662). Wires and labels are snapped as well, so a power symbol left off the grid could not be reached by them. On a real 8-sheet design that was 21 endpoint_off_grid warnings and a set of pin_not_connected errors. The handler now snaps before anything else uses the point, so the bound placement intent, the field anchors and the instance record all agree with what is written. The response already reported x and y from the committed file; they now show the snapped position. The shared placement acceptance test nearly caught this: it hands all three placers an off-grid point, but its snap assertion sat behind the U999 reference, and a power symbol is auto-numbered #PWRnnn, so the one placer that did not snap was the one placer the check skipped. The check now runs for every placed symbol and asserts both axes are on the grid. A served-dispatch regression places GND at (100.1, 80.2) on the KiCad-authored hierarchy fixture and reads (100.33, 80.01) from both the response and the committed file. Two field-anchor tests requested (100.0, 80.0), which is off the grid, and asserted the library's anchor offsets from that point. They now request an on-grid point, so the offsets they pin are not mixed with the snap. Closes #662 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
#663 has merged; please reconstruct this PR now. Current This remains the next PR in the merge train; #666 and #664 stay behind it. |
a66f322 to
f1db987
Compare
|
#663 merged as New exact head:
Body updated with the new base, head and counts. Label moved to One addition to the queue since your sweep: #668 ( |
neusse
left a comment
There was a problem hiding this comment.
Reviewed exact head f1db987e75a3e838540ce44ebdfcc8592840d374 against current main cdcafa8ceebc8cec28a110b4cac838c0bc407e81.
The reconstruction contains the same focused #662 change previously reviewed: snapping occurs before placement-derived state, committed-file readback remains authoritative, the widened all-placer/two-axis assertion closes the prior coverage hole, and the served regression plus real KiCad ERC evidence match the issue. Closes #662 is correct. All ten required checks are green and there are no unresolved review threads.
Summary
add_power_symbolwrote the requested position as given, whileadd_schematic_componentandbatch_place_componentssnap to KiCad's 1.27 mm schematic grid. Wires and labels are snapped too, so a power symbol left off the grid could not be reached by them. On a real 8-sheet design that was 21endpoint_off_gridwarnings and a set ofpin_not_connectederrors. It now snaps like the other two placers.Closes #662
Kept separate from #583 (
#PWR001versus eeschema's#PWR01), which is designator spelling in the same handler and needs its own migration note.Approach
Reproduced first through the release binary on
main's behaviour, with kicad-cli 10.0.5's ERC as the oracle (table below): the resistor lands on the grid, the three power symbols do not, and ERC reportsendpoint_off_grid: 3.Fix. One line in
handle_add_power_symbol:snap_point(x, y, 1.27), the shared helperplace_one_componentalready uses, applied before anything else reads the point. The bound placement intent the readback is checked against, the field anchors and the instance record therefore all agree with what is written.xandyin the response were already read back from the committed file throughplaced_component_readback, so they report the snapped position without any response change.Why the suite missed it. The shared placement acceptance test hands all three placers an off-grid point, but its snap assertion sat behind
symbol.reference() == Some("U999"). A power symbol is auto-numbered#PWRnnn, so the one placer that did not snap was the one placer the check skipped. The check now runs for every placed symbol, on both axes, and asserts the coordinate is on the grid rather than merely different from the request.Scope note. A request already on the grid, which includes any pin endpoint of a placed component, lands exactly where it did before. A pin endpoint that is itself off the 1.27 mm grid cannot be reached by any Konnect placer or wire today, because they all snap; that is not changed here.
Architectural fit: reuses the shared snap helper and the shared placement readback; it removes a divergence between the three placers and adds nothing new.
Branch and dependencies
Base branch:
mainatcdcafa8(rebuilt once after #663 merged, as the train asks). The rebuilt commitf1db987carries per-file changes byte-identical to the revieweda66f322; only its position indocs/API_MIGRATIONS.mdmoved, below #663's entry.Depends on: nothing. Series order: none. Unique commits/acceptance criteria owned by this PR: the whole of #662. Next PR to promote after this one: none. Overlap: the top of
docs/API_MIGRATIONS.mdand onetool-directory.mdrow (as #663 and #581's PR); whichever lands later reconstructs once. #666 (#583, designator spelling) touches the same handler lines apart and follows this PR in the train; a trial merge of the two passes both test modules.Compatibility and safety
GOVERNANCE.md: a fix, no argument or response field added or removed. Recorded indocs/API_MIGRATIONS.mdbecause the coordinates written for an off-grid request change, by at most 0.635 mm on each axis.kicad-schematicskill gains two lines saying the position is snapped and reported.Validation
Changed tool behavior
schematic,power_net,x,y,rotationdefault 0)..kicad_sch, with the existing sheet-instance validation before writing.Commands run on
f1db987(Windows 11, kicad-cli 10.0.5), exit codes captured directly:cargo fmt --all -- --checkcargo clippy --workspace --locked --all-targets -- -D warningscargo test --workspace --locked --lib --tests— 2018 passed / 0 failedcargo test --workspace --locked --doccargo xtask fix-doc-counts --check— unchangedcargo test -p konnect-core reliability_contract— greenTests.
native_placement_preserves_unique_and_reused_paths_with_committed_readbacknow asserts, for every symbol placed by each of the three placers on the KiCad-authoredcomplex_hierarchyfixture, that both axes are on the 1.27 mm grid. Two existing field-anchor tests insch_wiring::power_symbol_testsrequested (100.0, 80.0), which is off the grid, and asserted the library's anchor offsets from that point; the full gate caught them, and they now request an on-grid point so the offsets they pin are not mixed with the snap. New served regressiona_power_symbol_is_snapped_through_the_served_dispatch: GND requested at (100.1, 80.2) throughMcpHandler::handle_messagereads (100.33, 80.01) from the response and from the committed file.Neuters (each guard reverted alone,
--no-fail-fast):main's behaviour)native_placement_preserves_unique_and_reused_paths_with_committed_readback(the widened assertion:add_power_symbol: x = 100.1 is off the 1.27 mm grid) and the serveda_power_symbol_is_snapped_through_the_served_dispatchxis snappedyaxisBoth neuters compiled; neither was caught by a build break. The first row is the "widened assertion failing before the fix" the issue asks for: with the old guard behind
U999, that neuter passed the acceptance test.E2E through the release binary over stdio, kicad-cli 10.0.5 ERC as the oracle (
e2e662.py:create_project, oneDevice:Rrequested at (100.0, 100.0), threeGNDsymbols requested off grid):mainadd_schematic_componentDevice:Rrequested at (100.0, 100.0)add_power_symbolGND requested at (100.1, 120.2)add_power_symbolGND requested at (130.4, 120.2)add_power_symbolGND requested at (150.7, 90.3)kicad-cli sch erc --severity-allon the saved fileendpoint_off_grid: 3,pin_not_connected: 5,power_pin_not_driven: 1endpoint_off_grid: 0,pin_not_connected: 5,power_pin_not_driven: 1The reported coordinates are the ones in the file in both columns (the response is a readback); only where the symbol lands changed. The remaining two ERC kinds are the same before and after because the driver wires nothing: it isolates the grid defect.
Review checklist
upstream/main, has no merge conflicts, and CI passed on this exact head.upstream/main, not a release tag.Maintainer merge state
status:*workflow label.status:ready-to-mergeapplies to this exact head SHA.mainruleset.gh pr merge N --mergeafter final verification.Closes #662. Next PR to promote: none.🤖 Generated with Claude Code