SchrodingersGat / KiBoM

Configurable BoM generation tool for KiCad EDA (http://kicad.org/)
MIT License
352 stars 95 forks source link

exception on variant matching #101

Open michael-betz opened 3 years ago

michael-betz commented 3 years ago

I'm getting an exception when running KiBoM, using the +<VARIANT> and -<VARIANT> syntax in the config field:

KiBOM version 1.7.1
 Output directory: '/home/michael/kicad/Marble-Mini'
 Input: /home/michael/kicad/Marble-Mini/AMC_FMC_Carrier-PcbDoc.xml
 Configuration file: /home/michael/kicad/Marble-Mini/bom.ini
 PCB variant: standalone
Traceback (most recent call last):
  File "/usr/lib/python3.8/pdb.py", line 1701, in main
    pdb._runmodule(mainpyfile)
  File "/usr/lib/python3.8/pdb.py", line 1546, in _runmodule
    self.run(code)
  File "/usr/lib/python3.8/bdb.py", line 580, in run
    exec(cmd, globals, locals)
  File "/home/michael/kicad/KiBoM/kibom/__main__.py", line 3, in <module>
    """
  File "/home/michael/kicad/KiBoM/kibom/__main__.py", line 189, in main
    result = writeVariant(input_file, output_dir, output_file, variant, pref)
  File "/home/michael/kicad/KiBoM/kibom/__main__.py", line 54, in writeVariant
    groups = net.groupComponents(components)
  File "/home/michael/kicad/KiBoM/kibom/netlist_reader.py", line 344, in groupComponents
    if g.matchComponent(c):
  File "/home/michael/kicad/KiBoM/kibom/component.py", line 503, in matchComponent
    if c == self.components[0]:
  File "/home/michael/kicad/KiBoM/kibom/component.py", line 117, in __eq__
    if self.isFixed() != other.isFixed():
  File "/home/michael/kicad/KiBoM/kibom/component.py", line 354, in isFixed
    if opt[1:].lower() == self.prefs.pcbConfig.lower():
AttributeError: 'list' object has no attribute 'lower'
Uncaught exception. Entering post mortem debugging

this patch fixes it

diff --git a/kibom/component.py b/kibom/component.py
index 2eef873..0943855 100644
--- a/kibom/component.py
+++ b/kibom/component.py
@@ -346,12 +346,12 @@ class Component():

         for opt in opts:
             # Options that start with '-' are explicitly removed from certain configurations
-            if opt.startswith('-') and opt[1:].lower() == self.prefs.pcbConfig.lower():
+            if opt.startswith('-') and opt[1:].lower() in self.prefs.pcbConfig:
                 result = False
                 break
             if opt.startswith("+"):
                 result = False
-                if opt[1:].lower() == self.prefs.pcbConfig.lower():
+                if opt[1:].lower() in self.prefs.pcbConfig:
                     result = True

         # by default, part is not fixed
t3hcatpaw commented 3 years ago

I've found some more potential issues in the original code and in the patch above but I'm not 100% sure about the DNC stuff because I've never come across that property before. I've already committed fixes and a new feature to a fork of your repo but would like to discuss this before submitting a PR to make sure my understanding is correct.

For what I understand so far, a part marked DNC is considered to never change in any variant, effectively meaning, 'always populate.' In that case, and according to the function name, isFixed() should return True whenever a part is indeed fixed, i.e.

Is that correct?

set-soft commented 3 years ago

The meaning is different: parts marked as DNC shouldn't be replaced by generic equivalents. Is just a special mark in the BoM list to inform that this component is critical. Look PR #106 for the correct implementation. I introduced it in PR #84 but it got broken when self.prefs.pcbConfig changed from string to list. It also contained stuff related with variants just because I used isFitted as template. To make things worst the meaning is inverted isFixed was returning False for DNC components. It worked because the code used dnc=" (DNC)" if not self.isFixed() else "") (not)

SchrodingersGat commented 3 years ago

@t3hcatpaw has this been fixed for you now with the update in #106 ?

set-soft commented 3 years ago

This should be fixed by #105 The exception was in isFixed:

  File "/home/michael/kicad/KiBoM/kibom/component.py", line 117, in __eq__
    if self.isFixed() != other.isFixed():
  File "/home/michael/kicad/KiBoM/kibom/component.py", line 354, in isFixed
    if opt[1:].lower() == self.prefs.pcbConfig.lower():

This comparisson was removed by #105