Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Fix decimal pincounts in custom names #649

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

evanshultz
Copy link
Collaborator

As mentioned at #604 (comment), 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
Copy link

codeclimate bot commented Sep 30, 2020

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

View more on Code Climate.

@cpresser
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

One PR (#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
Copy link

@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
Copy link
Collaborator Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants