kicad-footprint-generator icon indicating copy to clipboard operation
kicad-footprint-generator copied to clipboard

Generate separate C, L, and R chip footprints

Open evanshultz opened this issue 4 years ago • 54 comments

Fixes https://github.com/pointhi/kicad-footprint-generator/issues/420

Because IPC tells us there are differences in the body sizes, not all chip parts should result in the same footprints. This generates unique footprints for each part type.

  • Add unique body size info for C, L, and R part types in accordance with IPC-SM-782 (with a slight deviation for 0805 capacitors)
  • File renames to remove 'chip' if file contained more than chip info
  • Aligned size definition file names
  • Print part type and definition during execution
  • Corrected descriptions and keywords in SMD_chip_devices.yaml
  • Fixed a few typos in comments

See a comparison of resulting footprints at https://docs.google.com/spreadsheets/d/1OAOwxWGSfmM8BzPS2x2mySmMnPzjYmWQ7ZMfdHtCbws/edit?usp=sharing.

Footprint PR: https://github.com/KiCad/kicad-footprints/pull/2280

Related: https://github.com/KiCad/kicad-footprints/issues/711

evanshultz avatar Oct 02 '19 17:10 evanshultz

Code Climate has analyzed commit f83b9878 and detected 0 issues on this pull request.

View more on Code Climate.

codeclimate[bot] avatar Oct 02 '19 17:10 codeclimate[bot]

@poeschlr Would you be willing and able to take a look at this?

evanshultz avatar Oct 02 '19 17:10 evanshultz

@poeschlr I made a separate commit I found for an error in one YAML file, but I should have made all the files changes in one commit and then done a separate commit for all the file renames (or put that in a separate branch). That probably would have been easier to review. My apologies for not thinking about how to do this better earlier. Is there anything I can do now to help?

And one question: because there's now a YAML file just for resistors, should the wide-body YAML file be merged into the regular resistor file? I kept them separate for continuity reasons but I really don't see why they can't be together in one file.

evanshultz avatar Oct 03 '19 21:10 evanshultz

Should I merge in master the same way I did at https://github.com/pointhi/kicad-footprint-generator/pull/439? Or is that not the right way?

evanshultz avatar Oct 07 '19 16:10 evanshultz

@poeschlr Could you take a look at this? Do you see problems with using unique body sizes for chip footprints as defined by IPC?

Also, merge in master as I did above to fix the conflict?

evanshultz avatar Nov 06 '19 21:11 evanshultz

I do not see a problem with using unique body sizes for different devices. However, be aware that we did not use the outdated IPC document everywhere. And we did avoid it for 0805 completely as we did our own research.

The reality is that such devices are much less standardized than we would like. Which means the footprints of the official lib can only ever be a compromise. Unless we really want to add one footprint per component series (not really feasible).

I hope that 5.1.6 finally fixes a deal breaker bug for the footprint wizard allowing me to continue working on a IPC footprint wizard for these sort of devices that will give users the option to generate fully compliant footprints for the device they really use.

poeschlr avatar Apr 26 '20 06:04 poeschlr

Regarding fixing the merge conflicts: You can either merge master into this one or rebase this onto the current master. The latter might be better in the sense of a cleaner git history but it also requires more git skills.


Edit: also remember that rebasing becomes impossible once one has used the merge workflow for a given branch.

poeschlr avatar Apr 26 '20 06:04 poeschlr

Yes. As this was a while ago I may have forgotten some details, but from memory and looking at the Google Sheets I linked the IPC-SM-782 0805 cap size and the body size you came up with from reviewing datasheets is quite similar. So I think the initial issues were related from us not realizing that C and L body sizes were given in the IPC doc, and instead using the R body size for all part types.

I will try a rebase, then. First time for everything...

evanshultz avatar Apr 26 '20 17:04 evanshultz

@poeschlr Now I see what you meant. I had already merged in master above to rebase wouldn't work.

I merged in master again with a little manual work. Branch conflicts resolved but the size definitions added at https://github.com/pointhi/kicad-footprint-generator/pull/440 have a SMD_ prefix instead of C_. When I've encountered this before I didn't find a way to resolve it in the same PR without GH seeing more branch conflicts so either made a second PR or a new replacement PR. Is there a way to fix that in this PR?

Also, I noticed a typo of 'tantalum' (there is a trailing 'n') so can that be done here too?

evanshultz avatar Apr 26 '20 18:04 evanshultz

One option to fix up the branch is to locally rename it, make a new branch with the current online name and then cherry-pick the commits from your original (now renamed branch) to your new branch correcting merge conflicts as necessary. At the end (after checking the state locally) force push the new branch to the online branch connected to this pull request.

This might help a bit: https://learngitbranching.js.org/

Edit: If necessary then i could look into fixing your branch but it might take a while till i find the time for it (don't tell that to normal contributors, this would be a service reserved to fellow maintainers).

poeschlr avatar Apr 26 '20 19:04 poeschlr

@poeschlr The branch is now fixed without conflicts. In addition, I fixed a typo and also an error when running the script. Not sure how that remained.

New footprints are submitted at https://github.com/KiCad/kicad-footprints/pull/2280.

evanshultz avatar May 12 '20 12:05 evanshultz

@poeschlr I created a footprint PR and requested your review, but most of the discussion was here so this is where I'm commenting. Will you be able to take a look at this? I have documented more info in the footprint PR.

evanshultz avatar Jun 19 '20 22:06 evanshultz

@poeschlr Ping!

evanshultz avatar Jun 28 '20 13:06 evanshultz

@diegoherranz If you're feeling up to it and want to review this that's fine with me. It seems @poeschlr is busy and @pointhi generally will merge whatever is approved into /scripts. But of course, take your time to review and make sure everything makes sense and you don't see anything I messed up. I don't want to break anything when I'm trying to fix things!

evanshultz avatar Jul 29 '20 23:07 evanshultz

@poeschlr Ping again!

evanshultz avatar Aug 06 '20 05:08 evanshultz

@cpresser You wanna take a look at this one?

evanshultz avatar Aug 24 '20 22:08 evanshultz

@cpresser Are you able to work on this now? I would quite like to have this in 5.1.7. I know we have a lot of footprints to sort out now as well.

Since Rene wrote the original generator script he knew the history. Let me try to capture what I recall here. There are also a few links above with more background info.

When Rene wrote the script we found that the cap footprints, specifically 0805, didn't see correct. Rene then surveyed info from cap datasheets and faked some body size for caps based on real datasheets. IPC-SM-782 has body size info and it turns out that the resistor dimensions were being used for all part types. When inputting the correct cap dimensions, the resulting footprints are almost exactly the same as the ones from Rene's research. While caps were noted by Wayne and precipitated making some manual adjustments earlier, other part types also have specific body sizes in that IPC doc. I captured all those body sizes here and then made updates so the correct body sizes are being used for each specific part type.

You may be able to search online and find IPC-SM-782, but if not I guess you just have to trust me on the dimensions??

In essence, it was just an oversight when developing the script to miss that IPC-SM-782 shows resistor body sizes first but there are other body sizes in later pages.

The reason why I kept pushing on this and marked it as a bug is that:

  1. We claim to have IPC 7351 footprints, but the body sizes are wrong for some parts. So bad input gives bad output. Some of our chip footprints are known to not meet the sizes they should according to IPC documentation.
  2. For some part types, the body size is quite wrong. There could be issues because the pad sizes don't match the part size. If there are soldering issues, I'd be very sad since this is a known problem and should have been corrected so users of the library weren't affected. Nobody wants to pay money and get back a bad board that is due to our library. Yes, this is only one of many known issues but it's one I found and care about.

evanshultz avatar Sep 10 '20 14:09 evanshultz

Sorry for the delay, I do have this open in a browser-tab for quite a while now. It is of medium size, so I guess It will be possible to get this done by september 23. image

However, I will invest my time the next few days into finishing some footprint-reviews I already started. My brain has a limited stacksize.

cpresser avatar Sep 10 '20 14:09 cpresser

@cpresser Thank you!

Yes, let's wrap everything else up and then go back to the tantalum YAML file.

Edit: I'll evaluate the footprint changes I've mentioned above and report back so we can look at the potential impact to users. Edit2: Done. See comments above for current vs proposed and my thoughts on it.

evanshultz avatar Sep 13 '20 22:09 evanshultz

While we're talking about this, I really dislike some of the naming here. I think it's because this was done first and Rene did things a little better with the gullwing and SON generator scripts.

  1. Each of the YAML files in /size_definitions starts with 'size_' which seems unnecessary. Remove it?
  2. The series_config parameter is the same file name as the global_config parameter. I would like to rename to package_config_KLCv3.yaml like the SON script. OK?
  3. The folder and script name of SMD_chip_package_rlc-etc is just ugly to type. This script does chip and tantalum capacitors body types, and other part types besides Rs and Ls and Cs, but there has to be a better name. Any ideas?

evanshultz avatar Sep 14 '20 05:09 evanshultz

1. Each of the YAML files in /size_definitions starts with 'size_' which seems unnecessary. Remove it?

Agree.

2. The `series_config` parameter is the same file name as the `global_config` parameter. I would like to rename to `package_config_KLCv3.yaml` like the SON script. OK?

I don't quite understand that. TBH the difference between series_config and global_config is not clear from the command-line help. It does not really describe what it does.

3. The folder and script name of `SMD_chip_package_rlc-etc` is just ugly to type. This script does chip and tantalum capacitors body types, and other part types besides Rs and Ls and Cs, but there has to be a better name. Any ideas?

Agree. Here are some of my ideas. None of them sound really appealing to me. Perhaps they give you the right inspiration.

  • Package_Chip_SMD
  • SMD_2Terminal_Packages
  • Generic_Chip_SMD
  • Generic_2Terminal_SMD

cpresser avatar Sep 15 '20 20:09 cpresser

One more point, should we move the discussion about the optimal pad geometry of the high-impact footprints somewhere else? This PR is getting quite cluttered. Also for documentation issues it would be nice if the discussion can easily be found.

cpresser avatar Sep 15 '20 20:09 cpresser

global_config is for the entire library, which all scripts can use. series_config is only for a particular generator (part/package type). It's simple and I just did it so let me know if you have an issue. We can now make further changes for all generator scripts since they will be aligned.

evanshultz avatar Sep 15 '20 20:09 evanshultz

global_config is for the entire library, which all scripts can use. series_config is only for a particular generator (part/package type). It's simple and I just did it so let me know if you have an issue. We can now make further changes for all generator scripts since they will be aligned.

Sorry, my comment also was not clear. I was referencing to the help text of the generator. Which currently has those two lines: the config file defining how the footprint will look like. (KLC) the config file defining series parameters. From what I understand they have the same effect, but series overrides global. That should be reflected in the help text.

cpresser avatar Sep 15 '20 21:09 cpresser

For the global_config vs series_config, this text matches the SON generator. I agree better wording could be used. Would it be OK to shelve this and update all the generators together in another PR?

Here are the three IPC 7351B tables that are specific to this generator: image image

It handles more than just chip packages. And SMD crystals and aluminum electrolytic caps also have 2 pins and a different table (I previously implemented the electrolytic cap IPC generator). Maybe ipc_chip_molded.py? The others are ipc_ plus the package types they handle.

evanshultz avatar Sep 16 '20 19:09 evanshultz

OK. I think that wraps up everything for chip parts. Yay!

While tantalum caps are out-of-scope for what I intended to do here, what are you thinking?

evanshultz avatar Sep 16 '20 21:09 evanshultz

Proposed process to continue with this PR and the footprints. We have a little more than 1 week left to get footprints merged for 5.1.7:

  • Merge this generator+yaml changes even when we are not 100% sure about the high-impact (0402 0603 0805) RLC footprints.
  • Compare and discuss the resulting footprints in https://github.com/KiCad/kicad-footprints/pull/2280
  • When changes are needed, open a new PR for footprint-generators, iterate the process

For the global_config vs series_config, this text matches the SON generator. I agree better wording could be used. Would it be OK to shelve this and update all the generators together in another PR?

Yes. We are building quite a backlog. I am going to open an issue for that right away so it does not get lost. edit: this one https://github.com/pointhi/kicad-footprint-generator/issues/631

cpresser avatar Sep 16 '20 21:09 cpresser

I find terminal spacing in the following YAML files:

  • resistor_chip_smaller_0603.yaml (only 0402)
  • default_chip_smaller_0603.yaml
  • default_chip.yaml
  • capacitor_tantal.yaml

The first one (R_04020) is simple enough so I'll share the difference when using terminal length here:

  • Pad centers: +/-0.485mm to +/-0.51mm
  • Pad width: 0.59mm to 0.54mm
  • Pad inside: 0.38mm to 0.48mm
  • Pad outside: 1.56mm to 1.56mm

image Current is above and new is below. In an absolute sense it a relatively large change but it because in comparison it's not as much. Pad spacing is much wider than the Kemet datasheet you shared at https://github.com/pointhi/kicad-footprint-generator/pull/438#discussion_r488921448, though. And compared with https://www.yageo.com/upload/media/product/productsearch/datasheet/mlcc/UPY-GPHC_X7R_6.3V-to-50V_18.pdf the min lead spacing could be 0.25mm (0.95mm body length - 2 * 0.35mm terminals) so even our existing footprint fails there. I'm not totally sure about this one. Here IPC pads seem to far apart compared to this datasheet while with 0805 they were too close together...

IPC-SM-782 has 3216, 3528, 6032, and 7343 tantalum cap body sizes.

For the others, however, there is no 'default' part in IPC-SM-782 so we don't really have any reference to use that I see. The link http://www.tortai-tech.com/upload/download/2011102023233369053.pdf is referenced for many of the non-tantalum parts but the link doesn't work so I'm not sure where to go for those. Thoughts? Just leave them if we don't have a reason to change?

And also, rename the generator. And then any tantalum stuff if you want to look into it.

evanshultz avatar Sep 16 '20 21:09 evanshultz

And also, rename the generator. And then any tantalum stuff if you want to look into it.

Tantalum: not as part of this PR. Rename: yes please.

The R_0402 change looks good to me at first glance. I like that there is more space between pads. That allows to route a signal between them.

I would like to merge this ASAP. And if necessary open another PR for fixes. See my answer here https://github.com/pointhi/kicad-footprint-generator/pull/438#issuecomment-693662901 Do you agree, or do you want to discuss all the high impact footprints as part of this PR?

cpresser avatar Sep 17 '20 15:09 cpresser

0402 resistor dimensions updated. One extraneous bit of info found and deleted. And file/folder renamed. I'm open to comments on the name as I'm not really in love with it.

Now that I also discovered that IPC-SM-782 has some tantalum cap body sizes I think perhaps it is worth taking a look at. Perhaps I should start by updating the format and then comparing the existing footprints to the ones that we would get if we used IPC dimensions?

@poeschlr http://www.tortai-tech.com/upload/download/2011102023233369053.pdf is used as a reference many times here, but that link is dead. Do you have the document or, if it's some kind of standard, can you share what it is? I have no idea and it may help.

evanshultz avatar Sep 18 '20 06:09 evanshultz