Fix variant logic for '+'-prefixed board variants

[romavis]

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...

[set-soft]

I think you have a good point here.
Ok, let me see this. First, I tried your code on the regression tests of my fork and it failed 3 out of the 5 tests. So even when you are right about the problem something is not quite ok ;-)

Let me try to understand the definitions:

• C1: Your definition is confusing, why -production? This isn't needed. If this is a component that will be added only for test using +test should be enough. Note that - is exclude, but + isn't just include, is include only.
• R1: Is a default component, found in all variants.
• R2: Will be excluded from test

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.

In order to make your code work I'll suggest using the following for the second loop:

        exclusive = False
        for opt in opts:
           # Options that start with '+' are fitted only for certain configurations  
            if opt.startswith("+"):
                exclusive = True
                if opt[1:] in variants:
                    return True
        # No match
        return not exclusive

Also note that now the two loops can be joined.
In case you are interested I added 3 regression tests using your example here: KiBot

The tests are test_int_bom_variant_t2_* I run them using:

$ make single_test SINGLE_TEST=test_int_bom_variant_t2

Your example is here: kibom-variant_2.sch
Let me know if this is properly interpreted.

[set-soft]

Why a loop here? The current code uses another (recycled) loop for this.

[romavis]

I didn't want to duplicate calls to strip() in both loops, so factored it out.

[set-soft]

Why not reinserting the spaces to make the patch more readable? you can searate the space removing in other patch.

[set-soft]

Why? the config field is used by other stuff, just think about this:

Config="-V2,DNC"

And you are not specifying variants. With this False the component will be excluded, right?

[romavis]

You're right, the behavior is definitely wrong. I will look into it...

[romavis]

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 :-)

[set-soft]

Hi @romavis !
I think you removed the check for DNF values mixed with other values and separated by comma.
I'm proposing an alternative solution, using the code I suggested before.
This PR is applied to the following fork: https://github.com/INTI-CMNB/KiBoM
BTW: if you are using variants I'll sugest you visiting: https://inti-cmnb.github.io/kibot_variants_arduprog/

[set-soft]

What about this check?

[romavis]

Another good catch! I got confused by check above which does almost the same, but that's for space-separated options...

[romavis]

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]

@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?