[SOLVED] FSM Verilog help with output

[forast]

I'm suppose to do a finite state machine and I'm still fairly new with verilog so if anyone can look over my code to see why it's not working correctly that'll be great. I have my test bench already and it seems to be working correctly but the outputs are all wrong. It doesn't seem to output where z=1. I'm pretty sure I did something wrong in the FF_function or OG_function with the if else but I'm not sure how I would go about fixing it. Any help is appreciated. :grin:


Also I should mention I get a few warnings but everything still runs, most of the warnings are in FSM module.

module NSG_function
(
        input x,
        input [1:0] q, // current_state,
        output [1:0] d // next_state
);

assign d[1] = ~x&q[0]&q[1] | x&~q[0]&q[1] | x&q[0]&~q[1];
assign d[0] = ~x | ~q[0]&q[1];

endmodule

module FF_function
(
        input clock, reset, next_state,
        output reg current_state
);

always@(posedge clock, posedge reset)
begin
        if (reset == 1)
                current_state <= 3'b000;
        else
                current_state <= next_state;
end
endmodule

module OG_function
(
        input current_state,
        output reg z

);

always@(*)
begin
        if(current_state == 3'b100)
                z = 1;
        else
                z = 0;
end
endmodule

`include "NSG_function.v"
`include "OG_function.v"
`include "FF_function.v"
module FSM
(
        input x, clock, reset,
        output z
);

        wire [2:0] current_state;
        wire [2:0] next_state;

        NSG_function    nsg1(x, q, d);
        OG_function     og1(current_state, z);
        FF_function     ff1(clock, reset, next_state, current_state);

endmodule

 

[ads-ee]

You are missing the bus dimmension on the state for the module ports.

Regards

 

[forast]

You are missing the bus dimmension on the state for the module ports.

Regards

Excuse my lack of knowledge but what do you mean exactly? I'm still trying to understand everything so I'm not sure what you mean. Here are some warnings I got also but not sure what it exactly mean or where to look.

Warning-[IWNF] Implicit wire has no fanin
FSM.v, 13
  Implicit wire 'q' does not have any driver, please make sure this is 
  intended.


Warning-[PCWM-W] Port connection width mismatch
FSM.v, 13
"NSG_function nsg1(x, q, d);"
  The following 1-bit expression is connected to 2-bit port "q" of module 
  "NSG_function", instance "nsg1".
  Expression: q
  	use +lint=PCWM for more details


Warning-[PCWM-W] Port connection width mismatch
FSM.v, 13
"NSG_function nsg1(x, q, d);"
module NSG_function
  The following 1-bit expression is connected to 2-bit port "d" of module 
  "NSG_function", instance "nsg1".
  Expression: d
  	use +lint=PCWM for more details

 

[ads-ee]

In FSM you are not driving the q input to NSG_function. If the comment in NSG_function is correct then the current state should be defined as

---

Code Verilog - [expand]
1
input [2:0] q;

---

You are also haven't connected the output d from NSG_function in FSM.

In FF_function you incorrectly defined the input and outputs for next_state and curremt_state. They should be declared as follows:

---

Code  - [expand]
1
2
input [2:0] next_state;
output [2:0] current_state;

---

You should also instantiate your modules using named port mapping:

---

Code Verilog - [expand]
1
2
3
4
OG_function  og1 (
    .current_state (current_state),
    .z            (z)
  );(

---

This will ensure you have the correct connections without having make sure the order of the ports is correct.

I would also use better names than single character names like d, q, x, and z. If you had to come back and look at this code a couple of months from now do you think you could remember what those signals represented? Descriptive names like current_state and next_state are what you want to name all your signals.

I don't understand why this FSM was broken up into so many files. Unless this was required by the instructor nobody in industry would code an FSM in this manner.

Regards

 

[forast]

In FSM you are not driving the q input to NSG_function. If the comment in NSG_function is correct then the current state should be defined as

---

Code Verilog - [expand]
1
input [2:0] q;

---

You are also haven't connected the output d from NSG_function in FSM.

In FF_function you incorrectly defined the input and outputs for next_state and curremt_state. They should be declared as follows:

---

Code  - [expand]
1
2
input [2:0] next_state;
output [2:0] current_state;

---

You should also instantiate your modules using named port mapping:

---

Code Verilog - [expand]
1
2
3
4
OG_function  og1 (
    .current_state (current_state),
    .z            (z)
  );(

---

This will ensure you have the correct connections without having make sure the order of the ports is correct.

I would also use better names than single character names like d, q, x, and z. If you had to come back and look at this code a couple of months from now do you think you could remember what those signals represented? Descriptive names like current_state and next_state are what you want to name all your signals.

I don't understand why this FSM was broken up into so many files. Unless this was required by the instructor nobody in industry would code an FSM in this manner.

Regards

Thanks, I had it working perfect in one file and it was a lot easier but breaking up complicated things for me since the instructor wanted us to break it up to understand it better. Not sure why since I understood it easier when it was in one file.

 

[ads-ee]

Maybe it's a case of those who can, do. Those who can't, teach.

 

[FvM]

Maybe it's a case of those who can, do. Those who can't, teach.

I agree that the simple FSM isn't a good example to introduce the concept of hierachical design. But nevertheless it can be used to implement it. We don't know what has been explained on the way, if other excercises of module instantiation have been studied before and if named versus ordered port connection has been addressed.

We see in post #1, that module NSG_function is completely unconnected, because none of the connected objects exist.

NSG_function    nsg1(x, q, d);

So obviously you have to go back to the module instantiation chapter in your Verilog lessons. As suggested, you better get rid of ordered port connection method from the start and use named connection only.

 

[forast]

Maybe it's a case of those who can, do. Those who can't, teach.

Maybe, thanks for teaching me about named port mappings, our professor just told us it has to be in order which is a pain sometimes. I fixed everything, no errors but my output still isn't correct, I don't think I'm doing my if else statement correctly under the OG_function. What I had before was a case statement for each state A-E and it was easy to set current_state to the case statement but now that I can't how do I point it to the right state? Since I'm using assign now instead of cases.

This is the exact part I'm talking about, the "2'b11"

I used 3 bit before since I had it set up differently and changed it to 2bit but it's still not working correctly.

begin
        if(q == [B]2'b11[/B])
                z = 1;
        else
                z = 0;
end

I don't think I'm pointing to the state correctly, that's suppose to say if the state is at the 5th state E which is "4" it will make z=1 but I'm not sure if it's even recognizing that it's going to that state, how do I implement this with the assign statement I have? I'm not sure how to do this since the case statement was the only way I know of.


PS: Yes I noticed I had things mixed up since I was changing things around, all the current_state = q and all the next_state = d


I'm not sure if this helps but at time 0 of the test bench "z = z" when I'm pretty sure "z = 0"

 

[FvM]

I fixed everything, no errors but my output still isn't correct, I don't think I'm doing my if else statement correctly under the OG_function.

Seriously speaking, it sounds rather unlikely that you "fixed everything" at once. In so far answering to your latest post without seeing the corrected design isn't but useless guessing.

Secondly, what's been presented as "NSG_function" calculating the next_state is so far from a reasonable FSM description that I didn't yet consider to decode the logic. Is this what your teacher want's you to write?

module NSG_function
(
        input x,
        input [1:0] q, // current_state,
        output [1:0] d // next_state
);

assign d[1] = ~x&q[0]&q[1] | x&~q[0]&q[1] | x&q[0]&~q[1];
assign d[0] = ~x | ~q[0]&q[1];

endmodule

 

[forast]

Seriously speaking, it sounds rather unlikely that you "fixed everything" at once. In so far answering to your latest post without seeing the corrected design isn't but useless guessing.

Secondly, what's been presented as "NSG_function" calculating the next_state is so far from a reasonable FSM description that I didn't yet consider to decode the logic. Is this what your teacher want's you to write?

module NSG_function
(
        input x,
        input [1:0] q, // current_state,
        output [1:0] d // next_state
);

assign d[1] = ~x&q[0]&q[1] | x&~q[0]&q[1] | x&q[0]&~q[1];
assign d[0] = ~x | ~q[0]&q[1];

endmodule

Ok to give you a clearer understanding of what I did before. This was the NSG part of the file, it's suppose to detect the series of 0110.

always@(*)
begin
    casex(current_state)
    A:           if (x == 1)
                            next_state = A;
                  else
                            next_state = B;
    B:           if (x == 1)
                            next_state = C;
                  else
                            next_state = B;
    C:           if(x == 1)
                            next_state = D;
                  else
                            next_state = B;
    D:           if(x == 1)
                            next_state = A;
                  else
                            next_state = E;
    E:           if(x == 1)
                            next_state = C;
                  else
                            next_state = B;
    endcase
end

This is the Output Generator aka OG_function:

always@(*)
begin
         if(current_state == E)
                 z = 1;
         else
                 z = 0;
end

Now my updated code I have just fixes all the errors and warnings I had but I'm still unsure if it works correctly as it should. I'll post it here but I know I'm not doing something correctly:

module OG_function
(
        input [1:0] q,
        output reg z

);

always@(*)
begin
        if(q == 2'b11)
                z = 1;
        else
                z = 0;
end
endmodule

module FF_function
(
        input clock, reset,
        input [1:0] d,
        output reg [1:0] q
);

always@(posedge clock, posedge reset)
begin
        if (reset == 1)
                q <= 2'b00;
        else
                q <= d;
end
endmodule

`include "NSG_function.v"
`include "OG_function.v"
`include "FF_function.v"
module FSM
(
        input x, clock, reset,
        output z,
);

        wire [1:0] q;
        wire [1:0] d;

        NSG_function    nsg1(x, q, d);
        OG_function     og1(q, z);
        FF_function     ff1(clock, reset, d, q);

endmodule

Now as far as the updated NSG_function goes my professor wanted us to use the assign statement. I'm still learning and trying to understand things so I'm not sure how I'd implement this assign into all the other blocks. I highly doubt I'm comparing the states correctly in OG_function and FF_function. I got the assign minimal SOP from the K-map I did which came from the truth table of OG and transition table of NSG. The Finite State Diagram I drew has 4 states A-D and detects '0110' Hopefully this clear things up a bit.

 

[ads-ee]

I looked at your case implementation and the state transitions look correct for various serial bit patterns with embedded 0110 in them. As that FSM look correct why did you reduce the number of state bits to 2 from 3? The original FSM had 5 states, which state did you get rid of?

You really should use names like next_state and current_state instead of q and d. I suggest using something other than single letter signal names, which for code clarity are typically a poor choice for signal names.

I suspect the problems you are having is somewhere in the translation of a 3-bit FSM state to a 2-bit FSM state. As I don't know what your 2-bit FSM looks like I've translated the 3-bit one into an assign statement:

assign next_state = ~x & (current_state==A) ? B : A |
                     x & (current_state==B) ? C : B |
                     x & (current_state==C) ? D : B |
                    ~x & (current_state==D) ? E : A |
                     x & (current_state==E) ? C : B;

This should be equivalent to the original FSM you wrote (which is also undefined for the other 3 values of current_state, you should fix that or verify the FSM will recover if it gets into one of those states). As you can see writing it this way still looks similar to the original FSM. From here you can easily translate it into a combinatorial assign statement based on the values given for the various states. I don't understand the teacher's reasoning behind having their students code an FSM using an assign statement. I think schools should teach Verilog in a practical manner, stressing good design practices and code maintainability. Coding an FSM using assign statements isn't maintainable.


Regards

 

[forast]

I looked at your case implementation and the state transitions look correct for various serial bit patterns with embedded 0110 in them. As that FSM look correct why did you reduce the number of state bits to 2 from 3? The original FSM had 5 states, which state did you get rid of?

You really should use names like next_state and current_state instead of q and d. I suggest using something other than single letter signal names, which for code clarity are typically a poor choice for signal names.

I suspect the problems you are having is somewhere in the translation of a 3-bit FSM state to a 2-bit FSM state. As I don't know what your 2-bit FSM looks like I've translated the 3-bit one into an assign statement:

assign next_state = ~x & (current_state==A) ? B : A |
                     x & (current_state==B) ? C : B |
                     x & (current_state==C) ? D : B |
                    ~x & (current_state==D) ? E : A |
                     x & (current_state==E) ? C : B;

This should be equivalent to the original FSM you wrote (which is also undefined for the other 3 values of current_state, you should fix that or verify the FSM will recover if it gets into one of those states). As you can see writing it this way still looks similar to the original FSM. From here you can easily translate it into a combinatorial assign statement based on the values given for the various states. I don't understand the teacher's reasoning behind having their students code an FSM using an assign statement. I think schools should teach Verilog in a practical manner, stressing good design practices and code maintainability. Coding an FSM using assign statements isn't maintainable.


Regards

The only reason I reduced the state bits from 3 to 2 is because when I drew out the Finite State Diagram there were only 4 states, 4 states = 2 bits but when I coded it in the case statements I ended up needing an extra state in order for it to work how it should. 2 bits only get up to the "011" if I'm correct. I know doing it with the way I had assign works because someone else did it this way but I can figure it out. Your way works also but I'm afraid he'll tell me to fix it since it's not how an assign statement should be to him, it should look like a Minimal SOP expression. My original FSM has 4 states only A-D. The truth table I did also only is 2 bit. Thanks for your help, still trying to get it, if not I think i'll be able to get help during the weekday.

 

[rahul.achates]

why so many module declare for a simple FSM ?

structure way is good , but code should be in readable format. if you have more complecated FSM then it will be difficult to understand as one have to go each door to knock it and get the data from each door and then compile for the output. Instead of that you can declare one module.

sample code is below

module (

input
output
)


always @(posedge clk or negedge reset) begin
if (reset)
cur_state <= 'd0;
else
cur_state <= next_state;
end


next block should be your state movement

always@(*) begin

if ()
next_state <= state1;
else
..
..
end

and next block should be output assignmanet based on input or state or both

always @(posedge clk or negedge reset) begin


if(cur_state == X)
output <= ...


end

endmodule


there are many ways to write FSM and you can pick up any one, one your code should be clean and in readable format.

 

[forast]

why so many module declare for a simple FSM ?

structure way is good , but code should be in readable format. if you have more complecated FSM then it will be difficult to understand as one have to go each door to knock it and get the data from each door and then compile for the output. Instead of that you can declare one module.

sample code is below

module (

input
output
)


always @(posedge clk or negedge reset) begin
if (reset)
cur_state <= 'd0;
else
cur_state <= next_state;
end


next block should be your state movement

always@(*) begin

if ()
next_state <= state1;
else
..
..
end

and next block should be output assignmanet based on input or state or both

always @(posedge clk or negedge reset) begin


if(cur_state == X)
output <= ...


end

endmodule


there are many ways to write FSM and you can pick up any one, one your code should be clean and in readable format.

I'm starting to see a trend here. I had it in one module and it ran perfect. Unfortunately the professor wanted us to separate them in different modules so we would understand it more so now I find myself confused. How you have it set up is pretty much how I had it before. Everyone is saying this is the incorrect way to do it and it should be taught correctly to industry standards.

 

[rahul.achates]

I have been coding FSM this way since 5 years and my code now in top branded gadgets ..working perfectly

- - - Updated - - -

Other disadvantage is , you may do a mistake while integrating large FSM ... you need to connect each signal.

 

[forast]

I have been coding FSM this way since 5 years and my code now in top branded gadgets ..working perfectly

- - - Updated - - -

Other disadvantage is , you may do a mistake while integrating large FSM ... you need to connect each signal.

Oh I'm sure, that way seems way easier to understand at least for me. This is why I'm having issues with my code and cannot get it to work properly. Seems like it's never reaching the state where z = 1. Then again I don't feel like I know what I'm doing so maybe it's because I'm doing something wrong also.

 

[ads-ee]

I was expecting you to convert the assign into a reduced combinatorial circuit. Just substitute the state values into the equations and reduce.
e.g.

If A='000' and B='001' then we will have:

~x & (current_state==A) ? B : A

x  cs[2]  cs[1]  cs[0]  | ns[2]  ns[1]  ns[0]
------------------------+--------------------
0    0      0      0    |   0      0      1
1    0      0      0    |   0      0      0
...

Once you have the truth table you can reduce it.

Off hand I don't see how you can reliably detect without 5 states as it seems to me you need to have at least one state to cycle in when waiting for the start of the sequence x == '0' (after any sequence of 3 or more 0's or 1's). If you try to cycle anywhere else you won't be able to detect the sequence correctly and if you create some other signal to detect the start of the sequence in the 2-bit FSM then you've effectively created another state variable bit to hold "state" information.

I'm starting to see a trend here. I had it in one module and it ran perfect. Unfortunately the professor wanted us to separate them in different modules so we would understand it more so now I find myself confused. How you have it set up is pretty much how I had it before. Everyone is saying this is the incorrect way to do it and it should be taught correctly to industry standards.

You'll pretty much hear the same thing from everyone else on this forum that has worked in industry. I really have to wonder about this professor if they think that separating the register, state transition logic, and the output logic in different files improves comprehension. Honestly I think it muddles comprehension as now you can't look at the bigger picture of how the sections of code interact. If they want to improve comprehension they should suggest you use proper formatting and comments to separate the sections of code so you can see each section as a separate piece of logic.

e.g.

---

Code Verilog - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
//-------------------------------------------
// The FSM state registers
//-------------------------------------------
always @ (posedge clock or posedge reset) begin
  if (reset) begin
    cs <= A;
  end begin
    cs <= ns;
  end
end
 
 
 
//-------------------------------------------
// The state transistion logic
//-------------------------------------------
always @ (*) begin
  case (cs)
    A       : if (x) ns = A;
              else   ns = B;
 
    B       : if (x) ns = C;
              else   ns = B;
 
    C       : if (x) ns = D;
              else   ns = B;
 
    D       : if (x) ns = A;
              else   ns = E;
 
    E       : if (x) ns = C;
              else   ns = B;
 
    default : ns = A;   // force the FSM to a valid state
                        // for all other values to make it
                        // safe.
  endcase
end
 
 
//-------------------------------------------
// The FSM output logic
//-------------------------------------------
always @ (*) begin
  if(cs == E) z = 1;
  else        z = 0;
end

---

Regards

 

[forast]

I agree with it being more difficult to understand but I finally got everything to work correctly. You are right I needed 5 states. I thought I needed 4 with the drawing but I do need 5. Everything works and if you'd like to see then I can show it but I have everything solved.

 

[ads-ee]

According to a previous post you had to make assigns for all the state transitions. Did you do that translation? I've actually never had to do that before (I leave it to the synthesis tool, but I've checked the synth results). Just curious if you finished off the truth table I started or had another way of doing the translation from the case.

Regards

 

[rahul.achates]

I guess this is resolved now .. if it is so , please make as " Resolved "