1
\$\begingroup\$

I am working on a project with an Arduino microcontroller and a Raspberry Pi. The code will have to do the following:

  • If the variance calculated of the ultrasonic sensor detections exceeds 1000, a event will be sent over the serial bus starting with the prefix /E/

  • If a request is received on the serial bus asking for the temperature, the Arduino shall measure the resistance of a NTC resistor. By adding another resistor to the NTC resistor, we can create a voltage divider. Once the output of the voltage divider is known, we can go back and calculate the resistance of the sensor. Once the resistance is known we can use the Steinhart–Hart equation to calculate the temperature. When the temperature is known it can be sent back to the Raspberry Pi in a serial message starting with the prefix /R/.

The code works fine, however the code is not optimized. My belief is the code can be made faster and more elegant. Since I am not very familiar with C++ I am looking for tips on how.

Any help would be appreciated!

#define TRIG_PIN 10
#define ECHO_PIN 9

#define THRESHOLD 1000.0f
#define COOLDOWN 1000

#define R2  72000.0f
#define Vin 3.3f

/*
 * These are the coefficients of Steinhart–Hart required to calculate
 * the temperature based on the resistance of my NTC resistor.
 */
#define A 0.00056510530716328503247902759198950661811977624893f
#define B 0.00024125080426957092754637612674883939689607359469f
#define C 0.00000000066126633960212339995476015836904301603560f


/*
 * In the setup function we initialize the serial bus and prepare the pins
 * because we want to communicate with the ultrasonic sensor.
 */
void setup() {
    Serial.begin(9600);
    pinMode(TRIG_PIN, OUTPUT);
    pinMode(ECHO_PIN, INPUT);
}

/* 
 * This function is used to calculate the variance of six samples from
 * the ultrasonic sensor.
 */
float calculate_variance_distance() {
    int i;
    float distances[6];
    float avrg, var, sum = 0, sum1 = 0;

    /* 
     * Obtain six samples and wait 25 ms between each sample to avoid 
     * fetching the same value over and over again. 
     */
    for (i = 0; i<6; i++) { 
        distances[i] = measure_distance(3);
        sum = sum + distances[i];
        delay(25);
    }

    /* 
     * To calculate the variance we need to:
     *
     *  Work out the Mean (the simple average of the numbers)
     *  Then for each number: subtract the Mean and square the result (the squared difference).
     *  Then work out the average of those squared differences.
     */
    avrg = sum / 6.0f;
    for (i = 0; i<6; i++) {
        sum1 = sum1 + pow((distances[i] - avrg), 2);
    }
    var = sum1 / 6.0f;

    /*
     * We need the variance to tell how spread out the samples are.
     */
    return var;
}

/*
 * In order to calculate the median we need a function to sort an array.
 */
void sort_array(float array[], int s) {
    int i, j, k;

    /* 
     * We read multiple values and sort them to get the median. The sorted
     * values are storted in the array sorted.
     */
    float n, sorted[s];

    for (i = 0; i<s; i++) {
        n = array[i];
        if (i == 0 || n<sorted[0]) {
            j = 0; // Insert at first position.
        }
        else {
            for (j = 1; j<i; j++) {
                if (sorted[j - 1] <= n && sorted[j] >= n) {
                    // Now j is insert position
                    break;
                }
            }
        }
        for (k = i; k>j; k--) {
            // Move all values higher than current reading up one position.
            sorted[k] = sorted[k - 1];
        }
        sorted[j] = n; // Insert current reading.
    }
    for (i = 0; i<s; i++) {
        array[i] = sorted[i];
    }
}

/* We need a function to calculate the distance according to the ultrasonic sensor.
 */
float measure_distance(int recursion) {
    long duration;
    float distance;

    digitalWrite(TRIG_PIN, LOW); // Set trigger pin low.
    delayMicroseconds(2); // Let signal settle.
    digitalWrite(TRIG_PIN, HIGH); // Set trigger pin high.
    delayMicroseconds(10); // Delay in high state.
    digitalWrite(TRIG_PIN, LOW); // Ping has now been sent.
    duration = pulseIn(ECHO_PIN, HIGH); // Duration is presented in microseconds.

    /* 
     * It takes half the time for sound to travel to the object and back. We
     * know that it takes approximently 29 µs to travel one centimeter.
     */
    distance = ((float)(duration) / 2.0) / 29.1;

    /* The accuracy of my ultrasonic sensor is 4 metres. If it exceeds 4 metres it is most
     * likely a false reading. So we try again until we get a readinge below that or recursion is zero.
     */
    if (distance > 400) {
        if (recursion > 0) return measure_distance(--recursion);
        else return 400;
    }
    else {
        return distance;
    }
}

/* 
 * We use Steinhart–Hart equation to calculate the temperature. 
 */
float measure_temperature() {
    int readValue = analogRead(0); // Fetch the analog read value in the range of 0-1023
    float Vout = 3.3f - ((float)(readValue) / 1023.0f*Vin); // Calculate the voltage.

    /*
     * In our voltage divider we have another resistor where the resistance is known,
     * so we can go back and calculate the resistance of the sensor.
     */
    float R1 = ((R2*Vin) / Vout) - R2;

    float T = 1.0f / (A + B*log(R1) + C*pow(log(R1), 3)) - 273.15; // Use the Steinhart–Hart equation to calculate the temperature.

    return T;
}

/* 
 * We fetch five samples and calculate the median to eleminate noise from sensor readings. 
 */
float measure_median_temperature() {
    int i;
    float temperatures[5];


    /*
     * Obtain five samples using the function above.
     */
    for (i = 0; i<5; i++) {
        temperatures[i] = measure_temperature();
        delay(50);
    }

    /* 
     * Sort the array with the function above.
     */
    sort_array(temperatures, 5);
    return temperatures[2];
}

/*
 * We use the loop function to check if there are any requests and also to check 
 * if the variance of the sensor readings exceeds THRESHOLD.
 */
void loop() {
    /* 
     * We have a cooldown value to avoid unneccesary repeats.
     */
    static unsigned int cooldown = 0;

    if (Serial.available() > 0) {
        String str = Serial.readStringUntil('\0');

        if (str == "temperature") {
            float temperture = measure_median_temperature();
            Serial.print("/R/");
            Serial.print(round(temperture * 10));
            return;
        }
    }
    else if (abs(millis() - cooldown) >= COOLDOWN && calculate_variance_distance() >= THRESHOLD) {
        Serial.print("/E/");
        Serial.print("motion");
        cooldown = millis();
    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

I see a number of things which may help you improve your code. First, I think it's important for you to know that the language used by Arduino is only based on C++, but isn't C++.

Simplify the mathematics

If we look at the equations used to derive the temperature in measure_temperature, the mathematics can be simplified. Using a little algebra, we can derive the fact that $$ R_1 = \frac{x R_2}{1023 - x}$$ with \$x\$ being the readValue. Further, we really need the log of that, rather that the resistance value, so the measure_temperature function can be written like this:

float measure_temperature() {
    int readValue = analogRead(0); // Fetch the analog read value in the range of 0-1023
    /*
     * In our voltage divider we have another resistor where the resistance is known,
     * so we can go back and calculate the resistance of the sensor.
     */
    float logR1 = log((R2 * readValue)/(1023 - readValue));

    return 1.0f / (A + B*logR1 + C*pow(logR1, 3)) - 273.15; // Use the Steinhart–Hart equation to calculate the temperature.
}

Understand the limits of floating point mathematics

On the Arduino, both float and double floating point types are represented as 32-bit quantities. That means that the terribly long values used for the coefficients A, B and C will mostly be unused in the actual calculations. Better would be to express them as floating point using no more than 7 digits of precision.

Consider using a look-up table

There are 1024 possible input values for the analog input and a float is 32 bits (4 bytes) so a 4K table could be used and would be much faster than all of that calculation. Precompute the results for all possible values and create a table to use within the source code. I'm sure you could write a simple program to do so. With many embedded processors, including that in the Arduino, there is no hardware support for floating point arithmetic, so floating point mathematics is typically much slower than integer calculations.

Spelling is important

The name of the variable in loop should be temperature rather than temperture. Having typos in your code leads to difficult-to-spot bugs and other maintenance difficulties. Being careful to spell things correctly will pay dividends in more robust, easier to maintain code.

Prefer to bail out early

In the measure_distance routine, if the distance is more than 400 cm, it is assumed to be faulty and the routine tries again. That corresponds to a reading of 23280 microseconds (29.1 microseconds times 400 cm time 2). However, the pulseIn function will wait up to 1 second. Since that's all just wasted time, it would instead make sense to use the form of pulseIn that has a timeout and then check for a value of 0 as indication that the value is faulty. See the pulseIn() reference page for details.

Prefer iteration to recursion

Recursion, as in the use in the measure_distance routine, is better expressed as an iteration instead. That is, something like this psuedocode:

for (tries = 3; tries; --tries) {
    if (attemptedMeasurement is good) {
        return attemptedMeasurement;
    }
}

The reason is that recursion uses stack space to set up a stack frame containing local variables for the function and the return location, while the iteration form only uses space for the tries counter. It's also a little bit more clear to the reader of the code.

Use a better sort algorithm

There are much better, more efficient (in terms of speed and space) sorting algorithms available that are better than you're using in your code. Even the much maligned bubble sort would likely outperform the algorithm currently implemented in sort_array.

\$\endgroup\$
7
  • \$\begingroup\$ Thanks, very helpful. Would it be smarter to just send readValue to the raspberry pi instead of having a lookup table on the arduino? Also would I pass in 23280 to pulseIn function to make sure no false readings occur? \$\endgroup\$
    – Linus
    Commented Oct 28, 2015 at 14:54
  • \$\begingroup\$ It's certainly possible to offload the math to the Pi, but I'd be inclined to do that kind of signal preprocessing in the Arduino, since it doesn't have anything better to do, allowing the Pi to just do higher level functions. This also makes it slightly simpler to test each piece separately. Yes, just pass 23280 as the timeout (third argument) for the pulseIn and that takes care of ot-of-range readings. \$\endgroup\$
    – Edward
    Commented Oct 28, 2015 at 17:57
  • \$\begingroup\$ Alright, I'll settle for the latter approach because it simplifies a lot of things. Thank you once more for the constructive feedback! \$\endgroup\$
    – Linus
    Commented Oct 28, 2015 at 18:05
  • \$\begingroup\$ Constructive feedback is what Code Review is all about! I'm sure you'll contribute too. Indeed you already have by asking a good question. \$\endgroup\$
    – Edward
    Commented Oct 28, 2015 at 18:41
  • \$\begingroup\$ I used timeout for pulseIn with value 23280 and I still get durations over that instead of zero. One time I got over 29000 microseconds. Could this be because I use an old version of the Arduino IDE (version 1.0.1)? \$\endgroup\$
    – Linus
    Commented Oct 30, 2015 at 12:19
1
\$\begingroup\$
  1. You are defining the loop variables separately, I don't think you need to, you could do for (int i = 0; i < 6; i++)

  2. In C++ post increment is slower for non-simple objects, so its good practice to pre-increment whenever you can. for (int i = 0; i < 6; ++i)

  3. There are a lot of literal values in the code. Looking a calculate_variance_distance() the number of samples, 6, should be a static const (local to the function).

  4. You are assigning ints to floats as initial values.
    float avrg, var, sum = 0, sum1 = 0;

  5. As a point of style I would always declare each variable of a separate line.

  6. Division is an expensive operation, so it might be preferable to multiply instead where you can.

  7. You are using a diddy little Arduino to do complex maths operations before passing a nicely formatted string to a comparatively supercharged Pi. Would it be better to shift all the maths onto the PI and just read the data on the Arduino and pass larger strings of data. You'd need to speed test it, but it might work out better, especially if you have one of the quad core PIs.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks, why didn't I think of doing the maths on the Raspberry Pi. It will involve a bit of work but that's okay. Do you believe it is smarter to send all the ultrasonic sensor readings to the pi aswell? My belief is that calculating the variance isn't too hard on the processor but maybe the microcontroller should do as little as possible. \$\endgroup\$
    – Linus
    Commented Oct 28, 2015 at 14:24
  • \$\begingroup\$ @Linus - I honestly don't know if it will be better, its just my gut instinct from your description. Breaking the system down into blocks the Arduino should be feeding back proper results to the PI. So none of the sensor logic needs to be on the PI. BUT you also have to consider you are talking about a ~16MHz processor vs a Quad core 900MHz machine. so the more processing you can do on the PI the better the performance. What you could do is break it all the data processing into platform independent functions and move them around as required. Does that make sense? \$\endgroup\$ Commented Oct 29, 2015 at 11:37
  • \$\begingroup\$ I don't follow your last statement, the main issue is that when the ultrasonic sensor varies a lot it will send an event in realtime. There is no time to ask the raspberry pi to check the readings because I'm using the ultrasonic sensor for motion detection (along with a PIR sensor). The temperature is only requested every 10 minutes so I don't know if it's worth the hassle of porting it to the pi. \$\endgroup\$
    – Linus
    Commented Oct 29, 2015 at 11:43
  • \$\begingroup\$ @Linus - What I mean by my last line is if you are working with different target platforms then it might be worth looking at writing you code so its platform independent. You then decide on a interface to your hardware that all targets can support and write two thin layers to sit between your code and the target hardware. So you call read temp in your code, your Thermometer class makes the necessary calls to your hardware layer to retrieve the data and then does the calculations before returning the result. Most of the code doesn't know what hardware its running on ... \$\endgroup\$ Commented Oct 30, 2015 at 10:29
  • \$\begingroup\$ ... you could be using direct IO port access, I2C, SPI, Serial, Serial over WIFI to an ESP8266, etc. Its a lot of hassle for what you are doing, but if you keep doing it, you will start building up a library over time and that could be useful. However it sounds like it would be better to do the hardware access and maths on the Arduino in this case. \$\endgroup\$ Commented Oct 30, 2015 at 10:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.