kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

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.suminski@xxxxxxx>
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

Attachment: signature.asc
Description: OpenPGP digital signature