unsigned int VoltLimitMin = 122; /* 3.30 Volts */
unsigned int VoltLimitMax = 179; /* 5.10 Volts */
unsigned int VoltLimitTrip = 205; /* 5.55 Volts */
The purpose of this code is to set min, max, and emergency shutdown thresholds for voltage coming from some A/D converter. The programmer has manually converted the voltage value to the number of A/D integer steps that corresponds to. (Similar code might be used to express time in terms of timer ticks as well.) Do you see the bug in the above code? Would you have noticed if I hadn't told you a bug was there?
Here is a way to save some trouble and get rid of that type of bug. First, figure out how many steps there are per unit that you care about, such as:
// A/D converter has 37 steps per Volt ( 37 = 1 Volt, 74 = 2 Volts, etc.)
#define UNITS_PER_VOLT 37
then, create a macro that does the conversion such as:
#define VOLTS(v) ((unsigned int) (((v)*UNITS_PER_VOLT)+.5))
Here is an example to explain how that works. If you say VOLTS(3.30) then "v" in the macro is 3.30. It gets multiplied by 37 to give 122.1. Adding 0.5 lets it round to nearest positive integer, and the "(int)" ensures that the compiler knows you want it to be an unsigned integer result instead of a floating point number.
Then you can use:
unsigned int VoltLimitMin = VOLTS(3.30);
unsigned int VoltLimitMax = VOLTS(5.10);
unsigned int VoltLimitTrip = VOLTS(5.55);
Because the macros are expanded in-line by the preprocessor, most compilers are able to compile exactly the same code as if you had hand-computed the number (i.e., they compile to a constant integer value). So this macro shouldn't cost you anything at run time, but will remove the risk of for hand computation bugs or someone forgetting to update comments if the integer value is changed. Give it a try with your favorite compiler and see how it works.
Notes:
- The rounding trick of adding 0.5 only works with non-negative numbers.Usually A/D converters output non-negative integers so this trick usually works.
- This may not work on all compilers, but it works on all the ones I've tried on various microcontroller architectures. I saw one compiler do the float-to-int conversion at run-time if I left out the "(unsigned int)" in the macro, so make sure you put it in. Do a disassembly on your code to make sure it is working for you.
- The "const" keyword available in some compilers can optimize things even further and possibly avoid the need for a macro if your compiler is smart enough, but I'll leave that up to you to play with if you use this trick in a real program.
- As mentioned by one of the comments, you might also check for overflow with an ASSERT.
Hi Phil,
ReplyDeleteOne small suggestion...
In the macro:
#define VOLTS(v) ((unsigned int) ((v*UNITS_PER_VOLT)+.5))
I might suggest addition of parentheses in the expansion, i.e.
#define VOLTS(v) ((unsigned int) (( (v) * UNITS_PER_VOLT)+.5))
Otherwise, when someone uses the macro in form such as:
unsigned int VoltLimitMin = VOLTS(3.0 + 0.30);
You could be in for a surprise thanks to C's operator precedence rules, and the "behind the scenes" macro expansion.
In the example above, suppose the 3.0 is some sort of DC offset or whatever, so the 0.30 amount is an offset, and the coder wants to make this explicit. If this was a function (including an inline function in C99 or C++) it wouldn't be a problem... but good old macros, they always find a way to bite you.
By the way, normally I also try to "use macros responsibly" - if such a thing is possible - by using parentheses on the calling side as well. For example:
unsigned int VoltLimitMin = VOLTS((3.0 + 0.30));
Since you don't always know how the macro will expand, it's a good idea to use parentheses liberally.
We've seen a lot of problems caused by things like this, problems that escape to the field. A coding standard (that is enforced!) is usually a good measure against this, but it's no guarantee - the code will still compile, and even test fine in some cases.
Minor nitpick: You are actually using double instead of float values.
ReplyDeleteWhat your macro does not do though, is to check for overflow. The computed float (or double) value might not fit into unsigned int. You could sneak an assert or something into your VOLTS macro.
Dan,
ReplyDeleteYou are absolutely right - thanks for catching that! And you are also right that peer review against a coding standard is the best way to catch this (which is kind of what happened here!).
I've changed the posting in case someone doesn't notice these comments. For completeness, before Dan's comments the offending line of code was:
#define VOLTS(v) ((unsigned int) ((v*UNITS_PER_VOLT)+.5))