search

SchrodingersGat / KiBoM

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

Fix variant logic for '+'-prefixed board variants #136

Closed romavis closed 3 years ago

romavis commented 3 years ago

Hello,

I've found something that looks like a bug in board variant processing. Readme.md describes the following example:

e.g. if we have a PCB with three components that have the following values in the fit_field field:

    C1 -> "-production,+test"
    C2 -> "+production,+test"
    R1 -> ""
    R2 -> "-test"

If the script is run with the flag --variant test, then C1, C2 and R1 will be loaded.

However, with existing code this doesn't quite work. The relevant piece in Component.isFitted() method is:

            if opt.startswith("+") and opt[1:] not in self.prefs.pcbConfig:
                return False

Which means that when it checks opt == '+production' with self.prefs.pcbConfig == ['test'] (the only variant specified on command line), the component will be discarded, which is not what we want here... On the next loop iteration opt value will be +test that should allow component to stay in the BOM, but it never checks for that.

This PR should fix the issue. Sorry for removal of unrelated trailing whitespace in a few places - it's just how my editor is configured...

romavis commented 3 years ago

I will follow up with feedback for rest of your questions a bit later, but for now I just want to mention that the usecase mentioned in my message is not something that I've invented myself - it was quoted from README.md in this repository.

The problem comes with C2. You say only added to production and only added to test, which generates the confusion, but we could interpret as only added to (test or production). And here is where I think you have a strong point.

From my point of view there's no confusion here. Documentation (README.md) defines an expected behavior which is not followed by the implementation. Whether the behavior stipulated by README makes sense is a different question, but IMHO it does :-)

romavis commented 3 years ago

I've pushed a commit to restore a missing DNF check. Please let me know if you see any more issues with this fix.

SchrodingersGat commented 3 years ago

@RomaVis thanks for this improvement, looks like there was a flaw in the logic here.

@set-soft do you have any further comments on this one?