Need advice about coding style

[Altera_Forum]

Hello I'm Alex and I am novice for VHDL coding. if my english awful pls forgive me. 

I trying to understand VHDL PROCESS statement. I try with 1bit full adder with carry save function in Quartus2 v15. I write down code:LIBRARY IEEE; USE IEEE.std_logic_1164.ALL; -- truth-table of full-adder -- with three inputs: ci{carry_in}, a{first in} and b{second in} -- and two ouputs: s{sum} and co{carry_out} -- ci a b | s co -- ----------------- -- 0 0 0 | 0 0 -- 0 0 1 | 1 0 -- 0 1 0 | 1 0 -- 0 1 1 | 0 1 -- 1 0 0 | 1 0 -- 1 0 1 | 0 1 -- 1 1 0 | 0 1 -- 1 1 1 | 1 1 ENTITY full_add_1bit IS PORT ( ci : IN STD_LOGIC; a : IN STD_LOGIC; b : IN STD_LOGIC; s : OUT STD_LOGIC; co : OUT STD_LOGIC ); END full_add_1bit; ARCHITECTURE b OF full_add_1bit IS BEGIN p0 : PROCESS(ci, a, b) VARIABLE aab, aob, axb, sxc : STD_LOGIC; BEGIN aab := a AND b; axb := a XOR b; sxc := axb XOR ci; s <= sxc; IF ci = '0' THEN co <= aab; ELSIF ci = '1' THEN aob := axb XOR aab; -- the same as (a OR b) co <= aob; END IF; END PROCESS p0; END b; but during compilation i recieves message    --- Quote Start ---  Warning (10631): VHDL Process Statement warning at full_add_1bit.vhd(30): inferring latch(es) for signal or variable "co", which holds its previous value in one or more paths through the process  --- Quote End ---    I follow the simple logic - let's compute new values then do all signal assignments and the question is question1 : does pressure on variables so need?!  I rewrite architecture to new one, let's compiler do all work: ARCHITECTURE b1 OF full_add_1bit IS BEGIN p0 : PROCESS(ci, a, b) VARIABLE result_i : INTEGER; VARIABLE result_v : STD_LOGIC_VECTOR (0 TO 1); BEGIN result_i := CONV_INTEGER(a)+CONV_INTEGER(b)+CONV_INTEGER(ci); result_v := CONV_STD_LOGIC_VECTOR (result_i, 2); co <= result_v(0); s <= result_v(1); END PROCESS p0; END b1; Finally i make schematic for this with 2mux1 blocks as for fun... in the attachments.  never use schematic for establishing any equation - you waste your time. But result is the same with that scheme. question 2: how to find the way to good coding style?

[Altera_Forum]

A slightly quirky one, I agree. However, the following code is confusing Quartus: IF ci = '0' THEN co <= aab; ELSIF ci = '1' THEN aob := axb XOR aab; -- the same as (a OR b) co <= aob; END IF;  

Using 'ELSE' covers all other cases. By using 'ELSIF' Quartus believes there may be some other paths it must handle - even though there aren't in this case.  Change these lines to: IF ci = '0' THEN co <= aab; ELSE aob := axb XOR aab; -- the same as (a OR b) co <= aob; END IF;  I recommend you always use an ELSE at the end of conditional statements that infer combinational logic.  Cheers, Alex

[Altera_Forum]

thanks for advice.

[Altera_Forum]

An another comment on coding style - the conv_std_logic_vector and conv_integer functions come from the non-standard std_logic_unsigned and std_logic_arith packages. You shouldnt use them, as they are non-standard. You should use the numeric_std package instead. (and probably use appropriate types to avoid type conversion as much as possible - you can use non-std_logic_vector types in ports - like unsigned or signed type.).

[Altera_Forum]

thanks for advice

[Altera_Forum]

Ok. I know how to use UNSIGNED ports. I make an example: 

LIBRARY IEEE; USE IEEE.std_logic_1164.ALL; USE IEEE.numeric_std.ALL; ENTITY full_adder_Nbit IS GENERIC ( N : POSITIVE := 4 ); PORT ( ci : IN STD_LOGIC := '0'; a : IN UNSIGNED (N-1 DOWNTO 0) := X"0"; b : IN UNSIGNED (N-1 DOWNTO 0) := X"0"; s : OUT UNSIGNED (N-1 DOWNTO 0) := X"0"; co : OUT STD_LOGIC := '0' ); END full_adder_Nbit; ARCHITECTURE b OF full_adder_Nbit IS BEGIN p0 : PROCESS(ci, a, b) VARIABLE result_v : UNSIGNED (0 TO N+1); BEGIN result_v := ('0' & a & '1') + ('0' & b & ci); co <= result_v(0); s <= result_v(1 TO N); END PROCESS p0; END b; I have question1 : When i write initial value for UNSIGNED like ... := "0" without prepand symbol 'X' ModelSim unable to start simulation (Error loading design and point to line number for port a). Which symbols allowed at the beggining? I think the code above can synthesis any combinatoral adder. I want some that work with clocking. question 2: why i reinvent adders, is there any class of ready function, where i can read about? question 3: If i instatiate my entity I am not sure about the length of result_v variable which n will be substituted. Thx in advance.

[Altera_Forum]

For your questions. 

1. What you have specified is a default value if nothing is connected to it. But the value have assigned is only 4 bits long, so if N is anything other than 4, there will be an error. 2. With unsigned type, you have all the arithmetic functions, as you've already done.   r <= a + b;   But in your code, you have extended a by 1 bit correct, but then you also append '1' to the LSB. You need to write:  result_v := ('0' & a) + ('0' & b) + ('0' & ci); -- if you get error about ambiguous types, write unsigned'('0' & ci);  You also have an error in your code. result should be created as   variable result_v : unsigned(N downto 0);  and s should be sliced   s <= result_v(N-1 downto 0); co <= result_v(N);  3. when you fix the direction errors, result_v will have a length of N+1.

[Altera_Forum]

I've changed as you wish. But I am confused what's wrong with my previous. I agree that no extra bit requried in variable , but the reverse order - i'm confused. I can only suggest that LSB is always at index 0 but I am not sure about it. If your advice is coming from code can be read clearly, i accept it. 

ARCHITECTURE b OF full_adder_Nbit IS BEGIN p0 : PROCESS(ci, a, b) VARIABLE result_v : UNSIGNED (N DOWNTO 0); BEGIN result_v := ('0' & a ) + ('0' & b) + ('0' & ci); co <= result_v(N); s <= result_v(N-1 DOWNTO 0); END PROCESS p0; END b;

[Altera_Forum]

Ok. Another one example. If I make my adder synchronus. Will be my code suitable for real application? 

LIBRARY IEEE; USE IEEE.std_logic_1164.ALL; USE IEEE.numeric_std.ALL; ENTITY full_adder_Nbit IS GENERIC ( N : POSITIVE := 4 ); PORT ( clk : IN STD_LOGIC := '0'; ci : IN STD_LOGIC := '0'; a : IN UNSIGNED (N-1 DOWNTO 0) := X"0"; b : IN UNSIGNED (N-1 DOWNTO 0) := X"0"; s : OUT UNSIGNED (N-1 DOWNTO 0) := X"0"; co : OUT STD_LOGIC := '0' ); END full_adder_Nbit; ARCHITECTURE b OF full_adder_Nbit IS SIGNAL reg_a, reg_b, out_s : UNSIGNED (N-1 DOWNTO 0); SIGNAL reg_ci, out_co : STD_LOGIC; BEGIN p0 : PROCESS (clk) BEGIN IF rising_edge(clk) THEN reg_a <= a; reg_b <= b; reg_ci <= ci; END IF; END PROCESS p0; p1 : PROCESS(reg_ci, reg_a, reg_b) VARIABLE result_v : UNSIGNED (N DOWNTO 0); BEGIN result_v := ('0' & reg_a) + ('0' & reg_b) + ('0' & reg_ci); out_co <= result_v(N); out_s <= result_v(N-1 DOWNTO 0); END PROCESS p1; p2 : PROCESS (clk) BEGIN IF falling_edge(clk) THEN co <= out_co; s <= out_s; END IF; ENd PROCESS p2; END b; I am very unsure does it really need to store output value in register and at the falling edge. I can drive it directly but what i get?! I'm also unsure about storing input values in registry/ Perhaps I need only do addtion when clock arrive. So as result I can write down FOUR different solutions: 1) no register at input and output, do result when clock transition from 0 to 1 arrive. 2) register at inputs and no register at output. 3) no register at input but at the output. 4) inputs and outputs have register. (as shown in code above). question 1. What will be the best solution, how it depends on application? question 2. Do I make transition to synchronus right?

[Altera_Forum]

in principle the addition (combinatorial only) will do the job. 

registering inputs/outputs is a measure to achieve synchronous design and best fmax. The tradition is to register outputs only (not inputs) per module since then inputs received are already registered. Excluding very first paths at io which need to be registered

[Altera_Forum]

thank you, Kaz. 

I think external inputs should lead to some state machine which do control for entire device or just for local module. if so i should develope a pulse generator with programmed delay and number of output cycle to provide internal clocks. But what type of state-machine to choose Melay or Moore?

[Altera_Forum]

I was going for a better answer but it took me 5 times to login successfully. This website is broken and ignored. 

 Your statement can't be generalised. but one thing sure can; do not generate clock from logic, never so as a beginner.  state machine is just a methodology of design, old but could be useful to clear a disturbed mind. It is not essential and is just based on renaming of state on some registers outputs (state coding). I very rarely used state machine and when I do I use single process approach and don't want to know what is mealy or Moore who incidentally I believe were two PhD students having a drink at a pub then came with the idea. The difference between them is one clock delay between I think outputs of state machine and state transition.   In short, what you want to do at algorithm level decides what you should do at implementation level.

[Altera_Forum]

Ok. I conclude - just describe your intetion as algorithm then compiler will decide about implemantion. By the way, looking at code for full_adder_nbit is there upper limit for N when the logic sysnthesis with plus operator will be worst than using build-in PARALLEL_ADD structure , the last i found in IP-catalog. It is also about coding style - using built-in functions and blocks rather than reinventing one's own.

[Altera_Forum]

Let's go further. 

I see that built-in function has pipelining, I want to use it in my code, Im' confused a little bit. Any idea are welcome. I wrote comments, is it good style for VHDL comments? What should be commented? The comments should be present in code for person who will be read it. I remove initial values. I believe it is useful in simulation only, correct me if it is falsehood. LIBRARY IEEE; USE IEEE.std_logic_1164.ALL; USE IEEE.numeric_std.ALL; -- addition of two operands with carry-save ENTITY full_adder_Nbit IS GENERIC ( N : POSITIVE := 4 -- default operands size ); PORT ( clk : IN STD_LOGIC; -- clock signal ci : IN STD_LOGIC; -- carry-in a : IN UNSIGNED (N-1 DOWNTO 0); -- augend b : IN UNSIGNED (N-1 DOWNTO 0); -- addend s : OUT UNSIGNED (N-1 DOWNTO 0); -- sum co : OUT STD_LOGIC -- carry-out ); END full_adder_Nbit; ARCHITECTURE b OF full_adder_Nbit IS SIGNAL out_s : UNSIGNED (N-1 DOWNTO 0); -- new value for sum SIGNAL out_co : STD_LOGIC; -- new value for carry-out BEGIN -- provide outputs with recently calculated values (from p1) p0 : PROCESS (clk) BEGIN IF rising_edge(clk) THEN co <= out_co; s <= out_s; END IF; END PROCESS p0; -- calculate new values that will be outputed (for p0) p1 : PROCESS (ci, a, b) VARIABLE result_v : UNSIGNED (N DOWNTO 0); -- intermediate result BEGIN result_v := ('0' & a) + ('0' & b) + ('0' & ci); out_co <= result_v(N); out_s <= result_v(N-1 DOWNTO 0); END PROCESS p1; END b;

[Altera_Forum]

Hi. Could anyone clarify me why i get multiplexor instead of register? 

function regCreate( signal clk : std_logic; aclr : std_logic; data : unsigned) return unsigned is variable temp : unsigned(data'range); begin if aclr = '1' then temp := to_unsigned(0, data'length); elsif rising_edge(clk) then temp := data; end if; return temp; end regCreate;

[Altera_Forum]

--- Quote Start ---  Hi. Could anyone clarify me why i get multiplexor instead of register?  --- Quote End ---    Because you tried to use the clock inside a function. Basically, dont do that.

[Altera_Forum]

Will it be the same if I will use "procedure". Or should i shift down to component instance? 

Before I believe that semantics of sequential operators the same as in "process". Perhaps i misunderstand "using function to generate schematic". I belive that fucntion do computation and generate some combinatorial schematic, why it can not manage with synchronous?  function isRegZero(reg : unsigned) return std_logic is variable temp : std_logic; begin temp := '1'; for i in reg'range loop temp := temp and (not reg(i)); end loop; return temp; end isRegZero; with this function we get simple and-gate in schematic rather than big comparator

[Altera_Forum]

Why not stick to the coding guidelines and use processes?

[Altera_Forum]

Functions are not used for synchronous description because they have to be called from a sequential context (like a process). Functions also complete in 0 time and cannot have any time associated with them. 

Procedures can have time within them, but I highly recommend you stick with processes.

[Altera_Forum]

It seems that rising_edge evaluated at compile time in function. 

so i chage to procedure  procedure regCreate( signal clk : IN std_logic; aclr : IN std_logic; data : IN unsigned; signal reg: OUT unsigned) is begin if aclr = '1' then reg <= to_unsigned(0, data'length); elsif rising_edge(clk) then reg <= data; end if; end regCreate; procedure regCreate( signal clk : IN std_logic; aclr, ce : IN std_logic; data : IN unsigned; signal reg: OUT unsigned) is begin if aclr = '1' then reg <= to_unsigned(0, data'length); elsif rising_edge(clk) then if ce = '1' then reg <= data; end if; end if; end regCreate; It works. test code library IEEE; use IEEE.std_logic_1164.all; use IEEE.numeric_std.all; use work.utility_pkg.all; entity PulseGenerator is generic( REG_WIDTH : natural := 4 ); port( reset clk, wrn, wrk, start : IN std_logic; data : IN unsigned(REG_WIDTH-1 downto 0); z : OUT std_logic ); end PulseGenerator; architecture rtl1 of PulseGenerator is signal reg_N, reg_K : unsigned(data'range); begin regCreate(wrn, reset, data, reg_N); regCreate(clk, reset, wrk, data, reg_K); z <= isRegZero(reg_N) and isRegZero(reg_K); end rtl1;

[Altera_Forum]

thank you a lot of, Tricky. Your advices help me. 

I understand why you insist on using "process". using procedure and function in such manner like i do, can be compared with old-macrofunction. We should stay in the middle.