bennymeg / Fabrication-Toolkit

An JLC PCB Fabrication Plugin for KiCad
Apache License 2.0
300 stars 52 forks source link

Wrong bottom rotation computation #101

Closed set-soft closed 6 months ago

set-soft commented 9 months ago

The following computation is wrong:

https://github.com/bennymeg/JLC-Plugin-for-KiCad/blob/8db1a4d446336aa4d34d0a66cc8adbae25e3a9d9/plugins/process.py#L207

It only works when the adjust is 0. I see the DB stuff is disabled in the code, so I don't know if you really want to fix it. The problem is that when fixing it people that already added manual rotations for components in the bottom side will get a different rotation.

It took me a while to figure out that the correct rotation is computed using:

(180 - KiCad_Angle) + JLCPCB_Rotation

And not:

180 - (KiCad_Angle + JLCPCB_Rotation)

I also had a wrong rotation in my code (KiBot) and now I fixed it and verified the rotations on JLCPCB, so I don't really need it fixed, but I wanted to explain you what's wrong in your plugin.

bennymeg commented 9 months ago

I'm not sure I understand, are commutative. Could you correct this line to the appropriate rotation? https://github.com/bennymeg/JLC-Plugin-for-KiCad/blob/8db1a4d446336aa4d34d0a66cc8adbae25e3a9d9/plugins/process.py#L207

set-soft commented 9 months ago

I'm not sure I understand, are commutative.

Nope, take a look at this:

(180 - KiCad_Angle) + JLCPCB_Rotation = 180 - KiCad_Angle + JLCPCB_Rotation

And not:

180 - (KiCad_Angle + JLCPCB_Rotation) = 180 - KiCad_Angle - JLCPCB_Rotation

Could you correct this line to the appropriate rotation?

You can't just correct this line because you already added rotation_offset. You should do it before adding the correction angle. What you need to do is to just undo the KiCad bottom mirroring, then apply the correction angle.

I suggest trying something like this: test.zip

Generate the BoM and CPL and upload it to JLCPCB. I'm getting correct rotations and positions with the following files: JLCPCB_position_bom_jlc.csv and JLCPCB_position_cpl_jlc.csv

Note that J1 to J6 also needs offset adjusts.

dzid26 commented 8 months ago

I wonder if this used to be treated differently by JLC.

It's curious that the other plugin has the same issue. (Although maybe not, perhaps one was inspired by another.)

I can confirm e.g. TO-277A with C895438 with 90deg correction computes incorrectly for top/bottom here as well.

set-soft commented 8 months ago

I wonder if this used to be treated differently by JLC.

Most probably, they changed at least twice

dzid26 commented 8 months ago

I think the calculation should be like this: https://github.com/Springer-Electronics/JLC-Plugin-for-KiCad/commit/33cec19b258c0aa36e3040cf5bdf3b9db1a3cc64

I applied offset with negative sign to match JLC button behavior (rotate clockwise corresponds to +90deg rotation offset now).

image

I believe this breaks compatibility with a lot of existing designs, so I am thinking to go back to inverted behaviour. I just would want to get an agreement on this.

With that, I took your example above and tested the change and used following offsets: image capture-2024-01-10T17_50_15 430Z capture-2024-01-10T17_50_25 367Z JLCPCB_position.kicad_sch.zip

It's all good now on both sides. JLCPCB_position_2024-01-10_17-36-51.zip

set-soft commented 7 months ago

I believe this breaks compatibility with a lot of existing designs, so I am thinking to go back to inverted behaviour. I just would want to get an agreement on this.

IMHO compatibility is better than matching the UI sign. You just need to document it.

dzid26 commented 7 months ago

I believe this breaks compatibility with a lot of existing designs, so I am thinking to go back to inverted behaviour. I just would want to get an agreement on this.

IMHO compatibility is better than matching the UI sign. You just need to document it.

Ok, agreed. This seems consistent with readme which suggest counterclockwise for positive values.

...add an 'JLCPCB Rotation Offset' field with an counter-clockwise orientation offset in degrees to correct...

I will test it a bit more to get position offset consistently applied at the bottom layer.

Btw, the through-hole components need to be have anchor in the center. I created separate fix for that and I think this one is worth the incompatibility of some old designs.

dzid26 commented 7 months ago

I believe this is the right way to do it - https://github.com/Springer-Electronics/JLC-Plugin-for-KiCad/commit/73062eab54663557ec8e945dc2106d70a7d48fc9 At this point I was just doing trial and error approach to achieve offsets consistency top/bottom. It's a bit weird but behaves consistent .

I believe this keeps compatibility for existing designs for top components rotations-offsets. Of course bottom rotation offsets and also position offsets will behave differently for existing designs.

test.zip