Problem with additions and std_logic

[XSterna]

Hi,

I am having a quite strange problem when I am using the addition with
std_logic types.

I am basically using a std_logic_vector containing the address for
accessing a SRAM. After writing an octect in the RAM I am adding 8 to
the address for the next word.

As I did not have the expecting design, I have displayed the content
of the address vector and I have something ... strange.

Here are the values I obtain :

0000 0000
1100 1000
1001 0000
0101 1000
0010 0000
1110 1000
1011 0000
0111 1000
0100 0000
0000 1000
1101 0000
etc

I can see something is quite correct with the 2^0 -> 2^5 bits but only
for a few time and the 2^6 & 2^7 bits are totally wrong.

I thought that it could come from the library I used but after
different tests it does not seem to be the problem. When I try for
exemple a LED_out <= "0000 0000" + 8; everything is correct ...

Here is the reduced portion of the concerning code. I have removed
some signals which does not concern the address mechanism.

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

library IEEE;
use IEEE.STD_LOGIC_1164.all;
use IEEE.STD_LOGIC_arith.all;
use IEEE.STD_LOGIC_UNSIGNED.ALL;


entity RAM_controller is
port (CLK, Reset,flag_data_in: in std_logic;
data_in : in std_logic_vector(7 downto 0);
data_out : out std_logic_vector(7 downto 0);

signal_on : in std_logic; -- 0 : mode transfert - 1 : mode signal
rw : in std_logic; -- 0 : ecriture - 1 : lecture

sram_io : inout std_logic_vector(15 downto 0);
sram_a : out std_logic_vector(17 downto 0);
LED_out : out std_logic_vector(7 downto 0);
);
end RAM_controller;

architecture arch_RAM_controller OF RAM_controller IS
type state_machine is(idle,RAM_read);
signal current_state, next_state : state_machine;
signal I : std_logic_vector(15 downto 0);
signal counter_write,counter_read : unsigned(7 downto 0);
signal counter_write_next,counter_read_next : unsigned(7 downto 0);
signal addr, addr_next : std_logic_vector(17 downto 0);
begin

process(CLK,Reset)
begin
if Reset='1' then
current_state<=idle;
counter_write <= (others => '0');
counter_read <= (others => '0');
count <= (others => '0');
addr <= (others => '0');
elsif (CLK'event and CLK='1') then
current_state<=next_state;
counter_read <= counter_read_next;
counter_write <= counter_write_next;
count <= count_next;
addr <= addr_next;
end if;
end process;

process(current_state,rw,flag_data_in,data_in)
begin
counter_read_next <= counter_read;
counter_write_next <= counter_write;
addr_next <= addr;

case current_state is

when idle=>
LED_out <= addr;
if rw = '0' then -- write
if flag_data_in = '1' then
next_state <= RAM_write;
else
next_state <= idle;
end if;
else
next_state <= idle;
end if;

when RAM_write=>--ecriture
LED_out <= addr;
sram_a <= addr;
sram_io <= "00000000" & data_in;
sram_we <= '0';
addr_next <= addr + 8;
counter_write_next <= counter_write + 1;
next_state <= idle;

end case;
end process;
end arch_RAM_controller;


---------------------------------------------
The flag_data_in is high when a word is transfered by a RS232 link
(the STOP state of the RS232 controller).

If anyone has an idea on this problem it would be great because I am
really clueless on that issue !

Xavier

 

[rickman]

Hi,

I am having a quite strange problem when I am using the addition with
std_logic types.

I am basically using a std_logic_vector containing the address for
accessing a SRAM. After writing an octect in the RAM I am adding 8 to
the address for the next word.

As I did not have the expecting design, I have displayed the content
of the address vector and I have something ... strange.

Here are the values I obtain :

0000 0000
1100 1000
1001 0000
0101 1000
0010 0000
1110 1000
1011 0000
0111 1000
0100 0000
0000 1000
1101 0000
etc

I can see something is quite correct with the 2^0 -> 2^5 bits but only
for a few time and the 2^6 & 2^7 bits are totally wrong.

I thought that it could come from the library I used but after
different tests it does not seem to be the problem. When I try for
exemple a LED_out <= "0000 0000" + 8; everything is correct ...

Here is the reduced portion of the concerning code. I have removed
some signals which does not concern the address mechanism.

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

library IEEE;
use IEEE.STD_LOGIC_1164.all;
use IEEE.STD_LOGIC_arith.all;
use IEEE.STD_LOGIC_UNSIGNED.ALL;

entity RAM_controller is
port (CLK, Reset,flag_data_in: in std_logic;
data_in : in std_logic_vector(7 downto 0);
data_out : out std_logic_vector(7 downto 0);

signal_on : in std_logic; -- 0 : mode transfert - 1 : mode signal
rw : in std_logic; -- 0 : ecriture - 1 : lecture

sram_io : inout std_logic_vector(15 downto 0);
sram_a : out std_logic_vector(17 downto 0);
LED_out : out std_logic_vector(7 downto 0);
);
end RAM_controller;

architecture arch_RAM_controller OF RAM_controller IS
type state_machine is(idle,RAM_read);
signal current_state, next_state : state_machine;
signal I : std_logic_vector(15 downto 0);
signal counter_write,counter_read : unsigned(7 downto 0);
signal counter_write_next,counter_read_next : unsigned(7 downto 0);
signal addr, addr_next : std_logic_vector(17 downto 0);
begin

process(CLK,Reset)
begin
if Reset='1' then
current_state<=idle;
counter_write <= (others => '0');
counter_read <= (others => '0');
count <= (others => '0');
addr <= (others => '0');
elsif (CLK'event and CLK='1') then
current_state<=next_state;
counter_read <= counter_read_next;
counter_write <= counter_write_next;
count <= count_next;
addr <= addr_next;
end if;
end process;

process(current_state,rw,flag_data_in,data_in)
begin
counter_read_next <= counter_read;
counter_write_next <= counter_write;
addr_next <= addr;

case current_state is

when idle=>
LED_out <= addr;
if rw = '0' then -- write
if flag_data_in = '1' then
next_state <= RAM_write;
else
next_state <= idle;
end if;
else
next_state <= idle;
end if;

when RAM_write=>--ecriture
LED_out <= addr;
sram_a <= addr;
sram_io <= "00000000" & data_in;
sram_we <= '0';
addr_next <= addr + 8;
counter_write_next <= counter_write + 1;
next_state <= idle;

end case;
end process;
end arch_RAM_controller;

---------------------------------------------
The flag_data_in is high when a word is transfered by a RS232 link
(the STOP state of the RS232 controller).

If anyone has an idea on this problem it would be great because I am
really clueless on that issue !

Xavier

The code seems overly complicated for what you are trying to do, but
the only thing I can see that might be a problem is the timing of the
two input signals, rw and flag_data_in. A write happens when rw is 0
and flag_data_in is 1. If this combination is asserted for more than
two clock cycles you will get multiple writes. I say two because the
first cycle with this asserted causes the state change and the inputs
are ignored for that cycle. On the third cycle the inputs are checked
again and if asserted a second write happens. This will continue as
long as the inputs are asserted.

Are you running a simulation? Look at the detailed timing of the
address. Does it change more than once rapidly? Or check the value
of counter_write. It will increment by 1 for each write. If that
changes by more than 1, your problem is the enable. Does the write
counter increment by 25 by any chance?

A lot of debugging can be done by the Sherlock Holmes method.
Eliminate whatever is impossible and that which is left must be the
truth. It looks like it is impossible for the increment by 8 to
change the address that way in one clock cycle. So it must be
happening in multiple clock cycles.

Rick

 

[KJ]

Hi,

I am having a quite strange problem when I am using the addition with
std_logic types.

Off to a bad start....std_logic_vectors and addition...oh well.

I am basically using a std_logic_vector containing the address for
accessing a SRAM. After writing an octect in the RAM I am adding 8 to
the address for the next word.

OK.

As I did not have the expecting design, I have displayed the content
of the address vector and I have something ... strange.

Displayed on what?
- Real hardware (possibly hardware issues or timing issues)
- Simulation (none of that nastiness).

Here are the values I obtain :

0000 0000
1100 1000
1001 0000
0101 1000
0010 0000
1110 1000
1011 0000
0111 1000
0100 0000
0000 1000
1101 0000
etc

I can see something is quite correct with the 2^0 -> 2^5 bits but only
for a few time and the 2^6 & 2^7 bits are totally wrong.

A simulation would likely clear this all up...

I thought that it could come from the library I used

Yes, suspect everything else first...then question your own code and design.

but after
different tests it does not seem to be the problem.

Frequently that is the result...library is fine, so it must be the new and
untested, unvalidated design...go figure.

When I try for
exemple a LED_out <= "0000 0000" + 8; everything is correct ...


Here is the reduced portion of the concerning code. I have removed
some signals which does not concern the address mechanism.

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

library IEEE;
use IEEE.STD_LOGIC_1164.all;
use IEEE.STD_LOGIC_arith.all;
use IEEE.STD_LOGIC_UNSIGNED.ALL;

Use ieee.numeric_std instead of std_logic_arith and std_logic_unsigned.
This isn't the cause of your problem, but numeric_std is an actual standard,
the others are not.

entity RAM_controller is
port (CLK, Reset,flag_data_in: in std_logic;
data_in : in std_logic_vector(7 downto 0);
data_out : out std_logic_vector(7 downto 0);

signal_on : in std_logic; -- 0 : mode transfert - 1 : mode signal
rw : in std_logic; -- 0 : ecriture - 1 : lecture

sram_io : inout std_logic_vector(15 downto 0);
sram_a : out std_logic_vector(17 downto 0);
LED_out : out std_logic_vector(7 downto 0);
);
end RAM_controller;

I wonder where the read and write signals are? Oh well, you're only
interested in the address so far.

architecture arch_RAM_controller OF RAM_controller IS
type state_machine is(idle,RAM_read);
signal current_state, next_state : state_machine;
signal I : std_logic_vector(15 downto 0);
signal counter_write,counter_read : unsigned(7 downto 0);
signal counter_write_next,counter_read_next : unsigned(7 downto 0);
signal addr, addr_next : std_logic_vector(17 downto 0);
begin

process(CLK,Reset)
begin
if Reset='1' then
current_state<=idle;
counter_write <= (others => '0');
counter_read <= (others => '0');
count <= (others => '0');
addr <= (others => '0');
elsif (CLK'event and CLK='1') then
current_state<=next_state;
counter_read <= counter_read_next;
counter_write <= counter_write_next;
count <= count_next;
addr <= addr_next;
end if;
end process;

Synchronous process, so far so good.

process(current_state,rw,flag_data_in,data_in)
begin
counter_read_next <= counter_read;
counter_write_next <= counter_write;
addr_next <= addr;

case current_state is

when idle=>
LED_out <= addr;
if rw = '0' then -- write
if flag_data_in = '1' then
next_state <= RAM_write;
else
next_state <= idle;
end if;
else
next_state <= idle;
end if;

when RAM_write=>--ecriture
LED_out <= addr;
sram_a <= addr;
sram_io <= "00000000" & data_in;
sram_we <= '0';
addr_next <= addr + 8;
counter_write_next <= counter_write + 1;
next_state <= idle;

end case;
end process;

Hmmm...you seem to be missing quite a few signals from the sensitivity list.
Quick perusal I see that 'counter_read', 'counter_write', 'addr',
'RAM_write' seem to be missing. This will create a difference between
simulation (if you did this) and synthesis. Synthesis should produce a
warning message to the effect that you have an incomplete sensitivity list
and list all of the above signals and say that it will be assuming that the
others should be added....sounds good, but like I said difference between
sim and actual very bad.

Options:
1. Add the signals and re-simulate
2. Get rid of the two processes and condense it into a single one and
retest. Splitting into a combinatorial process and a separate synchronous
process serves no purpose and creates problems such as:
- The incomplete sensitivity list which creates differences between real
world and simulation.
- Possible creation of latches if there is a signal that doesn't get
assigned for every path through I don't see any instances of this in your
posted code, but you also stripped it down for the posting so who knows.

end arch_RAM_controller;


---------------------------------------------
The flag_data_in is high when a word is transfered by a RS232 link
(the STOP state of the RS232 controller).

If anyone has an idea on this problem it would be great because I am
really clueless on that issue !

Is the input signal 'rw' synchronous to the clk? More generally any input
to the entity most likely needs to be synchronous to clk, but 'rw' is
definitely being used in a way that would require it to be synchronous to
clk.

Have you simulated this?

Timing analysis completed successfully?

KJ

 

[Symon]

If anyone has an idea on this problem it would be great because I am
really clueless on that issue !

Xavier

http://www.synthworks.com/papers/vhdl_math_tricks_mapld_2003.pdf

 

[XSterna]

I have sent an answer yesterday but it does not seem that it worked !
In fact rickman was right, I had a bad design with my flag_data_in
signal which was at the high state for more than one clock cycle. A
really 'basic' problem that shows actually that my design is not
really good, but I'm learning

So my actual problem is solved, but I still have others, now with the
read state ... I think I have to rebuild something cleaner and that is
why the advices you gave me are good for me

http://www.synthworks.com/papers/vhdl_math_tricks_mapld_2003.pdf

In fact before posting here I read a previous message in this
newsgroup where this link was given. I read the part about the
libraries (that's why i talked about it) but when I am using
ieee.numeric_std I experienced some troubles when I run the
Synthesize. The actual definition of libraries is the only one which
seems to work with my code. With the ieee.numeric_std library i have
this error : "+ can not have such operands in this context." for the

addr_next <= addr + "1000";
counter_write_next <= counter_write + 1;

For a counter signal I read I could use an unsigned type but for the
address one I can't since it is a 'physical' signal and the unsigned
type does not work when I synthesize.

Off to a bad start....std_logic_vectors and addition...oh well.

But I don't really get why it's not good to use std_logic_vectors. In
my beginner use, I just saw that I can't use an unsigned as an in or
out signal. But it's not that I don't want to use them, I just don't
know why I should and how

Displayed on what?
- Real hardware (possibly hardware issues or timing issues)
- Simulation (none of that nastiness).

Sorry I forgot an information ! I was displaying it on LEDs to be able
to visualise it. In fact I usually simulate my designs with modelsim
but in that case, since I use a SRAM, I can't really simulate it. Part
of my concern is to know if the SRAM is well "read and written" and a
Modelsim simulation can not give me that.

Yes, suspect everything else first...then question your own code and design.

In my case it was because at first I did not have a library which
understood my '+' operation.

Use ieee.numeric_std instead of std_logic_arith and std_logic_unsigned.
This isn't the cause of your problem, but numeric_std is an actual standard,
the others are not.

As i wrote previously in this message, I am having troubles with the
addr signal which is an 'out' one. With numeric_std I can't do a
addition with std_logic_vector but I need it as an output...

I wonder where the read and write signals are? Oh well, you're only
interested in the address so far.

In fact I wanted to lighten my code to focus on my addr problem. But I
now think for an experienced designer it would be better to have
everything to understand ...
I will give the entire code, but please bear in mind I am beginner

Hmmm...you seem to be missing quite a few signals from the sensitivity list.
Quick perusal I see that 'counter_read', 'counter_write', 'addr',
'RAM_write' seem to be missing. This will create a difference between
simulation (if you did this) and synthesis. Synthesis should produce a
warning message to the effect that you have an incomplete sensitivity list
and list all of the above signals and say that it will be assuming that the
others should be added....sounds good, but like I said difference between
sim and actual very bad.

Yes they are missing ... and this is maybe why the rest of my design
is not working well. I will test it !

Options:
1. Add the signals and re-simulate

When you say simulate do you think about modelsim for example ?
Because I as told before i don't really understand the good of a
simulation for the whole design (because for the adrr i agree i would
have see the problem !) since I can't have the 'real' SRAM signal.

2. Get rid of the two processes and condense it into a single one and
retest. Splitting into a combinatorial process and a separate synchronous
process serves no purpose and creates problems such as:
- The incomplete sensitivity list which creates differences between real
world and simulation.
- Possible creation of latches if there is a signal that doesn't get
assigned for every path through I don't see any instances of this in your
posted code, but you also stripped it down for the posting so who knows.

In fact from the few lectures and books I had about FPGA and VHDL I
always found two different process with the reset and clk separted
from the rest. Do you mean that I should do everything on a same
process with the if rst and if clk at the beginning of it ?

Is the input signal 'rw' synchronous to the clk? More generally any input
to the entity most likely needs to be synchronous to clk, but 'rw' is
definitely being used in a way that would require it to be synchronous to
clk.

rw is a switch so it is not sunchronous to the clk. The Idea of my
design is to load a file to the SRAM. the rw switch is used to "rw =
0" write it to the RAM by RS232 ; "rw = 1" read it back by RS232 to
control to file is well written.

Have you simulated this?

Timing analysis completed successfully?

No I don't have any simulation on this. I am visualising the result of
it with LEDs and with the RS232 communication. It sounds really
artisanal but one again I did not find any other solution to really be
able to test the RAM.

Thanks a lot for your help and your time. If you are curious about my
design here is the entire code :

library IEEE;
use IEEE.STD_LOGIC_1164.all;
--use IEEE.numeric_std.all;
use IEEE.STD_LOGIC_arith.all;
use IEEE.STD_LOGIC_UNSIGNED.ALL;

entity RAM_controller is
port (CLK, Reset,flag_data_in: in std_logic;
data_in : in std_logic_vector(7 downto 0);
data_out : out std_logic_vector(7 downto 0);

signal_on : in std_logic; -- 0 : mode transfert - 1 : mode signal
rw : in std_logic; -- 0 : ecriture - 1 : lecture
Flag_data_out : out std_logic;


sram_io : inout std_logic_vector(15 downto 0);
sram_a : out std_logic_vector(17 downto 0);
LED_out : out std_logic_vector(7 downto 0);
sram_ce : out std_logic;
sram_ub : out std_logic;
sram_lb : out std_logic;
sram_we : out std_logic;
sram_oe : out std_logic
);
end RAM_controller;

architecture arch_RAM_controller OF RAM_controller IS
type state_machine is(idle,RAM_read,RAM_write);
signal current_state, next_state : state_machine;
signal I : std_logic_vector(15 downto 0);
signal counter_write,counter_read : unsigned(7 downto 0);
signal counter_write_next,counter_read_next : unsigned(7 downto 0);
signal addr, addr_next : std_logic_vector(17 downto 0);
begin

process(CLK,Reset)
begin
if Reset='1' then
current_state<=idle;
counter_write <= (others => '0');
counter_read <= (others => '0');
addr <= (others => '0');
elsif (CLK'event and CLK='1') then
current_state<=next_state;
counter_read <= counter_read_next;
counter_write <= counter_write_next;
addr <= addr_next;
end if;
end process;


process(current_state,rw,flag_data_in,data_in,counter_write,counter_read,addr)
begin
counter_read_next <= counter_read;
counter_write_next <= counter_write;
addr_next <= addr;

sram_ce <= '0';
sram_a <= (others => '0');
sram_io <= (others => 'Z');
sram_ce <= '0';
sram_ub <= '1';
sram_lb <= '0';
sram_we <= '1';
sram_oe <= '1';
flag_data_out<='0';
data_out <= (others => '1');
case current_state is

when idle=>
--LED_out <= counter_write(3 downto 0)& counter_read(3 downto 0);
if rw = '0' then -- write
if flag_data_in = '1' then
next_state <= RAM_write;
else
next_state <= idle;
end if;
elsif rw = '1' then
addr_next <= (others => '0');

next_state <= RAM_read;
else
next_state <= idle;
end if;

when RAM_write=>--ecriture
--LED_out <= counter_write(3 downto 0)& counter_read(3 downto 0);
sram_a <= addr;
sram_io <= "00000000" & data_in;
sram_we <= '0';
addr_next <= addr + "1000";
counter_write_next <= counter_write + 1;

next_state <= idle;

when RAM_read=>--lecture
--LED_out <= counter_write(3 downto 0)& counter_read(3 downto 0);
if (counter_read < counter_write) then
sram_a <= addr;
sram_a <= (others => '0');
flag_data_out<='1';
sram_oe <= '0';
I <= sram_io;
data_out <= I(7 downto 0);
addr_next <= addr + 8;
counter_read_next <= counter_read + 1;
next_state <= RAM_read;
else
next_state <= idle;
end if;
end case;
end process;
end arch_RAM_controller;

 

[KJ]

In fact before posting here I read a previous message in this
newsgroup where this link was given. I read the part about the
libraries (that's why i talked about it) but when I am using
ieee.numeric_std I experienced some troubles when I run the
Synthesize. The actual definition of libraries is the only one which
seems to work with my code. With the ieee.numeric_std library i have
this error : "+ can not have such operands in this context." for the

addr_next <= addr + "1000";
counter_write_next <= counter_write + 1;

With addr and addr_next both being std_logic_vectors then you would...
addr_next <= std_logic_vector(unsigned(addr) + 8);

The reason for the error is that "1000" is ambiguous. YOU know that
you mean it to be a binary representation of the number 8, but it is
also the representation that would be used for -8. When adding/
subtracting constants, it is always much clearer to simply use
integers for the constants and not hard code in bit vectors.

For a counter signal I read I could use an unsigned type but for the
address one I can't since it is a 'physical' signal and the unsigned
type does not work when I synthesize.

No, 'addr' is an internal signal to your architecture; 'sram_a' is the
entity output so that needs to be a std_logic_vector. Personally, I
would define 'addr' as

signal addr: unsigned(sram_a'range);

I wouldn't have an 'addr_next' signal at all (because of the upcoming
discussion on two versus one process) but if I did then the
previously mentioned addition would simplify to

addr_next <= addr + 8;

But I don't really get why it's not good to use std_logic_vectors. In
my beginner use, I just saw that I can't use an unsigned as an in or
out signal.

It's not that you can't use unsigned as outputs, it is just that the
common practice is that the top level signals of a design should all
be std_logic/std_logic_vector.

But it's not that I don't want to use them, I just don't
know why I should and how

Why: When you do arithmetic, use unsigned (or signed...sometimes you
do need to signed arithmetic, like if you wanted to count down to -1
for some reason).

How: Simply declare the signal as unsigned (or signed) of the
appropriate width. Signed/unsigned vectors are really nothing more
than an interpretation of a collection of bits as having some numeric
value, whereas std_logic_vector is simply a collection of bits, there
is no inherent numeric value interpretation implied. When you had
trouble adding "1000" you mentally had this idea that "1000" is the
same thing as "8" because you wanted it to have a numeric
interpretation...but a collection of bits does not *have* to be viewed
in that manner, in fact since "1000" would just as easily be
interpreted as "-8" there is a problem.

When you do have some interface requirement for std_logic_vectors, it
does not prevent you from creating an internal signal (like 'addr')
that is of the appropriate type for usage and then when you're all
done add another concurrent statement that casts the signal to the
required type. In your case it would be

sram_a <= std_logic_vector(addr);

This statement converts the unsigned 'addr' into the std_logic_vector
'sram_a'. It costs zero hardware, no additional logic gets generated
to do this since 'unsigned' is simply a particular intepretation of a
collection of bits, and 'std_logic_vector' is simply a different
interpretation of the same collection of bits.

Sorry I forgot an information ! I was displaying it on LEDs to be able
to visualise it. In fact I usually simulate my designs with modelsim
but in that case, since I use a SRAM, I can't really simulate it. Part
of my concern is to know if the SRAM is well "read and written" and a
Modelsim simulation can not give me that.

You can simulate most anything, including the SRAM. There are likely
existing models for the SRAM you're using available on the web. Even
without such a model though, your posting was regarding the address
outputs to the SRAM you wouldn't need an SRAM model to see this.
Simulate.

In my case it was because at first I did not have a library which
understood my '+' operation.

My point was that many blame the tools, libraries, or whatever
first...when they should be questioning their own code first.

Yes they are missing ... and this is maybe why the rest of my design
is not working well. I will test it !

Good.


When you say simulate do you think about modelsim for example ?

Use whatever simulator you prefer.

Because I as told before i don't really understand the good of a
simulation for the whole design (because for the adrr i agree i would
have see the problem !) since I can't have the 'real' SRAM signal.

Sure you can.

In fact from the few lectures and books I had about FPGA and VHDL I
always found two different process with the reset and clk separted
from the rest.

The books and lectures are dispensing poor advice. Using a
combinatorial process and a separate synchronous process as you've
done leads to the possibility of the above mentioned design errors. A
good designer avoids methods that can lead to problems or that creates
extra work, therefore the dispensers of such knowledge would not be
considered good at design (in my opinion). Such books and lectures
though are common, so don't feel that you've been cheated, simply
question them directly if possible about the above two points and ask
how there method could in any way be considered superior to a method
that does not have such drawbacks?

Do you mean that I should do everything on a same
process with the if rst and if clk at the beginning of it ?

I usually use rst synchronously, but some prefer the async reset. So
I would use the following template
process(clk)
begin
if rising_edge(clk) then
if (rst = '1') then
...
else
...
end if;
end if;
end process;

For an async reset I would use the following template

process(clk,rst)
begin
if rising_edge(clk) then
...
end if;

if (rst = '1') then
...
else
...
end if;
end process;

Whether you use rst asynchronously or synchronously, you ALWAYS have
to use a *synchronized* rst signal (i.e. 'rst' wouldn't come from an
external input pin of the device, it would be the output of a flip
flop). The reason is that the trailing edge of 'rst' (i.e. when it
goes from active to inactive) needs to meet setup time requirements
relative to the rising edge of 'clk' otherwise, on the first clock
after 'rst' goes to '0' flip flops in the device could get into states
that you had not intended.

rw is a switch so it is not sunchronous to the clk. The Idea of my
design is to load a file to the SRAM. the rw switch is used to "rw =
0" write it to the RAM by RS232 ; "rw = 1" read it back by RS232 to
control to file is well written.

Switches are known to bounce...for long periods of time. One press of
the switch can likely produce a whole slew of apparent switch
pushes...if you've never seen this it's fun to watch for the first
time...a pain then to have add debounce logic later. So...
1. Synchronize 'rw' to clk before using it for anything.
2. Be prepared to add debounce logic once you discover what switches
reeeeeally do.

No I don't have any simulation on this. I am visualising the result of
it with LEDs and with the RS232 communication. It sounds really
artisanal but one again I did not find any other solution to really be
able to test the RAM.

It's almost always easier to debug in simulation than on real
hardware. If nothing else, when you get to the real hardware you
should have more confidence that the design is functionally sound.
Having said that though, it appears that you also have real world
issues like switch debounce and asynchronous inputs that will be
coming up to bite you...and those do not show up in simulation.

Thanks a lot for your help and your time.

You're welcome.

KJ

 

[Andy]

For an async reset I would use the following template

process(clk,rst)
begin
if rising_edge(clk) then
...
end if;

if (rst = '1') then
...
else
...
end if;
end process;

KJ has given very good advice, except there should not be an "else" in
the reset clause (I'm sure it was a typo). I use that template when I
want to have some storage devices be asynchronously reset, and others
in the same process not be asynchronously reset.

Otherwise, I use the more common async reset template:

process(clk,rst)
... -- declarations go here
begin
if (rst = '1') then
... -- reset assignments go here
elsif rising_edge(clk) then
... -- synchronous assignments go here
end if;
end process;

The only problem is that if you forget to include a register's signal/
variable in the reset assignments, the synthesis will (should) create
a feedback mux to ensure that the unreset register maintains its value
on clock cycles while the async reset is active, which creates timing
problems. Most synthesis tools will warn you that they are creating
the feedback mux, which is a hint that you left a reset assignment
out.

The following (corrected version of KJ's) template avoids that, but
also eliminates the warning if you unintentionally left a reset
assignment out. That's why I don't like using it unless I'm
intentionally leaving something unreset. Use of this template is a
reminder for me to double check the reset assignments.

process(clk,rst)
... -- declarations go here
begin
if rising_edge(clk) then
... -- synchronous assignments go here
end if;

if (rst = '1') then
... -- reset assignments go here
end if;
end process;

Andy

 

[KJ]

KJ has given very good advice, except there should not be an "else" in
the reset clause (I'm sure it was a typo).

Actually it was a copy/paste-o...or more specifically the lack of a
delete-o following the copy/paste rather than a typo...thanks for the
catch.

KJ

 

[XSterna]

Hi,

Thank you for the explanation about std_logic and unsigned. I
understand the "difference" and I will do my best to deal with it in a
better way.

About the switch, to be honnest I knew it but since my design seemed
to be working and I did not find my "old" debounce entity, I did not
used it. But I know I will need one to be sure everything is ok.

About the simulation, I began to build one for the RAM controller
because you are right it will be easier to debug and in fact the
"SRAM" is not really the one which seems to be problematic

Now, the main point I did not really understand is the "all-in-one"
process one. I understand the synchronous and asynchronous scheme in
what you wrote. But in my case I don't really catch how to implement
it.

The separation of the process allows me to manage the next state. I
don't really see how I will be able to manage the incrementation of a
signal for example the management of the addr signal. How to increment
by 8 ? with only one signal i don't see any solution since a <= addr ;
a<= addr + 8 will give me a <= addr +8 if it is in the same process.

Well I think I have to work on that because this is a really new
approach for me

 

[Mike Treseler]

The separation of the process allows me to manage the next state.

I only need to describe the Q side, "state" and it's update procedure.
Synthesis can work out the D side if I let it.

Here's how I pack lots of registers into a single process:
http://mysite.verizon.net/miketreseler/stack.vhd
http://mysite.verizon.net/miketreseler/stack.pdf

Good luck.

-- Mike Treseler

 

[KJ]

Now, the main point I did not really understand is the "all-in-one"
process one. I understand the synchronous and asynchronous scheme in
what you wrote. But in my case I don't really catch how to implement
it.

Below is a snippet of your current code with two processes

process(current_state,rw,flag_data_in,data_in,counter_write,counter_read,addr)
begin
if rw = '0' then -- write
if flag_data_in = '1' then
next_state <= RAM_write;
else
next_state <= idle;
end if;
elsif rw = '1' then
addr_next <= (others => '0');
next_state <= RAM_read;
else
next_state <= idle;
end if;
end process;

process(CLK,Reset)
begin
if Reset='1' then
current_state<=idle;
counter_write <= (others => '0');
counter_read <= (others => '0');
addr <= (others => '0');
elsif (CLK'event and CLK='1') then
current_state<=next_state;
counter_read <= counter_read_next;
counter_write <= counter_write_next;
addr <= addr_next;
end if;
end process;

Next is functionally the same code with one clocked process

process(CLK,Reset)
begin
if Reset='1' then
current_state<=idle;
counter_write <= (others => '0');
counter_read <= (others => '0');
addr <= (others => '0');
elsif (CLK'event and CLK='1') then
if rw = '0' then -- write
if flag_data_in = '1' then
current_state <= RAM_write; -- No need for 'next_state'
else
current_state <= idle;
end if;
elsif rw = '1' then
addr <= (others => '0'); -- No need for 'addr_next' either
current_state <= RAM_read;
else
current_state <= idle;
end if;
end if;
end process;

Basically, all of your *_next signals are not needed, you update the signals
directly. This is just a snip of your code, but should give you the basic
idea.

Except for possible typing errors on my part, both will synthesize to the
exact same thing. The single clocked process has three advantages over the
two process approach:
- Less typing (therefore less room for error)
- Very small chance of getting the sensitivity list wrong which leads to
synthesized results being functionally different than simulation.
- No chance of creating a latch.

KJ

 

[XSterna]

Once again thank you all for your help

I totally agree the use of only one process is much more simple (and
obviously i prefer it !) ; KJ your three points are right and that's
why I really want to re-code my designs like that. Mike your files are
really interesting for me, even it is a little bit to complicate for
my level, and I "learned" something new with your use of procedure
that I never came across before.

So, I am beginning to understand the one-process thing thanks to your
examples and I tried to modify my most simple entity : the modulo m
counter


First of all, not having a "next signal" imply for me to manage the
first iteration as an exception since it is the only case where I have
a signal a 0 and not a "previous + 1".
I don't really like having to manage exception but maybe there are no
others way to do it.

My main concern is that my design is like "one state" behind. Here is
my code :

entity baud_clk is
generic(
Nbits : integer := 8; -- Number of bits require for the counter
M : integer := 217 -- Counter Modulo
);

Port(
clk : in STD_LOGIC; -- Clock in
rst : in STD_LOGIC; -- reset
baud_tick : out STD_LOGIC -- Output tick at the baud frequency
);
end baud_clk;

architecture arch_baud_clk of baud_clk is
signal counter: unsigned(Nbits-1 downto 0);
signal counter_reg: unsigned(Nbits-1 downto 0);
signal first : boolean;
begin

process(clk,rst)
begin
if (rst = '1') then
counter <= (others => '0'); -- reset of the counter
first <= true;
baud_tick <= '0';
elsif rising_edge(clk) then
if counter = (M-1) then
counter <= (others => '0');
first <= true;
baud_tick <= '1';
else
if first = true then
counter <= (others => '0');
first <= false;
else
counter <= counter_reg + 1;
first <= false;
end if;
baud_tick <='0';
end if;
end if;
counter_reg <= counter;
end process;
end arch_baud_clk;

With this design where I would expect to have baud_tick = '1' when
counter = M-1, I have it at the next iteration... So with a M = 10 for
my simulation, I have baud_tick at 1 after a period of counter = 9 ...

I think I did not catch something

 

[rickman]

With all that said about combining processes, there are times when you
would want to pull some logic out of the clocked process and describe
it separately. For example, if logic for the condition of an if
statement is shared in several places, you might want to write that as
a concurrent statement or a non-clocked process. I uses non-clocked
processes when a case or if statement is more clear than a conditional
or selected statement, especially when I need to use both which you
can't do in concurrent statements.

process(CLK,Reset)
begin
if Reset='1' then
state <= idle;
elsif (CLK'event and CLK='1') then
case state is
when FOO =>
if ((stuff = 99) and (flag_data_in = '1') or ((stuff = 99) and
(flag_boolean)... then
state <= RAM_write; -- No need for 'next_state'
end if;
when
... other stuff ...
end case;
end if;
end process;

--- or ---

process (the full sensitivity list, many tools will warn you if it is
incomplete)
condition_1 <= FALSE;
case stuff is
when 13 =>
if (flag_data_in = '1') then
condition_1 <= TRUE;
end if;
when 99 =>
if (flag_boolean ...etc...) then
condition_1 <= TRUE;
end if;
...other stuff...
end case;
end process;

process(CLK,Reset)
begin
if Reset='1' then
state <= idle;
elsif (CLK'event and CLK='1') then
case state is
when FOO =>
if (condition_1) then
state <= RAM_write; -- No need for 'next_state'
end if;
when
... other stuff ...
end case;
end if;
end process;


This can make for a much cleaner and more readable clocked process.
Put the details elsewhere if they are very messy or if they are used
somewhere else.

Rick

 

[Tricky]

entity baud_clk is
        generic(
                Nbits : integer := 8; -- Number of bits require for the counter
                M : integer := 217 -- Counter Modulo
        );

   Port(
                clk : in  STD_LOGIC; -- Clock in
      rst : in  STD_LOGIC; -- reset
      baud_tick : out  STD_LOGIC -- Output tick at the baud frequency
        );
end baud_clk;

architecture arch_baud_clk of baud_clk is
        signal counter: unsigned(Nbits-1 downto 0);
        signal counter_reg: unsigned(Nbits-1 downto 0);
        signal first : boolean;
begin

        process(clk,rst)
        begin
                if (rst = '1') then
                        counter <= (others => '0'); -- reset of the counter
                        first <= true;
                        baud_tick <= '0';
                elsif rising_edge(clk) then
                  if counter = (M-1) then
                    counter <= (others => '0');
                    first <= true;
                    baud_tick <= '1';
                  else
                    if first = true then
                      counter <= (others => '0');
                      first <= false;
                    else
                      counter <= counter_reg + 1;
                      first <= false;
                    end if;
                    baud_tick <='0';
                   end if;
                end if;
                counter_reg <= counter;
        end process;
end arch_baud_clk;

you dont need counter_reg. You currently have it doing nothing. A
synthesiser would probably see it as just a wire from counter, and you
would get the response you desire. BUT, as the process is only
sensitive you clock and reset, counter reg would actually be 1 clock
cycle behind counter. Also, the first tick would occur 217 clock
cycles after power on, and from then on it would be every 218 clock
cycles, as you have the "first" signal holding the counter at 0 for an
extra clock cycle.

I recommend you remove counter_reg, as counter is implicitly
registered because you have it in a clocked process, so use this
instead:

counter <= counter + 1;

also, I dont quite know why you want the "first" signal, so to
condence the process so that the tick occurs every 217 clock cycles, I
think you'd want this instead:

process(clk,rst)
begin
if (rst = '1') then
counter <= (others => '0'); -- reset of the
counter
baud_tick <= '0';
elsif rising_edge(clk) then
if counter = (M-1) then
--this will cause baud_tick to occur every M clock
cycles.
counter <= (others => '0');
baud_tick <= '1';
else
counter <= counter + 1;
baud_tick <='0';
end if;
end process;

 

[XSterna]

This can make for a much cleaner and more readable clocked process.
Put the details elsewhere if they are very messy or if they are used
somewhere else.

Rick

Yes I agree with you. I also feel that only one process could be quite
messy in some cases. I should buy me a notebook to keep tracks of all
those architectures Thank you for the advice !

you dont need counter_reg. You currently have it doing nothing. A
synthesiser would probably see it as just a wire from counter, and you
would get the response you desire. BUT, as the process is only
sensitive you clock and reset, counter reg would actually be 1 clock
cycle behind counter. Also, the first tick would occur 217 clock
cycles after power on, and from then on it would be every 218 clock
cycles, as you have the "first" signal holding the counter at 0 for an
extra clock cycle.

I recommend you remove counter_reg, as counter is implicitly
registered because you have it in a clocked process, so use this
instead:

counter <= counter + 1;

Well i just understood that I could do counter <= counter + 1. I don't
really know from where it comes but I usually had a compiling error
doing so. I guess it is with std_logic type I was broadly using for
everything before this topic !
But someone else also told me today that counter <= counter + 1 works
as soon as it is in a process. So now I understand how my code could
be much lighter !

also, I dont quite know why you want the "first" signal, so to
condence the process so that the tick occurs every 217 clock cycles, I
think you'd want this instead:

You are right again it is totally useless.

process(clk,rst)
begin
if (rst = '1') then
counter <= (others => '0'); -- reset of the
counter
baud_tick <= '0';
elsif rising_edge(clk) then
if counter = (M-1) then
--this will cause baud_tick to occur every M clock
cycles.
counter <= (others => '0');
baud_tick <= '1';
else
counter <= counter + 1;
baud_tick <='0';
end if;
end process;

This is the disadvantage of being a beginner in something, you take 2h
doing a really messy thing that someone do in 10 minutes (just a
guess in a clear way.

So thank you it is exactly the design I would dream to have. I hope I
will be able to clean my old design in the same way with the one
process architecture. But I really feel I learned what I needed to do
so

Thanks a lot to everyone, you have all been really helpfull !

Xavier

 

[rickman]

process(clk,rst)
begin
if (rst = '1') then
counter <= (others => '0'); -- reset of the
counter
baud_tick <= '0';
elsif rising_edge(clk) then
if counter = (M-1) then
--this will cause baud_tick to occur every M clock
cycles.
counter <= (others => '0');
baud_tick <= '1';
else
counter <= counter + 1;
baud_tick <='0';
end if;
end process;

One suggestion. When implementing counters, it is slightly more
efficient to implement them as loadable down counters.

process(clk,rst)
begin
if (rst = '1') then
counter <= (others => '0'); -- reset of the
counter
baud_tick <= '0';
elsif rising_edge(clk) then
if counter = (0) then
--this will cause baud_tick to occur every M clock
cycles.
counter <= M-1;
baud_tick <= '1';
else
counter <= counter - 1;
baud_tick <='0';
end if;
end process;

This is because in most technologies there is a carry chain built in
that can detect when counter is 0. If you are counting up to (M-1)
the synthesizer has to use LUTs to detect the final state if M is not
a power of 2.

This is a small issue, but if you get used to counting down instead of
up, it will help out in designs where space is tight and potentially
run faster.

Rick

 

[Symon]

One suggestion. When implementing counters, it is slightly more
efficient to implement them as loadable down counters.


This is because in most technologies there is a carry chain built in
that can detect when counter is 0. If you are counting up to (M-1)
the synthesizer has to use LUTs to detect the final state if M is not
a power of 2.

Rick

Good point, but remember up counters are fine too! E.g. divide by 200...

if count = 255 then
count <= 56;
else
count <= count + 1;
end if;

Or something like that.


HTH, Syms.

 

[Andy]

Good point, but remember up counters are fine too! E.g. divide by 200...

if count = 255 then
count <= 56;
else
count <= count + 1;
end if;

Or something like that.

HTH, Syms.

Many synthesis tools will not infer the carry bit from the decrement
for a comparison = 0. They will implement an AND function to test each
bit.

However, if you use integers for counters, it is easy to detect
rollovers with the carry bit.

signal counter : integer range 0 to 2**n-1;
....
if counter - 1 < 0 then -- check the carry bit
counter <= start_val;
do_something;
else
counter <= counter - 1; -- reuse same decrementer
end if;

Note that integer operations are always signed, 32 bit. so the result
of the decrement in the conditional expression can in fact be less
than zero. Not to worry, synthesis will figure out which of the 32
signed bits really gets used, and throw away the rest.

You can also check if counter + 1 > 2**n-1 to get the carry bit from
an incrementer.

This code does not work with unsigned vectors, since the result of
decrementing an unsigned is always unsigned, and can never be larger
than the range defined for the vector (2**n-1).

Andy

 

[Mike Treseler]

You can also check if counter + 1 > 2**n-1 to get the carry bit from
an incrementer.

This code does not work with unsigned vectors

In that case, I do something like this:

a_v := a_v + 1;
if a_v(a_v'left) = '1' then -- a carry?
a_v(a_v'left) := '0'; -- clear carry
-- <code enabled by carry goes here>

If the left bit is not otherwise used,
it dissolves into an asynch carry chain bit.

-- Mike Treseler

 

[Marty Ryba]

Many synthesis tools will not infer the carry bit from the decrement
for a comparison = 0. They will implement an AND function to test each
bit.

However, if you use integers for counters, it is easy to detect
rollovers with the carry bit.
signal counter : integer range 0 to 2**n-1;
if counter - 1 < 0 then -- check the carry bit
counter <= start_val;
do_something;
else
counter <= counter - 1; -- reuse same decrementer
end if;
Note that integer operations are always signed, 32 bit. so the result
of the decrement in the conditional expression can in fact be less
than zero. Not to worry, synthesis will figure out which of the 32
signed bits really gets used, and throw away the rest.
Andy

I tried an experiment today with a unit with a 22-bit unsigned counter
(count down). I had initially implemented it as std_logic_vector; and it
turned out initially that I had to stop at one instead of zero. I changed
the logic to a 22-bit unsigned, and changed the load value by one so that
the "stop" point came at cnt_out = 0. Then, I tried it with an integer range
0 to 2*22-1 and the cnt_out -1 test mentioned by Andy.

Using Synplify Pro 8.8 into ISE 9.1, the *unsigned* came out noticeably
smaller (both worked). The *integer* was largest (even bigger than the
initial SLV code). Looking at the RTL view, it seemed like a big and was
still implied in the unsigned case, but I imagine that could be misleading.

I'm going to try a few more and see if the trend continues (at least see if
unsigned works better than SLV, though that difference may have just come
from changing the test from cnt_out = "0000000000000000000001" to cnt_out =
0).

Just one data point...
Marty (a physicist/systems engineer who's rapidly learning VHDL on the fly)