Verilog woes

.kbf?

I'm trying to use the module counter in Traffic_Light

I keep getting this error

Error (10159): Verilog HDL Task Definition error at TC.v(29): task "counter" is not used as a task (there are 4 of these errors for each instance of me trying to call counter)

I'm sure I'm missing something simple but I can't for the life of me figure out what.

Also I'm guessing that

if (counter_out >= 3)
done = 1;
is wrong? I think I need 011 instead of 3 but I'm not sure how to code that.

Any Verilog gurus out there that could lend a hand?


Spoilered for large

module Traffic_Light
	(
		Clk,
		Reset,
		carSensor,
		HG, HY, HR, FG, FY, FR
	);
	
	input Clk;
	input Reset;
	input carSensor;
	output HG, HY, HR, FG, FY, FR;
	reg HG, HY, HR, FG, FY, FR;
	
	wire countDone;

	parameter highwayGreen = 0, highwayYellow = 1, highwayRed = 2, farmYellow = 3;

	reg [3:0] state;

	always @ (posedge Clk or posedge Reset)
	begin
		if (Reset)
			state <= highwayGreen;
		else
			case (state)
				highwayGreen:
					if (carSensor == 1) begin
						counter(Clk, 1, 0, countDone);
						state <= highwayYellow;
					end else
						state <= highwayGreen;
						
				highwayYellow:
					if (countDone == 1)
						state <= highwayRed;
		
				highwayRed:
					if (carSensor == 0) begin
						counter(Clk, 1, 0, countDone);
						state <= farmYellow;
					end else
						state <= highwayRed;
						
				farmYellow:
					if (countDone == 1)
						state <= highwayGreen;
					
			endcase
	end

	always @ (state)
	begin
		case (state)
			highwayGreen :
			begin
				HG = 1;
				HY = 0;
				HR = 0;
				FG = 0;
			        FY = 0;
			        FR = 1;
			end
			
			highwayYellow :
			begin
				counter(Clk, 0, 1, countDone);
			
				HG = 0;
				HY = 1;
				HR = 0;
				FG = 0;
			        FY = 0;
			        FR = 1;
			end
			
			highwayRed :
			begin
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 1;
			        FY = 0;
			        FR = 0;
			end
			
			farmYellow :
			begin
				counter(Clk, 0, 1, countDone);
				
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 0;
			        FY = 1;
			        FR = 0;
			end
		endcase
	end

endmodule

module counter (
clock , 
reset , 
enable , 
done 
); 
 
input clock ;
input reset ;
input enable ;
 
output done;

wire clock ;
wire reset ;
wire enable ;

reg [3:0] counter_out ;
  
always @ (posedge clock)
  begin : COUNTER 

    if (reset == 1'b1) begin
      counter_out <=  #1  4'b0000;
		done = 0;
    end
   
    else if (enable == 1'b1) begin
      counter_out <=  #1  counter_out + 1;
      if (counter_out >= 3)
		done = 1;
    end
  end 
  
endmodule

ink4n3

I learned VHDL in school and not Verilog but did you include the right libraries for counter? That's the first thing that popped in my head.

.kbf?

hmm I'm not sure. I was following an online tutorial for the counter section and there are no include statements at all in it.

ink4n3

Is that the whole program what you have posted there or is there more above it?

.kbf?

That's it.

0blique

I'm not sure how you were taught verilog, so take my advice with a grain of salt, since I may use a different style that you do. Also it's been a long time since I last used verilog to do anything, and even then it wasn't anything complicated.

It seems like you are using a counter module as a function (like you would in procedural programming), when you really shouldn't. Every instance of a module acts sort of like a thread, which operates constantly at the same time as everything else. Thus, instantiating a module in a case statement doesn't seem to make sense; rather any modules should be instantiated outside of the always blocks.

In the case statements, I would also add a default case, and else statements to the ifs to prevent unwanted memory elements.

.kbf?

0blique wrote: »

I'm not sure how you were taught verilog, so take my advice with a grain of salt, since I may use a different style that you do. Also it's been a long time since I last used verilog to do anything, and even then it wasn't anything complicated.

It seems like you are using a counter module as a function (like you would in procedural programming), when you really shouldn't. Every instance of a module acts sort of like a thread, which operates constantly at the same time as everything else. Thus, instantiating a module in a case statement doesn't seem to make sense; rather any modules should be instantiated outside of the always blocks.

In the case statements, I would also add a default case, and else statements to the ifs to prevent unwanted memory elements.

That makes sense but how would I go about utilizing the counter in the various states without actually calling it in the state? Make a wire and set it to one when the counter is needed? This is very confusing...

0blique

For what you are doing, I don't think you would need a submodule. What I would do is have a register count in the traffic light module, and change its value in each state. For instance, when you change states from red to green, set the count to 0. Then each time you parse through the green state, you add 1 to count if its less than the amount you need and set the state to green, or reset it to 0 and change the state to yellow if it is at the final value. For this, you have to remember to assign a value to count and state in every possible path in the case statement to avoid latches or any other weird things.

When the state logic gets this complicated, I usually have the next state logic in a separate always block and keep the register instantiation very simple (state <= nextState or whatever).

Also another thing I noticed: you don't need the delay statements on a clocked module like the counter.

.kbf?

Thanks for the help! I was able to get it to compile.

Is this more along the lines of what you where talking about?

module Traffic_Light
	(
		Clk,
		Reset,
		carSensor,
		HG, HY, HR, FG, FY, FR
	);
	
	input Clk;
	input Reset;
	input carSensor;
	output HG, HY, HR, FG, FY, FR;
	reg HG, HY, HR, FG, FY, FR;
	
	reg countDone;

	parameter highwayGreen = 0, highwayYellow = 1, highwayRed = 2, farmYellow = 3;

	reg [3:0] state;

	always @ (posedge Clk or posedge Reset)
	begin
		if (Reset)
			state <= highwayGreen;
		else
			case (state)
				highwayGreen:
					if (carSensor == 1) begin
						state <= highwayYellow;
					end else
						state <= highwayGreen;
						
				highwayYellow:
					if (countDone == 1)
						state <= highwayRed;
					else
						state <= highwayYellow;
		
				highwayRed:
					if (carSensor == 0) begin
						
						state <= farmYellow;
					end else
						state <= highwayRed;
						
				farmYellow:
					if (countDone == 1)
						state <= highwayGreen;
					else
						state <= farmYellow;
					
			endcase
	end

	reg [3:0] count;

	always @ (state)
	begin
		case (state)
			highwayGreen :
			begin
				count = 0;
				HG = 1;
				HY = 0;
				HR = 0;
				FG = 0;
			    FY = 0;
			    FR = 1;
			end
			
			highwayYellow :
			begin
				count = count + 1;
				if (count >= 3)
					countDone = 1;
			
				HG = 0;
				HY = 1;
				HR = 0;
				FG = 0;
			    FY = 0;
			    FR = 1;
			end
			
			highwayRed :
			begin
				count = 0;
				countDone = 0;
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 1;
			    FY = 0;
			    FR = 0;
			end
			
			farmYellow :
			begin
				count = count + 1;
				if (count >= 3)
					countDone = 1;
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 0;
			    FY = 1;
			    FR = 0;
			end
		endcase
	end

endmodule

0blique

Something like that, yeah.

.kbf?

Well it compiles but it's still not working. At all. Which is weird because it generates a FSM diagram that looks correct.



This has me completely stumped.

ecco the dolphin

Hmmm... what clocks your count register?

Hint:

count only seems to change once when "state" changes.

Is this what you want?

Hint:

count needs to change when both "state" changes, and your clock signal produces an edge.

Also, are you going to be synthesising this and putting it on to a CPLD/FPGA?

If so, you might want to consider making reset level triggered instead of edge triggered - a lot of the synthesis tools have trouble with registers having more than one edge trigger source.

.kbf?

eeccccoo tthhee ddoollpphhiinn wrote: »

Hmmm... what clocks your count register?

Hint:

count only seems to change once when "state" changes.

Is this what you want?

Hint:

count needs to change when both "state" changes, and your clock signal produces an edge.

Also, are you going to be synthesising this and putting it on to a CPLD/FPGA?

If so, you might want to consider making reset level triggered instead of edge triggered - a lot of the synthesis tools have trouble with registers having more than one edge trigger source.

Well once the traffic light turns yellow it needs to stay yellow for 3 clock cycles.

I thought that because each clock cycle it it rotates back into the yellow state if clockDone = 0 it would increment clock.

state: yellow->yellow->yellow->red
clock: 1 -> 2 -> 3 ->0

I guess this doesn't work? Could I just use 3 extra states instead of using the clock register?

ecco the dolphin

.kbf? wrote: »

eeccccoo tthhee ddoollpphhiinn wrote: »

Hmmm... what clocks your count register?

Hint:

count only seems to change once when "state" changes.

Is this what you want?

Hint:

count needs to change when both "state" changes, and your clock signal produces an edge.

Also, are you going to be synthesising this and putting it on to a CPLD/FPGA?

If so, you might want to consider making reset level triggered instead of edge triggered - a lot of the synthesis tools have trouble with registers having more than one edge trigger source.

Well once the traffic light turns yellow it needs to stay yellow for 3 clock cycles.

I thought that because each clock cycle it it rotates back into the yellow state if clockDone = 0 it would increment clock.

state: yellow->yellow->yellow->red
clock: 1 -> 2 -> 3 ->0

I guess this doesn't work? Could I just use 3 extra states instead of using the clock register?

One thing about the always block

always @ (state)

is that it only triggers when "state" changes.

When "state" goes from yellow -> yellow, there's no change, and so no reason to increase clock.

Using three extra states is definitely an option. Perhaps not the most optimal one, but it will get the job done.

.kbf?

I added three extra "wait" states that it cycles through in between a yellow and red but somethings still not working.

I'm not getting any errors but I did get this warning.

Warning: Circuit may not operate. Detected 11 non-operational path(s) clocked by clock "Clk" with clock skew larger than data delay. See Compilation Report for details.

It seems theres a latch in state highwayRed? Is it talking about the register yellowHighOrFarm I added?

module Traffic_Light
	(
		Clk,
		Reset,
		carSensor,
		HG, HY, HR, FG, FY, FR
	);
	
	input Clk;
	input Reset;
	input carSensor;
	output HG, HY, HR, FG, FY, FR;
	reg HG, HY, HR, FG, FY, FR;
	
	

	parameter highwayGreen = 0, highwayYellow = 1, highwayRed = 2, farmYellow = 3, waitOne = 4
	, waitTwo = 5, waitThree = 6;

	reg [3:0] state;
	reg yellowHighOrFarm;  //set to 0 if setting highway to red, 1 if setting farm to red

	always @ (posedge Clk or posedge Reset)
	begin
		if (Reset)
			state <= highwayGreen;
		else
			case (state)
				highwayGreen:
					if (carSensor == 1) begin
						state <= highwayYellow;
					end else
						state <= highwayGreen;
				
				highwayYellow:
					state <= waitOne;
				
				waitOne:
					state <= waitTwo;
						
				waitTwo:
					state <= waitThree;
									
				waitThree:
					if (yellowHighOrFarm == 0)
						state <= highwayRed;
					else
						state <= highwayGreen;
		
				highwayRed:
					if (carSensor == 0) begin
						state <= farmYellow;
					end else
						state <= highwayRed;
						
				farmYellow:
					state <= waitOne;
					
			endcase
	end

		always @ (state)
	begin
		case (state)
			highwayGreen :
			begin
				yellowHighOrFarm = 0;
				HG = 1;
				HY = 0;
				HR = 0;
				FG = 0;
			        FY = 0;
			        FR = 1;
			end
			
			highwayYellow :
			begin
				HG = 0;
				HY = 1;
				HR = 0;
				FG = 0;
			        FY = 0;
			        FR = 1;
			end
			
			highwayRed :
			begin
				yellowHighOrFarm = 1;
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 1;
			        FY = 0;
			        FR = 0;
			end
			
			farmYellow :
			begin
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 0;
			        FY = 1;
			        FR = 0;
			end
		endcase
	end

endmodule

edit :

ecco the dolphin

Hm.

Could you try this?

See what it says?

1.) In your always @ (state) block, add a default: case where yellowHighOrFarm, HG, HY, HR, etc, are set to 0.

2.) In your always @ (state) block, make sure yellowHighOrFarm is always assigned in every case.

3.) In your always @ (posedge... ) block, add a default: case where state <= highwayGreen.

.kbf?

eeccccoo tthhee ddoollpphhiinn wrote: »

Hm.

Could you try this?

See what it says?

1.) In your always @ (state) block, add a default: case where yellowHighOrFarm, HG, HY, HR, etc, are set to 0.

2.) In your always @ (state) block, make sure yellowHighOrFarm is always assigned in every case.

3.) In your always @ (posedge... ) block, add a default: case where state <= highwayGreen.

When does it enter the default state?

If I have yellowHighOrFarm = 0; in default: it gives me this which is not what I want but it gets rid of the above warning.



comment out that line and it gives me the state transitions I want but I get
Warning: Circuit may not operate. Detected 2 non-operational path(s) clocked by clock "Clk" with clock skew larger than data delay. See Compilation Report for details.



Does this mean somewhere my logic is fucking up and It's reverting back to default resulting in a perpetual 0 on yellowHighOrFarm?

edit: I just noticed this warning
Warning (10240): Verilog HDL Always Construct warning at ELab3.v(66): inferring latch(es) for variable "yellowHighOrFarm", which holds its previous value in one or more paths through the always construct

not sure if it's relevent

module Traffic_Light
	(
		Clk,
		Reset,
		carSensor,
		HG, HY, HR, FG, FY, FR
	);
	
	input Clk;
	input Reset;
	input carSensor;
	output HG, HY, HR, FG, FY, FR;
	reg HG, HY, HR, FG, FY, FR;
	
	

	parameter highwayGreen = 0, highwayYellow = 1, highwayRed = 2, farmYellow = 3, waitOne = 4
	, waitTwo = 5, waitThree = 6;

	reg [3:0] state;
	reg yellowHighOrFarm;  //set to 0 if setting highway to red, 1 if setting farm to red

	always @ (posedge Clk or posedge Reset)
	begin
		if (Reset)
			state <= highwayGreen;
		else
			case (state)
			
				default:
					state <= highwayGreen;
				
				highwayGreen:
					if (carSensor == 1) begin
						state <= highwayYellow;
					end else
						state <= highwayGreen;
				
				highwayYellow:
					state <= waitOne;
				
				waitOne:
					state <= waitTwo;
						
				waitTwo:
					state <= waitThree;
									
				waitThree:
					if (yellowHighOrFarm == 0) begin
						state <= highwayRed;
					end else
						state <= highwayGreen;
		
				highwayRed:
					if (carSensor == 0) begin
						state <= farmYellow;
					end else
						state <= highwayRed;
						
				farmYellow:
					state <= waitOne;
					
			endcase
	end

		always @ (state)
	begin
		case (state)
			default:
			begin
				//yellowHighOrFarm = 0;
				HG = 0;
				HY = 0;
				HR = 0;
				FG = 0;
			    FY = 0;
			    FR = 0;
			end
		
			highwayGreen :
			begin
				yellowHighOrFarm = 0;
				HG = 1;
				HY = 0;
				HR = 0;
				FG = 0;
			    FY = 0;
			    FR = 1;
			end
			
			highwayYellow :
			begin
				yellowHighOrFarm = 0;
				HG = 0;
				HY = 1;
				HR = 0;
				FG = 0;
			    FY = 0;
			    FR = 1;
			end
			
			highwayRed :
			begin
				yellowHighOrFarm = 1;
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 1;
			    FY = 0;
			    FR = 0;
			end
			
			farmYellow :
			begin
				yellowHighOrFarm = 1;
				HG = 0;
				HY = 0;
				HR = 1;
				FG = 0;
			    FY = 1;
			    FR = 0;
			end
		endcase
	end

endmodule

ecco the dolphin

Hahaha, oh whoops - when you're in waitOne, waitTwo, etc, it enters the default state and ends up resetting the value of yellowHighOrFarm! =P

One solution is to explicitly enter those states in your block, or alternatively you could try moving all assignments of yellowHighOrFarm to your always @ (posedge... block, so it ends up something like:

        begin
                if (Reset)
                        state <= highwayGreen;
                        yellowHighOrFarm <= 0;
                else
                        case (state)
                        
                                default:
                                        state <= highwayGreen;
                                        yellowHighOrFarm <= 0;

-- snip --
                                
                                highwayYellow:
                                        state <= waitOne;
                                        yellowHighOrFarm <= 0;

And other appropriate places.

Edit: Coming to think about it, yellowHighOrFarm should not be combinationally generated.

That last always @ (state) block is combinationally generating the outputs of HG, HY, HR... but yellowHighOrFarm needs to retain its state.

In which case, I recommend assigning the value of yellowHighOrFarm in your sequential (always @ (posedge...)) block.

.kbf?

That did it! Thanks a ton for the help. I would have never figured this out.