fsm design for sequance detect (please help me)

[ads-ee]

jango123, You can't use a mechanical switch as a synchronous input to something that is clocked. By definition it won't be synchronous as it's not switching based on that clock.

Besides that how is one supposed to determine when a 1101 has been input on the switch?
e.g.
If I hold the switch in the '1' for 10 seconds, hold it in the '0' for 3 seconds, and put it in the '1' for 1 second is that a 1101?
How about '1' for 1 second, '0' for 50 seconds, and '1' for 3 seconds?

Do you see the problem with your switch input design? There is no sense of time to mark one bit from another. That is why the input bit sequence should be from sequential logic running off the same clock as the FSM.

 

[Akanimo]

Hi TrickyDicky,

Some clarification needed here.

Yes - I recommend you avoid latches - using them is poor design practice. And using buffer is generally frowned on, especally when y is not even used as a buffer.
Also because you have a clocked process, using else to hold the state machine in the same state is not required, as this will happen automatically.

Could you please clarify this: "And using buffer is generally frowned on, especally when y is not even used as a buffer."?

What should have been done here: "I recommend you avoid latches - using them is poor design practice."?

 

[ads-ee]

Could you please clarify this: "And using buffer is generally frowned on, especally when y is not even used as a buffer."?

Not sure which version of VHDL changed it, but something like 2005 VHDL changed the behavior of out to allow for reading the out within the code. Prior to that you could use buffer to allow reading of the output port within your code, but that was frowned upon as it results in buffer having to be propagated all over a design as buffer couldn't be connected to an out port. In this particular case you aren't even reading the signal y anywhere in the code, so you should have made it an out.
e.g. out causes an error in earlier versions of VHDL...

port (
  my_out : out std_logic;
end my_entity;
...
if ([COLOR="#FF0000"]my_out[/COLOR] = '1') then -- can't read out port in earlier versions of VHDL, throws an error
...
end if;
my_out <= something;

so some bad coders would use buffer to allow this reading of an output port.

port (
  my_out : [COLOR="#FF0000"]buffer[/COLOR] std_logic;
end my_entity;
...
if ([COLOR="#FF0000"]my_out[/COLOR] = '1') then -- can read my_out but now it can only be connected to other buffer ports.
...
end if;
my_out <= something;

What should have been done here: "I recommend you avoid latches - using them is poor design practice."?

latches are bad in synchronous designs as they are much more difficult to deal with in timing constraints and in STA. You are more likely to get those constraints wrong and have a design that intermittently fails after building a new image.

 

[TrickyDicky]

Not sure which version of VHDL changed it.

VHDL 2008
But even now, reading an out port may confuse VHDL engineers (but not verilog ones).
Plus you'll need tool support (which is mostly there nwo for newer tool versions).

 

[vGoodtimes]

I'm not sure how much blame there is on coders vs the language. There is one pointless restriction that some people solve by being affected by another pointless restriction.

 

[Akanimo]

ads-ee,
Thanks for the corrections, I appreciate.

However, I am still not cleared on this

...
latches are bad in synchronous designs as they are much more difficult to deal with in timing constraints and in STA. You are more likely to get those constraints wrong and have a design that intermittently fails after building a new image.

Do you mean that I should have replaced

...
y <= '1' when (state = s4) and (a = '1') else '0' when rst = '1';
...

with

synchronizing_with_clock : process (clk)
begin
  if rising_edge(clk) then
    if rst = '1' then
      y := '0';
    elsif (a = '1') and (state = s4) then
      y := '1';
    else
      y := y;
    end if;
  end if;
end process;

to imply a flipflop?

 

[ads-ee]

ads-ee,
Thanks for the corrections, I appreciate.

However, I am still not cleared on this


Do you mean that I should have replaced

with

synchronizing_with_clock : process (clk)
begin
  if rising_edge(clk) then
    if rst = '1' then
      y := '0';
    elsif (a = '1') and (state = s4) then
      y := '1';
    else
      y := y;
    end if;
  end if;
end process;

to imply a flipflop?

No I actually was just clarifying what TrickyDicky was mentioning about latches being bad, I'll freely admit I never looked at your code at all when I answered, so I have no idea what latches TrickyDicky was referring to (though the y assignment doesn't look like a latch unless a is somehow based on the y assignment...I'm too lazy to check).

 

[Akanimo]

ads-ee,
Thanks for the corrections, I appreciate.

However, I am still not cleared on this


Do you mean that I should have replaced

...
y <= '1' when (state = s4) and (a = '1') else '0' when rst = '1';
...

with

synchronizing_with_clock : process (clk)
begin
  if rising_edge(clk) then
    if rst = '1' then
      y := '0';
    elsif (a = '1') and (state = s4) then
      y := '1';
    else
      y := y;
    end if;
  end if;
end process;

to imply a flipflop?

What!!??
How I managed to use := for sequential assignments I have no idea. Please forgive my mistake. Sometimes you just get this surprised you did something that wrong. All assignments made above with := should be replaced with <=

TrickyDicky was right. The code inferred a latch. The correction (when you replace :=, which I used wrongly, with <=) infers a register.

That was a good one guys. Thanks.