Hi everyone,
I try to construct this statemachine as described in VHDL below:
(the machine is supposed to set the hold as soon as the trig-signal is
asserted (initialized only when all signals have been low for a
clock-cycle) and go low when both the read and holdoff signals have been
asserted for some time...
------------------------------------------
entity holdoffcontroller is
Port ( clk : in std_logic;
reset : in std_logic;
save : in std_logic;
trig : in std_logic;
read : in std_logic;
holdoff : in std_logic;
hold : out std_logic;
state : out std_logic_vector(4 downto 0));
end holdoffcontroller;
architecture Behavioral of holdoffcontroller is
constant stateStart : std_logic_vector(4 downto 0) := "00001";
constant stateWait : std_logic_vector(4 downto 0) := "00010";
constant stateTrigger : std_logic_vector(4 downto 0) := "00100";
constant stateHold : std_logic_vector(4 downto 0) := "01000";
constant stateRead : std_logic_vector(4 downto 0) := "10000";
begin
STATEMACHINE: block
signal current_state, next_state : std_logic_vector(4 downto 0)
:= stateStart;
begin
stateRegister : process(clk, reset)
begin
if reset = '1' then
current_state <= stateStart;
elsif rising_edge(clk) then
current_state <= next_state;
end if;
end process;
stateTransitions : process(current_state, holdoff, read, trig)
begin
-- stateStart
if current_state(0) = '1' then
hold <= '0';
if holdoff = '0' and read = '0' and trig = '0' then
next_state <= stateWait;
end if;
end if;
-- stateWait
if current_state(1) = '1' then
if trig = '1' then
next_state <= stateTrigger;
end if;
end if;
-- stateTrigger
if current_state(2) = '1' then
hold <= '1';
if holdoff = '1' and read = '0' then
next_state <= stateHold;
end if;
if holdoff = '0' and read = '1' then
next_state <= stateRead;
end if;
if holdoff = '1' and read = '1' then
next_state <= stateStart;
end if;
end if;
-- stateHold
if current_state(3) = '1' then
if read = '1' then
next_state <= stateStart;
end if;
end if;
-- stateRead
if current_state(4) = '1' then
if holdoff = '1' then
next_state <= stateStart;
end if;
end if;
end process;
state <= current_state;
end block;
end Behavioral;
------------------------------------------
Simulating the behavioral model works just fine, but when simulating the
Translate-model the function of the model is just awkward!
When synthesis runs i get these warnings:
WARNING:Xst:647 - Input <save> is never used.
WARNING:Xst:737 - Found 1-bit latch for signal <hold>.
WARNING:Xst:737 - Found 5-bit latch for signal <next_state>.
Found 5-bit register for signal <current_state>.
Furthermore I get these three strange clock-signals:
-----------------------------------+------------------------+-------+
Clock Signal | Clock buffer(FF name) | Load |
-----------------------------------+------------------------+-------+
_n0066(_n006637:O) | NONE(*)(next_state_4) | 4 |
clk | BUFGP | 8 |
current_state_0:Q | NONE | 1 |
-----------------------------------+------------------------+-------+
(*) This 1 clock signal(s) are generated by combinatorial logic,
and XST is not able to identify which are the primary clock signals.
Please use the CLOCK_SIGNAL constraint to specify the clock signal(s)
generated by combinatorial logic.
INFO:Xst:2169 - HDL ADVISOR - Some clock signals were not automatically
buffered by XST with BUFG/BUFR resources. Please use the buffer_type
constraint in order to insert these buffers to the clock signals to help
prevent skew problems.
Can anybody tell me what I do wrong in the design of the statemachine!
Thanks for helping me out!
One-hot statemachine design problems
Started by ●March 19, 2005
Reply by ●March 19, 20052005-03-19
Preben Holm wrote:> Hi everyone,[...] Really a comp.lang.vhdl question, but since you're here... at the beginning of your process (right after the begin), I think you'd want a default assignment for next_state: next_state <= current_state; This will make it obvious to the tools that they should remain in the current state unless one of the following conditions turns out to be true.> stateTransitions : process(current_state, holdoff, read,trig)> begin > -- stateStart > if current_state(0) = '1' then > hold <= '0'; > > if holdoff = '0' and read = '0' and trig = '0' then > next_state <= stateWait; > end if; > end if; >[...]> WARNING:Xst:737 - Found 5-bit latch for signal <next_state>. > Found 5-bit register for signal <current_state>.The latch warning for next_state is your clue. Good luck, Marc
Reply by ●March 19, 20052005-03-19
Hi, I would make a few of suggestions : * use an enumeration and traditional "case state is when =>" construct. This facilitates a lot of things, if only for the synthesis tool to properly recognize your FSM, but also for HSL simulation, readability, reliability, optimization etc... * to output the state vector, use a simple decoder (which will be probably eliminated by synthesis). And maybe you don't want the one-hot vector vector to come out, but an binary (more compact) version ? * Single-process style is easier to write (imo) and not prone to the usual errors in combinational processes. (you're not the first and not the last to get caught) You can even write Moore style in one process if you don't want to move your actions in the transitions. * Keep in mind that the synthesizer sometimes can guess the "parallel case" and sometime can't. This is likely your case (can't guess). It should have trouble understanding that individual bit comparisons are (probably) mutually exclusive. So I'm afraid it would code the priority you've told him implicitly to implement (last "if" does win). I think this is inefficient synthesis-wise. On my Website, you'll find many FSM examples, FWIW http://www.alse-fr.com/English/ips.html I never really liked the "textbook" two-processes style, and I've seen lots of problems in code written this way... But Text editors, FSM coding and VHDL vs Verilog are almost religious issues :-) ... Bert Preben Holm wrote:> Hi everyone, > > I try to construct this statemachine as described in VHDL below: > (the machine is supposed to set the hold as soon as the trig-signal is > asserted (initialized only when all signals have been low for a > clock-cycle) and go low when both the read and holdoff signals have been > asserted for some time... > > ------------------------------------------ > entity holdoffcontroller is > Port ( clk : in std_logic; > reset : in std_logic; > save : in std_logic; > trig : in std_logic; > read : in std_logic; > holdoff : in std_logic; > hold : out std_logic; > state : out std_logic_vector(4 downto 0)); > end holdoffcontroller; > > architecture Behavioral of holdoffcontroller is > constant stateStart : std_logic_vector(4 downto 0) := "00001"; > constant stateWait : std_logic_vector(4 downto 0) := "00010"; > constant stateTrigger : std_logic_vector(4 downto 0) := "00100"; > constant stateHold : std_logic_vector(4 downto 0) := "01000"; > constant stateRead : std_logic_vector(4 downto 0) := "10000"; > begin > > STATEMACHINE: block > signal current_state, next_state : std_logic_vector(4 downto 0) > := stateStart; > begin > stateRegister : process(clk, reset) > begin > if reset = '1' then > current_state <= stateStart; > elsif rising_edge(clk) then > current_state <= next_state; > end if; > end process; > > > stateTransitions : process(current_state, holdoff, read, trig) > begin > -- stateStart > if current_state(0) = '1' then > hold <= '0'; > > if holdoff = '0' and read = '0' and trig = '0' then > next_state <= stateWait; > end if; > end if; > etc..
Reply by ●March 19, 20052005-03-19
info_ wrote:> Hi, > > I would make a few of suggestions : > > * use an enumeration and traditional "case state is when =>" > construct. This facilitates a lot of things, if only for the > synthesis tool to properly recognize your FSM, but also for > HSL simulation, readability, reliability, optimization etc...I thought the one-hot approach was quite better for performance issues?> * to output the state vector, use a simple decoder (which > will be probably eliminated by synthesis). And maybe you don't > want the one-hot vector vector to come out, but an binary > (more compact) version ?Well, you're just right about that, but for simulation purposes this works just fine! I actually only need three bit-outputs!> * Single-process style is easier to write (imo) and not prone > to the usual errors in combinational processes. > (you're not the first and not the last to get caught) > You can even write Moore style in one process if > you don't want to move your actions in the transitions.Well, I tried doing this, but this gave me a lot of trouble so I took the "VHDL made easy" book and did the "cooking book"-version of a one-hot state machine design.> On my Website, you'll find many FSM examples, FWIW > http://www.alse-fr.com/English/ips.htmlThanks, i'll take a look!> I never really liked the "textbook" two-processes style, > and I've seen lots of problems in code written this way...Maybe, Xilinx recommends this too, in their "templates".
Reply by ●March 19, 20052005-03-19
Preben Holm wrote:> Hi everyone, > > I try to construct this statemachine as described in VHDL below: > (the machine is supposed to set the hold as soon as the trig-signal is > asserted (initialized only when all signals have been low for a > clock-cycle) and go low when both the read and holdoff signals have been > asserted for some time... >You made the typical mistake --- You didn't clock the inputs. Add one or even two Flip-Flops to every input. vax, 9000
Reply by ●March 19, 20052005-03-19
What is this for? If you are a student and this is part of an assignment I don't want to just give you the answer but that doesn't mean I won't try to help you. Are there any design requirements that need to be followed or met? Style and presentation is very important with VHDL. WIth a programming language like 'C' you can get away with creating compound and nested if statements with different structures and with an optimizing compiler end up with the same result. Early when I was learning VHDL, I noticed that depending on how you structure your code effects the circuit design. In VHDL the compiler tries to implement designs it recognizes ie. state machines. As, someone else pointed out, the way you went about implementing your state machine isn't the expected way. Try implementng using case structure instead of if control structure. Don't forget the differences between case and if statements - if statements are evaluated sequential and case states are evaluated parallel. In VHDL as in C, if you have a large chunk of code and are having trouble with it you should break it up into smaller chunks. Use block and process structures to make your code smaller. It should make your code easier to read and make the compilers job easier. Derek
Reply by ●March 19, 20052005-03-19
Preben wrote:> info_ wrote: > >> Hi, >> >> I would make a few of suggestions : >> >> * use an enumeration and traditional "case state is when =>" >> construct. This facilitates a lot of things, if only for the >> synthesis tool to properly recognize your FSM, but also for >> HSL simulation, readability, reliability, optimization etc... > > > I thought the one-hot approach was quite better for performance issues?Quite often it's the case, but using an enumeration does NOT mean at all giving up one-hot encoding !!! Quite the contrary.> > >> * to output the state vector, use a simple decoder (which >> will be probably eliminated by synthesis). And maybe you don't >> want the one-hot vector vector to come out, but an binary >> (more compact) version ? > > > Well, you're just right about that, but for simulation purposes this > works just fine! > I actually only need three bit-outputs!For simulation, isn't it nicer to see "Writing" in the waveform viewer rather than "00100" ?> >> * Single-process style is easier to write (imo) and not prone >> to the usual errors in combinational processes. >> (you're not the first and not the last to get caught) >> You can even write Moore style in one process if >> you don't want to move your actions in the transitions. > > > Well, I tried doing this, but this gave me a lot of trouble so I took > the "VHDL made easy" book and did the "cooking book"-version of a > one-hot state machine design.Dave wrote this book a long long time ago, and it was real nice at that time. Today, the synthesizers are very much smarter, so it's better to adopt more modern coding styles which are now well supported by all synthesis tools. Single process FSM usually means resynchronized Mealy style as I call it, meaning you have to move the state actions up in the arriving transitions. (But there is way to code Moore actions in a single process FSM). That's another way to desoign, but it's often even easier than Moore style. See my Quadrature decoder as an example.> Maybe, Xilinx recommends this too, in their "templates".I don't want to sound controversial here, but I wouldn't trust Xilinx as a model of well-written HDL code ! They still deliver this bad code with ISE 6.3.03i (despite my fixing this bad code more than 3 years ago). It's irritating. You will appreciate the inout (bidirectional buffer) for the LEDs, the absence of resynchronization for the async inputs, the initialization of signals, the absence of reset, the hard tabs in the source file, etc ! (and I won't comment the test bench) Maybe I misread and it was an example of things NOT to do :-) There are more experts than good code hanging around... ---------cut & paste from ISE 6.3.03i J2C_vhd Example ---------------------- library IEEE; use IEEE.std_logic_1164.all; -- defines std_logic types entity jc2_top is port ( LEFT : in STD_LOGIC; -- Active-low switch #3 (left) RIGHT : in STD_LOGIC; -- Active-low switch #0 (right) STOP : in STD_LOGIC; -- Active-low switch #2 CLK : in STD_LOGIC; Q : inout STD_LOGIC_VECTOR (3 downto 0) := "0000" -- Active-low LEDs ); end jc2_top; architecture jc2_top_arch of jc2_top is signal DIR: STD_LOGIC := '0'; -- Left=1, Right=0 signal RUN: STD_LOGIC := '0'; begin process (CLK) begin if (CLK'event and CLK='1') then -- CLK rising edge -- DIR register: if (RIGHT='0') then DIR <= '0'; elsif (LEFT='0') then DIR <= '1'; end if; -- RUN register: if (STOP='0') then RUN <= '0'; elsif (LEFT='0' or RIGHT='0') then RUN <= '1'; end if; -- Counter section: if (RUN='1') then if (DIR='1') then Q(3 downto 1) <= Q(2 downto 0); -- Shift lower bits (Left Shift) Q(0) <= not Q(3); -- Circulate inverted MSB to LSB else Q(2 downto 0) <= Q(3 downto 1); -- Shift upper bits (Right Shift) Q(3) <= not Q(0); -- Circulate inverted LSB to MSB end if; end if; end if; end process; end jc2_top_arch; ---------------------------------------
Reply by ●March 20, 20052005-03-20
DerekSimmons@FrontierNet.net wrote:> What is this for? If you are a student and this is part of an > assignment I don't want to just give you the answer but that doesn't > mean I won't try to help you.I am ofcourse a student, but it's quite some time ago that I learned about statemachine design (two years actually). Right now I'm trying to do my bachelor, where I deciden to use VHDL and the Spartan III for my project. I am supposed to build the digital sampling part of a digital storage oscilloscope. But all the theory and implementation which I haven't done a lot of work of is being my trouble. So it's not an assignment, more a tiny part of my own project.> Are there any design requirements that need to be followed or met?Well, as fast as possible (aiming for 200MHz maximum speed)!> Style and presentation is very important with VHDL. WIth a programming > language like 'C' you can get away with creating compound and nested if > statements with different structures and with an optimizing compiler > end up with the same result.Well, if my litterature is outdated I'm very happy if someone could give me advice for some more updated litterature.> Early when I was learning VHDL, I noticed that depending on how you > structure your code effects the circuit design. In VHDL the compiler > tries to implement designs it recognizes ie. state machines.Both the single-process, the xilinx-template, and the one-hot approach I tried to build did the job as being recognized as statemachines (FSM's).> As, someone else pointed out, the way you went about implementing your > state machine isn't the expected way. Try implementng using case > structure instead of if control structure. Don't forget the differences > between case and if statements - if statements are evaluated sequential > and case states are evaluated parallel.I'm aware of that if-statements are "sequential" (the last if-statement has the "power"), but in hardware they are still quite parallel (even though more if's costs more time in some cases).> In VHDL as in C, if you have a large chunk of code and are having > trouble with it you should break it up into smaller chunks. Use block > and process structures to make your code smaller. It should make your > code easier to read and make the compilers job easier.Yeah, but I'm not able to break a state machine into smaller pieces, and this is even though quite simple! How do you like my new design (single-process) ---------------------------------------------- entity holdoffcontroller is Port ( clk : in std_logic; reset : in std_logic; save : in std_logic; trig : in std_logic; read : in std_logic; holdoff : in std_logic; hold : out std_logic := '0'; state : out std_logic_vector(4 downto 0)); end holdoffcontroller; architecture Behavioral of holdoffcontroller is type states is (stateStart, stateWait, stateTrigger, stateHold, stateRead); begin STATEMACHINE: block signal current_state : states := stateStart; begin stateRegister : process(clk, reset) begin if reset = '1' then current_state <= stateStart; elsif rising_edge(clk) then current_state <= current_state; case current_state is when stateStart => --holdoff_counter_enable <= '0'; --holdoff_counter_reset <= '1'; hold <= '0'; if holdoff = '0' and read = '0' and trig = '0' then current_state <= stateWait; end if; when stateWait => if trig = '1' then current_state <= stateTrigger; end if; when stateTrigger => --holdoff_counter_enable <= '1'; hold <= '1'; if holdoff = '1' and read = '0' then current_state <= stateHold; end if; if holdoff = '0' and read = '1' then current_state <= stateRead; end if; if holdoff = '1' and read = '1' then current_state <= stateStart; end if; when stateHold => --holdoff_counter_enable <= '0'; if read = '1' then current_state <= stateStart; end if; when stateRead => if holdoff = '1' then current_state <= stateStart; end if; end case; end if; end process; process(current_state) begin case current_state is when stateStart => state <= "00001"; when stateWait => state <= "00010"; when stateTrigger => state <= "00100"; when stateHold => state <= "01000"; when stateRead => state <= "10000"; end case; end process; end block; end Behavioral; ----------------------------------------------
Reply by ●March 20, 20052005-03-20
Preben Holm wrote:> > Yeah, but I'm not able to break a state machine into smaller pieces, and > this is even though quite simple!There, I agree with you entirely ! Making many separate synchronous blocks "talk" to each other is not a good idea (it does waste usually precious clock cycles). I've seen people write a counter outside the one-process FSM which used and controlled it : a mess ! My recommendation = if you do it in one-process : don't split it.> How do you like my new design (single-process) > ----------------------------------------------Much better ! Just a few comments> entity holdoffcontroller is > Port ( clk : in std_logic; > reset : in std_logic; > save : in std_logic; > trig : in std_logic; > read : in std_logic; > holdoff : in std_logic; > hold : out std_logic := '0';-- I wouldn't initialize the output in the port. -- nice trick for optional inputs though.> state : out std_logic_vector(4 downto 0)); > end holdoffcontroller; > > architecture Behavioral of holdoffcontroller is-- I would rather use RTL rather than "behavioral" (cosmetic)> type states is (stateStart, stateWait, stateTrigger, stateHold, > stateRead); > begin > > STATEMACHINE: block > signal current_state : states := stateStart;No. It's "bad" enough this initialization does exist in VHDL without you writing it.... The issue of integer range & enums is that they are "automatically" initialized (at simulation) with the leftmost value, so you won't notice they may not be in hardware (synthesis). But since you've coded the async reset (almost) correctly, it's not an issue. Just don't initialize signals at declaration in RTL code.> begin > stateRegister : process(clk, reset) > begin > if reset = '1' then > current_state <= stateStart;Bad bad ! you forgot to reset all the FlipFlops you've created below ! (hold in this case, but any other signal on left side of an assignment should be reset also). This is bad because reset will show up in the datapath as an enable :-(> elsif rising_edge(clk) then > current_state <= current_state;Don't write this line (assign to self). It's useless. The 2-process version "next_state <= current_state" was necessary to code the "implied else" and avoid latches, but you don't need this in the one-process style.> > case current_state isbtw, you don't need the "current_" prefix anymore, used when you have two different signals, currunt_state (FF otput) and next_state (FF input). That's just cosmetic.> when stateStart => > --holdoff_counter_enable <= '0'; > --holdoff_counter_reset <= '1'; > etc....../...> > process(current_state) > beginstate <= (others=>'-'); -- play it safe !!!> case current_state is > when stateStart => state <= "00001"; > when stateWait => state <= "00010"; > when stateTrigger => state <= "00100"; > when stateHold => state <= "01000"; > when stateRead => state <= "10000"; > end case; > end process; > end block;I would at least make a default assignment on top to (others=>'-') as shown above, in the hope that the synthesis tool will create an optimized one-hot decoder in any (encoding method) case and will never create latches. It's still a VERY good idea to check what synthesis will do (should be just wires here). Last comment : I would use block / endblock only to restrict the scope of some signals. Since the state information is global (it even goes up in the hierarchy), then the block section (imho) is not useful here. Hope this help, Bert Cuzeau
Reply by ●March 22, 20052005-03-22
> Much better !Thanks>> entity holdoffcontroller is >> Port ( clk : in std_logic; >> reset : in std_logic; >> save : in std_logic; >> trig : in std_logic; >> read : in std_logic; >> holdoff : in std_logic; >> hold : out std_logic := '0'; > > -- I wouldn't initialize the output in the port. > -- nice trick for optional inputs though.Done! But why should I?> -- I would rather use RTL rather than "behavioral" (cosmetic)RTL meaning?>> STATEMACHINE: block >> signal current_state : states := stateStart; > > No. It's "bad" enough this initialization does exist in VHDL > without you writing it.... > The issue of integer range & enums is that they are "automatically" > initialized (at simulation) with the leftmost value, so you won't > notice they may not be in hardware (synthesis).So the initialization won't be like this in the real hardware - or did I misunderstand you there?> But since you've coded the async reset (almost) correctly, > it's not an issue. > > Just don't initialize signals at declaration in RTL code. > >> begin >> stateRegister : process(clk, reset) >> begin >> if reset = '1' then >> current_state <= stateStart; > > Bad bad ! > you forgot to reset all the FlipFlops you've created below ! > (hold in this case, but any other signal on left side of an > assignment should be reset also). > This is bad because reset will show up in the datapath as an enable :-(Why will reset show up as enable?> Don't write this line (assign to self). It's useless. > > The 2-process version "next_state <= current_state" was necessary > to code the "implied else" and avoid latches, but you don't need > this in the one-process style.Well, I'm getting latches for some of the signals I start to assign?>> process(current_state) >> begin > > > state <= (others=>'-'); -- play it safe !!! > >> case current_state is >> when stateStart => state <= "00001"; >> when stateWait => state <= "00010"; >> when stateTrigger => state <= "00100"; >> when stateHold => state <= "01000"; >> when stateRead => state <= "10000"; >> end case; >> end process; >> end block; > > > I would at least make a default assignment on top to (others=>'-') > as shown above, in the hope that the synthesis tool will create > an optimized one-hot decoder in any (encoding method) case and > will never create latches.Okay! I didn't know that!> It's still a VERY good idea to check what synthesis will do (should > be just wires here). > > Last comment : I would use block / endblock only to restrict > the scope of some signals. Since the state information is global > (it even goes up in the hierarchy), then the block section > (imho) is not useful here.Yeah, I just didn't remove it! The total new and even more perfected statemachine: ----- entity holdoffcontroller is Port ( clk : in std_logic; reset : in std_logic; prescale : in std_logic; -- asserted everytime data-sample should be saved trig : in std_logic; read : in std_logic; holdoff : in std_logic; holdoff_enable : out std_logic; holdoff_reset : out std_logic; hold : out std_logic); end holdoffcontroller; architecture Behavioral of holdoffcontroller is type states is (stateStart, stateWait, stateTrigger, stateHold, stateRead); signal holdoff_enable_i : std_logic; signal holdoff_reset_i : std_logic; signal current_state : states; begin stateRegister : process(clk, reset) begin if reset = '1' then current_state <= stateStart; holdoff_enable_i <= '0'; holdoff_reset_i <= '1'; hold <= '0'; elsif rising_edge(clk) then case current_state is when stateStart => holdoff_enable_i <= '0'; holdoff_reset_i <= '1'; hold <= '0'; if holdoff = '0' and read = '0' and trig = '0' then current_state <= stateWait; holdoff_enable_i <= '0'; holdoff_reset_i <= '1'; hold <= '0'; end if; when stateWait => if trig = '1' then current_state <= stateTrigger; holdoff_enable_i <= '1'; holdoff_reset_i <= '0'; hold <= '1'; end if; when stateTrigger => holdoff_enable_i <= '1'; holdoff_reset_i <= '0'; hold <= '1'; if holdoff = '1' and read = '0' then current_state <= stateHold; holdoff_enable_i <= '0'; holdoff_reset_i <= '0'; hold <= '1'; end if; if holdoff = '0' and read = '1' then current_state <= stateRead; holdoff_enable_i <= '1'; holdoff_reset_i <= '0'; hold <= '1'; end if; if holdoff = '1' and read = '1' then current_state <= stateStart; holdoff_enable_i <= '0'; holdoff_reset_i <= '1'; hold <= '0'; end if; when stateHold => holdoff_enable_i <= '0'; holdoff_reset_i <= '0'; hold <= '1'; if read = '1' then current_state <= stateStart; holdoff_enable_i <= '0'; holdoff_reset_i <= '1'; hold <= '0'; end if; when stateRead => holdoff_enable_i <= '1'; holdoff_reset_i <= '0'; hold <= '1'; if holdoff = '1' then current_state <= stateStart; holdoff_enable_i <= '0'; holdoff_reset_i <= '1'; hold <= '0'; end if; end case; end if; end process; holdoff_enable <= holdoff_enable_i and prescale; holdoff_reset <= holdoff_reset_i and prescale; end Behavioral; ----- Is this new mealy-like approach correctly done? Thanks for helping. I'm really learning something from this! /Preben Holm
