If statements not executing properly

Hi all!
i've recently got back into playing with the arduino after a very long time. I had a motorized fader laying around and felt like playing with it a bit.
the code is a bit crude and not done/optimized by any means but ive ran into something that i cant seem to figure out.
under the section marked as "ch recall settings" i have a few if statements that check the readout of analog in 0 (called fader) and compares it to a previously stored integer that was updated when a new "channel" is selected of which i currently have 3.
my problem is that the "ch recall settings" if statement only works once, which is when the code is first started and channel 2 is selected which is initialized as analogue read value 800.
the fader moves to position (aproximately for now) and then stops moving, updates the fader restore value to false so it does not run the code constantly and all is well.
however if i then switch to another channel, it correctly updates all values, every integer is correct but it does not start the movement of the fader again.
does anyone see any "obvious" thing that im missing right now?
ive been confused for long enough :wink:
id like to hear it!
code provided below

////////////////////////////////////// All Input / Output Pins (except A0) >
const int buttonPin[3] = {2,3,4};
const int ledPin[3] = {5,6,7};
const int fUp = 8;
const int fDown = 9;
////////////////////////////////////// integers for ch recall memory >
int selectedCh = 0;
int prevCh = 1;
int muteCh[3] = {0,0,0};
boolean faderRestore[3] = {false,false,false};
int faderPos[3] = {3,800,3};
////////////////////////////////////// integers for ch interactions >
int pushState[3] = {0,0,0};
int pushStateNext[3] {0,0,0};
int pushStatePrev[3] {0,0,0};
int ledState = LOW;
int pushPrev = LOW;
int faderPreMove = 0;
int faderReturn = 0;
////////////////////////////////////// Global debounce/Timer >
unsigned long globalDebounce = 20;
unsigned long debounceTimer[3] = {27,28,29};
unsigned long globalBlink = 150;
unsigned long ledBlink[3] = {24,25,26};
//////////////////////////////////////




void setup() {
  for (int i=0; i<3; i++) {
    pinMode(buttonPin[i], INPUT);
}
for (int i=0; i<3; i++) {
    pinMode(ledPin[i], OUTPUT);
}
pinMode(fUp,OUTPUT);
pinMode(fDown,OUTPUT);
prevCh = selectedCh;
analogWrite(ledPin[0],50);
delay(200);
digitalWrite(ledPin[0],LOW);
digitalWrite(fDown,LOW);
}

void loop() {


////////////////////////////////////// Rename Integers for live readings  
int fader = analogRead(A0);
int prevPress = digitalRead(buttonPin[0]);
int nextPress = digitalRead(buttonPin[1]);
int mutePress = digitalRead(buttonPin[2]);
fader = map(fader,0,1022,1,1021); // remap for les wonky extreme readings (needs buffering)

//////////////////////////////////////Led Button Interactions
if ((millis() - ledBlink[0]) < globalBlink) {
  analogWrite(ledPin[0],20);
} else {
  digitalWrite(ledPin[0],LOW);
}
if ((millis() - ledBlink[1]) < globalBlink) {
  analogWrite(ledPin[1],40);
} else {
  digitalWrite(ledPin[1],LOW);
}if ((millis() - ledBlink[2]) < globalBlink) {
  analogWrite(ledPin[2],30);
} else {
  digitalWrite(ledPin[2],LOW);
}
////////////////////////////////////// Ch recall Settings
if (faderRestore[selectedCh] == true && fader < faderPos[selectedCh] || fader > faderPos[selectedCh]) {
  if (fader < faderPos[selectedCh]) {
    digitalWrite(fUp,HIGH);
 if (fader == faderPos[selectedCh]) {
      digitalWrite(fUp,LOW);
      faderRestore[selectedCh] = false;
    }
  }   
      if (faderRestore[selectedCh] == true && fader > faderPos[selectedCh]) {
    digitalWrite(fDown,HIGH);   if (faderRestore[selectedCh] == true && fader == faderPos[selectedCh]) {
      digitalWrite(fDown,LOW);
      faderRestore[selectedCh] = false;
       } 
    }
}



////////////////////////////////////// Ch Next 
  if (nextPress != pushStateNext[selectedCh]) {
    debounceTimer[selectedCh] = millis();
  }
    if ((millis() - debounceTimer[selectedCh]) > globalDebounce) {
      if (nextPress != pushState[selectedCh]) {
        pushState[selectedCh] = nextPress;
        if (pushState[selectedCh] == HIGH) {
          if (selectedCh <= 1) {
            prevCh = selectedCh;
            faderPos[selectedCh] = fader;
            selectedCh++;
            faderRestore[selectedCh] = true;
            pushStateNext[prevCh] = nextPress;
            ledBlink[1] = millis();
            delay(100); ///////////////////////////////////// Needs Fixing (to prevent pushstate issues)
          }
        }
      }
    }
pushStateNext[selectedCh] = nextPress;

////////////////////////////////////// Ch Prev 

  if (prevPress != pushStatePrev[selectedCh]) {
    debounceTimer[selectedCh] = millis();
  }
    if ((millis() - debounceTimer[selectedCh]) > globalDebounce) {
      if (prevPress != pushState[selectedCh]) {
        pushState[selectedCh] = prevPress;
        if (pushState[selectedCh] == HIGH) {
          if (selectedCh >= 1) {
            prevCh = selectedCh;
            faderPos[selectedCh] = fader;
            selectedCh--;
            faderRestore[selectedCh] = true;
            pushStatePrev[prevCh] = prevPress;
            ledBlink[0] = millis();
            delay(100); ///////////////////////////////////// Needs Fixing (to prevent pushstate issues)
          }
        }
      }
    }
pushStatePrev[selectedCh] = prevPress;


////////////////////////////////////// Troubleshooting SERIAL BUS
//Serial.print(" pushstate ");
//Serial.print(pushStatePrev[1]);
Serial.print(" FaderRestore ");
Serial.print(faderRestore[2]);
Serial.print(" FaderPosOld ");
Serial.println(faderPos[2]);
//Serial.print(" Fader ");
//Serial.print(fader);
//Serial.print(" ch ");
//Serial.println(selectedCh);


}

Hi @d_p_b ,
Welcome to the forum..

just a quick look..

this is being used as a millis timer var in the if's??
but you never reset them and the way their init'd looks like it's a pin array??

you need to reset the var ledBlink[0] = millis();
and shouldn't that be > instead of <??

good luck.. ~q

thanks for the welcome!

Oh yeah thats a little beauty error, i used to set all of these to 0 with a for loop in setup, ive since removed the for loop[ in setup but i see ive forgotten to set these to 0 initially. since they get assigned millis() during a button press the value isnt really used besides initialization, but i agree its messy and it is now all set to 0 :slight_smile:

ive set this to less then instead of more because i use it to blink an led for the duration of globalblink(50ms) it basically functions as a confirmation that the button has been pressed and this function is working decently so far!

ok, sorry, bit confusing..

is the above if the issue??
can't be as simple as adding some quotes..

if ((faderRestore[selectedCh] == true && fader < faderPos[selectedCh] )|| fader > faderPos[selectedCh])

be nice if it was..

~q

nope unfortunately it isnt, although admittedly this if statement is a bit crude since its basically functioning as a "not equal to / !=" statement (which also doesnt work..... tried that aswell)
in theory the only neccesary requirement for the if statement to run could be the faderRestore value, since it should always be turned to false as soon as this if statement is run. it doesnt do that either however, just like moving the fader it only does it properly once when the code is just started.
after that it just isnt used entirely

time to add serial.begin to your setup and some debug prints in the sketch to see what is going on, better than guessing..

~q

if it's less than how could it also be equal??
~q

need to format..
here's another one..

    if (faderRestore[selectedCh] == true && fader > faderPos[selectedCh]) {
      digitalWrite(fDown, HIGH);  
       if (faderRestore[selectedCh] == true && fader == faderPos[selectedCh]) {
        digitalWrite(fDown, LOW);
        faderRestore[selectedCh] = false;
      }
    }

if it's greater than with another if equal??
maybe me old eye are reading it wrong..

~q

yeah i see i had that removed in the posted version, it is in my current version that im troubleshooting, all values that are supposed to make the thing move read out as valid. it updates values properly, does not crash or produce other unexpected results. the only thing it doesnt do is move the fader more then once

i have not used actual stored positions for the fader, it doesnt use a stepper motor just a regular dc motor.
so it kindoff reads as follows:

 if (fader **(Current analog Read value)** < faderPos[selectedCh] **(stored analogue read value when channel was last switched)** {
    digitalWrite(fUp,HIGH); **(moves the fader and thus the analog read up since its a moving potentiometer)**
 if (fader == faderPos[selectedCh]) { **(as soon as the analog read value equals the stored value disable the motor so that the fader halts in position at the same readout)**
      digitalWrite(fUp,LOW);
      faderRestore[selectedCh] = false;
    }
  }

i hope that makes sense
it also does not matter what channel i start the script at, if i set the selected channel as 0,1,2 i can switch to 1 channel succesfully with correct movement and after that it does not move the fader anymore.

no, it doesn't..
the if inside will never get executed as fader won't be equal when it's allowed to enter the outer if..

~q

agreed, i think it works by accident due to clock rate of the arduino allowing it to briefly fire when the analog read gets updated. it functions as intended, but probably only by accident.
i moved the second if outside of the first, it still functions the same however, it stops the fader and updates the faderRestore value to false.
but still, only once

did you do them both??
identified 2 so far, stopped looking..
~q

yes, they are now both outside.
currently looks like

////////////////////////////////////// Ch recall Settings
if (faderRestore[selectedCh] == true && fader != faderPos[selectedCh])
 {
  if (fader < faderPos[selectedCh]) {
    digitalWrite(fUp,HIGH);
  }   
    if (fader == faderPos[selectedCh]) {
        digitalWrite(fUp,LOW);
        faderRestore[selectedCh] = false;
    }
    if (fader > faderPos[selectedCh]) {
        digitalWrite(fDown,HIGH);   
    }
    if (fader == faderPos[selectedCh]) {
        digitalWrite(fDown,LOW);
        faderRestore[selectedCh] = false;
    } 
}

cool..
the buttons..
hooked up with external pull down resistors??
haven't implemented mute yet??
~q

yeah, all buttons are pulled down with 10k to ground from the related input pin from arduino. other pin goes to 5v rail.
Leds all have current limiting resistors and im using l2930 for driving the motor inside the fader.
can draw up a schematic if thats useful, i dont think its hardware related tho unfortunately, i think it might be related to the integer arrays ive used for the first time, since i have been able to move the fader with memory function (although more limited in nature) with the exact same wiring (have been messing with this setup doing different things for a few days)

typeconst int buttonPin [3] = {2,3,4};
const int ledPin [3] = {5,6,7};
const int f1Up = 8;
const int f1Down = 9;
//////////////////////////////////////
int muteCh1;
int muteCh2;
int muteCh3;
int faderPreMove1 =0;
int faderPreMuteOff1 = 0;
int faderPreMove2 =0;
int faderPreMuteOff2 = 0;
int faderPreMove3 =0;
int faderPreMuteOff3 = 0;
int selectedCh;
//////////////////////////////////////
int pushPrev1 = LOW;
int pushState1;
int ledState1 = LOW;
int pushPrev2 = LOW;
int pushState2;
int ledState2 = LOW;
int pushPrev3 = LOW;
int pushState3;
int ledState3 = LOW;
//////////////////////////////////////
unsigned long globalDebounce = 20;
unsigned long debounceTimer;
//////////////////////////////////////

void setup() {
Serial.begin(9600);
for (int i=0; i<3; i++) {
    pinMode(buttonPin[i], INPUT);
}
for (int i=0; i<3; i++) {
    pinMode(ledPin[i], OUTPUT);
}
pinMode (A0, INPUT);
pinMode (f1Up, OUTPUT);
pinMode (f1Down, OUTPUT);
if (analogRead(A0) < 1020) {
  digitalWrite(f1Up,HIGH);
} delay(800);
digitalWrite(f1Up,LOW);
digitalWrite(f1Down,HIGH);
delay(800);
digitalWrite(f1Down,LOW);
selectedCh = 1;
}

void loop() {
int fader1 = analogRead(A0);
int read1 = digitalRead(buttonPin[0]);
int read2 = digitalRead(buttonPin[1]);
int read3 = digitalRead(buttonPin[2]);

  if (read3 != pushPrev3) {
  debounceTimer = millis();
}
  if ((millis() - debounceTimer) > globalDebounce) {
    if (read3 != pushState3) {
      pushState3 = read3;
      if (pushState3 == HIGH) {
        ledState3 = !ledState3;
        muteCh1 = !muteCh1;
      }
    }
  }
if (ledState3 == HIGH) {
  digitalWrite(ledPin[2],HIGH);
} else {
  digitalWrite(ledPin[2],LOW);
}
pushPrev3 = read3;
///////////////////////////////////////// Fader Down When Muted + UPDATE Fader Prev
if (muteCh1 == HIGH) {
  if (pushState3 == HIGH) {
    faderPreMove1 = fader1;
  } else {
    if (fader1 > 20) {
      digitalWrite(f1Down,HIGH);
    } else {digitalWrite(f1Down,LOW);
    faderPreMuteOff1 = 1;}
  }
}
//////////////////////////////////////// Fader return to pre mute position when unmuted
if (muteCh1 == LOW && faderPreMuteOff1 == 1 && fader1 < faderPreMove1) {
  digitalWrite(f1Up,HIGH);
} 
if(fader1 >= faderPreMove1 || fader1 >= 1010) {
  faderPreMuteOff1 = 0;
  digitalWrite(f1Up,LOW);
}
///////////////////////////////////// Ch Select +
if (read2 != pushPrev2) {
  debounceTimer = millis();
}
  if ((millis() - debounceTimer) > globalDebounce) {
    if (read2 != pushState3) {
      pushState2 = read2;
      if (pushState2 == HIGH) {
        selectedCh++;
        
      }
    }
  }









//Serial.print( " FaderPM OFF ");
//Serial.print(faderPreMuteOff1);
//Serial.print( " MUTECH1 ");
//Serial.print(muteCh1);
//Serial.print( " fader ");
//Serial.print(fader1); 
//Serial.print( " fader PRE MUTE ");
//Serial.println(faderPreMove1);
} or paste code here

this also writes to an integer for a previous analogue read value and also returns the fader to that value when a button is pressed.
i was initially going to expand this script to deal with multiple memory settings but found the lack of integer arrays and general inflexibility of this code for expansion reason to try and rewrite it.
so this script also has some unused functions

i think the button code just needs a little tending too..
first the debounceTimer's belong to the button not the selectedCh..
And as you used arrays (nice job) you can combine the button readings into one section quite easily..
look how i read the buttons here..
something like that i'm thinking..
~q

ah yes, i agree the debounceTimer does not correspond to the selectedCh and should instead just be related to the button being pressed, it didn't break the buttons so far but ill definitely fix that since the entire reason i wanted numerous timers is to deal with multiple inputs at once which now does not work at all!
ill have a look at what youve made, looks a little above my current paygrade which is great, i enjoy seeing new ways to approach self made problems :stuck_out_tongue:
thanks so far though!

I'm taking a stab at it for you..
give me a few..
~q

bit old school added a few defines..

#define BTN_PREV 0
#define BTN_NEXT 1
#define BTN_MUTE 2

then for the buttons maybe give this a try..

  for (int i = 0; i < sizeof(buttonPin); i++) {
    if ((millis() - debounceTimer[i]) > globalDebounce) {
      byte btn = digitalRead(buttonPin[i]);
      if (btn != pushState[i]) {
        //start debouncing for this button
        debounceTimer[i] = millis();
        //remember state
        pushState[i] = btn;
        if (btn == HIGH) {
          //got a debounced button press..
          switch (btn) {
            case BTN_PREV: if (selectedCh <= 1) {
                prevCh = selectedCh;
                faderPos[selectedCh] = fader;
                selectedCh++;
                faderRestore[selectedCh] = true;
                pushStateNext[prevCh] = nextPress;
                ledBlink[1] = millis();
              }
              break;//prev button
            case BTN_NEXT: if (selectedCh >= 1) {
                prevCh = selectedCh;
                faderPos[selectedCh] = fader;
                selectedCh--;
                faderRestore[selectedCh] = true;
                pushStatePrev[prevCh] = prevPress;
                ledBlink[0] = millis();
              }
              break;//next button
            case BTN_MUTE: break;//mute button
          }
        }
      }
    }
  }

change from const int to const byte, save some memory..

~q

i see a bug..
nextPress and prevPress should be replaced with btn and then can be removed..
i just copied that code up into the switch..

~q