"Check for off grid pins" doesnt work properly

[Frank Severinsen]

Hopefully it's not just me being stupid or misunderstood the use.

Steps to replicate.

Open a symbol with all pins on 50 mil grid.
Select 25 mil grid and move a pin 25 mil eg to the left and up.
Select 50 mil grid, or any other grid.
Run off grid check
No errors eventhough the pin is not on grid.

I can get the error to trigger with other offset's but not with 25 mil

Application: kicad
Version: 5.1.0-rc2-unknown-036be7d~91~ubuntu18.10.1, release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.61.0 OpenSSL/1.1.1 zlib/1.2.11 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.4) nghttp2/1.32.1 librtmp/2.3
Platform: Linux 4.18.0-15-generic x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8) GTK+ 3.24
    Boost: 1.67.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.61.0
    Compiler: GCC 8.2.0 with C++ ABI 1013
Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=ON
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=ON
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=ON
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

[John Beard (john-j-beard)]

I see this too.

[Wayne Stambaugh (stambaughw)]

Before anyone does anything rash. I don't think this is technically a bug. There is nothing constraining pin spacing other than that they have to be on grid in order to ensure wires in the schematic editor can connect to them. The grid spacing can be 100 mils, 50 mils, 25 mils, etc as long as the pin connection point is on some known grid spacing. My guess is that you feel that all pins should be on a 50 mil grid which is the default for all of our library symbols but this is not inherently true for either the symbol editor or the schematic editor so changing this may break users symbols that are not on 50 mil grid spacing.

[Frank Severinsen (shack)]

I can see in the code it's hardcoded to check against 25 mil spacing

I would expect the check to check against whatever grid is selected.
It seems rather unintuitive to look at a pin not on grid, while the check claims otherwise.
Same the other way around

[Wayne Stambaugh (stambaughw)]

Logically you are correct. Practically is a different story. 25 mils was most likely chosen because that is the smallest reasonable grid size that you can use and still ensure that you can connect to the pins in the schematic editor. Grid sizes 10, 5, 2, and 1 mils are going to cause issues unless you are incredibly careful when placing pins. Change this at your own peril. I'm reasonably sure that there will be a bug report about not being able to connect a wire to a symbol pin not long after this change is made. Once wire snapping is implemented in the schematic editor during v6 development, this issue goes away so I am tempted to leave this alone until then.

[Frank Severinsen (shack)]

I agree the lower grids is not really usable.
Only reason I stumbled across this, was I personally prefer using 50 mil on my own symbols.
Unfortunately I had moved a lot of parts around with 25 mil grid, and had to go back and forth between Eeschema and symbol editor, whenever I couldn't connect the pin on 50 mil grid.
Found it pretty frustrating to not be able to catch the 25 mil pins with the check.

Could we perhaps check against grid if grid is higher than 25 mil?

[Seth Hillbrand (sethh)]

I like that solution.

[Wayne Stambaugh (stambaughw)]

@Frank, I can live with that. You may want to warn the user when clamping the minimum grid to 25 mils so the user knows that the current grid is not used for the on grid test. We are in string freeze so this fix would have to wait until after 5.1 is release. It is certainly something that can be cherry picked into 5.1.1.

[Frank Severinsen (shack)]

So, I made a patch which takes the current grid, clamps it to 25.
I have also almost finished the version which adds some info about the clamping to the user.
Thought this could perhaps be added to master now, and the user warning could be added in 5.1.1

As always feel free to give some critique if something should be done differently

[Seth Hillbrand (sethh)]

I'm fine with this approach. The only minor bits are:

1) Can you fix the capitalization formatting? I know MIN_GRID_SIZE is all-caps but it shouldn't be. It is a local variable so it should have the first character lower case and the rest either lower or camel case.

2) You have an extra line of spaces.

3) Grid size is an x/y pair of reals. You should cast the value you want explicitly to int and then use that for comparison. You want to use KiROUND() for this (note the capitalization of 'i')

[Frank Severinsen (shack)]

@Seth

Thanks for having a look.

1) I thought it looked a bit funny, but tried not to touch too much. They are all lowercase now.

2) Should be removed

3) Thanks, Done

[Wayne Stambaugh (stambaughw)]

@Frank, your second patch looks good except for some trailing white space left behind by your editor. I cleaned up the patch and I have it ready to merge as long as there are no other objections. In the future, please use `git format-patch` to submit patches, this makes life a bit easier to for the KiCad devs. Thank your for your contribution to KiCad.

[Seth Hillbrand (sethh)]

No objections. Looks good except for the whitespace missing around KiROUND

[Wayne Stambaugh (stambaughw)]

@Seth, good catch. I fixed the missing whitespace and pushed the patch.

[KiCad Janitor (kicad-janitor)]

Fixed in revision 71a62bb7821ddecde6c39b6e81dc62b60c88fc6b
https://git.launchpad.net/kicad/patch/?id=71a62bb7821ddecde6c39b6e81dc62b60c88fc6b