Help me writing better codes

Hi.

I kindly need guidance to code better in Arduino, since i have a lot lines of codes, writing all that in single tab is nightmare, so i wondering how can I split function between tabs ,

In this code, i have function that calculate the rpm,
Instead put all the function codes in loop, i wanted to make it simple by just calling the function

And also, variabel like

int

Is possible between tabs ?

Here's the codes

int period = 500;
int period2 = 500;
unsigned long time_now = 0;
int finalRPM = 0;
int QC_Duration = 0;

int RelayPin = D5;
int ShiftLight = D8;
int ShifterPin = D2;
int ShiftState = 0;

unsigned long previousMillis = 0;
const long interval = 50;


////////////////////////////////PART FROM RPM CONFIG/////////////////////////////////////////////////
///////////////
// Calibration:
///////////////

const byte PulsesPerRevolution = 1;  // Set how many pulses there are on each revolution. Default: 2.
// If the period between pulses is too high, or even if the pulses stopped, then we would get stuck showing the
// last value instead of a 0. Because of this we are going to set a limit for the maximum period allowed.
// If the period is above this value, the RPM will show as 0.
// The higher the set value, the longer lag/delay will have to sense that pulses stopped, but it will allow readings
// at very low RPM.
// Setting a low value is going to allow the detection of stop situations faster, but it will prevent having low RPM readings.
// The unit is in microseconds.
const unsigned long ZeroTimeout = 300000;  // For high response time, a good value would be 100000.
// For reading very low RPM, a good value would be 300000.


// Calibration for smoothing RPM:
const byte numReadings = 2;  // Number of samples for smoothing. The higher, the more smoothing, but it's going to
// react slower to changes. 1 = no smoothing. Default: 2.





/////////////
// Variables:
/////////////

volatile unsigned long LastTimeWeMeasured;  // Stores the last time we measured a pulse so we can calculate the period.
volatile unsigned long PeriodBetweenPulses = ZeroTimeout + 1000; // Stores the period between pulses in microseconds.
// It has a big number so it doesn't start with 0 which would be interpreted as a high frequency.
volatile unsigned long PeriodAverage = ZeroTimeout + 1000; // Stores the period between pulses in microseconds in total, if we are taking multiple pulses.
// It has a big number so it doesn't start with 0 which would be interpreted as a high frequency.
unsigned long FrequencyRaw;  // Calculated frequency, based on the period. This has a lot of extra decimals without the decimal point.
unsigned long FrequencyReal;  // Frequency without decimals.
unsigned long RPM;  // Raw RPM without any processing.
unsigned int PulseCounter = 1;  // Counts the amount of pulse readings we took so we can average multiple pulses before calculating the period.

unsigned long PeriodSum; // Stores the summation of all the periods to do the average.

unsigned long LastTimeCycleMeasure = LastTimeWeMeasured;  // Stores the last time we measure a pulse in that cycle.
// We need a variable with a value that is not going to be affected by the interrupt
// because we are going to do math and functions that are going to mess up if the values
// changes in the middle of the cycle.
unsigned long CurrentMicros = micros();  // Stores the micros in that cycle.
// We need a variable with a value that is not going to be affected by the interrupt
// because we are going to do math and functions that are going to mess up if the values
// changes in the middle of the cycle.

// We get the RPM by measuring the time between 2 or more pulses so the following will set how many pulses to
// take before calculating the RPM. 1 would be the minimum giving a result every pulse, which would feel very responsive
// even at very low speeds but also is going to be less accurate at higher speeds.
// With a value around 10 you will get a very accurate result at high speeds, but readings at lower speeds are going to be
// farther from eachother making it less "real time" at those speeds.
// There's a function that will set the value depending on the speed so this is done automatically.
unsigned int AmountOfReadings = 1;

unsigned int ZeroDebouncingExtra;  // Stores the extra value added to the ZeroTimeout to debounce it.
// The ZeroTimeout needs debouncing so when the value is close to the threshold it
// doesn't jump from 0 to the value. This extra value changes the threshold a little
// when we show a 0.

// Variables for smoothing tachometer:
unsigned long readings[numReadings];  // The input.
unsigned long readIndex;  // The index of the current reading.
unsigned long total;  // The running total.
unsigned long average;  // The RPM value after applying the smoothing.

////////////////////////// END OF PART FROM RPM CONFIG/////////////////////////////////////////////////


void ICACHE_RAM_ATTR Pulse_Event();


void setup()  // Start of setup:
{



  Serial.begin(9600);  // Begin serial communication.

  attachInterrupt(digitalPinToInterrupt(D7), Pulse_Event, FALLING);  // Enable interruption pin 2 when going from LOW to HIGH.

  pinMode(RelayPin, OUTPUT);
  pinMode(ShiftLight, OUTPUT);
  pinMode(ShifterPin, INPUT_PULLUP);

  delay(1000);  // We sometimes take several readings of the period to average. Since we don't have any readings
  // stored we need a high enough value in micros() so if divided is not going to give negative values.
  // The delay allows the micros() to be high enough for the first few cycles.

}  // End of setup.





void loop()  // Start of loop:
{

  // The following is going to store the two values that might change in the middle of the cycle.
  // We are going to do math and functions with those values and they can create glitches if they change in the
  // middle of the cycle.
  LastTimeCycleMeasure = LastTimeWeMeasured;  // Store the LastTimeWeMeasured in a variable.
  CurrentMicros = micros();  // Store the micros() in a variable.





  // CurrentMicros should always be higher than LastTimeWeMeasured, but in rare occasions that's not true.
  // I'm not sure why this happens, but my solution is to compare both and if CurrentMicros is lower than
  // LastTimeCycleMeasure I set it as the CurrentMicros.
  // The need of fixing this is that we later use this information to see if pulses stopped.
  if (CurrentMicros < LastTimeCycleMeasure)
  {
    LastTimeCycleMeasure = CurrentMicros;
  }





  // Calculate the frequency:
  FrequencyRaw = 10000000000 / PeriodAverage;  // Calculate the frequency using the period between pulses.





  // Detect if pulses stopped or frequency is too low, so we can show 0 Frequency:
  if (PeriodBetweenPulses > ZeroTimeout - ZeroDebouncingExtra || CurrentMicros - LastTimeCycleMeasure > ZeroTimeout - ZeroDebouncingExtra)
  { // If the pulses are too far apart that we reached the timeout for zero:
    FrequencyRaw = 0;  // Set frequency as 0.
    ZeroDebouncingExtra = 2000;  // Change the threshold a little so it doesn't bounce.
  }
  else
  {
    ZeroDebouncingExtra = 0;  // Reset the threshold to the normal value so it doesn't bounce.
  }





  FrequencyReal = FrequencyRaw / 10000;  // Get frequency without decimals.
  // This is not used to calculate RPM but we remove the decimals just in case
  // you want to print it.





  // Calculate the RPM:
  RPM = FrequencyRaw / PulsesPerRevolution * 60;  // Frequency divided by amount of pulses per revolution multiply by
  // 60 seconds to get minutes.
  RPM = RPM / 10000;  // Remove the decimals.





  // Smoothing RPM:
  total = total - readings[readIndex];  // Advance to the next position in the array.
  readings[readIndex] = RPM;  // Takes the value that we are going to smooth.
  total = total + readings[readIndex];  // Add the reading to the total.
  readIndex = readIndex + 1;  // Advance to the next position in the array.

  if (readIndex >= numReadings)  // If we're at the end of the array:
  {
    readIndex = 0;  // Reset array index.
  }

  // Calculate the average:
  average = total / numReadings;  // The average value it's the smoothed result.





  // Print information on the serial monitor:
  // Comment this section if you have a display and you don't need to monitor the values on the serial monitor.
  // This is because disabling this section would make the loop run faster.

  unsigned long currentMillis = millis();

  Serial.print("\tTachometer: ");
  Serial.println(average);
  finalRPM = average ;

  if (digitalRead(ShifterPin) == HIGH)
  {
    if (digitalRead(ShifterPin) == HIGH && ShiftState == 0)
      digitalWrite(RelayPin, HIGH);
    digitalWrite(ShiftLight, LOW);
    delay(QC_Duration);
    digitalWrite(RelayPin, LOW);
    ShiftState = 1;
  }
  else
  {
    ShiftState = 0;
  }

  //if ( average >6000)

  /// SHIFT LIGHT




  // QC CONFIG
  if ( finalRPM < 2000)
  {
    QC_Duration = 150;

  }

  if ( finalRPM > 2100)
  {
    QC_Duration = 80;
  }

  if ( finalRPM > 5000)
  { QC_Duration = 70;
    digitalWrite(ShiftLight, HIGH);
    delay(50);
    digitalWrite(ShiftLight, LOW);
  } else
  {
    digitalWrite(ShiftLight, LOW);
  }



  if ( finalRPM > 7000)
  {
    QC_Duration = 60;
  }











}// End of loop.





Pulse_Event()

Thank you in advance

Yes

Put the function in its own .ino file in the sketch directory created by using "Add tab" in the IDE and moving the function code to it

The Arduino upload process will combine all of the .ino files in the sketch folder into a single .ino before compiling it

Hello an3728
Delete the empty lines simply and condens the comments to facts.

you can certainly make your code more readable by using sub-functions instead of having all the code in loop()

i see no need to break a 171 line program up into separate files/tabs

but 2/3s of you file is comments ~6000 chars and ~3000 chars is code, such verbosity makes it harder to read the code. think of the code as an equation which is more compact than explaining it in text

some simplification may also help.

  • i see ShiftLight being set in several places in the code instead of just one base on QC_Duration
  • there are multiple comparisons of finalRPM that overlap, the condition for finalRPM > 2000 will also be true for > 5000 and > 7000 when it is > 7000. wouldn't it be better to do the following and set ShiftLight in one location using QC_Duration
    // QC CONFIG
    if (finalRPM > 7000)
        QC_Duration = 60;
    else if (finalRPM > 5000)
        QC_Duration = 70;
    else if (finalRPM > 2100)
        QC_Duration = 80;
    else
        QC_Duration = 150;
  • not sure there's a need for "FrequencyReal". i see the following lines
    FrequencyRaw = 10000000000 / PeriodAverage;
        FrequencyRaw = 0;
    FrequencyReal = FrequencyRaw / 10000;
    RPM = FrequencyRaw / PulsesPerRevolution * 60;
  • i would use simpler/short variable name, for example, "usec" and "usecLast", instead of LastTImeCycleMeasure, LastTimeWeMeasured and CurrentMicros
  • presumably if (CurrentMicros < LastTimeCycleMeasure) handles the wraparound case. there's no need, the math will work out when using unsigned int/longs
  • not sure about your frequency and RPM calculation. i believe the RPM is simply 60 * 10^6 / avgPeriodUsec (doesn't a 10 usec revolution correspond to 6000 RPM)? i see the following
    FrequencyRaw = 10000000000 / PeriodAverage;
        FrequencyRaw = 0;
    FrequencyReal = FrequencyRaw / 10000;
    RPM = FrequencyRaw / PulsesPerRevolution * 60;
  • finally, use a consistent naming format. only Capitalize Constants such as PulsesPerRevolution and it seems you prefer camelBack, so QC_Duration should be qcDuration.

there are other improvement

you did say " Help me writing better codes"

Whoa, big thanks for this, this literally help me better to code.

I'm still new to this that's why I need a lot of comments, so yeah, i will definitely use more constant var name next time.

Thanks for this, actually this is what I thought before, but i having difficult to understand, thank you

I've tried but i getting error message

expected constructor destructor before void

I just tried to move pulse_event()

Please post your revised main sketch, the contents of the new .ino file and the full error message as we cannot see exactly what you have done

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.