-
Notifications
You must be signed in to change notification settings - Fork 581
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
Wrong behavior of asynchronous RAM reads in generated VHDL #1996
Comments
@BFH-ktt1: Could you have a look, as you implemented this? |
It seems the patch I suggested isn't enough, as a cpu I designed (which works in Logisim and doesn't use any gated clocks as well) only works on an FPGA with the following patch which removes the output register @@ -44,8 +44,10 @@ public class RamHdlGeneratorFactory extends AbstractHdlGeneratorFactory {
final var async = StdAttr.TRIG_HIGH.equals(trigger) || StdAttr.TRIG_LOW.equals(trigger);
final var ramEntries = (1 << nrOfaddressLines);
final var truncated = (nrOfBits % 8) != 0;
+ if (byteEnables) {
+ myWires.addWire("s_ramdataOut", nrOfBits);
+ }
myWires
- .addWire("s_ramdataOut", nrOfBits)
.addRegister("s_tickDelayLine", 3)
.addRegister("s_dataInReg", nrOfBits)
.addRegister("s_addressReg", nrOfaddressLines)
@@ -120,9 +119,11 @@ public class RamHdlGeneratorFactory extends AbstractHdlGeneratorFactory {
inputRegs : {{process}}({{clock}}, {{tick}}, address, dataIn, we, oe) {{is}}
{{begin}}
{{if}} (rising_edge({{clock}})) {{then}}
+ {{if}} ({{tick}} = '0') {{then}}
+ s_addressReg <= address;
+ {{end}} {{if}};
{{if}} ({{tick}} = '1') {{then}}
s_dataInReg <= dataIn;
- s_addressReg <= address;
s_weReg <= we;
s_oeReg <= oe;
""");
@@ -182,7 +183,7 @@ public class RamHdlGeneratorFactory extends AbstractHdlGeneratorFactory {
{{if}} (s_we = '1') {{then}}
s_memContents(to_integer(unsigned(s_addressReg))) <= s_dataInReg;
{{end}} {{if}};
- s_ramdataOut <= s_memContents(to_integer(unsigned(s_addressReg)));
+ dataOut <= s_memContents(to_integer(unsigned(s_addressReg)));
{{end}} {{if}};
{{end}} {{process}} mem;
""");
@@ -206,18 +207,6 @@ public class RamHdlGeneratorFactory extends AbstractHdlGeneratorFactory {
.add(" {{end}} {{if}};")
.add("{{end}} {{process}} res{{1}};", i);
}
- } else {
- contents
- .add("""
- res : {{process}}({{clock}}, s_oe, s_ramdataOut) {{is}}
- {{begin}}
- {{if}} (rising_edge({{clock}})) {{then}}
- {{if}} (s_oe = '1') {{then}}
- dataOut <= s_ramdataOut;
- {{end}} {{if}};
- {{end}} {{if}};
- {{end}} {{process}} res;
- """);
}
}
return contents.empty(); But again, I have no idea what I'm doing as I don't really understand the generated VHDL code, so I would appreciate help. |
How'd you manage to use RAM? I thought it wasn't FPGA-supported |
I have a design with a RAM with line enables and a single line. When simulating it in Logisim reading from the RAM is asynchronous, but when running it on an fpga reading seems delayed by a clock cycle.
I made sure not to use a gated clock and to attach a clock component to the clock inputs directly, but the issue persists.
I used this circuit (which also triggers the bug I fixed in #1995) to test this, which, after letting it run, looks like the following in Logisim:
But, synthesizing the design and uploading it to a Basys3 board and connecting the leds results in this:
So the outputs for the led got shifted by one, which indicates there is a delay of one clock cycle from the address to the RAM changing to the output of the RAM reflecting the corresponding value.
This issue looks to be related to #1598.
I tried looking around in the code that generates the VHDL for the RAM and changing this
in RamHdlGeneratorFactory.java to load the address into the register earlier seems to fix at least this use case.
But I don't fully understand the hdl code for the RAM component so I don't know if this change would break other use cases or if it even makes sense.
I understand the RAM component uses 4 real clock cycles for one Logisim tick to simulate asynchronous RAM with the synchronous RAM of an fpga, so I would expect the asynchronous read to work in the fpga as well.
The text was updated successfully, but these errors were encountered: