KiCad / kicad-library-utils

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

checklib doesn't catch repeated/mismatched part errors #133

Open evanshultz opened 7 years ago

evanshultz commented 7 years ago

I created a test library with a few errors that runs cleanly through checklib.py. Here are the known errors:

  1. Two symbols with the same name (PART2).
  2. A symbol with the same name as an ALIAS (PART1 and PART2).
  3. Duplicated ALIAS entries (PART2).
  4. An entry in the DCM which has no corresponding symbol (PART3).

Most glaring, when I try to load a library in KiCad I see the following: image

This means that there is a error which KiCad is checking for that checklib.py isn't. A user could certainly think their library or updates are good when in fact there is a real problem.

There may be more errors I created which I overlooked, and there almost certainly are other pathological errors along these lines if we think harder.

Here is test.lib:

EESchema-LIBRARY Version 2.3
#encoding utf-8
#
# PART1
#
DEF PART1 U 0 20 Y Y 4 F N
F0 "U" 0 200 50 H V L CNN
F1 "PART1" 0 -200 50 H V L CNN
F2 "" -50 100 50 H I C CNN
F3 "" 50 200 50 H I C CNN
ALIAS PART1 PART2 PART2
$FPLIST
 DIP*W7.62mm*
$ENDFPLIST
DRAW
P 4 0 1 10 -200 200 200 0 -200 -200 -200 200 f
X V+ 4 -100 300 150 D 50 50 0 1 W
X V- 11 -100 -300 150 U 50 50 0 1 W
X ~ 1 300 0 100 L 50 50 1 1 O
X - 2 -300 -100 100 R 50 50 1 1 I
X + 3 -300 100 100 R 50 50 1 1 I
X + 5 -300 100 100 R 50 50 2 1 I
X - 6 -300 -100 100 R 50 50 2 1 I
X ~ 7 300 0 100 L 50 50 2 1 O
X ~ 8 300 0 100 L 50 50 3 1 O
X - 9 -300 -100 100 R 50 50 3 1 I
X + 10 -300 100 100 R 50 50 3 1 I
X + 12 -300 100 100 R 50 50 4 1 I
X - 13 -300 -100 100 R 50 50 4 1 I
X ~ 14 300 0 100 L 50 50 4 1 O
ENDDRAW
ENDDEF
#
# PART2
#
DEF PART2 U 0 20 Y Y 2 F N
F0 "U" 150 150 50 H V C CNN
F1 "PART2" 250 -150 50 H V C CNN
F2 "" 0 0 50 H I C CNN
F3 "" 0 0 50 H I C CNN
ALIAS PART1
$FPLIST
 DIP*W7.62mm*
$ENDFPLIST
DRAW
P 4 0 1 10 -200 200 200 0 -200 -200 -200 200 f
X V- 4 -100 -300 150 U 50 50 0 1 W
X V+ 8 -100 300 150 D 50 50 0 1 W
X ~ 1 300 0 100 L 50 50 1 1 C
X _ 2 -300 -100 100 R 50 50 1 1 I
X + 3 -300 100 100 R 50 50 1 1 I
X + 5 -300 100 100 R 50 50 2 1 I
X _ 6 -300 -100 100 R 50 50 2 1 I
X ~ 7 300 0 100 L 50 50 2 1 C
ENDDRAW
ENDDEF
#
# PART2
#
DEF PART2 U 0 20 Y Y 2 F N
F0 "U" 0 200 50 H V L CNN
F1 "PART2" 0 -200 50 H V L CNN
F2 "" 0 0 50 H I C CNN
F3 "" 0 0 50 H I C CNN
$FPLIST
 DIP*W7.62mm*
$ENDFPLIST
DRAW
P 4 0 1 10 -200 200 200 0 -200 -200 -200 200 f
X V- 4 -100 -300 150 U 50 50 0 1 W
X V+ 8 -100 300 150 D 50 50 0 1 W
X ~ 1 300 0 100 L 50 50 1 1 O
X - 2 -300 -100 100 R 50 50 1 1 I
X + 3 -300 100 100 R 50 50 1 1 I
X + 5 -300 100 100 R 50 50 2 1 I
X - 6 -300 -100 100 R 50 50 2 1 I
X ~ 7 300 0 100 L 50 50 2 1 O
ENDDRAW
ENDDEF
#
#End Library

And here is test.dcm:

EESchema-DOCLIB  Version 2.0
#
$CMP PART1
D 30V Vce, 100mA IC, Double NPN Transistors, Current mirror configuration, SOT-143
K Transistor Double NPN
F http://www.nxp.com/documents/data_sheet/BCV61.pdf
$ENDCMP
#
$CMP PART2
D 30V Vce, 100mA IC, Double PNP Transistors, Current mirror configuration, SOT-143
K Transistor Double PNP
F http://www.nxp.com/documents/data_sheet/BCV62.pdf
$ENDCMP
#
$CMP PART3
D Ferrite Bead
K Ferrite Bead
$ENDCMP
#
#End Doc Library
bobc commented 7 years ago

Rather than make checklib a python copy of KiCad, I suggest that libs should be valid KiCad libs before running checklib. Checklib performs additional checks for KLC that are not required by KiCad. If a lib does not load in KiCad, it is not even worth bothering to run checklib.

I guess we always need to be careful with unstated assumptions and what is considered too obvious to document. But I think if this is considered important, we should a new rule to KLC

  1. All libraries must be checked for validity by loading into latest stable KiCad
jkriege2 commented 7 years ago

The problem is that checklib runs (via Travis) before the lib is ever loaded into KiCAD ... often one can even make requests in a PR before checking that particular branch out, opening it in KiCAD ... In this sense, if checklib already finds some errors that make it impossible for KiCAD to load the lib, that will really help, as I don't ahve to put time into checkout+test, which will fail ...

Best, JAN

bobc commented 7 years ago

I'm not sure there really is a problem, it's theoretical. Have we had any such issues?

In general, people submitting PRs created the lib in Kicad, so it should load in KiCad. Only if you are creating libs via scripts, would you create an invalid lib. In that case, the onus should be on the submitter to make sure their lib loads in KiCad.

Recreating all the checks that KiCad does by writing Python scripts seems like an incredible waste of effort, especially for a theoretical case which has never or very rarely likely to happen.

On 6 July 2017 at 12:05, Jan W. Krieger notifications@github.com wrote:

The problem is that checklib runs (via Travis) before the lib is ever loaded into KiCAD ... often one can even make requests in a PR before checking that particular branch out, opening it in KiCAD ... In this sense, if checklib already finds some errors that make it impossible for KiCAD to load the lib, that will really help, as I don't ahve to put time into checkout+test, which will fail ...

Best, JAN

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/KiCad/kicad-library-utils/issues/133#issuecomment-313365832, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7VR7-b_XHI7nycwGY3P_vQSNXVRW7Qks5sLL9jgaJpZM4ONlc- .

jkriege2 commented 7 years ago

I think I already had problems, when libs were created with older KiCAD versions ... I agree that this might be wasted time, but it might as well be helpful to some and save some time of the librarians! Sometimes commits are also quick fixes by hand inside the files (changed typo ...) ... also in such cases unwanted side-effects may occur and then would be noted before any human has to look at it.

Best, JAN

jkriege2 commented 7 years ago

Last point: Such a check is usually significantly quicker than loading (and reloading) the lib in KiCAD.

bobc commented 7 years ago

It only takes my PC a couple of seconds to load all 93 libraries...

I just think that rewriting chunks of KiCad in Python just in case of some problems that "might" happen is not good "bang for buck". There are 160 outstanding issues+PRs on kicad-library (and not counting footprint libraries) which would be much more useful to address.

If we find regular problems with invalid libs, then it would be worth adding some checks. But here it seems we are looking for problems where there is not really a problem in practice. I'll bet you a beer that none of the existing PRs produce libs that don't load in KiCad :)

On 6 July 2017 at 12:40, Jan W. Krieger notifications@github.com wrote:

Last point: Such a check is usually significantly quicker than loading (and reloading) the lib in KiCAD.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/KiCad/kicad-library-utils/issues/133#issuecomment-313372317, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7VR9O-vHbdDxcGkHKTnljy7G1n3f57ks5sLMewgaJpZM4ONlc- .

SchrodingersGat commented 7 years ago

Rather than make checklib a python copy of KiCad, I suggest that libs should be valid KiCad libs before running checklib

Ideally, yes of course. But the KLC scripts are simply tools to automate the numerous tests that take humans ages to check. There's still a human-in-the-loop who loads the submitted files in KiCad - so that part gets checked too.

The scripts aren't meant to replace such checking. They are to reduce the burden on librarians having to repeat the same checks and suggestions time and time again.

If what you are saying is that we should avoid spending time adding rules that test for such unlikely nuanced cases, then I would agree.

Ratfink commented 6 years ago

In https://github.com/KiCad/kicad-symbols/pull/351, one of these issues (DCM entry with no corresponding symbol) showed up and I had to find it manually. It's an easy mistake to make when fixing symbol names in a text editor. A buggy symbol generator script could make any of these errors as well. I'm in favor of adding tests for these cases, but I think it should be a very low priority.