prevent arithmetic overflow

178 Views Asked by At

I want to calculate a voltage using ADC peripheral of PIC18F14K50. The result ranges between 0-1023 (10-bit). So I used this simple calculation:

uint16_t voltage = ADC_Result * 5000 / 1023;

However, the results are incorrect. I guess an arithmetic overflow happened. I tried many combination of parentheses, changing order of elements, etc.
The best result was 4088 when ADC_Result was 1023 using below code; which is really far from 5000.

uint16_t voltage = ADC_Result * (5000 / 1023);

What should I do to get better results in above calculation? Please don't suggest floating points as they cause disaster in MCUs! They use a lot of resources without any real benefit.

3

There are 3 best solutions below

9
Ian Abbott On BEST ANSWER

You could use a wider type for the intermediate multiplication, for example:

uint16_t voltage = (uint32_t)ADC_Result * 5000 / 1023;

EDIT

If division by 1023 is too slow, you can get an approximately equal conversion by changing 5000 / 1023 to 5005 / 1024 which can use a fast bit shift for the division:

uint16_t voltage = (uint32_t)ADC_Result * 5005 >> 10;

N.B. 1023 * 5005 / 1024 ≃ 5000.1123

0
ForceBru On

You should use a wider integer type for this calculation, such as uint32_t.

In your case, 1023 * 5000 == 3192 (because the real result 5115000 doesn't fit), so that's not correct. 5000 / 1023 == 4, which is the expected result for integer division. Dividing ADC_Result by 1023 will result in the same behaviour.

You can calculate this into uint32_t and then check if it fits in uint16_t:

uint32_t result_tmp = ADC_Result * (5000 / 1023);
uint16_t result;

if (result > 0xffff) {
    // this won't fit
} else {
    result = (uint16_t) result_tmp;
}
0
chux - Reinstate Monica On

What should I do to get better results in above calculation?

OP's code overflow 1`6-bit math.


To get a correct and rounded result use wider math and offset.

// uint16_t voltage = ADC_Result * 5000 / 1023;
uint16_t voltage = (ADC_Result * 5000LU + 1024u/2) / 1024u;
// or
#include <stdint.h>
...
uint16_t voltage = (ADC_Result * UINT32_C(5000) + 1024u/2) / 1024u;

The L in 5000LU provides at least a 32-bit math.

Use U for potentially simpler/faster math and simpler rounding given ADC_Result is not negative.

+ 1024/2 effects a round to closest rather than a truncate.

Use 1024 instead of 1023 for correct scaling given the usual characteristics of A/D converters. Side benefit: faster division as 1024 is a power-of-2.