1
\$\begingroup\$

I am trying to write a little counter in VHDL using the two process methodology. However it is not working. Could someone explain me why?

   library IEEE;
   use IEEE.std_logic_1164.all;
   use IEEE.std_logic_arith.all;
   USE ieee.numeric_std.ALL;
   use IEEE.std_logic_unsigned.all;


   entity myCounter is
   port(
    clk: in std_logic;
    clkEnable:in std_logic;
    reset:in std_logic;
   );
   end myCounter;


   architecture Behavioral of myCounteris

   TYPE STATE_TYPE IS (counterDecr, countIncr, resetCounter);

   signal stateMachine: STATE_TYPE:=counterDecr;
   signal nextState: STATE_TYPE:=counterDecr;

   signal counter: integer:=15;

   signal test: std_logic:='0';

   begin

synchronous: process(clk)begin
        if(clkEnable='1')then
            if(rising_edge(clk))then
                if(reset='1')then
                    stateMachine<=counterDecr;                      
                else 
                    stateMachine<=nextState;
                end if;

            end if;
        end if; 
end process;

combin:process(stateMachine)begin
            counter<=counter-1;
            nextState<=counterDecr;
            test<= not test;
end process;
   end;
\$\endgroup\$
  • \$\begingroup\$ Note that this question was also cross-posted to reddit. \$\endgroup\$ – Dave Tweed Apr 23 '16 at 11:33
  • \$\begingroup\$ Welcome to Electrical Engineering! Do not vandalise or edit important information out of your post. This question is supposed to remain for other users to benefit from, for posterity. You are not permitted to significantly worsen or vandalize your own posts on Electrical Engineering, as you have irrevocably licensed the post and its content to StackExchange under the CC BY SA license upon posting. \$\endgroup\$ – Ferrybig Apr 24 '16 at 13:00
  • 1
    \$\begingroup\$ @Ferrybig Scaring people with fake legalese is bullshit. The original content is still there. StackExchange has chosen to display the edits at a certain URI. The original edits are available at another. \$\endgroup\$ – pipe Apr 24 '16 at 13:03
1
\$\begingroup\$

The main problem is that StateMachine always has the value of counterDecr. Since it never changes, the combin process is never triggered.

The combin process should have all of the signals on the RHS (right-hand side) of any of its assignments — and no others — in its sensitivity list. It doesn't make sense to have it triggered by some other signal.

But a bigger problem is that a combinatorial process cannot have assignments in which signals on the LHS also appear on the RHS. Statements like counter <= counter - 1; and test <= not test; don't make any sense — they would never settle on a final value.


EDIT: I can't tell what your intentions are from your code, but if I needed an up/down counter, I would write something like this:

architecture Behavioral of myCounter is
  signal counter: std_logic_vector (3 downto 0);
begin
  process (clk) begin
    if rising_edge(clk) then
      if reset = '1' then
        counter <= (others => '0');
      elsif clkEnable = '1' then
        if updown = '1' then
          counter <= counter + 1;
        else
          counter <= counter - 1;
        end if;
      end if;
    end if;
  end process;
end Behavioral;

Note that the only "state" is the counter itself. If I wanted to have a state machine that controlled the counting mode of the counter, that would be a separate module.

I don't subscribe to the "two process" method for state machines; it's merely more verbose and prone to error, as you have found.


EDIT: In response to your latest comment, I would do a divide-by-ten something like this:

architecture Behavioral of myCounter is
  signal counter: std_logic_vector (3 downto 0);
begin
  process (clk) begin
    if rising_edge(clk) then
      if reset = '1' then
        counter <= (others => '0');
        output_pulse <= '0';
      elsif clkEnable = '1' then
        if counter = "1001" then
          counter <= (others => '0');
          output_pulse <= '1';
        else
          counter <= counter + 1;
          output_pulse <= '0';
        end if;
      end if;
    end if;
  end process;
end Behavioral;
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ In that case what would you suggest? I want a statemachine where in that specific state it decrements a vue. Thx for the other info! \$\endgroup\$ – J.Doe Apr 20 '16 at 22:18
  • 1
    \$\begingroup\$ I don't know what to suggest, because I can't tell what you really want to do from your code. For example, what would cause stateMachine to switch to a different state? \$\endgroup\$ – Dave Tweed Apr 20 '16 at 22:23
0
\$\begingroup\$

Your code has several bugs and problems. See my annotations inline.

library IEEE;
use IEEE.std_logic_1164.all;
-- use IEEE.std_logic_arith.all;     -- don't use this
use ieee.numeric_std.all;            -- use this package, it comes with new types for signed and unsigned operations
-- use IEEE.std_logic_unsigned.all;  -- don't use this either

entity myCounter is                  -- there was a space missing
  port (
    clk       : in std_logic;
    clkEnable : in std_logic;
    reset     : in std_logic   --;   -- ";"-signs are no line end signs like in C, they are list element delimiters, so no ";" on the port
                                     -- btw. entities with no output signal are considered empty or useless design units and get trimmed in many tools
  );
end entity;

architecture Behavioral of myCounter is -- there was a space missing
  type STATE_TYPE is (counterDecr, countIncr, resetCounter);

  signal stateMachine : STATE_TYPE := counterDecr;  -- space characters around operators do no harm, but increase readability
  signal nextState    : STATE_TYPE;        -- don't initialize combinatoric signals

  signal counter      : integer   := 15;   -- this is a 32-bit integer, you could constrain the range to 0..15

  signal test         : std_logic := '0';  -- this signal maps to a register, so an init value is 'correct'

begin
  synchronous: process(clk)
  begin
    --if(clkEnable='1')then         -- clock enables must be placed 'inside' of a rising_edge, see the HDL coding guide of you vendor tool, what patterns are supported
    if rising_edge(clk) then
      if (reset = '1') then
        stateMachine <= counterDecr;
      elsif (clkEnable = '1') then  -- note that ce (clock enable) signals usually have a lower priority than resets
        stateMachine <= nextState;
      end if;
    end if; 
  end process;

  combin: process(stateMachine)     -- your FSM has less events then your counter should have, so this sensitivity list will not work as expected
  begin
    counter   <= counter - 1;       -- this generates a combinatoric loop, such statement makes only sense in a sequential process
    nextState <= counterDecr;
    test      <= not test;          -- this generates a combinatoric loop
  end process;
end architecture;
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You make some good points, but you completely missed the main problems with the combin process. \$\endgroup\$ – Dave Tweed Apr 20 '16 at 22:04
  • \$\begingroup\$ Yes, I over looked the constant value of stateMachine in my first answer version. I don't want to copy this aspect from you coincident answer :) \$\endgroup\$ – Paebbels Apr 20 '16 at 22:07

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.