help with bad synchronous description error

D

delgeris

Guest
hello
i am trying to synthesize the following code.i am creating 4 differen
clocks by clock division and then i want to get different led output
according to switches change in my virtex2pro fpga
i am kind of new to vhdl and i get the following error

ERROR:Xst:827 - "C:/Xilinx91i/lab3/lcok.vhd" line 150: Signal LEDS<0
cannot be synthesized, bad synchronous description.

here is my code

entity lock is
Port ( SWITCHES : in STD_LOGIC_VECTOR (3 downto 0);
RESET : in STD_LOGIC;
fpga_clock: in STD_LOGIC;
LEDS : out STD_LOGIC_VECTOR (3 downto 0));
end lock;

architecture Behavioral of lock is

signal cur_state: std_logic_vector(2 downto 0);
signal next_state: std_logic_vector(2 downto 0);
signal state1s,state2s,state10s,state_miso,state_tetarto: std_logic;
signal clk,clk_1s,clk_2s,clk_10s,clk_miso,clk_tetarto,blink1: std_logic;

begin




process(fpga_clock)
variable counter2 : integer range 0 to 100000000;
variable counter1 : integer range 0 to 50000000;
variable counter_miso : integer range 0 to 25000000;
variable counter_tetarto : integer range 0 to 12500000;
-- variable a:integer:=0;


begin
-- if (a<1) then cur_state<="000"; else cur_state<=cur_state; end if;
-- a:=a+1;
if (rising_edge(fpga_clock)) then

if (counter1=50000000) then
if (state1s = '1')then
state1s<='0';
clk_1s<='1';
else
state1s<='1';
clk_1s<='0';
end if;

counter1:=1;
else
counter1 := counter1 +1 ;
end if;


if (counter2=100000000) then
if (state2s = '1')then
state2s<='0';
clk_2s<='1';
else
state2s<='1';
clk_2s<='0';
end if;

counter2:=1;
else
counter2 := counter2 +1 ;
end if;


if (counter_miso=25000000) then
if (state_miso = '1')then
state_miso<='0';
clk_miso<='1';
else
state_miso<='1';
clk_miso<='0';
end if;

counter_miso:=1;
else
counter_miso := counter_miso +1 ;
end if;


if (counter_tetarto=12500000) then
if (state_tetarto = '1')then
state_tetarto<='0';
clk_tetarto<='1';
else
state_tetarto<='1';
clk_tetarto<='0';
end if;

counter_tetarto:=1;
else
counter_tetarto := counter_tetarto +1 ;
end if;
end if;



if (reset = '1') then
cur_state <= "000";
end if;

CASE SWITCHES IS
WHEN "0110" => if (cur_state = "000") then cur_state<="001" ;
end if;
WHEN "1001" => if (cur_state = "001") then cur_state<="010" ;
end if;
WHEN "1010" => if (cur_state = "010") then cur_state<="011" ;
end if;
WHEN "0101" => if (cur_state = "100") then cur_state<="101" ;
end if;
WHEN OTHERS => cur_state<="100";
end case;


if ((cur_state = "101") and (reset = '1')) then
cur_state<= "000";
else cur_state<="100";
end if;

next_state<=cur_state;

end process;

process(next_state)

variable seq:integer:=0;
variable c:integer:=0;

begin

CASE next_state IS
WHEN "000" => if ((clk_2s='1') and (c < 1)) then
LEDS(3 downto 0) <= "0000";
c:=c+1;
else if (clk_1s='1') then
LEDS(2 DOWNTO 0)<= NOT ("000");
LEDS(3)<='1';
else
LEDS(3) <='0';
end if;
end if;
WHEN "001" => if (clk_1s = '1') then
LEDS(1 DOWNTO 0)<= "11";
LEDS(3 downto 2)<="11";
else
LEDS(3 downto 2) <="00";
end if;
WHEN "010" => if (clk_1s = '1') then
LEDS(0)<= '1';
LEDS(3 downto 1)<="111";
else
LEDS(3 downto 1) <="000";
end if;
WHEN "011" => if (clk_miso = '1') then
LEDS(3 downto 0)<="1111";
else
LEDS(3 downto 0) <="0000";
end if;
WHEN "100" => if (rising_edge(clk_tetarto)) then
if (seq=0) then
LEDS<= "0111";
seq:=1;
else if
(seq=1) then
LEDS<= "1011";
seq:=2;
else if
(seq=2) then
LEDS<= "1101";
seq:=3;
else if
(seq=3) then
LEDS<= "1110";
seq:=0;
end if;
end if;
end if;
end if;
end if;
WHEN OTHERS => LEDS<="0101";
end case;


end process;


end Behavioral;

any help would be appreciated!
thank you



---------------------------------------
Posted through http://www.FPGARelated.com
 
[code snipped - it was bad enough once!]

Open the "XST User Guide" (probably file xst.pdf).
It is a fine manual.
Read the chapter entitled "XST Hardware Description Language (HDL) Codin
Techniques".

Have you ever considered a career in golf course maintenance?
;-)


---------------------------------------
Posted through http://www.FPGARelated.com
 
On Oct 7, 6:30 am, "RCIngham"
<robert.ingham@n_o_s_p_a_m.n_o_s_p_a_m.gmail.com> wrote:
Open the "XST User Guide" (probably file xst.pdf).
It is a fine manual.
Read the chapter entitled "XST Hardware Description Language (HDL) Coding
Techniques".

Have you ever considered a career in golf course maintenance?
;-)
Click to expand...
That might be a bit harsh, depending on the OP's background, although
I agree it'll only get worse from here if he's not the manual- and/or
Google-reading sort. The specific question is addressed at
http://www.xilinx.com/support/answers/14047.htm .
 
Hi Delgeris,

The following will give you problems for sure:
WHEN "100" => if (rising_edge(clk_tetarto)) then
if (seq=0) then
LEDS<= "0111";
...

Above this line of code you also assign values to LEDS, but without
the clk_tetarto sync signal. This means, that the synthesis tool will
try to infer flip flops on LEDS, clocked by clk_tetarto and at the
same time do it asynchronously.

I think it would help you alot to read some basic design guides on
synchronous design. The XST guide is quite good.

Good luck!

Regards,
Kim P.



On Thu, 07 Oct 2010 06:42:19 -0500, "delgeris"
<demetrisdelgeris@n_o_s_p_a_m.hotmail.com> wrote:

hello
i am trying to synthesize the following code.i am creating 4 different
clocks by clock division and then i want to get different led outputs
according to switches change in my virtex2pro fpga
i am kind of new to vhdl and i get the following error

ERROR:Xst:827 - "C:/Xilinx91i/lab3/lcok.vhd" line 150: Signal LEDS<0
cannot be synthesized, bad synchronous description.

here is my code

entity lock is
Port ( SWITCHES : in STD_LOGIC_VECTOR (3 downto 0);
RESET : in STD_LOGIC;
fpga_clock: in STD_LOGIC;
LEDS : out STD_LOGIC_VECTOR (3 downto 0));
end lock;

architecture Behavioral of lock is

signal cur_state: std_logic_vector(2 downto 0);
signal next_state: std_logic_vector(2 downto 0);
signal state1s,state2s,state10s,state_miso,state_tetarto: std_logic;
signal clk,clk_1s,clk_2s,clk_10s,clk_miso,clk_tetarto,blink1: std_logic;

begin




process(fpga_clock)
variable counter2 : integer range 0 to 100000000;
variable counter1 : integer range 0 to 50000000;
variable counter_miso : integer range 0 to 25000000;
variable counter_tetarto : integer range 0 to 12500000;
-- variable a:integer:=0;


begin
-- if (a<1) then cur_state<="000"; else cur_state<=cur_state; end if;
-- a:=a+1;
if (rising_edge(fpga_clock)) then

if (counter1=50000000) then
if (state1s = '1')then
state1s<='0';
clk_1s<='1';
else
state1s<='1';
clk_1s<='0';
end if;

counter1:=1;
else
counter1 := counter1 +1 ;
end if;


if (counter2=100000000) then
if (state2s = '1')then
state2s<='0';
clk_2s<='1';
else
state2s<='1';
clk_2s<='0';
end if;

counter2:=1;
else
counter2 := counter2 +1 ;
end if;


if (counter_miso=25000000) then
if (state_miso = '1')then
state_miso<='0';
clk_miso<='1';
else
state_miso<='1';
clk_miso<='0';
end if;

counter_miso:=1;
else
counter_miso := counter_miso +1 ;
end if;


if (counter_tetarto=12500000) then
if (state_tetarto = '1')then
state_tetarto<='0';
clk_tetarto<='1';
else
state_tetarto<='1';
clk_tetarto<='0';
end if;

counter_tetarto:=1;
else
counter_tetarto := counter_tetarto +1 ;
end if;
end if;



if (reset = '1') then
cur_state <= "000";
end if;

CASE SWITCHES IS
WHEN "0110" => if (cur_state = "000") then cur_state<="001" ;
end if;
WHEN "1001" => if (cur_state = "001") then cur_state<="010" ;
end if;
WHEN "1010" => if (cur_state = "010") then cur_state<="011" ;
end if;
WHEN "0101" => if (cur_state = "100") then cur_state<="101" ;
end if;
WHEN OTHERS => cur_state<="100";
end case;


if ((cur_state = "101") and (reset = '1')) then
cur_state<= "000";
else cur_state<="100";
end if;

next_state<=cur_state;

end process;

process(next_state)

variable seq:integer:=0;
variable c:integer:=0;

begin

CASE next_state IS
WHEN "000" => if ((clk_2s='1') and (c < 1)) then
LEDS(3 downto 0) <= "0000";
c:=c+1;
else if (clk_1s='1') then
LEDS(2 DOWNTO 0)<= NOT ("000");
LEDS(3)<='1';
else
LEDS(3) <='0';
end if;
end if;
WHEN "001" => if (clk_1s = '1') then
LEDS(1 DOWNTO 0)<= "11";
LEDS(3 downto 2)<="11";
else
LEDS(3 downto 2) <="00";
end if;
WHEN "010" => if (clk_1s = '1') then
LEDS(0)<= '1';
LEDS(3 downto 1)<="111";
else
LEDS(3 downto 1) <="000";
end if;
WHEN "011" => if (clk_miso = '1') then
LEDS(3 downto 0)<="1111";
else
LEDS(3 downto 0) <="0000";
end if;
WHEN "100" => if (rising_edge(clk_tetarto)) then
if (seq=0) then
LEDS<= "0111";
seq:=1;
else if
(seq=1) then
LEDS<= "1011";
seq:=2;
else if
(seq=2) then
LEDS<= "1101";
seq:=3;
else if
(seq=3) then
LEDS<= "1110";
seq:=0;
end if;
end if;
end if;
end if;
end if;
WHEN OTHERS => LEDS<="0101";
end case;


end process;


end Behavioral;

any help would be appreciated!
thank you



---------------------------------------
Posted through http://www.FPGARelated.com
Click to expand...
 
"delgeris" <demetrisdelgeris@n_o_s_p_a_m.hotmail.com> writes:

Some comments on your code below (I don't normally write at this
length on Usenet, but I'm waiting for something long running to
finish... :)

hello
i am trying to synthesize the following code.i am creating 4 different
clocks by clock division and then i want to get different led outputs
according to switches change in my virtex2pro fpga
i am kind of new to vhdl and i get the following error

ERROR:Xst:827 - "C:/Xilinx91i/lab3/lcok.vhd" line 150: Signal LEDS<0
cannot be synthesized, bad synchronous description.

here is my code

entity lock is
Port ( SWITCHES : in STD_LOGIC_VECTOR (3 downto 0);
RESET : in STD_LOGIC;
fpga_clock: in STD_LOGIC;
LEDS : out STD_LOGIC_VECTOR (3 downto 0));
end lock;

architecture Behavioral of lock is

signal cur_state: std_logic_vector(2 downto 0);
signal next_state: std_logic_vector(2 downto 0);
signal state1s,state2s,state10s,state_miso,state_tetarto: std_logic;
signal clk,clk_1s,clk_2s,clk_10s,clk_miso,clk_tetarto,blink1: std_logic;

begin




process(fpga_clock)
variable counter2 : integer range 0 to 100000000;
variable counter1 : integer range 0 to 50000000;
variable counter_miso : integer range 0 to 25000000;
variable counter_tetarto : integer range 0 to
12500000;
Click to expand...
Interesting ranges - most people would use (2^n)-1 as a maximum value,
unless they have good reason not ot.

But, full marks for using variables (an endless debate on variable vs
signals around these parts :) for things which only need to be local
to this process.

And using integers for counters (not some horribleness like
std_logic_unsigned) - good also.

-- variable a:integer:=0;


begin
-- if (a<1) then cur_state<="000"; else cur_state<=cur_state; end if;
-- a:=a+1;
if (rising_edge(fpga_clock)) then
Click to expand...
Good, using the rising_edge() function.

But don't use brackets on if statements - they make you look like a
C-programmer :)

if (counter1=50000000) then
Click to expand...
Any time you are using a "magic" number more than once, use a constant
to refer to it.

so, when you define the variable above do this:

constant counter1_high : integer := 50000000;
variable counter1 : integer range 0 to counter1_high;

and then use:
if counter1 = counter1_high then.

or alternatively, use a proper subtype:

subtype t_counter1 is integer range 0 to 50000000;
variable counter : t_counter;

then:
if counter1 = t_counter1'high then

Both a re a little more typing (although with Emacs and tab
completion, they don't get in the way much), but it means you can
change the value in *one* place and not have to remember all the other
places you have to update.

if (state1s = '1')then
state1s<='0';
clk_1s<='1';
else
state1s<='1';
clk_1s<='0';
end if;

counter1:=1;
else
counter1 := counter1 +1 ;
end if;
Click to expand...
Try and indent properly - it makes it much easier for us to read the
code. And you in the coming weeks!

Also, try and be consistent in whether you have spaces around <= and
:=. I like having spaces around them. Others don't. But be
consistent.

Emacs VHDL-mode and Sigasi's HDT will reformat your code prettily
without any effort. I use emacs, and when my code starts to look a
mess (about every compile :) I run the beautify function.

if (counter2=100000000) then
Click to expand...
Again, use a constant or suitable subtype.

if (state2s = '1')then
state2s<='0';
clk_2s<='1';
else
state2s<='1';
clk_2s<='0';
end if;

counter2:=1;
else
counter2 := counter2 +1 ;
end if;


if (counter_miso=25000000) then
Click to expand...
And again.

if (state_miso = '1')then
state_miso<='0';
clk_miso<='1';
else
state_miso<='1';
clk_miso<='0';
end if;

counter_miso:=1;
else
counter_miso := counter_miso +1 ;
end if;


if (counter_tetarto=12500000) then
Click to expand...
And again!

if (state_tetarto = '1')then
state_tetarto<='0';
clk_tetarto<='1';
else
state_tetarto<='1';
clk_tetarto<='0';
end if;

counter_tetarto:=1;
else
counter_tetarto := counter_tetarto +1 ;
end if;
end if;



if (reset = '1') then
cur_state <= "000";
Click to expand...
use an enumerated type for your states, so they have useful names
rather than just being numbers. Let the synth figure out a good
encoding for them.

end if;
Click to expand...
And usually the reset bit is done:

if (reset = '1') then
-- do reset stuff
else
-- do the normal stuff
end if;

rather than being stuck in the middle (and without an else to stop the
rest of the code "running" during reset)...

CASE SWITCHES IS
WHEN "0110" => if (cur_state = "000") then cur_state<="001" ;
end if;
Click to expand...
Don't write lots of code on one line it's harder to understand
usually. If you must, then at least make it *all* on one line. Not
and endif; on a 2nd line!

WHEN "1001" => if (cur_state = "001") then cur_state<="010" ;
end if;
WHEN "1010" => if (cur_state = "010") then cur_state<="011" ;
end if;
WHEN "0101" => if (cur_state = "100") then cur_state<="101" ;
end if;
WHEN OTHERS => cur_state<="100";
Click to expand...
The synthesizer will likely ignore this others clause.

end case;
Click to expand...
This is an unusual state machine - usually we do "case cur_state" and
then you'd have "if switches = some_value" inside each of the "when"
sections of the case statement.

if ((cur_state = "101") and (reset = '1')) then
cur_state<= "000";
else cur_state<="100";
end if;
Click to expand...
This seems to set cur_state to "100" all the time that reset is '0' or
cur_state isn't "101" - unless I'm reading wrong?

next_state<=cur_state;
Click to expand...
Normally, "next state" is assigned to in the case statements and then
at the end cur_state <= next_state.

But (and there are many others who'd agree) I'd use a single process
for the state machine, keep it all together. If you google "single
process state machines" you'll find many previous discussions ;)

end process;

process(next_state)

variable seq:integer:=0;
variable c:integer:=0;

begin

snip more badly formatted code
end if;
WHEN "100" => if (rising_edge(clk_tetarto)) then
Click to expand...
And, as someone else pointed out - you can't do this!

This is a process which is sensitive to the change of state - you
can't then make it sensitive to the edge of a clock signal (the
flipflops only have one clock input in the vast majority of FPGAs)

If you do all your code in a single process, you'll find it easier to
avoid doing this!

<snip more badly formatted code>

end process;


end Behavioral;

any help would be appreciated!
Click to expand...
Essential:
* Reformat the code to be indented and laid out consistently
* Use a single process with a single clock and reset. (Make sure that
the reset signal you are using *is* synchronised to the clock).
* Make sure the reset covers everything (you can relax this later on
when you know what the implications are, but for now do this:

process (clk)
begin
if rising_edge(clk) then
if reset = '1' then
--- resetty things
else
-- normal running things
end if;
end if;
end process;

The template outlined in the XST synthesis templates guide looks a lot
like this :)
* Avoid using magic numbers more than once - make use of the language
to define them in one place and then just refer to them elsewhere.
Even if they are only used once, it can help to define them as a
constant which describes how you came to that number, rather than the
number just being there. For example using a constant called
"maxcount" doesn't help, but one called "setup_time" might (depending
on the context!)

Improvement:
* Use an enumerated type for your state

thank you
Click to expand...
You're welcome!

Martin

--
martin.j.thompson@trw.com
TRW Conekt - Consultancy in Engineering, Knowledge and Technology
http://www.conekt.co.uk/capabilities/39-electronic-hardware
 
Martin gave you some excellent advise. I would add the following.

It looks like your state machine's next state logic is coded in the
synchronous process, but the output decoding is in a combinatorial
process. This is fine, if you need combinatorial outputs. However,
combinatorial processes can easily create latches if you don't watch
it very closely. Short answer: if you must use a combinatorial
process, assign every output with a default value right up front in
that process, before and flow-control code (if-then, case-when, etc.).
Since the code will make an assignment to every output on every pass,
there is no "remembering what was the previous value" required, and no
latches get generated.

In your state transition coding, most users code a case statement
using the state, and use if-statements or case statements within each
state description to define what happens in response to inputs while
in that state. You've switched it around by using an outer case
statement to consider the switch inputs, then using subordinate if-
then statements to manage what you do in which states. While this may
work, most people find it easier to mentally manage what's going on in
a state machine using the traditional coding approach.

Also most users do not use separate current_state and next_state
signals in one synchronous process. The way you have done it appears
to insert an additional clock cycle delay from current_state to
next_state, and then you have a combinatorial output process that uses
next_state. But next_state is never used to create current_state (?).
If you omit the additional next_state, and move the output logic to
your synchronous process (and thus avoid any latches), but use
current_state instead of next_state, the clock-cycle timing of your
outputs should not change. It's the difference between combinatorial
outputs derived from a delayed registered state, or registered outputs
derived from a non-delayed registered state: both take the same number
of clocks to generate the outputs from the state, but the latter is
easier to code in the same case statement as the state transition
logic.

Andy
 
On 10/7/2010 4:42 AM, delgeris wrote:
hello
i am trying to synthesize the following code.i am creating 4 different
clocks by clock division and then i want to get different led outputs
Click to expand...
A related example:
http://mysite.ncnetwork.net/reszotzl/count_enable.vhd

-- Mike Treseler
 
You must log in or register to reply here.

Welcome to EDABoard.com

Sponsor

Back
Top