Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parts with sub parts are not propagated #266

Open
timoalho opened this issue Jun 7, 2018 · 9 comments
Open

Parts with sub parts are not propagated #266

timoalho opened this issue Jun 7, 2018 · 9 comments
Labels
enhancement Improvements on features already existent. wont fix Features that will be not implemented or that diverges from KiCost context.

Comments

@timoalho
Copy link
Contributor

timoalho commented Jun 7, 2018

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 ca26938
SubPartGroupTest.xml.txt (had to add the .txt to make github allow upload)

@hildogjr hildogjr self-assigned this Jun 8, 2018
@hildogjr
Copy link
Owner

hildogjr commented Jun 8, 2018

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
Copy link
Contributor Author

timoalho commented Jun 8, 2018

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 hildogjr changed the title Parts with sub parts are not grouped Parts with sub parts are not propagated Jun 13, 2018
@hildogjr hildogjr added the enhancement Improvements on features already existent. label Jun 22, 2018
@hildogjr
Copy link
Owner

hildogjr commented Jun 29, 2018

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
Copy link
Owner

hildogjr commented Aug 4, 2018

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 hildogjr removed their assignment May 3, 2019
@hildogjr hildogjr added this to the 2.1 milestone May 3, 2019
@hildogjr hildogjr added bug Bugs that impacts on main KiCost functionality. and removed enhancement Improvements on features already existent. labels Jun 14, 2019
@hildogjr
Copy link
Owner

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

@hildogjr hildogjr modified the milestones: 1.2, Group improvement Jul 17, 2019
mdeweerd pushed a commit to mdeweerd/KiCost that referenced this issue May 4, 2021
The manf# and manf field should propagate for SubParts too.
The results of the testcase are still not as expected.
@set-soft
Copy link
Collaborator

set-soft commented May 5, 2021

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
Copy link
Owner

hildogjr commented May 5, 2021

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
Copy link
Collaborator

set-soft commented May 5, 2021

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 hildogjr added enhancement Improvements on features already existent. wont fix Features that will be not implemented or that diverges from KiCost context. and removed bug Bugs that impacts on main KiCost functionality. labels May 5, 2021
@hildogjr
Copy link
Owner

hildogjr commented May 5, 2021

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

set-soft added a commit that referenced this issue May 6, 2021
The manf# and manf field should propagate for SubParts too.
The results of the testcase are still not as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements on features already existent. wont fix Features that will be not implemented or that diverges from KiCost context.
Projects
None yet
Development

No branches or pull requests

3 participants