bennymeg / Fabrication-Toolkit

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

Update the code that matches entries in the DB to match documented behavior. #96

Closed bicknell closed 6 months ago

bicknell commented 11 months ago

Modify _get_rotation_from_db to match the database as described in the documentation.

Modify generate_tables to call both _get_rotation_from_db AND _get_rotation_offset_from_footprint. It's not clear from the docs the intended behavior, this patch applies BOTH. This allows the footprint to "override" the database.

It may make more sense to see if the footprint has a value, if it does use it directly, and if not look up in the database. Either way, it should be clearly documented in the docs which behavior happens.

Shackmeister commented 8 months ago

I tried you changes, but got the following error: 'str' object has no attribute 'HasProperty'

bicknell commented 8 months ago

I found the problem, I was passing footprint_name where I should have passed footprint.

In the process of debugging I also found two exceptions that were not being properly caught and showing the right error message.

Please check again.

Frank-UXV commented 8 months ago

Thansk Leo! I'll give it another test shortly :)

Shackmeister commented 8 months ago

Hi @bicknell Just tested, works fine now. Any chance you can add the optional X and Y offset? Perhaps it could be made compatible with this: https://github.com/matthewlai/JLCKicadTools/blob/master/jlc_kicad_tools/cpl_rotations_db.csv

"Footprint pattern","Rotation","Offset X", "Offset Y"

bicknell commented 8 months ago

Let me first add the documentation it looks like I forgot at this point:

Each footprint goes through the following steps:

First, let's place an example part called U1 from KiCad in the PCB, rotating it 90 degrees (so clockwise, 1/4 turn) and placing it at an x,y of 100,100.

  1. Get the rotation and position from KiCad for the part, representing what KiCad thinks compared to the footprint in the library. This will be 90, 100, 100 in the example.
  2. Look up the footprint in rotations.cf, if it exists, apply the rotation there on top of the kicad rotation. For example if the file contained "FootprintName 90" that matched the example part, it's rotation would now be 180, 100, 100.
  3. See if any of ['JLCPCB Rotation Offset', 'JlcRotOffset', 'JLCRotOffset'] keys exist on the specific part. These are checked IN ORDER, and the first match stops. Consider if our example U1 part has JlcRotOffset=90 and JLCRotOffset=45, our example part would become 270,100,100 because it matched JlcRotOffset and thus ignored JLCRotOffset.
  4. See if any of keys = ['JLCPCB Position Offset', 'JlcPosOffset', 'JLCPosOffset'] keys exist on the specific part. These are checked IN ORDER, and the first match stops. Consider if our example U1 part has JlcPosOffset=50,50 and JLCPosOffset=75,75. The part will become 270,150,150 because it matched JlcPosOffset and thus ignored JLCPosOffset.

The result is placed in the positions.csv file to be sent to the fab.

My basic thinking being that rotations.cf should handle 99% of the cases, and when it doesn't because some specific part has a different orientation the user will override that one part only with the keys in the BOM.

Now, back to your request. It would absolutely be possible to replace the current rotations.cf file with some improvement. You suggested a CSV, and I think the right thing to do there is to use Python's CSV module (https://docs.python.org/3/library/csv.html) to parse the file, and adjust the rest to read from the resulting data structure. The additional X,Y in the file would be applied in step 2, and the rest of the flow would be unchanged.

If all of this sounds reasonable, I will work on adding the CSV sometime in the coming days.

bicknell commented 8 months ago

Pushed updates that switch to reading from transformations.csv and support having an x and y offset in the file on disk.

Logic should be unchanged.

I have done some testing, and so far it works as I expect, however I would recommend much more extensive testing by others to be sure all cases work.

bicknell commented 8 months ago

Also, https://github.com/joanbono/awesome-kicad has a list of KiCad plugins including the following for JLC:

And of course, this plugin. In the "can't we all just get along" category it would be nice if everyone involved worked on a single plugin and made it extra super awesome, rather than continuing to reinvent the wheel. I don't have time or interest to try and heard those cats, but if someone wanted to do so and try and build a single community I think that would be awesome.

Frank-UXV commented 8 months ago

@bicknell Thanks for the changes! Couple of thoughts:

  1. Some parts with the same FP has different rotations. Any good idea how to solve it? Perhaps add an option to enter LCSC partnumber with different rotation/offset.
  2. Are the listing adding all true cases of rotations or only the first? Eg: FP name = DFN-10-3.3x3.3mmP0.5 Rules: ^DFN-, 90 ^DFN-10, 90

Would the final rotation be 90 or 180 deg?

Shackmeister commented 8 months ago

@bicknell Any thoughts on the abovE? :) Also another feature request, would be the option for a seperate database input for personal libraries

bicknell commented 7 months ago

Yes and no, I haven't had a lot of time to think about it. I think incorporating the ideas in this thread would require a complete rethink/rewrite of how it works.

My $0.02 on what that would look like:

Spitballing, a file would look something like this:

# Match Field, RegEx, Rotation, DeltaX, DeltaY
mpn,^PIC18F16Q40-I/P$,-90,0,0
lcsc,^C1547776$,-180,2,-2
footprint,^PDIP-,-180,0,0

Then the following real parts get these transformations:

Mfr. Part # PIC18F16Q40-I/P LCSC Part # C5239646 Package PDIP-20-300mil Matches the first mpn line, rotated -90, will not make it to the footprint match.

Mfr. Part # CD74ACT573E LCSC Part # C1547776 Package PDIP-20 Matches the second lcdc line, rotated -180 and shifted (+2, -2), will not make it to the footprint match.

Mfr. Part # SN74ACT374N LCSC Part # C180194 PackagePDIP-20 Matches the last footprint line, rotated -180.

My thinking is the file shipped with the plugin would have mostly generic rotations based on footprint (e.g. all SOT-23 are off 180). Users could then create their own personal overrides based on MPNs or LCSC PNs to override specific parts that are wrong with the generic rotation. When figuring out a new one, it can just be override in the BOM at first to avoid having to edit the file over and over.

I don't have the time to work on this in the next couple of weeks.

Shackmeister commented 6 months ago

@bicknell I think this makes a lot of sense. I hope this can get implemented in the future

bicknell commented 6 months ago

KiCad 8.0 has been released. One of the items is a new BOM Exporter. I suspect that's going to change everything about how this works. People will want to use the built in tool. This functionality should really now be built into the built in tool to make it seamless as the CLI can also trigger these exports.

Shackmeister commented 6 months ago

That sounds pretty reasonable. Have anybody tested the current plugin with V8?