3
\$\begingroup\$

My professor looked at this code for a good 10 minutes, but could not find the problem. So, I'm hoping a fresh pair of eyes will see something both of us missed. As always, I'll be grateful for any hints you can provide.

Context: A module that counts from FFF to 000, then repeats.

Problem: Only lowest 2 displays decrement, skipping several numbers at a time and then jump back up to FF at a seemingly random point.

Assumptions: 1. SevenSegment driver is working properly. It has been checked manually and will display the hex number it is given 2. The ClockDivider module works as intended. The countdown decreases every 1 second as it should (CLOCK_50 = 50 MHz)

Possible Hint: Verilog warns that "cd" is an inferred latch and retains its value through one or more paths in the "always" block

module ClockDivider( input CLOCK_50, output reg[ 31:0] count );
parameter clockDivisor = 50_000_000;

always @( posedge CLOCK_50 )
    if ( count == 0)
        count <= clockDivisor;
        else
            count <= count - 1;
endmodule


module Test (

    //////////// CLOCK //////////
    input                       CLOCK_50,
    //////////// SEG7 //////////
    output           [6:0]      HEX0,
    output           [6:0]      HEX1,
    output           [6:0]      HEX2
);

//ClockDivider Output
wire                [31:0]          cout;

reg                 [11:0]            cd;

ClockDivider a( CLOCK_50, cout);

always @(cout)
begin

if (cout == 32'h0)
        if (cd == 12'h0)
                cd <= 12'hFFF;
        else
                cd <= (cd - 12'h001);

end

SevenSegment    C2( cd[11:8],  1'b0, HEX2 ); 
SevenSegment    C1( cd[7:4],   1'b0, HEX1 );
SevenSegment    C0( cd[3:0],   1'b0, HEX0 );

endmodule
\$\endgroup\$
4
\$\begingroup\$

Personally, I have had problems with always @(cout) that can be flakey.

I would always use always @(posedge CLOCK_50) . In my opinion, you are guaranteed proper results this way...

EDIT: added clearer code for above suggestion

always @(cout) -> always @(posedge CLOCK_50)

cout should only change with the positive edge of the clock in theory (or code) but what is actually happening maybe different. Your timing report will tell you about setup time and slack - and this may be your problem. change your logic to all "synchronous" and I'll bet you it works.

also, get rid of the inferred latch as good practice. all this would take is an else statement:

if (cout == 32'h0)
    if (cd == 12'h0)
        cd <= 12'hFFF;
    else
        cd <= (cd - 12'h001);
else
    cd <= cd;

EDIT: suggested working code

module ClockDivider(input CLOCK_50, 
                    output reg[ 0:0] en 
                   );
    parameter clockDivisor = 50_000_000;
    reg [31:0] i;

    always @( posedge CLOCK_50 ) begin
            if ( i == clockDivisor ) begin
                    en <= 1;
                    i <= 0;
            end
            else begin
                    en <= 0;
                    i <= i + 32'b1;
            end
    end
endmodule

//1Hz clock output
wire                [0:0]           en;
//Holds the countdown value FFF-000
reg                 [11:0]              cd;

ClockDivider CLOCK_1Hz( CLOCK_50, en);

always @(posedge CLOCK_50)
    if (en) begin
        if (cd == 12'h0)
            cd <= 12'hFFF;
        else
            cd <= (cd - 12'h001);
    end 
    else 
        cd <= cd;
end
\$\endgroup\$
  • \$\begingroup\$ johnnymopo, thank you for your reply. The revised code still brought about an inferred latch (see ThePhoton above). And CLOCK_50 (50 MHz) would have been too fast for the countdown. But thanks to your suggestion of using posedge, I managed to figure it out. So wherever you are in this world, I wish the best of luck to you and thanks again for helping out a random stranger. \$\endgroup\$ – Meridian Nov 7 '15 at 7:05
  • \$\begingroup\$ Just to be clear, adding else cd <= cd; still creates a latch; all you've done is make it explicit rather than implicit. In general, combinatorial latches created in this way should be avoided. \$\endgroup\$ – Dave Tweed Nov 7 '15 at 12:01
  • \$\begingroup\$ @dave tweed, how would you write this to avoid a latch at all if you want to make synchronous logic with a common clock domain? cd has to be something.... I ask as I do this and it seems to work well, at least to 125 MHz where I've been working at. But I have no formal vhdl training. \$\endgroup\$ – johnnymopo Nov 7 '15 at 15:16
  • \$\begingroup\$ @johnnymopo, I think your answer would be more clear if you included the always @(<xxx>) that you recommend. \$\endgroup\$ – The Photon Nov 7 '15 at 15:43
  • \$\begingroup\$ Added code suggestion \$\endgroup\$ – johnnymopo Nov 7 '15 at 18:38
2
\$\begingroup\$

Thanks to ThePhoton and johhnymopo I was able to figure out the solution. The solution was to have ClockDivisor act as a slower clock itself. The code becomes:

module ClockDivider( input CLOCK_50, output reg[ 0:0] count );
    parameter clockDivisor = 50_000_000;
    reg [31:0] i;

always @( posedge CLOCK_50 )
    begin
            i <= i + 32'b1;
            if ( i == clockDivisor )
                    begin
                    count <= 1;
                    i <= 0;
                    end
            else
                    count <= 0;
    end

endmodule

In this case, this is a 1Hz Clock. The countdown decrement can then be triggered using the posedge of that slower clock:

//1Hz clock output
wire                [0:0]           slowClock;

//Holds the countdown value FFF-000
reg                 [11:0]              cd;

ClockDivider CLOCK_1Hz( CLOCK_50, slowClock);

always @(posedge slowClock)
begin

if (cd == 12'h0)
    cd <= 12'hFFF;
else
    cd <= (cd - 12'h001);

end
\$\endgroup\$
  • \$\begingroup\$ Glad it worked. I would personally try to make it all work with one clock. I believe this would infer a new clock and on a larger design this could make timing more difficult. Could you post the exact inferred latch warning? \$\endgroup\$ – johnnymopo Nov 7 '15 at 15:31
  • \$\begingroup\$ @johnnymopo The exact warning was: "inferring latch(es) for variable "cd", which holds its previous value in one or more paths through the always construct". \$\endgroup\$ – Meridian Nov 7 '15 at 16:14
  • \$\begingroup\$ I wonder if this happens because slowClock is not a new clock domain in synthesis? if that were the case, cd changes once per second but still based on the 50 MHz clock, and hence, still a latch - but that seems weird. I still recommend using a common clock for all logic when possible. \$\endgroup\$ – johnnymopo Nov 7 '15 at 17:51

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.