Bit setting and code reading

Question

I have an Arduino application (well, the library itself), in which there are several status flags, and initially I just declared them as ints (well, uint8_t and 8 bits unsigned in this case). But I could combine them all into a single whole and use bit masks to set and check the status.

An example of the first:

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

last example

 #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 effective (space and time). Should time and time effectively win in this situation, or is it some kind of optimization that should only happen when necessary?

(Added)

For completeness, here is the definition of the wiring of these Write bits, 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 Cheng great on this. Thus, you need to make a detailed calculation to find out whether the latter case is actually more effective, depending on how many objects exist and how many calls really set these conditions.

In terms of readability, it looks like you are doing this with member variables, which means you probably encapsulated them in good functions. In this case, I'm not so interested in readability, because at least the code for people using the class looks good. However, you can always encapsulate it in private functions if that bothers you.

Answer 2

Depending on the correspondence of the AVR-GCC compiler, which I do not know about, you can do something like this and keep something 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 to access the entire int flag immediately, 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; 

Then you can access the bit with two levels of indirection: variable.bits.flag1 <- returns a one-bit flag or with one level to get the full value of flags int: variable.val <- returns int

Answer 3

It might be clearer if you eliminate the need to use the HIGH and LOW constants by splitting into two methods. Just create the bitSet and bitClear . bitSet sets the bit to HIGH , and bitClear sets the bit to LOW . Then it will be:

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

And of course, if you only have HIGH == 1 and LOW == 0 , you do not need the == check.

Answer 4

In my opinion, even your latest code is still quite readable. By giving a name to each of your flags, you can read the code effortlessly.

A bad way to do this is to use magic numbers:

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

Answer 5

If you do not need to optimize, do not do this and use the simplest solution.

If you need to optimize, you need to know why:

• The first version will be minimally fast if you set or clear the bit instead of switching it, because then you do not need to read the memory.

• The first version is better wrt concurrency. In the second, you have read-modify-write, so you need to make sure that the byte of memory is not accessing at the same time. You usually disable interrupts, which slightly increases the interrupt delay. In addition, forgetting to disable interrupts can lead to very unpleasant and difficult to find errors (the most unpleasant error that I have encountered so far was just that).

• The first version is minimally better than wrt (less use of flash), since each access is a download or storage operation. The second approach requires additional bit operations.

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

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

Answer 6

If you're talking about readability, bit sets, and C ++, why don't I find something on std::bitset there? I understand that the programmer’s built-in race is quite comfortable with bitmasks and developed blindness for its obvious ugliness (masks, not races :), but in addition to masks and bitfields, the standard library also has a rather elegant solution.

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 is better to use logical operations so that you can:

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

It now looks first with the latter's effectiveness. Now you can have macros, not functions, therefore (if I succeed, it will be something like):

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

You have no overhead for calling functions, and the code becomes more understandable.

I have not played with Arduino (yet), but there may already be macros for this kind of thing, I don’t know.

Answer 8

I would say that the first thing that interests me is: "#define SHIFT 0" Why not use a constant, not a macro? In terms of efficiency, the constant allows you to determine the type, thereby ensuring that no conversion is required after that.

As for the efficiency of your technique: - firstly, get rid of the else clause (why set the bit to HIGH if its value is already HIGH?) - secondly, they prefer to have something readable first, built-in setters / getters that use internal masking bits, would do the job, be efficient and readable.

As for storage, for C ++ I would like to use a bit set (in combination with an enumeration).

Answer 9

Too easy to say:

 flags ^= bit;