Code Review - best practice for sequential execution control

extendedSolutions

Member
E
Join Date
Jun 2016
Location
Boston, MA
Posts
7
Could experienced PLC programmer take a look at these lines of code? It is for controlling a stepper servo motor, as you will see some familiar motor commands. This code is intended for a Panasonic FP-x plc, so there are some FP specific elements, such as R904A=1 flag being COM2 ready, F7_MV2 for moving two words into a dword, and the mess needed to call MODBUS send (F145_SEND) and receive (F146_RECV).

Mainly, I am wondering if the structure I am using to control sequential MODBUS communications are as efficient (in writing style, as well as execution) as they can be. For example, the DF(X1) actions would ideally be:

if(DF(X1)) then
setSpeedAcceleration(10,10,200);
setFeedDistance(1000);
motorFeedToDistance();
while (readMotorCompleteStatus = false);
end_if

and the DF(X2) would be:

if(DF(X2)) then
setDirection(-1);
setHome();
motorSeekHome();
while (readMotorCompleteStatus = false);
zeroEncoderCount();
zeroPosition();
end_if


But it seems I need to separate each MODBUS communicating step into individual ladder rungs. Please let me know if there are smarter ways to do the same thing shown here.








if (DF(X1)) then

	(* set speed and accel, do a feed cmd, and read status for complete *)

	seq_feed := TRUE;

	tms_readStatusDone :=FALSE;

	tms_feedDone :=FALSE;

	tms_setSpeed:=TRUE;

	tms_setSpeedDone :=FALSE;

end_if;



if (DF(tms_setSpeedDone)) then

	tms_feed:=TRUE;

	tms_feedDone :=FALSE;

end_if;



if (DF(tms_feedDone)) then

	tms_readStatus:=TRUE;

	tms_readStatusDone :=FALSE;

	seq_feed := FALSE;

end_if;



IF (tms_setSpeed) THEN

	DT110:=20;  //acceleration of 20

	DT111:=20;  //deacceleration of 20

	DT112:=100;  //speed of 100

	DT113:=0;  //higher byte of move distance

	DT114:=distance;  //lower byte of move distance

	

	F7_MV2(s1 := 16#5, s2 := 16#2001, d => controlword);

	

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 27);  //address to put all 5 words

		tms_setSpeed := FALSE;

		tms_setSpeedDone :=TRUE;

	end_IF;

end_IF;



IF (tms_feed) THEN

	DT110:=16#66;   //cmd 66 is feed to distance

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);  

		tms_feed := FALSE;

		tms_feedDone := TRUE;

	end_IF;

end_IF;



IF (tms_readStatus) THEN

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	

	IF (R904A) THEN

		F146_RECV(s1_Control := controlword, 

		  	s2_AdrType := DT0, 

		 	s2_AdrOffs := 1,  (* 40002 for SC status code *)

		  	d_Start := datafromtsm);

		(* tms_readStatus := FALSE; *)

		if (datafromtsm = 16#9) then    //9 means motor move done

			(* motor is in position and enabled *)

			tms_readStatus := FALSE;

			tms_readStatusDone := TRUE;

		end_if;

	end_IF;

end_IF;









if (DF(X2)) then

	(* call Seek Home *)

	

	seq_seekHome:=TRUE; 

	tms_setDir := TRUE;

	tms_setDirDone:=FALSE;

end_if;



if (tms_setDirDone) then

	tms_setDirDone:=FALSE;

	tms_setHome:=TRUE;

	tms_setHomeDone :=FALSE;

end_if;



if (tms_setHomeDone) then

	tms_setHomeDone:=FALSE;

	tms_seekHome:=TRUE;

	tms_seekHomeDone :=FALSE;

end_if;



if (tms_setHomeDone) then

	tms_setHomeDone:=FALSE;

	tms_seekHome:=TRUE;

	tms_seekHomeDone :=FALSE;

end_if;



if (tms_seekHomeDone) then

	tms_seekHomeDone :=FALSE;

	tms_readStatus:=TRUE;

	tms_readStatusDone :=FALSE;

end_if;



if (tms_readStatusDone AND seq_seekHome) then

	tms_readStatusDone :=FALSE;

	tms_zero126:=TRUE;

	tms_zero126Done:=FALSE;

end_if;



if (tms_zero126Done) then

	tms_zero126Done:=FALSE;

	tms_resetEncoder:=TRUE;

	tms_resetEncoderDone :=FALSE;

end_if;



if (tms_resetEncoderDone) then

	tms_resetEncoderDone:=FALSE;

	tms_resetPosition:=TRUE;

	tms_resetPositionDone :=FALSE;

	seq_seekHome :=FALSE;

end_if;









IF (tms_setDir) THEN

	DT110:=16#FF;

	DT111:=16#FF;

	F7_MV2(s1 := 16#2, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 30);

		tms_setDir := FALSE;

		tms_setDirDone :=TRUE;

	end_IF;

end_IF;



IF (tms_setHome) THEN

	DT110:=16#35;

	DT111:=16#4C;

	F7_MV2(s1 := 16#2, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 125);

		tms_setHome := FALSE;

		tms_setHomeDone :=TRUE;

	end_IF;

end_IF;



IF (tms_seekHome) THEN

	DT110:=16#6E;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);

		tms_seekHome := FALSE;

		tms_seekHomeDone := TRUE;

	end_IF;

end_IF;



IF (tms_zero126) THEN

	DT110:=16#0;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 125);

		tms_zero126 := FALSE;

		tms_zero126Done := TRUE;

	end_IF;

end_IF;



IF (tms_resetEncoder) THEN

	DT110:=16#98;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);

		tms_resetEncoder := FALSE;

		tms_resetEncoderDone := TRUE;

	end_IF;

end_IF;



IF (tms_resetPosition) THEN

	DT110:=16#A5;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);

		tms_resetPosition := FALSE;

		tms_resetPositionDone := TRUE;

	end_IF;

end_IF;

 
You're on your way but there are some minor things you need to fix.

I suggest using an integer to keep track of what you are doing instead of a bunch of flags (booleans). The flags are never active at the same time while an integer can only have one value at the time. Integer is easier to check, easier to set to any value and easier to debug (only one thing to look at).

My preference is to use a case statement instead of a bunch of if statements. Check my post in your other thread.

The code were you read the status wont work. You need to do the RECV statement first THEN wait for it to complete (R904A) and then check whatever status you received to know if you're done or not.

You also need to check for errors if you have any and determine what to do with them.

Always figure that you need to be able to pull the plug on the communication at any time (or cycle power on the drive) and the communication should resume in a meaningful way, no matter what you were doing in your program.
 
Last edited:
Revised code for review - with case and int for state control

Pete.S: Thank you for your very constructive comments, I love the ideas you suggested. I have implemented some of the ideas you mentioned (minus error checking.. I will work on that tomorrow.) The revised code is below. Should I consider encapsulating the repetitive part of the code (IF (R904A)....) in a function or function block?





(* state_feed:=0; *)

(* state_home = 0; *)







CASE state_feed OF

0:	(*do nothing, idle*)

	state_feed :=0;

1: (* start sequence *)

	DT110:=20;

	DT111:=20;

	DT112:=100;

	DT113:=0;

	DT114:=distance;

	F7_MV2(s1 := 16#5, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 27);

		state_feed := 10;

	end_IF;



10:	(* execute feed command *)

	DT110:=16#66;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);  

		state_feed :=20;

	end_IF;



20:	(* check status until feed is complete *)

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F146_RECV(s1_Control := controlword, 

		  	s2_AdrType := DT0, 

		 	s2_AdrOffs := 1,  (* 40002 for SC status code *)

		  	d_Start := datafromtsm);

		 state_feed :=21;

	end_IF;

	

21:	(* check status read data *)

	IF(datafromtsm = 16#9)  THEN

		(* motor is in position and enabled *)

		state_feed := 999;

	ELSE

		state_feed := 20;

	end_IF;





999:	(* squence complete *)

	state_feed = 999;

	

ELSE	(* catch unspecified states *)

	state_feed = 888;



END_CASE;









case state_home of



0:	(*do nothing, idle*)

	state_home :=0;



1:	(* start sequence - set DI direction*)

	DT110:=16#FF;

	DT111:=16#FF;

	F7_MV2(s1 := 16#2, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 30);

		state_home :=10;

	end_IF;



10:	(* Set seek home parameters *)

	DT110:=16#35;

	DT111:=16#4C;

	F7_MV2(s1 := 16#2, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 125);

		state_home := 20;

	end_IF;



20:	(* execute seek home command *)

	DT110:=16#6E;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);

		state_home :=30;

	end_IF;	

	

30:	(* check status until feed is complete *)

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F146_RECV(s1_Control := controlword, 

		  	s2_AdrType := DT0, 

		 	s2_AdrOffs := 1,  (* 40002 for SC status code *)

		  	d_Start := datafromtsm);

		 state_feed :=31;

	end_IF;

	

31:	(* check status read data *)

	IF(datafromtsm = 16#9)  THEN

		(* motor is in position and enabled *)

		state_feed := 40;

	ELSE

		state_feed := 30;

	end_IF;



40:	(* prepare register 126 for zero encoder and position *)

	DT110:=16#0;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 125);

		state_home := 50;

	end_IF;



50:	(* reset encoder count to 0 *)

	DT110:=16#98;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);

		state_home := 60;

	end_IF;



60:	(* reset position count to 0 *)

	DT110:=16#A5;

	F7_MV2(s1 := 16#1, s2 := 16#2001, d => controlword);

	

	IF (R904A) THEN

		F145_SEND(s1_Control := controlword, 

			s2_Start := DT110, 

			d_AdrType := DT0, 

			d_AdrOffs := 124);

		state_home := 999;

	end_IF;



999:	(* sequence complete *)

	state_home := 999;



ELSE	(* catch unspecified states *)

	state_home := 888;

	

END_CASE;

 
extendedSolutions said:
Pete.S: Thank you for your very constructive comments, I love the ideas you suggested. I have implemented some of the ideas you mentioned (minus error checking.. I will work on that tomorrow.) The revised code is below. Should I consider encapsulating the repetitive part of the code (IF (R904A)....) in a function or function block?
Click to expand...

Maybe you should. I'm not sure I would though. But if you can make the code more compact it's often easier to read.

It's generally better to format the code for clarity than it is to always follow a set of fixed rules.

A couple of things you do over many lines I would just do in one.

The important thing for each state is:

  • What is this state doing?
  • Where does it go when it's ready?
The first thing is the comment and the second thing is setting the state at the end. Those should be on their own lines.
But the rest could be written more compact. Unfortunately ST forces you to write the name of the parameters your passing to a function so that makes the code a bit heavy handed compared to regular programming languages.

...

Now you made two state machine but only one can run at the time since they are communicating with the same driver. So I would put them together. Put the homing section starting at 100 or something.

If you wanted to you could have the state machine start itself by checking your the start condition and then going to the next state. So in the first state you would check if you should do any homing (in that case goto 100) or if you do the other (goto 2 or whatever).

I still like to keep 0 as my "do nothing at all" because sometime you want to be able to turn something completely off.

.
 
Last edited:
I prefer to use variables (constants) as step numbers. Instead of using 0, 1, 100, 200 etc.

That way you can count your steps 0, 1, 2 , 3, 4 and if you need to add a step, you can just move them all down with a copy paste command in the definition of the constants.

I usually use unsigned short integers for steps (max 255). Rarely do I need more than 255 steps but if I do, I can just change the definition quickly anyway.
 
Diameter157 said:
I prefer to use variables (constants) as step numbers. Instead of using 0, 1, 100, 200 etc.

That way you can count your steps 0, 1, 2 , 3, 4 and if you need to add a step, you can just move them all down with a copy paste command in the definition of the constants.

I usually use unsigned short integers for steps (max 255). Rarely do I need more than 255 steps but if I do, I can just change the definition quickly anyway.
Click to expand...

That's some good points.

For really large sequences I usually expand into two variables to track where I am, a step and a sub step. I write them like this in the documentation: 1.1, 1.2, 35.3 etc.

Regarding the use of bytes I'm not sure that you will gain any performance by going short int. If the PLC is 16 bit internally and I think they are at least 16 or more likely 32, its faster to use a 16 bit integer than 8 bit.
 
Last edited:

Similar Topics

D
Been asked to review some code, looking for a sanity check with pump/valve control
Hey guys, I've recently been asked to review some code at a water treatment plant that isn't working right; seems like sometimes it'll work, but...
Replies
7
Views
5,257
daba
D
Mas01
S7-TIA Portal: How can I make a project if I've only got the code loaded on the PLC?
Hi All, Someone at work has put a PLC system on my desk, that's just been taken off an idle production line. He said "It's an S7 PLC. We don't...
Replies
10
Views
253
bkottaras
B
A
sepro robot error code 3
hello, I'm a student and for my final grade I have to repare a sepro robot. the only problem I have for now is that i have an error code 3...
Replies
0
Views
37
aleksander990
A
R
Can Anyone Identify Where this Code may have come from?
I received an email from a student with the following code attached. This is supposed to control a floodgate system, and supposed to be written...
2
Replies
23
Views
785
drbitboy
drbitboy
A
SEW IPOS Drive Assembler Code conversion to PLC Ladder Logic
I have a machine which is undergoing upgradation. As part of the process two SEW drives are being replaced., existing Gen B with new Gen C. The...
Replies
3
Views
202
JeffB
J
Back
Top Bottom