Commit c50b4994 authored by Theodor-Adrian Stana's avatar Theodor-Adrian Stana

Added Erik's comments

parent b44e39dc
bicolor_led_ctrl_pkg
================
led_intensity_i : in std_logic_vector(6 downto 0);
Not defined what intensity range means and what would be a good value by default. Is it compensated for R/G difference in intensity?
Constant declarations constant c_LED_RED should be documented to which input it fits.
conv_ttl_blo.vhd
===============
-- Firmware version
constant c_fwvers : std_logic_vector(15 downto 0) := x"0200";
Specify the format (2.00)
How do you define all the signals on the board that are not used?
What are the levels of these? Any thoughts on how current consumption can be reduced (disable, low f clock setting etc)?
(after our discussion: do not 'just' set all signals to inputs of the Xilinx. Inputs from the other chips on the board should get a defined level. It really can blow up chips if an input is floating or by bad luck at the wrong level).
See chapter 3 of http://www.ti.com/lit/an/sdya009c/sdya009c.pdf for an example.
Indeed waving a hand above the circuit may change the level. Maybe an unused input was important actually, but tested under your circumstances. Waving a hand may make it fail.
signal vbcp_err : std_logic; 1 space too many
-- Process to blink the LED for a finite amount of time when the vbcp_tip
-- signal is set. [can explain vbco_tip?]
conv_regs_id_bits_o => open, [Is VHDL way to show that is not connected?]
Use much more spacing to clarify that there's a lot in this generate statement:
-- And now the OR gate at the inputs of the pulse generator blocks
trig_a <= trig_ttl_a or trig_blo_a;
||||| more line spaces (3 or 4)
gen_ttl_pulse_generators : for i in 1 to g_nr_ttl_chan generate
-- First, resync the trigger signal into clk125 domain
cmp_sync_ffs: gc_sync_ffs
port map
(
clk_i => clk125,
rst_n_i => rst_n,
data_i => trig_a(i),
.....
led_intensity_i => "1111111", [Could it be a bit less lighter to save current and reliability]?
reset_gen component declaration in top:
g_reset_time : positive := 5_000_000
while in the real component itself:
g_reset_time : positive := 12_000_000
-> does not correspond! Should give a warning during synthesis?
End of file, add some line to show clearly end of file
--============================================================================
-- end of conv_ttl_blo.vhd
--============================================================================
(actually do that already on reset_gen and many other files)
reset_gen.vhd
===========
Gave example of typical reset time.
Should it not stop on things like > n to be more resilient to errors?
Pulse generator with trigger
======================
Very nice description and clear header! Rest of file very nice too. Really how I like to see it.
Unfortunately the header is missing the entity name ctb_pulse_gen.
No need to have your e-mail address on change list. Use the space for the explanation.
Dependencies: none. Should this not list the other entities it's using (glitch_filt)?
" This module generates a variable-width pulse." Actually it generates a constant width pulse. The width is settable at synthesis time by the generic .....
Specify the current width that is set and why.
Likely not used. Remove these from the project.
In reset_gen/top
Top level entity of CONV-TTL-BLO V1 (in file called V2)
---
FIFO_simple.vhd is not a FIFO (e.g. it has no full/empty flags). It is just a resettable register.
FIFO variable stack length - doc not clear. The image is not clear (does it take the LSB of each Reg?) The output is a vector of depth * width in size. Again, not a FIFO.
----
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment