VHDL and State Machine and if..then... with no else

AMCC · Sep 10, 2012

VHDL and State Machine and if..then... with no else... and other code Styling

Hi All,

This is a post about GOOD/BAD coding style or even ACCEPTABLE code style. I hope to have (with YOUR contribution) some guidelines for avoiding some particular problems... that seems to be more common than desired.

My thanks in advance.

I have recently received a task of improving some features in an existing VHDL design.

After some looks to the code, which has a QUITE different style of my own ... I was raised with some question.

I will just post one process (a hell of big process... Sorry.. I was about to clip-it when I realized that you might miss some "features".)

So... what I submit to you the code and some styling code choices made... and submit to your consideration. Please feel free to comment on SHOULD and SHOULDN'T throughout this code. I express my personal opinion...

Single Process State Machine: This is a Moore machine, which I prefer to use two or three processes. Mixing everything seems (to me) a very big mess. Your comments? (Would you go to two or three processes: state-nextstate; state transition; output combinatorial).
Synchronous State machine and Initialization: Seems fine to me. Initialization is made on 'Reset'. Any negative consideration?
(BIG DOUBT!!) 'Initialization/Default' on 'first' "else": What is you opinion regarding these assignments? (ex. Rd_EnReg <= Rd_En; on line 14). This will be interpreted by the synthesizer as a 'default' value. Your thoughts?
(ANOTHER BIG DOUBT!!: 'Rd_AckReg' is assigned in TWO different 'if' statements (lines 25, 30)... STILL PRIOR to state machine description. For me this is a 'bomb', either for code analysis... or even for synthesis. What do you think?
if...then... end if: What is you opinion regarding the use of if..then statements WITHOUT any 'else' statement? This is something that I tend to avoid (due to implicit memory of VHDL). Do you find it an acceptable coding? Do you have argument either in favor or against this?
I have found a good coding style guide (**broken link removed**)... that include some issues above... but not all. In any case you view is very welcome.

I highly appreciate your feedback. Thank you very much for your (LONG) reading of this .
Best Regards

AMCC

P.S... AND NOW THE CODE:

Code VHDL - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
process (Reset, clk)
   begin
      if clk'event and clk = '1' then
         if Reset = '1' then
            State       <= IDLE;
            RAM_EnReg   <= '0';
            RAM_WrReg   <= '0';
            RdReg       <= '0';
            Rd_EnReg    <= '0';
            Rd_AckReg   <= '0';
         else
            RAM_EnReg   <= '0';
            RAM_WrReg   <= '0';
            RAM_Wr_Last <= '0';
            Rd_EnReg    <= Rd_En;
            
            if Rd_EnReg = '1' and RdReg = '0' then
               RdReg <= '1';  -- will disactivate after completing this read operation
               Rd_AddrReg((Rd_Addr'high - 3) downto 0) <= Rd_Addr(Rd_Addr'high downto 3);  -- converting to 64b wise addressing
               Rd_SizeReg <= (others => '0');
               if Rd_Size(2 downto 0) = 0 then
                  Rd_SizeReg((Rd_Size'high - 3) downto 0) <= Rd_Size(Rd_Size'high downto 3);
               else
                  Rd_SizeReg((Rd_Size'high - 2) downto 0) <= ('0' & Rd_Size(Rd_Size'high downto 3)) + 1;
               end if;
               Rd_AckReg <= '1';
            end if;
            
            if Rd_EnReg = '0' then
               Rd_AckReg <= '0';
            end if;
 
            RAM_DataIn <= FIFO_In_Data;
            
            case State is
               when IDLE =>
                  if RAM_Rdy = '1' then
                     if RAM_Wr_Rdy = '1' and FIFO_In_AlmostEmpty = '0' and FIFO_In_Addr(FIFO_In_Addr'high) = '1' then
                        -- BL8 write
                        State <= WRITING_BL8;
                        RAM_Cmd <= "000";  -- write command
                        RAM_EnReg <= '1';
                        RAM_Addr <= FIFO_In_Addr(RAM_Addr'high downto 0);
                        RAM_WrReg <= '1';
 
                     elsif RAM_Wr_Rdy = '1' and FIFO_In_Empty = '0' and FIFO_In_Addr(FIFO_In_Addr'high) = '0' then
                        -- BL4/BC4 write
                        RAM_Cmd <= "000";  -- write command
                        RAM_EnReg <= '1';
                        RAM_Addr <= FIFO_In_Addr(RAM_Addr'high downto 0);
                        RAM_WrReg <= '1';
                        RAM_Wr_Last <= '1';
 
                     elsif RdReg = '1' and Rd_AckReg = '0' and FIFO_Out_ProgFull = '0' then
                        -- BL8 read
                        RAM_Cmd <= "001";  -- read command
                        RAM_EnReg <= '1';
                        RAM_Addr <= Rd_AddrReg(RAM_Addr'high downto 0);
                        
                        if Rd_SizeReg >= 8 then
                           Rd_AddrReg <= Rd_AddrReg + 8;  -- BL8 read
                           Rd_SizeReg <= Rd_SizeReg - 8;
                        else
                           Rd_SizeReg <= (others => '0');
                           RdReg <= '0';
                        end if;
                     end if;
                  end if;
               when WRITING_BL8 =>
                  if RAM_Wr_Rdy = '1' then
                     State <= IDLE;
                     RAM_WrReg <= '1';
                     RAM_Wr_Last <= '1';
                  end if;
               when others =>
                  State <= IDLE;
            end case;
         end if;
      end if;
   end process;

barry · Sep 10, 2012

I didn't look at this THAT closely, but I think you are right, it is kind of a mess.

The one thing that jumps out at me is in the ELSE block after "if RESET='1'..." for signals RAM_EnReg and RAM_WrReg get assignments, but these signals are also assigned in the CASE block. The synthesis tools will, I think, not flag this as an error, but the results may not be what you expect.

Also, the IF/ELSE blocks don't assign values to all signals for each IF/ELSE segment. I think this is ok, but I generally prefer to assign all signals just for completeness. This is a personal preference, but it will absolutely avoid unintended results. (In the past I think some tools would give erroneous results for incomplete IFs).

If it were me, I'd start from scratch and re-write this, rather than try and fix somebody else's catastrophe.

FvM · Sep 10, 2012

I don't believe that changing the code to a standard two process state machine improves anything.

This is a kind of poor man's state machine with just two states. Most of the code's action isn't commanded by state information and most of the actual system state information isn't reflected in the state variable. This results first of all in poor code readability.

I don't think that any of the other points 2 to 6 is particularly bringing up problems. Some design decision may or may not have specific reasons, e.g. using a synchronous reset. Without knowing the application enviroment this can't be judged.

TrickyDicky · Sep 10, 2012

barry said:
I didn't look at this THAT closely, but I think you are right, it is kind of a mess.

The one thing that jumps out at me is in the ELSE block after "if RESET='1'..." for signals RAM_EnReg and RAM_WrReg get assignments, but these signals are also assigned in the CASE block. The synthesis tools will, I think, not flag this as an error, but the results may not be what you expect.

Also, the IF/ELSE blocks don't assign values to all signals for each IF/ELSE segment. I think this is ok, but I generally prefer to assign all signals just for completeness. This is a personal preference, but it will absolutely avoid unintended results. (In the past I think some tools would give erroneous results for incomplete IFs).

If it were me, I'd start from scratch and re-write this, rather than try and fix somebody else's catastrophe.

There is no errors or latches, because the whole process is clocked. So you can have as many if's without else's as you want, and theres no problem. This is the strength of single process state machines

answer to the specific questions

1. forget the idea that a single process state machine is only moore, or mealy. You can make a mealy machine by either changing the outputs on the state transitions, or create some async logic outside of the clocked process that looks at the current state and inputs.

2. there is no async reset in the code posted, so there is no initialisation, unless they are given an initialisation on signal declaration.

3. This is fine and good coding. It means these signals will take this value unless overridden below. remember signals take the last thing assigned to them, and as its a clocked process, its all good. Its like putting default assignments in the asynchronous process when you use a two process style (like you alwayd did anyway right?)

4. not a problem. Its a synchronous process, and just takes the last value. This just means you get a priority encoder for the enable input of the register based on the various diufferent paths.

5. Its a synchronous process, so its no problem (and probably essential). Remember that if there is no else the if-elsif connects to the enable input of the register. with an else, it just muxes the data into d input with enable always set to '1'.

6. This is a good start, but if you look at the date, its from 2005 - synthesisers have come a long way since.
For example:
C_6 - std_logic only for ports. What an old school idea. This comes from when synthesisors would only accept std_logic. Now they will accept anything, and you should do anything that makes life easier for other designers who may read your code at a future date. So if its a number, make the port an unsigned/signed or even an integer (that is ranged). The only rule I would stick to is an array type at the top top level so you can easily map the bits to physical pins. But this means you can use almost anything except integers.

But if you read these rules, the code you posted breaks none of the rules. Because its all syncrhonous.

So the big problem with this code, is the lack of comments. Remember comments should be more about "why" than "what"

verylsi · Sep 15, 2012

Hello Tricky,

Following your post,
" There is no errors or latches, because the whole process is clocked. So you can have as many if's without else's as you want, and theres no problem. This is the strength of single process state machines "

when we call the If statement then a mux is generated and if there is no else then a latch is generated to store the previous value, this is what I knew..

You must be wright about the single process state machines.. but can you please explain , why dosn't it generate latches when else statement is not writte..

Thanks in advance..

TrickyDicky · Sep 15, 2012

anything that follows this template will generate synchronous registers, which is what you want.

Code VHDL - [expand]
1
2
3
4
5
6
process(clk)
begin
  if rising_edge(clk) then
    --ALL signal in here are registers. you cannot ever create a transparent latch with signals in here. it may be important to have no else.
  end if;
end process;

this is a very basic vhdl template. if you don't understand it, I. suggest you review your vhdl notes, or create some simple designs, synthesise them and look at them in the rtl veiwer. you will never never get latches like this.

std_match · Sep 16, 2012

verylsi said:
You must be wright about the single process state machines.. but can you please explain , why doesn't it generate latches when else statement is not writte..

A latch is a combinatorial circuit with a memory, caused by some output being fed back to an input. This is normally not wanted because the tools can't handle it well. They will detect it and give you a warning. The probability is high that the design will not work and that you will have a mismatch between a simulation and the final circuit..

A clocked process that doesn't assign an output is different. The output of the register will be fed back to it's input so the value will be unchanged.
This is very different from a latch since the behavior is controlled by the clock and the tools have no problem to handle it. A simulation will behave as the final circuit.

TrickyDicky · Sep 17, 2012

std_match said:
A clocked process that doesn't assign an output is different. The output of the register will be fed back to it's input so the value will be unchanged.

I would never expect the output to be fed back to the input unless I specifically asked it to. The control logic (ie. the if paths) should generate a clock enable.

FvM · Sep 17, 2012

I would never expect the output to be fed back to the input unless I specifically asked it to.

I think std_match is referring to the way, a clock enable is implemented in usual FPGA hardware. If the register primitive doesn't expose a clock enable (it usually doesn't) than the clock enable is implemented as a MUX at the D input between the register output and the new data. A register signal that isn't updated in a particular conditional path does the same.

VHDL and State Machine and if..then... with no else

AMCC

Member level 2

barry

Advanced Member level 7

AMCC

FvM

Super Moderator

AMCC

TrickyDicky

Advanced Member level 7

AMCC

FvM

verylsi

verylsi

Full Member level 2

TrickyDicky

Advanced Member level 7

verylsi

std_match

Advanced Member level 4

verylsi

TrickyDicky

Advanced Member level 7

FvM

Super Moderator

std_match

Similar threads

VHDL and State Machine and if..then... with no else

Member level 2

Advanced Member level 7

Super Moderator

Advanced Member level 7

Full Member level 2

Advanced Member level 7

Advanced Member level 4

Advanced Member level 7

Super Moderator

Similar threads

Privacy & Transparency

Privacy & Transparency