Electronic – How to get rid of shift and logical operations

cfirmwaremacromicrocontrollerstm32

I am trying to upgrade an old firmware of a 32-bit MCU. The fw has many macros in it which uses various operations eg., logical shift etc. I want to replace these shift and logical operations. An example is the following macro.

#define ERROR_BIT(x)  ((x & ( 1 << 10 )) >> 10)

The main reason to get rid of these operations is to improve the code readability. What can be a good alternate approach here?

Best Answer

The code you have posted is quite readable. C programmers are assumed to understand the meaning of commonly used operators and expressions. The bitwise operators in particular, in the context of embedded systems programming. They are however not required to understand the meaning of mysterious macros - I'll get back to that later.

First of all, there are other issues with the code not related to readability, namely that the shift operators are dangerous to use on signed types. For this reason, the 1 should always be written as 1u to guarantee an unsigned type. Also, there's a beginner-level bug: the macro parameter is not surrounded by parenthesis.

Furthermore, the right shift with 10 is strange and unnecessary. If the purpose of the macro is to give 1 or 0 depending on if a bit is set, you can convert it to a boolean expression by comparing the result with != 0, or by using !! in front of it. That way we don't risk bugs related to arithmetic shifts or implicit type promotions. Bugs fixed, we end up with this:

#define ERROR_BIT(x)  ( ((x) & (1u << 10 )) != 0 )

Did this improve readability? Not in the slightest! But it fixed 1 obvious and 2 potential bugs.

As it turns out, macros are not great at all for the purpose of increasing readability, quite the contrary. The first thing we should do to actually improve readability, is to replace the "magic numbers" with constants:

#define MASK (1u << 10)
...

#define ERROR_BIT(x)  ( ((x) & MASK)) != 0 )

where "MASK" should be given a more meaningful name depending on what the code does.

And this leads us to the root of the problem: namely programmers writing secret macro languages for the purpose of carrying out trivial operations. To vastly improve readability, we get rid of the macro:

if(var & MASK)

Which perfectly is readable to 100% of all C programmers. In a real-world example, you might have something like:

if(SPISR & SPISR_TXBUSY)
Related Topic