KiCad / kicad-library-utils

Some scripts for helping with library development
GNU General Public License v3.0
128 stars 95 forks source link

Check F7.4 for vias in exposed pad #288

Closed JacobEFO closed 4 years ago

JacobEFO commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

JacobEFO commented 5 years ago

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 commented 5 years ago

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)

JacobEFO commented 4 years ago

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.

JacobEFO commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

JacobEFO commented 4 years ago

There should be a new commit ready for merge.

poeschlr commented 4 years ago

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 commented 4 years ago

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.

JacobEFO commented 4 years ago

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 commented 4 years ago

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

JacobEFO commented 4 years ago

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

poeschlr commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

JacobEFO commented 4 years ago

Great. I'll make a makeshift solution ;)

JacobEFO commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

poeschlr commented 4 years ago

For now i documented our findings in https://github.com/KiCad/kicad-library-utils/issues/322

Feel free to comment on that issue with your ideas.