pointhi / kicad-footprint-generator

creating kicad footprints using python scripts
GNU General Public License v3.0
186 stars 175 forks source link

Fix decimal pincounts in custom names #649

Open evanshultz opened 3 years ago

evanshultz commented 3 years ago

As mentioned at https://github.com/pointhi/kicad-footprint-generator/pull/604#issuecomment-700885439, I broke custom names that used the pincount. This fixes that. At least, the test_hidden_deleted_pins.yaml footprints still generate with the correct names and soic.yaml runs cleanly and gives, I believe, correct outputs. The footprints all look fine to me.

I check for deleted or hidden pins and, if so, use a custom name format which retains a digit for the pincount. A second pincount can be used, but is an empty string if not needed. And it goes nowhere for a custom name format that doesn't know about the second pincount. This way, I don't need any fixed format and the second pincount is more like a free-form text field I can populate if needed.

As I wrote the above, I realized I didn't check for all instances of pincount in the script so I pushed another couple of commits. The first corrects the EP name and also uses the number of pins present in the package in the description. The last corrects the name of footprints with hidden or deleted pins to match the IPC 7351 convention as shown on page 2 of http://ohm.bu.edu/~pbohn/__Engineering_Reference/pcb_layout/pcbmatrix/IPC-7x51%20&%20PCBM%20Land%20Pattern%20Naming%20Convention.pdf. I proposed adopting their terminology and then I didn't even get it right in the script! If you don't like it or don't want to change since only a few footprints have been merged I put it in a separate commit. But it seems right and we can now easily re-generate any footprints with hidden or deleted pins.

codeclimate[bot] commented 3 years ago

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

View more on Code Climate.

cpresser commented 3 years ago

I gave it a quick look and tbh, I don't like it. It seems way to complicated. I would rather keep pincount a string and use some sed magic to fix all the custom format strings. That would be much cleaner. But that is just my first impression. I am open for discussing the whole issue.

evanshultz commented 3 years ago

OK. I'm fine with another approach that works better for everyone. I can take a swing at that method in a bit. No problem.

Is this holding anything up? I know we're almost out of time so the more discussion and time it takes to merge a fix, the less time we have to merge in the rest of the library assets that rely on this fix. Is this a problem or we're OK?

cpresser commented 3 years ago

One PR (https://github.com/pointhi/kicad-footprint-generator/pull/645) I know of is related on this. But I could merge the footprints without this fix - the author did change the format strings. Once this is sorted out, we can merge #645

One might argue it would be best to fix the problem ASAP. But I would rather discuss the "best" solution. Unfortunately, I am not sure what the best solution to this problem is. What do you think?

thatoddmailbox commented 3 years ago

@cpresser @evanshultz Another option, if it's preferred, is that I leave the name formats alone in my PR, since I don't actually use the custom name format in any of the three footprints I added. This would mean soic.yaml would still be broken, but I would still use the workaround (replace pincount:d with pincount:s) on my local machine to create the output of the generator in KiCad/kicad-footprints#2432. Then you don't have to worry about blocking #645.

In terms of fixing the issue, I think it would make sense to just convert all the custom format strings to handle pincount as a string?

I suppose another alternative would be to make pincount an integer and add a pincount_suffix string. So then for hidden/deleted pins, you could have something like pincount of 20 and pincount_suffix of "_24N"? That way you wouldn't need to change the existing format strings.

evanshultz commented 3 years ago

So #645 is the only issue we know of.

@cpresser Go basically go back to the way it was before, but change line 228 (or add a line after it) to change pincount:d to pincount:s such that the code is master, which always uses a string for the pincount, works? That's easy. If I thought of it I'd probably have done that the first time. It was waaaay too late for me last night when I tried to figure this out. Thanks for the suggestion.

@thatoddmailbox

Your proposal to get the footprints made works for me. But I'll leave it up to you and @cpresser since you two are the ones commenting on that PR.

That was what I attempted to do, using a string suffix after the digit number of pins. Or I misunderstood you. But either way, if what I wrote above is correct I'm more happy to go that way.