Go Down

Topic: Trouble with some "IF Statements". (Read 420 times) previous topic - next topic

roguemike

So a little context first. I have 4 boxes, each one has a laser detection sensor and a relay. Basically they are set up to activate the relay when the sensor is hit with a laser. Each box has been individually tested and are all confirmed to interdependently work.
When they are all set up together, box 1 will work as intended, but box 2, 3 and 4 never react. I've tried the code 2 different ways (A lot of ways but 2 that were actually down the right path of thinking). All I know is, the IF statement works, but only on the first IF statement.
"R1-4" is Receiver 1 through 4. "O5-8" is Output 1 through 4 (on pins 5-8). When the Arduino receives an input on pin 1, it outputs a signal to the relay on pin 5 for one second. When I put any box onto input pin 1 and output pin 5 it works, but the rest do not. I'm sure its some syntax I'm not doing correctly.
Here is my 2 variations. I believe the second code is closer to the correct way, but I don't really know.
Thanks in advanced for the help! <3


Code: [Select]

{
//Lights
  int val1 = digitalRead(R1);
  if (val1 == HIGH){
  digitalWrite(O5,HIGH);
  delay (1000);
}
  else{
  digitalWrite(O5,LOW);
  delay(1);
}   
{
//Buzzer - Might need fixed
  int val2 = digitalRead(R2);
  if (val2 == HIGH){
  digitalWrite(O6,HIGH);
  delay (1000);
}
  else{
  digitalWrite(O6,LOW);
  delay(1);
}
{
//Motor
  int val3 = digitalRead(R3);
  if (val3 == HIGH){
  digitalWrite(O7,HIGH);
  delay (1000);
}
  else{
  digitalWrite(O7,LOW);
  delay(1);
}
{
//Servo
  int val4 = digitalRead(R4);
  if (val4 == HIGH){
  digitalWrite(O8,HIGH);
  delay (1000);
}
  else{
  digitalWrite(O8,LOW);
  delay(1);
}


Code: [Select]

  int val1 = digitalRead(R1);
  int val2 = digitalRead(R2);
  int val3 = digitalRead(R3);
  int val4 = digitalRead(R4); 
//Lights
  if (val1 == HIGH){
    digitalWrite(O5,HIGH);
    delay (1000);
  }
//Buzzer
  else if (val2 == HIGH){
    digitalWrite(O6,HIGH);
    delay (1000);
  }
  //Motor
  else if (val3 == HIGH){
    digitalWrite(O7,HIGH);
    delay (1000);
  }
//Servo
  else if (val4 == HIGH){
    digitalWrite(O8,HIGH);
    delay (1000);
  }
//All off
  else{
    digitalWrite(O5, LOW);
    digitalWrite(O6, LOW);
    digitalWrite(O7, LOW);
    digitalWrite(O8, LOW);
    delay(1);
    }

Robin2

You need to post a complete program.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

roguemike

It's more of a format question. The problem I'm having isn't from the setup. Again, the first IF statement works, so the issue is in the IF statements.

pcbbc

As said: Please post complete examples, not snippets of what you thing is wrong.


But trying to make sense of what you have posted.


Do you want...
A) R1, R2, R3 and R4 to act independently of each other?

Or...
B)
- R1 take precedence over R2, R3 and R4.
- R2 take precedence over R3 and R4.
- R3 take precedence over R4.
- Finally R4 activates if nothing else is set?


Your first example code seems to have a lot of mismatched "{" which do not have a closing "}".
In C/C++ every "{" should be followed by a matching "}".
Either that's because of some code you didn't post, or you've made a mistake with your brackets and it won't compile.

You should really have a consistent approach to your { }.  Follow either of these patterns...
1. Place each new "{" on a new line, aligned under the if/else/while/for/etc. statement above it.  Indent all code in the code block that follows by 1 tab.  Finish with a "}" on a line on its own directly aligned under its matching "{".
2. Place each new "{" at the end of the if/else/while/for/etc. statement it belongs with.  Indent all code in the code block that follows by 1 tab.  Finish with a "}" on a line on its own directly aligned under the if/else/while/for/etc. statement that began the code block.

Personally I prefer option 1.  The Tools -> Auto Format (CTRL+T) option in the IDE will help tidy up existing code.



But getting back to your code.  If you goal is option A), here is a fixed version of your first code snippet that achieves that...
Code: [Select]
//Lights
int val1 = digitalRead(R1);
if (val1 == HIGH)
{
  digitalWrite(O5,HIGH);
  delay (1000);
}
else
{
  digitalWrite(O5,LOW);
  delay(1);
}  

//Buzzer - Might need fixed
int val2 = digitalRead(R2);
if (val2 == HIGH)
{
  digitalWrite(O6,HIGH);
  delay (1000);
}
else
{
  digitalWrite(O6,LOW);
  delay(1);
}

//Motor
int val3 = digitalRead(R3);
if (val3 == HIGH)
{
  digitalWrite(O7,HIGH);
  delay (1000);
}
else
{
  digitalWrite(O7,LOW);
  delay(1);
}

//Servo
int val4 = digitalRead(R4);
if (val4 == HIGH)
{
  digitalWrite(O8,HIGH);
  delay (1000);
}
else
{
  digitalWrite(O8,LOW);
  delay(1);
}




Otherwise your second code snippet does option B) because you have used the "else if" construct.  That means if a preceding "if or "else if" was TRUE then none of the other "else if" expressions are evaluated.

I guess that is not what you wanted as you say...
Quote
When they are all set up together, box 1 will work as intended, but box 2, 3 and 4 never react.

pcbbc

#4
Jun 25, 2020, 12:25 pm Last Edit: Jun 25, 2020, 12:27 pm by pcbbc
It's more of a format question. The problem I'm having isn't from the setup. Again, the first IF statement works, so the issue is in the IF statements.
Sorry, but if you do not know what or where the problem is yourself, then you are not qualified to say where the problem IS.

You are making an assumption of where the problem is.  You formed that opinion by looking at your entire program and discounting sections of it because you thought they weren't relevant.  Maybe those assumption are correct, or maybe they are fallacious.

It's really only fair you post your entire sketch (or at least a minimal sketch that compiles and demonstrates the problem) so we can form our own opinions.

As I pointed out, your first snipped has 4 x "{" which have no matching "}".  Right of the bat (Edit: assuming your code compiles) that says to me there may be some outer nested control statement (if/else/while/etc) which we are not seeing.  That will make all the difference as to how the code performs.

pcbbc

#5
Jun 25, 2020, 01:51 pm Last Edit: Jun 25, 2020, 01:51 pm by pcbbc
ment.
"R1-4" is Receiver 1 through 4. "O5-8" is Output 1 through 4 (on pins 5-8). When the Arduino receives an input on pin 1, it outputs a signal to the relay on pin 5 for one second. When I put any box onto input pin 1 and output pin 5 it works, but the rest do not. I'm sure its some syntax I'm not doing correctly.
Also delay is blocking.  While you are delaying for 1 second you can't be responding to any other inputs.


If you want:
a) All inputs to act independently from each other
b) The output to go HIGH for 1 second when triggered by a HIGH input, and then return LOW (even if the input is still HIGH)

You need to check out the "blink without delay" and "state machine" examples in the sticky thread.

A very quick first stab (untested) at that...
Code: [Select]
typedef enum {
  WAIT_HIGH = 0,
  WAIT_1SEC = 1,
  WAIT_LOW = 2
} e_TIMER_STATES;

#define R1 1
#define R2 2
#define R3 3
#define R4 4

#define O5 5
#define O6 6
#define O7 7
#define O8 8

uint8_t pinIn[] = { R1, R2, R3, R4 };
uint8_t pinOut[] = { O5, O6, O7, O8 };

e_TIMER_STATES timerState[4];
uint32_t timerStart[4];

void setup()
{
  Serial.begin(57600);

  for (uint8_t i = 0; i < 4; i++)
  {
    pinMode(pinIn[i], INPUT);
    pinMode(pinOut[i], OUTPUT);
  }
}

void loop()
{
  uint32_t now = millis();
  for (uint8_t i = 0; i < 4; i++)
  {
    switch (timerState[i])
    {
      case WAIT_HIGH:
        // If pin has gone HIGH...
        if (digitalRead(pinIn[i] == HIGH))
        {
          // Turn output ON
          digitalWrite(pinOut[i], HIGH);
          // Setup for wait 1 second
          timerStart[i] = now;         
          timerState[i] = WAIT_1SEC;
        }
        break;
       
      case WAIT_1SEC:
        // If 1 second has passed...
        if (now - timerStart[i] >= 1000)
        {
          // Turn pin OFF
          digitalWrite(pinOut[i], LOW);
          // Setup to wait for pin low
          timerState[i] = WAIT_LOW;
        }       
        break;
       
      case WAIT_LOW:
        // If pin has returned LOW
        if (digitalRead(pinIn[i] == LOW))
        {
          // Setup to wait for pin high
          timerState[i] = WAIT_HIGH;
        }
        break;
    }
  }
}

It does at least compile.


roguemike

Alright so, here is the full code. I only ever want one sensor to dictate their corresponding relays action. So R1 being High will only signal O5, R2 only tells O6 to go high, R3 to O7 and lastly R4 to O8. (R1-O5)(R2-O6)(R3-O7)(R4-O8). Basically think of this as a shooting gallery. A laser gun shoots a target and you want a mechanical response for the hit target only. For example use, I just have them at 1 second activation.
To try to answer some of the questions:
-I didn't post the full code and assumed it was in this section because the first box (Sensor relay combo) worked fine, but I see now that assumptions should be avoided.
-The delay at the end of 1MS is a way to constantly check that all the signals are low. It never stopped R1/O5 from functioning. When I raised the value it just seemed to make the target respond much slower to being hit so I set it to 1 and was going to explore that later.
-Everything compiles just fine, it's just that everything after the first IF statement doesn't respond to being hit. Though I know my code can be rough, I'm very new to this and some of the formatting confuses me. I can post the full version of the first code I posted but it's even messier.

Code: [Select]

//Laser Sensor Inputs
int R1=1;
int R2=2;
int R3=3;
int R4=4;


//Laser Sensor OutPuts
int O5=5;
int O6=6;
int O7=7;
int O8=8;

//Value of Sensor Reads
int val1;
int val2;
int val3;
int val4;

void setup() {
  //Pin Setup for the In and Outputs
  Serial.begin(9600);
  pinMode(R1, INPUT);
  pinMode(R2, INPUT);
  pinMode(R3, INPUT);
  pinMode(R4, INPUT);
  pinMode(O5, OUTPUT);
  pinMode(O6, OUTPUT);
  pinMode(O7, OUTPUT);
  pinMode(O8, OUTPUT);

}

void loop() {
 
 
  int val1 = digitalRead(R1);
  int val2 = digitalRead(R2);
  int val3 = digitalRead(R3);
  int val4 = digitalRead(R4); 
  //Lights
  if (val1 == HIGH){
    digitalWrite(O5,HIGH);
    delay (1000);
  }
  //Buzzer
  else if (val2 == HIGH){
    digitalWrite(O6,HIGH);
    delay (1000);
  }
  //Motor
  else if (val3 == HIGH){
    digitalWrite(O7,HIGH);
    delay (1000);
  }
  //Servo
  else if (val4 == HIGH){
    digitalWrite(O8,HIGH);
    delay (1000);
  }
  //All off
  else{
    digitalWrite(O5, LOW);
    digitalWrite(O6, LOW);
    digitalWrite(O7, LOW);
    digitalWrite(O8, LOW);
    delay(1);
    }       
  }

roguemike

Hmm, Ive tried both newly posted codes and they are having the same effect. Box 1s laser recognizes being hit and signals the relay to turn on, but all the other I/O after R1,O5 don't respond. Pulling my hair out, Ive tried everything and this should be so simple : (

aarg

If you want to test all of the conditions, remove the 'else's from the 'else if's... in an if...else if... else if... control structure, if the first test succeeds, the else part isn't performed, and so all the ifs following it are not performed.
  ... with a transistor and a large sum of money to spend ...
Please don't PM me with technical questions. Post them in the forum.

roguemike

Alright well, I have a "kinda" answer here. I ran the first code PCBBC posted but it still wouldnt work past the first IF Statement. I worked on it for another day and nothing worked. Well I remember hearing that pin 0 and 1 can be special so I tried the code but starting at pin 2. So moving off 0 and 1 and running pin 2-5 as inputs and 6-9 as outputs.
It seems to work now but it needs more testing. So I guess avoid pins 0 and 1 if you can? Something to give a shot at least. Hpe this helps someone someday.

pcbbc

It seems to work now but it needs more testing. So I guess avoid pins 0 and 1 if you can? Something to give a shot at least. Hpe this helps someone someday.
This is why you should provide complete sketches....

Pins 0 and 1 are the serial Tx and Rx pins on lots of boards (although I note that you do not tell us which board you are using). Unusable if you are using the Serial debug output at the very least. I do apologise that I used pin 1 in my post #5 example...

Although none of this explains why ONE relay works and ALL the others don't. The normal result of using pins 0 and 1 are that the things connected to those pins, and also the serial Tx/Rx, do not work. Other pins, independent of the first, should work as long as they do rely on input/output from the serial.

Your problem with the other pins is as arg says: you are using "else if" instead of starting a new "if". I thought I'd covered that fairly in depth in my original replay, and in my first sketch I corrected that mistake for you (unless of course you typed it in wrong). That does indeed appear the case in your post #6.

You also need to think about what happens if a laser hits more than one target, or moves from one target to another while those delays are happening. Those may not occur in your use case, but they're the kind of thing you need to start thinking about as a programmer for when they might.

roguemike

Well, I understood that I should have posted the full code after being corrected. Though I really should add, that this forum is going to have a lot of noobs like me asking basic questions and making basic mistakes, and many of the posts here were rather short on kindness to these basic mistakes. But when being helped its best not to dote, so I shrugged it off. But if we are reiterating here, I will explain further myself on that. I have no problems taking criticism and corrected course best I could after the fact, but that is never a way to tackle a simple mistake like a rookie like me posting a little mistake.

Stepping off my high horse though, I could tell what I was looking for and to explain the whole scenario would have been way to much information. As far as lasers hitting more than one target or other issues, I have thought though that, but I was limiting my scope for this issue, since this IF Statement issues was my only hurdle here. (I had segregated my other programs and tested them separately). As far as why only box 1 was reacting, past its input pin being pin1 I don't know. But moving off pin 1 I now have 2 test boxes working, I just haven't gotten the rest on yet, but I assume they will now work.

But credit where credit is due, they all work thanks your your code correction. As soon as I saw your code I thought, "Thats what I needed". I used it and to my surprise it didn't work and I was lost at that point. Until I just tried every crazy thing and just tried to move off pin 1, and poof.
So while I think bedside manner is important and something some posters might want to take into account, your posts and care on the matter were very helpful and I would still be lost without it. And I do really appreciate it.
So thank you, very much PCBBC. You provided the code and the formatting I was looking for and because of that I have learned more about coding in the process. I have also learned more about troubleshooting online and in programming forums. And never to assume : )

Robin2

Well, I understood that I should have posted the full code after being corrected. Though I really should add, that this forum is going to have a lot of noobs like me asking basic questions and making basic mistakes, and many of the posts here were rather short on kindness to these basic mistakes. But when being helped its best not to dote, so I shrugged it off. But if we are reiterating here, I will explain further myself on that. I have no problems taking criticism and corrected course best I could after the fact, but that is never a way to tackle a simple mistake like a rookie like me posting a little mistake.
I have looked back over this Thread and I see no sign of any posts being short of kindness. If there is a specific case that you are unhappy with please tell us which if is.


I accept that my Reply #1 was short - but it was a simple request. And you ignored it.



...R
Two or three hours spent thinking and reading documentation solves most programming problems.

johnwasser

Are you trying to run this on an Arduino UNO?  If so, you have a conflict on Pin 1 (a.k.a. Serial TX).
Code: [Select]
//Laser Sensor Inputs
int R1=1;


void setup() {
  //Pin Setup for the In and Outputs
  Serial.begin(9600);
  pinMode(R1, INPUT);
}


If you want to use Pin 0 or Pin 1 as a data pin, DON'T call Serial.begin().
Send Bitcoin tips to: 1G2qoGwMRXx8az71DVP1E81jShxtbSh5Hp
See who has no social life: https://forum.arduino.cc/index.php?action=stats :)

Go Up