Go Down

Topic: ****What is wrong or problematic with this Function**** (Read 938 times) previous topic - next topic

aerouta

Hello forum users! After trying endlessly to get this function working correctly I am at the verge of throwing in the towel. This function is performing a color matching rountine. The error seems to be due to the large "const float" arrays. This function is part of a larger program. When I include this function in the program, the program complies but does not function. The arduino mega board just continues to reset. If I reduce the size of the arrays to  ~4 entires it works fine. I dont know if the issue is with this function or the rest of the code but I figure I can start with the last thing added, which is this function.

Any general suggestions would great.

Code: [Select]

void TC_colormatch(int red, int green, int blue){  //pass RGB valves that will be converted to HSL and matched to "estimate"

float h = red; float s = green; float l = blue;
float TC_value;
rgb2hsl(h,s,l); //this replace the current hsl address values with real HSL
Serial.print("hue "); Serial.println(h); Serial.print("sat "); Serial.println(s); Serial.print("lum "); Serial.println(l);  Serial.print("\n"); 
h = h*6.2832; //convert h from 0-1 to 0-2pi  (h*2pi)

Serial.println("matching...");
// these estimates are being used to find where the measured color sample(h,s,l) match.
const float h_estimate[] = {0.4233, 0.3781, 0.3305, 0.2805, 0.2282, 0.2011, 0.1729, 0.1132, 0.1007, 0.0879, 0.0748, 0.0654, 0.0558, 0.0462, 0.0414, 0.0366, 0.0317, 0.0052, 6.2611, 6.2333};
const float S_estimate[] = {0.5054, 0.4921, 0.4798, 0.4687, 0.4588, 0.4499, 0.4413, 0.4253, 0.4207, 0.4162, 0.4118, 0.4094, 0.4071, 0.4048, 0.4024, 0.4, 0.3976, 0.3928, 0.3883, 0.3841};
const float L_estimate[] = {0.7294, 0.7137, 0.698, 0.6824, 0.6667, 0.6706, 0.6745, 0.6824, 0.6771, 0.6719, 0.6667, 0.6641, 0.6614, 0.6588, 0.6562, 0.6536, 0.651, 0.6484, 0.6458, 0.6431};
const float TC_test[] = {1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 2, 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 2.8, 2.9, 3, 3.1, 3.2, 3.3, 3.4, 3.5};

const int est_size = sizeof(h_estimate)/sizeof(float); //determines the size of the array
float mag_vector[est_size];   //create an array for the difference vectors
for (int i = 0; i<est_size; i++){
mag_vector[i] = sqrt(S_estimate[i]*S_estimate[i]+s*s - 2*S_estimate[i]*s*cos(h_estimate[i]-h));
}
int n; int min_location; //used to determine the minium location in the array mag_vector
float minval = 3.402823466e+38f;  //maxium value of a float;
for(n=0; n<est_size-1; n++){   //step through all values of the array
if(mag_vector[n] < minval){  //check to see if indexed value is less than previous
minval = mag_vector[n];  //if less assign to minval
min_location = n;  // cature the location of the minium value
}
}
TC_value = TC_test[min_location]; //get the value of "TC_test" at min_location
Serial.print("Value = ");Serial.println(TC_value);Serial.print("\n\n");
}


PaulS

Quote
If I reduce the size of the arrays to  ~4 entires it works fine. I dont know if the issue is with this function or the rest of the code but I figure I can start with the last thing added, which is this function.

Then, that is a clue that you are running out of memory.

What does this tell you, when called in that function?
http://playground.arduino.cc/Code/AvailableMemory

aerouta

Thanks, I will look into that once I get home.

Any other suggestions regarding the function. I have only been using C++/Arduino for a few months so I am open to suggestions.

kelvin31415

Just some general code critique:

In place of
Code: [Select]
float minval = 3.402823466e+38f;  //maxium value of a float;
you should consider including <float.h> and using FLT_MAX.

In this line
Code: [Select]
for(n=0; n<est_size-1; n++){   //step through all values of the array
it looks like you probably want "n < est_size".

Your L_estimate array is unused. Did you mean to use it in the computation of mag_vector?

Your TC_test array unnecessarily has more elements than the other arrays, and it appears that it could be dispensed with in favor of a calculation.

Your use of "sizeof(h_estimate)/sizeof(float)" is not incorrect, but it would be more conventional to use "sizeof(h_estimate)/sizeof(h_estimate[0])".

Your mag_vector array is not needed if you have no other plans for it. You could do your computation in one loop:
Code: [Select]

float min_mag = FLT_MAX;
int min_index;

for (int i = 0; i < est_size; i++) {
float mag = sqrt(S_estimate[i]*S_estimate[i]+s*s - 2*S_estimate[i]*s*cos(h_estimate[i]-h));

if (mag < min_mag) {
min_mag = mag;
min_index = i;
}
}

ckiick

Without the whole sketch, can only offer suggestions.

For memory saving:
- L_estimate is not used in the function, so it can be removed.

- There is a direct relationship between TC_test value and the index. Therefore it can be computed instead of stored.  TC_value = (11.0 + (float)n)/10.0;

- Since S_estimate and h_estimate are const, you can use PROGMEM to store them, saving some ram there. Note that float is not directly supported, but you can use longs and fake it.
http://arduino.cc/en/Reference/PROGMEM

- Since mag_vector is only referenced once, you can compute it on the fly instead of storing it as well, thus eliminating yet another array.

Other possible problems:
- the function rgb2hsl() doesn't look correct.  SInce you didn't include the code, I can't tell.  If the h, s, l variables are really going to be changed, then they should be passed by reference: rgb2hsl(&h, &s, &l);

- The whole function's only purpose as written is to print out the TC value.  It doesn't change any globals, doesn't modify any of its parameters and doesn't return anything.  That doesn't seem very useful.

- I think you may have an off-by-one error in your second for loop.  You should use n<est_size. As written it will skip the last element of the array.

Other optimizations/changes:
- You don't need the max value of a float, just start with the first element of the array.


Here's a quick cut at an improved function. WARNING: haven't tested or even compiled it. The PROGMEM stuff may be wrong.  This is just to give you an idea.

Code: [Select]

#define EST_SIZE        20

PROGMEM prog_uint32_t h_est_store[EST_SIZE] = { 4233, 3781, 3305, 2805, 2282, 2011, 1729, 1132, 1007,  879,  748,  654,  558,  462,  414,  366,  317,   52, 62611, 62333 };

PROGMEM prog_uint32_t S_est_store[EST_SIZE] = { 5054, 4921, 4798, 4687, 4588, 4499, 4413, 4253, 4207, 4162, 4118, 4094, 4071, 4048, 4024, 4000, 3976, 3928, 3883, 3841 };

//pass RGB valves that will be converted to HSL and matched to ?estimate?
float TC_colormatch(int red, int green, int blue)
{

        float h = red; float s = green; float l = blue;
        int n;
        float minval;
        float curval;
        int min_location;
        float S_estimate, h_estimate;
        float TC_value;

        rgb2hsl(&h,&s,&l); //this replace the current hsl address values with real HSL
        Serial.print("hue "); Serial.println(h); Serial.print("sat "); Serial.println(s); Serial.print("lum "); Serial.println(l);  Serial.print("\n");
        h = h*6.2832; //convert h from 0-1 to 0-2pi  (h*2pi)

        Serial.println("matching...");
        // these estimates are being used to find where the measured color sample(h,s,l) match.

        S_estimate = (float)pgm_read_dword_near(S_est_stor) / 10000.0;
        h_estimate = (float)pgm_read_dword_near(h_est_stor) / 10000.0;
        minval = sqrt(S_estimate*S_estimate+s*s - 2*S_estimate*s*cos(h_estimate-h));
        min_location = 0;

        for(n=1; n < EST_SIZE; n++){
                S_estimate = (float)pgm_read_dword_near(S_est_stor + n) / 10000.0;
                h_estimate = (float)pgm_read_dword_near(h_est_stor + n) / 10000.0;
                curval = sqrt(S_estimate*S_estimate+s*s - 2*S_estimate*s*cos(h_estimate-h));
                if(curval < minval){
                        minval = curval;
                        min_location = n;  // capture the location of the minium value
                }
        }
        TC_value =  (11.0 + (float)n)/ 10.0
        Serial.print("Value = ");Serial.println(TC_value);Serial.print("\n\n");
        return TC_value;
}

Chris J. Kiick
Robot builder and all around geek.

aerouta

#5
Jun 05, 2013, 10:23 pm Last Edit: Jun 05, 2013, 10:29 pm by aerouta Reason: 1
    Very helpful response.

    • I will look into using float.h

    • The L_estimate is not currently being used so I can remove

    • TC_test was replaced with generic values since I am posting in a public forum. But you do bring up a good point. If the real data can be fit to a polynomial it would be worth replacing.

    • I looked into the PROGMEM. I have never used it before and I was discourage by the lack of float support. I will go ahead and implement this change this evening

    • I like the suggestion on the magvector variable that will be changed also

    • I believe the varibles h s l are passed to rgb2hsl the declaration of the function looks like this. void rgb2hsl(float &R, float &G, float &B). I have not had an issue doing it this way. Please let me know if there is a proper way to do this

    • You are correct that the function does not change any globals etc. The purpose is to match the RGB readings to a predetermined set.

    • You guys are correct on the use of -1 on the for loop.




    Thanks again for the help

PQ

Hi there
Try declaring S_estimate and h_estimate as global variables instead of declaring them inside the function.
Since they are "const" it is safe to make them global variables.
Depending on how smart the compiler is, declaring these variables inside the function may use double the memory you'd naively think to store them. 
It is also possible that the compiler is storing the variables on the "stack" and causing a stack overflow. Declaring them as global variables avoids this.
As this coding change is easy to make, I suggest you just try it out.

aerouta

Thanks a ton for the suggestions. Used the MemoryFree.h file and I noticed that I was very low on mem. I implemented the PROGMEM routine and it seems to have solved the issue.

Now I need to do this at several other locations in the code.

Go Up