Undefined output on Verilog Testbench

[Alberto]

Hey guys,

Try as I might I can't find a fix to this problem, this verilog code is designed to make the FPGA act like a simple 4026 Decade Counter. However when I run the testbench all I get is undefined values on the outputs.  Previous simpler code I have written for things like counters, in exactly the same way, has been fine. But this doesn't seem to work! My untrained eye cannot spot the mistakes or fundamental errors I have made, please can you help me?

Here's the code:

// This is the Verilog Code for a 4026 Decade Counter, complete with all of it's functionality including enables, etc.
// This shall be a work in progress as I learn more about verilog code, eventually this should end in some highly efficient Verilog code,
// as I become more profficient with the language.

// Creating Initial Module
module ic (clock, a, b, c, d, e, f, g);

// Defining Input and Output Ports
input clock;
output a, b, c, d, e, f, g;

// Defining Port Types
reg a, b, c, d, e, f, g;


// Creating Tracking Variables
reg [4:0] counter, counter_n;

// Main behavoural code
always @ (posedge clock) begin
   counter_n = counter + 1;
   if (counter == 0) begin
      a = 1; b = 1; c = 1; d = 1; e = 1; f = 1; g = 0;
   end else if (counter == 1) begin
      a = 0; b = 1; c = 1; d = 0; e = 0; f = 0; g = 0;
   end else if (counter == 2) begin
      a = 1; b = 1; c = 0; d = 1; e = 1; f = 0; g = 1;
   end else if (counter == 3) begin
      a = 1; b = 1; c = 1; d = 1; e = 0; f = 0; g = 1;
   end else if (counter == 4) begin
      a = 0; b = 1; c = 1; d = 0; e = 0; f = 1; g = 1;
   end else if (counter == 5) begin
      a = 1; b = 0; c = 1; d = 1; e = 0; f = 1; g = 1;
   end else if (counter == 6) begin
      a = 1; b = 0; c = 1; d = 1; e = 1; f = 1; g = 1;
   end else if (counter == 7) begin
      a = 1; b = 1; c = 1; d = 0; e = 0; f = 0; g = 0;
   end else if (counter == begin
      a = 1; b = 1; c = 1; d = 1; e = 1; f = 1; g = 1;
   end else if (counter == 9) begin
      a = 1; b = 1; c = 1; d = 1; e = 0; f = 1; g = 1;
   end
   counter = counter_n;
end
endmodule

// Testbench
module ic_tb;

reg clock;
wire a, b, c, d, e, f, g;

initial begin
   $monitor ("clock=%b, a=%b, b=%b, c=%b, d=%b, e=%b, f=%b, g=%b", clock, a, b, c, d, e, f, g);
   clock = 0;
   #5;
   #10;
   #10;
   #10;
   #10;
   #10 $finish;
end

always begin
   #5 clock = !clock;
end

ic ic_1 (.clock(clock), .a(a), .b(b), .c(c), .d(d), .e(e), .f(f), .g(g));
 
endmodule


Thanks very much for any help in advance!

Alberto

[langwadt]

you don't have a reset or at least a initial value for your registers

[Alberto]

Oh ok so I need to define the state of the counter at the start. Seems obvious now! Doh!

Thank's very much for the help!

[Alberto]

Hey,

I tried to implement some form of reset but I'm still getting the same output! Is there anything I'm doing wrong still?

Here's the new code:

// This is the Verilog Code for a 4026 Decade Counter, complete with all of it's functionality including enables, etc.
// This shall be a work in progress as I learn more about verilog code, eventually this should end in some highly efficient Verilog code,
// as I become more profficient with the language.

// Creating Initial Module
module ic (clock, reset, chip_enable, display_enable, a, b, c, d, e, f, g);

// Defining Input and Output Ports
input clock, chip_enable, display_enable, reset;
output a, b, c, d, e, f, g;

// Defining Port Types
reg a, b, c, d, e, f, g;


// Creating Tracking Variables
reg [4:0] counter, counter_n;

// Main behavoural code
always @ (posedge clock) begin
   counter_n = counter + 1;
   if (counter == 0) begin
      a = 1; b = 1; c = 1; d = 1; e = 1; f = 1; g = 0;
   end else if (counter == 1) begin
      a = 0; b = 1; c = 1; d = 0; e = 0; f = 0; g = 0;
   end else if (counter == 2) begin
      a = 1; b = 1; c = 0; d = 1; e = 1; f = 0; g = 1;
   end else if (counter == 3) begin
      a = 1; b = 1; c = 1; d = 1; e = 0; f = 0; g = 1;
   end else if (counter == 4) begin
      a = 0; b = 1; c = 1; d = 0; e = 0; f = 1; g = 1;
   end else if (counter == 5) begin
      a = 1; b = 0; c = 1; d = 1; e = 0; f = 1; g = 1;
   end else if (counter == 6) begin
      a = 1; b = 0; c = 1; d = 1; e = 1; f = 1; g = 1;
   end else if (counter == 7) begin
      a = 1; b = 1; c = 1; d = 0; e = 0; f = 0; g = 0;
   end else if (counter == begin
      a = 1; b = 1; c = 1; d = 1; e = 1; f = 1; g = 1;
   end else if (counter == 9) begin
      a = 1; b = 1; c = 1; d = 1; e = 0; f = 1; g = 1;
   end else if (counter == 10) begin
      counter <= 0;
   end
     counter <= counter_n;
     if (reset == 0) begin
        counter <= 0;
    end
end
endmodule

// Testbench
module ic_tb;

reg clock, chip_enable, display_enable, reset;
wire a, b, c, d, e, f, g;

  ic ic_1 (.clock(clock), .reset(reset), .chip_enable(chip_enable), .display_enable(display_enable), .a(a), .b(b), .c(c), .d(d), .e(e), .f(f), .g(g));

initial begin
  $monitor ("clock=%b, reset=%b, a=%b, b=%b, c=%b, d=%b, e=%b, f=%b, g=%b", clock, reset, a, b, c, d, e, f, g);
   clock = 0;
     reset = 0;
   #5 reset = 1;
   #10;
   #10;
   #10;
   #10;
   #10 $finish;
end

always begin
   #5 clock = !clock;
end
endmodule


Thanks for any help in advance!

Alberto

[Alberto]

Ok, an even further update, I am sort of spamming this with reply's :S

I have managed to get an output with this code:

// This is the Verilog Code for a 4026 Decade Counter, complete with all of it's functionality including enables, etc.
// This shall be a work in progress as I learn more about verilog code, eventually this should end in some highly efficient Verilog code,
// as I become more profficient with the language.

// Creating Initial Module
module ic (clock, reset, enable, a, b, c, d, e, f, g);

// Defining Input and Output Ports
input clock, reset, enable;
output a, b, c, d, e, f, g;

// Defining Port Types
reg a, b, c, d, e, f, g;


// Creating Tracking Variables
reg [3:0] counter, counter_n;

// Main behavoural code
always @ (posedge clock) begin
     if (counter == 4'b0000) begin
      a = 1; b = 1; c = 1; d = 1; e = 1; f = 1; g = 0;
     end
     if (counter == 4'b0001) begin
      a = 0; b = 1; c = 1; d = 0; e = 0; f = 0; g = 0;
    end
     if (counter == 4'b0010) begin
      a = 1; b = 1; c = 0; d = 1; e = 1; f = 0; g = 1;
    end
     if (counter == 4'b0011) begin
      a = 1; b = 1; c = 1; d = 1; e = 0; f = 0; g = 1;
    end
     if (counter == 4'b0100) begin
      a = 0; b = 1; c = 1; d = 0; e = 0; f = 1; g = 1;
    end
     if (counter == 4'b0101) begin
      a = 1; b = 0; c = 1; d = 1; e = 0; f = 1; g = 1;
    end else if (counter == 4'b0110) begin
      a = 1; b = 0; c = 1; d = 1; e = 1; f = 1; g = 1;
    end    
     if (counter == 4'b0111) begin
      a = 1; b = 1; c = 1; d = 0; e = 0; f = 0; g = 0;
    end
     if (counter == 4'b1000) begin
      a = 1; b = 1; c = 1; d = 1; e = 1; f = 1; g = 1;
    end
     if (counter == 4'b1010) begin
      counter <= 0;
   end
     else begin
      a = 1; b = 1; c = 1; d = 1; e = 0; f = 1; g = 1;
   end
     if (reset == 1'b1) begin
       counter <= 4'b000;
    end
     if (enable == 1'b1) begin
       counter_n <= counter + 1'b1;
    end
   counter <= counter_n;
end
endmodule

// Testbench code beginning here
module ic_tb;

reg clock, reset, enable;
wire a, b, c, d, e, f, g;

  ic ic_1 (.clock(clock), .reset(reset), .enable(enable), .a(a), .b(b), .c(c), .d(d), .e(e), .f(f), .g(g));

initial begin
  $monitor ("clock=%b, reset=%b, enable=%b, a=%b, b=%b, c=%b, d=%b, e=%b, f=%b, g=%b", clock, reset, enable, a, b, c, d, e, f, g);
     clock = 0;
     reset = 0;
     enable = 0;
   #5 reset = 1;
   #10 reset = 0;
     enable = 1;
   #10;
   #10;
   #10;
   #10;
   #10 $finish;
end

always begin
   #5 clock = !clock;
end
endmodule

However it is a constant output from the else statement at the end of the behavioral code! I still don't know what I am doing wrong!

Here is the output for reference:

clock=0, reset=0, enable=0, a=x, b=x, c=x, d=x, e=x, f=x, g=x
clock=1, reset=1, enable=0, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=0, reset=1, enable=0, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=1, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=0, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=1, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=0, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=1, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=0, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=1, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=0, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=1, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
clock=0, reset=0, enable=1, a=1, b=1, c=1, d=1, e=0, f=1, g=1
$finish called from file "design.sv", line 83.
$finish at simulation time                   65
           V C S   S i m u l a t i o n   R e p o r t
Time: 65 ns
CPU Time:      0.240 seconds;       Data structure size:   0.0Mb
Tue Jan 19 20:54:50 2016
Done

Once again thanks for any help in advance!

[langwadt]

I'll give you a few hints.

your always@ only get evaluated on the posedge clock so unless reset overlap a positive clock edge you will never get to your reset

usually you would do something like this to get an asynchronous reset :

always@(posedge clock or posedge reset)
begin
  if(reset)
  begin
  ...
  end
   else
  begin
  ...
  end
end

you are mixing =  and <= ,  that is bad

counter <= counter_n overrides the counter = 0 in the if(counter == 10)  (which should probably have been in the counter == 9)

the code would probably be easier to read with a case statement instead of the long if-else

[Alberto]

Once again thanks very much for your reply, this is all really helpful!

I will make the amendments you suggested, should have remembered not to mix <= and =, I've seen that that's not really a great thing to do in a tutorial. However, why does the counter <= counter_n override the counter = 0 ? I thought the <= operator meant that you were applying things in parallel, or am I just getting confused?

I tried to use a case statement when I first wrote the code, however for some reason it would not let me change the value of more than one of the outputs. Again this is probably just me being stupid.

Thanks for your patience, I'm just learning so all of this is super helpful!

Alberto

[langwadt]

Once again thanks very much for your reply, this is all really helpful!

I will make the amendments you suggested, should have remembered not to mix <= and =, I've seen that that's not really a great thing to do in a tutorial. However, why does the counter <= counter_n override the counter = 0 ? I thought the <= operator meant that you were applying things in parallel, or am I just getting confused?

I tried to use a case statement when I first wrote the code, however for some reason it would not let me change the value of more than one of the outputs. Again this is probably just me being stupid.

Thanks for your patience, I'm just learning so all of this is super helpful!

Alberto

I find there is rarely a good reason to use = in a clocked always block
see the differences here: http://www.asic-world.com/tidbits/blocking.html

counter <= counter_n override the counter <= 0 because for <= assignments it is only the last that takes effect

basically if you stick to <= everything you read is what is was before the clock edge and the last assignment
you make is what you will read on the next clock edge

i.e. for 0-9 counter

always@(posedge clk)
begin
   cnt <= cnt + 1;
  if(cnt == 9) cnt <= 0;
end

you probably forgot the begin-end for multiline cases

i.e.

case(cnt)
4'd0: begin
           a <= 1'b1;
           b <= 1'b0;
         end
4'd1: begin
           a <= 1'b0;
           b <= 1'b1;
         end
....
endcase
 

[Alberto]

Again super helpful, thanks so much! A slightly more broad question if I may? How would you say it is best to learn Verilog? Should I do it by example and complete little projects where I learn something new each time, but not in a particularly set order. Or should I read the text based tutorials through and try some projects at the end?

Again thanks so much for all the help, I wouldn't have got this far without it!

Alberto

[marshallh]

Code: [Select]

module ic (
input wire clk,
input wire reset_n,
input wire enable,

output wire a, b, c, d, e, f, g
);

reg [6:0] out;
assign {a, b, c, d, e, f, g} = out;

reg [3:0] counter;

always @(posedge clk or negedge reset_n) begin
if(~reset_n) begin
// reset stuff put here
counter <= 0;
end else begin
// normal opoeration
case(counter)
0: out <= 7'b1111110;
1: out <= 7'b0110000;
2: out <= 7'b1101101;
3: out <= 7'b1111001;
4: out <= 7'b0110011;
5: out <= 7'b1011011;
6: out <= 7'b1011111;
7: out <= 7'b1110000;
8: out <= 7'b1111111;
9: out <= 7'b?; //what
default: out <= 9l
endcase

if(enable) counter <= counter + 1'b1;
end
end

endmodule


You might need to add the counter extra reg and flesh out the case statement. 90% of the verilog stuff out there is garbage. There is very little good code on Opencores that I would recommend for learning. But even if it sucks you can still learn something from it.
You can join ##fpga on freenode IRC and ask questions. Getting expert direction while doing independent work is the quickest way to success. Don't waste time with Verilog books, focus on synthesizable verilog (the bit that isn't can be used for testbenches, but not much is needed there)

[azer]

It was missing the forced rollover of the counter, looks like it runs ok now.

Code: [Select]

module ic (
input wire clk,
input wire reset_n,
input wire enable,

output wire a, b, c, d, e, f, g
);

reg [6:0] out;
assign {a, b, c, d, e, f, g} = out;

reg [3:0] counter;

always @(posedge clk or negedge reset_n) begin
 if(!reset_n) begin
  // reset stuff put here
  counter <= 3'd0;
  out <= 7'b1111110;
 end else begin
  // normal opoeration
  case(counter)
  0: out <= 7'b1111110;
  1: out <= 7'b0110000;
  2: out <= 7'b1101101;
  3: out <= 7'b1111001;
  4: out <= 7'b0110011;
  5: out <= 7'b1011011;
  6: out <= 7'b1011111;
  7: out <= 7'b1110000;
  8: out <= 7'b1111111;
  9: out <= 7'b1111011;
  default: out <= 7'b1111110;
  endcase

  if(enable) begin
   if (counter == 4'd9)
    counter <= 4'd0;
   else
    counter <= counter + 4'd1;
  end
 end
end

endmodule

[Alberto]

Awesome thanks for the example code, that will be great to reference with mine to see what's different. I will carry on doing independent projects and keep asking questions!

Speaking of which! In the case statement, is the "default" case the value the code goes to if it has no other specific orders (one of the other cases)? I was told that I need to make sure I define a starting position, so is this a good way to do it?

Also, to define a starting state for your outputs, is it best to use a reset section and when testing activate the reset for the first few seconds of testing?

Thanks again for all the help!

Alberto

[azer]

The default is there to handle if the counter has a value other than 0-9. Since the counter is 4 bits it can be 0-15.
It is mostly to tell the synthesizer what to wire up for the remaining cases.
You need to set reset low for a clock cycle at startup to bring the counter to a defined value. Since the output is defined by the value of the counter, it will have a defined starting position after reset has been set high again.
We also put a reset value for the output so it will have a defined value while we are in reset and in the first cycle after.

[Alberto]

Ok so I need a reset in everything I do, or some other way of making everything has a definition on startup. That was the problem I was essentially running into before.

Again, thanks very much for all the help, this has cleared a lot of things up for me!

Alberto

[langwadt]

The default is there to handle if the counter has a value other than 0-9. Since the counter is 4 bits it can be 0-15.
It is mostly to tell the synthesizer what to wire up for the remaining cases.
You need to set reset low for a clock cycle at startup to bring the counter to a defined value. Since the output is defined by the value of the counter, it will have a defined starting position after reset has been set high again.
We also put a reset value for the output so it will have a defined value while we are in reset and in the first cycle after.

you  don't have to put in a default: but it is a good habit, especially for combination cases because if you don't cover all case you risk infering
latches which you almost never want.

if it is going to match a 4026 the reset has to by async so it doesn't have to be high for a whole clock cycle

and the outputs can't be registered, the have to be done combinatorical from the the counter and enable 

something along the lines of:

Code: [Select]

module cd4026(input clock,
              input reset,
              input clockinhibit,
              input enablein,
              output A,
              output B,
              output C,
              output D,
              output E,
              output F,
              output G,
              output Cungated,
              output reg carryout
);

reg [3:0] cnt;
reg [6:0] out;

assign {A,B,C,D,E,F,G} = enablein ? out : 7'b0000000;

assign Cungated = out[4];

always@(posedge clock or posedge reset)
begin
  if(reset)
  begin
    cnt  <= 4'd0;
    carryout <= 1'b1
  end
  else if(~clockinhibit)   
  begin
    cnt <= cnt + 4'd1;

    if(cnt == 4'd9)
    begin
      cnt <= 0;
      carryout <= 1'b1;
    end

    if(cnt == 4) carryout <= 1'b0;

  end
end

always@(*)
begin
  case(cnt)
  4'd0: out <= 7'b1111110;
  4'd1: out <= 7'b0110000;
  4'd2: out <= 7'b1101101;
  4'd3: out <= 7'b1111001;
  4'd4: out <= 7'b0110011;
  4'd5: out <= 7'b1011011;
  4'd6: out <= 7'b1011111;
  4'd7: out <= 7'b1110000;
  4'd8: out <= 7'b1111111;
  4'd9: out <= 7'b1111011;
  default: out <= 7'b1111110;
  endcase
end

endmodule


[Alberto]

So from what I understand, the outputs have to be wires that immediately respond to the change in the value of the counter? But they will only do so when the enable pin is not active.

So does the counter advance even though the output does not, and when the enable pin is deactivated the output will jump to the value of the counter?

Also you have used the syntax: always @(*) when putting in the case command. Do you mind me asking what this means?

Thanks for all the help again!

Edit: Also another question!

If I put the code:
a = 2;
a = 3;

Would a be 2 or 3 on the next clock edge? Would it be 2 on the first and then 3 on the second?
Sorry for going back to this, just need to understand exactly what's going on here!

[langwadt]

So from what I understand, the outputs have to be wires that immediately respond to the change in the value of the counter? But they will only do so when the enable pin is not active.

So does the counter advance even though the output does not, and when the enable pin is deactivated the output will jump to the value of the counter?

Also you have used the syntax: always @(*) when putting in the case command. Do you mind me asking what this means?

Thanks for all the help again!

Edit: Also another question!

If I put the code:
a = 2;
a = 3;

Would a be 2 or 3 on the next clock edge? Would it be 2 on the first and then 3 on the second?
Sorry for going back to this, just need to understand exactly what's going on here!

the out immediately respond to the counter, but when the enable pin is low the outputs are all 0, 

the counter  advance when clockinhibit is low

always@(*)  means when any of the relevant  signals change, could have written always@(cnt)
(*) is shorter and you don't forget any


a = 2;
a = 3;

doesn't make much sense it set a = 3 there's no clocks involved

look at it this way, what is inside an always block happens instantly when ever the always@() "triggers"

[andre_teprom]

a = 2;
a = 3;
doesn't make much sense it set a = 3 there's no clocks involved

Perhaps not strictly...
Compiler should issue an Warning due to the fact that it involves not a syncronous operation (<=), but a combinational assignment for 2 values attempting to load onto the same variable at same time, that's not right ?

[Alberto]

Ok I think I get it now! I think I was looking at it as though it would make something different happen on the rising clock edge. But having read the original link you posted again it makes sense to me. From what I understand, this code:

always @ (posedge clk) begin
       a = 2;
       a = 3;
end

would set the value of a to 3 always, a would never take the value of 2.

Is this not a synchronous operation because you are essentially "blocking" the code from continuing instead of letting it flow like you would with a non blocking statement?

[langwadt]

a = 2;
a = 3;
doesn't make much sense it set a = 3 there's no clocks involved

Perhaps not strictly...
Compiler should issue an Warning due to the fact that it involves not a syncronous operation (<=), but a combinational assignment for 2 values attempting to load onto the same variable at same time, that's not right ?

the a =2 won't have any affect so it shouldn't give you a warning,
<= or = doesn't make it synchronous  or combinatorical, it just makes it blocking or non-blocking
to make it synchronous you have to put it in an clocked always block

[langwadt]

Ok I think I get it now! I think I was looking at it as though it would make something different happen on the rising clock edge. But having read the original link you posted again it makes sense to me. From what I understand, this code:

always @ (posedge clk) begin
       a = 2;
       a = 3;
end

would set the value of a to 3 always, a would never take the value of 2.

Is this not a synchronous operation because you are essentially "blocking" the code from continuing instead of letting it flow like you would with a non blocking statement?

it is synchronous because it is a clocked always block

blocking vs. non-blocking is something else http://www.asic-world.com/tidbits/blocking.html

Don't look at verilog as if it is code, it is not, it is a description of hardware
unless you have  a very specific reason don't use blocking statements, it probably won't what you want

[Alberto]

Ok awesome, that's cleared it all up for me now. Thanks very much for your help, everything has been really informative! Time to finish up the other parts of this project such as the display enable, etc. If I need anymore help then I know where to come!

Thanks again,

Alberto