Continue to Site

Welcome to EDAboard.com

Welcome to our site! EDAboard.com is an international Electronics Discussion Forum focused on EDA software, circuits, schematics, books, theory, papers, asic, pld, 8051, DSP, Network, RF, Analog Design, PCB, Service Manuals... and a whole lot more! To participate you need to register. Registration is free. Click here to register now.

Register Log in

VHDL Possible timing problem?

Status
Not open for further replies.
S

sh-eda

Member level 1
Joined
Oct 1, 2010
Messages
40
Helped
1
Reputation
2
Reaction score
1
Trophy points
1,288
Activity points
1,735
Hi,
I'm relatively new to VHDL and I wonder if someone could have a look through this bit of vhdl code. The FPGA is a A42MX09, the IDE is libero.
I have simulated the code, before and after synthesis and it appears fine. Synthesis does not show any errors.
Basically it's a serial data stream, that sends 28bits of data. I was adding a simple 8bit sum of bytes checksum calculation as the last byte.
I have programmed it into an FPGA but it did not work. From checking the oscilloscope the only data that is being sent is the '0101' that I've added to make a byte up.
It appears to be intermittent, sometimes it does work, so I'm guess there's a race condition or timing issue happening.
I'm wondering if it that I am assigning 'tx_reg' in two places, but I'm not sure.
I've attached the full code.
Thanks

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

begin
    if reset = '1'then
        tx_out <= '0';
        counter := 0;
        tx_reg := (others => '0');
        elsif clk'event and clk= '1' then
            counter := counter + 1;
            if counter = 1 then 
            tx_synch <= '1';
            tx_reg := tx_data_bus ;  ----  <<<<<<<<<<< 
 
-- Checksum calcs-----------------------------------
-- Calculation is the sum of bytes. 
-- Added "0101" onto the end of last 4 bits to make a byte.
-- Checksum is the three bytes added together.
-- This is now the last 8bits of data sent
            high_byte (3 downto 0) := tx_reg(19 downto 16) ;
            high_byte (7 downto 4) := Extradata ;
            checksum:= std_logic_vector(unsigned(high_byte (7 downto 0))+ unsigned(tx_reg(15 downto 8)) + unsigned(tx_reg(7 downto 0)) ) ;
            tx_reg(27 downto 20) := checksum ; ----  <<<<<<<<<<< Is this the problem?
----------------------------------------------------
 
            tx_out <= '1' ;        
            end if;
            
            if counter >= 2 then 
                if counter <= tx_length-1 then 
                tx_out <= '0';        
                tx_synch <= '0';
                tx_out <= tx_reg (counter-2) ;
                end if;
            end if;
 
            if counter = tx_length then
            counter := 0;
            tx_out <= '0';        
            end if;
        end if;
end process;
 

Attachments

  • edaboard.txt
    2.2 KB · Views: 46
Last edited:

Although variable usage for tx_reg and counter reduces the achievable design speed (creates large combinational pathes), the design looks functional.

How did you stimulate tx_data_bus in your hardware test?
 

Hi

This block is part of a slight larger FPGA circuit, though the rest of the circuit is very simple.
It just takes in lots of inputs (around 48) from the FPGA pins, deglitches them, then ORs them together to create tx_data_bus.
In term of simulation, I use Modelsim. I created some stimulus files, one to test this block and another that tests the whole chip.
In term of hardware, Nothing very complicated, the FPGA is fitted in a PCB (in a socket) with some pulls-resistors and switches to toggle the inputs. I've tested that the inputs are being pulled up to 5v correctly. .
This all worked before I made the FPGA Checksum changes.
What would I have used instead of variable? Signal?
 

O.K., just can say, if you don't see the expected output, look for the upper hierarchy.

What would I have used instead of variable? Signal?
Click to expand...
Basically yes. But you need to consider the different behavior of signals when rewriting the code.
 

I have similar issues with this code as I did with the code on this thread, which I rewrote as this.

In summary, layering if statements and distributing the generation of a signal across multiple if statements to me takes more work to understand and to ensure there are no subtle problems in the code. My preference is to isolate each signal to it's own if statement and generate everything with a structure that is very template like or exactly like the standard synthesis templates.

I'm sure there are other experienced coders on the forum that think differently and like to have elegant looking code. To me brute force and simple is my motto and it reduces the ambiguity of the code as it's clear what was intended (without having to think too much)
 

@sh-eda:
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

begin
    if reset = '1'then
        tx_out <= '0';
        counter := 0;
        tx_reg := (others => '0');
        -- (1) tx_synch is not reset
    elsif clk'event and clk= '1' then
        counter := counter + 1; -- (2) counter is preincremented
        if counter = 1 then 
            tx_synch <= '1';
            tx_reg := tx_data_bus; -- (3) blocking assignment.  tx_reg is now tx_data_bus
 
            high_byte (3 downto 0) := tx_reg(19 downto 16); -- (3)why use tx_reg vs tx_data_bus?
            high_byte (7 downto 4) := Extradata ;
 
 
            checksum:= std_logic_vector(unsigned(high_byte (7 downto 0))+ unsigned(tx_reg(15 downto 8)) + unsigned(tx_reg(7 downto 0)) ) ;
            tx_reg(27 downto 20) := checksum ;
 
            tx_out <= '1' ;        
        end if;
            
        if counter >= 2 then 
            if counter <= tx_length-1 then 
                tx_out <= '0'; -- (4) This does nothing
                tx_synch <= '0';
                tx_out <= tx_reg (counter-2) ; --(5) shift register seems more natural.
            end if;
        end if;
        
        if counter = tx_length then
            counter := 0;
            tx_out <= '0';
            -- (6) tx_synch should be '0'
        end if;
end if;
end process;
[/QUOTE]

1.) The lack of an async reset on tx_synch means "reset" will be used as a clock enable. This is a minor issue here.
2.) This assumes the synthesis tool will do the correct optimization to avoid adding the adder delay to the output mux, if that is the critical path.
3.) Not sure why you did this. It is ok, but appears like you are re-using a variable for both combinatorial and sequential logic when you aren't.
4.) extra line of code.
5.) A shift register would be a bit nicer as it removes the mux.
6.) The synthesis tool may add logic to the clock enable for tx_synch to handle this case.

These are probably minor. #2 and #6 would be helpful. the checksum can also be pipelined if desired -- you have 28 cycles before it is needed...


@ads_ee:
I'm mixed on doing many assignments in a long if-else vs having an if-else for each group of signals with the same control set. It does make it easy to see the logic for each signal, which is nice. However, it seems more difficult to read from a non-writer's view. Likewise there are possible maintainability issues (can be mitigated).

For the readability, you effectively have a set of circuits and must figure out how the solution works. This seems like a harder problem than having a description of how a solution works and needing to find the logic for each circuit.

For maintainability, it might not be clear when signals are related when adding to the control sets. For example, if a ram read and a ram enable were in different if-else's it might be easier to add additional control logic to one, but not the other.

For readability, likewise, when looking at code that has been affected in this manner it seems difficult to easily determine that it is a logic mistake vs intentional.

For maintainability, this seems more prone to duplicating the logic for the conditions. This can lead to issues similar to the above.

It probably makes sense to start a new thread about this specific coding style choice, as it has come up on multiple threads. It is a topic that I'm open to hear more about, or to better explain my points as I'm not sure I expressed them well. Specifically if you have methods to mitigate the potential issues I've mentioned.
 

Status
Not open for further replies.

Similar threads

Part and Inventory Search

Welcome to EDABoard.com

Sponsor

Back
Top