search

KiCad / kicad-symbols

Official KiCad schematic symbol libraries for Kicad 5
https://kicad.github.io/symbols
Other
700 stars 747 forks source link

Many "small" symbols have wrong pin-label-offset <20mil #260

Open jkriege2 opened 6 years ago

jkriege2 commented 6 years ago
poeschlr commented 6 years ago

We could also make the script more intelligent. If there is no pin name than the offset for it does not really make sense.

jkriege2 commented 6 years ago

yes, but getting those values to a good default value makes sense to me!

evanshultz commented 6 years ago

@jkriege2 I just merged #242. Can you merge and make sure that symbol conforms before I merge this?

Edit: Oops, I was thinking you made some change based on your comment above.

Ratfink commented 6 years ago

I'd prefer to make the symbols follow the existing rules by setting the offset to 20 mil where appropriate. Yes, the scripts could be changed to detect if no pin text is actually rendered and allow 0 offset then, but that would be a whole complicated new code path to maintain, and a new addition to KLC to write and make sure contributors understand, just to avoid the one-time work of adding some 2's to the libraries.

Of course, I consider this a low-priority item that could wait until after 5.0, but I'd just like to add my thoughts.

evanshultz commented 6 years ago

It would make sense to just have the symbols all be 20mil, but that doesn't work in all cases. Open the below symbol in KiCad, change pin name offset to 20mil, and save. Check the LIB file. It's 1mil again.

I'm not sure why, but I've only seen this on FET symbols. A diode doesn't do it and bigger black-box symbols don't either. I noticed this a while ago but didn't comment on it above.

I also prefer getting everything to 20mil so we meet KLC, but it won't stay at 20mil for all symbols.

It feels like a potential bug in KiCad? Can anybody else confirm? I'll submit it if so.

Application: kicad Version: (5.0.0-rc2-dev-114-g8fcbb64a4), release build Libraries: wxWidgets 3.0.3 libcurl/7.54.1 OpenSSL/1.0.2l zlib/1.2.11 libssh2/1.8.0 nghttp2/1.23.1 librtmp/2.3 Platform: Windows 7 (build 7601, Service Pack 1), 64-bit edition, 64 bit, Little endian, wxMSW Build Info: wxWidgets: 3.0.3 (wchar_t,wx containers,compatible with 2.8) Boost: 1.60.0 Curl: 7.54.1 Compiler: GCC 7.1.0 with C++ ABI 1011

Build settings: USE_WX_GRAPHICS_CONTEXT=OFF USE_WX_OVERLAY=OFF KICAD_SCRIPTING=ON KICAD_SCRIPTING_MODULES=ON KICAD_SCRIPTING_WXPYTHON=ON KICAD_SCRIPTING_ACTION_MENU=ON BUILD_GITHUB_PLUGIN=ON KICAD_USE_OCE=ON KICAD_SPICE=ON

# C3M0030090K
#
DEF C3M0030090K Q 0 0 Y N 1 F N
F0 "Q" 200 75 50 H V L CNN
F1 "C3M0030090K" 200 0 50 H V L CNN
F2 "Package_TO_SOT_THT:TO-247-4_Vertical" 0 0 50 H I C CIN
F3 "" 0 0 50 H I L CNN
ALIAS C3M0065100K C3M0075120K C3M0120100K
$FPLIST
 TO?247*
$ENDFPLIST
DRAW
C 65 0 111 0 1 10 N
C 100 -70 11 0 1 0 F
C 100 70 11 0 1 0 F
P 2 0 1 0 -100 0 10 0 N
P 2 0 1 0 30 -70 100 -70 N
P 2 0 1 10 30 -50 30 -90 N
P 2 0 1 0 30 0 100 0 N
P 2 0 1 10 30 20 30 -20 N
P 2 0 1 0 30 70 100 70 N
P 2 0 1 10 30 90 30 50 N
P 2 0 1 0 100 -70 100 -100 N
P 2 0 1 0 100 -70 100 0 N
P 2 0 1 0 100 100 100 70 N
P 2 0 1 0 145 15 115 15 N
P 3 0 1 0 -100 -100 -100 -150 100 -150 N
P 3 0 1 10 10 75 10 -75 10 -75 N
P 4 0 1 0 40 0 80 15 80 -15 40 0 F
P 4 0 1 0 100 -70 130 -70 130 70 100 70 N
P 4 0 1 0 130 15 115 -10 145 -10 130 15 N
X D 1 100 200 100 D 50 50 1 1 P
X S 2 100 -200 100 U 50 50 1 1 P
X DS 3 -200 -100 100 R 50 50 1 1 P
X G 4 -200 0 100 R 50 50 1 1 I
ENDDRAW
ENDDEF
#
Ratfink commented 6 years ago

Oh, I think I see what's happening there! You can't change the offset for symbols with "Place pin names inside" disabled. This seems fine to me, since the offset is only for when the pin names are inside the symbol. So I guess we do need an exception in the library-utils and KLC, at least for symbols whose pin names aren't inside.

It would make sense for the offset entry to be insensitive when "Place pin names inside" is unchecked, since it has no effect and changes to it are lost after clicking OK, so that should probably be mentioned to the developers.

evanshultz commented 6 years ago

Perhaps. I tried it on a diode symbol thinking the same thing, but the diode saved the 20mil pin name offset. Did you try other symbols without pin names inside?

Ratfink commented 6 years ago

I tried on a few other symbols (diodes, ICs) and it worked the same way. Pin names inside→offset can be changed, ¬Pin names inside→offset always 1.

Looking at the source code, I understand now! Within the library file, an offset of 0 disables "Place pin names inside," and any other value enables it and uses that offset. So an offset of 0 should always be allowed, because that just means "Place pin names inside" is disabled, and that setting is not forbidden by KLC. I suggest we change KLC to clarify this, and I'll make a PR to the library-utils to not complain about 0 offset shortly.

Ratfink commented 6 years ago

Script PR created: https://github.com/KiCad/kicad-library-utils/pull/235

evanshultz commented 6 years ago

Yep. That explains the diode I found which saved the pin offset.

But there may be more, as well. Did anyone try to open the diode symbol I posted above, changed pin name offset to 20mil, and then save the library? I always see the offset go back to 1. This seems like a separate issue. It has pin names placed outside.

evanshultz commented 6 years ago

Oh, I think I see what's happening. The UI doesn't allow '0' to be shown. So even if the line of the symbol is DEF C3M0030090K Q 0 0 Y N 1 F N, it will display the offset as 1 in the UI if pin names are placed outside.

Ratfink commented 6 years ago

Now that we've cleared up part of the problem, and concluded that S3.6 needs clarification as to when exactly it applies, I'm fine with adding the additional condition that the rule only applies when draw_pinname is set to Y, which should remove most of the warnings this issue is about.

Ratfink commented 6 years ago

I've created another PR, https://github.com/KiCad/kicad-library-utils/pull/236, that checks if pin names are drawn before checking the pin name position offset. This would be a KLC change, so I'd like to see some consensus that this is the right move before merging!

Ratfink commented 6 years ago

With both https://github.com/KiCad/kicad-library-utils/pull/235 and https://github.com/KiCad/kicad-library-utils/pull/236 merged, I feel like anything the scripts report now could reasonably be considered an actual library bug. Do we all agree, or is there some other case that the scripts should be smarter about?

One case I see now is e.g. Device:C, where the pin name position offset is 10 mil, the pin names are set to be drawn, but neither pin has a name. I suppose it could be argued that this should be allowed as well, but I consider this to be a library problem (if there are no named pins, why are pin names set to be drawn?).

The only other case I can think of is if all the pins that have names are invisible. But again, if no named pins are being drawn, why are pin names set to be drawn?