[SOLVED] FSM Verilog help with output

ads-ee said:

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

Regards

Click to expand...

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 said:

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

Click to expand...

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

Click to expand...

NSG_function    nsg1(x, q, d);

ads-ee said:

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

Click to expand...

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

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.

Click to expand...

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

FvM said:

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?

Code:

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

Click to expand...

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

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

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

ads-ee said:

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:

Code:

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

Click to expand...

rahul.achates said:

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.

Click to expand...

rahul.achates said:

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.

Click to expand...

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
...

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.

Click to expand...

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