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

Comments

• Tor Posts: 2,010

  2017-07-26 06:19 edited 2017-07-26 06:20

  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.
  
• Heater. Posts: 21,230

  2017-07-26 06:34

  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.
  
  
• DavidZemon Posts: 2,973

  2017-07-26 13:58

  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?
• Heater. Posts: 21,230

  2017-07-26 16:48

  I think it's fine.
  
  If portability is an issue than don't do it. Else why not?
• tonyp12 Posts: 1,950

  2017-07-26 17:28 edited 2017-07-26 18:34

  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;
  
• DavidZemon Posts: 2,973

  2017-07-26 17:52

  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.
• JasonDorie Posts: 1,930

  2017-07-26 18:58

  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.
• DavidZemon Posts: 2,973

  2017-07-27 00:52 edited 2017-07-27 01:07

  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.
• DavidZemon Posts: 2,973

  2017-07-29 19:26 edited 2017-07-29 19:28

  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();
  }
  
• apcountryman Posts: 33

  2017-07-30 00:05

  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).
• DavidZemon Posts: 2,973

  2017-07-30 02:27

  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.
• JonM Posts: 318

  2017-07-30 07:08

  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.