7

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?

homecloud
  • 139
  • 8
  • 9
    Replace with what? For me this is perfectly readable. Maybe just add comment that this isolate bit 10 from (I assume) integer. – Rokta Jun 01 '20 at 07:16
  • Will it be possible to use some struct or union etc that can replace these operations? – homecloud Jun 01 '20 at 07:30
  • 2
    Nope. Bitfields are compiler-specific, those shifts work everywhere the same. – Turbo J Jun 01 '20 at 07:33
  • If for learning purpose I want to know how can I use Bitfields in this case, and assume any possible 'compiler-specific' thing, then how to do that? – homecloud Jun 01 '20 at 07:38
  • 3
    @homecloud You couldn't be using bit-fields, so don't study them. They are very poorly defined by the C standard, leading to brittle and completely non-portable code. Bit-shifts is the correct solution. – Lundin Jun 01 '20 at 11:33
  • 2
    It might be slightly more readable as ( ( x >> 10 ) & 1 ), but it's fine either way. – Cristobol Polychronopolis Jun 01 '20 at 19:32
  • @Lundin You have improved the macros.. but I wonder if there can be any alternate approach to do it other than using macros and bit-shift operations? – homecloud Jun 02 '20 at 12:31
  • Nothing that makes sense to use, no. The bitwise operators in C is what boils down to bit/byte instructions in the machine code. You _could_ do regulator arithmetic, such as + instead of OR, multiply instead of left shift etc, but that's probably just obfuscation making the code harder to read. There's also bit-fields, but they have so much non-deterministic behavior associated with them that code relying on bitfields can neither be read, understood or ported. – Lundin Jun 02 '20 at 13:04

1 Answers1

23

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)
Lundin
  • 17,577
  • 1
  • 24
  • 67
  • 1
    Goal reached: Provoked an answer from the experts. :-) – the busybee Jun 01 '20 at 11:35
  • 1
    @thebusybee I don't know why the answer from 'thebusybee' is deleted.. It was good to see that alternate solution also. – homecloud Jun 01 '20 at 11:49
  • One advantage of the `ERROR_BIT` macro over a named bitmask is that it returns either 0 or 1. You can use this to do things like count errors that you can't do with a mask. – Mark Jun 02 '20 at 00:42
  • 1
    I really, really, *really* wish you would have advised the person to replace dangerous macros with type-safe inline functions... Although I totally agree with the remainder of the answer. – Cody Gray - on strike Jun 02 '20 at 03:35
  • 1
    @CodyGray It's complicated. For basic stuff like using bitwise operations, you should not have macros or functions. For more complex operations, inline functions are overall better than macros, though they are not without problems of their own, such as namespace clutter and the inability to privately encapsulate the function (which you get with macros too). In many cases the correct solution is rather to use an ordinary function - and modern compilers may sometimes inline those. Yet another scenario is modern `_Generic` that can be used to increase type safety but requires macros to work. – Lundin Jun 02 '20 at 06:29
  • 3
    Please, name that thing `ERROR_MASK` or `ERROR_BIT_MASK` instead of just `MASK`. – Guntram Blohm Jun 02 '20 at 07:51
  • @GuntramBlohmsupportsMonica It's just an artificial example. In a real application, they would be named something self-explanatory, as in the example at the bottom of the post. – Lundin Jun 02 '20 at 08:04