prevent arithmetic overflow

Question

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.

There's two ways to fix this. Use a wider integer, as has already been suggested, or, if you can sustain some loss of precision and resolution, scale the input values down by first dividing by 10 or 100. I'd give an example in an answer if I had more time, but you can probably do the math and work out the details on your own, or someone will spot this and write it up for us. — jwdonahue, Jun 19 '20 at 16:12

Ian Abbott · Accepted Answer · 2020-06-20T23:07:43.027

2

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

edited Jun 20 '20 at 23:07

answered Jun 19 '20 at 16:05

Ian Abbott

15,083
19
33

When ADC_Result = 1023, result is __4995__ by your code which is much better than __4088__! – AmirSina Mashayekh Jun 19 '20 at 16:14
1

@AmirSinaMashayekh That's strange. The above code should convert an ADC_Result value of 1023 to a voltage value of exactly 5000. (Ah! I think you really meant ADC_Result = 1022.) – Ian Abbott Jun 19 '20 at 16:19
1

remebber that 32 bit division is very slow on 8 bits microcontrollers. It you do not need it for humans just do not convert it at all. – 0___________ Jun 19 '20 at 17:15
@IanAbbott Sorry, you are right! That was my error! – AmirSina Mashayekh Jun 19 '20 at 18:28
@P__J__ By now I read raw data received from USB HID. To be able to read them easily, I convert results to numbers then convert normal numbers to BCD numbers on MCU (eg. 0x4995 for 4995). As soon as I start developing PC software for this project, I'll use raw data and process them on PC. Also, PIC18 devices have an **8 x 8 Hardware Multiplier** which makes multiplications (and probably divisions) much faster. – AmirSina Mashayekh Jun 19 '20 at 18:39
@AmirSinaMashayekh no it is very slow. – 0___________ Jun 19 '20 at 19:37
@P__J__ The whole process of processing data and sending them from terminal to MCU, processing data on MCU, doing 8 ADC conversions, converting them to real voltages and BCD numbers, sending them to PC and showing them in terminal just takes 5ms; which is fast enough for design and debug level! My MCU is clocked at 12MHz (48/4=12). And as I said, I'll eliminate these divisions and conversions from MCU after finishing debug. – AmirSina Mashayekh Jun 20 '20 at 06:05
1

@AmirSinaMashayekh If division by 1023 is too slow, you could divide by 1024 instead as that can be done with a simple bit shift. You can compensate by multiplying by 5005 instead of 5000. e.g. `uint16_t voltage = (uint32_t)ADC_Result * 5005 >> 10;` – Ian Abbott Jun 20 '20 at 22:48
@P__J__ Division by a constant _could_ use the fast hardware multiplier, using the reciprocal method. (I have no idea if the compiler for the PIC18 does that though.) – Ian Abbott Jun 20 '20 at 23:03

score 0 · Answer 2 · answered Jun 19 '20 at 16:06

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;
}

chux - Reinstate Monica · Answer 3 · 2020-06-20T01:19:59.180

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.

prevent arithmetic overflow

3 Answers3