Skip to content
This repository has been archived by the owner on Oct 2, 2020. It is now read-only.

Handle footprint name as string #336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akosmarton
Copy link

@akosmarton akosmarton commented Sep 13, 2020

If the footprint name contains only numeric characters, it should be converted to string to ensure the proper rules checking.

@akosmarton akosmarton changed the title Handle module name as string Handle footprint name as string Sep 13, 2020
@chschlue chschlue added the Bug label Sep 13, 2020
Copy link
Contributor

@cpresser cpresser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks simple enough.
But I have no clue why this change is needed. Can you elaborate on that?
Do you have an example footprint where this patch changes the outcome of the check script?

@@ -29,7 +29,7 @@ def checkMissingValue(self):
self.error("Missing 'value' field")
return True

if not val['value'] == mod.name:
if not str(val['value']) == mod.name:
errors.append("Value text should match footprint name:")
errors.append("Value text is '{v}', expected: '{n}'".format(
v = val['value'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also convert to str here.

@@ -49,7 +49,7 @@ def check(self):
self.error("footprint name (in file) was '{0}', but expected (from filename) '{1}'.\n".format(module.name, os.path.splitext(os.path.basename(module.filename))[0]))
err = True

if module.value['value'] != module.name:
if str(module.value['value']) != module.name:
self.error("Value label '{lbl}' does not match filename '{fn}'".format(
lbl=module.value['value'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example Molex uses numeric-only part numbers.

image

You can see the error message below if the module name contains only numeric characters:

./check_kicad_mod.py ~/work/sharkrf-kicad-library/footprints/srf_Molex.pretty/5051100892.kicad_mod
Traceback (most recent call last):
  File "./check_kicad_mod.py", line 117, in <module>
    rule.check()
  File "/home/makos/work/kicad-library-utils/pcb/rules/F7_4.py", line 140, in check
    return any([self.checkPads(module.pads)])
  File "/home/makos/work/kicad-library-utils/pcb/rules/F7_4.py", line 46, in checkPads
    exposed_pad_search = re.search('\-\d+[EP]{2}', fpName, re.IGNORECASE)
  File "/usr/lib64/python3.8/re.py", line 201, in search
    return _compile(pattern, flags).search(string)
TypeError: expected string or bytes-like object

@evanshultz
Copy link
Collaborator

That is not a valid footprint name for this library. See our footprint naming scheme at https://kicad-pcb.org/libraries/klc/F3.6/. And these check scripts are for those guidelines/rules. So as @cpresser said, this doesn't hurt anything but it's not needed when building footprints that conform to the rules of this library.

@cpresser
Copy link
Contributor

I think this is a use full addition anyway. The scripts should not throw a cryptic error message just because the user put in stupid input.

The next logical step is to create a F3.py that actually checks that rule. I just skimmed over the F3 section of KLC, and it seems there is no way a footprint can only contain numbers. So we could check that.
And perhaps throw a warning if its number of characters is <3.
And throw errors if it contains evil code-points (thinking of emojis, german umlauts, ...)

@cpresser cpresser self-assigned this Sep 14, 2020
@chschlue
Copy link
Contributor

german umlauts

Those are still considered evil?
SCNR

@chschlue
Copy link
Contributor

Bad jokes aside, we might have to (and be able to) relax naming rules. See KiCad/kicad-symbols#2938

@cpresser
Copy link
Contributor

Nevertheless, we can have a script output a warning if non latin chars are used.
A good check would verify that any char is present in the kicad font. Basically an explicit whitelist.

Somewhat off-topic
Footprint-names are also file-names. Yay!
I have no clue how the Pi would do as filename for windows/linux/mac users. With different locales even more complication could arise.

@chschlue
Copy link
Contributor

chschlue commented Sep 14, 2020

Yes, I'm not opposed to this PR.

OT: True, the π example is a symbol. But all supported platforms can handle multibyte filenames and assuming there have already been users calling their schematics MyCool80'sHeävÿMëtälPCB.sch or something without issue, I guess KiCad's codebase isn't too brittle.

Even more OT: I wonder how many ERP systems these π parts broke along the supply chain ;)

Edit: Not to mention Cyrillic/Greek/Arabic/whatever users in general.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants