Skip to content
Projects
Groups
Snippets
Help
Loading...
Sign in
Toggle navigation
C
Conv TTL Blocking - Gateware
Project
Project
Details
Activity
Cycle Analytics
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Charts
Issues
1
Issues
1
List
Board
Labels
Milestones
Merge Requests
0
Merge Requests
0
Wiki
Wiki
image/svg+xml
Discourse
Discourse
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Charts
Create a new issue
Commits
Issue Boards
Open sidebar
Projects
Conv TTL Blocking - Gateware
Commits
f052675d
Commit
f052675d
authored
Jan 27, 2017
by
Grzegorz Daniluk
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Greg's comments
parent
45c22dbc
Hide whitespace changes
Inline
Side-by-side
Showing
1 changed file
with
29 additions
and
0 deletions
+29
-0
greg.txt
doc/code-review/v4-27-01-2017/greg.txt
+29
-0
No files found.
doc/code-review/v4-27-01-2017/greg.txt
0 → 100644
View file @
f052675d
* Missing Copyright CERN notice
* test wrapping for Description
* indentation sometimes done with spaces, sometimes with tab
138: process p_pulse_redge is not clear
what happens if en_i goes to '0'? It looks like in this design, this is not a
problem because in conv_common_gw en_i => '1'. This input should be either
removed or the process should take into account also en_i = '0'.
154: process p_pulse_redge_detect: if pulse_burst_i is not synchronized to clk_i
domain, it needs gc_sync_ffs (and this module btw. can also detect rising and
falling edge). This process has not enough flip-flops to be a synchronizer.
pulse_train_in_f_edge_p is not reseted
169: (pulse_train_in_r_edge_p = '1' and burst_ctrl_rst = '0') could be part of
the reset condition together with rst_n_i = '0', just for cleanup.
* lookig at the state machine, it looks like temp_rise can go up only after the
falling edge of the pulse, so process in linse 138 can be simplified to gating
pulse with burst_ctrl_rst signal.
287, 299: instead of overwriting burst_ctrl_rst signal value, put <= '0'
assignment after the first 'if' statement.
Similar thing with burst_err_p_o.
What is the value of burst_err_p_o if temperature is within limits? Missing
burst_err_p_o <= '0';
314: use 'else' for burst_err_p_o <= '0' in conditional instruction.
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment