Great forum. First post, so forgive the FNG if I cause irritation.
A lot of good points made already.
It sounds obvious, but it may be helpful to point out that good practice may just be the absence of bad practice. It's hard to say what makes code easy to read and maintain, but very easy for me to list what makes it terrible.
After being unemployed for about 6 months, I had some consulting jobs find me. Fixing/finishing PLC (AB and idec) programming where the origional programmers had either quit, or been fired, so I have recent experience with what I consider poor code.
First is a thing I've cleaned up in a couple of programs, and from what I have seen, it is a sin common to many PLC programmers (I was guilty of it on my first couple of projects, in fact):
If part of a process has, say six valid states, DO NOT use 6 boolian variables (internal relays, bits, flags, whatever you want to call them) to represent these 6 states. Only use a boolian (one) if you are sure there will only ever be, at most, two states.
Instead use one integer variable (Data register- whatever) and put a state number in there. You can always add more valid states later on, so there is no restriction here. One simple test can insure that it does not get set to illeagle values (assuming you don't skip around).
You can then test that integer to figure out which state you're in. You can then only be in one state at a time, and no logic at all is required to force that to be true. Yes, a compare eats more time and memory than a simple contact, but you make up for it reliability, readability, etc. Also consider that the boolian approach requires lots of logic to prevent more than one state at a time. If you ever do manage to create enough logic to reliably insure that no more than one of those boolians is true at once, there is no way the next guy will be able to follow it.
Two real world examples of this:
-A step-by-step process that used a series of internal relays where each step finished by clearing it's own relay, and setting the relay for the next step. All kinds of micky mouse stuff replaced by a "step counter". As it turned out, the process later had to be modified to skip some steps under certain conditions, and it was simple to just advance the step counter to where it needed to go, rather than insuring the correct relays got set and cleared.
-A water treatment system which used two sand-filters. Three valves at inlet, and three at outlet for each filter. Only 4 permissable states (of 64 possible) for the 6 valves on each filter. So origoinal progammer had 4 boolians per filter to establish the state. Of course two or more of these would sometimes get set at the same time, and of course the code was so convoluted it was impossible for me to figure out why. After about 8 hours of trying to make it work the way it was orgiounally written, I spent a couple hours re-writing and debugging it so each filter had ONE state register, and all (those) problems were solved.
Next thing:
I like to see all the conditions that establish the state of an output (or anything, but it usually comes up with outputs) on the same rung as the output coil. It can lead to some messy looking rungs, but it is way easier to debug.
Instead, I see one or two conditons that control an intermediate relay,
and then a couple of those relays controlling a third intermediate relay, and finally that last intermediate contact doing absolutlely nothing except controlling one coil. Thus the four rungs that control the one output are scattered all over the code, and you have to do a lot of backtracking to find any problems.
I understand the argument for doing it this way. Some of those intermediate relays may be used elsewhere, and if you get it right on the rung where it is controlled, then you don't have to fix it three other places. But then when the next guy has to fix it, he has to find the three places that broke when he fixed the fourth one.
The way I see it, for every byte of memory and microsecond of cycle time that this "effiency" saves will cost about an hour of troubleshooting.
Anyway, I'd preach for some middle ground. It seems pretty stupid to me to have logic controlling an internal relay who's only function is to drive a single output. If you need to trace back more than two rungs to determine what inputs/conditions control a given output, then the programmer was being too fancy.
As for naming conventions: [rant] It is perfectly obvious to anyone looking at ladder logic when they are looking at a relay. Therefore, using the word "relay" in every freaking variable name that is in fact a "relay" is just stupid. In fact, I can tell if it is an internal relay, or a special function relay by it's allocation prefix, so "internal" and "special function" don't need to be in the name either. Please use the keystrokes thus saved to provide less cryptic information about the actual function of these "relays". The same applies to registers, inputs, and outputs. Thank you. [/rant]
As for how to segment, or organize a program. Putting all the valve controls in one module, and all the pump controls in another, and all the level controls in a third may make sense when you start.
However, since the pumps require that certain valves be set a certain way, and those pumps are used to control the levels, then the guy troubleshooting will end up bouncing between all your modules to solve every single problem. And, to make it all work, your going to have lots of intermediate variables to communicate states between those modules. So your nice, well organized program is anything but when viewed from the standpoint of "why process X isn't working".
Thus I find it much easier if the program is modular in the form of having all the controls for each process bunched togethor. Sometimes processes interact, or share resources, so this doesn't always work out though.
Sorry for the ranting. First time I've encountered a group who whould understand these issues, and couldn't resist temptation to vent.
ciao,
Kevin