Support hidden and deleted pins #604

evanshultz · 2020-09-03T05:31:06Z

As discussed at #586 and specifically at #586 (comment), 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):

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?

It turns out this was actually already supported, but with a different name and we have agreed on new terminology (pointhi#586 (comment))

codeclimate · 2020-09-03T05:31:28Z

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

View more on Code Climate.

cpresser

This is just an initial reading over the code.
I have yet to use and understand scripts/tools/custom_pad_layout.py.

I hope to get a full review done on the weekend.

scripts/Packages/Package_Gullwing__QFP_SOIC_SO/ipc_gullwing_generator.py

scripts/Packages/Package_Gullwing__QFP_SOIC_SO/size_definitions/test_hidden_deleted_pins.yaml

cpresser · 2020-09-03T20:39:38Z

scripts/tools/pad_number_generators.py

+    j = init # pad number for deleted pins
+
+    if kwargs.get("deleted_pins"):
+        skip_pins = kwargs["deleted_pins"]


I really don't like this naming. I think the variable should be called deleted_pins as well.

I tried to abstract things. In the PadArray node, for example, I kept the exclude_pin_list variable name but I allow two entry points of hidden_pins and deleted_pins since there are some cases where the same list works and other places where I need to know if the pins are hidden or deleted. Similarly, I decided to use a general term under the hood that could be used for other purposes in the future while the parameter name is specific to the behavior the user requests. .I can change it if you like.

How about that:

def increment(pincount, init=1, skip_pins=[]):

That makes it more abstract, it does not need to know about the deleted_pins keyword.

I thought about that, but I will still have to pick this keyword up in get_generator() so I figured it was essentially the same thing one place or the other. Is it better to grab this keyword in one place or the other?

I think it is better to extract the keyword in the calling function.
That way the increment is more abstract. Also the user/programmer can clearly see what it does when looking at the arguments.

edit I now also looked at the get_generator() function and it suffers from the same problem. It just gets passed a dictionary of arguments. It is virtually impossible to see which keys are needed.

I prefer the style when functions are called with named parameters so one can clearly see what is happening. Like so

But that would require substabtial refactoring when looking at the generators. Thus, your argument that it does not really matter where the keyword is processed is correct.

Yes, that's what I was feeling. Sorry for not being more clear and/or verbose. I originally had the pin-skipping increment generator as a separate function but decided from the outside it was similar enough to just combine them. I'll leave this alone unless I hear another comment from you.

Edit: And I agree with you, but I figured 'fitting in' with the code style was best here.

Currently, custom pad numbering doesn't work with deleted pins. If that's going to work in the future the refactoring will need to be done since the pins to be deleted will have to be removed for any generator function and not just the increment one. But that can happen when/if needed.

scripts/tools/pad_number_generators.py

evanshultz · 2020-09-03T20:55:32Z

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

- Reuse variable where possible - Replace datasheet links with tilde - Correct SOT-23-6 custom FP name - Improve increment pin number generator description

evanshultz · 2020-09-05T14:06:41Z

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?

KicadModTree/nodes/specialized/PadArray.py

cpresser · 2020-09-06T17:52:11Z

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 · 2020-09-06T20:49:35Z

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 · 2020-09-06T23:05:26Z

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...

- Also fixed a typo on line 55 of the test YAML file

evanshultz · 2020-09-09T05:17:32Z

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 · 2020-09-09T20:41:43Z

@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?

cpresser

I checked you recent commits and skimmed through all changes.
Looks good! ❤️
Just one import that you missed.

scripts/Packages/Package_Gullwing__QFP_SOIC_SO/ipc_gullwing_generator.py

evanshultz · 2020-09-14T06:01:30Z

@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 · 2020-09-15T18:43:15Z

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 · 2020-09-29T18:03:47Z

@evanshultz @evanshultz #645 (comment)
Was that change intentional?

evanshultz · 2020-09-29T20:35:48Z

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 · 2020-09-29T20:45:10Z

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

evanshultz · 2020-09-30T08:06:31Z

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 #649, which I think should take care of things.

evanshultz added 3 commits August 27, 2020 06:53

support hidden pins

eccb29e

It turns out this was actually already supported, but with a different name and we have agreed on new terminology (pointhi#586 (comment))

Support deleted pins and sot23 pad layout

f62447d

Minor cleanup

d036462

evanshultz requested a review from cpresser September 3, 2020 05:31

Improve error checking and code style

58bd613

evanshultz mentioned this pull request Sep 3, 2020

Add SOT-23-6 and sot.yaml #586

Merged

cpresser reviewed Sep 3, 2020

View reviewed changes

cpresser self-assigned this Sep 3, 2020

cpresser added the enhancement label Sep 3, 2020

evanshultz added 2 commits September 3, 2020 22:57

Review updates, round 1

5d9e9a9

- Reuse variable where possible - Replace datasheet links with tilde - Correct SOT-23-6 custom FP name - Improve increment pin number generator description

Handle hidden_pins as int type

9f32303

cpresser reviewed Sep 6, 2020

View reviewed changes

KicadModTree/nodes/specialized/PadArray.py Outdated Show resolved Hide resolved

Fix handling of deleted pins

c0f0709

- Also fixed a typo on line 55 of the test YAML file

Review updates, round 2

a3eab2c

evanshultz mentioned this pull request Sep 9, 2020

Add Onsemi TLV431 precision shunt regulators KiCad/kicad-symbols#2807

Closed

6 tasks

evanshultz added 2 commits September 9, 2020 13:38

Removed extra whitespace

e744bb9

Removed custom pad layout stuff

47d71f0

cpresser reviewed Sep 9, 2020

View reviewed changes

scripts/Packages/Package_Gullwing__QFP_SOIC_SO/ipc_gullwing_generator.py Outdated Show resolved Hide resolved

Remove cutom pad layout file import

a701822

cpresser mentioned this pull request Sep 10, 2020

Reproducable builds, Basic CI #620

Open

cpresser merged commit 738ff16 into pointhi:master Sep 10, 2020

evanshultz deleted the hidden-deleted-pins branch September 10, 2020 18:20

evanshultz mentioned this pull request Sep 30, 2020

Fix decimal pincounts in custom names #649

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support hidden and deleted pins #604

Support hidden and deleted pins #604

evanshultz commented Sep 3, 2020 •

edited

Loading

codeclimate bot commented Sep 3, 2020 •

edited

Loading

cpresser left a comment

cpresser Sep 3, 2020

evanshultz Sep 4, 2020

cpresser Sep 5, 2020 •

edited

Loading

evanshultz Sep 5, 2020

cpresser Sep 6, 2020 •

edited

Loading

evanshultz Sep 6, 2020 •

edited

Loading

evanshultz Sep 9, 2020

evanshultz commented Sep 3, 2020

evanshultz commented Sep 5, 2020

cpresser commented Sep 6, 2020

cpresser commented Sep 6, 2020

evanshultz commented Sep 6, 2020

evanshultz commented Sep 9, 2020

evanshultz commented Sep 9, 2020

cpresser left a comment

evanshultz commented Sep 14, 2020

cpresser commented Sep 15, 2020

chschlue commented Sep 29, 2020

evanshultz commented Sep 29, 2020

chschlue commented Sep 29, 2020

evanshultz commented Sep 30, 2020

Support hidden and deleted pins #604

Support hidden and deleted pins #604

Conversation

evanshultz commented Sep 3, 2020 • edited Loading

codeclimate bot commented Sep 3, 2020 • edited Loading

cpresser left a comment

Choose a reason for hiding this comment

cpresser Sep 3, 2020

Choose a reason for hiding this comment

evanshultz Sep 4, 2020

Choose a reason for hiding this comment

cpresser Sep 5, 2020 • edited Loading

Choose a reason for hiding this comment

evanshultz Sep 5, 2020

Choose a reason for hiding this comment

cpresser Sep 6, 2020 • edited Loading

Choose a reason for hiding this comment

evanshultz Sep 6, 2020 • edited Loading

Choose a reason for hiding this comment

evanshultz Sep 9, 2020

Choose a reason for hiding this comment

evanshultz commented Sep 3, 2020

evanshultz commented Sep 5, 2020

cpresser commented Sep 6, 2020

cpresser commented Sep 6, 2020

evanshultz commented Sep 6, 2020

evanshultz commented Sep 9, 2020

evanshultz commented Sep 9, 2020

cpresser left a comment

Choose a reason for hiding this comment

evanshultz commented Sep 14, 2020

cpresser commented Sep 15, 2020

chschlue commented Sep 29, 2020

evanshultz commented Sep 29, 2020

chschlue commented Sep 29, 2020

evanshultz commented Sep 30, 2020

evanshultz commented Sep 3, 2020 •

edited

Loading

codeclimate bot commented Sep 3, 2020 •

edited

Loading

cpresser Sep 5, 2020 •

edited

Loading

cpresser Sep 6, 2020 •

edited

Loading

evanshultz Sep 6, 2020 •

edited

Loading