problems with FSMs

[Gandalf]

http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=524
http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=526

those are my two Finished States Machines.
I think the code it's ok, but i receive some warnings like this:
http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=522

and i have another problem with the simulation with ModelSim (i use ISE
6.3).

can u help me?
what's wrong in my code?

thank u

 

[Ralf Hildebrandt]

Gandalf wrote:

http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=524

1) Don't use:
use IEEE.STD_LOGIC_ARITH.ALL;
use IEEE.STD_LOGIC_UNSIGNED.ALL;

2) Your sensitivity list of the process
stati: process(sc,cmd)
is incomplete. You are reading more signals, that listed here (din).

3) Your process
stati: process(sc,cmd)
is not pure combinational logic. You don't assign a value in every
possible case to sh_reset, sh_ce ... . -> You get latches.

4) Your loops
for j in 0 to 6 loop
sh_ce<='1';
end loop;
are completely useless.

http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=526

Your process

process(clk,reset,cmd_in,din)
variable sc,sf : stato;
begin

cmd<="00000"+cmd_in;

if reset='1' then sc:=s0;
elsif (clk'event and clk='1') then sc:=sf;
else null;
end if;

case sc is
....

is not a synchronous process. Don't place any code after the "end if" ,
that belongs to a if-clause with an 'event inside.

process(reset,clk)
begin
if (reset='1') then
-- do some reset
elsif rising_edge(clk) then
-- do something
end if;
-- don't write anything here
end process;

http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=522

-> Latches, Registers replaced by logic and so on... Read these warnings
and refer them to the reason in your code.

Ralf

 

[Gandalf]

3) Your process
stati: process(sc,cmd)
is not pure combinational logic. You don't assign a value in every
possible case to sh_reset, sh_ce ... . -> You get latches.

i have to set every values to every port in every state?
isn't too "verbose"?

4) Your loops
for j in 0 to 6 loop
sh_ce<='1';
end loop;
are completely useless.

this set the Chip Enable of the shift register to 1 so the shift reg.
continue to work and put out values (the shift register is 8 bit)


i make some changes:
http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=528
http://pastebot.perl.it/cgi-bin/PasteIt.pl?rm=showpaste;id=529

for the first i have those warnings:
WARNING:Xst:647 - Input <din<7:5>> is never used.
WARNING:Xst:737 - Found 5-bit latch for signal <min>.
WARNING:Xst:737 - Found 5-bit latch for signal <max>.
WARNING:Xst:737 - Found 2-bit latch for signal <mplx_cmd>.
WARNING:Xst:737 - Found 5-bit latch for signal <addr_rom_cmd>.

for the second i have those ones:
WARNING:Xst:647 - Input <din<7:5>> is never used.
WARNING:Xst:737 - Found 5-bit latch for signal <count_max>.
WARNING:Xst:737 - Found 5-bit latch for signal <count_min>.
WARNING:Xst:737 - Found 5-bit latch for signal <addr_rom_cmd>.

i know what are latches...but don't understand what the warnings seyed....
maybe it has to use n-bit latches like memories to have some values?
u think my code is ok?

 

[Mike Treseler]

Gandalf wrote:

i have to set every values to every port in every state?
isn't too "verbose"?

I don't like it either.
The alternative is to use a single process.
See the reference design here:
http://home.comcast.net/~mike_treseler/
Another advantage for you is that it is
impossible to infer a latch using this style.

-- Mike Treseler

 

[Ralf Hildebrandt]

Gandalf wrote:

3) Your process
stati: process(sc,cmd)
is not pure combinational logic. You don't assign a value in every
possible case to sh_reset, sh_ce ... . -> You get latches.

i have to set every values to every port in every state?
isn't too "verbose"?

Why is it verbose?
Lets look at a simple latch:

process(en,data)
begin
if (en='1') then
my_latch<=data;
end if;
end process;

In the case of en='0' I don't want, that the signal my_latch changes! It
has to hold its value.
Here in the example I wrote only one if-statement. I can easily add
additional contitions:


process(reset,en1,en2,data)
begin
if (reset='1') then
my_latch<='0';
elsif (en1='1' AND en2='0') then
my_latch<=data;
end if;
end process;

In any case I have modeled something, that holds its value, if these
special conditions are not true.

You can easily avoid latches setting default values:

process(sel)
begin
--defaults - begin
comb1<='0';
comb2<='1';
--defaults - end
case sel is
when 1 => comb1<='1';
when 2 => comb2<='0';
end case;
end process;

So put defaults into the process and you may use deeply nested if- and
case-statements as you wish and you will never get a latch.

4) Your loops
for j in 0 to 6 loop
sh_ce<='1';
end loop;
are completely useless.

this set the Chip Enable of the shift register to 1 so the shift reg.
continue to work and put out values (the shift register is 8 bit)

The loop

for j in 0 to 6 loop
sh_ce<='1';
end loop;

can be replaced by

sh_ce<='1';
sh_ce<='1';
sh_ce<='1';
sh_ce<='1';
sh_ce<='1';
sh_ce<='1';
sh_ce<='1';

which is obviosly the same as a single statement.

sh_ce<='1';

You don't write software - VHDL is not a programming language - you
model hardware.

for the first i have those warnings:
WARNING:Xst:647 - Input <din<7:5>> is never used.

If you really don't need these bits, ignore this warning. Its a warning
- not an error. The tool gives you a hint, that something may be not
correct.

WARNING:Xst:737 - Found 5-bit latch for signal <min>.

If you really want these latches, ignore this warning. Many synthesis
tools and especially FPGA-synthesis tools warn you, if you use latches,
because a synchronous design without latches is less error-prone, can be
automatically checked by timing checks and a scan-path can be added.

I personally use latches whenever possible, but I carefully decide, if I
don't get problems using it.
I guess, your design is a state machine that drives control signals. In
this case often latches do not make sense.

And furthermore be aware of the muxed-latch-problem!

process(en1,en2,data1,data2)
begin
if (en1='1') then
my_latch<=data1;
elsif (en2='1') then
my_latch<=data2;
end if;
end process;

This is a latch with _one_ enable signal (two signals gated by an OR
gate) and with a mux to select data1 or data2. Therefore the
latch-enable and the mux are controlled by the same signals! This may
result in the possible case, that when the enable signal becomes
inactive and the mux changes too, the latch may buffer the _new_ output
of the mux, before the latch is closed! (This depends on the timing of
the gates and signals.)

i know what are latches...but don't understand what the warnings seyed....
maybe it has to use n-bit latches like memories to have some values?

There is no "maybe" - latches and flipflops are storage elements. If you
need to store some values, you need such an element. If you don't need
it, obviously there is something wrong.
=> The warnings are given, because latches are tricky and in most
designs they have to be avoided. So check every single latch, if it is
necessary. (I also check every single flipflop in my designs.)

u think my code is ok?

For a state machine and the derived control signals you don't need
latches. You may need flipflops for the control signals to make them
hazard-free.
Run simulation and then post-synthesis simulation. If there are
differences - the code is obviously "bad".

Ralf