Problems Using If Statements

Hi, hope someone can help please.

I'm making a small intervalometer for a DSLR and I want to be able to switch between six fixed values using a rotary switch.
I've never used IF statements before and i'm having trouble with them.

I'n the code below, when the loop gets to the IF statements at the bottom, it just ignores them completely.
Am I doing something wrong or, can't I use IF statements in this way.

Thanks
Paul

// Basic intervalometer for DSLR.

// Pins 4 and 5 connected to the camera cable via two optocouplers.

// Action 1 (FOCUS) is turned on for one second to allow the cama=era to auto focus.

// Action 2 (SHUTTER) is then turned on for one second to allow for the shutter to activate.

// The interval between shots is controlled by the six position switch and ground connected to pins 6 to 11.


#define FOCUS  4             // Define the Focus Pin as Digital Pin 4.             
#define SHUTTER  5           // Define the Shutter Pin as Digital Pin 5.          

#define ONESEC 6             // Define the six switch pins.
#define TENSEC 7
#define THIRTYSEC 8
#define SIXTYSEC 9
#define THIRTYMINS 10
#define SIXTYMINS 11


void setup()

{    
  
 // Initialise the Arduino data pins for OUTPUT.

  pinMode(FOCUS, OUTPUT);         // Initialise the Arduino data pins for OUTPUT.
  pinMode(SHUTTER, OUTPUT);
 
  pinMode(ONESEC, INPUT);         // Initialise the Arduino data pins for INPUT.
  digitalWrite(ONESEC, HIGH);     // Attach internal pull-up resistors.
  pinMode(TENSEC, INPUT);
  digitalWrite(TENSEC, HIGH);
  pinMode(THIRTYSEC, INPUT);
  digitalWrite(THIRTYSEC, HIGH);
  pinMode(SIXTYSEC, INPUT);
  digitalWrite(SIXTYSEC, HIGH);
  pinMode(THIRTYMINS, INPUT);
  digitalWrite(THIRTYMINS, HIGH);
  pinMode(SIXTYMINS, INPUT);
  digitalWrite(SIXTYMINS, HIGH);


}

 

 void loop()

{
    
  
   digitalWrite(FOCUS,HIGH);           // Turns optocoupler 1 on, activating the auto focus.

   delay(1000);                        // Wait 1 second.

   digitalWrite(SHUTTER,HIGH);         // Turns optocoupler 2 on, activating the shutter.

   delay(1000);                        // Wait 1 second

   digitalWrite(FOCUS,LOW);            // Turns optocoupler 1 off.
   
   digitalWrite(SHUTTER,LOW);          // Turns optocoupler 2 off.

   if (ONESEC == LOW) {                // Delays the loop a fixed time, depending on which switch pin is low.
     delay(1000);}
 
   if (TENSEC == LOW) {                // Delays the loop a fixed time, depending on which switch pin is low.
   delay(10000);}
   
   if (THIRTYSEC == LOW) {             // Delays the loop a fixed time, depending on which switch pin is low.
   delay(30000);}
   
   if (SIXTYSEC == LOW)  {            // Delays the loop a fixed time, depending on which switch pin is low.
   delay(60000);}
   
   if (THIRTYMINS == LOW) {           // Delays the loop a fixed time, depending on which switch pin is low.
   delay(1800000);}
   
   if (SIXTYMINS == LOW) {             // Delays the loop a fixed time, depending on which switch pin is low.
   delay(3600000);}

 }

You are not reading the state of the input pins anywhere in the code.

By the way, there is a much neater way to read the switches using an array of pin numbers and a for loop, but we can come back to that once the immediate problem is solved. You would also be better off not using delay() so that you can do other things, such as aborting the wait once started, but that can come later too.

First, in your code:

   if (ONESEC == LOW) {                // Delays the loop a fixed time, depending on which switch pin is low.
     delay(1000);}
 
   if (TENSEC == LOW) {                // Delays the loop a fixed time, depending on which switch pin is low.
   delay(10000);}
   
   if (THIRTYSEC == LOW) {             // Delays the loop a fixed time, depending on which switch pin is low.
   delay(30000);}
   
   if (SIXTYSEC == LOW)  {            // Delays the loop a fixed time, depending on which switch pin is low.
   delay(60000);}
   
   if (THIRTYMINS == LOW) {           // Delays the loop a fixed time, depending on which switch pin is low.
   delay(1800000);}
   
   if (SIXTYMINS == LOW) {             // Delays the loop a fixed time, depending on which switch pin is low.
   delay(3600000);}

it appears that only one of the pins is supposed to be LOW at any one time. Therefore, if that’s the case, once you find the one that is LOW, there’s no point in looking at the rest of them. For example, if pin ONESEC is LOW, there’s no point in checking the others. This suggests you want to avoid further checking once you find the desired state. This requires a “cascading if statement”, such as:

   if (ONESEC == LOW) {                // Delays the loop a fixed time, depending on which switch pin is low.
     delay(1000);
   } else { 
      if (TENSEC == LOW) {                // Delays the loop a fixed time, depending on which switch pin is low.
         delay(10000);
      } else {
         if (THIRTYSEC == LOW) {             // Delays the loop a fixed time, depending on which switch pin is low.
            delay(30000);
         } else {
            if (SIXTYSEC == LOW)  {            // Delays the loop a fixed time, depending on which switch pin is low.
               delay(60000);
            } else {
               if (THIRTYMINS == LOW) {           // Delays the loop a fixed time, depending on which switch pin is low.
                  delay(1800000);
               } else {
                  delay(3600000);               // Don't need if, since this is the last possible state...I assume.
              }

Note that once the actual pin state is found, all others are skipped…saves time.

I hate cascading if statements.

I would probably code this as a two dimension array, but will do two single arrays to make it simple:

// Your preprocessor directives...

int pins[] = {ONESEC , TENSEC, THIRTYSEC, SIXTYSEC, THIRTYMINS, SIXTYMINS};
long delay = {1000, 10000, 30000, 60000, 1800000, 3600000};

// some code...

 void loop()
{
    int i;

    for (i = 0; i < sizeof(pins)/sizeof(int); i++) {   // Loop through the pins...
       if (pin[i] == LOW) {                            // Is one LOW...
           delay(delays[i]);                           // ...yep, turn on delay()
           break;                                      // Get outta the for loop and do...whatever...
       }
    }
}

I haven’t tried this, but something like this should work…

#define ONESEC 6

So, ONESEC will be replaced by 6 in the code

int pins[] = {ONESEC , TENSEC, THIRTYSEC, SIXTYSEC, THIRTYMINS, SIXTYMINS};Will becomeint pins[] = {6, TENSEC, THIRTYSEC, SIXTYSEC, THIRTYMINS, SIXTYMINS};
Then when i equals 0      if (pins[i] == LOW) will test if 6 equals LOW
Shouldn't that be       if (digital.Read(pins[i]) == LOW)?

UKHeliBob: Yep.

I’d write a function to read the pins and return a corresponding value. Then I could have an array of delays. Then I could select the delay duration like this:

delay(delayArray[readSwitchFunction()]);

With an array of pins the function is simple.

pin[] = {6, 7, 8, 9, 10, 11};
const int pinCount = sizeof(pin) / sizeof(pin[0]);

int readSwitchFunction(void)
{
     for (int i = 0; i < pinCount; i++)
     {
          if (digitalRead(pin[i]) == LOW) return i;  // check for first LOW value
     }
}

Thank you all very much for your time, I really do appreciate it.

Unfortunately, being totally new to this, most of your answers mean very little to me I'm afraid.

With just little snippets of code, it's very difficult for me to work out what goes where.

Please don't take this as criticism or a complaint, I do greatly appreciate the time and trouble you have all taken to help me.

Thank you
Paul

What's hanging you up? The arrays? Making your own functions?

Learning to use arrays is a must in programming.

We've been sharing concepts that you can take your own code. We haven't been sharing code that you can use directly.

I understand you are new to programming, and would like to make some suggestions that I hope make sense. When your device in use, will you be changing the switch with the power applied? This might lead to some undesirable results. If you are in the longest delay period and want to change to anything slower, the delay has to time out before the program ever notices the switch has changed. That could be close to an hour. If you are going from the shortest delay period to one that is longer, if you don't move the switch quickly enough you might have an intermediate timer start before you get the one you want. You would have to wait for the intermediate one to time out. A power off/on cycle or a reset cycle will correct this of course. Another option could be to use one more input as a "start/reset" function. By pressing it you cause the program to change to the timing of whatever the switch is at. Or you could write the program to look at the inputs frequently and look for a change. If there is a change, then alter the timing. In either of these cases you won't be able to use delay() functions. You would need to look at and record the start time of the timer, then repeatedly look to see if enough time has gone by to say the timer has timed out. Make sense? There is no reason to do what I suggest if you are going to rely on reset or power cycles, but I thought I'd pass on these ideas in the hopes they might help.
Lyle

Fair enough Jimmy, point taken and thank you.

The switch will be set before power on.
The delay will be set by the switch, powered on and remain on until the job is finished.

Thanks
Paul

Your code is not doing what you think it’s doing.

#define ONESEC 6      

if (ONESEC == LOW) 
{
  delay(1000);
}

The evaluates to “if (6 == LOW)”, which it never does, because the constant LOW equates to zero. This is why none of your if() statements actually does anything.

What you need to do is digitalRead() each of the pins to see which one is LOW, as such:

if (digitalRead(ONESEC) == LOW)
{
  delay(1000);
}

Two other suggestions. First, your code will be much more readable if you use Auto-format on it. CTRL+T. Second, you can enable the internal pullups in a single line like this:

pinMode(pinNum, INPUT_PULLUP);

This is identical to what you’re doing by following the INPUT pinMode with a HIGH digitalWrite, but it’s one line instead of two, and IMO a little more clear to read.

It might be a good exercise to write a sketch just for your switch. Then how it will work with everything else should become more apparent. Joshua makes some good points lets put them to use.

This should be pretty easy to follow. It will (should :D) print out the new delay value when you change your switch.

const long BAUD_RATE = 9600;   // set your preferred  rate here
const int SWITCH_PIN[] = {6, 7, 8, 9, 10, 11};  
const int PIN_COUNT = sizeof(SWITCH_PIN) / sizeof(SWITCH_PIN[0]);
const unsigned long INTERVAL[] = {1000, 2000, 4000, 8000, 16000, 32000};

// notice that it's the index, i that we want
int readSwitchFunction()
{
  for (int i = 0; i < PIN_COUNT; i++) // start looping through switch pins
  {
    if (digitalRead(SWITCH_PIN[i]) == LOW) return i;  // check for first LOW value
  }
}

void setup() 
{
  Serial.begin(BAUD_RATE);
  for (int i = 0; i < PIN_COUNT; i++)
  {
    pinMode(SWITCH_PIN[i], INPUT_PULLUP); // set pinmode
  }
}

void loop() 
{
  static unsigned long lastInterval = 0;  // a static variable will keep its value
  unsigned long interval = INTERVAL[readSwitchFunction()];  // get interval value each loop
  if (interval != lastInterval) // act only when interval changes
  {
    lastInterval = interval; // save this value for next loop
    Serial.println(interval);  // print results
  }
}

Colaboy:
The switch will be set before power on.
The delay will be set by the switch, powered on and remain on until the job is finished.

Thanks
Paul

This may be overkill then, but you might consider making it that once a value is chosen after power up, you stop looking at the inputs. This is probably unlikely, but if you change the switch inadvertently with the unit powered you could alter the timing to a period that you're not wanting. Kind of what I suggested earlier, but for a different reason :slight_smile: Probably not necessary, but it does make it safer in some respects.

Lyle