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

🐞 Bug Input pre is not used in the Verilog code for DflipFlop. #321

Open
1 task done
philipabbey opened this issue Jun 17, 2024 · 3 comments
Open
1 task done
Labels
🐞 bug Something isn't working help wanted Extra attention is needed

Comments

@philipabbey
Copy link

philipabbey commented Jun 17, 2024

Is there an existing issue for this?

  • I have searched the existing issues.

Describe the bug

Input pre is not used in the Verilog code for DflipFlop.

module DflipFlop(q, q_inv, clk, d, a_rst, pre, en);
    parameter WIDTH = 1;
    output reg [WIDTH-1:0] q, q_inv;
    input clk, a_rst, pre, en;
    input [WIDTH-1:0] d;

    always @ (posedge clk or posedge a_rst)
    if (a_rst) begin
        q <= 'b0;
        q_inv <= 'b1;
    end else if (en == 0) ;
    else begin
        q <= d;
        q_inv <= ~d;
    end
endmodule

This can cause the entire design to be optimised away during synthesis in Xilinx's Vivado, and it took some finding (since I only know VHDL ;-)

Steps to Reproduce

Not used the cv-frontend-vue code base at all, just noted the same mistake has been copied across from the original simulator.

Expected Behavior

The pre input should be used to set the value on reset with a_rst.

The following is a guess at the correction since I don't actually code in Verilog.

    if (a_rst) begin
        q <= pre;
        q_inv <= ~pre;

Screenshots

No response

Is the faced issue/bug related to the Vue simulator?

Yes

Used Vue simulator with or without backend?

None

Is the bug present only on the dev server, the build, or both?

Both

Device Information

Is the bug present only on the dev server, the build, or both?

In truth, no idea. I only note you have copied the mistake across from the original simulator and its there in your source code at https://github.com/CircuitVerse/cv-frontend-vue/blob/main/src/simulator/src/sequential/DflipFlop.js#L151-L152, so its there in at least one of the servers.

Additional Context

CircuitVerse/CircuitVerse#4985

Are you working on this issue?

No

@philipabbey philipabbey added pending triage issue yet to be reviewed by maintainers 🐞 bug Something isn't working labels Jun 17, 2024
@philipabbey philipabbey changed the title 🐞 Bug: [Title] 🐞 Bug Input pre is not used in the Verilog code for DflipFlop. Jun 17, 2024
@Arnabdaz Arnabdaz added help wanted Extra attention is needed and removed pending triage issue yet to be reviewed by maintainers labels Jan 15, 2025
@Sourabh-awasthy
Copy link

@philipabbey
I believe using both a_rst and pre together in this context might not make sense. If preset is to be used, it would depend only on the clock cycle, which might make the asynchronous counter unnecessary. Perhaps we could handle both cases separately—when a_rst is inputted, we can use (posedge clk or posedge a_rst), and when pre is input, only posedge clk would be needed. Does this approach seem reasonable?

@philipabbey
Copy link
Author

philipabbey commented Feb 2, 2025

In short: In order for the Verilog code to match your DflipFlop primitive, you must use the corrected Verilog I supplied.

Longer Answer: The code as written has a redundant input pre. So the first rule here is "if you don't need it don't code it." The default answer would be to remove the input pre completely. But just a moment...

This does beg the question, "what was intended by the input pre?" Well your Verilog model must match your existing simulation of a DflipFlop. Usually its the HDL code (Verilog of VHDL) that infers the primitives during synthesis, but hey we're doing things backwards here as you provide a schematic builder and simulator and then want to write out the Verilog that achieves it.

Image

You need to read the related issue at CircuitVerse/CircuitVerse#4985 where I solved the simulation mismatch. I solved it with the suggested code change and proved it by simulation and synthesis in Vivado. Therefore my answer is proven to be correct. Please study it and the DflipFlop simulation more throughly before any further replies.

I believe using both a_rst and pre together in this context might not make sense. If preset is to be used, it would depend only on the clock cycle, which might make the asynchronous counter unnecessary. Perhaps we could handle both cases separately—when a_rst is inputted, we can use (posedge clk or posedge a_rst), and when pre is input, only posedge clk would be needed. Does this approach seem reasonable?

As for your suggestion, I think you are confused about the intended function of pre. The intended function must now match the existing DflipFlop, as it is too late to change the desired behaviour of the DflipFlop. The functionality and requirements of the Verilog code is preciesely defined by the existing DflipFlop. That means you have to use the Verilog correction I supplied as nothing else will match. You do not get to change the Verilog code to something else that does not match the simulation model of a DflipFlop either, even if you think it makes more sense for some reason. Hence please don't suggest an alternative functionality (leave that for your own Verilog code on your own projects outside CircuitVerse). It has to describe the behaviour of the existing DflipFlop or its wrong. That behaviour is not my choice, nor yours either. Neither you nor I can choose a different behaviour.

@Sourabh-awasthy
Copy link

@philipabbey ohkay understood! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants