Strange VHDL behavior on hardware unlike simulation

Question

I have a simple VHDL code which has two process. The second process updates the output port m_LED based on the state CF value. In simulation I see the behavior as expected. But when I program the FPGA I noticed that, the output port m_LED is producing some random values which is not even assigned in the code. I am totally clueless from where these values are coming. Any hint will be much appreciated!

entity monitor is
Port ( 
           io_LED: in std_logic_vector(3 downto 0); 
           m_LED: out std_logic_vector(3 downto 0) 
     );
end monitor;
 
architecture Behavioral of monitor is
    type state_type is (s0, s1, s2,s3);
    signal state: state_type :=s0;
    signal nrst:std_logic:='1';
begin

Process (io_LED)
    begin 
        if (nrst= '0') then    
            state<= s0;  
        else
                case state is                       
                   when s1 =>          
                        if (io_LED = "0000") then
                            state <= s2;
                        end if;                                                                        
                    when others => state <= s0; 
                                         
                end case;            
            end if;    
    end Process;
    
    Process(io_LED)
    begin
        m_LED<="0000";
        case state is 
            when s0 =>
                m_LED<="0000";
            when s1 =>
                m_LED<="0001";
            when others => m_LED<="0000";            
        end case; 
    end Process;

end Behavioral;

One thing I can see is that you are missing an "else" in the first if of the first process. You could infer a latch. Does it change behavior when you fix it? — Fra93, Jul 05 '22 at 05:59
Another good hint would be if you could paste here the warnings given by the synthesizer — Fra93, Jul 05 '22 at 06:00
Last suggestion is that since your code is completely combinational, there is no need for initializing your signals, as they can't hold a value. You should not rely on any wire holding a value because it might work in simulation but it's impossible to have in the real circuit. — Fra93, Jul 05 '22 at 06:03
i think its better to use Process(monit_LED,nrst) instead Process(io_LED,nrst) in second process. — AlexB, Jul 05 '22 at 07:30
@Fra93: If I add else in the first if of the first process it's throwing an error saying - [DRC LUTLP-1] Combinatorial Loop Alert: 1 LUT cells form a combinatorial loop. In the else loop I assigned CF <=i; — karthik, Jul 05 '22 at 09:29
Can you draw what you are actually doing? There is combinational loop which will never work as expected in hardware. — Fra93, Jul 05 '22 at 09:34
This is a combinational loop. You probably want a clocked process. The sim/synth mismatch comes from the mistake in the sensitivity list. — , Jul 05 '22 at 11:15

Fra93 · Answer 1 · 2022-07-05T11:31:22.290

2

I believe that what you are doing is a very long combinational path for a state machine. State machines are better implemented using a register to actually keep the state.

The downside is that you will need to provide a clock for the state register to sample the input and launch the output.

Changes:

CF is made as a register CF_r, to hold the state.\
First process is a sequential process, with the CF_r register holding the code { i, t, f,e } as defined by the input.
CF is the only element in the second process sensitivity list as it is a combinational output based only on the state.


-- Synchronous reset
    entity monitor is
    Port ( 
               io_LED: in std_logic_vector(3 downto 0); 
               monit_LED: out std_logic_vector(3 downto 0) 
         );
    end monitor;
     
    architecture Behavioral of monitor is
        type Code is (i, t, f,e);
        signal CF_r: Code:=i;
        signal nrst:std_logic:='1';
    begin

    -- Synchronous reset
    Process (clk)
        begin 

            if rising_edge(clk) then
                if (nrst= '0') then    --should be adapted to postive logic
                    CF_r <= t;  --reset
                else
                    case CF_r is                       
                       when i =>          
                            if (io_LED = "0000") then
                                CF_r <= t;
                            end if;
                        
                       when t =>
                            if (io_LED = "0001") then
                                CF_r <= f;
                            else 
                                CF_r <=e;
                            end if;
                            
                        when f =>
                            if (io_LED = "0010") then
                                CF_r <= t;   
                            else 
                                CF_r <=e;
                            end if;
                            
                        when others => CF_r <= i; 
                    end case;
                end if;            
            end if;    
        end Process;
        
        -- The output depends only on the state
        Process(CF_r)
        begin
            case CF_r is 
                when i =>
                    monit_LED<="0000";
                when t =>
                    monit_LED<="0001";
                when f =>
                    monit_LED<="0010";
                when e =>
                    monit_LED<="1111";
                when others => monit_LED<="0000";            
            end case; 
        end Process;

    end Behavioral;

edited Jul 05 '22 at 11:31

answered Jul 05 '22 at 11:14

Fra93

1,992
1
9
18

Downvote will be removed when you correct the clocked process. – Jul 05 '22 at 11:16
@user_1818839 ups I added the `rising edge` for the clock. Thanks for stating it was wrong, however it would be nice if you pointed out exactly what was uncorrect :) – Fra93 Jul 05 '22 at 11:19
Still not correct : what happens when you assert nrst? – Jul 05 '22 at 11:20
It's a negative reset, when you assert it, the process is not in reset and the clocked process runs. – Fra93 Jul 05 '22 at 11:22
It's a negative reset and "assert" means "set to its active level". Do that, and ... nothing happens, in simulation. Synth is another story. – Jul 05 '22 at 11:24
Ups sorry, fixed the order of the `rising edge` and `if` – Fra93 Jul 05 '22 at 11:31
Note that isn't the only possible fix, but it'll do. – Jul 05 '22 at 11:36
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/246170/discussion-between-fra93-and-user-1818839). – Fra93 Jul 05 '22 at 11:56
@Fra93 the behavior changes when process has clock in the sensitivity list. Here state 'e' stands for errored state. With every clock cycle if the process is triggered, it ends up in e state. For eg- at one clock cycle, let's say CF_r=t and io_LED ="0001" (second case in first process) then the first process updates CF_r to 'f'. In the next clock cycle, io_LED is still "0001" as CF_r is f now, it goes to else statement and updates CF_r=e. This does not happen in the code that I have shared. – karthik Jul 05 '22 at 12:11
@karthik I see two big problems in your student's code: the first process is a big combinatorial loop, which changes behaviour based on routing and acutal delays and it could even never get stable. The second is that the second process should have CF in the sensitivity list, since the output changes when CF changes. The two problems combined can give unexpected results outside of simulation. – Fra93 Jul 05 '22 at 12:31
@karthik it only "works" in simulation because the mistake in the first process sensitivity list hides the combinatorial loop. – Jul 05 '22 at 21:28
Plus: `clk` is not declared. Hard-wiring `nrst` to `'1'` makes it useless and prevents proper initialization of your state machine. Better declare it as an input port. Using `std_logic` and `std_logic_vector` (resolved types) should be reserved to multiple drive situations (tri-state buffers...); using them instead of their `std_ulogic` and `std_ulogic_vector` parent types just hides useful error messages when accidentally creating short-circuits. – Renaud Pacalet Jul 06 '22 at 11:26
Note that the `when others` clauses don't hurt but they are useless as you already enumerated all possible choices. Note also that the initialization at declaration of `CF_r` (`i`) and in the reset branch (`t`) look a bit contradictory. – Renaud Pacalet Jul 06 '22 at 11:39
@Fra93 Thank you for pointing out that the problem is the combinational loop in the first process. I added clock as you did and added a condition to detect change in io_LED and then enter the case statement. This works as expected! But in general, Vivado catches combinational loops during implementation. Not sure why it didn't in this case. – karthik Jul 06 '22 at 12:52
Thanks @user_1818839 for highlighting the issue with simulation. – karthik Jul 06 '22 at 12:52

Strange VHDL behavior on hardware unlike simulation

1 Answers1