Haresh median filter#8
Conversation
| localparam int WINDOW_LEN = KERNEL_LEN * KERNEL_LEN; | ||
|
|
||
|
|
||
| // BRAM signals |
There was a problem hiding this comment.
Small style choice, but don't use _i with your intern module code. It should look like this:
logic bram_wr_en;
....
bram #(
.DATA_W (DATA_W),
.ADDR_W (ADDR_W)
) bram_inst_prev (
.clk (clk),
.wr_en_i (bram_wr_en),
....
| logic fire_s1_q; | ||
| logic [DATA_W-1:0] pix_s1_q; | ||
| logic [ADDR_W-1:0] col_s1_q; | ||
| logic [ROW_W-1:0] row_s1_q; |
There was a problem hiding this comment.
Don't define a new signal in the middle of a module. Just put them at the top of a module
| end | ||
|
|
||
|
|
||
| function automatic logic [PIXEL_W-1:0] median4_chan( |
There was a problem hiding this comment.
This function doesn't rely on module parameters. Define in a package
| end | ||
|
|
||
| if (fire_s1_q) begin | ||
| if (col_s1_q == 0) begin |
There was a problem hiding this comment.
Why set these values to 0? It doesn't matter what these values are if we never use them
| upper_median = high_1 < high_2 ? high_1 : high_2; | ||
|
|
||
|
|
||
| median4_chan = (lower_median + upper_median + 1) >> 1; |
| endfunction | ||
|
|
||
|
|
||
| function automatic logic [PIXEL_W-1:0] get_red (input logic [DATA_W-1:0] px); |
There was a problem hiding this comment.
Why not just do px.red and store px as a pixel_t? This would save a lot of code and you could just read the structs fields
|
|
||
|
|
||
| logic [DATA_W-1:0] window_q [0:WINDOW_LEN-1]; | ||
| logic [DATA_W-1:0] window_d [0:WINDOW_LEN-1]; |
There was a problem hiding this comment.
Why make these logic [DATA_W-1:0] and not pixel_t?
| pixel_valid_if_o.valid = 1'b0; | ||
| pixel_valid_if_o.pixel.red = '0; |
There was a problem hiding this comment.
We can always set our pixel values equal to [color]_out. If [color]_out is garbage, we don't care. We won't read it anyway because pixel_valid_if_o.valid = 1'b1;
ixel_valid_if_o.pixel.red = red_out;
pixel_valid_if_o.pixel.green = green_out;
pixel_valid_if_o.pixel.blue = blue_out;
DonnyC123
left a comment
DonnyC123
left a comment
There was a problem hiding this comment.
Good job on the testbench code.
The hardware looked mostly good. There are just a few small changes that I think you should make
2 participants
I am a pull