I2C API is mixing two incompatible definitions of bit-fields

[zephyrbot]

Reported by Piotr Mienkowski:

Zephyr's I2C API contains the following construct

union dev_config {
    u32_t raw;
    struct __bits {
        u32_t        use_10_bit_addr : 1;
        u32_t        speed : 3;
        u32_t        is_master_device : 1;
        u32_t        reserved : 26;
    } bits;
};

This is incorrect. C99 §6.7.2.1, paragraph 10 says: "The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined.". I.e. - using union dev_config as an example - compiler is free to map use_10_bit_addr either to MSB or to LSB. The two methods of specifying bit fields are not equivalent and should not be mixed.

(Imported from Jira ZEP-2579)

[zephyrbot]

by Andy Ross:

Agreed, that's sort of a mess. Even beyond the portability issues, providing an un-typechecked variant to a typesafe API is kinda crazy. Any idea how much existing code uses the "raw" field? Guess I could run sanitycheck to find out...

[zephyrbot]

by Piotr Mienkowski:

Actually, looking at the state of current Zephyr API we tend not to use C bit-fields but rather #defines to implement bit-fields. The only other driver using C bit-fields in its API is DMA.

One difference between using C style bit-fields and #defines is that with C bit-fields if we are assigning to an automatic variable we have to explicitly assign values of all bit-fields. With #define only those bit-fields that we want to change from its default behavior (assuming value 0 is the default). Defining configurations with many optional flags is much more handy with #defines.

Another advantage of using #defines is much more compact assembly code. Here is an example:

union dev_config cfg;
cfg.raw = I2C_SPEED_STANDARD | I2C_MODE_MASTER;
{code}
{code}
movs    r1, <span>#</span>17
{code}
vs.
{code}
cfg.bits.use_10_bit_addr = 0;
cfg.bits.speed = I2C_SPEED_STANDARD;
cfg.bits.is_master_device = 1;
{code}
{code}
movs    r1, <span>#</span>0
bfc     r1, <span>#</span>0, <span>#</span>1
movs    r5, <span>#</span>1
bfi     r1, r5, <span>#</span>1, <span>#</span>3
orr.w   r1, r1, <span>#</span>16

But then of course with #defines there is no type checking. And setting and getting a value of e.g. 4 bit wide bit-field is a challenge of its own. In fact the above assignment to cfg.raw contains a bug.

[zephyrbot]

by Andy Ross:

Nah, it's true it's not a common paradigm in OS code, but the assembly example is specious. Those two examples do different things: one writes the whole word assuming everything not specified is zero, the other carefully changes only the bits requested. You can write an assignment for a whole word with C bitfields using a structure literal and it generates the same code that the integer assignment would.

It's not like I think preprocessor bitfields should be disallowed, but if we're going to take an API that has both, we should choose to keep the typesafe one. :)

[zephyrbot]

by Piotr Mienkowski:

You can write an assignment for a whole word with C bitfields using a structure literal and it generates the same code that the integer assignment would.

Good point. I should have used an initializer list

union dev_config cfg = {
	.bits = {
		.use_10_bit_addr = 0,
		.speed = I2C_SPEED_STANDARD,
		.is_master_device = 1,
	},
};

But the resulting assembly code is the same.

movs    r5, <span>#</span>1
movs    r1, <span>#</span>0
bfi     r1, r5, <span>#</span>1, <span>#</span>3
orr.w   r1, r1, <span>#</span>16

I'm using Zephyr SDK 0.9.2-rc2 and CONFIG_DEBUG is not set, so all the optimizations are on.

Also an initializer list can only be used at the place the variable is defined. That's a bit limiting.

[galak]

I think we can close this with the merger of PR #4254