pcb

ipc-d-356 netlist creation

Bug #996319 reported by Jerome Marchand
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
pcb
Fix Released
Low
Bert Timmerman

Bug Description

This patch provides IPC-D-356 netlist creation ability to PCB.
The units of the netlist is determined by the units set in the main window when the netlister is run (Metric for mm and Imperial for mils).
My testing was quite limited: LED2.pcb (from the examples) and a test design I created.
So let me know how it work for you...

Revision history for this message
Jerome Marchand (jerome-marchand) wrote :
Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Hi,

I converted the above patch to a git-patch for others to play with.

First impressions after patching, building and running pcb with this new feature:

1) I'm happy Jerome took the time and effort to code this long outstanding feature request (IIRC in the Sourceforge era).

2) The IPC-D-356 netlist export functionality is not implemented as a HID-exporter, nor as a plug-in for pcb.

3) Furthermore, when tested with the LED2 example, I can't find a netlist file generated by the exporter anywhere, nor is a file-chooser dialog opened to be able to chose a (different) name for the netlist file.

I poke around some more to see what can be done about 1) and 2).

Kind regards,

Bert Timmerman

Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Addendum:

3) In the current state the IPC-D-356 netlist code is GTK UI dependant, and can not be used by the lesstiff UI. The build with the lesstif UI fails with:
<quote>
In file included from ipc-d-356.c:39:
hid/gtk/gui.h:36:21: error: gtk/gtk.h: No such file or directory
In file included from hid/gtk/gui.h:37,
                 from ipc-d-356.c:39:
hid/gtk/ghid-coord-entry.h:26: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘*’ token
In file included from hid/gtk/ghid-main-menu.h:8,
                 from hid/gtk/gui.h:38,
</quote>

Ad 2) If/when the IPC-D-356 netlist export functionality is implemented as a plug-in for pcb the code can be maintained outside the official pcb git repository. If/when the IPC-D-356 netlist exporter functionality is implemented as a HID-exporter this code will have to be added to, and has to be assimilated for, the official pcb git repository.

Kind regards,

Bert Timmerman.

Revision history for this message
Jerome Marchand (jerome-marchand) wrote :

Sorry for the long delay in looking at this.

for the example LED2, an error is generated in the message log window. The error is due to the fact that the Crystal / oscillator part in the example is not named. Give it a name and the netlist should work. (it's the part right of C17).

I might have some free time early next year, and will look at implementing it as a HID-exporter...

Jerome

Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Hi Jerome,

Have a look at:

https://github.com/bert/pcb/compare/master...LP996319rev1

Kind regards,

Bert Timmerman.

Revision history for this message
Jerome Marchand (jerome-marchand) wrote :

Bert,

Boy, this is great! I'm not sure what I'll do with that free time now :-)

It did require a few minor changes to compile with me ( see the included patch).

It then works fine (In LED2 example the oscillator still needs to be named though).

Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Hi Jerome,

Maybe add some tests to the test suite ?

Will have a look at your patch shortly during the coming holiday weeks.

Kind regards,

Bert Timmerman.

Revision history for this message
Peter Clifton (pcjc2) wrote :
Download full text (3.5 KiB)

Hi guys,

As you might have noticed, I'm currently somewhat in the process of breaking connectivity scanning code, and our existing find.c implementation is a mess.

This might impact upon this patch slightly, so we should adopt one of two strategies..

1. Assume impact will be minimal, and fix-up the patch later.
2. Merge it to core ASAP, and make it my problem not to break it ;)

I was wondering whether you thought it might be appropriate to refactor out some of the netlist extraction code from this branch into the core. (Where it can be cleaned up along withe the existing stuff).

A few style points...

1. Please send unified diffs, as they are easier to read.
2. Please don't add comments with your name or initials into the code, e.g. //JM // END JM etc... use the git history to track these things.
3. Not a requirement, but I usually mark a file's copyright as
"Copyright (C) $YEAR PCB Contributors (see ChangeLog for details)"

If you wrote the whole file, feel free to add yours as well, but I always like to leave the "PCB Contributors" as a second line, as this encourages people to feel contribution is welcome, and not the sole preserve of the original author / copyright holder.

4. I want to see changes to the core (like the addition of LookupConnectionByPin() committed as a separate commit.
5. PLEASE don't re-introduce type aliases with "Ptr" on the end. These are gone now in PCB, good riddance ;)

IPCD356_AliasPtr and IPCD356_AliasListPtr should become IPCD356_Alias * and IPCD356_AliasListPtr *

6. Looking at the above structures, we have a few problems:

+ char NName[11];
+ char NetName[256];

I believe PCB supports arbitrary length in its net names (and if it doesn't, it should!) - so I'm unhappy to see this fixed length field. I appreciate the limit of 14 characters on the IPC standard, so that is OK,

I'm not sure why you limit the aliased net names to 10 characters (+termination), I thought the standard supported up to 14? Is this due to the format of your alias strings?

7. PCB's core now depends on glib, so feel free to use GLists, hash tables, string functions, whatever makes the code easier. I think IPCD356_AliasList is a candidate for being replaced if you wanted.

8. Lots more [256] limited strings, and string handling without bounds checking in the code. I'm really trying to keep people from introducing code like this on both grounds of: 1. Arbitrary length limitations are bad, and 2. Unsafe string handling which could cause a crash is bad.

9. Refactoring is better than copy+paste alteration... e.g. rather than adding ResetVisitPinsViasAndPads() you could have extended ResetFoundPinsViasAndPads().

As it happens, I ran into some buggy code which needed refactoring recently, and did just that... so your calls now become:

- ResetFoundLinesAndPolygons (false);
- ResetFoundPinsViasAndPads (false);
+ ResetConnections (false, FOUNDFLAG);

(ResetConnections combines a call to ResetFoundLinesAndPolygons and ResetFoundPinsViasAndPads)

And I think (not build-tested) your new function ResetVisitPinsViasAndPads can be removed if you change:

- ResetVisitPinsViasAndPads ();
+ ResetFoundPinsViasAndPads (fals...

Read more...

Revision history for this message
Peter Clifton (pcjc2) wrote :

And yes.. Bert is right, the test-suite is important for merging to PCB.

Any exporter should be tested as part of PCB's test-suite, and it should be simply a matter making sure this exporter gets run in the test-suite, and adding some "golden" output files for the expected results on our existing test input files.

Look at tests/tests.list

You might want to hand-check the output of the golden files to make sure it is vaguely sane, but for the most part, our test-suite is used to verify we didn't accidentally break something which used to work.

Revision history for this message
Peter Clifton (pcjc2) wrote :

Regarding visualising test-results, perhaps you could look at the gerbv project, and see whether there is any scope (or desire) to add support for importing and overlaying IPC-D-356 data there.

Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Hi Peter,

<quote>
This might impact upon this patch slightly, so we should adopt one of two strategies..

1. Assume impact will be minimal, and fix-up the patch later.
2. Merge it to core ASAP, and make it my problem not to break it ;)
</quote>

I will look into this stuff later on in the coming week.

I'll go for option #1, for I think merging is best done when tests are in place.

Kind regards,

Bert Timmerman.

BTW: Merging can be speeded up by popular vote: hit the "Does this bug affect you ?" at the top of the page. All votes count !

Revision history for this message
Peter Clifton (pcjc2) wrote :

"BTW: Merging can be speeded up by popular vote: hit the "Does this bug affect you ?" at the top of the page. All votes count !"

I'm not sure I agree....

IMHO, the best way to improve the speed of merging is to increase the code quality ;)

Revision history for this message
Peter Clifton (pcjc2) wrote :

Oh, and I renamed the find.c APIs, so you will have to substitute:

ResetFoundLinesAndPolygons -> ClearFlagOnLinesAndPolygons
ResetFoundPinsViasAndPads -> ClearFlagOnPinsViasAndPads
ResetConnections -> ClearFlagOnAllObjects

Hopefully those names are somewhat less confusing than our previous ones.

Revision history for this message
Jerome Marchand (jerome-marchand) wrote :

Hi Peter,

I have implemented most of the changes you mentioned, and I'm now working on the test.

To answer you question on #6: The IPC spec requires Aliases to be composed of: "NNAME" + 5 chars, so with the termination we get Nname[11].

Your question got me to look back at the spec and discover that net names can't have spaces in them. So I added code to replace spaces with underscores in the netlist.

I will post the patch when I'm done with the test.

Jerome

Revision history for this message
Jerome Marchand (jerome-marchand) wrote :

Hi Peter, Bert,

Here's the patch. This includes the test-suite.
Note that I ran this patch from a clean tree so the previous patch (from 2012-19-12) is included in this one.

Jerome

Revision history for this message
Jerome Marchand (jerome-marchand) wrote :

I had some time, so I got the IPC-d-356 netlister to compile / work with the tree in the git repository...

Jerome

Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Hi Jerome,

Due to a vacation with limited internet access it took some time to test your patch.

I converted your patch to a git-patch and stashed it in my pcb devel repository at https://github.com/bert/pcb/tree/LP996319rev2.

The patch applied, the pcb build (gtk UI) is OK and the IPC-D-356 exporter passed the test script.

What I can't find is the generated netlist (LED2.ipc) when I try to export the LED2.pcb in the example directory.

Kind regards,

Bert Timmerman.

Revision history for this message
Jerome Marchand (jerome-marchand) wrote :

Bert,

I think it's probably the same issue as before (See Comment #4 above). The netlister outputs the error in the message log window.
Maybe it needs to popup a window instead?

Jerome

Changed in pcb:
status: New → In Progress
Traumflug (mah-jump-ing)
Changed in geda-project:
importance: Undecided → Wishlist
Changed in pcb:
assignee: nobody → Bert Timmerman (bert-timmerman)
milestone: none → next-feature-release
Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Hi all,

Updated topic branch for testing lives here:

http://git.geda-project.org/pcb/log/?h=home/bert/LP996319

or just:

home/bert/LP996319

Kid regards,

Bert Timmerman.

Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

s/Kid/Kind/

Revision history for this message
Bert Timmerman (bert-timmerman) wrote :

Hi,

I cherry picked this exporter into master and removed the topic branch.

Kind regards,

Bert Timmerman.

Changed in pcb:
status: In Progress → Fix Committed
importance: Undecided → Low
milestone: next-feature-release → next-bug-release
Changed in pcb:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.