Check F7.4 for vias in exposed pad #288

JacobEFO · 2019-02-07T20:31:28Z

As it currently is, F7.4 causes travis and library-utils to generate errors upon thermal vias in exposed pads.

To solve this I have made (started making) a script to disregard the check of required_layers, if the through-hole pad is within the exposed pad.

For simple exposed pads placed in (0,0) it works splendidly. But when it comes to exposed pads offset from (0,0) like Texas_RWH0032A_ThermalVias or double exposed pads like in Microsemi_QFN-40-32-2EP_6x8mm_P0.5mm it obviously fails.

Then comes a question, how an exposed pad is most easily recognized.

My train of thought was:

Get number of pins of package based package name, ie: MSOP-16
Find origin and width/height of exposed pad(s) that will be pad number 17, 18 etc.
Check through-hole pads within either of those

The only issue with this technique is for packages with x number of pins, where only y is populated, for instance Microsemi_QFN-40-32-2EP_6x8mm_P0.5mm where only 32 pins are populated, and exposed pads are labelled 33 and 34, no 41 and 42 as the script would suppose.

Do you guys have any suggestions, as how best to solve this?

Shackmeister · 2019-02-07T20:38:36Z

First thought is that pretty much all EP's with thermal vias has solder paste/mask disabled if I recall correctly. This could perhaps help in finding the EP

poeschlr · 2019-02-07T21:12:49Z

I think for now we do not need to think about every conceivable way this could go wrong. Anything that checks a normal ep pad with thermal vias would already be a huge (and very much welcome) improvement.

If a package is missing pins then it is still using ep pin number is one higher than if there are no missing pins.

Another hint: for parts with a single ep you can use the size of the pad as it must be included in the footprint name. (We could even add another rule check for that fact.)

JacobEFO · 2019-02-08T17:14:18Z

I think for now we do not need to think about every conceivable way this could go wrong. Anything that checks a normal ep pad with thermal vias would already be a huge (and very much welcome) improvement.

If a package is missing pins then it is still using ep pin number is one higher than if there are no missing pins.

Another hint: for parts with a single ep you can use the size of the pad as it must be included in the footprint name. (We could even add another rule check for that fact.)

Ah yeah, I think just reading the number of exposed pads in the name and getting the "x" amount of highest pad numbers is the way to go.

Ideally there should be a check, that the footprint follows KLC naming convention, but assuming it does follow xxx-yEP-xxx this could work actually.

poeschlr · 2019-02-08T18:37:13Z

TI footprints do not really follow that convention right now. (And other manufacturer specific ones might be added later)
I would only go that far if there is no pad at 0,0 to be honest. If there is (and it is a smd part) then we can assume that it is the ep. (maybe check if the size of that pad fits the size given in the footprint name to be double sure. The size is guaranteed to be in the name even if it is a manufacturer specific naming like the one for TI footprints)
But you are right if there is a -xEP string then we can definitely use it to our advantage. Not sure which approach will give us better or more easier results.

…ultiple exposed pads

JacobEFO · 2019-02-10T22:42:44Z

Right now I have updated it to handle exposed pads NOT placed in (0,0), but it assumes its placed on F.Cu, and it uses the -xep_ string in the footprint name to find the amount of exposed pads.

Then for each exposed pad found, it iterates through the maximum pad number and down (as set by -xEP_) and adds (x,y) and size_x and size_y to an array, that is later iterated through.

Seems to be working quite well. But again, it assumes the footprint name contains -xep (EP is NOT case sensitive for this script though).

Please let me know, what you think.

poeschlr · 2019-08-19T07:11:36Z

I kind of missed that there was progress made here. Sorry.

Could you fix the merge conflict? I think this would otherwise be good to go (to be live tested)

cpresser · 2019-08-19T11:02:27Z

pcb/rules/F7_4.py

+        # Looks for the string:
+        # '-xEP' where x is any number between 0 and 9 and designates number of exposed pads.
+        # EP is NOT case sensitive and registers 'ep' just as well as 'EP'
+        m = re.search('\\-[0-9].[a-zA-Z]', fpName)


I don't understand this re. Why is there a dot?
how about
-\d+[EP]{2}
with case insensitive set?

cpresser · 2019-08-19T11:03:05Z

pcb/rules/F7_4.py

+        fpName = module.name
+
+        # Check if footprint name contains the standard text strings for
+        # footprints with exposed pad and _ThermalVias


That comment seems to be outdated. The _ThermalVias stuff is commented out.

JacobEFO · 2020-02-22T23:34:57Z

I had forgotten all about this PR.

I'll have a look at this tomorrow to fix the merge conflict and update the code based @cpresser 's comments.

@cpresser

Regular expression search is now more narrow, and still case-insensitive. Suggested by @cpresser.

JacobEFO · 2020-02-23T07:57:37Z

Alright. I cannot for the life of me figure out, why there still is a merge conflict.

Apparently it does not like me adding the 'import re' statement. But however I try to put it, it complains. Any suggestions on that?

poeschlr · 2020-02-23T10:56:55Z

A merge conflict is not fixed by simply adding stuff to the files of the branch. You need to use git locally.
It will mean the following (while being on your branch):

git fetch upstream
git merge upstream/master
git mergetool
# fix all conflicts
git commit
git push

Edit: upstream is a remote pointer pointing to the official repo not to your personal fork

An alternative is the rebase workflow. That leaves the history in a nicer state but is harder to understand for beginners

JacobEFO · 2020-02-23T11:00:10Z

I am familiar with my origin and my upstream, so that's gonna be easy enough. I was not familiar with the way of solving merge conflicts though. I'll have a new go.

JacobEFO · 2020-02-23T11:11:35Z

I believe, I managed to fix it. But boy, what a hassle for you to verify my changes now.
You have my sincere apologies.

poeschlr · 2020-02-23T11:42:04Z

github is intelligent here. It knows what you changed and what was pulled from master. The same i would do if github would not be intelligent.

poeschlr · 2020-02-23T16:36:19Z

pcb/rules/rule.py

-        return graph['end']
-    elif 'angle' in graph:
-        # dosome magic to find the actual start point
-        # fetch values
-        x_c = graph['start']['x']
-        y_c = graph['start']['y']
-        x_e = graph['end']['x']
-        y_e = graph['end']['y']
-        a_arc = graph['angle']
-        # calculate radius
-        dx = x_c - x_e
-        dy = y_c - y_e
-        r = math.hypot(dx, dy)
-        # calculate vector of length 1 of the end point
-        dx_s = dx / r
-        dy_s = dy / r
-        # now get the angle of the end point
-        a = math.degrees(math.atan2(dy_s, dx_s))
-        a_s = math.radians(a + a_arc)
-        x_s = x_c - math.cos(a_s) * r
-        y_s = y_c - math.sin(a_s) * r
-        return {'x': x_s, 'y': y_s}


Was this on purpose or is this a merge artefact?

Pretty sure it should be a merge artifact. I did not deliberately touch that.

So could you revert that?

Odd that occurred. I thought the upstream/master also had this deleted. But just checked and it should in fact be there. I hope to get some time within the next few days to fix it.

JacobEFO · 2020-02-25T06:56:37Z

There should be a new commit ready for merge.

poeschlr · 2020-03-05T19:16:11Z

Running this against the LED_THT lib leads to an exception

Traceback (most recent call last):
  File "check_kicad_mod.py", line 117, in <module>
    rule.check()
  File "/home/rene/Dokumente/Hobby/kicad/90_scripts/kicad-library-utils/kicad-library-utils/pcb/rules/F7_4.py", line 139, in check
    return any([self.checkPads(module.pads)])
  File "/home/rene/Dokumente/Hobby/kicad/90_scripts/kicad-library-utils/kicad-library-utils/pcb/rules/F7_4.py", line 83, in checkPads
    if EParray:
UnboundLocalError: local variable 'EParray' referenced before assignment

JacobEFO · 2020-03-05T21:14:17Z

Running this against the LED_THT lib leads to an exception

Traceback (most recent call last):
  File "check_kicad_mod.py", line 117, in <module>
    rule.check()
  File "/home/rene/Dokumente/Hobby/kicad/90_scripts/kicad-library-utils/kicad-library-utils/pcb/rules/F7_4.py", line 139, in check
    return any([self.checkPads(module.pads)])
  File "/home/rene/Dokumente/Hobby/kicad/90_scripts/kicad-library-utils/kicad-library-utils/pcb/rules/F7_4.py", line 83, in checkPads
    if EParray:
UnboundLocalError: local variable 'EParray' referenced before assignment

Interesting. I didn't catch this issue. Apparently I did not do the troubleshooting thoroughly enough.

EParray was used without it being set, because it was defined in the wrong scope. Tidy up reg_exp search variable, because 'm' for a variable is not very intuitive.

JacobEFO · 2020-03-05T21:43:52Z

Apparently I had been dumb enough to check for 'EParray' at other places, where it could happen not to be defined. It should be fixed now, while I also tidied up a little within the code.

I can see, I used all sorts of conventions within my code, for which I apologize, and would like to know, if we adhere by pep8 and thus use underscores for function and variable names, or use camelCase (like I did).

I think it would be beneficial, to clean up the code to make it adhere by whatever standard, we like.

poeschlr · 2020-03-06T14:10:41Z

I don't think there is an official programming standard. It might be a good idea to define one long term.

JacobEFO · 2020-03-06T14:39:18Z

Should it become relevant, I'll update the code.

poeschlr · 2020-03-06T14:54:57Z

Microchip_DRQFN-44-1EP_5x5mm_P0.7mm_EP2.65x2.65mm leads to

Traceback (most recent call last):
  File "check_kicad_mod.py", line 122, in <module>
    raise e
  File "check_kicad_mod.py", line 118, in <module>
    rule.check()
  File "/home/rene/Dokumente/Hobby/kicad/90_scripts/kicad-library-utils/kicad-library-utils/pcb/rules/F7_4.py", line 137, in check
    return any([self.checkPads(module.pads)])
  File "/home/rene/Dokumente/Hobby/kicad/90_scripts/kicad-library-utils/kicad-library-utils/pcb/rules/F7_4.py", line 55, in checkPads
    if pad['number'] > padNoMax:
TypeError: '>' not supported between instances of 'str' and 'int'

i modified the check_kicad_mod.py file line 117 with

        try:
            rule.check()
        except Exception as e:
            printer.red("exception encountered checking rule {} on file {}"\
                            .format(rule.name, module.name))
            raise e

and then ran the script with
python3 check_kicad_mod.py -r F7.4 <path to lib>/kicad-footprints/*.pretty/*.kicad_mod

JacobEFO · 2020-03-06T15:06:13Z

Clearly my troubleshooting skills are poor. I manually tested this with 16 footprints, that all came up with no issue. Apparently I need some better testing skills.

You terminal command tells me "zsh: argument list too long: python3". I can however replicate the issue otherhow.

Interesting, I did not consider this corner case. I'll have a look at it.

poeschlr · 2020-03-06T15:23:19Z

Well i use bash. Bash forwards the file string to python which then uses glob to unpack it. If zsh does this up front then it might just fail (as the extended list is quite long)

JacobEFO · 2020-03-06T15:32:00Z

If you just decide on a given .pretty directory, it's no big deal. But I may just set up a script, that does this for me.
Or find a way for zsh to handle it.

Handling letters within pad numbers was definitely not a consideration of mine, and I see no straightforward way to handle it, especially if exposed pads are just given the EP name.
This is gonna take some time to solve.

poeschlr · 2020-03-06T15:44:26Z

I think the proper way would be not to explicitly check for exposed pads but flag any tht pad inside a smd pad as a via. One can then require these pads to have the same pad number plus allow more freedoms for the layers of the via.

Edit: An easy way out for now would be to flag pads that are non integer numbers as "not an exception" and let it be checked in the old way.

JacobEFO · 2020-03-06T15:52:19Z

I actually think, your first suggestion would be the better solution. Especially because it offers much more flexibility in the long-term.

The second one is more the quick-fix solution, which albeit would work in this case, but not be the better long-term solution.

Which one to go for depends a little on the librarians.

poeschlr · 2020-03-06T17:33:48Z

I prefer the long term solution but i would not require you to invest the time to implement it. I would already be happy with the hotfix. (You are the only one who can know if you have the time for the full solution)

JacobEFO · 2020-03-06T21:51:06Z

Obviously depends on the haste, I am currently about to finish my master thesis and begin a new job, so time will be short very soon. But given it's taken a while already, it may not be a big deal, if we add some more time for the better solution.

poeschlr · 2020-03-06T21:55:41Z

Or we put in a first solution working for most of the lib and then improve it later on.

JacobEFO · 2020-03-06T22:01:50Z

Great. I'll make a makeshift solution ;)

Components with letters in pad names caused the script to crash. Now they are handled with an exception instead, that just breaks the loop. The script still catches violations in these cases.

JacobEFO · 2020-03-06T22:25:03Z

Here is one. I tested it by jumping into the 'kicad-footprints/' directory and executing:

for d in ./*.pretty/ ; do (echo "$d" && python3 ~/kicad_lib/kicad-library-utils/pcb/check_kicad_mod.py -r F7.4 "$d"*.kicad_mod); done

Which looped all .kicad_mod files in all .pretty directories over. I caught no python errors. The script still catches errors in general, but will not catch the violations of rule F7.4 in footprints with pads with lettered names.

poeschlr · 2020-03-07T08:50:12Z

I did a few tests and am happy with the current result. Of course, we can improve it further and you are welcome to come back to this topic at any time.

JacobEFO · 2020-03-07T08:59:54Z

Great to hear @poeschlr.

I think I'll come up with a new merge request eventually.
First I am looking into improving F6_3 and also doing some symbol keyword seperation control first. But it would be lovely to see, a more robust solution that is not makeshift.

Vias have different layer requirements compared to normal through hole pads. This meant they wrongly triggered F7.4. This change excludes THT pads identified as thermal vias from this test. Known limitations of this implementation: - Reliance on the footprint name for identifying possible exposed pads with vias - The largest (integer) pad number is assumed to be the pad that holds possible vias - Footprints with non integer pad numbers are ignored. - Different pad numbering conventions (example the use of pad 0 for the EP) are not supported - No support for handling vias in pads other than an exposed pad

poeschlr · 2020-03-07T09:12:33Z

For now i documented our findings in #322

Feel free to comment on that issue with your ideas.

Check F7.4 for vias in exposed pad

cc3c696

JacobEFO added 2 commits February 10, 2019 23:38

Include python re - Regular expression operations

0980b95

Update exposed pad algorithm to utilize footprint name. Now handles m…

ed461ce

…ultiple exposed pads

poeschlr mentioned this pull request Aug 19, 2019

F6.3 & F7.4: Improper handling of custom mask/paste pads and thermal vias #245

Open

cpresser reviewed Aug 19, 2019

View reviewed changes

poeschlr mentioned this pull request Sep 22, 2019

Added QFN-16-1EP_4x4mm_P0.65mm_EP2.5x2.5mm KiCad/kicad-footprints#1870

Merged

5 tasks

poeschlr mentioned this pull request Nov 27, 2019

Skip THT checks for SMD components #319

Closed

JacobEFO added 5 commits February 23, 2020 08:20

Fix merge conflict by also also having pcb/rules 'import math'.

bb63dee

Update regular expression search.

674203f

Regular expression search is now more narrow, and still case-insensitive. Suggested by @cpresser.

Fix typo in regular expression for exposed pad search.

200679c

New merge conflict update.

173ac6a

Tidy up comments.

eca004f

poeschlr self-assigned this Feb 23, 2020

poeschlr added the Ready for review Use this to mark pull requests that are updated but you could not review instantly label Feb 23, 2020

poeschlr reviewed Feb 23, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into SolderPaste_EP

cfea135

JacobEFO force-pushed the SolderPaste_EP branch from b4bd3f0 to cfea135 Compare February 25, 2020 06:53

poeschlr added the Enhancement label Feb 25, 2020

poeschlr removed the Ready for review Use this to mark pull requests that are updated but you could not review instantly label Mar 5, 2020

Fix the scope of EParray

54ef58f

EParray was used without it being set, because it was defined in the wrong scope. Tidy up reg_exp search variable, because 'm' for a variable is not very intuitive.

Makeshift an exception for pads with letters in pad name.

1652f9d

Components with letters in pad names caused the script to crash. Now they are handled with an exception instead, that just breaks the loop. The script still catches violations in these cases.

poeschlr merged commit 0756eeb into KiCad:master Mar 7, 2020

poeschlr mentioned this pull request Mar 7, 2020

F7.4 improve detection of vias #322

Open

JacobEFO deleted the SolderPaste_EP branch March 7, 2020 20:18

Check F7.4 for vias in exposed pad #288

Check F7.4 for vias in exposed pad #288

Conversation

JacobEFO commented Feb 7, 2019

Shackmeister commented Feb 7, 2019

poeschlr commented Feb 7, 2019

JacobEFO commented Feb 8, 2019

poeschlr commented Feb 8, 2019 • edited Loading

JacobEFO commented Feb 10, 2019

poeschlr commented Aug 19, 2019

cpresser Aug 19, 2019

Choose a reason for hiding this comment

cpresser Aug 19, 2019

Choose a reason for hiding this comment

JacobEFO commented Feb 22, 2020

JacobEFO commented Feb 23, 2020

poeschlr commented Feb 23, 2020 • edited Loading

JacobEFO commented Feb 23, 2020

JacobEFO commented Feb 23, 2020

poeschlr commented Feb 23, 2020

poeschlr Feb 23, 2020

Choose a reason for hiding this comment

JacobEFO Feb 23, 2020

Choose a reason for hiding this comment

poeschlr Feb 23, 2020

Choose a reason for hiding this comment

JacobEFO Feb 23, 2020

Choose a reason for hiding this comment

JacobEFO commented Feb 25, 2020

poeschlr commented Mar 5, 2020

JacobEFO commented Mar 5, 2020

JacobEFO commented Mar 5, 2020 • edited Loading

poeschlr commented Mar 6, 2020

JacobEFO commented Mar 6, 2020

poeschlr commented Mar 6, 2020

JacobEFO commented Mar 6, 2020

poeschlr commented Mar 6, 2020

JacobEFO commented Mar 6, 2020

poeschlr commented Mar 6, 2020 • edited Loading

JacobEFO commented Mar 6, 2020

poeschlr commented Mar 6, 2020

JacobEFO commented Mar 6, 2020

poeschlr commented Mar 6, 2020

JacobEFO commented Mar 6, 2020

JacobEFO commented Mar 6, 2020

poeschlr commented Mar 7, 2020

JacobEFO commented Mar 7, 2020

poeschlr commented Mar 7, 2020

poeschlr commented Feb 8, 2019 •

edited

Loading

poeschlr commented Feb 23, 2020 •

edited

Loading

JacobEFO commented Mar 5, 2020 •

edited

Loading

poeschlr commented Mar 6, 2020 •

edited

Loading