Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle correction offsets and rotation separately #707

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikesmitty
Copy link

I ran into some very confusing/unintuitive behavior when setting both rotation and X/Y offsets on a component, and I think it's a bug, but please correct me if I'm wrong.

I have a PCB file where the rotation of a component is set to -90, and I noticed when uploading the position file for a PCBA job that it is off by 180 degrees from JLCPCB's library. I set the JLCPCB_CORRECTION field to 0;0;180 in my schematic to fix that, then went to update the X/Y offsets which were also offset. But because the component's rotation ended up being converted to 90 degrees this function effectively swapped X and Y, which is very confusing, especially when making changes to both X and Y at the same time.

I don't think it makes sense for the rotation angle to be taken into account when applying the X/Y correction fields here. Intuitively, I expect X and Y to be applied as is to the final positions regardless of the rotation, but let me know what you think

@yaqwsx
Copy link
Owner

yaqwsx commented Jun 29, 2024

This is a documented behavior: https://yaqwsx.github.io/KiKit/latest/fabrication/jlcpcb/#assembly

The idea is that you reference the XY correction in the footprint coordinate system, not the board one. It means that you can directly enter the difference between the origin in the component's datasheet and KiCAD footprint. It can, however, be confusing when you are trying to fine-tune position based on the board view.

I was always correcting my components based on the datasheet. Hence, this is what is implemented in KiKit.

If we are about to change this, then this is a breaking change and it needs to be part of a major release. To make such a significant change, we need to collect more opinions among the KiKit's user.

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