Bitfield memory layout is implementation defined

[5frank]

In LoRaMac-node/src/radio/sx126x/sx126x.cand perhaps elsewhere, bit-fields are used to parse data coming from the chip, but memory layout of bit-fields is implementation defined so there is no guarantee that it will work as it can change with compiler and options. I suggest a change to bitmasks defines that will always work.

[mluis1]

That's right the SX126x driver as of today can only work on litle-endian processors. As all the currently supported platforms use a litle-endian processor we never noticed the issue.

If you are willing to, could you please propose a pull-request to fix this?

[5frank]

In this case I do not think the endianness matters that much as all bit-fields seem to be 8-bit or less. The problem is that the C standard allows a compiler to put bit-fields in any order and not necessarily the order they are declared. This is a problem as the bit order in data read from the SX126x chip is always the same and not up to the compiler... https://stackoverflow.com/questions/19376426/order-of-fields-when-using-a-bit-field-in-c

A more subjective critique against bit-fields is that you can not handle them as a "set" in the same way, example to check if any of the error bits are set assert(!(foo & FOO_ALL_ERROR_BITS_MASK)).

I can try to make a PR, but it would require a change in the API and other code might depend on it...

[mluis1]

Thanks for the link and for checking that all bit fields are not bigger than 8 bits.

We think then that it is preferable for the time being to not change the current implementation.

[5frank]

Ok. Consider the following:

typedef union RadioStatus_u
{
    uint8_t Value;
    struct
    {   //bit order is lsb -> msb
        uint8_t Reserved  : 1;  //!< Reserved
        uint8_t CmdStatus : 3;  //!< Command status
        uint8_t ChipMode  : 3;  //!< Chip mode
        uint8_t CpuBusy   : 1;  //!< Flag for CPU radio busy
    }Fields;
}RadioStatus_t;

How do you know if CpuBusy corresponds to bit 7 (MSB) in Value? Unless I'm missing something there is no C compiler that can guarantee it (C++ compilers might).

[mluis1]

As far as I'm aware of the C compilers use the processor endianess to code the bit fields.

What I don't know is if there is a definition that indicates what is the endianess of the used processor.

If the processor is big endian the bit field should be encoded as follows:

typedef union RadioStatus_u
{
    uint8_t Value;
    struct
    {   // bit order is MSB-> LSB
        uint8_t CpuBusy   : 1;  //!< Flag for CPU radio busy
        uint8_t ChipMode  : 3;  //!< Chip mode
        uint8_t CmdStatus : 3;  //!< Command status
        uint8_t Reserved  : 1;  //!< Reserved
    }Fields;
}RadioStatus_t;

and if the processor is litle endian the bit field should be encoded as follows:

typedef union RadioStatus_u
{
    uint8_t Value;
    struct
    {   // bit order is LSB -> MSB
        uint8_t Reserved  : 1;  //!< Reserved
        uint8_t CmdStatus : 3;  //!< Command status
        uint8_t ChipMode  : 3;  //!< Chip mode
        uint8_t CpuBusy   : 1;  //!< Flag for CPU radio busy
    }Fields;
}RadioStatus_t;

If the compiler provides a define saying what is the processor endianess then we could define the bit field as follows:

typedef union RadioStatus_u
{
    uint8_t Value;
    struct
    {
#if ( PROCESSOR_ENDIANESS == PROCESSOR_BIG_ENDIAN )
        // bit order is MSB-> LSB
        uint8_t CpuBusy   : 1;  //!< Flag for CPU radio busy
        uint8_t ChipMode  : 3;  //!< Chip mode
        uint8_t CmdStatus : 3;  //!< Command status
        uint8_t Reserved  : 1;  //!< Reserved
#else
        // bit order is LSB -> MSB
        uint8_t Reserved  : 1;  //!< Reserved
        uint8_t CmdStatus : 3;  //!< Command status
        uint8_t ChipMode  : 3;  //!< Chip mode
        uint8_t CpuBusy   : 1;  //!< Flag for CPU radio busy
#endif
    }Fields;
}RadioStatus_t;

[kjendal]

In general, the encoding of bits in bitfields is not only processor, but compiler dependent. The C/C++ standards are silent on how this is to be accomplished (or even if they need to be packed). A much more supportable strategy is to use MACRO definitions to pack by hand. This is never less efficient (computationally) than what the compiler would do and is guaranteed to have the result you desire. Further portable code never assumes endianness, this needs to be handled via macros or functions which have the local platform knowledge. Historically, on xNIX systems hton and ntoh functions were used when converting from h(ardware) to n(etwork) and vis-versa. Note that outside of LoRaWAN and Zigbee, network-byte-order is always (or almost) Big-Endian.

[5frank]

If I understand it correctly this order do not depend on endianness at all and is implementation defined. https://hackaday.com/2015/08/28/firmware-factory-bit-fields-vs-shift-and-mask/ This is probably also why you hardly ever see bit-fields used for register access, they can not be trusted.

[mluis1]

Thanks Dave for the additional info.

I'll remove the bit fields usage from the SX126x radio driver in order to avoid future issues.

[mluis1]

Could you please check the provided fix?

[temporaryaccount]

I'd suggest a slightly different syntax.

For example, instead of:

error.Fields.PllLock    = ( err[1] & 0x40 ) >> 6;

Consider this:

error.Fields.PllLock    = ( err[1] & ( 1 << 6 ) ) ? 1 : 0;

Pros:

• There are no magic numbers: ( 1 << 6 ) is more clear to know that it's the 6th bit, unlike 0x40 where you are required to do some math in your head.

• There is one operation instead of two

Cons:

• There is a condition

[mluis1]

Thanks for the feedback.

In order to supress the ternary condition we could instead do the following:

error.Fields.PllLock    = ( err[1] & ( 1 << 6 ) ) >> 6;

[mluis1]

I have just committed the fix. But, unfortunately I referenced the wrong issue. #687 instead of #697

[temporaryaccount]

That's great!

For consistency we could've written 0x01 as (1 << 0) too :-)

[5frank]

There are still bitfields in the API.

Consider the following typical case: you want to check if any error bits are set and then print this to some log.

with bitfields it would look somehing like this:

RadioError_t re;
...  // read RadioError from chip

if ( re.Rc64kCalib &&
        re.Rc13mCalib &&
        re.PllCalib &&
        re.AdcCalib &&
        re.ImgCalib &&
        re.XoscStart &&
        re.PllLock &&
        re.PaRamp ) {

// alternativ pack them back in correct order
    printf("RadioError: ");
    prinftf("Rc64kCalib %d, " re.Rc64kCalib);
    prinftf("Rc13mCalib  %d, " re.Rc13mCalib);
    prinftf("PllCalib  %d, " re.PllCalib);
    prinftf("AdcCalib %d, "  re.AdcCalib);
    prinftf("ImgCalib  %d, " re.ImgCalib);
    prinftf("XoscStart  %d, " re.XoscStart);
    prinftf("PllLock  %d, " re.PllLock);
    prinftf("PaRamp: r%d\n", re.PaRamp);
}

... and without bitfields something like this:

uint16_t re;
...  // read RadioError from chip
if (re) {
    printf("RadioError: 0x%\n" re); // bits are documented in data sheet, yay!
}

Please explain how bitfields add any value here (unless you are paid per line of code of course)? The defacto standard and portable way to do this is with preprocessor defines for all bits in all register. Both the sx1272 and the sx1276 drivers follows this pattern, why not the sx126x?

[mluis1]

Could you please show which API you are referring too?

The usage of bit fields avoids one to compute the different bit masks. So, your example would look like exactly the same.

As per your example if we used bit masks then

#define RC_64K_CALIB                    ( 1 << 0 )
#define RC13M_CALIB                     ( 1 << 1 )
#define PLL_CALIB                       ( 1 << 2 )
#define ADC_CALIB                       ( 1 << 3 )
#define IMG_CALIB                       ( 1 << 4 )
#define XOSC_START                      ( 1 << 5 )
#define PLL_LOCK                        ( 1 << 6 )
#define PA_RAMP                         ( 1 << 0 )

...

uint8_t re[2];
...  // read RadioError from chip

if ( re[1] & RC_64K_CALIB &&
     re[1] & RC13M_CALIB &&
     re[1] & PLL_CALIB &&
     re[1] & ADC_CALIB &&
     re[1] & IMG_CALIB &&
     re[1] & XOSC_START &&
     re[1] & PLL_LOCK &&
     re[0] & PA_RAMP ) {

Concerning the printf example. I guess that you have noticed that the bit fields are part of a union. Thus, your code could be written as follows (which is not too much different from your example)

typedef union
{
    struct
    {
        uint8_t Rc64kCalib              : 1;                    //!< RC 64kHz oscillator calibration failed
        uint8_t Rc13mCalib              : 1;                    //!< RC 13MHz oscillator calibration failed
        uint8_t PllCalib                : 1;                    //!< PLL calibration failed
        uint8_t AdcCalib                : 1;                    //!< ADC calibration failed
        uint8_t ImgCalib                : 1;                    //!< Image calibration failed
        uint8_t XoscStart               : 1;                    //!< XOSC oscillator failed to start
        uint8_t PllLock                 : 1;                    //!< PLL lock failed
        uint8_t                         : 1;                    //!< Buck converter failed to start
        uint8_t PaRamp                  : 1;                    //!< PA ramp failed
        uint8_t                         : 7;                    //!< Reserved
    }Fields;
    uint16_t Value;
}RadioError_t;

RadioError_t re;
...  // read RadioError from chip
if (re.Value) {
    printf("RadioError: 0x%04X\n" re.Value); // bits are documented in data sheet, yay!
}

[5frank]

I was referring to headers such as src/radio/sx1272/sx1272Regs-LoRa.h where bitmasks are defined (instead of bitfields). These are then used in src/radio/sx1272/sx1272.c.

Sorry, my example was incorrect (meant || instead of &&) and I also assumed reserved bits to be zero which might not be the case. Anyways, regarding the union and your example printf("RadioError: 0x%04X\n" re.Value); - this is exactly the problem. The printed hex value doesn't say anything as it only represents the the bitfield in the union (which memory layout is implementation defined, as discussed earlier).

[5frank]

Perhaps I should add, that if the only metric of code quality is whatever it works or not, we can end this discussion and close the issue. My reasoning was that bitfields could be removed as a related change was needed anyways, but now a fix is already there...