Code issues (slowness) and rectifications.

Hi, I have created this code for a project in school. The principale is that given a certain amount of claps lights turn via a relay or doors open ect. What I have done for now is a code that detects and counts the amount of claps, I know there is another way to do with "double claps detectors" for example that are faster but i did not manage to make it work and I need a detector for 1, 2 and 3 claps. This code seems to work but after some time it goes wild for a unknown reason to me. Aswell when it "counts" the claps it seems quite slow, again i am not sure how to make that slightly faster. Any help would be greatly appreciated. Thanks for reading

 #define signalToRelayPin  12
    #define sensorPin 7

    int lastSoundValue=0;
    int soundValue;
    long lastNoiseTime = 0;
    long currentTime = 0;
    long lastLightChange = 0;
    int relayStatus = HIGH;
    int clap_interval = 500;
    int claps = 0;

    void setup() {
      /*
       pinMode(sensorPin, INPUT);
       pinMode(signalToRelayPin, OUTPUT);
       */
       pinMode(7, INPUT);
       pinMode(12, OUTPUT);
       Serial.begin(9600);
    }



void loop() {
    soundValue = digitalRead(7);
    currentTime = millis();

    if ((soundValue == HIGH)  || (lastSoundValue == 1)){ 
      Serial.println("clappp");
      lastSoundValue=currentTime;
      claps++;
    }

    if(currentTime-lastSoundValue>4000){
      if(claps!=0){
        Serial.print(claps);
        Serial.println(" claps");
      }
      claps=0;
      lastSoundValue=currentTime;
    }
    
    delay(10); // delay polling
    }
  soundValue = digitalRead(7);
  currentTime = millis();
  if ((soundValue == HIGH)  || (lastSoundValue == 1))
  {
    Serial.println("clappp");
    lastSoundValue = currentTime;
    claps++;
  }

Part of this code tests whether lastSoundValue equals 1. How often will it equal 1 ? Being derived from the value of millis() the answer will be every 49 and a bit days. With that in mind, does the test make sense ?

UKHeliBob:

  soundValue = digitalRead(7);

currentTime = millis();
  if ((soundValue == HIGH)  || (lastSoundValue == 1))
  {
    Serial.println("clappp");
    lastSoundValue = currentTime;
    claps++;
  }



Part of this code tests whether lastSoundValue equals 1. How often will it equal 1 ? Being derived from the value of millis() the answer will be every 49 and a bit days. With that in mind, does the test make sense ?

um no, i am not sure what you mean. The programme works but after 3 mins or so it starts doing werird stuff

int lastSoundValue=0;Well Bob has a point, actually lastsoundvalue will most probably never be 1, every time it rolls over maybe(chances remain very small) but all other arithmetic goes off simply because this should not be an int but an unsigned long. (the other 'long' should also be unsigned.)

Deva_Rishi:
int lastSoundValue=0;Well Bob has a point, actually lastsoundvalue will most probably never be 1, every time it rolls over maybe(chances remain very small) but all other arithmetic goes off simply because this should not be an int but an unsigned long. (the other 'long' should also be unsigned.)

okay, I am starting to understand, wich other 'long' are you talking about?

all the variables that store the result of millis() (or a calculation of them) so at least

     unsigned long lastSoundValue=0;  // this name is confusing    
    unsigned long currentTime = 0;

these, but probably these as well

    unsigned long lastNoiseTime = 0;
    unsigned long lastLightChange = 0;

although i haven't seen the code that uses them, their name suggests they are time related. millis() returns an unsigned long (aka uin32_t) and any variables you use with it should have the same size & format.

Deva_Rishi:
all the variables that store the result of millis() (or a calculation of them) so at least

     unsigned long lastSoundValue=0;  // this name is confusing    

unsigned long currentTime = 0;



these, but probably these as well

unsigned long lastNoiseTime = 0;
    unsigned long lastLightChange = 0;


although i haven't seen the code that uses them, their name suggests they are time related. millis() returns an unsigned long (aka uin32_t) and any variables you use with it should have the same size & format.

okay thank you, would you have any recommendations on how I could maj=ke the code better as well? more efficient for example?

the code itself is not to bad outside of the incorrect declarations (or confusing names) and some variables that are not being used (which always confuse people that are trying to work out what is going on.)
thedelay(10); // delay polling is probably quite effective, (who can clap for 10ms, maybe in a bathroom) as a debounce, but maybe this is what you were trying to do here ?if ((soundValue == HIGH)  || (lastSoundValue == 1)){ so how about remove the delay and change this line to if ((soundValue == HIGH)  && (currentTime-lastSoundValue >30)){ and the part where you reset the counter

    if ((claps) && (currentTime-lastSoundValue>4000)) {  // test the both conditions here, if the first one is false the
                                     // second one doesn't even get tested
        Serial.print(claps);
        Serial.println(" claps");
        claps=0;   // this can be here

      
      //lastSoundValue=currentTime;  // this line is redundant and may cause you to miss a clap.
    }

test it, rename 'lastSoundValue' and continue building (or maybe put this part into a separate function first.)

Deva_Rishi:
the code itself is not to bad outside of the incorrect declarations (or confusing names) and some variables that are not being used (which always confuse people that are trying to work out what is going on.)
thedelay(10); // delay polling is probably quite effective, (who can clap for 10ms, maybe in a bathroom) as a debounce, but maybe this is what you were trying to do here ?if ((soundValue == HIGH)  || (lastSoundValue == 1)){ so how about remove the delay and change this line to if ((soundValue == HIGH)  && (currentTime-lastSoundValue >30)){ and the part where you reset the counter

    if ((claps) && (currentTime-lastSoundValue>4000)) {  // test the both conditions here, if the first one is false the

// second one doesn't even get tested
        Serial.print(claps);
        Serial.println(" claps");
        claps=0;  // this can be here

//lastSoundValue=currentTime;  // this line is redundant and may cause you to miss a clap.
    }


test it, rename 'lastSoundValue' and continue building (or maybe put this part into a separate function first.)

okay ill try all this out thank you for your help that was great!