search

pointhi / kicad-footprint-generator

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

Support hidden and deleted pins #604

Closed evanshultz closed 3 years ago

evanshultz commented 4 years ago

As discussed at https://github.com/pointhi/kicad-footprint-generator/pull/586 and specifically at https://github.com/pointhi/kicad-footprint-generator/pull/586#issuecomment-680057204, I've added support for deleted pins (pins which are not present but do not consume a pin number) and custom pad layouts like SOT23.

Here are some example footprints (from test_hidden_deleted_pins.yaml): image

I had to change scripts/Packages/Package_Gullwing__QFP_SOIC_SO/size_definitions/soic.yaml just to conform to this new terminology.

@pointhi The node PadArray seems to barely resemble the original implementation. There are so many caveats with deleted and hidden pins now. Since the values were being validated already, I chose to use a negative number as a filter since negative pin numbers seemed like they should not be allowed. Are you OK with this or do you have some other direction you'd like to pursue?

codeclimate[bot] commented 4 years ago

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

View more on Code Climate.

evanshultz commented 4 years ago

@cpresser Thanks! I'll take a closer look at this and make any updates a little later.

evanshultz commented 4 years ago

The node Pad only accepts int and string types for the pad number. Just below line 244 in PadArray this variable is used to determine if the pad is created or not (includePad). Since PadArray now includes iterators and many ways to create the pad numbers, not just simply building a list from the pincount argument, perhaps we should validate the type of the number after line 244? Then I could use some silly thing from line 35 of pad_number_generators.py instead of -1 which would get filtered out. And I don't have to check for a specific keyword on line 246 of PadArray. @cpresser @pointhi Thoughts?

cpresser commented 4 years ago

I did play around with the generator a little and don't quite understand the need for add_single_pin_right_center.

The SOT-23 can also be build with:

# can we do a SOT-23 without the custom_pad_layout? Seems like that is a yes!
SOT-23:
  size_source: '~'
  custom_name_format: 'SOT-23'
  body_size_x: 1.6
  body_size_y: 2.9
  overall_size_x: 2.8
  lead_width: 0.3 .. 0.5
  lead_len: 0.3 .. 0.45 .. 0.6
  pitch: 0.95
  num_pins_x: 0
  num_pins_y: 3
  deleted_pins: [2, 4 ,6]

It seems to identical to the one you build with the custom layout.

cpresser commented 4 years ago

The node Pad only accepts int and string types for the pad number. Just below line 244 in PadArray this variable is used to determine if the pad is created or not (includePad). Since PadArray now includes iterators and many ways to create the pad numbers, not just simply building a list from the pincount argument, perhaps we should validate the type of the number after line 244? Then I could use some silly thing from line 35 of pad_number_generators.py instead of -1 which would get filtered out. And I don't have to check for a specific keyword on line 246 of PadArray. @cpresser @pointhi Thoughts?

I took a short look at PadArray. It seems like the pad_numbers variable is populated in lines 212-223.

How about returning None from the generator? And just having a check like

if number == None:
  continue

after line 244?

evanshultz commented 4 years ago

Something is now broken when using deleted_pins. Let me sort this out and then I'll commit with the changes I described above, and also reply to your last two comments about the SOT-23 footprint and PadArray. Hang on...

evanshultz commented 3 years ago

Sorry about the delay. I didn't have as much time as I expected to work on this the last couple days. I had it figured out everything with deleted pins before and then forgot what I was doing. I've added more comments now to hopefully help me and everybody else who (sadly) has to deal with my ineptitude.

Good call on the SOT-23. I felt stuck on the deleted pins so I did the custom pad layout for SOT-23. Once the deleted pins was working I didn't realize the deleted pins meant the one custom pad layout wasn't necessary. Thanks! I didn't remove the custom pad layout file, yet, so please confirm and then I'll remove it in another commit and clean up the YAML file.

We are thinking similarly about filtering out pads that are deleted. I did it slightly differently so that it handles any other disallowed type for a pad number.

Anything else?

evanshultz commented 3 years ago

@cpresser I removed the custom pad layout stuff. Hopefully it just makes things easier if that's done since it seems unneeded now.

@pointhi Do you have any issues with the PadArray changes?

evanshultz commented 3 years ago

@cpresser When thinking back on this, I realize that the changes to PadArray may not have been necessary. It could have kept the parameter exclude_pin_list since it really doesn't need to know if the pads are hidden or deleted. Filtering out pad numbers which aren't int or str gives me the hook to support deleted pins, and the hidden pins work just by slinging a list from some external source to exclude_pin_list. There's a level of abstraction that really isn't needed and it makes things more opaque because the meaning and use of hidden pins and deleted pins is outside of PadArray. Adding the type filter I mentioned earlier if a good change, but what do you think about undoing the rest of the changes to that file?

cpresser commented 3 years ago

Tbh, I would need to look at that again in detail. But the overall idea sounds promising. PadArray should be as agnostic as possible. Feel free to rework this, I will gladly review it.

chschlue commented 3 years ago

@evanshultz @evanshultz https://github.com/pointhi/kicad-footprint-generator/pull/645#issuecomment-700878817 Was that change intentional?

evanshultz commented 3 years ago

No. Well, yes. If a standard footprint name is used, which is built inside the script, the pincount is always changed to a string so it works. But I didn't think about custom footprint names. Do you want me to push a fix?

chschlue commented 3 years ago

Yes, please. I'd rather not change every custom name, especially if the value is fundamentally a positive integer.

evanshultz commented 3 years ago

It was a positive integer until we adopted the IPC naming format for deleted and hidden pins. I think it's the right way to go, but I had overlooked custom footprint names.

I submitted https://github.com/pointhi/kicad-footprint-generator/pull/649, which I think should take care of things.