Bit setting and code readability

Question

I have an Arduino application (well actually a library) that has a number of status flags in it - and originally I simply declared them as ints (well uint8_t so 8 bit unsigned chars in this case). But I could have combined them all into one integer and used bitmask operations to set and test the status.

An example of the former:

if (_shift == HIGH)
{
    _shift = LOW;
}
else
{
    _shift = HIGH;
}

an example of the latter

#define SHIFT_BIT 0

if (bitRead(_flags, SHIFT_BIT) == HIGH)
{
   bitWrite(_flags, SHIFT_BIT, LOW);
}
else
{
   bitWrite(_flags, SHIFT_BIT, HIGH);
}

The former reads better, but the latter is more efficient (space and time). Should the space and time efficiency always win in this situation or is this a kind of optimization that should only happen when needed?

(Added)

For completeness here's the Wiring definition of those bitWrite etc. macros:

#define bitRead(value, bit) (((value) >> (bit)) & 0x01)
#define bitSet(value, bit) ((value) |= (1UL << (bit)))
#define bitClear(value, bit) ((value) &= ~(1UL << (bit)))
#define bitWrite(value, bit, bitvalue) (bitvalue ? bitSet(value, bit) : bitClear(value, bit))

Answer 1

Check out Raymond Chen's excellent take on this issue. In summary, you need to do some detailed calculation to find out whether the latter case is actually more efficient, depending on how many objects there are vs. how many callsites actually set these states.

As far as readability, it looks like you're doing this with member variables, which means you've probably encapsulated them in nice functions. In that case, I'm not as concerned with readability because at least the code for the people using the class looks nice. However, you could always encapsulate it in private functions if it's a concern.

Answer 2

Depending on the compliance of the AVR-GCC compiler, which I'm unsure about, you can do something like this and keep things nice and clean.

struct flags {
    unsigned int flag1 : 1;  //1 sets the length of the field in bits
    unsigned int flag2 : 4;
}; 

flags data;

data.flag1 = 0;
data.flag2 = 12;

if (data.flag1 == 1)
{
    data.flag1 = 0;
}
else
{
    data.flag1 = 1;
}

If you also want access to the entire flag int at once, then:

union 
{
    struct {
        unsigned int flag1 : 1;  //1 sets the length of the field in bits
        unsigned int flag2 : 4;
    } bits;
    unsigned int val;
} flags;

You can then access a bit with 2 levels of indirection: variable.bits.flag1<--returns single bit flag or with a single level to get the entire int worth of flags: variable.val <--returns int

Answer 3

It can be clearer if you remove the need to use constants HIGH and LOW, by splitting into two methods. Just make bitSet and bitClear methods. bitSet sets the bit to HIGH, and bitClear sets the bit to LOW. Then it becomes:

#define SHIFT_BIT 0

if (bitRead(_flags, SHIFT_BIT) == HIGH)
{
    bitClear(_flags, SHIFT_BIT);
}
else
{
    bitSet(_flags, SHIFT_BIT);
}

And of course, if you just have HIGH == 1 and LOW == 0, then you don't need the == check.

Answer 4

To my eye, even your latter code is still quite readable. By giving a name to each one of your flags, the code can be read without much effort.

A poor way to do it would be to use "magic" numbers:

if( _flags | 0x20 ) {  // What does this number mean?
   do_something();
}

Answer 5

If you don't need to optimize, don't do it and use the simplest solution.

If you do need to optimize, you should know for what:

• The first version would be minimally faster if you only set or clear the bit instead of toggling it, because then you don't need to read the memory.

• The first version is better w.r.t. concurrency. In the second you have read-modify-write, so you need to make sure that the memory byte is not accessed concurrently. Usually you would disable interrupts, which increases your interrupt latency somewhat. Also, forgetting to disable interrupts can lead to very nasty and hard to find bugs (the nastiest bug I encountered so far was of exactly this kind).

• The first version is minimally better w.r.t. code size (less flash usage), because each access is a single load or store operation. The second approach needs additional bit operations.

• The second version is uses less RAM, especially if you have a lot of these bits.

• The second version is also faster if you want to test several bits at once (e.g. is one of the bits set).

Answer 6

If you're talking readability, bit-sets and C++, why don't I find anything on std::bitset in there? I understand that the embedded programmer race is quite comfortable with bitmasks, and has evolved a blindness for its sheer uglyness (the masks, not the races:), but apart from masks and bitfields, the standard library has a quite elegant solution, too.

An example:

#include <bitset>

enum tFlags { c_firstflag, c_secondflag, c_NumberOfFlags };

...

std::bitset<c_NumberOfFlags> bits;

bits.set( c_firstflag );
if( bits.test( c_secondflag ) ) {
  bits.clear();
}

// even has a pretty print function!
std::cout << bits << std::endl;// does a "100101" representation.

Answer 7

For bit fields, it's better to use logical operations, so you could do:

if (flags & FLAG_SHIFT) {
   flags &= ~FLAG_SHIFT;
} else {
   flags |= FLAG_SHIFT;
}

This now has the look of the former with the efficiency of the latter. Now you could have macros rather than functions, so (if I've got this right - it would be something like):

#define bitIsSet(flags,bit) flags | bit
#define bitSet(flags,bit) flags |= bit
#define bitClear(flags,bit) flags &= ~bit

You don't have the overhead of calling functions, and the code becomes more readable again.

I've not played with Arduino (yet) but there might already be macros for this sort of thing, I don't know.

Answer 8

I would say the first thing I am concerned with is: "#define SHIFT 0" Why not make use of a constant rather than a macro? As far as efficiency get, the constant allow to determine the type, thus making sure than no conversion will be needed afterwards.

As for the efficiency of your technic: - first, get rid of the else clause (why set the bit to HIGH if its value is already HIGH ?) - second, prefer to have something readable first, inlined setters / getters using bit masking internally would do the job, be efficient and be readable.

As for the storage, for C++ I would tend to use a bitset (combined with an enum).

Answer 9

Is it too simple just to say:

flags ^= bit;