Help optimizing a function

Projects Programming
1

I'm doing a project that uses multiple timers with millis() . After much troubleshooting as to why the time was sometimes off I came to the conclusion that a particular function was taking an inordinate amount of time to compute. The purpose of this functions is to check if the readings coming from a sensor have stabilized(allowing some variance) .

#include "Nanoshield_Thermocouple.h"
#include "Thermocouple.h"
#include "arrayFunctions.h"
#include "config.h"

Thermocouple::Thermocouple(){
  time = 0;
  interval = 1000;
  timeout = 0;
  temp_index = 0;
  last_readings[recording_time];
  thermocouple.begin();     
}

float Thermocouple::temperature(){
  thermocouple.read(); 
  return thermocouple.getExternal();
}

bool Thermocouple::is_stable(){
  float max_value;
  float min_value;
  last_readings[temp_index] = temperature();
  if(temp_index < (recording_time - 1) ){
    temp_index++;
  }
  else{
    temp_index = 0;
  }  
  max_value = get_array_max_value(last_readings);
  min_value = get_array_min_value(last_readings);
  return ((max_value - min_value) < (min_value * (variance/100) ) );
}

This is the relevant code. The trouble function is is_stable(). The array functions are on another file defined as follows:

float get_array_max_value(float array[]){
  int i;
  float max_value;
  for(i=0;i<recording_time;i++){
    max_value = max( array[ i ], max_value );
  }
  return max_value;
}

float get_array_min_value(float array[]){
  int i;
  float min_value =0;
  for(i=0;i<recording_time;i++){
    if (i==0) 
      min_value = array[i];
    else
      min_value = min( array[ i ], min_value );
  }
  return min_value;
}

The function is then simply called inside the main loop like this:

void loop()
{
  if(millis() == thermocouple.timeout){//only execute once every thermocouple.interval ms
    thermocouple.timeout += thermocouple.interval;  
    
    if(thermocouple.is_stable() == true){
      Serial.println("estavel");
    }
    
  }
  
}

Can anyone enlighten me on how can I achieve this in a simpler and faster way?

Thanks.

2

What does this do?

  last_readings[recording_time];

Pete

3

el_supremo:
What does this do?

  last_readings[recording_time];

Pete

Just initializing the array. Sorry I didn't include the config file:

#ifndef CONFIG_H
#define CONFIG_H

const int recording_time = 5;
const float variance = 3;

#endif
4

So, what is the trouble?
Lots of important code missing from your post.
I note in the function get_array_max_value(), max_value is not initialized to anything. Is that what you intended?

5

jremington:
So, what is the trouble?
Lots of important code missing from your post.
I note in the function get_array_max_value(), max_value is not initialized to anything. Is that what you intended?

Thanks for pointing that out. Can you tell me what else is missing? I took out some stuff that was irrelevant to the question. I guess I should say that I have this before the main loop:

#include "Thermocouple.h"
Thermocouple thermocouple;

The problem is this code seems be taking a lot of time to run, which is weird because I'm not really using such a big array.

6

Just initializing the array

I don't think so - it accesses an element off the end of the array and does nothing with it. That might be at least part of your problem.

Pete

7

el_supremo:

Just initializing the array

I don't think so - it accesses an element off the end of the array and does nothing with it. That might be at least part of your problem.

Pete

My mistake. I removed that line and put it in the header file.

#ifndef THERMOCOUPLE_H
#define THERMOCOUPLE_H

#include <Arduino.h>
#include "Nanoshield_Thermocouple.h"
#include "config.h"

class Thermocouple{
  public:
    Thermocouple();
    bool is_stable();
    float temperature();
    void send_serial_data();
    unsigned int time;
    unsigned long timeout;
    unsigned long interval;
  protected:    
    int temp_index;
    float last_readings[recording_time];
    Nanoshield_Thermocouple thermocouple;
};

#endif

The problem still persists though. Does anyone have a better implementation for the max and minimum value of an array functions?

8

I'm probably not seeing something, but if you are calling the functions to get the max or min value in the array and those values are constantly updated, send the most recent value in and have a variable defined in each with the static storage class:

float get_array_max_value(float val){
   static long tempMax = -100000F;
 
   if (val >= tempMax)
      tempMax = val;
  return tempMax;
}

float get_array_min_value(float val){
   static long tempMin = 100000F;
 
   if (val <= tempMin )
      tempMin = val;
  return tempMin ;
}

If you are only updating the last value in the array, why read the first 4 values when you know which one is the max value from a previous call? What am I missing?

9

What do you mean by "a long time"? It should take no longer than a millisecond or so to determine the min and max values of an array of just a few elements. (5 elements? -- as suggested by the declaration "const int recording_time = 5;" ?)

If you want to tell where your program is stuck, sprinkle some serial.print statements throughout the code, saying "I'm here" and print out the current time using millis().

10

econjack:
I'm probably not seeing something, but if you are calling the functions to get the max or min value in the array and those values are constantly updated, send the most recent value in and have a variable defined in each with the static storage class:

float get_array_max_value(float val){

static long tempMax = -100000F;

if (val >= tempMax)
      tempMax = val;
  return tempMax;
}

float get_array_min_value(float val){
   static long tempMin = 100000F;

if (val <= tempMin )
      tempMin = val;
  return tempMin ;
}




If you are only updating the last value in the array, why read the first 4 values when you know which one is the max value from a previous call? What am I missing?

I wasn't aware of static, but I'm not sure this is what I need. I need to know the max and min values from the last 5 readings, not from the start of the program.

jremington:
What do you mean by "a long time"? It should take no longer than a millisecond or so to determine the min and max values of an array of just a few elements. (5 elements? -- as suggested by the declaration "const int recording_time = 5;" ?)

If you want to tell where your program is stuck, sprinkle some serial.print statements throughout the code, saying "I'm here" and print out the current time using millis().

I agree, although my code is really inefficient it seems weird it could actually cause a delay, but my tests show that is the case. Maybe I'll create a thread later about that problem but for now it would be nice to just improve this part of the code a little.

11

OK...my code won't work then.

12

Maybe I'll create a thread later about that problem but for now it would be nice to just improve this part of the code a little.

If you are talking about the functions that find min/max, those are coded pretty much as I would. I don't know of a better or faster way.

I think something else is slowing the program down, whatever "slow" means. You haven't answered the question about how long is "a long time".

13

I haven't read everything here but I spotted this line in your code

if(millis() == thermocouple.timeout)

That is not the correct way to use millis() for 2 reasons.

First millis() does not return every successive value so it might never actually equal the timeout value - you should use >=

Second, the code will fail gloriously when millis() rolls over to 0.

Your test should be "millis() - prevMillis >= interval"

You need to post the entire code rather than little pieces that have no context. If it's a large program perhaps you can write a short sketch that illustrates the problem.

...R