bennymeg / Fabrication-Toolkit

An JLC PCB Fabrication Plugin for KiCad
Apache License 2.0
311 stars 53 forks source link

BOM generation broken on some PCBs #148

Closed TheColorman closed 5 months ago

TheColorman commented 6 months ago

BOM generation was broken in 88ae87bf48d12ef76094094ce9dfae731e9aca18 (not stable release). After this commit, BOM is only generated if "Exclude DNP components" is checked.

I'm not 100% sure why as I have a bit of a hard time wrapping my head around the KiCad api, but thought I'd make an issue regardless as I won't have time to look into it for a few days.

bennymeg commented 5 months ago

Could you please test again with the latest release.

CreativeRobotics commented 5 months ago

I just installed the latest release and now BOM and position generation is broken when "exclude DNP.." is selected.

fh-nbt commented 5 months ago

Can confirm the same on my machine and setup. No BOM or position files are being generated. Win11 Kicad 8.1 Fabrication-Toolkit 4.30

bennymeg commented 5 months ago

Does some part get excluded or the entire files do not get generated? Could you please test again with the 4.4.0 pre-release.

fh-nbt commented 5 months ago

Unfortunately, I only get the following outputs on the 4.4 pre-release:

Just to check and be sure, I went back to 4.0 and there I still get the BOM output.

bennymeg commented 5 months ago

I traced the bug to here: #128 The added condition seems to misbehave, some more exploration needed.

msalau commented 5 months ago

@bennymeg I traced the issue to the commit https://github.com/bennymeg/Fabrication-Toolkit/commit/88ae87bf48d12ef76094094ce9dfae731e9aca18

            skip_footprint = ((footprint.GetAttributes() & pcbnew.FP_EXCLUDE_FROM_POS_FILES)
                                or footprint.GetPadCount() == 0
                                or exclude_dnp
                                    and (footprint_has_field(footprint, 'dnp')
                                        or (footprint.GetValue().upper() == 'DNP')
                                        or getattr(footprint, 'IsDNP', bool))
            )

The issues I see:

  1. getattr(footprint, 'IsDNP', bool) is not called anymore. It was getattr(footprint, 'IsDNP', bool)() but became getattr(footprint, 'IsDNP', bool) and since it is a function reference and is not None - it evaluates to True
  2. not enough parentheses

How to fix:

            skip_footprint = ((footprint.GetAttributes() & pcbnew.FP_EXCLUDE_FROM_POS_FILES)
                                or footprint.GetPadCount() == 0
                                or (exclude_dnp
                                    and ((footprint_has_field(footprint, 'dnp')
                                        or (footprint.GetValue().upper() == 'DNP')
                                        or getattr(footprint, 'IsDNP', bool)())))
            )

I've tested the fix on KiCAD v8.01 with the plugin 4.3.0 + fixes.

In the master branch the fix should look like below: https://github.com/bennymeg/Fabrication-Toolkit/blob/master/plugins/process.py#L162

            is_dnp = (footprint_has_field(footprint, 'dnp') 
                      or (footprint.GetValue().upper() == 'DNP') 
                      or getattr(footprint, 'IsDNP', bool)())

I.e. add parentheses to call the function returned by getattr()

bennymeg commented 5 months ago

Thank you @msalau, I'll fix it.

bennymeg commented 5 months ago

Fixed in v4.4.1