Embedded C code review

Question

Hi, I am required to write a function that uses a look up table for ADC values for temperature sensor analog input, and it finds out the temperature given an ADC value by "interpolating" - linear approximation. I have created a function and written some test cases for it, I want to know if there is something which you guys can suggest to improve the code, since this is supposed to be for an embedded uC, probably stm32.

I am posting my code and attaching my C file, it will compile and run.

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

I also want to know a bit about the casting from uint32_t to float that I am doing, if it is efficient way to code.

#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

There is a lot you can improve. First of all the best integer data type depends on the machine (word size). I don't know how are your int8_t and uint16_t declared.

Also, not for performance but for readability, I usually don't use "cascading" ifs, like

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 the dir==1 twice, better to use ifs:

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

But there's more to say. For example you could use a faster searching algorithm.

Answer 2

Why does your bisection search have to handle both ascending and descending tables? The table is hard coded anyhow, so prepare your table offline, and you halve the complexity. You also generally don't need to do half your comparisons - just stop when high and low are adjacent or equal.

In such as small program, there is very little point checking for non-null table and other inputs and silently reporting no match - either return and error code, or make sure that the one place the function is called it's not being called with invalid pointers ( there is no reason to assume that an invalid pointer is NULL, just because NULL may be an invalid pointer on some systems).

Without the extra complexities, the search would become something like:

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 has been pre-prepared to be ascending, and search returns a meaningful enum rather than an integer code, you don't need that 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 all terms in the gradient calculation are 16 bit ints, you could perform the interpolation in 32 bits, as long as you calculate all terms of the numerator before dividing:

*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

It is good that you include the test framework, but your test framework lacks rigour and abuses the DRY (Don't Repeat Yourself) principle.

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 the test code for a single 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 drives 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 is going to be hard to excuse not spotting the problem. With your original code, someone could overlook the mistake.

Clearly, if you want the comments about the tests too, you can add a const char *tag to the array and supply and print those tags. If you really want to get fancy, you can even encode the null pointer tests (kudos for including those) in the regime by including appropriately initialized pointers in the array - you might add a pair of bit-flags for 'table is null' and 'temperature pointer is null' and conditional code in the function.