Comments on pulse generation

e1851be4 · Javier Serrano · 40b1950d · e1851be4
Commit e1851be4 authored Nov 07, 2013 by Javier Serrano
Hide whitespace changes
Inline Side-by-side

Showing with 37 additions and 4 deletions

javier.txt design-review/javier.txt +37 -4

No files found.
--- a/design-review/javier.txt
+++ b/design-review/javier.txt
@@ -100,10 +100,43 @@ signal. In particular, consider the fact that the signal is going to
 the enable inputs of the counter flip-flops. What happens if in a
 given clock cycle some FFs see the signal as '1' and others as '0'?

+It would be good to declare the interfaces to the DAC chips driving
+the VCXOs and to drive safe constant values into those lines. In
+particular, before we include WR support, the SYNC_N inputs of the
+DACs should be driven by a constant '1' to make sure there is no risk
+of spurious changes in DAC output voltage.
+
+glitch_filt.vhd
+===============
+
+I think dat_i should be passed through a 2-FF synchronizer before
+being fed into the shift register. If you don't do this, you can have
+a metastable state in after the first stage of the shift register, and
+that bit is going to two destinations: the comparator and the next
+stage of the register. These two destinations could see different
+values of that bit in the same clock cycle. This is all hard to
+analyze and evaluate. Why not simplify and sync the input before using
+it anywhere? It will add a couple of ticks of delay, but who
+cares. The deglitching block already adds quite a bit of delay.
+
+ctb_pulse_gen.vhd
+=================
+
+I like the state machine approach. It's simple and robust.
+
+There is clearly a minimum value for the width of the pulse and a
+maximum value for the glith filter length. I wonder if there is an
+elegant way of refusing to generate gateware for people who
+instantiate this block with illegal values for these generics. Maybe
+constrain their range in the generic declaration clause?
+
+The names of the constants used to compare the current counter value
+with them and decide on state transition are a bit misleading. If I
+see a constant called "c_pulse_width_gf_off" I expect it to hold the
+pulse width, not some other value which is needed in the state machine
+so the actual pulse width turns out to be g_pwidth.
+
 Todo
 ====

- Check DAC output for OSC1 has a stable voltage. 
-
- Check that the duty cycle of 1/5 max is also imposed when the glitch
-  filter is active.
+- Check important files singled out by Thedi in his message.