Just discovered C's "bitfields"! How cool!
DavidZemon
Posts: 2,973
Someone told me about this crazy thing called "bitfields" about a month ago and I've been waiting for an opportunity to try it out. Of course, for normal desktop applications that sounds absolutely and utterly awful.... but for embedded applications, not at all!
So I just wrote a simple little test app and I'm really happy with it (compared to not having bitfields). I'm looking to use this in peripheral drivers where I have to set and clear various bits of an 8-bit value before writing to a register. My first test will be an ADXL345 driver, and the sample below is inspired by the BW_RATE register (0x2C).
The resulting output is:
I stuck the sizeof() in there because I wanted to see if it remained as compact (and accurate) as possible. Indeed, in the form above it prints a size of 1. If I print sizeof(Foo) I also get 1. If I change rateCode to be 9 bits instead of 3, I get sizeof(MyByte) = 2. Life seems dandy and it's working great!
So has anyone else run across this before? Do you have any observations about this syntax that you think I should know about before I spend time overhauling parts of my codebase? (Keep in mind this codebase is a "for fun" project and a bragging point on my resume, not something where I'm wasting anyone's money with useless refactors.)
So I just wrote a simple little test app and I'm really happy with it (compared to not having bitfields). I'm looking to use this in peripheral drivers where I have to set and clear various bits of an 8-bit value before writing to a register. My first test will be an ADXL345 driver, and the sample below is inspired by the BW_RATE register (0x2C).
struct MyByte {
unsigned int rateCode: 3;
bool lowerPowerMode:1;
};
union Foo {
MyByte fields;
uint8_t raw;
};
MyByte myByte = {
rateCode: 0x5,
lowerPowerMode: true
};
pwOut << "Size = " << sizeof(MyByte) << "\n";
pwOut.printf("Value: 0b%08b\n", Foo({fields: myByte}).raw);
myByte.lowerPowerMode = static_cast<bool>(CNT % 2);
pwOut.printf("Value: 0b%08b\n", Foo({fields: myByte}).raw);
The resulting output is:
Size = 1 Value: 0b00001101 Value: 0b00001101
I stuck the sizeof() in there because I wanted to see if it remained as compact (and accurate) as possible. Indeed, in the form above it prints a size of 1. If I print sizeof(Foo) I also get 1. If I change rateCode to be 9 bits instead of 3, I get sizeof(MyByte) = 2. Life seems dandy and it's working great!
So has anyone else run across this before? Do you have any observations about this syntax that you think I should know about before I spend time overhauling parts of my codebase? (Keep in mind this codebase is a "for fun" project and a bragging point on my resume, not something where I'm wasting anyone's money with useless refactors.)
Comments
The first time I used bitfields back in the early days was for mapping memory to known I/O data fields. Then you move the software to another computer.. which doesn't have the same endianity. Disaster. And so on and so forth. There's much more. The end result is that nobody use them, in practice. It's much more reliable and portable to use shifts and masks.
Heck, even the C types of int, char etc are a disaster if you are moving code from architecture to architecture.
However, I suspect if you are writing Propeller specific code you are not likely to be expecting that code to run elsewhere. So stucts, bitfields, whatever can be convenient.
1) Writing to the Propeller's CTRx and PHSx registers. Clearly, that code will never be portable even if the rest of the library is ported to another platform.
2) Writing to or reading from a local copy of a hardware peripheral chip's registers. For example, preparing an 8-bit value before writing it to the ADXL345's power control register. I would think that this would be portable to other target platforms, no matter that target platform's endianness.
I agree about not using it to mimic 6-bit signed numbers or the like. Or for instance, a using it to mimic a 12-bit signed number coming out of a 12-bit ADC.... but no... that doesn't sound worth it to me. I'm concentrating on manipulating the individual bits of hardware-specific registers. Any objections to that?
If portability is an issue than don't do it. Else why not?
Most ARM's can only access individual bits in RAM but not in FLASH with the use of bit-banding,
as it don't cover all memory as the alias-addresses eats up 1 fake byte=1bit access.
GPIO/register bit toggling most ARM's also have atomic bit access there too.
It can still read bits in flash, but due to ARM being load-store, takes like 4 instructions and is of course not atomic anymore.
As C don't allow you to specify how to access a memory location, like asm would let you,
union of all the bits in to single a word, is a good way to read if any bit is set.
typedef volatile union eventTag { unsigned int all; /* the union of 16 below bits */ struct { unsigned char door : 1; unsigned char alarm : 1; unsigned char rtc : 1; unsigned char alert : 1; unsigned char wlan : 1; unsigned char minute : 1; unsigned char signa : 1; unsigned char hour : 1; unsigned char day : 1; unsigned char adc10 : 1; unsigned char flash : 1; }; }eventunion;That's certainly the question I'm trying to ask
The portability is only a problem if you intend to transmit the structs between platforms or compilers - IE, saving/loading, or sending across a network. In those cases you'd need to perform the serialization to the transmission structure yourself and enforce endianness and bit order rules.
If the data structs are runtime only, or you only operate on one platform and compile with one compiler, go to town - they're useful. And to the folks telling you memory is free, they're right, but cache performance is anything but. Using 8 "bool" values instead of a 8 single bits, for example, means a data structure that will trigger a cache line miss as much as 8x more often. Not an issue on the Prop, but the "memory and cpu are basically free" thinking has gotten us to where we are today, with computers that are literally thousands of times as fast as my old Commodore Amiga, yet no more responsive.
union Foo { struct { unsigned int rateCode: 3; bool lowPowerMode:1; } fields; uint8_t raw; }; int main () { Foo foo = { fields: { rateCode: 5, lowPowerMode: 1 } }; pwOut << "sizeof(Foo): " << sizeof(Foo) << "\n"; pwOut << "sizeof(foo): " << sizeof(foo) << "\n"; pwOut << "sizeof(Foo::fields): " << sizeof(Foo::fields) << "\n"; pwOut << "sizeof(Foo::raw): " << sizeof(Foo::raw) << "\n"; pwOut << "sizeof(foo.fields): " << sizeof(foo.fields) << "\n"; pwOut << "sizeof(foo.raw): " << sizeof(foo.raw) << "\n"; pwOut.printf("foo.fields.rateCode: %d\n", foo.fields.rateCode); pwOut.printf("foo.fields.lowPowerMode: %d\n", foo.fields.lowPowerMode); pwOut.printf("foo.raw: 0b%08b\n", foo.raw); return 0; }Produces:
Exactly what one would hope for and expect. This makes it reasonable to drastically reduce the number of methods in the driver. Instead of having two methods for the RateAndPowerMode register (one to set rate, and another to set the power), I can have something that is 90% as easy but requires only a single method to update any of the registers. Here's what I'm looking at now:
class ADXL345 { public: /** * @brief Data rate codes for the `Register::RATE_AND_POWER_MODE` register. Enumeration names are based on * the data rate with underscores representing decimal points for 6.25 and 12.5 Hz. */ typedef enum { /** Output data rate (Hz) = 6.25, Bandwidth (Hz) = 3.125, Current (uA) = 145 */ DR_6_25 = 6, /** Output data rate (Hz) = 12.5, Bandwidth (Hz) = 6.25, Current (uA) = 100 */ DR_12_5, /** Output data rate (Hz) = 25, Bandwidth (Hz) = 12.5, Current (uA) = 145 */ DR_25, /** Output data rate (Hz) = 50, Bandwidth (Hz) = 25, Current (uA) = 145 */ DR_50, /** Output data rate (Hz) = 100, Bandwidth (Hz) = 50, Current (uA) = 145 */ DR_100, /** Output data rate (Hz) = 200, Bandwidth (Hz) = 100, Current (uA) = 145 */ DR_200, /** Output data rate (Hz) = 400, Bandwidth (Hz) = 200, Current (uA) = 100 */ DR_400, /** Output data rate (Hz) = 800, Bandwidth (Hz) = 400, Current (uA) = 65 */ DR_800, /** Output data rate (Hz) = 1600, Bandwidth (Hz) = 800, Current (uA) = 55 */ DR_1600, /** Output data rate (Hz) = 3200, Bandwidth (Hz) = 1600, Current (uA) = 40 */ DR_3200 } DataRate; /** * Bit-mapping for the rate and power mode register */ union RateAndPowerMode { struct { /** * Set the data polling rate. */ DataRate dataRate: 3; /** * Enable or disable low-power mode. Low power mode has a somewhat higher noise but reduces power * consumption. True to enable low power mode, false for normal operation. */ unsigned int lowPowerMode: 1; } fields; uint8_t raw; }; /** * @brief Perform a manual write to the device * * @param[in] address Address of the register that should be modified * @param[in] value Value that should be written to the register */ void write (const Register address, const uint8_t value) const { if (this->m_alwaysSetMode) this->m_bus->set_mode(SPI_MODE); this->m_cs.clear(); this->m_bus->shift_out(8, static_cast<uint32_t>(address)); this->m_bus->shift_out(8, value); this->m_cs.set(); } } int main () { const ADXL345 accelerometer(Port::P4); accelerometer.write(ADXL345::Register::RATE_AND_POWER_MODE, ADXL345::RateAndPowerMode{fields: { dataRate: ADXL345::DataRate::DR_50, lowPowerMode: 0 }}.raw); }It does put a little more responsibility on the user. For instance, if low power mode was enabled then a user might more easily and accidentally disable low power mode by simply setting the data rate and forgetting that the lowPowerMode field exists. Of course, there's nothing stopping me from providing both the bitfield union and convenience functions (ones that first read from the register, then update the corrects bits, then write to the register again)... but at least now it's not quite so important that I do so.
1) Would you use them here if you wrote your own driver?
2) Would you actively discourage someone else from using them in a driver?
Here's my final product:
Driver
Demo code
You can see the whole init code in the link above, but here's a snippet on what I landed with:
void initialize (const ADXL345 &accelerometer) { // SPI init stuff... ADXL345::RateAndPowerMode rateAndPowerMode; rateAndPowerMode.fields.dataRate = DATA_RATE; accelerometer.write(ADXL345::Register::RATE_AND_POWER_MODE, rateAndPowerMode.raw); ADXL345::DataFormat dataFormat; dataFormat.fields.range = RANGE; accelerometer.write(ADXL345::Register::DATA_FORMAT, dataFormat.raw); ADXL345::FIFOControl fifoControl; fifoControl.fields.fifoMode = ADXL345::FIFOMode::STREAM; accelerometer.write(ADXL345::Register::FIFO_CONTROL, fifoControl.raw); accelerometer.start(); }1) I would not use them to write my own drivers because the packing of bit fields is entirely implementation defined (see the "Notes" section of en.cppreference.com/w/cpp/language/bit_field) which could lead portability problems when working with multiple microcontrollers, and could lead to the code's behavior changing when building with a different compiler or a different version of the same compiler.
2) If someone asked I would recommend they not use them.
If you are going to continue using them for this driver, I would recommend declaring the fields as std::uint8_t since you are writing to 8-bit registers with the added benefit of reducing the size of the structs (though this could have a negative impact on performance).
struct __attribute__ ((__packed__)) MyByte { ... }Also, if your driver is intended for the Propeller only then there should be no issue since you will not be running into any endian-ness issues between architectures. However, if you plan for your driver to be portable to other platforms, then you may want to shy away from bit-fields all together.
Although, a few years back I worked on Linux Device Drivers for a company and bit-fields were strictly enforced. That is, if a struct was defined, then it had to implement bit-fields. I'm not aware of this causing any issues when the driver was ported from the x86_64 architecture to an ARM based processor platform.
There are pointer limitations with bit-fields that you may have to code around:
c0x.coding-guidelines.com/6.7.2.1.html]