Built-in C code overview

Question

I need to write a function that uses the lookup table for the ADC values ​​for the analog input of the temperature sensor, and detects the temperature based on the ADC value by "interpolation" - a linear approximation. I created a function and wrote some test cases for it, I want to know if there is something that you guys can offer to improve the code, as it is assumed for the built-in uC, possibly stm32.

I submit my code and attach my C file, it will compile and run.

Please let me know if you have comments / suggestions for improvement.

I also want to learn a little about casting from uint32_t for float, which I do if this is an efficient way of coding.

#include <windows.h> #include <stdio.h> #include <stdlib.h> #include <stdint.h> #define TEMP_ADC_TABLE_SIZE 15 typedef struct { int8_t temp; uint16_t ADC; }Temp_ADC_t; const Temp_ADC_t temp_ADC[TEMP_ADC_TABLE_SIZE] = { {-40,880}, {-30,750}, {-20,680}, {-10,595}, {0,500}, {10,450}, {20,410}, {30,396}, {40,390}, {50,386}, {60,375}, {70,360}, {80,340}, {90,325}, {100,310} }; // This function finds the indices between which the input reading lies. // It uses an algorithm that doesn't need to loop through all the values in the // table but instead it keeps dividing the table in two half until it finds // the indices between which the value is or the exact index. // // index_low, index_high, are set to the indices if a value is between sample // points, otherwise if there is an exact match then index_mid is set. // // Returns 0 on error, 1 if indices found, 2 if exact index is found. uint8_t find_indices(uint16_t ADC_reading, const Temp_ADC_t table[], int8_t dir, uint16_t* index_low, uint16_t* index_high, uint16_t* index_mid, uint16_t table_size) { uint8_t found = 0; uint16_t mid, low, high; low = 0; high = table_size - 1; if((table != NULL) && (table_size > 0) && (index_low != NULL) && (index_mid != NULL) && (index_high != NULL)) { while(found == 0) { mid = (low + high) / 2; if(table[mid].ADC == ADC_reading) { // exact match found = 2; } else if(table[mid].ADC < ADC_reading) { if(table[mid + dir].ADC == ADC_reading) { // exact match found = 2; mid = mid + dir; } else if(table[mid + dir].ADC > ADC_reading) { // found the two indices found = 1; low = (dir == 1)? mid : (mid + dir); high = (dir == 1)? (mid + dir) : mid; } else if(table[mid + dir].ADC < ADC_reading) { low = (dir == 1)? (mid + dir) : low; high = (dir == 1) ? high : (mid + dir); } } else if(table[mid].ADC > ADC_reading) { if(table[mid - dir].ADC == ADC_reading) { // exact match found = 2; mid = mid - dir; } else if(table[mid - dir].ADC < ADC_reading) { // found the two indices found = 1; low = (dir == 1)? (mid - dir) : mid; high = (dir == 1)? mid : (mid - dir); } else if(table[mid - dir].ADC > ADC_reading) { low = (dir == 1)? low : (mid - dir); high = (dir == 1) ? (mid - dir) : high; } } } *index_low = low; *index_high = high; *index_mid = mid; } return found; } // This function uses the lookup table provided as an input argument to find the // temperature for a ADC value using linear approximation. // // Temperature value is set using the temp pointer. // // Return 0 if an error occured, 1 if an approximate result is calculate, 2 // if the sample value match is found. uint8_t lookup_temp(uint16_t ADC_reading, const Temp_ADC_t table[], uint16_t table_size ,int8_t* temp) { uint16_t mid, low, high; int8_t dir; uint8_t return_code = 1; float gradient, offset; low = 0; high = table_size - 1; if((table != NULL) && (temp != NULL) && (table_size > 0)) { // Check if ADC_reading is out of bound and find if values are // increasing or decreasing along the table. if(table[low].ADC < table[high].ADC) { if(table[low].ADC > ADC_reading) { return_code = 0; } else if(table[high].ADC < ADC_reading) { return_code = 0; } dir = 1; } else { if(table[low].ADC < ADC_reading) { return_code = 0; } else if(table[high].ADC > ADC_reading) { return_code = 0; } dir = -1; } } else { return_code = 0; } // determine the temperature by interpolating if(return_code > 0) { return_code = find_indices(ADC_reading, table, dir, &low, &high, &mid, table_size); if(return_code == 2) { *temp = table[mid].temp; } else if(return_code == 1) { gradient = ((float)(table[high].temp - table[low].temp)) / ((float)(table[high].ADC - table[low].ADC)); offset = (float)table[low].temp - gradient * table[low].ADC; *temp = (int8_t)(gradient * ADC_reading + offset); } } return return_code; } int main(int argc, char *argv[]) { int8_t temp = 0; uint8_t x = 0; uint16_t u = 0; uint8_t return_code = 0; uint8_t i; //Print Table printf("Lookup Table:\n"); for(i = 0; i < TEMP_ADC_TABLE_SIZE; i++) { printf("%d,%d\n", temp_ADC[i].temp, temp_ADC[i].ADC); } // Test case 1 printf("Test case 1: Find the temperature for ADC Reading of 317\n"); printf("Temperature should be 95 Return Code should be 1\n"); return_code = lookup_temp(317, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); printf("Temperature: %d C\n", temp); printf("Return code: %d\n\n", return_code); // Test case 2 printf("Test case 2: Find the temperature for ADC Reading of 595 (sample value)\n"); printf("Temperature should be -10, Return Code should be 2\n"); return_code = lookup_temp(595, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); printf("Temperature: %d C\n", temp); printf("Return code: %d\n\n", return_code); // Test case 3 printf("Test case 3: Find the temperature for ADC Reading of 900 (out of bound - lower)\n"); printf("Return Code should be 0\n"); return_code = lookup_temp(900, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); printf("Return code: %d\n\n", return_code); // Test case 4 printf("Test case 4: Find the temperature for ADC Reading of 300 (out of bound - Upper)\n"); printf("Return Code should be 0\n"); return_code = lookup_temp(300, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); printf("Return code: %d\n\n", return_code); // Test case 5 printf("Test case 5: NULL pointer (Table pointer) handling\n"); printf("Return Code should be 0\n"); return_code = lookup_temp(595, NULL, TEMP_ADC_TABLE_SIZE, &temp); printf("Return code: %d\n\n", return_code); // Test case 6 printf("Test case 6: NULL pointer (temperature result pointer) handling\n"); printf("Return Code should be 0\n"); return_code = lookup_temp(595, temp_ADC, TEMP_ADC_TABLE_SIZE, NULL); printf("Return code: %d\n", return_code); // Test case 7 printf("Test case 7: Find the temperature for ADC Reading of 620\n"); printf("Temperature should be -14 Return Code should be 1\n"); return_code = lookup_temp(630, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); printf("Temperature: %d C\n", temp); printf("Return code: %d\n\n", return_code); // Test case 8 printf("Test case 8: Find the temperature for ADC Reading of 880 (First table element test)\n"); printf("Temperature should be -40 Return Code should be 2\n"); return_code = lookup_temp(880, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); printf("Temperature: %d C\n", temp); printf("Return code: %d\n\n", return_code); // Test case 9 printf("Test case 9: Find the temperature for ADC Reading of 310 (Last table element test)\n"); printf("Temperature should be 100 Return Code should be 2\n"); return_code = lookup_temp(310, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); printf("Temperature: %d C\n", temp); printf("Return code: %d\n\n", return_code); printf("Press ENTER to continue...\n"); getchar(); return 0; } 

Answer 1

I usually compute the lookup table offline, and the runtime code comes down to:

  temp = table [dac_value];

In particular, if you are going to be introduced, you do not need a floating point, often it is not needed. Pre-mailing the table also solves this problem.

Precomputing also solves the problem of having an efficient algorithm, you can be sloppy and slow as you want, you only need to do this calculation rarely. No algorithm can compete with a lookup table at runtime. As long as you have space for your lookup table, this is a win-win. If you don't say 256 places in the final exam for an 8-bit dac, for example, you can have 128 locations and you can do a little real-time interpolation:

  // TODO add special case for max dac_value and max dac_value-1 or make the table 129 entries deep
 if (dac_value & 1)
 {
   temp = (table [(dac_value >> 1) +0] + table [(dac_value >> 1) +1]) >> 1;
 }
 else
 {
    temp = table [dac_value >> 1];
 }

I often find that the table being fed can and will change. Yours can be thrown into stone, but the same calculations occur with calibrated devices. And you did the right thing by checking that the data is in the right general direction (decreasing with respect to dac, increasing or increasing with respect to dac values) and, more importantly, check for division by zero. Although this is a hard coded table, habits are developed with the expectation that it will change to another hard coded table with the desire not to change each interpolation code.

I also believe that the raw dac value is more important here, the calculated temperature can occur at any time. Even if the conversion to degrees of some flavor was cast in stone, it would be nice to display or save the original dac value along with the calculated temperature. You can always recalculate the temperature from the dac value, but you cannot always accurately reproduce the raw dac value from the calculated value. It depends on what you are building naturally, if it is a thermostat for public use in their homes, they do not want to have any hexadecimal value on the display. But if this is some kind of test or engineering environment in which you collect data for subsequent analysis or verification that a product is good or bad, transferring this dac value can be good. This will take one or two times in a situation where the engineer who provided you with the table claims that it is the final table and then changes it. Now you need to go back to all the logs that used the wrong table, calculate back the dac value using the previous table and recompile temp with the new table and write a new log file. If you had a raw dac value and everyone was trained to think in terms of dac values ​​and that temperature was just a reference, you might not have to restore the old log values ​​for each new calibration table. The worst case is only the temperature in the log file and cannot determine which cal table was used for this log file, the log file becomes invalid if the module under test becomes a risk element, etc.

Answer 2

Why should your bisection search process both upstream and downstream tables? In any case, the table is hardcoded, so prepare the table offline, and you will reduce the complexity by half. You also usually do not need to make half of your comparisons - just stop when high and low are adjacent or equal.

In such a small program, there are very few check points for a nonzero table and other input data and silently reporting inconsistencies - either a return code or an error code, or make sure that one place where the function is called is not called with invalid pointers (there is no reason to assume that invalid pointer is NULL, only because NULL may be an invalid pointer to some systems).

Without additional complexity, the search would look something like this:

 enum Match { MATCH_ERROR, MATCH_EXACT, MATCH_INTERPOLATE, MATCH_UNDERFLOW, MATCH_OVERFLOW }; enum Match find_indices (uint16_t ADC_reading, const Temp_ADC_t table[], uint16_t* index_low, uint16_t* index_high ) { uint16_t low = *index_low; uint16_t high = *index_high; if ( low >= high ) return MATCH_ERROR; if ( ADC_reading < table [ low ].ADC ) return MATCH_UNDERFLOW; if ( ADC_reading > table [ high ].ADC ) return MATCH_OVERFLOW; while ( low < high - 1 ) { uint16_t mid = ( low + high ) / 2; uint16_t val = table [ mid ].ADC; if ( ADC_reading > val) { low = mid; continue; } if ( ADC_reading < val ) { high = mid; continue; } low = high = mid; break; } *index_low = low; *index_high = high; if ( low == high ) return MATCH_EXACT; else return MATCH_INTERPOLATE; } 

Since the table was previously prepared for ascending, and the search returns a meaningful enumeration, not an integer code, you do not need so much in lookup_temp:

 enum Match lookup_temp ( uint16_t ADC_reading, const Temp_ADC_t table[], uint16_t table_size, int8_t* temp) { uint16_t low = 0; uint16_t high = table_size - 1; enum Match match = find_indices ( ADC_reading, table, &low, &high ); switch ( match ) { case MATCH_INTERPOLATE: { float gradient = ((float)(table[high].temp - table[low].temp)) / ((float)(table[high].ADC - table[low].ADC)); float offset = (float)table[low].temp - gradient * table[low].ADC; *temp = (int8_t)(gradient * ADC_reading + offset); break; } case MATCH_EXACT: *temp = table[low].temp; break; } return match; } 

Given that all terms in gradient computing are 16-bit ints, you can interpolate in 32 bits if you calculate all the numerator members before division:

 *temp = temp_low + uint16_t ( ( uint32_t ( ADC_reading - adc_low ) * uint32_t ( temp_high - temp_low ) ) / uint32_t ( adc_high - adc_low ) ); 

Answer 3

There are many opportunities that you can improve. First of all, the best integer data type depends on the machine (word size). I do not know how your int8_t and uint16_t are declared.

In addition, not for performance, but for readability, I usually do not use cascading ifs, for example

 if condition { if another_condition { if third condition { 

but instead:

 if not condition return false; // Here the condition IS true, thus no reason to indent 

Another point of attention:

  low = (dir == 1)? mid : (mid + dir); high = (dir == 1)? (mid + dir) : mid; 

you do dir == 1 twice, it is better to use ifs:

 int sum = mid+dir; if dir == 1 { low = mid; high = sum; } else { low=sum; high=mid; } 

But there is still to say. For example, you can use a faster search algorithm.

Answer 4

It’s good that you included the test structure, but your test structure does not have rigor and abuses the principle of DRY (Do not Repeat Yourself).

 static const struct test_case { int inval; /* Test reading */ int rcode; /* Expected return code */ int rtemp; /* Expected temperature */ } test[] = { { 317, 1, 95 }, { 595, 1, -10 }, { 900, 0, 0 }, // Out of bound - lower { 300, 0, 0 }, // Out of bound - upper { 620, 1, -14 }, { 880, 2, -40 }, // First table element { 310, 2, 100 }, // Last table element }; 

Now you can write test code for one test in a function:

 static int test_one(int testnum, const struct test_case *test) { int result = 0; int temp; int code = lookup_temp(test->inval, temp_ADC, TEMP_ADC_TABLE_SIZE, &temp); if (temp == test->rtemp && code == test->rcode) printf("PASS %d: reading %d, code %d, temperature %d\n", testnum, test->inval, code, temp); else { printf("FAIL %d: reading %d, code (got %d, wanted %d), " "temperature (got %d, wanted %d)\n", testnum, test->inval, code, test->rcode, temp, test->rtemp); result = 1; } } 

And then the main program can have a loop that controls the test function:

 #define DIM(x) (sizeof(x)/sizeof(*(x))) int failures = 0; int i; for (i = 0; i < DIM(test); i++) failures += test_one(i + 1, &test[i]); if (failures != 0) printf("!! FAIL !! (%d of %d tests failed)\n", failures, (int)DIM(test)); else printf("== PASS == (%d tests passed)\n", (int)DIM(test)); 

Now, if there is a problem with any of the tests, it will be difficult to excuse if you do not identify the problem. With your source code, someone might ignore the error.

Obviously, if you want to get comments about the tests, you can add const char *tag to the array and provide and print those tags. If you really want to get fancy, you can even encode null pointer tests (for example, to enable these) into mode by including the corresponding initialized pointers in the array - you can add a couple of bit flags for "table is null" and "the temperature pointer is zero "and conditional code in function.