search

SchrodingersGat / KiBoM

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

add support for sheetpath field in BOM #126

Closed hkennyv closed 3 years ago

hkennyv commented 3 years ago

Hi there,

I've implemented adding a sheetpath column to the output BOM. currently, i just added it as a default column because it wasn't clear to me right away how to add it without it being default. Also, to avoid accidentally grouping components together with different sheetpaths, I had to it to the default GROUP_FIELDS section in the bom.ini.

Is this something that could be added to KiBoM? If so, could I get a review please? :)

ref #125

hkennyv commented 3 years ago

Thanks for the help! See my changes

As for the use case, perhaps my use is a contrived example. I needed to analyze cost of certain circuits in the design (organized by sheets in kicad eeschema). Being able to group line items based on the Sheetpath column allowed me to figure that out pretty easily.

set-soft commented 3 years ago

Thanks for the help! See my changes

Good! Lets wait for Oliver opinion.

As for the use case, perhaps my use is a contrived example. I needed to analyze cost of certain circuits in the design (organized by sheets in kicad eeschema). Being able to group line items based on the Sheetpath column allowed me to figure that out pretty easily.

Nice. I'm doing a KiBoM alternative that directly uses the schematic, no need for the XML netlist, and I added the sheetpath as a possible column.

SchrodingersGat commented 3 years ago

Hey @hkennyv this is an interesting idea. I would like to see it only as an "optional" output, e.g. with a command line flag or setting an option in the .ini file.

By default, components should definitely not be split by sheet. I can see potential use cases, so I'd support having it in as an option, but only if the default is "off".

set-soft commented 3 years ago

Hi! @SchrodingersGat After the review the column is not displayed if you don't use an INI file. And it was removed from the grouping list. You get the column when using an INI file with an explicit IGNORE_COLUMNS that doesn't mention this column. This is a normal case for an already existing INI file. And IMHO is a consequence of the mechanism used to configure KiBoM, you don't say which columns must be used, you say which ones are excluded. Every time you add a new attribute to your components you must review the columns.

hkennyv commented 3 years ago

Hey @SchrodingersGat, thanks for the feedback. I agree that the default doesn't make sense to have components grouped by sheet.

@set-soft gave a good review and now the PR implemented as-is doesn't change the default output of KiBoM as you expect. The only noticeable change is the bom.ini now has "Sheetpath" in the "IGNORE_COLUMNS" section by default, allowing users to enable it by removing it from that section.

SchrodingersGat commented 3 years ago

Thanks for the clarification guys. I've now had a chance to test this, and it appears to work as expected. Thanks for the contribution!