24Jan code review comments update

git-svn-id: http://svn.ohwr.org/cern-fip/trunk/hdl/design@213 7f0067c9-7624-46c7-bd39-3fb5400c0213

24Jan code review comments update
git-svn-id: http://svn.ohwr.org/cern-fip/trunk/hdl/design@213 7f0067c9-7624-46c7-bd39-3fb5400c0213
babfa06e · egousiou · 0325ed9a · babfa06e · babfa06e · babfa06e
Commit babfa06e authored Apr 13, 2011 by egousiou
4 changed files
--- a/CodeReview_24Jan11/Gonzalo_EvasComments.txt
+++ b/CodeReview_24Jan11/Gonzalo_EvasComments.txt
@@ -13,12 +13,12 @@ rx_fss_crc_fec_manch_ok_p_i is valid.
 In my opinion s_cons_lgth_byte_ok should not be forced to 0 because there 
 is (for example) an error on the crc. When debugging a simulation it is easy
 assume that if s_cons_lgth_byte_ok is 0, it's only because the length was wrong.
-In fact as the file is written now, the AND between rx_fss_crc_fes_manch_ok_p_i
+In fact as the file is written now, the AND between rx_fss_crc_fes_ok_p_i
 and s_cons_lgth_byte_ok on lines 205 and 206 is redundant with the 
 "if then else"  of the lines 173, 185 and 187.
---> bug  corrected! input rx_crc_or_manch_wrong_p_i added and the Length byte
+---> bug  corrected! input rx_crc_wrong_p_i added and the Length byte
-     is checked if either rx_fss_crc_fec_manch_ok_p_i OR rx_crc_or_manch_wrong_p_i
+     is checked if either rx_fss_crc_fes_ok_p_i OR rx_crc_wrong_p_i
     (the output of this OR signals the end of a frame) is activated
@@ -35,7 +35,7 @@ Line 267: s_base_addr is an input and an output of the combinatorial process.
 Synplify should give a "combinatorial loop" warning about this.
 This line could be written as a separate process.
---> s_base_addr is out of the process, as a concurrent statement
+---> OK! s_base_addr is out of the process, as a concurrent statement
 Lines 290 to 294: the comment is not very clear. Is this to avoid writing
@@ -62,7 +62,8 @@ The second one will not be processed correctly since it will be treated as an
 RP_DAT and therefore discarded because of wrong CTRL byte. But if the second was 
 a correct ID_DAT, nanoFIP should interpret it correctly.
---> ..........................................................................
+---> After discussions with Gonzalo+Erik, we decided to accept that if 2 consecutive
+     ID_DATs and then an RP_DAT arrive, nanoFIP will discard them all.
 Line 775: 
@@ -73,7 +74,7 @@ Does it generate a latch?
 Is the construction with FOR and EXIT statement necessary?
 Line 817: The same as above
---> Processes for the var identification rewritten!
+---> OK! Processes for the var identification rewritten!
 wf_rx_tx_osc.vhd
@@ -87,7 +88,7 @@ data, but with a CRC that is correct with respect to the bits actually
 received, the frame would effectively be discarded as it should.
 (As responsible for the testbench, I take note that I should check that :-)
---> bug corrected! The FES_detected_p pulse is combined with the byte_ready_p pulse, 
+---> bug corrected! The FES_detected signal is combined with the byte_ready_p pulse, 
     that arrives only after the reception of 8 bits.

--- a/CodeReview_24Jan11/Javier_EvasComments.txt
+++ b/CodeReview_24Jan11/Javier_EvasComments.txt
@@ -22,7 +22,10 @@ I did not have time to look at it ;)
 Question for Eva (did not have time to look): we know all inputs are
 well registered. Are all outputs registered as well?
---> Yes!
+---> Yes, apart from the DAT_O bus, where, depending on the operational
+     mode signals are clocked with either the user or the wb clock.
+     Therefore there is a multiplexor right before the output.
+     We have added though an output constraint of 10 ns in the synthesis.
 nanofip.vhd
 -----------
@@ -166,7 +169,11 @@ time. This would ensure the state machine is always sitting in the
 Idle state before the beginning of a frame. With the current
 implementation, it might well be the case, but there are many
 scenarios to analyze and we risk forgetting something.
--->................................................................
+---> After discussions with Erik+Gonzalo, we decided not to add this extra
+     check, to avoid complications. A counter depending only on the system
+     clock has been added though. A frame could potentially be lost until
+     the counter times-out, but in principle there there wont be a hunging.
 wf_rx_tx_osc.vhd
 ----------------

--- a/CodeReview_24Jan11/Matthieu_EvasComments.txt
+++ b/CodeReview_24Jan11/Matthieu_EvasComments.txt
@@ -38,7 +38,7 @@ general comments
 * Use @details for big descriptions and @brief just for a summary.
---> The details in the @details are the dependencies, modified by, etc
+---> Removed doxygen structure
 * To clean file layout, in emacs you could use C-c C-b and M-x delete-trailing-whitespace
@@ -104,7 +104,7 @@ Line 83. assert_RSTON should be replaced by assert_RSTON_p
 Lines 269, 435. Use of s_por (asynchronous) in a synchronous process!
 ---> Was not a completed processs (mentioned in the todo list)!
-     Now the falling edge of RSTPON is synched with the uclk and the wb_clk.
+     Now the deactivation of RSTPON is synched with the uclk and the wb_clk.
 Line 408. Assignment of s_counter_full is far from here.
@@ -232,7 +232,7 @@ Line 323. The only way to get out of switch_to_deglitched is to receive a
 	  -> A reset is needed.
 	  All other states can return to IDLE, but not this one!
 #################### IMPORTANT ####################
---> OK! state removed (deglitcher changed) and general timeout relying only on
+---> OK! state removed (deglitcher changed) and timeout relying only on
     the system clock added.
@@ -246,8 +246,9 @@ Line 586. Instance value same as default value.
 Line 631. You say "that doesn't belong to the FES" but in the if there is
     	  s_fes_wrong_bit = '1'. So either the comment or the test is wrong.
---> I think it is all correct! If s_fes_wrong_bit=1, the bit does not
+---> I think it was all correct! If s_fes_wrong_bit=1, the bit does not
-     belong to the FES
+     belong to the FES. However, finally it was decided to remove the manch.
+     encoding check.
 Line 692. (not s_manch_not_ok) = manchester ok, which is probalby not what you want.
@@ -260,7 +261,7 @@ wf_cons_bytes_processor.vhd
 Line 95. Still some stuff to do...
 Line 292. two can be replaced by 2 as you're dealing with unsigned.
---> OK!
+---> Oups..OK!
 Line 392. Here is the var_whatever!
@@ -283,7 +284,7 @@ You should mention that this entity is vendor/component specific.
 WF_cons_bytes_to_dato.vhd
 --------------------------------------------------------------------------------
 Lines 136, 143. how could "it" stays there if the signal in the test is a pulse?
---> There is no "else" in the "if"
+---> There is no "else" in the "if".

--- a/CodeReview_24Jan11/Tom_EvasComments.txt
+++ b/CodeReview_24Jan11/Tom_EvasComments.txt
@@ -211,7 +211,7 @@ wf_rx_deserializer.vhd:
 ---> OK!
 - (O) line 424: this case is never reached
---> Not sure..u mean the "when others"? That is essential for recovery from SEU
+---> Not sure..u mean the "when others"? That is essential bc of the one hot encoding and the radiation environment
 wf_cons_bytes_to_dato.vhd
@@ -231,7 +231,7 @@ wf_cons_frame_validator.vhd
 >                    var_type_match = '1'
 >               and cons_ctrl_byte_i = c_RP_DAT_CTRL_BYTE
 >               and cons_pdu_byte_i = c_PROD_CONS_PDU_TYPE_BYTE
->               and unsigned(rx_byte_index_i) = unsigned(cons_lgth_byte_i) + 5
+>               and unsigned(byte_index_i) = unsigned(cons_lgth_byte_i) + 5
 >               else '0';
 - line 211: value of nfip_status_r_tler_o is equal to cons_frame_ok_p_o (because s_cons_lgth_bytes_ok depends on rx_fss_crc_fes_manch_ok_p_i already)
 This can be simply integrated into WF_consumption.vhd, it would take less than the instantiation of the validator entity.