Skip to content

fix: use declared pin side when grouping same-side passives (#15) - #169

Open
DPS0340 wants to merge 1 commit into
tscircuit:mainfrom
DPS0340:fix/vcap-series-cap-rows
Open

fix: use declared pin side when grouping same-side passives (#15)#169
DPS0340 wants to merge 1 commit into
tscircuit:mainfrom
DPS0340:fix/vcap-series-cap-rows

Conversation

@DPS0340

@DPS0340 DPS0340 commented Jul 24, 2026

Copy link
Copy Markdown

Progress on #15 — fixes the corner-pin misgrouping that leaves caps out of the decoupling rows.

Problem

findSameSidePassiveGroups infers a main-chip pin's side from its offset (|x| vs |y|). On tall chips this misclassifies corner pins: on the LayoutPipelineSolver06 board, U3.44 is declared side: "x-" (left edge) but sits near the top at offset (-1.9, 2.0), where |y| > |x| reads as y+.

This split U3's decoupling caps into an x- group and a phantom y+ group (C18, C7, C10 — pins U3.23/U3.50/U3.44, all actually left-edge pins). Result on the 06 board: C10 and C11 pack as loose singles floating between the cap rows and U3 instead of joining the rows — the "messy" arrangement in the issue screenshot.

Fix

The declared ChipPin.side is now authoritative (rotated by the chip's fixed rotation); offset inference remains only as a fallback for pins without a declared side.

Before/after grouping on the 06 board:

before: {main=U3 side=x- caps=C11,C19,C15,C13,C8,C14,C12} + phantom {main=U3 side=y+ caps=C18,C7,C10}
after:  {main=U3 side=x- caps=C11,C19,C15,C13,C8,C14,C12,C10,C7,C18}

Verification

  • New regression test tests/PackInnerPartitionsSolver/findSameSidePassiveGroups-corner-pins.test.ts (fails on main: phantom y+ group)
  • bun test — 56 pass, 0 fail; RP2040 board snapshots unchanged (their rows were already correct and stay byte-identical)
  • bunx tsc --noEmit / bun run format:check — clean

Scoped intentionally small: this fixes the grouping stage only. If you'd like, I can follow up on the row placement for the merged group (C10/C11 vertical spread on the 06 board) as a separate PR so each change stays reviewable.

@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
matchpack Ready Ready Preview, Comment Jul 25, 2026 12:19pm

Request Review

@DPS0340

DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Author

@mohan-bee you've been deep in the crystal/decoupling layout work here — mind casting an eye over this one? It's adjacent to #15.

Small change (2 files, +50 total): findSameSidePassiveGroups infers a main-chip pin's side from its offset (|x| vs |y|), which misreads corner pins and leaves caps out of the decoupling rows. This uses the pin's declared side instead.

Deliberately scoped to the grouping stage only — I left the row placement for the merged group alone so it stays reviewable. CI green, mergeable.

@mohan-bee

Copy link
Copy Markdown
Contributor

no snapshots !

@DPS0340

DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Author

@MustafaMulla29 adding you — you've merged 8 of the last ~25 here, more than anyone, and I'd only tagged @mohan-bee. Whoever has a moment.

2 files, +45/-5, one of which is the test.

Progress on #15. findSameSidePassiveGroups infers a main-chip pin's side from its offset (|x| vs |y|), which misclassifies corner pins on tall chips. On the LayoutPipelineSolver06 board, U3.44 is declared side: "x-" but sits at offset (-1.9, 2.0), where |y| > |x| reads as y+.

That split U3's decoupling caps into an x- group and a phantom y+ group (C18, C7, C10 — all actually left-edge pins), so C10 and C11 packed as loose singles floating between the cap rows and U3 rather than joining the rows. That's the messy arrangement in the issue screenshot.

The fix makes the declared ChipPin.side authoritative (rotated by the chip's fixed rotation), keeping offset inference only as a fallback for pins with no declared side.

Regression test covers the corner-pin case and fails on main. CI green, MERGEABLE / CLEAN.

Progress on tscircuit#15.

findSameSidePassiveGroups inferred a main-chip pin's side from its offset
(|x| vs |y|), which misclassifies corner pins on tall chips: U3.44 at
offset (-1.9, 2.0) is declared side x- but reads as y+ from the offset.
On the LayoutPipelineSolver06 board this split U3's decoupling caps into
an x- group and a phantom y+ group, so C10/C11 (and rail siblings C7/C18)
were never grouped with the left-edge rows and packed as loose singles.

The declared ChipPin.side is now authoritative, rotated by the chip's
fixed rotation; offset inference remains as a fallback for pins without
a declared side.

All caps on the 06 board now group on x-, and the RP2040 boards keep
their existing rows (snapshots unchanged).
@DPS0340

DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Author

@mohan-bee — rebased onto current main (was ~10 commits behind, now CLEAN) and re-verified the fix still holds.

Asking about this one specifically because you're the person merging in this repo and it's small: 2 files, +45/-5.

Re-verified after the rebase:

  • Full suite 56 pass / 0 fail / 1 skip, tsc --noEmit clean, all CI green.
  • The regression test actually bites. Reverting just findSameSidePassiveGroups.ts to main and re-running:
    with fix:      1 pass / 0 fail
    main's version: 0 pass / 1 fail
    
    So it's pinning real behaviour, not passing vacuously.

Recap of what it fixes, since it's been open a couple of days:

findSameSidePassiveGroups inferred a main-chip pin's side from its offset (|x| vs |y|), which misclassifies corner pins on tall chips. On the LayoutPipelineSolver06 board, U3.44 is declared side: "x-" but sits at offset (-1.9, 2.0) where |y| > |x|, so it read as y+.

That split U3's decoupling caps into a real x- group and a phantom y+ group (C18, C7, C10 — all actually left-edge pins), and C10/C11 then packed as loose singles floating between the cap rows instead of joining them. That's the arrangement in #15's screenshot.

The fix treats the declared ChipPin.side as authoritative, rotating it by the chip's ccwRotationDegrees rather than re-deriving it from geometry.

Happy to close it if the approach isn't what you want for #15 — I'd rather that than have it sit.

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.

2 participants