I have a somewhat complex structure in my C code, and driver functions to read or write data with it.
The simpler parts of the struct (such as ioflags2 below) and their driver functions work just fine. However, the more complex parts of the struct (such as ioflags3) do not.
The symptom is that only the drivers for the first part of the more complex parts of the structure work right, and the rest don't appear to change the "word" part of the structure.
However, if I step through the code with the simulator, all of the individual parts of the more complex parts of the struct work just fine with the driver functions, but the "word" part doesn't change (well, except for the 1st part of the more complex parts, i.e., module1 works, but module2 - module5 don't).
Finally, I cut/paste the whole struct and drivers to a Visual C++ program and stepped through the code with their debugger. *ALL* of the driver functions had the expected effect on *ALL* of the parts of the whole struct.
Can anyone tell me what, if anything, is wrong with this code, and how to make it work with KEIL's ARM tools?
union{ u16 word; struct{ unsigned sensor1:1; unsigned sensor2:1; unsigned sensor3:1; unsigned sensor4:1; unsigned sensor5:1; unsigned sensor6:1; unsigned sensor7:1; unsigned sensor8:1; unsigned sensor9:1; unsigned sensor10:1; unsigned reserved_bit_10:1; unsigned reserved_bit_11:1; unsigned reserved_bit_12:1; unsigned reserved_bit_13:1; unsigned reserved_bit_14:1; unsigned reserved_bit_15:1; }bit; }ioflags2; union{ u16 word; struct{ struct{ struct{ unsigned run:1; unsigned direction:1; }stepper_motor; }module1; union{ struct{ unsigned extend:1; unsigned retract:1; }ioctl; struct{ unsigned run:1; unsigned direction:1; }dc_motor; }module2; union{ struct{ unsigned open:1; unsigned close:1; }ioctl; struct{ unsigned run:1; unsigned direction:1; }dc_motor; }module3; union{ struct{ unsigned open:1; unsigned close:1; }ioctl; struct{ unsigned run:1; unsigned direction:1; }dc_motor; }module4; union{ struct{ unsigned reserved_bit_0:1; unsigned fan:1; }ioctl; struct{ unsigned direction:1; unsigned run:1; }dc_motor; }module5; unsigned reserved_bit_10:1; unsigned reserved_bit_11:1; unsigned reserved_bit_12:1; unsigned reserved_bit_13:1; unsigned reserved_bit_14:1; unsigned reserved_bit_15:1; }bit; }ioflags3;
And here's how my drivers manipulate the structure.
int set_module2_retract(int state){ int previous_state; ASSERT(module2_type() == TYPE_IOCTL); if(module2_type() != TYPE_IOCTL){ return(-1); } if(state){ set_module1_extend(0); // protect the hardware! } previous_state = variable.ioflags3.bit.module2.ioctl.retract; variable.ioflags3.bit.module2.ioctl.retract = state ? 1u : 0u; return(previous_state); }
You are making expectations that are not always true.
Note that the bit fields in C have a number of details that are left to the implementation.
You have a struct that contains multiple unions that each contain bitfields. Don't expect the compiler to concatenate all this into a single continuous bit field container.
gcc or M$ Visual C++ that generates 32-bit code will think that sizeof(ioflags2) is 4 bytes (because the bitfields are unsigned). It will think the ioflags3 combo is 24 bytes. Replacing the unsigned bit fields with u16 would shrink ioflags2 to 2 bytes and ioflags3 to 12 bytes.
Look closer at ioflags3.
It is a union of a u16 and a struct. The u16 is 2 bytes large. But how large is the struct? The struct has the following elements: module1 - maybe 1, maybe 2, maybe 4, ... bytes module2 - maybe 1, maybe 2, ... module3 - maybe 1, maybe 2, ... module4 - maybe 1, maybe 2, ... module5 - maybe 1, maybe 2, ...
For 32-bit gcc, each of module1..module5 will be four bytes large if the bit fields in them are declared unsigned, since unsigned is a 32-bit integer.
For 32-bit gcc, each of module1..module5 will be two bytes large if the bit fields in them are declared u16.
But the bit fields in module1 will not share the same unsigned or u16 as the bitfields in module2 or module3, even if the container is large enough to store the 10 bits.
Never expect to get portable code when using bit fields. If you do use them - verify with sizeof() that you get the expected size. And verify that each field will affect exactly the bits you think they will affect.
I would implement your code using traditional bit operations.
enum { MODULE1_RUN = 0, MODULE1_DIRECTION = 1, MODULE2_EXTEND = 2, MODULE2_RETRACT = 3, MODULE2_RUN = 2, MODULE2_DIRECTION = 3, MODULE3_OPEN = 4, MODULE3_CLOSE = 5, MODULE3_RUN = 4, MODULE3_DIRECTION = 5, MODULE4_OPEN = 6, MODULE4_CLOSE = 7, MODULE4_RUN = 6, MODULE4_DIRECTION = 7, MODULE5_FAN = 9, MODULE5_DIRECTION = 8, MODULE5_RUN = 9, }; int set_module2_retract(int state) { int previous_state; if (module2_type() != IOCTL) { return -1; } if (state) { set_module1_extend(0); } previos_state = (variable.ioflags3 & (1u << MODULE2_RETRACT)) != 0; if (state) { variable.ioflags3 |= 1u << MODULE2_RETRACT; } else { variable.ioflags3 &= ~(1u << MODULE2_RETRACT); } return previous_state; }
Hi Per
Thanks for the detailed analysis.
Some things to be clarified here:
> Note that the bit fields in C have a number of details that are left > to the implementation.
I suppose "implementation" means processor rather than compiler implementation, as in "ARM may behave differently from x86", which could be critical indeed in a SW simulation of your code.
As far as only ARM is concerned, the exact bit field behavior (modulo unintended omissions) is defined in the ARM Architecture Procedure Call Standard (AAPCS), which is available as PDF. ARM ABI compliant compilers must implement these rules.
> I would implement your code using traditional bit operations.
Setting up bit fields in C can be a royal pain at first, but it often pays off when you use them a lot in code.
In this case of single bit values, I might agree with you. If you have to deal with multi-bit values though, the necessary bit masking decreases readability a lot. And as DEK says (approximately from my memory) "code gets written once, but will be read hundreds of times".
My apologies for mentioning "intellisense"-like features as another argument for bit fields...
Best regards Marcus http://www.doulos.com/arm/
"I suppose 'implementation' means processor rather than compiler implementation, as in 'ARM may behave differently from x86', which could be critical indeed in a SW simulation of your code."
The C standard leaves a lot of these things open to the individual compiler implementation. In some situations, an OS vendor may then define an ABI that may lock down some of these open issues for compilers supporting that OS. In the case of the ARM processor, ARM has decided to define an ABI since the value if their core is greatly affected by the portability of code between different ARM processors, and since ARM is developing their own compilers.
But most OS ABI do not bother to define bit fields for the simple reasons that most OS do not use bit fields. They instead define their API with manual masking of bits, just because bit fields are so often avoided.
Anyway - if you use your ARM compiler and check the sizes, you will notice that ioflags2 is 4 bytes and ioflags3 is 24 bytes. And changing all bit fields to u16 will shrink the sizes to 2 and 12 bytes. Your ARM ABI will not combine the multiple bit fields since they are not neighbours.
All very interesting and illuminating responses. I did abandon the bit field concept for my struct, and no longer have the problem.
I've always used the manual masking of bit fields in the past, but was looking for a cleaner (i.e., more readable) method; combined with a way to have a single variable that could be written to or read from non-volatile memory as a single block of bytes; that also enforced that *all* of the configuration/setup/operational settings would have to be in that single block of bytes. Accessing them via the "dot" operator from the single struct seemed like a logical way to do all of that.
Thanks for all your help.
Dave.
View all questions in Keil forum