Adding Eva C code review comments

45c22dbc · Denia Bouhired-Ferrag · a06d4d47 · 45c22dbc
Commit 45c22dbc authored Jan 26, 2017 by Denia Bouhired-Ferrag
Hide whitespace changes
Inline Side-by-side

Showing with 116 additions and 0 deletions

EvaC_comments.txt doc/code-review/v4-27-01-2017/EvaC_comments.txt +116 -0

No files found.
--- a/doc/code-review/v4-27-01-2017/EvaC_comments.txt
+++ b/doc/code-review/v4-27-01-2017/EvaC_comments.txt
+1) Process "p_pulse_redge" starting at line 138. 
+
+- In that case, I belive 'en_i' should be in the sensitivity list. (There is a warning during synthesis).
+- What happens if en_i='0' and burst_ctrl_rst = '0'? 
+Is 'en_i' synchronous to something or can it change at any moment ? What would you like to happen 
+happens if 'en_i' changes in the midle of a pulse ? 
+
+To be studied if something like that would do... It depends if we wish en_i to act sync or async.
+
+-- Async en_i:
+
+    p_pulse_redge: process (pulse_burst_i)
+	begin
+		if falling_edge(pulse_burst_i) then 
+            if (burst_ctrl_rst = '1')  then
+				gate <= '0';
+			else      
+                gate <= '1'; --gate only changes during falling edges of pulse_burst_i
+			end if;
+        end if;
+	end process p_pulse_redge; 
+
+    pulse_burst_o <= en_i and gate and pulse_burst_i;  --en_i can make '0' burst_o at any time.
+
+Or.. 
+
+-- Sync en_i:
+ 
+      p_pulse_redge: process (pulse_burst_i)
+	begin
+			if falling_edge(pulse_burst_i) then 
+                if (burst_ctrl_rst = '1')  then
+					gate <= '0';
+                else       
+                    gate <= en_i; -- gate will be sync to falling edges pulse_burst_i 
+			    end if;
+            end if;
+	end process p_pulse_redge; 
+
+    pulse_burst_o <= gate and pulse_burst_i;  
+
+
+2) Process p_pulse_redge_detect starting at line 154.
+
+- The signal pulse_train_in_f_edge_p needs to be reseted also.
+- If I am not mistaken, usually one uses 2 flip-flops for synchronising and an additionaly one for 
+detecting the rising/falling edge. There is a module already in general-cores/common folder that 
+can be used for that: "gc_sync_ffs.vhd". Not sure if there could be an issue with one flip-flop less...  
+
+3) Line 125. n_cycle_cnt is used as index within the array g_temp_decre_step. Maybe it would be wise 
+to constraint its value to the maximum array length.
+Alternatives:
+signal n_cycle_cnt	: integer range 0 to 15;  
+signal n_cycle_cnt	: integer range 0 to g_temp_decre_step'LENGTH;  
+signal n_cycle_cnt	: integer range g_temp_decre_step'RANGE;
+
+
+4) Process "p_n_cycle_cnt" starting at line 169.
+
+- Usually reset sets the signal at '0'. It is not a big issue, but I believe it is more standard and maybe 
+eventually it reduces the logic. 
+- It is preferable to compare with '>' or '<' than with '=' since many times you do not need to compare 
+all the bits this way.(line 183)
+
+- 'single_cycle_cnt' is reseted after reaching 'thermal_res'. But I do not see "n_cycle_cnt" to be resetted 
+when it reaches 'thermal_array_lenth'. Is that normal/wished?
+
+Proposal:
+
+
+	p_n_cycle_cnt : process(clk_i)
+	begin
+	if rising_edge(clk_i) then
+		if rst_n_i = '0' then
+			single_cycle_cnt <= 0;
+			n_cycle_cnt   <= 0;
+		elsif (en_i = '1') then
+
+			if pulse_train_in_r_edge_p = '1' and burst_ctrl_rst = '0' then	
+				single_cycle_cnt <= 0;
+				n_cycle_cnt   <= 0;
+
+			elsif pulse_train_in = '0' then
+				if single_cycle_cnt < thermal_res-2
+                   single_cycle_cnt <= single_cycle_cnt + 1; 	
+                   if n_cycle_cnt < thermal_array_lgth-1 then
+						n_cycle_cnt <= n_cycle_cnt + 1; 
+                   else
+   						n_cycle_cnt <= 0;
+                   end if; 
+				end if;
+			end if;
+		end if;
+	end if;
+  end process p_n_cycle_cnt;
+
+
+5) FSM:
+
+@line 221 : May be I am missing something.. how is that we need to test temp_rise >= 0 additionally that 
+temp_rise <= g_max_temp ? Why is not enough with the last check? Additionally  temp_rise is an unsigned 
+integer, so it should always be >= 0, doesn't it?
+
+@line 288 : Same thing.
+
+@line 291. I believe there is no need to check temp_rise /=0. You check right afterwards if it is temp_rise >= than g_temp_decre_step.
+
+
+
+
+
+
+
+
+
+