Stuck in Loop, Among other things....

[whitnasty1]

Hello all!
I am having some problems with piece of software I am trying to get to work. I'm a manufacturing engineer, so all of this is a little bit foreign to me. And we're on a skeleton crew, so this got delegated to me, ha.

I am making an optical bonding station that includes a few switches, valves, and relays. The program is pretty basic and doesn't need to do much, but I'm having some issues. So this basic idea of the program is, it checks two switch inputs to initiate the process (lid switch and start button). Once those switches are made, the RUN indicator, and the vac valve go HIGH... Once proper vac has been achieved, the vac switch goes HIGH and we activate the air cylinder (press) and turn the vac valve OFF. Once the micro sees the bottom cylinder switch go HIGH (the cylinder is down), there is a 5 sec delay to allow the bonding to take place. After the delay, the air valve is turned OFF and the cylinder goes back up, the top cylinder switch (cylSwitch1) then goes HIGH (the cylinder is in the up position). Lastly, once the cylSwitch1 goes HIGH, we actuate the vacuum release valve and turn the RUN indicator OFF. End program.

I have two problems with the code though..

1. cylSwitch1 is HIGH the entire program, except when it goes down to press the optical material. So when the program begin, the cylSwitch1 input is HIGH, and the code skips over everything before it and actuates the vacuum release valve. I tried setting the pin to an output up until the switch is read, and then switching it back, but that did not seem to help anything.

2. If I keep cylSwitch1 LOW until the point is needed, it executes the code correctly up until the LAST if statement. When the program checks cylSwitch1 and cylSwitch2 states, the program gets caught in a loop and just keep flipping the vac release valve on and off, because the statement remains true. The cylinder isn't moving anymore, so that if statement is indefinitely true and it just loops that last if statement.

I'm sure it has something to do with how I have the program structured, and I am not a software guy, so I know this isn't pretty. But I've been working on this all week and have not made much process. I designed and laid out the PCBs for the bonding station, and have built the station, just can not get this last thing to work.

Any help or guidance would be greatly appreciated. I really just need the program to step through in order, so cylSwitch1 does not force the program to jump to the very end of the loop, and I need to figure out how to get out of the loop at the end, even though the last if statement is true. I need the program to move on haha.

This is going to be used in a production setting, so when the program finishes, pressing the start button again (and closing the lid on the system) will begin the process over again. Again, I'm sure this isn't the right way to do what I'm trying, but I'm trying. Any and all help is greatly appreciated!!

Thanks very much everyone! Code below...

const int lidSwitch = 5;
const int startButton = 6;
const int cylSwitch1 = 7;
const int cylSwitch2 = 8;
const int vacSwitch = 9;
const int greenLED = 4;
const int vacValve = 12;
const int airValve = 2;
const int vacRelease = 3;
int lidVal = 0;
int startVal = 0;
int vacVal = 0;
int cyl1Val = 0;
int cyl2Val = 0;

void setup() {

pinMode(lidSwitch, INPUT);
pinMode(startButton, INPUT);
pinMode(cylSwitch2, INPUT);
pinMode(vacSwitch, INPUT);
pinMode(greenLED, OUTPUT);
pinMode(vacValve, OUTPUT);
pinMode(airValve, OUTPUT);
pinMode(vacRelease, OUTPUT);
}

void loop() {
pinMode(cylSwitch1, OUTPUT);
lidVal = digitalRead(lidSwitch);
startVal = digitalRead(startButton);
if (lidVal == HIGH && startVal == HIGH) {
digitalWrite(greenLED, HIGH);
digitalWrite(vacValve, HIGH);
delay(100);}

vacVal = digitalRead(vacSwitch);
if (vacVal == HIGH) {
digitalWrite(vacValve, LOW);
digitalWrite(airValve, HIGH);
delay(100);}

cyl2Val = digitalRead(cylSwitch2);
if (cyl2Val == HIGH){
delay(5000);
digitalWrite(airValve, LOW);
delay (100);}

delay(100);
pinMode(cylSwitch1, INPUT);
cyl2Val = digitalRead (cylSwitch2);
cyl1Val = digitalRead (cylSwitch1);
if (cyl2Val == LOW && cyl1Val == HIGH) {
digitalWrite (vacRelease, HIGH);
delay (1000);
digitalWrite (vacRelease, LOW);
digitalWrite (greenLED, LOW);
pinMode(cylSwitch1, OUTPUT);
}

}

[whitnasty1]

Attached are schematics and the PCBs... I have tested the boards and wiring, all of that is working properly. My last problem is just wit the software it seems....

Thanks!

[Robin2]

You are going to need a more complex program that works its way through the different states of the process. For example,

• WaitForStart,
• WaitForVacuum,
• WaitForBottomSwitch
• AllowBondingTime
• WaitForTopCylinderHIGH

This idea of a "state machine" is a very common programming paradigm. It is generally easy to implement using one or more variables to keep track of the situation.

The code in Reply #15 in this link should give you a flavour of what I have in mind

...R

PS ... Please display your image(s) in your post so we can see it(them) without downloading it(them). See this Simple Image Posting Guide

[anon73444976]

delay(100);} Two things you don't want to do, on one line.
Avoid delay(), and always put } on their own line.

[whitnasty1]

I'm guessing you are referring to this section of the code in the previous thread?

enum motorStateENUM {
WAITING,
FIRSTRUN,
BETWEENRUNS,
SECONDRUN,
FINISHED,
};

motorStateENUM motorState = WAITING;

The problem seems to be centered around that cylSwitch1. If I could just not use that switch, the program would work as intended. Even though its extremely ugly... I'm doing some reading on the "state machine" programming that you mentioned. I'm learning this stuff as I go, so excuse the ignorance on the subject! I have kind of came up with a hardware work around to get the board to ignore the switch input at the beginning of the program, but am still trying to find a way to get out of the last if statement in the program since it continues to be true. Just tried using an output to hit the reset pin as a part of the last if statement but that doesn't seem to be doing the trick.

Thanks for the response, I really appreciate it!

[whitnasty1]

So I've been playing some more and figured out the RESET section at the end, by setting the RESET output pin to HIGH in the void setup () . Now, once we make it to the end of the program, the board resets as it should... However I am still having some issues with it reading the cylSwitch1 input immediately, once the program starts and it just starts looping the last if statement again. I just need to ignore the cylSwitch1 somehow, until the very end of the program when it's called. Fixed the delays, and moved the brackets to their own separate lines as well.

Thanks for the tips!

const int lidSwitch = 5;
const int startButton = 6;
const int cylSwitch1 = 7;
const int cylSwitch2 = 8;
const int vacSwitch = 9;
const int greenLED = 4;
const int vacValve = 12;
const int airValve = 2;
const int vacRelease = 3;
const int toReset = 10;
int lidVal = 0;
int startVal = 0;
int vacVal = 0;
int cyl1Val = 0;
int cyl2Val = 0;

void setup() {

digitalWrite (toReset, HIGH);
pinMode(lidSwitch, INPUT);
pinMode(startButton, INPUT);
pinMode(cylSwitch2, INPUT);
pinMode(vacSwitch, INPUT);
pinMode(greenLED, OUTPUT);
pinMode(vacValve, OUTPUT);
pinMode(airValve, OUTPUT);
pinMode(vacRelease, OUTPUT);
pinMode(toReset, OUTPUT);
}

void loop() {
pinMode(cylSwitch1, OUTPUT);
digitalWrite (toReset, HIGH);
lidVal = digitalRead(lidSwitch);
startVal = digitalRead(startButton);
if (lidVal == HIGH && startVal == HIGH) {
digitalWrite(greenLED, HIGH);
digitalWrite(vacValve, HIGH);
delay(100);
}

vacVal = digitalRead(vacSwitch);
if (vacVal == HIGH) {
digitalWrite(vacValve, LOW);
digitalWrite(airValve, HIGH);
delay(100);
}

cyl2Val = digitalRead(cylSwitch2);
if (cyl2Val == HIGH){
delay(5000);
digitalWrite(airValve, LOW);
}

delay(2000);
pinMode(cylSwitch1, INPUT);
cyl2Val = digitalRead (cylSwitch2);
cyl1Val = digitalRead (cylSwitch1);
if (cyl2Val == LOW && cyl1Val == HIGH) {
digitalWrite (vacRelease, HIGH);
delay (1000);
digitalWrite (vacRelease, LOW);
digitalWrite (greenLED, LOW);
pinMode(cylSwitch1, OUTPUT);
digitalWrite (toReset, LOW);
}

}

[Robin2]

whitnasty1:
So I've been playing some more and figured out the RESET section at the end,

Your program is still not trying to work through the different states of your system. Using delay()s is not the way to do it - the timing will get out of sync and the whole thing will go haywire.

...R

PS ... when posting code please use the code button </>

so your code 
looks like this

and is easy to copy to a text editor See How to use the forum

[whitnasty1]

Would setting up some kind of switch...case scenario be a, better way to make it flow without using the delays?

[SteveMann]

This line:

pinMode(cylSwitch1, OUTPUT);

belongs in the setup().

Also, are you aware that you are powering the 328P with 4.4V?

[whitnasty1]

SteveMann:
This line:

pinMode(cylSwitch1, OUTPUT);

belongs in the setup().

Also, are you aware that you are powering the 328P with 4.4V?

SteveMann:
This line:

pinMode(cylSwitch1, OUTPUT);

belongs in the setup().

Also, are you aware that you are powering the 328P with 4.4V?

Yeah, the external 5v supply is outputting about 5.5VDC to make up for the diode.
Trying to come up with a switch..case statement that will do the same thing I'm trying to do above. Might have better luck with that.

Thanks for the help!

[Robin2]

whitnasty1:
Would setting up some kind of switch...case scenario be a, better way to make it flow without using the delays?

SWITCH-CASE is just a special (limited) form of IF-ELSE. These are the essential building blocks of any program that must make decisions.

But they, of themselves, are not a substitute for your delay()s although they are likely to be part of the solution.

The first requirement is to get your head around the concept of a program that starts in one STATE, something (such as a switch) moves it to another STATE in which it marks time until something else happens which causes it to move on to a third STATE etc etc.

In the link I gave you in Reply #2 I used IF statements to move between the states but I could equally well have used SWITCH-CASE.

...R

[SteveMann]

I just looked at the second post of your code. Please use code tags. Also, in the IDE, use CTRL-T to clean up your line alignment.

Pinmode commands belong in the setup() and you have two in the last if().
Further, the pin is either an output or an input; you are switching between them and the results will be unpredictable at worst, or just not do what you expect because, I don't think you can change pinmode at runtime.

Please explain again what cylSwitch2 and cylSwitch1 do? How are they actuated?

[SteveMann]

Robin2:
But they, of themselves, are not a substitute for your delay()s although they are likely to be part of the solution.

I don't like delays in the loop either, but until we understand the hardware better, we're just guessing if they have an undesirable effect.

OP- Please explain the hardware better. What actuates the inputs and what do the outputs control. Particularly does toReset do?

[alto777]

check two switch inputs to initiate the process

		Once both switches are made
				the RUN indicator, and the vac valve go HIGH..
				
		Once the vac switch goes HIGH we 
				activate the air cylinder (press) 
				turn the vac valve OFF.
						
						
		Once the micro sees the bottom cylinder switch go HIGH 
			5 sec delay to allow the bonding to take place.
					
		After the delay, the air valve is turned OFF
				
		when the top cylinder switch (cylSwitch1) goes HIGH
				
			we actuate the vacuum release valve and
			turn the RUN indicator OFF. End program.

It is a bit unclear what is switched by what, and what controls what, &c. And you already see ppl are advising you make this way more complicated in order to simplify it.

But the plain truth is that this looks like a fine place to just program it literally. Yes, you will compromise your future ability to change the functionality. Yes, you will miss out on learning very useful and important techniques like state machinery and such like, but finally YES, you will get the stupid thing to work just fine.

There is a place and time for literal programming. I have many very ugly programs running, I do mean to rework them so any committee would love to see what I've done, but I am too lazy, plus it works, so what?

But - I do wonder if there are any safety or diagnostic things you'd like to sneak in there. Soon enough you will find that the literal program is, indeed, unwieldily and inflexible.

HTH

a7

[whitnasty1]

It seems like this is pretty much doing what I want it to do. Just using separate cases with if statements to progress through the program. Then just pointing it back to case 0 to "restart the operation".

CylinderSwitch1 and CylinderSwitch2 are two reed type switches located on an air cylinder (press). CS1 is the upper switch, that would be the HOME position basically. CS2 is the lower reed switch that is actuated when the cylinder is in the DOWN (press) position. So CS1 is typically always high with the exception of when we actuate the air valve that forces the cylinder down. It's basically just a double check to ensure the cylinder is out of the way, and in the correct position before we release the vacuum and allow the enclosure to be opened.

const int lidSwitch = 5;             
const int startButton = 6;              
const int cylSwitch1 = 7;                 
const int cylSwitch2 = 8;                  
const int vacSwitch = 9;
const int greenLED = 4;             
const int vacValve = 12;              
const int airValve = 2;                 
const int vacRelease = 3;

int lidVal = 0;
int startVal = 0;
int vacVal = 0;
int cyl1Val = 0;
int cyl2Val = 0;

int programCount = 0;

void setup() {

  Serial.begin(9600);
  pinMode(lidSwitch, INPUT);
  pinMode(startButton, INPUT);
  pinMode(cylSwitch2, INPUT);
  pinMode(vacSwitch, INPUT);
  pinMode(greenLED, OUTPUT);
  pinMode(vacValve, OUTPUT);
  pinMode(airValve, OUTPUT);
  pinMode(vacRelease, OUTPUT);
  programCount = 0;
}

void loop() {

  switch (programCount) {

    case 0:

    digitalWrite (greenLED, LOW);
    digitalWrite (vacValve, LOW);
    digitalWrite (airValve, LOW);
    digitalWrite (vacRelease, LOW);
    Serial.println ("Waiting to Start Sequence...");
    lidVal = digitalRead(lidSwitch);
    startVal = digitalRead(startButton);
    if (lidVal == HIGH && startVal == HIGH){
    programCount = 1;
    }
    break;

    case 1:

    digitalWrite (greenLED, HIGH);
    digitalWrite (vacValve, HIGH);
    Serial.println ("Pulling vacuum.");
    vacVal = digitalRead(vacSwitch);
    if (vacVal == HIGH){
    programCount = 2;
    }
    break;

    case 2: 

    digitalWrite (vacValve, LOW);
    delay (100);
    digitalWrite (airValve, HIGH);
    Serial.println ("Vacuum achieved, pressing gel.");
    cyl2Val = digitalRead(cylSwitch2);
    if (cyl2Val == HIGH){
    programCount = 3;
    }
    break;

    case 3:

    delay (3000);
    digitalWrite (airValve, LOW);
    Serial.println ("Gel Press Complete.");
    programCount = 4;
    break;

    case 4:

    Serial.println ("Waiting for cylinder.");
    cyl2Val = digitalRead(cylSwitch2);
    cyl1Val = digitalRead(cylSwitch1);
    if (cyl2Val == LOW && cyl1Val == HIGH) {
    programCount = 5;
    }
    break;

    case 5:

    digitalWrite (vacRelease, HIGH);
    delay (1000);
    digitalWrite (vacRelease, LOW);
    Serial.println ("Vacuum has been released.");
    programCount = 6;
    break;

    case 6:

    digitalWrite (greenLED, LOW);
    delay (100);
    Serial.println ("Remove lid and part. Ready for next unit.");
    programCount = 0;
    break;

  }
}

[Robin2]

whitnasty1:
It seems like this is pretty much doing what I want it to do.

That's the sort of thing I had in mind.

If you don't want to use an ENUM you could define variables with the numbers 0 to 6 to make things easier to read. For example

byte WaitToStart = 0;
byte PullVacuum = 1;
// etc

and then your code could be

case WaitToStart:
    // other stuff
       programCount = PullVacuum;
    }
    break;

case PullVacuum:
  // etc

And I think programState might be more appropriate than programCount.

...R

[alto777]

It's still just a linear program, uses delay and is otherwise sorta dead-end.

But it works, OK. <-- I am all for literal programs that just do what the code sez.

At a glance it seems like you also accomplished getting lotsa output on the serial monitor - case 2, for example will

Serial.println ("Vacuum achieved, pressing gel.");

10 times a second.

case 4 will

Serial.println ("Waiting for cylinder.");

at whatever loop rate you are achieving, like a crap-ton of lines of output.

You could just print those once, set a flag saying you had, clear the flag (for use in the next case) only when you are changing the state variable.

bool printedMessage = FALSE;

…


if (!printedMessage) {
  Serial.println ("Waiting for cylinder.");
  printedMessage = TRUE;
}

…


if (cyl2Val == LOW && cyl1Val == HIGH) {
  printedMessage = FALSE;
  programCount = 5;
}

Or just do what you had originally. Just write the sequence of steps, use while (! to hang up instead of if( to change the program counter.

In this case, making state machinery or something that sorta looks state-y is a step backwards.

a7

[SteveMann]

whitnasty1:
It seems like this is pretty much doing what I want it to do. Just using separate cases with if statements to progress through the program. Then just pointing it back to case 0 to "restart the operation".

CylinderSwitch1 and CylinderSwitch2 are two reed type switches located on an air cylinder (press). CS1 is the upper switch, that would be the HOME position basically. CS2 is the lower reed switch that is actuated when the cylinder is in the DOWN (press) position. So CS1 is typically always high with the exception of when we actuate the air valve that forces the cylinder down. It's basically just a double check to ensure the cylinder is out of the way, and in the correct position before we release the vacuum and allow the enclosure to be opened.

Is this code working?

Just a hint- don't be so cryptic with your device and pin names. If you called say something descriptive like "homeSwitch" instead of "CylinderSwitch1", then your narrative and code would make more sense.

"cyl2Val" probably has meaning to just one person. Meaning that if someone else reads the code in the future, they will be completely lost. Use something more descriptive, like "downSwitchVal" so whomever is reading the code doesn't need a codex to figure out what "cyl2val" means.

[whitnasty1]

SteveMann:
Is this code working?

Just a hint- don't be so cryptic with your device and pin names. If you called say something descriptive like "homeSwitch" instead of "CylinderSwitch1", then your narrative and code would make more sense.

"cyl2Val" probably has meaning to just one person. Meaning that if someone else reads the code in the future, they will be completely lost. Use something more descriptive, like "downSwitchVal" so whomever is reading the code doesn't need a codex to figure out what "cyl2val" means.

Yes, this code seems to be doing what it should with the limited amount of testing got to do this afternoon. It steps through the processes that need to happen and ends up back at case 0. Definitely working a whole lot more than the first piece of code I tried, haha. It doesn't have to be anything fancy, it just needs to do this simple set of operations... Thanks for the tips about the naming schemes. Like I said, I am a manufacturing engineer so this stuff is a little bit out of my wheelhouse. I normally don't do much software, other than ladder logic for PLCs and SPEL+ for programming Epson robots. Someone didn't put the PLC in the budget though, so I had to improvise. Got the controller/relay pcbs working for under $100 including components, so not too bad.

Thanks very much for all of the help! I greatly appreciate it!