LED matrix controller - Verilog

Question

I'm an ECE student. My experience in Verilog and FPGAs is mainly from my digital logic design class. To practice Verilog, I decided to implement a controller for Adafruit LED matrices. It interfaces with a single-port BRAM to access pixel data.

If helpful, here is an overview of how it works:

The design has an FSM that alternates between the DISPLAY_1 and DISPLAY_2 states to read pixel data and generate an address for the BRAM each clock cycle. In the DISPLAY_2 state, it writes the pixel data to registers connected to the o_color1 and o_color2 output signals. Since this design drives the clock (o_clk) for the LED matrix, I ensure that these output signals only change on the falling edge of o_clk. After driving the data for all the pixels in a row, it increments the o_row_sel register to select the next two rows.

led_matrix_controller.v:

`timescale 1ns / 1ps
module led_matrix_controller 
 #(parameter MATRIX_COLS = 64,
 parameter MATRIX_ROWS = 32,
 parameter PWM_BITS = 1
 )
 (input i_clk,
 input rst,
 input [(3*PWM_BITS)-1:0] i_pixel_data,
 output reg [$clog2(MATRIX_COLS*MATRIX_ROWS)-1:0] o_pixel_addr,
 output reg o_clk,
 output reg o_oe,
 output reg o_latch,
 output reg [$clog2(MATRIX_ROWS/2)-1:0] o_row_sel,
 output [2:0] o_color1,
 output [2:0] o_color2
 );
 wire [$clog2(MATRIX_ROWS/2)-1:0] o_row_sel_next = o_row_sel + 1;
 reg [$clog2(MATRIX_COLS):0] pixel_counter;
 reg [PWM_BITS-1:0] pwm_counter;
 reg first_cycle;
 reg [(3*PWM_BITS)-1:0] pixel_1;
 reg [(3*PWM_BITS)-1:0] pixel_2;
 reg [(3*PWM_BITS)-1:0] temp_pixel;
 assign o_color1 = color(pixel_1);
 assign o_color2 = color(pixel_2);
 function [2:0] color (input reg [(3*PWM_BITS)-1:0] pixel);
 color = {
 (pwm_counter < pixel[PWM_BITS-1:0]),
 (pwm_counter < pixel[(2*PWM_BITS)-1:PWM_BITS]),
 (pwm_counter < pixel[(3*PWM_BITS)-1:2*PWM_BITS])
 };
 endfunction
 reg [2:0] state;
 localparam STATE_DISPLAY_1 = 3'b000;
 localparam STATE_DISPLAY_2 = 3'b001;
 localparam STATE_BLANK_1 = 3'b010;
 localparam STATE_BLANK_2 = 3'b011;
 localparam STATE_BLANK_3 = 3'b100;
 always @(posedge i_clk)
 begin
 if(rst)
 begin
 o_clk <= 0;
 o_oe <= 0;
 o_latch <= 0;
 o_row_sel <= ~0;
 o_pixel_addr <= 0;
 
 pixel_1 <= 0;
 pixel_2 <= 0;
 temp_pixel <= 0;
 
 pixel_counter <= 0;
 pwm_counter <= 0;
 first_cycle <= 1;
 
 state <= STATE_DISPLAY_2;
 end
 else
 begin
 case(state)
 STATE_DISPLAY_1:
 begin
 o_oe <= 0;
 o_latch <= 0;
 o_clk <= !first_cycle;
 temp_pixel <= i_pixel_data;
 if(pixel_counter > MATRIX_COLS)
 begin
 state <= STATE_BLANK_1;
 end
 else
 begin
 o_pixel_addr <= pixel_counter + MATRIX_COLS*o_row_sel_next;
 state <= STATE_DISPLAY_2;
 end
 end
 STATE_DISPLAY_2:
 begin
 o_clk <= 0;
 first_cycle <= 0;
 if(!first_cycle)
 begin
 pixel_1 <= temp_pixel;
 pixel_2 <= i_pixel_data;
 end
 o_pixel_addr <= pixel_counter + MATRIX_COLS*(MATRIX_ROWS/2 + o_row_sel_next);
 pixel_counter <= pixel_counter + 1;
 state <= STATE_DISPLAY_1;
 end
 STATE_BLANK_1:
 begin
 if(o_pixel_addr != 0)
 begin
 pixel_1 <= temp_pixel;
 pixel_2 <= i_pixel_data;
 end
 else
 begin
 pixel_1 <= i_pixel_data;
 pixel_2 <= temp_pixel;
 end
 o_clk <= 0;
 pixel_counter[$clog2(MATRIX_COLS)] = 0;
 state <= STATE_BLANK_2;
 end
 STATE_BLANK_2:
 begin
 o_oe <= 1;
 state <= STATE_BLANK_3;
 end
 STATE_BLANK_3:
 begin
 o_row_sel <= o_row_sel + 1;
 o_latch <= 1;
 first_cycle <= 1;
 if(o_row_sel == (MATRIX_ROWS/2)-2)
 begin
 pwm_counter <= pwm_counter + 1;
 end
 state <= STATE_DISPLAY_1;
 end
 endcase
 end
 end
endmodule

single_port_ram_sync.v:

`timescale 1ns / 1ps
//////////////////////////////////////////////////////////////////////////////////
//
// Single-port RAM With Synchronous Read (Read Through)
// Modified from XST v-rams-07
//
//////////////////////////////////////////////////////////////////////////////////
module single_port_ram_sync
 #(parameter ADDR_WIDTH = 6,
 parameter DATA_WIDTH = 8,
 parameter INIT_FILE = ""
 )
 (input clk, 
 input we,
 input [ADDR_WIDTH-1:0] addr,
 input [DATA_WIDTH-1:0] din,
 output [DATA_WIDTH-1:0] dout
 );
 
 reg [DATA_WIDTH-1:0] ram [2**ADDR_WIDTH-1:0];
 reg [ADDR_WIDTH-1:0] r_addr;
 
 initial
 begin
 $readmemh(INIT_FILE, ram);
 end
 
 always @(posedge clk)
 begin
 if (we)
 begin
 ram[addr] <= din;
 end
 r_addr <= addr;
 end
 
 assign dout = ram[r_addr];
endmodule

Answer 1

The code follows recommended practices regarding the use of nonblocking assignments, consistent indentation and parameter usage.

One exception is the line:

 pixel_counter[$clog2(MATRIX_COLS)] = 0;

That should use a nonblocking assignment:

 pixel_counter[$clog2(MATRIX_COLS)] <= 0;

The state machine in the led_matrix_controller module has a 3-bit state register. This means that there are 8 possible values for the register (0-7). Since you only require 5 defined states (0-4), this leaves 3 unassigned states (5-7). Although you can not reach the unassigned states in a normal Verilog simulation, in silicon it may be possible if you encounter a soft error. It is a common practice to guard against this type of error by accounting for all possible states using a default clause in your case statement. For example:

 default:
 begin
 state <= STATE_DISPLAY_2;
 end
 endcase

The ~0 syntax is legal, and it does what you want, but I don't think it is all that common or easy to understand. It would be better to use the '1 syntax, which does require you to enable SystemVerilog features in your tools (most tools support SV these days). Refer to IEEE Std 1800-2017, section 5.7.1 Integer literal constants. The code would then be:

 o_row_sel <= '1;

Another common approach is to use the replicated concatenation operator to explicitly declare the signal width:

 o_row_sel <= {ROWSEL_WIDTH{1'b1}};

where you would create a parameter named ROWSEL_WIDTH, for example.