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

EDIF files generated by Atmel CPLD fitter unparsable. #215

Open
peterzieba opened this issue Oct 15, 2023 · 11 comments
Open

EDIF files generated by Atmel CPLD fitter unparsable. #215

peterzieba opened this issue Oct 15, 2023 · 11 comments
Labels
resolved? Believed to be resolved and ready to close

Comments

@peterzieba
Copy link

peterzieba commented Oct 15, 2023

Hi,

I'm using Atmel's fitter for the ATF150x parts, which can be made to generate an EDIF file, which I was hoping to parse with this project.

The first error I received was in regard to the timestamp format, however, I was able to work around this.

I am now receiving the error "ValueError: Target name not valid for the current namespace policy.", and it was not obvious where this was coming from.

Ultimately, I was hoping to make a quick visualization using this:
https://github.com/JensRestemeier/EdifTests

I know there are a lot of variants on EDIF files and so I appreciate that there may not necessarily be an intention to support this, but thought I'd ask.

Below is the EDIF file:
ediftest.edn.txt

In case it is of interest, also attached is the CUPL code, ultimately calling the Atmel Fitter:
ediftest.PLD.txt

@peterzieba
Copy link
Author

peterzieba commented Oct 15, 2023

I think I figured out the issues:

  1. Time stamp provided is (timeStamp"10/15/23 10/15/23") whereas something like (timeStamp 2023 10 15 13 18 45) is expected.
  2. Backslashes, colons, and dashes in the cell name break things. Line 171 of ediftest.edn.txt had the following:
    (cell Z:\home\owner\Nextcloud\ee-pld\ediftest\ediftest (celltype GENERIC)
    Shortening this to just (cell ediftest (celltype GENERIC) fixes things.

Again, not sure there is an intention to support these files, but if they were it would be amazing!

If nothing else, the errors produced regarding the timestamp formatting made it very clear what the problem was. The other error did not make it clear where the problem was.

@jacobdbrown4
Copy link
Collaborator

Thanks for submitting this issue. I have improved the error message. Now it says ValueError: Target name not valid for the current namespace policy. Target name: Z:\home\owner\Nextcloud\ee-pld\ediftest\ediftest. This change is found on the next_release branch.

As for supporting this type of EDIF, we will look into it.

@peterzieba
Copy link
Author

peterzieba commented Oct 19, 2023

Thanks! I've checked out next_release and confirmed the error message change.

I've found that the only thing preventing more complex examples from working are the assertions on lines 58 and 62 of wire.py:

assert outer_pin.wire is None, "Pin already connected to a different wire"
assert pin.wire is None, "Pin already connected to a different wire"

Once these assertions are inhibited, it looks like at least in the case of line 58 assertions, they have to do with multiple connections to vcc/gnd:

`
$ ./edif2dot.py ediftest2.edn
---- wire.py Line 58 assertion:
<class 'spydrnet.ir.Wire'; Contained by Cable.name 'NET_1' <class 'spydrnet.ir.Cable'; is_downto: True; is_scalar: True; lower index: 0>>
<class 'spydrnet.ir.Instance'; name 'PowVCC'; parent definition.name 'ediftest2'; reference definition.name 'VCC'>
<class 'spydrnet.ir.InnerPin'; Wire connected undefined>

---- wire.py Line 58 assertion:
<class 'spydrnet.ir.Wire'; Contained by Cable.name 'NET_4' <class 'spydrnet.ir.Cable'; is_downto: True; is_scalar: True; lower index: 0>>
<class 'spydrnet.ir.Instance'; name 'PowGND'; parent definition.name 'ediftest2'; reference definition.name 'GND'>
<class 'spydrnet.ir.InnerPin'; Wire connected undefined>
`

The only difference between the previous file (ediftest) and this one (ediftest2) is that the first example is simply a buffer from one pin to another. ediftest2.edn.txt takes this a step further and has another pair of pins where the input is inverted before sending to an output pin, however the second example is where multiple connections hang off of vcc/gnd as shown in the Graphviz output ultimately generated:
ediftest2.gv.pdf

It looks way more complicated than it is because I think it is modeling what is happening inside of the chip (almost all of the blocks are simply passing the signal through unchanged).

This is the EDIF that is responsible for the problems:
ediftest2.edn.txt

While the timestamp and cell name invalid characters are easy enough to work around, this particular issue happen quite a lot of times in more complicated examples.

I'm well beyond my depth in understanding how this could be properly supported (or even if it should be), but the nature of the issue seems simple enough: It looks like multiple things being tied to vcc/gnd is the root of the problem, and this does not happen in the first example (ediftest.edn.txt), where vcc and gnd were only used once.

@jacobdbrown4
Copy link
Collaborator

jacobdbrown4 commented Oct 23, 2023

Thanks for providing the example netlist to let me test it out myself. It looks like the issue is because it is trying to connect multiple wires to a single pin. In your example, there is a net from port Q of PowGND to port A2 of XOR2_3 and another net from port Q of PowGND to port A2 of XOR2_9. Usually (from what I've seen), if multiple cells are driven by the same cell, they would all be connected by the same wire, not multiple wires for each one. Here is the PowGND connections from you netlist:

(net NET_4  (joined
    (portref A2 (instanceref XOR2_3))
    (portref Q (instanceref PowGND))
    )
)

(net NET_13  (joined
    (portref A2 (instanceref XOR2_9))
    (portref Q (instanceref PowGND))
    )
)

And SpyDrNet expects it to be this:

(net NET_13  (joined
    (portref A2 (instanceref XOR2_9))
    (portref A2 (instanceref XOR2_3))
    (portref Q (instanceref PowGND))
    )
)

The same goes for PowVCC. I manually changed these in the netlist and SpyDrNet parsed it just fine.

When you generate the EDIF file, can you tell the tool you are using to merge these nets into one as shown above?

@peterzieba
Copy link
Author

peterzieba commented Oct 23, 2023

Unfortunately there is no way to adjust how the these EDIF files are returned. The Atmel fitters for these CPLDs are closed-source and have not been updated by the manufacturer in a long time (Formerly Atmel, now Microchip). I agree that these should just be the same net and so this would seem to be somewhat invalid EDIF on the side of the fitter.

My feeling is that if an input is always driven by Vcc or Gnd, there is probably a better way of representing this but this is unfortunately what the fitter provides. Is this related to some degree to what #216 (<const0> and <const1> missing drivers) is getting at? I've ran some more elaborate examples and have not seen this happen with anything other than GND/VCC cells, and it would be great if when converting to Verilog these were simplified to 1'b0 or 1'b1 (or similar if there is a better way of doing this in Verilog).

Is this something that could be implemented, or is this particular EDIF format too problematic as is?

@jacobdbrown4
Copy link
Collaborator

jacobdbrown4 commented Oct 24, 2023

I updated the EDIF parser to work around the issue here. Here's what would happen before:

When the parser reaches this

(net NET_4  (joined
    (portref A2 (instanceref XOR2_3))
    (portref Q (instanceref PowGND))
    )
)

It will create a wire for NET_4 and connect A2 of XOR2_3 and Q of PowGND.

Then when it reaches this

(net NET_13  (joined
    (portref A2 (instanceref XOR2_9))
    (portref Q (instanceref PowGND))
    )
)

It will create a wire for NET_13 and connect A2 of XOR2_9 and throw an error when trying to connect Q of PowGND because Q of PowGND is already connected to a different wire.

Now, with the update, the parser will see that Q is already connected to another wire and change A2 of XOR2_9 to also connect to that existing wire. NET_13 will be unused and removed.

The composed netlist will then show

(net NET_4 (joined
      (portref A2 (instanceref XOR2_3))
      (portref Q (instanceref PowGND))
      (portref A2 (instanceref XOR2_9))
)

and NET_13 will not exist.

This was the quickest and simplest solution I could think of. The fix is found on the next_release branch. Does this solution work?

@peterzieba
Copy link
Author

Thanks! I just tried using next_release and confirm that it works. This does sound like an ideal/logical solution as there is no reason for the other nets to exist. I do wonder whether something like this should generate warnings in the case that someone is not expecting this or has an otherwise unusual file.

My goal is ultimately to simplify the netlist, removing redundant parts so that the behavior is more clear as much of the netlist currently is simply passing along a signal unchanged (So, removing things like BUF_n and OR1_n and simply tying the nets together). Is this something that is possible in the normal usage of Spydrnet?

@jacobdbrown4
Copy link
Collaborator

I have added a simple warning message as you suggested.

As for simplifying a netlist, you can do that in spydrnet. See:

import spydrnet as sdn
from spydrnet.util.selection import Selection

netlist = sdn.parse("ediftest2.edn")

for instance in netlist.get_instances():

    if instance.reference.name not in ["BUF", "OR1"]:
        continue

    print("Removing " + instance.name)

    in_pins = list(x for x in instance.get_pins(selection=Selection.OUTSIDE, filter=lambda x: x.inner_pin.port.direction is sdn.IN))
    out_pins = list(x for x in instance.get_pins(selection=Selection.OUTSIDE, filter=lambda x: x.inner_pin.port.direction is sdn.OUT))

    for in_pin, out_pin in zip(in_pins, out_pins):

        in_wire = in_pin.wire
        in_wire.disconnect_pin(in_pin)

        out_wire = out_pin.wire

        for out_wire_pin in out_wire.pins:
            if out_wire_pin is out_pin:
                continue
            out_wire.disconnect_pin(out_wire_pin)
            in_wire.connect_pin(out_wire_pin)

        out_wire.cable.remove_wire(out_wire)
    
    instance.parent.remove_child(instance)


netlist.compose("ediftest2_simplified.edf")

Here is a visualization file of the simplified netlist: ediftest2_simplified.gv.pdf

@jacobdbrown4 jacobdbrown4 added the resolved? Believed to be resolved and ready to close label Oct 26, 2023
@peterzieba
Copy link
Author

Something is odd about that code snippet -- when I add "AND1" to the list of things to be removed, it doesn't actually remove it, unless that whole for-loop is run twice... Otherwise this looks like exactly what I'm trying to do! Thank you!

@jacobdbrown4
Copy link
Collaborator

I think it misses the AND1 instances the first time around because we are removing an item from a list we are iterating through. A better way is to iterate through the instances, set aside the ones we want to remove, and then later remove them. See the edited code:

import spydrnet as sdn
from spydrnet.util.selection import Selection

netlist = sdn.parse("ediftest2.edn")

to_remove = []

for instance in netlist.get_instances():
    if instance.reference.name not in ["BUF", "OR1", "AND1"]:
        continue
    to_remove.append(instance)


for instance in to_remove:

    print("Removing " + instance.name)

    in_pins = list(x for x in instance.get_pins(selection=Selection.OUTSIDE, filter=lambda x: x.inner_pin.port.direction is sdn.IN))
    out_pins = list(x for x in instance.get_pins(selection=Selection.OUTSIDE, filter=lambda x: x.inner_pin.port.direction is sdn.OUT))

    for in_pin, out_pin in zip(in_pins, out_pins):

        in_wire = in_pin.wire
        in_wire.disconnect_pin(in_pin)

        out_wire = out_pin.wire

        for out_wire_pin in out_wire.pins:
            if out_wire_pin is out_pin:
                continue
            out_wire.disconnect_pin(out_wire_pin)
            in_wire.connect_pin(out_wire_pin)

        out_wire.cable.remove_wire(out_wire)

    instance.parent.remove_child(instance)


netlist.compose("ediftest2_simplified.edf")

@peterzieba
Copy link
Author

That looks like it works very well, however, I'm just now discovering that the Atmel fitter generated EDIF file is not properly connecting Tristate cells to their final destination pin when EN is used. The fitter also produces a Verilog file, however, which does not seem to have these problems.

So, I was hoping to switch to having Spydrnet read the Verilog file instead with the same goal of removing the redundant elements.

Unfortunately, the verilog is problematic enough where Spydrnet does not want to open it (beyond just changing semicolons to commas), and so my question would be, does it look like this format has any chance of working? I'd like to make a preprocessor for it and am hoping it would only take a few changes with some sed expressions, but it's not clear to me if this file is too far gone for that to be sensible.

Below is an example of the Verilog it generates for an inverter from one pin to another. Does this look like it could be made to be parsed by Spydrnet?

// ATF15xx verilog timing model
// Atmel ATF15xx Fitter Version 1918 created on Tue Nov 06 23:47:47 2023

`timescale 1ns / 1ps

module invv(
input YOURSIGNAL;
output YOURSIGNAL_OUT;
supply0 gnd;
supply1 vcc;

wire
s_0, s_1, s_2, s_3, s_4;

TRI TRI_5 (.Q(YOURSIGNAL_OUT), .A(s_0), .EN(vcc));
BUF BUF_4 (s_0, s_1);
XOR2 XOR2_3 (s_1, s_2, gnd);
OR1 OR1_2 (s_2, s_3);
AND1 AND1_1 (s_3, s_4);
INV INV_0 (s_4, YOURSIGNAL);

endmodule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved? Believed to be resolved and ready to close
Projects
Status: In Progress
Development

No branches or pull requests

2 participants