eva_g first comments

7e23550b · Evangelia Gousiou · 8e820682 · 7e23550b
Commit 7e23550b authored Jan 26, 2017 by Evangelia Gousiou
Hide whitespace changes
Inline Side-by-side

Showing with 39 additions and 0 deletions

eva_g.txt doc/code-review/v4-27-01-2017/eva_g.txt +39 -0

No files found.
--- a/doc/code-review/v4-27-01-2017/eva_g.txt
+++ b/doc/code-review/v4-27-01-2017/eva_g.txt
+General
+----------------------
+- presentation with simulation has been very useful!
+
+- as there is code from several people, the path of a pulse from input to output is not very obvious;
+sometimes the input is referred to as "pulse", others as "trigger" etc 
+
+- structure clean up as in http://www.ohwr.org/projects/ohr-support/wiki/Folder_structure_example
+(eg: conv-ttl-blo-gw\ip_cores\conv-common-gw\ip_cores\general-cores -> conv-ttl-blo-gw\ip_cores\general-cores)
+
+- cleanup of syn folder (eg: conv_ttl_blo (2).xise :-)
+
+conv_dyn_burst_ctrl
+----------------------
+- line 8: as git does the job, version/date/dependencies/last changes could be removed
+
+- line 12: for readability if possible max 100 characters per line
+
+- line 100: could be: th_array_lgth <= 6 when pwidth = 5 else 15;
+
+- line 153: could use gc_sync_ffs; otherwise pulse_train_in_f_edge_p could also be initialized if (rst_n_i = '0')
+and could use another dff before the edge detection for metastability
+
+- line 138: en_i not in sensitivity list for sim; process not very clear and the comment "Generate the
+pulse on rising edge of pulse_burst_i" is not clear.
+could the pulse_train_in assignments be moved in the p_thermal_fsm_outputs process or
+do sth like "pulse_train_in <= '0' when burst_ctrl_rst = '1' else pulse_burst_i and en_i;"?
+
+- line 237: 6 could be replaced by a constant
+
+- line 286: should be moved inside the "if" (after line 287)
+
+- line 299: i think this could be skipped, as in this case the FSM will already be in the PULSE_REJECT state
+
+- line 315: should be moved inside an "else"
+
+conv_comm_gw
+----------------------
+- line 767: burst_en_n_i could be renamed to something reflecting the lg vs sh option
\ No newline at end of file