What is wrong with my if-else statment in this VHDL code for a counter? [SOLVED]

[Dmeads]

Hello all!

Been really scratching my head trying to figure out what I am doing wrong here. I am a VHDL beginner so take it easy

code errors say

Code: [Select]

design.vhd:49:12: 'if' is expected instead of 'process'
design.vhd:49:11: missing ";" at end of statement
design.vhd:49:12: 'end' is expected instead of 'process'
you can see the edaplayground link for yourself. https://www.edaplayground.com/x/8bqF

I really thought I had my end if statments in the right place, but maybe i have been staring at this for too long, a fresh pair of eyes would help.

Thanks, here is the code (the error is somewhere in the COUNT_PROC0 near the end of the entity):

Code: [Select]

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity decade_counter_behavioral is
port (
i_clk   : in std_logic;
o_count : out std_logic_vector (3 downto 0)
);
end decade_counter_behavioral;

architecture behavioral of decade_counter_behavioral is

signal r_count : unsigned (3 downto 0) := "0000";  -- output register for counter
signal r_clk_count : unsigned (25 downto 0) := x"0000000";  -- counts clk pulses
signal r_count_en : std_logic := '0';  -- count enable

begin

    CLK_COUNT_PROC : process (i_clk)  -- counts the clk cycles
    begin
        if rising_edge (i_clk) then
            if (r_clk_count < x"3B9ACA0") then  -- 62,500,500 or 0.5 sec @ 125 MHZ system clk
                r_clk_count <= r_clk_count + 1;
            else
                r_clk_count <= x"0000000";
            end if;
        end if;
    end process;  -- CLK_COUNT_PROC
     
    COUNT_EN_PROC : process (r_clk_count)  -- keeps the count enable high for two clk cycles for ff setup/hold time
    begin
        if (r_clk_count > x"3B9AC9E") then
            r_count_en <= '1';
        else
            r_count_en <= '0';
        end if;
    end process;  -- COUNT_EN_PROC
   
COUNT_PROC : process (i_clk, r_count_en)  -- only counts up when r_count_en is high (every 0.5 sec)
begin
if rising_edge (i_clk) then
if (r_count < "1001" and r_count_en = '1') then
r_count <= r_count + 1;
else if (r_count = "1001" and r_count_en = '1') then
r_count <= "0000";
end if;
end if;
end process;  -- COUNT_PROC

o_count <= std_logic_vector(r_count);  -- output assignments

end behavioral;

[hamster_nz]

`else if' opens an other code block/level of indentation.

'elsif' is what you want.

[NorthGuy]

-- keeps the count enable high for two clk cycles for ff setup/hold time

This is not related to setup/hold time, but will make r_count increment twice in a row.

[Bassman59]

I'm going to make some general observations on coding style here. The code as originally written is a mish-mash of various styles and is .. meh. So indulge me. Most of my comments are about easily seeing what an "end" closes.

Code: [Select]

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity decade_counter_behavioral is
port (
i_clk   : in std_logic;
o_count : out std_logic_vector (3 downto 0)
);
end decade_counter_behavioral;

For completeness' sake, VHDL allows the closing "end" of a thing to include both its label and the type of thing, so let's make this entity declaration complete:

Code: [Select]

library ieee;
entity decade_counter_behavioral is
port (
i_clk   : in std_logic;
o_count : out std_logic_vector (3 downto 0)
);
end entity decade_counter_behavioral;
   
Here is the architecture ...

Code: [Select]

architecture behavioral of decade_counter_behavioral is

signal r_count : unsigned (3 downto 0) := "0000";  -- output register for counter
signal r_clk_count : unsigned (25 downto 0) := x"0000000";  -- counts clk pulses
signal r_count_en : std_logic := '0';  -- count enable

I prefer to use the (others => '0') idiom here. Why? Because when you change the counter width from four bits to eight bits you don't need to change the initializer. This works for std_logic_vector as well as its subtypes unsigned and signed.
   

Code: [Select]

architecture behavioral of decade_counter_behavioral is

signal r_count : unsigned (3 downto 0) := (others => '0');  -- output register for counter
signal r_clk_count : unsigned (25 downto 0) :=(others => '0');  -- counts clk pulses
signal r_count_en : std_logic := '0';  -- count enable

Now the main block of the entity.

Code: [Select]

begin

    CLK_COUNT_PROC : process (i_clk)  -- counts the clk cycles
    begin
        if rising_edge (i_clk) then
            if (r_clk_count < x"3B9ACA0") then  -- 62,500,500 or 0.5 sec @ 125 MHZ system clk
                r_clk_count <= r_clk_count + 1;
            else
                r_clk_count <= x"0000000";
            end if;
        end if;
    end process;  -- CLK_COUNT_PROC

First, I'd declare a constant for the terminal count and put it in the declarative part of the architecture (between the keyword architecture and the first begin).  Of you could make it a generic on your entity!

Second, the decorations. VHDL allows a process declaration to be:

Code: [Select]

    label : process (i_clk) isNote the word "is." Using it here makes it consistent with the entity and architecture declarations.

Third, there's the line:

Code: [Select]

    end process; -- CLK_COUNT_PROC
VHDL allows you to use a label as part of an "end" clause -- and you've already written the label in the comment! So fix it:

Code: [Select]

   end process CLK_COUNT_PROC;   

Code: [Select]

    COUNT_EN_PROC : process (r_clk_count)  -- keeps the count enable high for two clk cycles for ff setup/hold time
    begin
        if (r_clk_count > x"3B9AC9E") then
            r_count_en <= '1';
        else
            r_count_en <= '0';
        end if;
    end process;  -- COUNT_EN_PROC

In addition to my notes about the decorations and the use of constants for terminal counts, you do realize that this is a combinatorial process, right? r_count_en is not a register. Is that what you want? 

If that's what you want, you can also write this as a continuous assignment:

Code: [Select]

r_count_en <= '1' when r_clk_count > MY_TERMINAL_COUNT else '0';
and even simpler in VHDL 2008 (someone please check this!)

Code: [Select]

r_count_en <= r_clk_count > MY_TERMINAL_COUNT;

Code: [Select]

    COUNT_PROC : process (i_clk, r_count_en)  -- only counts up when r_count_en is high (every 0.5 sec)
begin
if rising_edge (i_clk) then
if (r_count < "1001" and r_count_en = '1') then
r_count <= r_count + 1;
else if (r_count = "1001" and r_count_en = '1') then
r_count <= "0000";
end if;
end if;
end process;  -- COUNT_PROC

This is a synchronous process, sensitive only to the clock edge. r_count_en doesn't have to be on the sensitivity list, and in a code review this will get flagged.
   

Code: [Select]

o_count <= std_logic_vector(r_count);  -- output assignments
Declare your output count port to be unsigned and you won't need to do the typecast.

Code: [Select]

end behavioral;
For completeness' sake this should be:

Code: [Select]

end architecture behavioral;

[NorthGuy]

The code as originally written is a mish-mash of various styles and is .. meh.

Styles may be important for some people. I prefer simplicity.

What I don't like in this code is the behaviour when the counters overflow. In reality, it doesn't matter what they do in the overflown state because they never overflow. So, there's no need to create a special case for this situation. It's better to let the counter do something which it already does in other situations. This will lead to smaller logic. This also let us get rid of the complex ordered comparison in favour of much simple equality. Like this:

Code: [Select]

  process (i_clk)  -- counts the clk cycles
  begin
    if rising_edge (i_clk) then
      r_clk_count <= r_clk_count + 1;     
      if r_clk_count = x"3B9ACA0" then  -- 62,500,500 or 0.5 sec @ 125 MHZ system clk
        r_clk_count <= x"0000000";
        r_count <= r_count + 1;
        if r_count = "1001" then
          r_count <= "0000";
        end if;
      end if;
    end if;
  end process;
Disclamer: Justed typed it in as an illustration. May contain bugs.