Added pre condition in the verilog module #620
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe Verilog string in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/simulator/src/sequential/TflipFlop.js (1)
154-160: Good alignment with simulation logic, consider multi-bit syntax improvement.The conditional preset logic correctly aligns the Verilog behavior with the JavaScript simulation logic (lines 72-74), where reset uses
this.preset.value || 0. This change successfully addresses the PR objective of adding precondition logic.For better multi-bit signal handling, consider using width-specific syntax:
- if (pre == 'bx) begin - q <= 'b0; - q_inv <= 'b1; + if (pre === {WIDTH{1'bx}}) begin + q <= {WIDTH{1'b0}}; + q_inv <= {WIDTH{1'b1}}; end else begin q <= pre; q_inv <= ~pre; endHowever, since the original code used
'b0and'b1, maintaining consistency with the existing style is also reasonable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/sequential/TflipFlop.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 092vk
PR: CircuitVerse/cv-frontend-vue#591
File: v1/src/simulator/src/sequential/JKflipFlop.js:159-188
Timestamp: 2025-06-10T11:26:38.102Z
Learning: In the CircuitVerse project, Verilog modules for circuit elements like JKflipFlop are designed to be consistent with the circuit simulation design, which means reset and preset signals are implemented synchronously (triggered on clock edge) rather than asynchronously, even when the signals are named as asynchronous reset/preset.
src/simulator/src/sequential/TflipFlop.js (1)
Learnt from: 092vk
PR: CircuitVerse/cv-frontend-vue#591
File: v1/src/simulator/src/sequential/JKflipFlop.js:159-188
Timestamp: 2025-06-10T11:26:38.102Z
Learning: In the CircuitVerse project, Verilog modules for circuit elements like JKflipFlop are designed to be consistent with the circuit simulation design, which means reset and preset signals are implemented synchronously (triggered on clock edge) rather than asynchronously, even when the signals are named as asynchronous reset/preset.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/simulator/src/sequential/TflipFlop.js (3)
152-156: Asynchronous vs. synchronous reset: confirm project-wide convention.The Verilog uses an async reset (
always @(posedge clk or posedge a_rst)), while prior learnings for CircuitVerse indicate resets/presets are implemented synchronously for some modules. Your simulator-side resolve path does look asynchronous for T-FF, so this may be intentional. Please confirm whether T-FF is the intended exception or whether we should switch to synchronous reset for consistency.If synchronous reset is desired, the change would be:
- always @ (posedge clk or posedge a_rst) begin - if (a_rst) begin + always @ (posedge clk) begin + if (a_rst) begin q <= pre; - q_inv <= ~pre; end else if (en) begin - // toggle logic... + // toggle logic... end endNote: If you adopt this, also ensure simulator resolve() semantics match.
154-156: Derive q_inv combinationally from q to prevent divergence.Optional simplification: drive
q_invas~qvia a continuous assignment. This guarantees complementarity without having to remember to updateq_invon every path.- output reg [WIDTH-1:0] q, q_inv; + output reg [WIDTH-1:0] q; + output [WIDTH-1:0] q_inv; @@ - input [WIDTH-1:0] t, pre; + input [WIDTH-1:0] t, pre; + assign q_inv = ~q; @@ - if (a_rst) begin - q <= pre; - q_inv <= ~pre; + if (a_rst) begin + q <= pre; end else if (en) begin - q <= q ^ t; - q_inv <= ~q ^ t; // == ~(q ^ t) + q <= q ^ t; end
11-17: Doc nits: terminology and typos.
- “data input” → “T input” for T flip-flop.
- Spelling: “ciruit” → “circuit”; “direcion” → “direction”.
Proposed adjustments:
- Line 11: “clock, T input, preset, reset, enable.”
- Lines 15–16: fix spelling in JSDoc params.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/simulator/src/sequential/TflipFlop.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 092vk
PR: CircuitVerse/cv-frontend-vue#591
File: v1/src/simulator/src/sequential/JKflipFlop.js:159-188
Timestamp: 2025-06-10T11:26:38.102Z
Learning: In the CircuitVerse project, Verilog modules for circuit elements like JKflipFlop are designed to be consistent with the circuit simulation design, which means reset and preset signals are implemented synchronously (triggered on clock edge) rather than asynchronously, even when the signals are named as asynchronous reset/preset.
📚 Learning: 2025-06-10T11:26:38.102Z
Learnt from: 092vk
PR: CircuitVerse/cv-frontend-vue#591
File: v1/src/simulator/src/sequential/JKflipFlop.js:159-188
Timestamp: 2025-06-10T11:26:38.102Z
Learning: In the CircuitVerse project, Verilog modules for circuit elements like JKflipFlop are designed to be consistent with the circuit simulation design, which means reset and preset signals are implemented synchronously (triggered on clock edge) rather than asynchronously, even when the signals are named as asynchronous reset/preset.
Applied to files:
src/simulator/src/sequential/TflipFlop.js
🔇 Additional comments (2)
src/simulator/src/sequential/TflipFlop.js (2)
156-166: Unconnected en/pre semantics may not match simulator defaults.In the simulator,
enunconnected behaves like 1 andpreunconnected defaults to 0 (Line 74 usespreset.value || 0). In generated Verilog, unconnected inputs become Z/X, which leads to holding state (foren) or X-propagation (forpre). Ensure the codegen ties:
ento 1’b1 when not wired,preto {WIDTH{1’b0}} when not wired.I can update the exporter to auto-insert these ties at instantiation sites; confirm if desired.
149-151: Double-check port ordering consistency across modules and codegen.You changed the declaration grouping to separate scalar and vector inputs and the module port list is
(..., clk, t, a_rst, pre, en). If other elements (e.g., JK/D flip-flops) or the exporter assume a specific positional order, this could break instantiations. If named port connections are always used, we’re fine.Run this quick scan to compare port lists across sequential modules:
| end else if (en) begin | ||
| if (t) begin | ||
| // Toggle when T input is high | ||
| q <= ~q; | ||
| q_inv <= q; | ||
| end | ||
| // When t == 0, hold current state | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix multi-bit T semantics: use per-bit XOR instead of toggling all bits.
When WIDTH > 1, if (t) q <= ~q; flips all bits whenever any bit of t is 1. Simulator logic (Line 83) uses per-bit XOR (masterState = T ^ slaveState), so the Verilog must mirror that: q <= q ^ t; and keep q_inv complementary of the new q. This is a functional mismatch for vector widths and breaks correctness.
Apply this minimal fix:
- end else if (en) begin
- if (t) begin
- // Toggle when T input is high
- q <= ~q;
- q_inv <= q;
- end
- // When t == 0, hold current state
- end
+ end else if (en) begin
+ // Per-bit toggle: only bits with T=1 flip (matches simulator XOR semantics)
+ q <= q ^ t;
+ q_inv <= ~q ^ t; // == ~(q ^ t), ensures q_inv remains the complement of new q
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| end else if (en) begin | |
| if (t) begin | |
| // Toggle when T input is high | |
| q <= ~q; | |
| q_inv <= q; | |
| end | |
| // When t == 0, hold current state | |
| end | |
| end else if (en) begin | |
| // Per‐bit toggle: only bits with T=1 flip (matches simulator XOR semantics) | |
| q <= q ^ t; | |
| q_inv <= ~q ^ t; // == ~(q ^ t), ensures q_inv remains the complement of new q | |
| end |
🤖 Prompt for AI Agents
In src/simulator/src/sequential/TflipFlop.js around lines 156-163, the code
toggles the entire vector when any bit of T is high; replace the scalar toggle
with per-bit XOR so multi-bit T works: set q <= q ^ t; and set q_inv to be the
bitwise complement of the resulting q (q_inv <= ~(q ^ t)) so q_inv remains the
complement of the updated q.
2 participants
Fixes #561
Describe the changes you have made in this PR -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit