So i have this line of C code that takes 10-bit ADC value on the ATmega2560 and scales it to be used later by counters. results should be between 44 and 4166 or something like that
uint16_t tcnt = (60 * 16000000)/((100 adc_latest * 9) * 64 * 36);
I compiled with avr-gcc and send the value of tcnt to the serial to check it .. it was way outside the expected range
after checking my math and parenthesis multiple times i thought i would simplify and re-write it as follow
uint16_t tcnt = (416667) / (100 adc_latest *9);
this time i got the expected values
can anyone explain this behavior ? is this because there is a very large intermediate value involved ?
CodePudding user response：
I'm guessing you're working on a 16-bit processor, and that you've been bitten by integer overflow.
I'm guessing adc_latest is an int (16 bits), or perhaps a uint16_t.
And the constants 100, 9, 64, and 36 are all small enough to be represented as 16-bit int. So the subexpression
(100 adc_latest * 9) * 64 * 36
will all be computed using 16-bit arithmetic.
Suppose adc_latest is 9000. Then (100 adc_latest * 9) * 64 * 36 is 20966400 — but that's not a number you can represent in 16 bits. So it'll overflow, with undesired and perhaps undefined results.
In the numerator, though, the constant 16000000 is already too big to be represented in 16 bits, so the compiler will implicitly treat it as a long int.
When you rewrote it as
416667 / (100 adc_latest * 9)
you eliminated the overflow in the denominator, so everything worked.
If my suppositions here are correct, another fix would be
(60 * 16000000) / ((100 adc_latest * 9L) * 64 * 36)
The simple change of 9 to 9L will force everything in the denominator to be computed using long arithmetic also, eliminating overflow there and (I predict) resulting in a correct overall result.
This is all a consequence of the way expressions are evaluated "bottom up" in C. Subexpressions get promoted to a larger, common type only when they have to be. So in your original expression, you ended up with
long / int
At that point, the int in the denominator got promoted to long — but by then it was too late; the overflow had already occurred. The compiler did not notice that since the denominator was eventually going to be promoted to long, it might as well evaluate the whole denominator as a long. It did not evaluate the whole denominator as a long, so overflow in the denominator occurred.
Addendum: I wrote, "When you rewrote it... you eliminated the overflow in the denominator", but that was an oversimplification. The example I gave, with adc_latest = 9000, works out to 81900, which is still an overflow. So that rewrite would work for most (but not all) values of adc_latest, those up to 7180 or so. So the other rewrite, forcing the entire denominator to be evaluated in 32 bits, is preferable.
There's probably some other way of rearranging the expression so that you could evaluate the whole thing using 16-bit arithmetic, if for some reason you wanted to avoid 32-bit arithmetic, but I'm not going to try to work it out just now.