if statement for checking digitalRead not executing

I am debouncing and reading value from 4 different buttons(toggle button,SPST) and here is fragment for one of the buttons

void swytch_control1() {
  char a;
  static unsigned long last_check_time = 0;
  unsigned long check_time = millis();
  if (digitalRead( swytch_in[1]) == HIGH) {
    if (check_time - last_check_time > 300) {
      a = '0';
      last_check_time = check_time;
    }
  }
  else if (digitalRead(swytch_in[1]) == LOW) {
    if (check_time - last_check_time > 300) {
      a = '1';
      last_check_time = check_time;
    }
  }
  if (hardware_state[1] != a) {
    if (a == '0')
      digitalWrite( swytch_out[1], HIGH);
    else if (a == '1')
      digitalWrite( swytch_out[1], LOW);
    board_state[1] = a;
    hardware_state[1] = a;
    set[1] = 0;
    sets[1] = '0';
    Serial.println("1"); //log
    Serial.println(a); //log
  }
}

in the main loop I am making calls to these functions one after another as
swytch_control1();
swytch_control2();
swytch_control3();
swytch_control4();

but the problem is if (digitalRead( swytch_in[1]) == HIGH) and else if (digitalRead(swytch_in[1]) == LOW) are skipped and arduino enters the if loop right after it. If I induce a delay of 1000ms after all 4 function calls the code works fine. I do not want to induce any delay as I am reading data from the serial as well (in another function). Please suggest any changes that can be made.
PS: Sketch uses 10,330 bytes (32%) of program storage space. Maximum is 32,256 bytes.
Global variables use 570 bytes (27%) of dynamic memory, leaving 1,478 bytes for local variables. Maximum is 2,048 bytes.

Please post all your code, even if it is repetitive, as the error is seldom where you think it is.

  if (digitalRead( swytch_in[1]) == HIGH) {
  }
  else if (digitalRead(swytch_in[1]) == LOW) {
   }
  }

Remove the if after the else - if it has failed the 'HIGH' test, then digitalRead can only be 'LOW'.

swytch_in[1] cannot be HIGH and LOW at the same time, try changing:

else if (digitalRead(swytch_in[1]) == LOW) {

To:

else{
      if (check_time - last_check_time > 300) {
      a = '1';
      last_check_time = check_time;
      }
    }

outsider:
swytch_in[1] cannot be HIGH and LOW at the same time, try changing:

else if (digitalRead(swytch_in[1]) == LOW) {

To:

else{

if (check_time - last_check_time > 300) {
      a = ‘1’;
      last_check_time = check_time;
      }
    }

Intitialy it was if and else only. I changed it to see whether or not it entered the else if loop when the button was on.
And the same thing happens with the if else too.

whole code

byte swytch_in[5] = {0, 5, 6, 7, 8};
byte swytch_out[5] = {0, 9, 10, 11, 12};
boolean set[14];
unsigned long check_time[6];
String readString, hardware_state = "H00000";
String board_state = "S0000";
String sets = "T00000";

void setup() {
  Serial.begin(9600);
  for ( byte i = 1; i <= 4; i++) {
    pinMode( swytch_in[i], INPUT_PULLUP);
    pinMode( swytch_out[i], OUTPUT);
    digitalWrite( swytch_out[i], HIGH);
  }
}  
void loop() {
  bt_collect();
  swytch_control1();
  swytch_control2();
  swytch_control3();
  swytch_control4();
}
void bt_collect() {
  while (Serial.available()) {
    delay(10);
    char c = Serial.read();
    readString += c;
  }
  readString.trim();
}

void swytch_control1() {
  char a  ;
  static unsigned long last_check_time = 0;
  unsigned long check_time = millis();
  delay(10);
  if (digitalRead( swytch_in[1]) == HIGH) {
    if (check_time - last_check_time > 300) {
      a = '0';
      last_check_time = check_time;
    }
  }
  else {
    if (check_time - last_check_time > 300) {
      a = '1';
      last_check_time = check_time;
    }
  }
  if (hardware_state[1] != a) {
    if (a == '0')
      digitalWrite( swytch_out[1], HIGH);
    else if (a == '1')
      digitalWrite( swytch_out[1], LOW);
    board_state[1] = a;
    hardware_state[1] = a;
    set[1] = 0;
    sets[1] = '0';
    Serial.println("in 1");
    Serial.println(a);
  }
}
void swytch_control2() {
  char a  ;
  static unsigned long last_check_time = 0;
  unsigned long check_time = millis();
  if (digitalRead( swytch_in[2]) == HIGH) {
    if (check_time - last_check_time > 300) {
      a = '0';
      last_check_time = check_time;
    }
  }
  else {
    if (check_time - last_check_time > 300) {
      a = '1';
      last_check_time = check_time;
    }
  }
  if (hardware_state[2] != a) {
    if (a == '0')
      digitalWrite( swytch_out[2], HIGH);
    else if (a == '1')
      digitalWrite( swytch_out[2],  LOW);
    board_state[2] = a;
    hardware_state[2] = a;
    set[2] = 0;
    sets[2] = '0';
    Serial.println("in 2");
    Serial.println(a);
  }
}
void swytch_control3() {
  char a  ;
  static unsigned long last_check_time = 0;
  unsigned long check_time = millis();
  if (digitalRead( swytch_in[3]) == HIGH) {
    if (check_time - last_check_time > 300) {
      a = '0';
      last_check_time = check_time;
    }
  }
  else{
    if (check_time - last_check_time > 300) {
      a = '1';
      last_check_time = check_time;
    }
  }
  if (hardware_state[3] != a) {
    if (a == '0')
      digitalWrite( swytch_out[3], HIGH);
    else if (a == '1')
      digitalWrite( swytch_out[3],  LOW);
    board_state[3] = a;
    hardware_state[3] = a;
    set[3] = 0;
    sets[3] = '0';
    Serial.println("in 3");
    Serial.println(a);
  }
}
void swytch_control4() {
  char a ;
  static unsigned long last_check_time = 0;
  unsigned long check_time = millis();
  if (digitalRead( swytch_in[4]) == HIGH) {
    if (check_time - last_check_time > 300) {
      a = '0';
      last_check_time = check_time;
    }
  }
  else {
    if (check_time - last_check_time > 300) {
      a = '1';
      last_check_time = check_time;
    }
  }
  if (hardware_state[4] != a) {
    if (a == '0')
      digitalWrite( swytch_out[4], HIGH);
    else if (a == '1')
      digitalWrite( swytch_out[4],  LOW);
    board_state[4] = a;
    hardware_state[4] = a;
    set[4] = 0;
    sets[4] = '0';
    Serial.println("in 4");
    Serial.println(a);
  }

}

Output when all 4 on constantly looping as (tx blinking all the time)
in 1

in 2

in 3

in 4

in 1
1
in 2
1
in 3
1
in 4
1

Your if … else if usage cannot be source of your problem.
It is not necessary to check twice but still logical.
I am just puzzled why you are using boolean values and characters for similar function - to alternate between two states.
Is that by design or just a requirement to use different method?(school project?)

I am also assuming this is a first version of your code and you are well aware that lots of code can be simplified.
For example the timing check is common to both true and false conditions , so is the last_check_time = check_time;
You if … else code actually only toggles the a variable if it was a boolean and not character.

But again, that is not the problem.

Start by running only one of these swytch_control4() functions, the problem is probably in loopl() function and in wrong usage of time interval.

Of course these swytch_controlX() functions can be replaced by one passing parameters to it.
But make one work first than tackle the build of common function.

Good luck.
Jim

Your variable 'a' in the swytch_ functions is uninitialized and hence can have ANY value. It will only get a value assigned if a certain time has passed.

So when the code gets to [i]if (hardware_state[2] != a) {[/i], 'a' is not necessarily '0' or '1'. That is why your first lines of the output show the empty lines.

Is there some fundamental difference between swytch_control1(), swytch_control2(), etc. that I am not seeing? All I see is that each uses a different static variable to hold the last time it was called, and uses a different pin number. One function with an index argument and an array of pin numbers and last times would get rid of 75% of your code.

Unless you are being paid by the line of code, get rid of the excess.

As stated earlier by sterretje, in the line

if (hardware_state[1] != a)

‘a’ is uninitialised if the timer has not expired. You should change the variable declaration to initialise ‘a’ to a value that makes sense, or, simply put the time check sat the start of the function and exit the function if it is not time to do anything.

if (check_time - last_check_time < 300)
  return;

PaulS:
Of course these swytch_controlX() functions can be replaced by one passing parameters to it.
But make one work first than tackle the build of common function.

Yes of course it was ending up like that. I had passed arguments into it initially I just wrote it with separate functions to check whether the abrupt behavior was due to the parameters passed.

PaulS:
Is there some fundamental difference between swytch_control1(), swytch_control2(), etc.
Unless you are being paid by the line of code, get rid of the excess.

I have already answered jim for the same. Did it just check after N number of unsuccessful tries. Sometime changing the code and rewriting it hepls. May be it wasn’t the case here.

So here lied the problem

At this very line
unsigned long x =check_time - last_check_time;
if(x<300){…}
works

and the updated code is like

// called inside the loop

 for (byte i = 1; i <= 5; i++) {
    check_time[i] = millis();
    unsigned long int x = check_time[i] - last_check_time[i];
    if (i != 5) {
      if (digitalRead( swytch_in[i]) == HIGH) {
        if ( x > 200) {
          a = '0';
          last_check_time[i] = check_time[i];
          swytch_control(a, i);
        }
      }
      else {
        if (x > 200) {
          a = '1';
          last_check_time[i] = check_time[i];
          swytch_control(a, i);
        }
      }
    }

// and the function

void swytch_control(char a, int i) {
  if (b[i]  != a) {
    if (a == '0')
      digitalWrite( swytch_out[i], HIGH);
    else
      digitalWrite( swytch_out[i], LOW);
    board_state[i] = a;
    b[i]  = a;
    set[i] = 0;
    sets[i] = '0';
  }
}

and the updated code is like

Not exactly like that I assume, unless you really do have most of it in italics,

Why no code tags around the code ?

unsigned long x =check_time - last_check_time; not here

but here
static unsigned long last_check_time = 0;

Two things
static variables are initialized to zero and setting them to zero in each function usage defeats the storage class of the variable.

setting them to zero in each function usage defeats the storage class of the variable.

I thought that a static variable was only initialised once and then retained its value if had been changed subsequently.

UKHeliBob:
I thought that a static variable was only initialised once and then retained its value if had been changed subsequently.

See this reply in another thread; the 3rd quote.

Always initialised to zero; afterwards possibly initialised to another value somewhere along the line.

And the next reply (mine :wink: ) in that thread might also be of interest.

Always initialised to zero; afterwards possibly initialised to another value somewhere along the line.

I am sorry if I seem at odds with the C specification, but by definition a variable can only be initialised once. True, its value may initially be set to zero and later changed to another value but the change is not an initialisation.

OK :wink:

I don't even know the specification for any programming language, don't care about it and have happily been coding for the last 30 years :slight_smile:

are initialized to zero and

I just hate to say "automatically initialized to zero " , it is not automatic, but by using the storage type "static".

And yes , initialization process is done only once - in plain English, computers are nor exempt.

I wish this forum would quit reinventing terms to pleasure the audience and keep them at the lowest possible level of knowledge.

Jim