Just discovered C's "bitfields"! How cool!

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).
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.)
David
PropWare: C++ HAL (Hardware Abstraction Layer) for PropGCC; Robust build system using CMake; Integrated Simple Library, libpropeller, and libPropelleruino (Arduino port); Instructions for Eclipse and JetBrain's CLion; Example projects; Doxygen documentation
CI Server: https://ci.zemon.name?guest=1

Comments

  • TorTor Posts: 1,980
    edited 2017-07-26 - 06:20:19
    Ah, it happens to everyone.. yes, we all used them early on, but everbody I know soon began to shun them like the plague. There are a number of problems with them which makes them not worth it. To start with, unless you remember to declare them all as 'unsigned' as a general rule, you'll run into signed overflow issues where you didn't want it (the only use I can find for that is in certain cases where you actually want to emulate e.g. a 6-bit signed int - something I could use in my minicomp emulator, except that it's too trivial to bother using a bitfield).
    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.
  • In general C structs are a disaster for moving your programs between machines or transmitting data between machines. Often due to endian issues as noted above.

    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.

  • Is endianness really a problem for this use case? I can only think of two places that I'd use this at the moment:

    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?
    David
    PropWare: C++ HAL (Hardware Abstraction Layer) for PropGCC; Robust build system using CMake; Integrated Simple Library, libpropeller, and libPropelleruino (Arduino port); Instructions for Eclipse and JetBrain's CLion; Example projects; Doxygen documentation
    CI Server: https://ci.zemon.name?guest=1
  • I think it's fine.

    If portability is an issue than don't do it. Else why not?
  • tonyp12tonyp12 Posts: 1,947
    edited 2017-07-26 - 18:34:55
    As MCU's are getting more ram every month, being frugal with bits is not that important anymore.
    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;
    
  • Heater. wrote: »
    If portability is an issue than don't do it. Else why not?

    That's certainly the question I'm trying to ask :). So far I haven't heard any compelling reasons not to (for these very specific use cases), but we'll see if anyone else comes up with anything.
    David
    PropWare: C++ HAL (Hardware Abstraction Layer) for PropGCC; Robust build system using CMake; Integrated Simple Library, libpropeller, and libPropelleruino (Arduino port); Instructions for Eclipse and JetBrain's CLion; Example projects; Doxygen documentation
    CI Server: https://ci.zemon.name?guest=1
  • Different compilers are free to pack the bits differently. So, each compiler is free to begin allocating bits from top-down or bottom-up within the value. In addition, endianness of the target hardware affects the layout of everything larger than a single byte, so these are truly not portable.

    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.
  • DavidZemonDavidZemon Posts: 2,791
    edited 2017-07-27 - 01:07:30
    Ah, found a much better syntax. I really wasn't a fan of the fact that I'd have to declare both a struct and a union for each register... but this is much better:
    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:
    sizeof(Foo): 1
    sizeof(foo): 1
    sizeof(Foo::fields): 1
    sizeof(Foo::raw): 1
    sizeof(foo.fields): 1
    sizeof(foo.raw): 1
    foo.fields.rateCode:     5
    foo.fields.lowPowerMode: 1
    foo.raw:                 0b00001101
    

    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.
    David
    PropWare: C++ HAL (Hardware Abstraction Layer) for PropGCC; Robust build system using CMake; Integrated Simple Library, libpropeller, and libPropelleruino (Arduino port); Instructions for Eclipse and JetBrain's CLion; Example projects; Doxygen documentation
    CI Server: https://ci.zemon.name?guest=1
  • DavidZemonDavidZemon Posts: 2,791
    edited 2017-07-29 - 19:28:00
    Particularly the critics of bitfields, I'm curious what your thoughts are on my final draft of an ADXL345 driver. Two questions for you:

    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();
    }
    
    David
    PropWare: C++ HAL (Hardware Abstraction Layer) for PropGCC; Robust build system using CMake; Integrated Simple Library, libpropeller, and libPropelleruino (Arduino port); Instructions for Eclipse and JetBrain's CLion; Example projects; Doxygen documentation
    CI Server: https://ci.zemon.name?guest=1
  • DavidZemon wrote: »
    Particularly the critics of bitfields, I'm curious what your thoughts are on my final draft of an ADXL345 driver. Two questions for you:

    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).
  • The fact that the fields are not always packed and that the order in which they are packed (when they are packed) also varies by implementation did not sink in until just now. That's a real bummer. I do have unit tests in PropWare though... I could alleviate (or at least identify) packing issues if I ever port the code by writing some simple assertions in this driver's test suite.
    David
    PropWare: C++ HAL (Hardware Abstraction Layer) for PropGCC; Robust build system using CMake; Integrated Simple Library, libpropeller, and libPropelleruino (Arduino port); Instructions for Eclipse and JetBrain's CLion; Example projects; Doxygen documentation
    CI Server: https://ci.zemon.name?guest=1
  • In some instances you could try to force Packing as follows:
    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]
    1417 104) The unary & (address-of) operator cannot be applied to a bit-field object;

    1418 thus, there are no pointers to or arrays of bit-field objects.
Sign In or Register to comment.