Need a critque for my code.

Hello.

I am trying to run a bike with variable controlled frame geometry and a hall switch to measure rpm together. I am getting unwanted RPM measurements every few seconds when a relay is activated even though the RPM should be zero. I would like to make sure the problem is not in my code before trying to trouble shoot the actual electrical hookup of the Hall and Relays to the Arduino. Thank you for your kindness. :slight_smile:

/*
 * This Arduino Uno program runs a special bicycle with change-on-the-fly frame geometry 
 * and braking.  The bike has a linear actuator for increasing or decreasing frame geometry 
 * and rotary actuator for brake tension each with pots for positional feedback of the two 
 * actuators . 
 * 
 * A block of 4 relay modules control the two actuators. (5V 4 Channel Relay Shield Module 
 * optocoupler Arduino) For the following program if you change the target value (tgtactValue) 
 * the bike frame will move to that value and at the same time change the brake tension to 
 * match a desired setting. The existing program works well.
 * 
 * I want to be able to add an Hall sensor (Sunfounder Hall Switch 
 * https://www.sunfounder.com/switch-hall-sensor-module.html) on the front wheel to monitor 
 * front wheel RPM and a LCD display (Sunfounder IIC/I2C/TWI Serial 2004/20x4 LCD Module 
 * Shield for Arduino Uno/Mega2560) that displays bicycle wheel rpm, frame geometry and 
 * brake tension settings.  When I try to add a Hall Switch Routine, LCD Shield Routine 
 * and my program they interfere with each other in a way I am not able to debug.
 * Thank you for your consideration!!!!  
 * Curtis Platt    projecteinsteinvr@att.net
 */

volatile int rpmcount = 0;//see http://arduino.cc/en/Reference/Volatile 
unsigned long rpm = 0;
unsigned long lastRPMmillis = 0;


//pin 2 reserved for interrupt for hall sensor
const byte RELAY1=3; //linear actuator expand
const byte RELAY2=4; //linear actuator contract
const byte RELAY3=5; //rotary actuator clockwise
const byte RELAY4=6; //rotary actuator counter clockwise
const byte actPot = A2;  // actuator pot pin
const byte brkPot = A3; // brake pot pin


int tgtActValue = 0; //target value you want frame's linear actuator to be at
int tgtBrkValue = 0;//target value you want brake's rotary actuator to be at
int actValue = 0; //actual value of frame's linear actuator from pot
int brkValue= 0; //actual value of brake's rotary actuator from pot
int Actdiff = 0; //difference of actual and target value of linear actuator
int Brkdiff = 0;//difference of actual and target value of rotary actuator

unsigned long lastmillis = 0;


void setup()
{    
  Serial.begin(9600); //needed to display on serial monitor
   attachInterrupt(0, rpm_fan, FALLING); 
  // Initialise the Arduino data pins for OUTPUT
  pinMode(RELAY1, OUTPUT);
  pinMode(RELAY2, OUTPUT);
  pinMode(RELAY3, OUTPUT);
  pinMode(RELAY4, OUTPUT);
  //turn them all off
  digitalWrite(RELAY1,HIGH);
  digitalWrite(RELAY2,HIGH);  
  digitalWrite(RELAY3,HIGH);
  digitalWrite(RELAY4,HIGH);
} 
 void loop(){
 if (millis() - lastRPMmillis >= 1000){  /*Update every one second, this will be equal to reading frecuency (Hz).*/
 detachInterrupt(0);    //Disable interrupt when calculating
 rpm = rpmcount* 60;  /* Convert frecuency to RPM, note: this works for one interruption per full rotation. For two interrups per full rotation use rpmcount * 30.*/
 Serial.print(" RPM = ");
 Serial.println(rpm);
 rpmcount=0;
 
 lastRPMmillis = millis(); // Update lastmillis
  attachInterrupt(0, rpm_fan, FALLING); //enable interrupt
  }
    if(millis()-lastmillis>=50){    //updates 20x second keeps relay chatter down
      
      //user inputs tgtactValue here between 390 and 630 and the programs computes a second 
      //tgtactValue and also finds the two pot's outputs
      
      tgtActValue=550;                       //midway of frame geometry
      tgtBrkValue=int (776.769 - (.5152 * actValue));    
      actValue=analogRead(actPot);       //value of linear actuator's yo-yo pots  390  457  525  577  650 (-10 degree down to +10degree up)
      delay(5);//needed for ADC to settle(??)
      brkValue=analogRead(brkPot);       // value of brake rotary actuator's pot 575  540  510  480  450  (fully to openfully closed)
      delay(5);//needed for ADC to settle(??)
      
      /*we run a simple linear regression analysis of the corresponding pot's 
       * points so when frame is -10 degrees down the brake is fully open, when 
       * the frame is neutral the brake is at neutral or when the frame is 10 degrees up 
       * the brake is fully closed or any proportion in between.
       * 
       *  frame, brake
       *    x, y
       *   390,575
       *   457,540
       *   525,510
       *   577,480
       *   630,450
       *   //www.alcula.com/calculators/statistics/linear-regression/
       *   using the above 5 plot points we come up with a very close approximation of y(brake tension) 
       *   whenever you input x(frame position) with the simple linear Regression  y=776.769 - .5152x  .
       */
       
      //this routine compares target values (one input from user and one calculated) and actual pot feedback
      //values and activates the relays and thus the actuators accordingly 
      
    
        Actdiff=(tgtActValue-actValue);
    
        while(actValue < 300 || actValue > 750){Serial.print("TILT1");//if the values are out of bound the pot or something else broke
        delay(500);
        actValue=analogRead(actPot);
       }
          
      if (Actdiff <-5){
        digitalWrite(RELAY3, HIGH);
        digitalWrite(RELAY4, LOW);
        }
      else if (Actdiff >5 ) {
        digitalWrite(RELAY3, LOW);
        digitalWrite(RELAY4, HIGH);
      }
      else{    
        digitalWrite(RELAY3, HIGH);
        digitalWrite(RELAY4, HIGH);
      }


      
        Brkdiff=(brkValue-tgtBrkValue);
      
        while(brkValue < 350 || brkValue > 650){Serial.print("TILT2"); //if the pot is out of bounds the pot probably broke
        delay(500);
        brkValue=analogRead(brkPot);
        }
      
        if (Brkdiff <-5){
          digitalWrite(RELAY1, HIGH);
          digitalWrite(RELAY2, LOW);
        }
        else if (Brkdiff >5 ) {
          digitalWrite(RELAY1, LOW);
          digitalWrite(RELAY2, HIGH);
        }
        else{digitalWrite(RELAY1, HIGH);
           digitalWrite(RELAY2, HIGH);
        }

        //Serial.print("actValue:");      
        //Serial.println(actValue);
        //Serial.print("Actdiff:");
        //Serial.println(Actdiff);
        // Serial.print("brake:");
        //Serial.println(brkValue);
        // Serial.print("Brkdiff:");
        //Serial.println(Brkdiff);
        lastmillis = millis(); // Update lastmillis
      }
 }
 
 void rpm_fan(){ /* this code will be executed every time the interrupt 0 (pin2) gets low.*/
  rpmcount++;
}

Commented code - gold star
C is "space delimited " language so scrunched stuff like while …}DoFunction(… is little harder to read
You may want to INCLUDE some if(condition <x ) to include x (condition <= x)
Try code formatter - CTRL T - and post the results here , just for “because”.
Jolly good show.

Hi,

Sounds like Electromagnetic Interference.

Look THIS PAGE over and define your Grounding.

Let us know how it goes..

Terry lots of good suggestions there. I am running a long run of ribbon cable for the sensor...bad. Will switch it out and get back here in a day or two. Thank you so much!!!!!!!!!!!!!

Thank you 232…so easy to format properly with auto formatter in the IDE.

/*
   This Arduino Uno program runs a special bicycle with change-on-the-fly frame geometry
   and braking.  The bike has a linear actuator for increasing or decreasing frame geometry
   and rotary actuator for brake tension each with pots for positional feedback of the two
   actuators .

   A block of 4 relay modules control the two actuators. (5V 4 Channel Relay Shield Module
   optocoupler Arduino) For the following program if you change the target value (tgtactValue)
   the bike frame will move to that value and at the same time change the brake tension to
   match a desired setting. The existing program works well.

   I want to be able to add an Hall sensor (Sunfounder Hall Switch
   https://www.sunfounder.com/switch-hall-sensor-module.html) on the front wheel to monitor
   front wheel RPM and a LCD display (Sunfounder IIC/I2C/TWI Serial 2004/20x4 LCD Module
   Shield for Arduino Uno/Mega2560) that displays bicycle wheel rpm, frame geometry and
   brake tension settings.  When I try to add a Hall Switch Routine, LCD Shield Routine
   and my program they interfere with each other in a way I am not able to debug.
   Thank you for your consideration!!!!
   Curtis Platt    projecteinsteinvr@att.net
*/

volatile int rpmcount = 0;//see http://arduino.cc/en/Reference/Volatile
unsigned long rpm = 0;
unsigned long lastRPMmillis = 0;


//pin 2 reserved for interrupt for hall sensor
const byte RELAY1 = 3; //linear actuator expand
const byte RELAY2 = 4; //linear actuator contract
const byte RELAY3 = 5; //rotary actuator clockwise
const byte RELAY4 = 6; //rotary actuator counter clockwise
const byte actPot = A2;  // actuator pot pin
const byte brkPot = A3; // brake pot pin


int tgtActValue = 0; //target value you want frame's linear actuator to be at
int tgtBrkValue = 0;//target value you want brake's rotary actuator to be at
int actValue = 0; //actual value of frame's linear actuator from pot
int brkValue = 0; //actual value of brake's rotary actuator from pot
int Actdiff = 0; //difference of actual and target value of linear actuator
int Brkdiff = 0;//difference of actual and target value of rotary actuator

unsigned long lastmillis = 0;


void setup()
{
  Serial.begin(9600); //needed to display on serial monitor
  attachInterrupt(0, rpm_fan, FALLING);
  // Initialise the Arduino data pins for OUTPUT
  pinMode(RELAY1, OUTPUT);
  pinMode(RELAY2, OUTPUT);
  pinMode(RELAY3, OUTPUT);
  pinMode(RELAY4, OUTPUT);
  //turn them all off
  digitalWrite(RELAY1, HIGH);
  digitalWrite(RELAY2, HIGH);
  digitalWrite(RELAY3, HIGH);
  digitalWrite(RELAY4, HIGH);
}
void loop() {
  if (millis() - lastRPMmillis >= 1000) { /*Update every one second, this will be equal to reading frecuency (Hz).*/
    detachInterrupt(0);    //Disable interrupt when calculating
    rpm = rpmcount * 60; /* Convert frecuency to RPM, note: this works for one interruption per full rotation. For two interrups per full rotation use rpmcount * 30.*/
    Serial.print(" RPM = ");
    Serial.println(rpm);
    rpmcount = 0;

    lastRPMmillis = millis(); // Update lastmillis
    attachInterrupt(0, rpm_fan, FALLING); //enable interrupt
  }
  if (millis() - lastmillis >= 50) { //updates 20x second keeps relay chatter down

    //user inputs tgtactValue here between 390 and 630 and the programs computes a second
    //tgtactValue and also finds the two pot's outputs

    tgtActValue = 550;                     //midway of frame geometry
    tgtBrkValue = int (776.769 - (.5152 * actValue));
    actValue = analogRead(actPot);     //value of linear actuator's yo-yo pots  390  457  525  577  650 (-10 degree down to +10degree up)
    delay(5);//needed for ADC to settle(??)
    brkValue = analogRead(brkPot);     // value of brake rotary actuator's pot 575  540  510  480  450  (fully to openfully closed)
    delay(5);//needed for ADC to settle(??)

    /*we run a simple linear regression analysis of the corresponding pot's
       points so when frame is -10 degrees down the brake is fully open, when
       the frame is neutral the brake is at neutral or when the frame is 10 degrees up
       the brake is fully closed or any proportion in between.

        frame, brake
          x, y
         390,575
         457,540
         525,510
         577,480
         630,450
         //www.alcula.com/calculators/statistics/linear-regression/
         using the above 5 plot points we come up with a very close approximation of y(brake tension)
         whenever you input x(frame position) with the simple linear Regression  y=776.769 - .5152x  .
    */

    //this routine compares target values (one input from user and one calculated) and actual pot feedback
    //values and activates the relays and thus the actuators accordingly


    Actdiff = (tgtActValue - actValue);

    while (actValue < 300 || actValue > 750) {
      Serial.print("TILT1");//if the values are out of bound the pot or something else broke
      delay(500);
      actValue = analogRead(actPot);
    }

    if (Actdiff < -5) {
      digitalWrite(RELAY3, HIGH);
      digitalWrite(RELAY4, LOW);
    }
    else if (Actdiff > 5 ) {
      digitalWrite(RELAY3, LOW);
      digitalWrite(RELAY4, HIGH);
    }
    else {
      digitalWrite(RELAY3, HIGH);
      digitalWrite(RELAY4, HIGH);
    }



    Brkdiff = (brkValue - tgtBrkValue);

    while (brkValue < 350 || brkValue > 650) {
      Serial.print("TILT2"); //if the pot is out of bounds the pot probably broke
      delay(500);
      brkValue = analogRead(brkPot);
    }

    if (Brkdiff < -5) {
      digitalWrite(RELAY1, HIGH);
      digitalWrite(RELAY2, LOW);
    }
    else if (Brkdiff > 5 ) {
      digitalWrite(RELAY1, LOW);
      digitalWrite(RELAY2, HIGH);
    }
    else {
      digitalWrite(RELAY1, HIGH);
      digitalWrite(RELAY2, HIGH);
    }

    //Serial.print("actValue:");
    //Serial.println(actValue);
    //Serial.print("Actdiff:");
    //Serial.println(Actdiff);
    // Serial.print("brake:");
    //Serial.println(brkValue);
    // Serial.print("Brkdiff:");
    //Serial.println(Brkdiff);
    lastmillis = millis(); // Update lastmillis
  }
}

void rpm_fan() { /* this code will be executed every time the interrupt 0 (pin2) gets low.*/
  rpmcount++;
}

232:
C is "space delimited " language

That is nonsense - in C/C++ whitespace is utterly irrelevant.

CtrlAltElite:
That is nonsense - in C/C++ whitespace is utterly irrelevant.

I agree that it is irrelevant to the compiler. But it is very useful for the human reader.

...R

CtrlAltElite:
That is nonsense - in C/C++ whitespace is utterly irrelevant.

Such commment is an (good) indicator you did not read / get the WHOLE sentence.
But it is understandable since I did not spelled it out.

But your comment also did not actually made a comment toward the OP request.

So your post is what is irrelevant.

… and if you cannot take a heat / criticism stay out of the kitchen…

But it is understandable since I did not spelled it out.

if you cannot take a heat

It's clear English is not your first language, so I guess I need to cut you some slack.

But your comment also did not actually made a comment toward the OP request.

But hopefully, my comment prevented the OP from forming a misunderstanding about the language. Think of it as a meta critique.

.. and if you cannot take a heat / criticism stay out of the kitchen..

What's that supposed to mean?
If you post falsehoods, be prepared to be called-out on them.

Such commment is an (good) indicator you did not read / get the WHOLE sentence.

I did read the whole sentence, but I'm a little like a program when it encounters something like "if (x && y)" - if I see that 'x' is false, I don't evaluate the rest of the condition.

CtrlAltElite:
I did read the whole sentence, but I'm a little like a program when it encounters something like "if (x && y)" - if I see that 'x' is false, I don't evaluate the rest of the condition.

If you had not written Reply #5 I would have written it in much the same terms. The statement in Reply #1 C is "space delimited " language is plain wrong.

...R