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

Question

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;

• Grabul: [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.
• Cody495: 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?
• JHBonarius: 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? ;)
• Cody495: 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?
• Grabul: 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).

Answer 1

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;

• Cody495: 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: 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?
• JHBonarius: 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.
• Cody495: 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?
• JHBonarius: 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.