hildogjr / KiCost

Build cost spreadsheet for a KiCad project.
MIT License
520 stars 97 forks source link

kicost.VARIANT:manf# field doesn't work #370

Closed mdeweerd closed 4 years ago

mdeweerd commented 5 years ago

The documentation says:

KiCost supports another way of specifying the variant associated with a part. Using the example from above, labeling the part number for J1 as kicost.v1:manf# will assign it to the v1 variant. This method is not as flexible as using the variant field and may be removed in future versions of KiCost .

I think it has to be maintained as this seems to be the only way to select different components according to the variant. The "modern variant" field only allows to specify if the component is present or not in the BOM.

The Old style variant specification allows to select one type of connector in one case, an another type in another case, a smaller fuse in one case or a bigger one, different resistor values according to the variant, etc.

So it seems best to maintain it, update the documentation and add a reminder in the source code -).

hildogjr commented 5 years ago

Yes, @mdeweerd I use in same circuit problem. I assembly different low-pass filters frequency (anti-aliasing) depending of the signal that I want to mesuare or application.

About the documentation, I had reviewed something but because I coded the features, it is all "clear" to me. I think an other person should review the KiCost documentations about installation, use, features, the GUI...

I think old and new could create the interpretation. This should be change to "method 1" and "method 2" in the docs.

mdeweerd commented 4 years ago

I just tried this "Old style variant" specification and it does not work as I expected: image

I specified "64k" in the variant option ( "^(64k|inp1)$" ) but I still get the normal value in the excel file.The compoent attribute "kicost.64k:manf#" is set to be "STM32F030C8T6TR" so I expect that to override the normal value when "64k" is set as a variant for this component.

I am using Kicost 1.0.4 installed using pip (the current github version does not work for me).

Should I specify this differently? What can I do (in 1.0.4)?

hildogjr commented 4 years ago

This code part remains unchanged since some pre-alpha (versions 0.1.x). Could you attach the XML (it can be by a design generated with just this single part).

mdeweerd commented 4 years ago

Hi I am using Kicost 1.1.2 in fact - my kicost installation was not where I expected.

The following zip contains a testcase.

DebugSample.zip

mdeweerd commented 4 years ago

It looks like I have a partial explication - the "variant" regex is not always interpreted in the same way... Explication: From tools.py: if re.match(variant, v, flags=re.IGNORECASE):

In this case a partial match is possible.

From eda_kicad.py:

                    key_re = 'kicost(\.{})?:(?P<name>.*)'.format(variant)
                    mtch = re.match(key_re, name, flags=re.IGNORECASE)

In this case a full match is required.

My regex for the variant looks like this: "^(64k|inp1)$"

I do this because I only want to match full names and because there are variants like "inp1_option1" - I do not want inp1 to match.

This means that the regex that is matched too looks like: 'kicost(\.^(64k|inp1)$)?:(?P<name>.*)'

The "^" and "$" tokens are not welcome in this context.

So the test of the variant should be changed in eda_kicad.py

mdeweerd commented 4 years ago

I fixed some code, but unfortunately, the matching also depends on order:

      <fields>
        <field name="desc">Microcontroller CI STM32F0 ARM Cortex -M0 32-Bit 48MHz FLASH 32Ko (32K x 8) 48-LQFP (7x7)</field>
        <field name="kicost.64k:manf#">STM32F030C8T6TR</field>
        <field name="kicost.64k:value">STM32F030C8T6TR</field>
        <field name="kicost.64k:desc">Microcontroller CI STM32F0 ARM Cortex -M0 32-Bit 48MHz FLASH 64KB (64K x 8) 48-LQFP (7x7)</field>
        <field name="manf">STMicroelectronics</field>
        <field name="manf#">STM32F030C6T6TR</field>
      </fields>

After adding the 'value' and 'desc' fields to the list to avoid them being "local":

                        if name not in ('manf#', 'manf', 'desc', 'value') and name[:-1] not in distributor_dict:
                            if SEPRTR not in name: # This field has no distributor.
                                name = 'local:' + name # Assign it to a local distributor.

I got the expected description and value in the spreadsheet, but the manf# overrides the "kicost.64k:manf#" value . I'll add a test and only assign the values if the key does not exist yet (if the value is not based on a variant).

mdeweerd commented 4 years ago

While trying to reduce the code changes I note that the original regex is like this:

key_re = 'kicost(\.{})?:(?P<name>.*)'.format(variant)

and that the code assumes that name is a distributor unless it matches manf or manf#.

                        # If the field name isn't for a manufacturer's part
                        # number or a distributors catalog number, then add
                        # it to 'local' if it doesn't start with a distributor
                        # name and colon.
                        if name not in ('manf#', 'manf') and name[:-1] not in distributor_dict:
                            if SEPRTR not in name: # This field has no distributor.
                                name = 'local:' + name # Assign it to a local distributor.

The regex matches:

So the original code "breaks" when "name" is not related to a distributor, so when ":" is missing in name. I am going to change the code further with that in mind.