# My VHDL code is not processing an incoming signal correctly during a select set of time

I am programming using VHDL, and I am having an issue processing an incoming signal when trying to look at it only for a specific time frame.

Lets say I have an incoming signal, GPIO.

In a process (triggered by 50 MHz clock) an internal signal, A, gets assigned the value of GPIO during a .1 ms window, while it is assigned '0' for all other times during a 2 ms period.

Signal A is then put through an LCELL several times to delay the signal.

Some selected components of this delayed signal (vector A_internal(23 downto 0) ) are put into a mux4to1 with A (A gets NOT-ANDED with a delayed signal to produce a shorter signal, A_s).

A_s is then counted and used to calculate some other data.

My issue is this: when A is constantly reading GPIO (it does not go through the clock 50 process), the code runs correctly. But when I try to use this method to look at GPIO only at certain times, the code will never read GPIO (or at least I am getting counts of 0, always). Is there an error in the way I am approaching this "selection window" method or is there something I and doing wrong in the code itself that is causing this to not be read?

Here is the relevant code (please to tell me if any other information is needed to help answer my question):

LIBRARY ieee;
USE ieee.std_logic_1164.all;

ENTITY Project IS
PORT (
Clock_50 : IN STD_LOGIC; -- The 50 MHz clock that is provided on the DE2 Board
SW       : IN STD_LOGIC_VECTOR(17 DOWNTO 0); -- The switches 0 through 17 on the DE2 Board
GPIO_0   : IN STD_LOGIC_VECTOR(16 DOWNTO 10); -- The 40 pin expansion header GPIO_0 pins, which can be used as input or output signals
);
END Project;

ARCHITECTURE Behavior OF Project IS
-- This component chooses one of the three delayed pulses, inverts the chosen pulse,
-- then ANDs the inverted, delayed pulse with the original (effectively shortening the original)
COMPONENT mux4to1
PORT (
delayedpulse_0 : IN  STD_LOGIC;
delayedpulse_1 : IN  STD_LOGIC;
delayedpulse_2 : IN  STD_LOGIC;
pulse          : IN  STD_LOGIC;
SW             : IN  STD_LOGIC_VECTOR(1 DOWNTO 0);
pulseout       : OUT STD_LOGIC
);
END COMPONENT;

COMPONENT LCELL
PORT (
a_in  : IN  STD_LOGIC;
a_out : OUT STD_LOGIC
);
END COMPONENT;

-- These SIGNALs represent the four input pulse from the detectors
SIGNAL A, B, C, D, AAA, BBB, CCC, DDD: STD_LOGIC;

-- These SIGNALs represent the three shortened versions of the pulse,
-- one of which (along with the original signal) will be chosen by the 4-to-1 mux
SIGNAL A_internal, B_internal, C_internal, D_internal: STD_LOGIC_VECTOR(27 DOWNTO 0);

-- These SIGNALs represent the shortened pulses output by the mux4to1 COMPONENT
SIGNAL A_s, B_s, C_s, D_s: STD_LOGIC;

--The SYN_KEEP attribute preserves the signals through the compiler,
--so that they are not automatically optimized away as redundant logic
ATTRIBUTE SYN_KEEP : BOOLEAN;
ATTRIBUTE SYN_KEEP of A_internal: SIGNAL is TRUE;
ATTRIBUTE SYN_KEEP of B_internal: SIGNAL is TRUE;
ATTRIBUTE SYN_KEEP of C_internal: SIGNAL is TRUE;
ATTRIBUTE SYN_KEEP of D_internal: SIGNAL is TRUE;

-- NOTE: These shared variables are changed and modified in a different process,
-- which was left out of this code. This is why they are shared variables
-- and not regular variables used in the one process.
shared variable PhaseShift     : integer := 0; -- multiples of 20 ns
shared variable SelectionWidth : integer := 10000;
Begin
AAA <= GPIO_0(10);
BBB <= GPIO_0(12);
CCC <= GPIO_0(14);
DDD <= GPIO_0(16);

-- This creates, using iteration, a chain of LCELL buffers for each A, B, C, D signal,
-- which act to delay the signals.
LCA_1: LCELL PORT MAP(a_in=> A, a_out=>A_internal(0));
Gen_delay_A : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => A_internal(i),a_out => A_internal(i+1));
END GENERATE;

LCB_1: LCELL PORT MAP(a_in=> B, a_out=>B_internal(0));
Gen_delay_B : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => B_internal(i),a_out => B_internal(i+1));
END GENERATE;

LCC_1: LCELL PORT MAP(a_in=> C, a_out => C_internal(0));
Gen_delay_C : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => C_internal(i),a_out => C_internal(i+1));
END GENERATE;

LCD_1: LCELL PORT MAP(a_in=> D, a_out => D_internal(0));
Gen_delay_D : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => D_internal(i),a_out => D_internal(i+1));
END GENERATE;

process(Clock_50)
variable Period : integer := 100000;
variable count  : integer := 0;
begin
if (rising_edge(Clock_50)) then
-- Increase the count by one every time there is a rising edge on the 50MHz clock
count := count+1;

if (count=0) then
-- Begin by ignoring the GPIO_0 pins
A <= '0';
B <= '0';
C <= '0';
D <= '0';
elsif (count=1+PhaseShift) then
-- Each signal begins to read GPIO_0 once the count reaches a certain time,
-- set by PhaseShift.
A <= GPIO_0(10);
B <= GPIO_0(12);
C <= GPIO_0(14);
D <= GPIO_0(16);
elsif (count=1+PhaseShift+SelectionWidth) then
-- Once the signals have read GPIO_0 for a set amount of time SelectionWidth,
-- the signals are then set back to 0.
A <= '0';
B <= '0';
C <= '0';
D <= '0';
elsif (count=Period) then
-- Once the cycle has finished its period,
-- the count is reset and the process begins again.
count := 0;
end if;
end if;
end process;

MA: mux4to1 PORT MAP(A_internal(8), A_internal(16), A_internal(24), AAA, SW(17 DOWNTO 16), A_s);
MB: mux4to1 PORT MAP(B_internal(8), B_internal(16), B_internal(24), BBB, SW(17 DOWNTO 16), B_s);
MC: mux4to1 PORT MAP(C_internal(8), C_internal(16), C_internal(24), CCC, SW(17 DOWNTO 16), C_s);
MD: mux4to1 PORT MAP(D_internal(8), D_internal(16), D_internal(24), DDD, SW(17 DOWNTO 16), D_s);
END Behavior;


Here is my new code with the suggestions from below:

LIBRARY ieee;
USE ieee.std_logic_1164.all;
USE ieee.numeric_std.all;

ENTITY Project IS
PORT (
Clock_50 : IN STD_LOGIC; -- The 50 MHz clock that is provided on the DE2 Board
SW       : IN STD_LOGIC_VECTOR(17 DOWNTO 0); -- The switches 0 through 17 on the DE2 Board
GPIO_0   : IN STD_LOGIC_VECTOR(16 DOWNTO 10); -- The 40 pin expansion header GPIO_0 pins, which can be used as input or output signals
);
END Project;

ARCHITECTURE Behavior OF Project IS
-- This component chooses one of the three delayed pulses, inverts the chosen pulse,
-- then ANDs the inverted, delayed pulse with the original (effectively shortening the original)
COMPONENT mux4to1
PORT (
delayedpulse_0 : IN  STD_LOGIC;
delayedpulse_1 : IN  STD_LOGIC;
delayedpulse_2 : IN  STD_LOGIC;
pulse          : IN  STD_LOGIC;
SW             : IN  STD_LOGIC_VECTOR(1 DOWNTO 0);
pulseout       : OUT STD_LOGIC
);
END COMPONENT;

COMPONENT LCELL
PORT (
a_in  : IN  STD_LOGIC;
a_out : OUT STD_LOGIC
);
END COMPONENT;

CONSTANT Period                                        : positive := 100000;        -- number of counts, by multiples of 20 ns (every clock rising edge is 20ns)
CONSTANT SelectionWidth                                : positive := Period/10;     -- number of counts, by multiples of 20 ns

SIGNAL   PhaseShift                                    : natural range 0 to Period-SelectionWidth := 0;
SIGNAL   count                                         : natural range 0 to Period := 0;        -- one count for every rising edge of 50 MHz clock

SIGNAL   A, B, C, D, AAA, BBB, CCC, DDD                : STD_LOGIC;    -- These SIGNALs represent the four input pulse from the photon detectors
SIGNAL   A_internal, B_internal, C_internal, D_internal: STD_LOGIC_VECTOR(27 DOWNTO 0); -- These SIGNALs represent the three shortened versions of the pulse, one of which (along with the original signal) will be chosen by the 4-to-1 mux
SIGNAL   A_s, B_s, C_s, D_s                            : STD_LOGIC; -- These SIGNALs represent the shortened pulses output by the mux4to1 COMPONENT

ATTRIBUTE SYN_KEEP : BOOLEAN;   --The SYN_KEEP attribute preserves the signals through the compiler, so that they are not automatically optimized away as redundant logic
ATTRIBUTE SYN_KEEP of A_internal: SIGNAL is TRUE;
ATTRIBUTE SYN_KEEP of B_internal: SIGNAL is TRUE;
ATTRIBUTE SYN_KEEP of C_internal: SIGNAL is TRUE;
ATTRIBUTE SYN_KEEP of D_internal: SIGNAL is TRUE;

Begin
AAA <= GPIO_0(10);
BBB <= GPIO_0(12);
CCC <= GPIO_0(14);
DDD <= GPIO_0(16);

clk_count: process(Clock_50)
begin
if (rising_edge(Clock_50)) then
if (count<=(Period-1)) then
count <= count + 1;
else
count <= 0;
end if;

if (count=(0))then  -- Begin by setting the output pulse to 0
A <= '0';
B <= '0';
C <= '0';
D <= '0';
elsif ((count>(1+PhaseShift)) and  (count<(1+PhaseShift+SelectionWidth)))then  -- Each pin is triggered once the count reaches a certain time delay, set by PhaseShift
A <= GPIO_0(10);
B <= GPIO_0(12);
C <= GPIO_0(14);
D <= GPIO_0(16);
elsif (count>=(1+PhaseShift+SelectionWidth))then  -- Once the pulse has reached a set amount of time SelectionWidth, the pulse is then turned back to 0.
A <= '0';
B <= '0';
C <= '0';
D <= '0';
end if;
end if;
end process;

LCA_1: LCELL PORT MAP(a_in=> A, a_out=>A_internal(0));  -- This creates, using iteration, a chain of LCELL buffers for each A, B, C, D signal, which act to delay the signals.
Gen_delay_A : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => A_internal(i),a_out => A_internal(i+1));
END GENERATE;

LCB_1: LCELL PORT MAP(a_in=> B, a_out=>B_internal(0));
Gen_delay_B : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => B_internal(i),a_out => B_internal(i+1));
END GENERATE;

LCC_1: LCELL PORT MAP(a_in=> C, a_out => C_internal(0));
Gen_delay_C : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => C_internal(i),a_out => C_internal(i+1));
END GENERATE;

LCD_1: LCELL PORT MAP(a_in=> D, a_out => D_internal(0));
Gen_delay_D : FOR i in 0 to 23 GENERATE
LC : LCELL PORT MAP(a_in => D_internal(i),a_out => D_internal(i+1));
END GENERATE;

MA: mux4to1 PORT MAP(A_internal(8), A_internal(16), A_internal(24), AAA, SW(17 DOWNTO 16), A_s);
MB: mux4to1 PORT MAP(B_internal(8), B_internal(16), B_internal(24), BBB, SW(17 DOWNTO 16), B_s);
MC: mux4to1 PORT MAP(C_internal(8), C_internal(16), C_internal(24), CCC, SW(17 DOWNTO 16), C_s);
MD: mux4to1 PORT MAP(D_internal(8), D_internal(16), D_internal(24), DDD, SW(17 DOWNTO 16), D_s);
END Behavior;

• [Disclaimer : I don't understand well the purpose of this code] Don't you need < and > operators for your selection window ? Maybe (count<=1+PhaseShift)?. When there is no assignment, outside the IF conditions (count=Period, count=1+PhaseShift+SelectionWindow,count=1+PhaseShift, count=0), signals A, B, C and D will keep the last assigned value. – TEMLIB Mar 26 '17 at 21:05
• The purpose of this code is to count coincidences between the 4 signals, so the code shortens the signals so that there are no accidental overlaps. What I'm trying to do now is instead of looking at the signals constantly, I want to only look at a select "window" of time in a given period. Could you please explain to me why I would need and operators? And would the signals not keep their last assignment when outside the IF conditions? – Cody495 Mar 27 '17 at 0:46
• I see you are using variables and even shared variables in our code. Only use these when you know what you are doing. DO you know what your doing? ;) – JHBonarius Mar 27 '17 at 10:00
• I know exactly what I am trying to do, and I know exactly what everything else in my for sf doing as well, but I also do not know what I do not know! I figured that I am using these correctly since I am just using integers to count up or as numbers to count up to. Is there any issue with that? And why do you say that you should only use them when you know what you are doing? – Cody495 Mar 27 '17 at 14:08
• Not AND operators, but inferior (<) or superior (>) comparison operators. For inter-process communications, signals are less exotic than shared variables (which may require in strict VHDL some restrictions as guarding...). Shared variables are principally used in test benches or for synthesisable multiport large memories (as variables are faster for simulation than signals). – TEMLIB Mar 27 '17 at 19:26

The problem lies here:

process(Clock_50)
variable Period : integer := 100000;
variable count  : integer := 0;


This means that every time the Clock_50 triggers this process, variable count is set to 0. Just like it states

That is why I mentioned in the comments about the variables. Only use them if you know what you are doing. You should use signals instead.

    constant Period : positive:= 100000;
signal count : natural range 0 to Period-1 := 0;
begin
[...]
clk_proc: process(Clock_50) -- I added a label. easier for debugging.
begin
if (rising_edge(Clock_50)) then
if (count<(Period-1)) then
count <= count + 1;
else
count <= 0;
end if;

• Thank you for this help, I did move those two variables to the process recently thinking it would clear up clutter, I didn't think about that! Although I think I was still having the issue before i made the change as well.. I will need to test again on Wednesday and see if that really was the only issue. I have been reading a lot about using signals/variables, and I am going to switch everything to signals and constants in my code like you have suggested. – Cody495 Mar 28 '17 at 13:11
• I do have one more question: what is the difference between using a signal type integer vs natural vs positive? Are there any advantages/disadvantages to using a specific one? – Cody495 Mar 28 '17 at 13:11
• the simulator will warn you when a natural becomes negative, or when a positive becomes less then 1. They are subtypes of integer, just more strict. – JHBonarius Mar 28 '17 at 13:32
• Okay, then natural is the better choice for this situation. In your solution and my original code, we use a process triggered by Clock_50 to increment our count signal. I have some other code (used for rs232 communications; baud counters, etc.) That use an lpm megafunction counter to increment an std_logic_vector instead, then use another process to trigger a clock and rest the vector. Is one of them a preferred or "more standard" method, or does it just depend on the situation and which is more convenient? – Cody495 Mar 28 '17 at 14:57
• It depends on what you want. imho you should aim for readable and maintainable code. I don't consider a number of component instantiations to be readable. The market is even slowly going to C/C++ to HDL compilers (HLS), because more and more developers think VHDL/verilog rtl is not sufficiently high level. – JHBonarius Mar 28 '17 at 16:03