Ripping into what you've written...
Code Verilog - [expand] |
1
2
3
4
| module state (
input btn, clk, rst,
output [1:0] ld
); |
It's better to use one port on separate lines, primarily because you can then add comments describing the function of each port. Also lining things up in collumns makes reading things easier:
Code Verilog - [expand] |
1
2
3
4
5
6
| module state (
input btn, // async button press, active ___?___
input clk, // ?? MHz clock input
input rst, // async/sync reset
output [1:0] ld // led output count value, in binary
); |
See, this means your code is becoming self documenting and when you or someone else picks it up later they can easily see what you are doing.
Code Verilog - [expand] |
1
2
| reg [1:0] count = 0; //gives values from binary 0-3
reg enable = 1; //enable whenever the button is released |
enable is not necessary for the functioning of this circuit, and is just making things more complicated.
Code Verilog - [expand] |
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
| always @ (posedge clk) begin
if (rst) // synchronous reset! I hope there is an upper level file, which syncrhonizes this to clk!!
count <= 0;
if (btn) begin // Need to debounce and syncrhonize to clk!!
if (enable) begin
count <= count+1;
enable <= 0;
if (count > 3) // Uh, count can never reach a value greater than 3 and will rollover to 0 anyway.
count <= 0;
end
end else // pet peave, I can't stand mixed use of begin/end and none used. I always use them.
enable <= 1;
end
assign ld = count;
endmodule |
You were using blocking assignments (I fixed that).
The problems here are the lack of synchnization of rst and btn to clk, unless that is done in a file that you did not include (I did say post all your code).
The
if (rst) should be part include as part of the code that follows
if (btn) begin. As it is written now the synthesis tool has to determine if the reset is priority encoded over the
if (btn) begin code as they both affect the count value generated. Also if the two if statements were separated into different always blocks you would end up with a multiple driver error in your code. This might synthesize right now but is bad coding practice IMO.
The enable isn't helping and is actually making this more complicated looking. The original code I posted contains a rising edge detector, which generates a single clock cycle pulse (which is embedded in the compare) from the level output of a
debounce circuit that uses a synchronized button press.
As I noted in the code the
count > 3 does nothing and will be removed by synthesis. I've been seeing a lot of problems with being able to count and decide when to stop counting or rollover these past couple of months. Are schools not teaching students how to count anymore!?