Is this a true FSM?

Question

I have a conceptual question about FSM's and whether or not the following code is a true FSM. This is for my own curiosity and understanding about the subject. When I wrote this code, I was under the impression that this was an FSM, but now I am not so sure. According to a lot of reading I have done, a TRUE FSM should only consist of a sequential state transition block, followed by either one or two combination blocks to calculate the next state and the output logic.

My current code synthesizes and works on my Basys3 board, so I understand there may be an argument for "if it ain't broke, don't fix it" but it's been bugging me for a while now that I may have an incorrect understanding about how to write an FSM in HDL.

I have tried at least 4 or 5 different ways to rewrite my code using the format mentioned above, but I can't seem to get it without inferring latches, mainly due to the use of a counter, and the fact that pckd_bcd needs to remember its previous value.

Could somebody please explain to me either why this algorithm isn't fit for a true FSM, why it is fit for a true FSM and where my misunderstanding is, or how to rewrite this into the format mentioned above.

module double_dabble #
    (
    parameter NUM_BITS = 8
    )
    (
    input   wire                    i_clk,
    input   wire                    i_rst,
    input   wire    [NUM_BITS-1:0]  i_binary,
    output  wire    [15:0]          o_pckd_bcd
    );

localparam  s_IDLE      =   3'b000;
localparam  s_SHIFT     =   3'b001;
localparam  s_CHECK     =   3'b010;
localparam  s_ADD       =   3'b011;
localparam  s_DONE      =   3'b100;

reg     [2:0]           state;
reg     [NUM_BITS-1:0]  bin;
reg     [3:0]           count;
reg     [15:0]          pckd_bcd;
reg     [NUM_BITS-1:0]  input_reg;
reg     [15:0]          output_reg;

assign o_pckd_bcd   = output_reg;

always @(posedge i_clk) 
begin
    if(i_rst)
    begin
        state       <= s_IDLE;
        bin         <= 0;
        count       <= 0;
        pckd_bcd    <= 0;
        output_reg  <= 0;
        input_reg   <= 0;
    end

    else
    begin
        input_reg   <= i_binary;
        state       <= s_IDLE;

        // FSM
        case(state)
            s_IDLE :
            begin
                state       <= s_IDLE;
                bin         <= 0;
                count       <= 0;
                pckd_bcd    <= 0;
                if (input_reg!=i_binary)
                begin
                    bin     <= i_binary;
                    state   <= s_SHIFT;
                end
            end

            s_SHIFT :
            begin
                state       <= s_CHECK;
                bin         <= bin<<1;
                count       <= count+1;
                pckd_bcd    <= pckd_bcd<<1;
                pckd_bcd[0] <= bin[NUM_BITS-1];
            end

            s_CHECK :
            begin
                state   <=  s_ADD;
                if(count>=NUM_BITS)
                    state   <=  s_DONE;                                
            end

            s_ADD :
            begin
                state           <= s_SHIFT;
                pckd_bcd[15:12] <= add_three(pckd_bcd[15:12]);
                pckd_bcd[11:8]  <= add_three(pckd_bcd[11:8]);
                pckd_bcd[7:4]   <= add_three(pckd_bcd[7:4]);
                pckd_bcd[3:0]   <= add_three(pckd_bcd[3:0]);
            end

            s_DONE :
            begin
                state       <= s_IDLE;
                output_reg  <= pckd_bcd;
            end
        endcase
    end
end

function [3:0] add_three;
    input   [3:0] bcd;
    if (bcd > 4)
        add_three   = bcd +3;
    else
        add_three   = bcd;
endfunction

endmodule

Answer 1

In general, your code looks like it implements an FSM. You have evidence that the design is working as desired on your board.

The coding style you have chosen (one always block) is a valid Verilog approach. Another common FSM coding style involves two always blocks: one sequential and one combinational. See here for an example. It is not mandatory for an FSM to use two always blocks in Verilog.

One thing that I do notice in your code is that you are making multiple nonblocking assignments to the state signal at the same time. That is unusual.

For example, right before the case statement, you have:

    state       <= s_IDLE;

Then, in the IDLE case-item, you have the same code:

            state       <= s_IDLE;

I recommend trying to clean that up by adding a default to the case statement. A default is also a good coding practice when you have some undefined states, as in your FSM. You have only defined 5 of the 8 possible states (state is a 3-bit register).

Another example of multiple nonblocking assignments is:

        s_CHECK :
        begin
            state   <=  s_ADD;
            if(count>=NUM_BITS)
                state   <=  s_DONE;                                
        end

That would be better coded as:

        s_CHECK :
        begin
            state <= (count>=NUM_BITS) ? s_DONE : s_ADD;
        end

Following recommended coding practices yields more predictable simulation and synthesis results.