4
\$\begingroup\$

I was working on a project where I read a value from a 16-bit ADC and scaled it to obtain the reading of a sensor. For example:

uint16_t reading = sampleSensor();
uint16_t temperature = reading/0xFFFF*2.5*1000*2;

Where the ADC has a full scale value of 2.5 volts with output code 0xFFFF and my sensor has a response of 2 °C/mV output.

Obviously when this code runs, it doesn't work. The value of temperature jumps from 0 to 5000 when reading increases from 65534 to 65535.

So I rewrote it and evaluated the scaling expression to a single multiplication:

uint16_t reading = sampleSensor();
uint16_t temperature = reading*0.076295;

This works as expected and temperature increases by 1 °C when reading has increased by 14 bits. I like writing out the full expression so I can keep track of the ADC bit count, VFS, and sensor gain, and I assumed the compiler would evaluate this arithmetic expression, before performing the operation on the reading variable.

Is this an issue with a compiler (GNU ARM v7.2.1) setting, or do I have a fundamental misunderstanding?

\$\endgroup\$
7
  • 3
    \$\begingroup\$ The specific order of operations is not defined for the first statement as they all have the same ordering rules. The division by 0xFFFF could be the first operation and that would be legal. \$\endgroup\$ Jan 26 at 12:24
  • 3
    \$\begingroup\$ reading/0xFFFF seems to be an integer division. Did you try: uint16_t temperature = (reading*2.5*1000*2)/0xFFFF; and maybe there is no need for parentheses. \$\endgroup\$ Jan 26 at 12:27
  • 8
    \$\begingroup\$ Nothing to do with microcontrollers, it's just a C language problem. \$\endgroup\$
    – user253751
    Jan 26 at 20:37
  • 1
    \$\begingroup\$ This is fairly basic stuff. Unrelated to the bug, you shouldn't use floating point unless you have an actual FPU. \$\endgroup\$
    – Lundin
    Jan 27 at 14:22
  • 2
    \$\begingroup\$ If performance was an issue, it can even make sense to optimize the /0xFFFF part. That's an integer division and almost equal to >>16. The difference would be in or after the 16th bit, and you're rounding temperature to 16 bits anyway. \$\endgroup\$
    – MSalters
    Jan 27 at 15:15
27
\$\begingroup\$

This is not a compiler issue: doing the division first here is the legal behaviour, as division and multiplication have equal precedence and are evaluated left-to-right. (Also, when in doubt: use parentheses; there's no penalty.)

You are working with integers, so reading / 0xFFFF will always evaluate to 0 if reading is a uint16_t, unless reading == 0xFFFF.

If you want to use integers only, force the multiplications to be done first by using something like (reading * 10000) / 0xFFFF and make sure both the intermediate result (reading * 10000) and the result fit in the available bits (use uint32_t for such calculations).

Note that on MCUs without an FPU floating-point arithmetic is very slow and best avoided.

\$\endgroup\$
11
  • 5
    \$\begingroup\$ Not any bracket. Parentheses only. \$\endgroup\$ Jan 26 at 20:54
  • 4
    \$\begingroup\$ "doing the division first here is legal behaviour" -- or indeed the only way it may be compiled? \$\endgroup\$
    – ilkkachu
    Jan 26 at 22:28
  • 4
    \$\begingroup\$ @PeterMortensen "brackets" there would be normal in British English instead of "parentheses" (in both mathematical and programming usage) \$\endgroup\$
    – Chris H
    Jan 27 at 10:55
  • 3
    \$\begingroup\$ @Chris-H: Yes, language glitch. I suppose "parentheses" is less ambiguous, but, with tongue firmly in cheek: I am Dutch, and not all that well-versed in the patois spoken in the colonies. \$\endgroup\$
    – ocrdu
    Jan 27 at 11:16
  • 2
    \$\begingroup\$ Regarding overflow, one dirty detail is that 0xFFFF is of type unsigned int which is 32 bits on a EFR32. (Unlike 0x7FFF which is int... because... C language...) So it will type promote the uint16_t implicitly. Which is subtle and dangerous. For those who aren't certain of how all implicit type promotion rules work in C, then it is good advise to only ever use uint32_t when coding 32 bit embedded systems and tag all integer constants with postfix u. \$\endgroup\$
    – Lundin
    Jan 27 at 14:34
14
\$\begingroup\$

This is a fundamental C issue: you need to be extremely clear whether you're doing integer or floating-point arithmetic.

 uint16_t temperature = reading*0.076295;

That promotes "reading" to "float", because 0.076295 is a float literal, then does the multiplication.

uint16_t temperature = reading/0xFFFF*2.5*1000*2;

The first two elements are integers, so the division is done as integer. Then it's promoted to float.

uint16_t temperature = ((float)reading)/0xFFFF*2.5*1000*2;
uint16_t temperature = reading/((float)0xFFFF)*2.5*1000*2;

Either of those two ought to work, and makes the arithmetic explicit.

Note that on some microcontroller architectures, the floating point arithmetic may take much longer than the integer arithmetic. Try it on godbolt: you'll see that it's implemented as function calls rather than CPU instructions.

\$\endgroup\$
3
  • \$\begingroup\$ shouldn't even need the second set of parens, (float) reading / 0xFFFF * 2.5 * 1000 * 2; should be fine (and more readable, IMO). \$\endgroup\$
    – ilkkachu
    Jan 26 at 22:21
  • 3
    \$\begingroup\$ Even in your "improved" form, the operation will require a single-precision division and three double-precision multiplies. If the code were to compute reading * (float)(2.5*1000.0*2.0/65535.0), all of those computations would be simplified to one single-precision multiply. \$\endgroup\$
    – supercat
    Jan 26 at 23:13
  • 4
    \$\begingroup\$ "That promotes "reading" to "float"," Actually it promotes it to double, because 0.076295 is a double literal. \$\endgroup\$
    – Lundin
    Jan 27 at 14:51
6
\$\begingroup\$

In C, operators at the same level of precedence are evaluated in left-to-right order. So, in your first equation the division is done first.

As a general rule in integer arithmetic you should try to perform the multiplications first, while avoiding overflow. Do the division last.

But you have other concerns here. If you are using a 16-bit ADC then you should use uint32_t types for the computations. If you use a uint16_t and then divide by 0xFFFF you will never get anything other than 0x0001 or 0x0000. Also, you should be dividing by \$2^{16}\$ rather than \$2^{16}-1\$, which can be accomplished by a right shift of 16 bits if multiplication is expensive on your processor.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ +1 for mentioning uint32_t to avoid overflow \$\endgroup\$ Jan 27 at 10:31
  • \$\begingroup\$ A typical 16-bit ADC will represent a full-scale voltage with a reading of 0xFFFF. \$\endgroup\$
    – supercat
    Jan 27 at 22:39
  • \$\begingroup\$ @supercat: What ADC? The ones we sourced saturate around 0xFE00 though it would return 0xFFFF for overvoltage. \$\endgroup\$
    – Joshua
    Jan 28 at 0:30
  • \$\begingroup\$ @Joshua A unipolar 16-bit ADC should provide a maximum output value of 0xFFFF. It sounds like you had an 8-bit ADC but your driver returned the output value left-justified in a 16-bit word. \$\endgroup\$ Jan 28 at 2:57
  • \$\begingroup\$ @supercat True. But the value of an LSB is \$V_{REF}/2^N\$ where \$N\$ is the number of bits. So the maximum digital output value does not correspond exactly to \$V_{REF}\$ but to \$V_{REF}\times (2^N-1)/2^N\$....assuming that's the issue you were trying to address with your comment. \$\endgroup\$ Jan 28 at 3:01
3
\$\begingroup\$

Other answers have pointed out what you're doing wrong with precedence. But you do still have problems with using floating-point if your micro doesn't have floating-point support. The compiler will sort it out, but it'll be very slow.

For dividing by a constant value where you're scaling one fixed range to another fixed range, you can get a reasonable approximation by using selective bit-shifting. Right-shifting divides the value by powers of 2, and you can use this to get something close to the division you need. This will not be perfectly linear, but it will be much faster than using software floating-point, and if you have noise on your analogue signal and ADC measurement then the linearity of this scaling may not be an issue.

For your example, you can scale 0 to 65535 down to 0 to 4999 with the following:

uint16_t temperature = (reading >> 4) + (reading >> 6) + (reading >> 13) + (reading >> 15) - (reading >> 9);

With regular divides, this would be

uint16_t temperature = (reading / 16) + (reading / 64) + (reading / 8192) + (reading / 32768) - (reading / 512);

but of course you don't want to use actual divides! Compilers sometimes aren't smart enough to work out that dividing by a power-of-2 constant can be done with a bitshift, and if you rely on it with compilers that are smart enough, you can get tripped up when you change micros/compilers.

Checking this with Excel over the 0 to 65535 range, it is linear to 3 lsbs over the full range.

The general method is to start with a right-shift for the maximum input value (65535) which puts you in the right ballpark for the desired output (4999), and then progressively add (or subtract) more shifted terms until you get to the right total. Because you are correcting the difference using the upper bits of the value, it remains relatively linear.

This is basically a riff on the Taylor series. Surprisingly though, I've never seen it published anywhere before I came up with the idea. I first used it for the 10-bit ADCs which were/are common on PIC and Atmel microcontrollers, to normalise an ADC measurement of 0 to 1023 into a convenient hundredths-of-volts scaling of 0 to 1000 with less than 0.5 bit linearity error over the range. It was published in Everyday Practical Electronics in the early 2000s (and won me an LCR meter as the best submission for that edition! :) I would link to the article, but EPE's archives are a bit spotty and I can't easily find it now.

\$\endgroup\$
5
  • \$\begingroup\$ Order of evaluation doesn't really apply here. You seem to be mixing up the terms. See What is the difference between operator precedence and order of evaluation? \$\endgroup\$
    – Lundin
    Jan 27 at 14:24
  • \$\begingroup\$ "Compilers often aren't smart enough to work out that dividing by a power-of-2 constant can be done with a bitshift" 20 years ago maybe... can you name a modern one which can't do that optimization? \$\endgroup\$
    – Lundin
    Jan 27 at 14:39
  • 1
    \$\begingroup\$ @Lundin It depends on what you call "modern". For microcontrollers, their compiler support isn't necessarily improved substantially over the years. My last encounter with a little Atmel, about 6 years ago, the compiler certainly couldn't. I'll change that line to say "sometimes" though, because you're right that more modern compilers should be able to, but equally it's still more reliable not to make that assumption. \$\endgroup\$
    – Graham
    Jan 27 at 16:30
  • \$\begingroup\$ @Lundin Fair point that I should have said "precedence" instead of "order of evaluation". I was assuming the OP was a newbie and they might not have met the word before, so I thought that might be clearer. But yes, I should make that correct. \$\endgroup\$
    – Graham
    Jan 27 at 16:34
  • \$\begingroup\$ If you speak of AVR then it's some 30 years old legacy architecture. Overall, we ran out of arguments for using 8 bit MCUs well over 10 years ago. Sure, there's still tons of old crap out there, I maintain numerous products with old MCUs myself. But when writing code for a modern Cortex M, you shouldn't need to do such trivial optimizations (such as swapping x*2 with x<<1 etc) manually. \$\endgroup\$
    – Lundin
    Jan 28 at 7:55
2
\$\begingroup\$

[edited in response to comments]

To add to the other excellent answers, here is something I have found extremely helpful to clarify both order-of-ops and type conversion, avoid many of the integer math related bugs on the CERT list (mentioned here as an example, not for compliance purposes), and write significantly more reliable integer math code in C, compared to use of more complex expressions, parenthesized or otherwise:

1 arithmetic operation per line of code.

In addition, and this is even more controversial, I would prefer operators like += , -= , *= etc, rather than the infix forms ("a+b" etc). It will avoid many but not all gotcha's and draw attention the intermediate types.

Please don't take the above as an authoritative claim, rather it is my personal approach for making it easier to follow the effects of type conversion, overflows, and special cases, when writing integer math code.

\$\endgroup\$
18
  • 3
    \$\begingroup\$ This doesn't really answer the question at all. You should probably have made this a comment instead of an answer. \$\endgroup\$ Jan 26 at 18:02
  • 1
    \$\begingroup\$ fair enough, but the OP seems to already know that the source of the problem is type conversion, so this is intended to be more of a useful practice to bring it into the light in the future. \$\endgroup\$
    – Pete W
    Jan 26 at 18:22
  • 1
    \$\begingroup\$ @Peter Mortensen -- CERT list, integer section is here // re: division by 0xFFFF - one operation per line allows clear thinking when going through code looking for bugs \$\endgroup\$
    – Pete W
    Jan 26 at 21:19
  • 2
    \$\begingroup\$ This isn't very helpful advise, because the compound assignment operators like x+=y; are equivalent to x = x + y; and there may be implicit promotions inside those expanded expressions and you can't cast the operands to avoid such. So it is hard to achieve MISRA or CERT compliance when using compound assignment. Overall I would recommend to use MISRA-C over CERT-C, since the former is much more suited for embedded systems. \$\endgroup\$
    – Lundin
    Jan 27 at 14:38
  • 2
    \$\begingroup\$ Consider something like uint8_t x,y; ... x -= y;. Here both x and y are promoted to signed int by implicit integer promotion. An unsigned operation can't underflow but a signed one can. It's then undefined behavior and the compiler is free to generate some nonsense code and might do so during optimization. The bug fix is to cast both operands to a large unsigned type that isn't implicitly promoted. Now how do you do that with compound assignment? You can't. With simple assignment you could simply do x = (uint32_t)x - (uint32_t)y;. \$\endgroup\$
    – Lundin
    Jan 27 at 15:08
0
\$\begingroup\$

Magic Numbers

This is a magic number. Define it once at the top of your code, outside any individual function, so that it just gets calculated or assigned once, and include a comment (syntax will vary by language, and I haven't programmed in C in many years, but the concept will work in any language):

# temperature = reading/0xFFFF*2.5*1000*2
SENSOR_TEMP_FACTOR = 0.076295;

or calculate it in that statement:

float SENSOR_TEMP_FACTOR = 2.5 * 1000 * 2 / 0xffff;

You can (should) include more comments, explaining why 0xFFFF, 2.5, 1000 and 2.

Then use this constant everywhere:

uint16_t reading = sampleSensor() * SENSOR_TEMP_FACTOR

If you change to a different sensor type, you will only have to change one line in your code, instead of potentially several different lines for different readings.

\$\endgroup\$
6
  • 3
    \$\begingroup\$ um, no, manually writing it out is not good. Consider what happens if someone has to change one of the intermediate factors. They'll have to change it in two places, which is just a recipe for either the value or the comment getting not changed. Also, why bother deciding on the rounding when the compiler can do it for you. float SENSOR_TEMP_FACTOR = 2.5 * 1000 * 2 / 0xffff; \$\endgroup\$
    – ilkkachu
    Jan 26 at 22:26
  • \$\begingroup\$ @ilkkachu I actually considered writing out both methods. Thank you - I'll edit. \$\endgroup\$ Jan 26 at 22:46
  • 5
    \$\begingroup\$ Better still, give all those magic numbers names, and then the calculation line could look something like float SENSOR_TEMP_FACTOR = TEMP_SENSOR_MAX_VOLTS * MV_PER_V * TEMP_SENSOR_DEGREES_C_PER_MV / TEMP_SENSOR_MAX_TICKS \$\endgroup\$
    – Nate S.
    Jan 26 at 23:28
  • 1
    \$\begingroup\$ Also note that 2.5 is a double constant. Probably doesn't matter in this case, but it will on some FPUs. Make it a habit of always using 2.5f when using float. \$\endgroup\$
    – Lundin
    Jan 27 at 14:42
  • 1
    \$\begingroup\$ Just a detail - the advise about avoiding magic numbers is sound. But well, this question is about C. I wouldn't use pythons for embedded systems or as scarfs - in either case I might end up strangled... :) \$\endgroup\$
    – Lundin
    Jan 27 at 15:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.