Undefined behaviour in LTC681x_run_adc_overlap()

[gudnimg]

This includes LTC681x.cpp, LTC6813.cpp, LTC6812.cpp.

See below LTC681x.cpp case:

for (int cic = 0; cic<total_ic; cic++)
{
    measure_delta = (int32_t)ic[cic].cells.c_codes[6]-(int32_t)ic[cic].cells.c_codes[7];

    if ((measure_delta>failure_pos_limit) || (measure_delta<failure_neg_limit))
    {
            error = error | (1<<(cic-1)); // <-- when cic = 0 we have a negative bitwise shift which is undefined behaviour
    }
}

On top of this I think the error counter should only count the errors.

for (int cic = 0; cic<total_ic; cic++)
{
    measure_delta = (int32_t)ic[cic].cells.c_codes[6]-(int32_t)ic[cic].cells.c_codes[7];

    if ((measure_delta>failure_pos_limit) || (measure_delta<failure_neg_limit))
    {
            error += 1;
    }
}

[gudnimg]

To add to this issue. We should remove these two variable definitions int32_t a, b; from LTC681x_run_adc_overlap() since they are never used for anything. Let's call it cleanup 👍

I'm also thinking if we can somehow use this function for LTC6813/LTC6812 as well? They have an additional overlap test. I'm thinking of a solution that looks something like this: Notice the #ifdef (LTC6813 || LTC6812)

I do realise this would require a systematic change across the library. Shouldn't be too much work though if there's time.

/*  Runs the ADC overlap test for the IC */
uint16_t LTC681x_run_adc_overlap(uint8_t total_ic, // Number of ICs in the system
                                cell_asic *ic // A two dimensional array that will store the data
                                )
{
    uint16_t error = 0;
    int32_t measure_delta =0;
    int16_t failure_pos_limit = 20;   //  20mV
    int16_t failure_neg_limit = -20; // -20mV
    uint32_t conv_time=0;  
    wakeup_idle(total_ic);
    LTC681x_adol(MD_7KHZ_3KHZ,DCP_DISABLED);
    conv_time = LTC681x_pollAdc();

    wakeup_idle(total_ic);
    error = LTC681x_rdcv(0, total_ic,ic);
    for (int cic = 0; cic<total_ic; cic++)
    {

        measure_delta = (int32_t)ic[cic].cells.c_codes[6]-(int32_t)ic[cic].cells.c_codes[7]; 
        if ((measure_delta>failure_pos_limit) || (measure_delta<failure_neg_limit))
        {
          error += 1
        }

                #ifdef (LTC6813 || LTC6812)
        measure_delta = (int32_t)ic[cic].cells.c_codes[12]-(int32_t)ic[cic].cells.c_codes[13]; 
        if ((measure_delta>failure_pos_limit) || (measure_delta<failure_neg_limit))
        {
                error += 1
        }
            #endif
        }
    return(error);
}

[gudnimg]

Another quick point is the header doxygen comment for the function

/*!
 Helper Function that runs the ADC Overlap test 
 @return uint16_t, error
  0: Pass
 -1: False, Error detected 
 */

It should look something like this:

/*!
 Helper Function that runs the ADC Overlap test 
 @return uint16_t, error
  0: Pass, No errors detected.
  Else returns the number of errors detected > 0.
 */

This makes things inline with how this function is used for example for the LTC6811 evaluation board DC2259A.