Extract footprint-center from F.Fab outline #260

ghost · 2018-09-19T11:51:24Z

Some footprints have weird pad shapes, making the F6.2 test fail.
To fix this, we can also calculate the center of the F.Fab layer. Of those
two possible center-locations, the one closer to (0,0) is
selected.
If Pads-center and F.Fab-center differ a warning is displayed.

This fixes the travis error for this PR KiCad/kicad-footprints#951

Some footprints have weird pad shapes, making the F6.2 test fail. To fix this, we can also calculate the center of the F.Fab layer. Of those two possible center-locations, the one closer to (0,0) is selected. If Pads-center and F.Fab-center differ a warning is displayed.

evanshultz · 2018-09-19T12:42:24Z

This should apply only to SMT footprints, since THT footprints have the origin at pin 1.

I didn't look at the code, but I'm not sure what you described above is the best approach. Consider this footprint:

The center of the pads and fab lines need to be considered together. Either one separately wouldn't work. This might be a nice incremental improvement and I'm not against merging it, just saying that it's not a comprehensive fix to consider only pads or only fab lines.

Ratfink · 2018-09-19T13:26:21Z

I'd just like to pop in to point out that barring some miracle (like all manufacturers providing drawings in a standardized machine-readable format), there is no comprehensive fix. This is because a drawing can specify any pick-and-place origin, which need not be the center of any features of the footprint. The best we can hope for is an improved heuristic, resulting in fewer false negatives. So in short, if this is better, of course it should be accepted.

I haven't taken a look at the patch though, so take everything I wrote above as a general statement, not as a recommendation to merge.

ghost · 2018-09-19T13:27:29Z

Hello,

It does not apply to non SMD-Footprints, see Line24. I have not touched that :)

But you are right, this is not a comprehensive fix. There are still valid footprints which will fail. But less will fail with that changeset.

Example: the mentioned HDMI-Footprint the check will not fail, but display a warning.

$ ../kicad-library-utils/pcb/check_kicad_mod.py Connector_HDMI.pretty/HDMI_A_Contact_Technology_HDMI-19APL2_Horizontal.kicad_mod -vv
Checking footprint 'HDMI_A_Contact_Technology_HDMI-19APL2_Horizontal':
  Violating F6.2
    For surface-mount devices, footprint anchor is placed in the middle of the footprint (IPC-7351).
    Footprint centers of pads and F.Fab do not match
     - Footprint center calculated as Pad: (0.0,0.0)mm F.Fab: (0.0, 6.55)mm

ghost · 2019-02-07T13:19:30Z

This has been idle quite some time.
Does anybody want to review/merge it?
I still consider this a small but usefull addition.

poeschlr · 2019-07-28T13:14:18Z

pcb/rules/F6_2.py

+        diff_pads = sqrt(center_pads['x']**2 + center_pads['y']**2)
+        diff_fab  = sqrt(center_fab['x']**2 +  center_fab['y']**2)
+        x = (center_pads['x'], center_fab['x'])[diff_pads > diff_fab]
+        y = (center_pads['y'], center_fab['y'])[diff_pads > diff_fab]


What is the rationale behind this?

Line 43 & 44 do calculate the euclidic distance of the pads & fab centers to the origin.
Then the one closes to the origin is selected. (45-46)

That the acutal idea of the patch. Look at the center of the F.Fab lines the one calculated from the Pads.

If they are not the same, display a warning (line 31-37)

If they are not the same, select the one that is closer to the footprint origin (line 43-36)

Then check if the one selected above is to far from the origin (lines 48-52)

Just a stylistic point: Python has a conditional operator true_value if condition else false_value that would be better to use than subscripting a tuple on lines 45-46. But since there are two lines with the same condition, using a normal if block could be considered as the more DRY option.

tbh. I don't know why I have written it this way. After the question from poeschlr I had to look twice myself. This is a good indication that the code-quality is shitty.
A simple if-statement is way more readable. Will fix.

poeschlr

Sorry for the long delay, this repo is often forgotten

poeschlr · 2020-03-07T09:51:07Z

pcb/rules/F6_2.py

+        center_fab  = module.geometricBoundingBox("F.Fab").center
+
+        if center_pads['x'] != center_fab['x'] or center_pads['y'] != center_fab['y']:
+            self.warning("Footprint centers of pads and F.Fab do not match")


I would not call this a warning. A lot of footprints would result in this and it would just confuse users.

How about moving the warning output into the code-path that displays the error (if there is any). Then the user has the additional info on which two origins were checked.
Otherwise it will be a mystery for the user why exactly the check failed.

Well if the check fails then i would assume your script knows why and can generate a clear output (you could for example word it such that it is clear which options you checked). This warning on its own however might be read as "i need to ensure the centers are equal" which is not the case.

I spend some time on better wording, but could not come up with something good.

Actually I don't know why I introduced it in the first place. There is no real use-case for it.
Now one line is shown for each calculated center. This has all the information the user needs. Its also much clearer than the previous version that had all coordinates in one line.

New look:

ghost · 2020-03-12T16:11:37Z

A good testcase is LED-L1T2_LUMILEDS in LED_SMD (which should get fixed).

The warning "Footprint centers of pads and F.Fab do not match" was not usefull. It could be understood as 'this needs to be fixed'. Also there is no real use-case where this warning actually helps the user. Instead now one line for each check is output which shows the calculated centers. This way easier to read than the previous solution which had all the coordinates in the same line.

poeschlr · 2020-07-18T05:13:30Z

pcb/rules/F6_2.py

+                xf = round(center_fab['x'], 5),
+                yf = round(center_fab['y'], 5)))
+            if center_pads['x'] != center_fab['x'] or center_pads['y'] != center_fab['y']:
+                self.errorExtra("Footprint centers of pads and F.Fab do not match")


This message is still here. It still suggests that the user needs for some reason ensure pads and fab need to be on the same center.

Edit: hm it seems i looked at an outdated diff here.

poeschlr · 2020-07-18T05:16:17Z

Thanks looks good now (ignore my last comment made as i seem to have misclicked and looked on an outdated diff)

ghost mentioned this pull request Sep 19, 2018

Add WDFN-8_2.2x2mm_P0.5mm_1EP KiCad/kicad-footprints#951

Merged

cpresser mentioned this pull request Feb 20, 2019

F6.2 center check does not consider fab lines #242

Open

ghost mentioned this pull request Jul 25, 2019

Add Atmel_ATSAMR21G18-MR210UA_NoRFPads KiCad/kicad-footprints#1744

Merged

5 tasks

poeschlr reviewed Jul 28, 2019

View reviewed changes

Improve F6.2 coding style

36e147a

poeschlr suggested changes Mar 7, 2020

View reviewed changes

poeschlr self-assigned this Mar 7, 2020

poeschlr added the Pending changes label Mar 7, 2020

Rework F6.2 warnings

5aa4135

poeschlr added Ready for review Use this to mark pull requests that are updated but you could not review instantly and removed Pending changes labels Apr 26, 2020

Carsten Presser added 2 commits June 16, 2020 11:58

F6.2 remove plural s from exta error

cb90458

poeschlr reviewed Jul 18, 2020

View reviewed changes

poeschlr merged commit 0084a5f into KiCad:master Jul 18, 2020

antoniovazquezblanco removed the Ready for review Use this to mark pull requests that are updated but you could not review instantly label Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract footprint-center from F.Fab outline #260

Extract footprint-center from F.Fab outline #260

ghost commented Sep 19, 2018

evanshultz commented Sep 19, 2018

Ratfink commented Sep 19, 2018

ghost commented Sep 19, 2018

ghost commented Feb 7, 2019

poeschlr Jul 28, 2019

ghost Jul 29, 2019

Ratfink Jul 29, 2019

ghost Jul 29, 2019

poeschlr left a comment

poeschlr Mar 7, 2020

ghost Mar 12, 2020

poeschlr Jun 11, 2020

ghost Jun 16, 2020

ghost commented Mar 12, 2020

poeschlr Jul 18, 2020 •

edited

poeschlr commented Jul 18, 2020

Extract footprint-center from F.Fab outline #260

Extract footprint-center from F.Fab outline #260

Conversation

ghost commented Sep 19, 2018

evanshultz commented Sep 19, 2018

Ratfink commented Sep 19, 2018

ghost commented Sep 19, 2018

ghost commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poeschlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 12, 2020

poeschlr Jul 18, 2020 • edited

Choose a reason for hiding this comment

poeschlr commented Jul 18, 2020

poeschlr Jul 18, 2020 •

edited