Am I Doing This Right?

Recommended Posts

I wanted to decode a quadrature rotary signal, so I started with the quadrature decoder code from opencores,quadraturecount

I was never able to get that code to count, even after ruling out other sources of problems


So I modified the code in a way that made sense to myself, it turned out much shorter then the original and best of all it works correctly


Yes, It's not enough that it works, I need outside confirmation that I did not do something bad.


My question is, did I get lucky, and am I ignoring possible timing issues, race conditions?

Under what conditions can I know that writing to a signal visible to outside circuits is safe? My head kind of hurts from thinking about it.


Code follows, thanks

library IEEE;use IEEE.STD_LOGIC_1164.ALL;use IEEE.STD_LOGIC_ARITH.ALL;use IEEE.STD_LOGIC_SIGNED.ALL;library work;--How we 'talk' to the outside world:entity QuadratureCounterPorts is    Port ( clock : in std_logic;	--system clock, i.e. 10MHz oscillator		 QuadA : in std_logic;	--first input from quadrature device  (i.e. optical disk encoder)		 QuadB : in std_logic;	--second input from quadrature device (i.e. optical disk encoder)		 CounterValue : out std_logic_vector(23 downto 0); --just an example debuggin output		 setCount : in std_logic_vector(23 downto 0);		 writeEnable : in std_logic		);end QuadratureCounterPorts;--What we 'do':architecture behave of QuadratureCounterPorts is	signal Count : unsigned(23 downto 0);		signal clockD : std_logic;	signal Quad : std_logic_vector(1 downto 0);	signal lastQuad : std_logic_vector(1 downto 0);	function gray(a:std_logic_vector) return std_logic_vector isbegin  if a="00" then    return "00";  elsif a="10" then    return "01";  elsif a="11" then    return "10";  elsif a="01" then    return "11";  end if;  return "11";end function;	begin --architecture QuadratureCounter		 	-- do our actual work every clock cycle	process(clock)	begin		--keep track of the counter		if ( rising_edge(clock) ) then					lastQuad <= Quad;		Quad <= gray((0=>QuadA,1=>QuadB));					if((unsigned(Quad) = unsigned(lastQuad)+1)) then			Count <= Count + 1;		end if;		if((unsigned(Quad) = unsigned(lastQuad)-1)) then			Count <= Count - 1;		end if;						if(writeEnable = '1') then			   --Count <= unsigned(setCount);			end if;		end if; --clock'event		CounterValue <= std_logic_vector(Count);	end process; --(clock)				   			  					end behave;
Link to comment
Share on other sites

It could help if you post a simulation result, and a synthesis report...


I assume the issue with OpenCores design is yours, not the design. Those designs are most of the time tested and proved right.


I don't understand your code at all, not the implementation (despite it might work).


You also don't seem to debounce your inputs: it's this done on purpose ?


You also use double-buffer for the output (counter and countervalue are both flipflops).

Link to comment
Share on other sites

Ok let me explain.

So quadrature is like a 2 bit gray code. Function gray() decodes the gray code into an int. Thus decoded values 0,1,2,3 correspond to sequential phases in the progression of a quadrature signal.


Every clock cycle I read the value of the 2 quadrature signals. I set "quad" to the decoded current value. Then I set "lastQuad" to the value of "quad" on the previous cycle.


By comparing the current value to the last value, i can see if the quadrature signal has progressed forward or backward. 

If quad = lastQuad+1, then it has moved forward

If quad = lastQuad-1 then if has moved backward


I don't think debouncing is necessary since I only read the signal values once a clock cycle. 



Will this really buffer the output a single clock cycle? I thought within a process any values written are immediately visible to following statements?



Link to comment
Share on other sites

I've got a PMODENC ( in the mail, so this is quite timely for me!


If you have a problem could be that you haven't denounced and synchronised the inputs. At first glace this shouldn't be a problem , as the encoder is using grey code any jitter/bounce shouldn't cause an error, just make the count value bounce up and down for a few ms. What will be a problem is that "quad" is tested in multiple places. If you look in the FPGA viewer you will see that the incoming signal gets buffered then distributed to multiple CLBs. This can lead to odd effects.


Say the latches for "LastQuad" are further away from the buffer than the logic for "unsigned(Quad) = unsigned(lastQuad)+1". If "Quad" is in transition then "count" might get updated but lastQuad latches the old value. This will cause double counting.


Alternatively if the latches for "LastQuad" are closer than the logic for "unsigned(Quad) = unsigned(lastQuad)+1" then LastQuad might get updated but the value for "Count" may not. This will cause counts to get skipped.


How likely this is depends on the different path delays and the clock speed of the design. It would be an interesting experiment to run your design at the highest clock speed that meets timing, and see how often it fails (e.g. If you know that 0 degrees should have a count of "0" then spin the encoder and see how often it gets out of sync).


The really big problem that occurs if you don't synchronise your signals. If the  input signal changes just before the value of "count" is latched you can attempt to capture the value for count while signals are still propagating  - that could cause 'count' to go from say "111111111111111111111111" to "111100000000000000000000". That might not be a big problem in most cases, but you might not want it to happen on the autopilot for your jet :-)


It would be an interesting experiment to see if you could cause that failure too.... 


As purely a matter of style, perhaps it would be good move the "lastQuad <= quad;" statement to the end of the "if" block. That takes the "delayed assignment" of lastQuad out of play.


Also as a matter of style (and hopefully discussion), do you think that converting the quadrature values into numeric values actually makes sense? Maybe soemthing like 

case lastQuad is

   when "00" => 

     up     <= quad(0);

     down <= quad(1);

   when "01" => 

     up     <= quad(1);

     down <= not quad(0);

   when "11" => 

     up     <= not  quad(0);

     down <= not quad(0);

   others  => 

     up     <= not quad(1);

     down <= quad(0);

end case;


And then act on 'up' and 'down' to update the counter. That way you can even get it right should both signals change at the same time for some weird reason.


It just occurred to me that you can also un-grey the value of quad using logic  "ungrey  <= quadA & (quadB xor quadA);"...

Link to comment
Share on other sites

Thanks hamster, I have read your comments several times now, I think it makes sense.


If I understand right, the only thing missing is that reading the inputs should be synchronized, latched, and then processed in the next clock cycle.


I'm not particularly concerned about the case when both input signals change in the same cycle, it would seem to indicate the encoder is either spinning too fast (not really possible) or falling apart.


Yeah, the decoding the "gray code" into an int is just what made sense to me  :)

Link to comment
Share on other sites

I got my encoder, and did it this way:

------------------------------------------------------------------------------------ Engineer:  Mike Field <>-- -- Module Name:    pmodenc - Behavioral -- Description:    Process the quadrature signals from the rotary encoder on --                 the Digilent PMODENC ------------------------------------------------------------------------------------library IEEE;use IEEE.STD_LOGIC_1164.ALL;use IEEE.NUMERIC_STD.ALL;entity pmodenc is    Port ( clk : in  STD_LOGIC;           quad : in  STD_LOGIC_VECTOR (1 downto 0);           value : out  STD_LOGIC_VECTOR (7 downto 0));end pmodenc;architecture Behavioral of pmodenc is   signal sr : std_logic_vector(7 downto 0) := (others => '0');   signal count : unsigned(7 downto 0) := (others => '0');begin   value <= std_logic_vector(count);process(clk)   begin      if rising_edge(clk) then         case sr(3 downto 0) is            when "0100" => count <= count+1;            when "1101" => count <= count+1;            when "1011" => count <= count+1;            when "0010" => count <= count+1;            when "1000" => count <= count-1;            when "1110" => count <= count-1;            when "0111" => count <= count-1;            when "0001" => count <= count-1;                        when others =>         end case;         sr <= quad & sr(sr'high downto 2);      end if;   end process;end Behavioral;
Link to comment
Share on other sites

I just run a simulation with sr being 4 bits and it seemed to work fine. Essentially sr holds the current and previous quadrature bits (2+2)


EDIT: nevermind I see what you're doing. This introduces a sort of hysteresis when you reverse the rotation direction (several clicks before the counter starts moving the other way).

Link to comment
Share on other sites

Four bits will work fine in simulation - but thinking about it a little more this might actually be two bits too long. 


async signal quad(1) -> register sr(7) -> register sr(5) -> register sr(3)  -> logic.

async signal quad(0) -> register sr(6) -> register sr(4) -> register sr(2)  -> logic.


Yeah, it is two bits longer than needed :  :rolleyes:

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.