Skip to content
This repository was archived by the owner on Jul 29, 2022. It is now read-only.

Conversation

@ctrea99
Copy link
Contributor

@ctrea99 ctrea99 commented Aug 13, 2020

Module for instantiating the ALTLVDS_RX IP and for performing word alignment on the incoming data from the VNIR sensor

@alex-epp alex-epp self-requested a review August 13, 2020 03:31
Copy link
Member

@alex-epp alex-epp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I looked at your code. For the most part it looks great. I can't see any obvious bugs or anything.

I made some comments in some of the files that highlight some issues I saw.

One other potential concern is I don't see any code that tests to see if your code is synthesizable (e.g. a top-level VHDL file that instantiates the LVDS reader and prevents it being optimized out somehow with e.g. keep attributes).

Also, have you looked into what kind of timing constraints we need for the LVDS input (if ALTLVDS doesn't handle that for us)?

@@ -0,0 +1,332 @@
----------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You aren't using the sensor_comm files anywhere right? If not they shouldn't be in this pull request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my fault. I updated my local files from the github repo and it left those ones for some reason. Still not sure how to properly keep branches updated on a local machine.

generic (
NUM_CHANNELS : integer := 16
);
port(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand right, the ports for this entity are in grouped according to three clock domains: system (reset_sys, alignment_done, cmd_start_align, word_alignment_error), LVDS in (lvds_signal_in) and parallel data out (data_par_XX, ctrl_par). A comment explaining this might make it a little more clear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I intended there to be only 2 clock domains: the serial LVDS signals and clock which operate at 10x system clock frequency and everything else which runs at the system clock frequency.

@ctrea99 ctrea99 requested a review from alex-epp September 4, 2020 18:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants