hildogjr / KiCost

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

Parts with sub parts are not propagated #266

Open timoalho opened 6 years ago

timoalho commented 6 years ago

Making a part with sub part, then duplicating otherwise identical parts except for empty manf#, the sub parts are not propagated. See attached .xml and the generated .xlsx.

Expected result: quantity for D1#1 to D1#3 should be 3. Actual result: quantities are 1.

N.B.: I have a faint recollection of seeing an issue on this here, but couldn't find it again...

Tested with commit SHA1 ca269388e9a521710bd1767902173612c6a5aaae SubPartGroupTest.xml.txt (had to add the .txt to make github allow upload)

hildogjr commented 6 years ago

The old case was just a error mine doing len(string)... This case yours have never be reported, the manf# was not propagated. It will need some refacture on group_parts(foo) and subpartqty_split(foo), because it splits the subgroups before propagate. The opposite have to be made: propagate the manf#, manf and cat#s and after split.

When I developed this module I was concerned with different parts that use the same subparts, because this split before groups. This case have to be also in the need code.

Checking the code, parts manf#can be also propagated between semi-identical parts of different projects (in multi files case). In my opinion, this is not good.

timoalho commented 6 years ago

Concerning semi-identical parts in different projects: I agree that if they're combined by default, this could be confusing. However, it might be good to leave that as an option: I can imagine for example making a file with all my standard passives listed once with manfs, and then I could just include that in the KiCost run on other projects, and I won't need to enter these manfs in the main project at all. Thinking a bit further, this would also really need an option to use a file just for manfs, but not for the actual quantities.

To clarify, something like:

kicost -i main.xml --manfs parts.xml

where --manfs causes the matching manfs from parts.xml to be propagated, but no other info.

If you think that could be smart, I could make a feature request issue out of this, to be implemented once you have the time (or I could take a stab at it too, but can't really say yet when I would have the time do it)?

Also, sorry for spamming you with all my bug reports, it's really a show of appreciation that this is a very useful app that I can once again use!

hildogjr commented 6 years ago

Sorry by delay. Yes create another issue, not because of the implementation it self (the code reformulated will allow). But because it will be need a change on the input parameters, and I would like to keep reported the steps of development (also because needs to be compatible with all EDAs already supported).

hildogjr commented 6 years ago

Linked with #304 The group_parts() have to be re-write.

Create a function propagate_parts_manf() that will be call twice to solve this and the issue #302. So:

  1. Read all files (in the case of multi files project), appending the project info (this is done in loop of line 123 of `kicost.py);
  2. propagate_parts_manf() so even parts designed with subpart will be propagated (as requered in this issue). After the complete read in the loop; a. Make a option to propagate of not between different project files;
  3. subpartqty_split() to split the components subparts;
  4. group_parts() that will put together even subparts of different designed ref-part (and the case of one subpart that is the full part of other component!), as already is done now.

Files affected in this change kicost.py (only main loop) and eda_tools.py. Split group_parts() in different functions.

hildogjr commented 5 years ago

Pumping up as a low level bug to evaluate the necessity of correction.

set-soft commented 3 years ago

IMHO is OK not to propagate it. Propagation of values is a hack for lazy people. You should have libraries where components has all the needed fields, and not rely on ambiguous propagations. I understand the propagation mechanism can be handy, but in this case: how do you know that D2 and D3 must also include the subparts? Why D2 and D3 should also have a LED pipe? This is known only by the user. One thing is saying: the user is using LED "LTST-C193KGKT-5A", here we have another LED ... no P/N, ok lets guess this is also "LTST-C193KGKT-5A". Another thing is assuming that all the LEDs are the same and has the same LED pipe and has the same gommet. Too much guessing for my taste.

Again: the correct solution here is to make a library component, lets say "my_LED" with the correct information. Then you just instantiate "my_LED" instead of a generic LED.

And in this case: Why not creating a symbol for the LED pipe and another for the grommet? They can be virtual components in the PCB. And people will see what these part are. KiCad 6 has an explicit flag for components that doesn't belong to the PCB.

hildogjr commented 3 years ago

The propagation is legacy code of time that KiCad-Eeschema (v4 if I remember correctly) didn't have a minimum table symbol field control...

Maybe it is a feature that doesn't make sense anymore. We could mark this as "wont fix" but remains the discussion keep or not the propagation for the not-sub-parts.

set-soft commented 3 years ago

Hi @hildogjr !

I agree with marking it as "wont fix". And I agree this issue can remain open for discussion.

Maybe it could be labeled as enhancement, like you did back in 2018. Because the current propagation isn't intended to go that far. Both are better than marking it as a bug.

hildogjr commented 3 years ago

Also this workflow of subpart could suffer some change, I think is planned some better MPN control in Eeschema for v7.