VHDL expert puzzle

The big flaw is the combinatorial feedback of bit 0. since d0 <=
xnor...bit0 
and bit0 is assigned do.

Kaz	   
					
---------------------------------------		
Posted through http://www.FPGARelated.com

>The big flaw is the combinatorial feedback of bit 0. since d0 <=
>xnor...bit0 
>and bit0 is assigned do.
>
>Kaz	   
>					
>---------------------------------------		
>Posted through http://www.FPGARelated.com
>

sorry correcting myself: there is one register between bit0 and d0.
I believe the author has wrong observation.
if simulation worked it should work in hardware.
The fact that it didn't then it worked by moving d0 to clocked process is
fishy.

Kaz	   
					
---------------------------------------		
Posted through http://www.FPGARelated.com

On 11/25/2012 08:11 PM, Jan Decaluwe wrote:
> In the following link, a design is presented that alledgedly
> has a flaw. The claim is that this is a simple case and
> that any experienced designer will see the flaw immediately.
>
> (I don't.)
>
> http://www.programmableplanet.com/author.asp?section_id=2551&doc_id=254705&
>

I thank each one of you very much for the comments.

It's good to talk to people who care about these matters
and know what they are talking about. On APP, there was
almost no progress during 5 (five!) days.

Even better, one of the comments inspired me to construct
an hypothesis of what went wrong here. Alan Fitch wondered
why lfsr was not an unsigned to make the comparison nicer.

Suppose that in his original code (not the one he posted)
the comparison would be with a bitstring of the wrong
size. That would always fail and explain the behavior
that he describes.

He may have misunderstood the fix when making changes
to the code. Where he then gets his strange explanations
is anybody's guess.

Thanks,

Jan

-- 
Jan Decaluwe - Resources bvba - http://www.jandecaluwe.com
     Python as a HDL: http://www.myhdl.org
     VHDL development, the modern way: http://www.sigasi.com
     World-class digital design: http://www.easics.com

Le 25/11/2012 21:53, rickman a �crit :

> One blog comment is rather pertinent.  Hamster said, "I'm quite sure
> that a good test bench will be far more complex than the unit under
> test, and far harder to write."  This has been my experience and I think
> this point should be emphasized with engineers time and time again!
> Writing code is easy.  Writing verified code, not so much.

Rule of thumb I once heard is to put twice as much engineers in 
verification as there are in design.
Funnily, the company I worked for at the time did exactly the reverse. 
And surprisingly they were kicked out of the project (and I resigned)

Nicolas

On 11/25/2012 6:31 PM, Jan Decaluwe wrote:
> On 11/25/2012 08:11 PM, Jan Decaluwe wrote:
>> In the following link, a design is presented that alledgedly
>> has a flaw. The claim is that this is a simple case and
>> that any experienced designer will see the flaw immediately.
>>
>> (I don't.)
>>
>> http://www.programmableplanet.com/author.asp?section_id=2551&doc_id=254705&
>>
>>
>
> I thank each one of you very much for the comments.
>
> It's good to talk to people who care about these matters
> and know what they are talking about. On APP, there was
> almost no progress during 5 (five!) days.
>
> Even better, one of the comments inspired me to construct
> an hypothesis of what went wrong here. Alan Fitch wondered
> why lfsr was not an unsigned to make the comparison nicer.
>
> Suppose that in his original code (not the one he posted)
> the comparison would be with a bitstring of the wrong
> size. That would always fail and explain the behavior
> that he describes.
>
> He may have misunderstood the fix when making changes
> to the code. Where he then gets his strange explanations
> is anybody's guess.

This is one of the reasons why I've never created a blog or other 
"expert" column on the web.  I may be fairly experienced, but by writing 
things like this blog I may be showing what things I *don't* know rather 
than what I do... lol

Rick

On 26 Nov., 19:14, rickman <gnu...@gmail.com> wrote:
> This is one of the reasons why I've never created a blog or other
> "expert" column on the web. =A0I may be fairly experienced, but by writin=
g
> things like this blog I may be showing what things I *don't* know rather
> than what I do... lol

I sign this statement ;)

On 25 Nov., 20:11, Jan Decaluwe <j...@jandecaluwe.com> wrote:
> In the following link, a design is presented that alledgedly
> has a flaw. The claim is that this is a simple case and
> that any experienced designer will see the flaw immediately.

If this code is from an experienced designer I see that much flaws,
that have no effect of synthesis or simulation itself, but on
readability.
The sensitivity list is horrible and will cause unnecessary simulator
load, the code indention is best effort to confuse readers. Some code
beautify would do better. And as simple as this design is, a real
world code without any comments is only good for protecting your
failures from reviewers.

The usage of integer as start value for a lfsr is quite error prone. A
hex value would be easier to read and would allow to design this
module without integer or unsigned values.
That clk_out is missing in reset path leads to one cycle latency of
rst to clk_out, that is most likely not intended here. But you could
only guess if that has impact on system or is even necessary.
If there is an abvious error in cycle lenght (be it start value or
feedback function), you can see this in simulation. I assume if you
use lfsr, you know that you should double check this, if you are not
really sure what you are doing.

bye Thomas

On 11/25/2012 09:22 PM, Alan Fitch wrote:

> OK I cracked and read it.
>
> I don't really understand what's going on, I would have to simulate it
> (though of course you've already done that Jan!).

Yes.

> I agree there's something fishy about adding output delays. What
> difference would that make in this case?

According to me, none whatsoever. And that is also what I
see in my simulation.

The crazy thing is that he says he does see a difference,
in the sense that the version with delays "reveals the problem."
In particular, he claims that the output clock clk_out
never toggles in the version with delays, which is what
he says he sees on the FPGA also.

He includes waveforms and uses that observation to claim
that "the mystery has been solved".

But I see clk_out toggling correctly (as I would expect).
In particular, in my simulations it toggles at exactly
the same moments as in the version without delays.

The code has been posted on APP in ready-to-run format.
What is driving me crazy is that noone else on APP seems
to be interested in running it and confirming my results,
or not.

I'm not going to ask that from anybody here - you have helped
me enough already. Still, I include the code as it was posted
below, perhaps someone is interested anyway. (The test bench
as posted contains the design with the delays, and is ready
to run.)

Thanks,

Jan

----

First pass at code


library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity lfsr_counter is
   port (
      rst, clk : in std_logic;
     clk_out : out std_logic);
end lfsr_counter;

architecture imp_lfsr_counter of lfsr_counter is	
   signal lfsr: std_logic_vector (13 downto 0);	
   signal d0, clk_i: std_logic;	
   constant countmax: integer:=2685;
begin

   d0 <= lfsr(13) xnor lfsr(4) xnor lfsr(2) xnor lfsr(0) ;

   process(clk,clk_i,lfsr,rst) begin
	
	  if rising_edge(clk) then
		   if rst='1' then
			  clk_i<='0';
			  lfsr <= (others=>'0');
		  else	 lfsr <= lfsr(12 downto 0) & d0;
     	  		if lfsr = std_logic_vector((TO_UNSIGNED(countmax,14))) then
       				clk_i<=not(clk_i);
				lfsr<=(others=>'0');
			end if;
		 end if;	
        	 	  clk_out<=clk_i;
   	  end if;
   end process;

  
end architecture imp_lfsr_counter;


---------------------------------------------------


So here is a very simple testbench :-

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity test_bench is
---there are no ports to declare so the entity is empty
end test_bench;

architecture behavioural of test_bench is
--call the device under test from the standard 'work' library
	component lfsr_counter_pd
		port  (
     			 rst, clk : in std_logic;
     			clk_out : out std_logic);
	end component;
--declare inputs and initialise them
signal clock_in : std_logic :='0';
signal reset    : std_logic :='0';
--declare outputs and initialise them
signal clock_out : std_logic:='0';
--clock period definition
constant clk_period : time :=10 ns;

begin

	--instantiate the unit under test
uut: lfsr_counter_pd port map (
			rst	=>	reset,
			clk	=>	clock_in,
			clk_out =>	clock_out);

        --generate the clock with 50% duty cycle
        -- this runs for ever it has no sensitivity list
clk_process : process
begin
	clock_in <='0';
	wait for clk_period/2;
	clock_in <='1';
	wait for clk_period/2;
end process;
--generate the stimulus for the unit under test

stim_process :process
begin
	wait for 100 ns;
	reset<='1';
	wait for clk_period * 2;
	reset <='0';
	wait;

end process;



end architecture behavioural;
	

--------------------------------------------------------

Another pass at the code


library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity lfsr_counter_pd is
   port (
      rst, clk : in std_logic;
     clk_out : out std_logic);
end lfsr_counter_pd;

architecture lfsr_counter of lfsr_counter_pd is	
   signal lfsr: std_logic_vector (13 downto 0);	
   signal d0, clk_i: std_logic;	
   constant countmax: integer:=2685;
   constant tpd     :time:=2 ns;
begin

  d0 <= lfsr(13) xnor lfsr(4) xnor lfsr(2) xnor lfsr(0) after tpd;

   process(clk,clk_i,lfsr,rst) begin
	
	  if rising_edge(clk) then
		   if rst='1' then
			  clk_i<='0';
			  lfsr <= (others=>'0') after tpd;
		  else	 lfsr <= lfsr(12 downto 0) & d0 after tpd;
		    if lfsr = std_logic_vector((TO_UNSIGNED(countmax,14))) then
       				clk_i<=not(clk_i)after tpd;
				lfsr<=(others=>'0') after tpd;
			end if;
		 end if;	
        	 	  clk_out<=clk_i;
   	  end if;
   end process;

  
end architecture lfsr_counter;


-----------------------------------------------------

Final pass at code

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity lfsr_counter is
   port (
      rst, clk : in std_logic;
     clk_out : out std_logic);
end lfsr_counter;

architecture lfsr_counter of lfsr_counteris	
   signal lfsr: std_logic_vector (13 downto 0);	
   signal d0, clk_i: std_logic;	
   constant countmax: integer:=2685;
   
begin

  
   process(clk,clk_i,lfsr,rst) begin
	
	  if rising_edge(clk) then
		   if rst='1' then
			  clk_i<='0';
			  lfsr <= (others=>'0') ;
		  else	 lfsr <= lfsr(12 downto 0) & d0;
		    d0 <= lfsr(13) xnor lfsr(4) xnor lfsr(2) xnor lfsr(0);
     	  		if lfsr = std_logic_vector((TO_UNSIGNED(countmax,14))) then
       				clk_i<=not(clk_i);
				lfsr<=(others=>'0') ;
			end if;
		 end if;	
        	 	  clk_out<=clk_i;
   	  end if;
   end process;

  
end architecture lfsr_counter;

----------





-- 
Jan Decaluwe - Resources bvba - http://www.jandecaluwe.com
     Python as a HDL: http://www.myhdl.org
     VHDL development, the modern way: http://www.sigasi.com
     World-class digital design: http://www.easics.com

<snip>
>
> I'm not going to ask that from anybody here - you have helped
> me enough already. Still, I include the code as it was posted
> below, perhaps someone is interested anyway. (The test bench
> as posted contains the design with the delays, and is ready
> to run.)
>
> Thanks,
>
> Jan
>

For giggles I ran the simulation, both are identical (except clock_out 
is delayed by tpd in the second).  The two waveforms he has in the post 
are not on the same "zoom".  The first is zoomed way out where you can 
see clock_out (solid for clock_in).  The second is zoomed way in where 
you can see the individual clock_in.

So, clock_out is probably toggling but he's not centered over an edge to 
actually view it.  If he used a self checking testbench he might have 
caught it (or rather not caught it) instead of relying on manually 
viewing the waveform.

Post should have been titled "Foiled by testbenches"?

Regards,
Chris

On 11/27/2012 07:22 PM, Christopher Felton wrote:

> For giggles I ran the simulation, both are identical (except
> clock_out is delayed by tpd in the second).

Many thanks!

Mm, that holds for the intermediate clock signal clk_i, but
clk_out itself has no delay, correct?

>  The two waveforms he has
> in the post are not on the same "zoom".  The first is zoomed way out
> where you can see clock_out (solid for clock_in).  The second is
> zoomed way in where you can see the individual clock_in.
>
> So, clock_out is probably toggling but he's not centered over an edge
> to actually view it.

Could that be it? He wouldn't have bothered to do a full zoom first?
That would be such silly error that I admit I didn't consider it!

My hypothesis of initially faulty code would be consistent
with what he sees on the fpga.

Heck, perhaps I should just throw the towel. Noone on APP seems
to care.

> If he used a self checking testbench he might
> have caught it (or rather not caught it) instead of relying on
> manually viewing the waveform.

Of course. Actually, sometimes I think that if it's not
self-checking, it shouldn't be called a testbench.

It wouldn't be hard for a case like this (by checking edges,
you can verify whether the generated clock with some
frequence or period spec.)

Jan

-- 
Jan Decaluwe - Resources bvba - http://www.jandecaluwe.com
     Python as a HDL: http://www.myhdl.org
     VHDL development, the modern way: http://www.sigasi.com
     World-class digital design: http://www.easics.com