Skip to main content
5 votes

Simple GPIO design module in SystemVerilog

Generally speaking, the code has consistent layout, is easy to understand and uses descriptive variable names. You have a good separation between sequential logic and combinational logic, and you take ...
toolic's user avatar
  • 3,751
4 votes
Accepted

Design and stimulus for a simple Mealy finite state machine

You have done an excellent job of partitioning your design code from your testbench code into different modules. Similarly, you used good partioning between your sequential logic and combinational ...
toolic's user avatar
  • 3,751
4 votes
Accepted

Full Adder in Verilog

In the design module, use ANSI-style port declarations to reduce redundant port lists (refer to IEEE-Std 1800-2017, section 23.2.1 Module header definition): ...
toolic's user avatar
  • 3,751
4 votes
Accepted

Verilog implementation of trapezoidal integration method

Why are the inputs signed, are you working with negative dimensions? Most FPGAs have dedicated logic for multiplication so you usually can simply write ...
Greg's user avatar
  • 443
4 votes
Accepted

8-Bit ALU in Verilog

Design Issue: From a design perspective I see one issue in LogicUnit. Out is an inferred latch because it is not assigned in ...
Greg's user avatar
  • 443
4 votes
Accepted

pseudo-random binary sequence (prbs)

I get compile errors on 2 different simulators (Cadence and Synopsys) from these 2 lines in the testbench: logic prbs = '0; logic prbs_n = '0; Since you ...
toolic's user avatar
  • 3,751
3 votes

Generate a sine wave

When I compile your code on some simulators, I see warning messages which could indicate potential problems. For example, with the Cadence simulator: ...
toolic's user avatar
  • 3,751
3 votes
Accepted

Parameterized Verilog shift register code

I don't see any problems with the code functionally, and the layout follows good practices for the most part. You chose meaningful names for the signals and parameters, and the parameters make your ...
toolic's user avatar
  • 3,751
3 votes
Accepted

Pulse-width modulation module

The layout of your code is consistent and easy to read. I don't see any functional problems. One consideration for all digital logic is whether you can tolerate glitches on your outputs. If your PWM ...
toolic's user avatar
  • 3,751
3 votes

4-stage pipelined RV32I CPU in Verilog

Have I made any rookie mistakes or committed major sins against Verilog style? Overall it is coded very well and easy to read, no Verilog sins (unlikely to synthesize if there were). Clean Verilog-...
Greg's user avatar
  • 443
3 votes

Heart beat RTL module

I get compile errors on 2 different simulators (Cadence and Synopsys) from this line in the testbench: bit heart_beat = '0; Since you connect that signal to the ...
toolic's user avatar
  • 3,751
3 votes

LED matrix controller - Verilog

The code follows recommended practices regarding the use of nonblocking assignments, consistent indentation and parameter usage. One exception is the line: ...
toolic's user avatar
  • 3,751
3 votes
Accepted

ROM memory in SystemVerilog and cocotb

This review is limited to the SystemVerilog code (rom.sv). You declared the DATA_WIDTH parameter, but your code does not use it. ...
toolic's user avatar
  • 3,751
2 votes
Accepted

Sensored BLDC Commutation Controller code

Overview It is good that you did the following: Chose a meaningful name for the module Used ANSI-style module ports instead of the older style Declared one port per line for better readability Added ...
toolic's user avatar
  • 3,751
2 votes
Accepted

Detect when X-axis inputs and Y-axis inputs go high

Overview You've done a good job with the following: Code layout and use of indentation Using compact ANSI-style module ports Using the compact implicit sensitivity list ...
toolic's user avatar
  • 3,751
2 votes
Accepted

Variable output stream delay no shift register

Generally speaking, the code has consistent layout and is easy to understand. It uses descriptive variable names and comments. I don't see much room for improvement. The recommended Verilog coding ...
toolic's user avatar
  • 3,751
2 votes
Accepted

Clock frequency meter module

Generally speaking, the code has consistent layout, is easy to understand, uses descriptive variable names and makes good use of parameters for constant values. You have a good separation between ...
toolic's user avatar
  • 3,751
2 votes

SystemVerilog implementation of an N-bit prefix adder logic design

The code uses good indentation, and it makes good use of for loops to avoid code duplication. It's great that you used a ...
toolic's user avatar
  • 3,751
2 votes

Simple GPIO design module in SystemVerilog

Adding my two cents to the already great answer by toolic: Using "integer" as a loop iterator is not recommended, as each bit in integer is a 4 state bit (0, 1, x, z). Since your loop ...
EEliaz's user avatar
  • 121
2 votes

fixed pseudo-random binary sequence (prbs)

I don't see too much room for improvement. Since you have some code which is common to 2 modules (the localparam TAP_0 and ...
toolic's user avatar
  • 3,751
2 votes

3-way multiplexer built on 2-way multiplexers in Verilog

I don't see any problems with the code functionally, and the layout follows good practices for the most part. That style guide seems mostly reasonable, and it is a good idea to use a set of ...
toolic's user avatar
  • 3,751
2 votes
Accepted

32-bit ALU design implementation and testbench

For the design Issues The biggest issues with your code is out is an inferred latch because the condition f == 3'b011 is ...
Greg's user avatar
  • 443
2 votes
Accepted

Haskell Parsec parser of Verilog-style number literals

Let's start with your imports. You should get rid of those you don't need, e.g. System.IO, import Data.Either.Extra (fromRight). ...
Zeta's user avatar
  • 19.3k
2 votes

Simple SystemVerilog AXI controller

From a style/best practice your code looks pretty good. The only glaring error is that your module multiflipflop_ack is missing port directions. ...
Greg's user avatar
  • 443
2 votes
Accepted

AXI stream data generator

The layout of code the follows good practices, and you have a clean separation between design and testbench. There is a syntax error which your compiler should have detected. An instance name is ...
toolic's user avatar
  • 3,751
2 votes
Accepted

Carry Lookahead Adder - SystemVerilog

The layout of your code is good, and you did a good job partitioning the design from the testbench. In your design, you could take advantage of the "reduction" operators to make your code ...
toolic's user avatar
  • 3,751
1 vote
Accepted

Finding the carry out of the "+" operator in SystemVerilog

The layout of your code is good, and you chose meaningful names for your signals. There are some improvements you can make, however. The following signals are essentially unused: ...
toolic's user avatar
  • 3,751
1 vote
Accepted

A byte endian swapper

The code makes good use of parameters. However, I see a potential problem. if (0 == (DATA_WIDTH % 4)) begin That code seems to imply that any multiple of 4 is ...
toolic's user avatar
  • 3,751
1 vote
Accepted

Range checking function

Verilog is flexible with how it interprets integers. It interprets 5 and 4'b0101 as the same numeric value. Yes, it interprets ...
toolic's user avatar
  • 3,751
1 vote

VGA sync generator for 640x480@60Hz

If you had ran simulation or loaded onto FPGA, you would noticed you didn't get the expected behavior. Run simulation and look at waveforms before loading to FPGA. You have ...
Greg's user avatar
  • 443

Only top scored, non community-wiki answers of a minimum length are eligible