2
\$\begingroup\$

I made a simple 0 to 9 up counter using Verilog. The output is 4 LEDs which are turned on when their corresponding bit is 1. The code synthesizes fine, but on the FPGA, only one LED lights on and off repeatedly. The other three LEDs do not light up at all. Here is my code:

module counter(
   input wire clock,
   input wire reset,

   output wire [3:0] o_number
);

reg [3:0] cur_state;
reg [3:0] next_state;

always @(posedge clock or posedge reset) begin
   if (reset) cur_state <= 4'b0;
   else 
    cur_state <= next_state;
end

// Next state function
always @(*) begin
   if (cur_state == 4'b1001) next_state = 4'b0000;
   else next_state = cur_state + 4'b0001;
end

 // Output function
assign o_number = cur_state;

endmodule

module display(
   input wire [3:0] inumber,

   output wire [3:0] LEDs
);

assign LEDs = inumber;

endmodule

None of the LEDs are broken as they all light up when tested individually. Clock has been adjusted so its period is around 2 seconds.

Please help me out. Thank you.

I revised the counter module. Here are all the files I used:

top.v

module top ( input wire clock, input wire reset, output wire [3:0] LEDs);
counter c1 (.clock(clock), .reset(reset), .o_number(number));
display d1 (.inumber(number), .LEDs(LEDs));
endmodule

counter.v

module counter ( input wire clock, input wire reset, output wire [3:0] o_number);
reg [3:0] cur_state;
always @ (posedge clock or posedge reset) begin
    if (reset) cur_state[3:0] <= 4'b0000;
    else begin
        if (cur_state[3:0]==4'b1001) cur_state[3:0] <= 4'b0000;
        else cur_state[3:0] <= cur_state[3:0] + 4'b0001;
    end
end
assign o_number[3:0]=cur_state[3:0];
endmodule

display.v

module display( input wire[3:0] inumber, output wire [3:0] LEDs);
assign LEDs[3:0] = inumber[3:0];
endmodule

top.ucf

NET "LEDs[0]" LOC = K12;
NET "LEDs[1]" LOC = P14;
NET "LEDs[2]" LOC = L12;
NET "LEDs[3]" LOC = N14;

NET "clock" LOC = T9;
NET "reset" LOC = L14;
\$\endgroup\$
  • \$\begingroup\$ Wouldn't you want the next state to change on the clock signal rather than * ?? \$\endgroup\$ – Makoto Dec 5 '16 at 11:22
  • \$\begingroup\$ It is changing at the positive edge of the clock. \$\endgroup\$ – HelperKing Dec 5 '16 at 11:22
  • \$\begingroup\$ You change to the next state on clock but you change the state of nextState on * \$\endgroup\$ – Makoto Dec 5 '16 at 11:24
  • \$\begingroup\$ Would that be a big problem? \$\endgroup\$ – HelperKing Dec 5 '16 at 11:25
  • 1
    \$\begingroup\$ There is nothing wrong from a Verilog perspective. My guessing only the LED mapped to LEDs[3] is the only one lighting up. If that is the case, try slowing down your clock to 1/4 its current frequency. If that does not work, double check the mapping in your top.ucf \$\endgroup\$ – Greg Dec 6 '16 at 4:33
3
\$\begingroup\$

There was a problem with the top module. I did not declare the wire that is used to connect the display and counter module.

Here is the revised top.v:

module top ( input wire clock, input wire reset, output wire [3:0] LEDs);
wire [3:0] number;
counter c1 (.clock(clock), .reset(reset), .o_number(number));
display d1 (.inumber(number), .LEDs(LEDs));
endmodule

The LEDs light up correctly according to each state now.

\$\endgroup\$
  • 2
    \$\begingroup\$ There should have been warnings or errors about it. Can you please compile with this declaration and without it and see the difference in compiler/synthesizer messages. \$\endgroup\$ – Anonymous Dec 6 '16 at 9:34
  • \$\begingroup\$ Actually, there were no warnings or any messages indicating this problem. The documentation says that when no wire is specified in the top module, a one-bit wire is automatically assumed. Hence, I got the result where only one LED was working. \$\endgroup\$ – HelperKing Dec 8 '16 at 1:51
1
\$\begingroup\$

My guess the issue may be in using cur_state and next_state without indexes. Try

always @(posedge clock or posedge reset) begin
    if (reset) cur_state[3:0] <= 4'b0;
    else begin
        cur_state[3:0] <= next_state[3:0];
end

always @(*) begin
    if (cur_state[3:0] == 4'b1001) next_state[3:0] <= 4'b0000;
    else next_state[3:0] <= cur_state[3:0] + 4'b0001;
end

and

assign o_number[3:0] = cur_state[3:0];

and

assign LEDs[3:0] = inumber[3:0];

Note me adding [3:0] everywhere and changing = to <= in second always construct.

The goal can be achieved by much simpler configuration:

module counter(
input wire clock,
input wire reset,
output wire [3:0] LEDs
);

reg [3:0] cur_state;
wire reset_n=~reset;
assign LEDs[3:0] = cur_state[3:0];

always @(posedge clock or negedge reset_n) begin
    if (!reset_n) cur_state[3:0] <= {4{1'b0}};
    else begin
        if (cur_state[3:0] == 4'b1001) cur_state[3:0] <= {4{1'b0}};
        else cur_state[3:0] <= cur_state[3:0] + 4'b1;
    end
end

Update: It did not help - then first thing to do it just to assign LEDs[3:0] with 4'b1111 and see if all LEDs are lit.

Next possible cause of issue is always @(*) begin which is being triggered on change of any input wires for the contents always construct. The only event it happens when you issue reset and cur_state changes to 0, and thus cur_state becomes 4'b0001 and keeps this value.

Update 1: So you have LSB working, other 3 bits not working. You tested that all LEDs are lit when you write 4'b1111 to LEDs's wires. I think code is correct, there's something else being wrong. Consider the following:

  • simplify changing from decimal to hexadecimal. In this case you will only have cur_state[3:0] <= cur_state[3:0] + 4'b1; without any conditions checking for state #10

    always @ (posedge clock or posedge reset) begin if (reset) cur_state[3:0] <= 4'b0000; else cur_state[3:0] <= cur_state[3:0] + 4'b0001; end

  • consider what happens to reset during test. Are you sure it is steady low? Alternatively, remove it from the configuration at all:

    always @ (posedge clock) begin cur_state[3:0] <= cur_state[3:0] + 4'b0001; end

  • synthesize my code above with only one module and reset_n to see if it does not work too.

  • try using other LEDs:

    always @ (posedge clock) begin cur_state[3:1] <= cur_state[3:1] + 3'b001; end

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer, but it didn't solve the problem. I also want to implement it using two modules, so simplification is not needed. I'm using Spartan 3 FPGA board, by the way. \$\endgroup\$ – HelperKing Dec 5 '16 at 11:08
  • 1
    \$\begingroup\$ I updated the answer. I think the cause of issue is always @(*). You can keep two modules, but there should be no need to complicate first module with two always constructs. Implementation should be as concise and readable as possible - this is a goal of any programming language including Verilog. \$\endgroup\$ – Anonymous Dec 5 '16 at 11:22
  • \$\begingroup\$ I can't quite understand the last paragraph of your answer. Can you please explain it again? \$\endgroup\$ – HelperKing Dec 5 '16 at 11:27
  • 1
    \$\begingroup\$ Add your new code to the question (but do not remove old). I can not guess what you tried. Did you prove that all LEDs are working by writing all 1s into output register? My last paragraph says that always @(*) seem to not perform what you want from it. RTFM on what it does and, most importantly, when. \$\endgroup\$ – Anonymous Dec 5 '16 at 12:37
  • 1
    \$\begingroup\$ I updated answer with update #1. Your latest code looks ok for me, thus you should exclude statement by statement to see when you start it working as expected. \$\endgroup\$ – Anonymous Dec 5 '16 at 14:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.