Hi, Recently I have found out that the netlist exporter uses an algorithm that for multiunit components uses non empty field values from the last processed unit [1]. The problem is that the processing order depends on the order of the units in the item list, so completely random.
Instead, I propose we try to get all field values from the first available unit (e.g. unit A), and resort to other units only if there are any fields left empty. To give an example of what can go wrong with such approach: recently I generated BOM and assembly documentation, where unit A had 'Mounted' field set to 'No'. I was surprised to find out that in the generated output it has been marked as mounted, as the unit B had 'Yes' set for the field. I would qualify it as a bug, but I seek your input before I proceed with pushing my changes. See my proposal in the attached patch. There is a discussion regarding sharing the field values among all units [2], but I see there is a technical problem preventing such solution right now. Cheers, Orson 1. https://github.com/KiCad/kicad-source-mirror/blob/master/eeschema/netlist_exporters/netlist_exporter_generic.cpp#L94 2. https://bugs.launchpad.net/bugs/1765771
From 6ee2a072853de33ab72e5874998bb2b49f22c981 Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.sumin...@cern.ch>
Date: Thu, 17 May 2018 17:10:26 +0200
Subject: [PATCH] Deterministic algorithm for picking field values in multiunit
components
The original algorithm picked the value from the last component having
non empty value for a given field, but the processing order was
dependent on the layout of the components in the memory. It means that
for each component, the field values could have been taken from any
unit, randomly.
The patch improves the algorithm, trying to get all values from the unit
with the lowest number and resorts to other units only when there are
field values left empty.
---
.../netlist_exporters/netlist_exporter_generic.cpp | 31 ++++++++++++++--------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/eeschema/netlist_exporters/netlist_exporter_generic.cpp b/eeschema/netlist_exporters/netlist_exporter_generic.cpp
index 9556b2a22..2cf4cbc06 100644
--- a/eeschema/netlist_exporters/netlist_exporter_generic.cpp
+++ b/eeschema/netlist_exporters/netlist_exporter_generic.cpp
@@ -91,17 +91,18 @@ void NETLIST_EXPORTER_GENERIC::addComponentFields( XNODE* xcomp, SCH_COMPONENT*
{
if( comp->GetUnitCount() > 1 )
{
- // Sadly, each unit of a component can have its own unique fields. This block
- // finds the last non blank field and records it. Last guy wins and the order
- // of units occuring in a schematic hierarchy is variable. Therefore user
- // is best off setting fields into only one unit. But this scavenger algorithm
- // will find any non blank fields in all units and use the last non-blank field
+ // Sadly, each unit of a component can have its own unique fields. This
+ // block finds the unit with the lowest number having a non blank field
+ // value and records it. Therefore user is best off setting fields
+ // into only the first unit. But this scavenger algorithm will find
+ // any non blank fields in all units and use the first non-blank field
// for each unique field name.
COMP_FIELDS fields;
wxString ref = comp->GetRef( aSheet );
SCH_SHEET_LIST sheetList( g_RootSheet );
+ int minUnit = comp->GetUnit();
for( unsigned i = 0; i < sheetList.size(); i++ )
{
@@ -117,26 +118,34 @@ void NETLIST_EXPORTER_GENERIC::addComponentFields( XNODE* xcomp, SCH_COMPONENT*
if( ref2.CmpNoCase( ref ) != 0 )
continue;
- // The last guy wins. User should only set fields in any one unit.
+ int unit = comp2->GetUnit();
+
+ // The lowest unit number wins. User should only set fields in any one unit.
// remark: IsVoid() returns true for empty strings or the "~" string (empty field value)
- if( !comp2->GetField( VALUE )->IsVoid() )
+ if( !comp2->GetField( VALUE )->IsVoid()
+ && ( unit < minUnit || fields.value.IsEmpty() ) )
fields.value = comp2->GetField( VALUE )->GetText();
- if( !comp2->GetField( FOOTPRINT )->IsVoid() )
+ if( !comp2->GetField( FOOTPRINT )->IsVoid()
+ && ( unit < minUnit || fields.footprint.IsEmpty() ) )
fields.footprint = comp2->GetField( FOOTPRINT )->GetText();
- if( !comp2->GetField( DATASHEET )->IsVoid() )
+ if( !comp2->GetField( DATASHEET )->IsVoid()
+ && ( unit < minUnit || fields.datasheet.IsEmpty() ) )
fields.datasheet = comp2->GetField( DATASHEET )->GetText();
for( int fldNdx = MANDATORY_FIELDS; fldNdx < comp2->GetFieldCount(); ++fldNdx )
{
- SCH_FIELD* f = comp2->GetField( fldNdx );
+ SCH_FIELD* f = comp2->GetField( fldNdx );
- if( f->GetText().size() )
+ if( f->GetText().size()
+ && ( unit < minUnit || fields.f.count( f->GetName() ) == 0 ) )
{
fields.f[ f->GetName() ] = f->GetText();
}
}
+
+ minUnit = std::min( unit, minUnit );
}
}
--
2.16.2
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
