We are running a survey to help us improve the experience for all of our members. If you see the survey appear, please take the time to tell us about your experience if you can.
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.
> 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.
Did I imply anywhere that your analysis was wrong? It was in fact dead on.
> Your ARM ABI will not combine the multiple bit fields since they are > not neighbours.
With "multi-bit values" I didn't mean multiple independent bit field structures or containers. Coming up with a readable, concise implementation of a complex (in terms of number of bit-fields and variety of sizes) data structure for e.g. a v6/v7 page table with just CPP macros can turn out to be quite a challenge. Why not leave choosing the proper bit operation to the compiler.
d[i] &= ~(ARMV6_L1SECDESC_AP_MASK | ARMV6_L1SECDESC_APX_MASK) d[i] |= ARMV6_L1SECDESC_APX_RWNA; // similar style for other bits
typedef struct { unsigned int type : 2; unsigned int b : 1; unsigned int c : 1; unsigned int xn : 1; unsigned int domain : 4; unsigned int p : 1; unsigned int ap : 2; unsigned int tex : 3; unsigned int apx : 1; unsigned int s : 1; unsigned int ng : 1; unsigned int subtype : 1; unsigned int ns : 1; unsigned int secbase : 12; } l1_section_t; d[i].l1_section.ap = AP_RWNA; d[i].l1_section.apx = APX_RWNA; // similar style for other bits
Isn't it just much more elegant using a bit field for this? I guess we tend to prefer what we are used to.
No, I just thought that since you did bring up the ARM ABI, I would mention that the ARM ABI produces the same result as gcc does for a 32-bit x86 processor.
Yes, the source code will look cleaner with bit fields. But with some compilers, the compiler will produce worse code than if you manually mask the bits.
One funny thing with the ARM compiler in the MDK is that if you build without optimization and assign the value 1 to a single-bit field, the compiler will first add code to clear that bit to a known state, before setting the bit. With optimization on, this redundant clear will be removed. Even for 1-bit fields, the compiler goes the full and/or route.
There are compilers that will fail to optimize away the "and" or the "or" when your assign clears or sets all bits of the bit field.
A big problem with bit fields in relation to volatile memory addresses such as SFR, is that they tend to fool the developer into believing that you are reading or writing a variable named <container>.<field>, when the processor in reality will read and write a variable named <container>. It is very easy to miss that since a full-register access is made, you may trig unexpected features, such as acknowledging the reception of a byte on the UART, or acknowledging an interrupt, or starting a new A/D conversion.
With code looking like:
register = (register & ~FIELD_MASK) | (field_state << FIELD_SHIFT);
every reader will clearly see that there was one read and one write of the full register. By not hiding the ugly access, it will be easier for a reader to correlate the code side effects with the datasheet information.
> But with some compilers, the compiler will produce worse code than if > you manually mask the bits.
True, but compilers have improved over the years and every once in a while we should revisit our findings from the past.
> One funny thing with the ARM compiler in the MDK is that if you build > without optimization and assign the value 1 to a single-bit field, the > compiler will first add code to clear that bit to a known state, > before setting the bit. With optimization on, this redundant clear > will be removed. Even for 1-bit fields, the compiler goes the full > and/or route.
Sorry, I can't reproduce this with MDK or RVDS.
> There are compilers that will fail to optimize away the "and" or the > "or" when your assign clears or sets all bits of the bit field.
Please check out C99 compound literals. If that is not good enough, I define registers as unions of an (anonymous) bit field structure and a uint32_t. To initialize the entire value, I can write to the integer element.
> A big problem with bit fields in relation to volatile memory addresses > such as SFR, is that they tend to fool the developer into believing > that you are reading or writing a variable named <container>.<field>, > when the processor in reality will read and write a variable named > <container>.
The proper behavior of volatile complex bit field structures is rather difficult to comprehend, indeed. But please ask yourself whether you have ever been in a situation where this was an issue.
Either you have a (volatile) HW register set which consists of only one container per register (else you should rethink the structure definition), or you have a complex data structure (perhaps a packet header) which you normally wouldn't want to be volatile anyway.
> It is very easy to miss that since a full-register access is made, > you may trig unexpected features, such as acknowledging the > reception of a byte on the UART, or acknowledging an interrupt, or > starting a new A/D conversion.
Sorry, but this is a made up example. Possible in theory, but in real life HW registers just won't consist of multiple containers.
'Please check out C99 compound literals.'
you lost Per right there.
:)
Ashley - thank you for yet one more very meaningful post that adds important information to a debate.
Keep the good work up.
"Sorry, but this is a made up example. Possible in theory, but in real life HW registers just won't consist of multiple containers."
There is a saying that if sometihng seems to be too good to be true, it usually is.
If you find it very unlikely that a hardware has a HW register consisting of multiple containers, why are you then convinced that I was discussing such a "made up example"?
I was talking about transparency.
I was thinking about that junior developer who gets the support of the software a couple of years from now. He sees assigns to <container>.<field> and decides to make some modification, adding one more such assign. But what isn't visible when reading the *.c files, is that the assign to the field did not just write to the named field, but did perform one read and one write of the full container.
And quite a number of processors have SFR where a read or a write has some form of side effect. And that side effect is not always immediately visible, so an introduced bug can survive a long time before people start to notice the result of that extra side effect.
One processor I worked with in a project had a status register where a read reported all error cases seen since the previous read. No need to write to any register to acknowledge the error flags - just reading them was enough to also clear them.
If mapping such a regsiter to bit fields, the expression
if (com1.status.rx_overrun) ...
would automagically also clear any reported framing, parity, ... errors that may have been detected.
void set_bit(void) { FIO0PIN_BITS.b2 = 1; }
When compiled for a Cortex-M3 processor (Thumb2 instruction set) with -O0, the following code is generated (b2 is a one-bit field):
120: void set_bit(void) { 0x000001CA 4770 BX lr 121: FIO0PIN_BITS.b2 = 1; 0x000001CC 480D LDR r0,[pc,#52] ; @0x00000204 0x000001CE 6940 LDR r0,[r0,#0x14] 0x000001D0 F0200004 BIC r0,r0,#0x04 0x000001D4 1D00 ADDS r0,r0,#4 0x000001D6 490B LDR r1,[pc,#44] ; @0x00000204 0x000001D8 6148 STR r0,[r1,#0x14] 122: }
When compiled with -O1, the problem is solved:
120: void set_bit(void) { 0x000001C4 4770 BX lr 121: FIO0PIN_BITS.b2 = 1; 0x000001C6 4809 LDR r0,[pc,#36] ; @0x000001EC 0x000001C8 6941 LDR r1,[r0,#0x14] 0x000001CA F0410104 ORR r1,r1,#0x04 0x000001CE 6141 STR r1,[r0,#0x14] 122: }
Switching from a Cortex-M3 processor to a processor with full 32-bit ARM instructions, the compiler manages without the clear even ast -O0:
124: void set_bit(void) { 0x00000130 E12FFF1E BX R14 125: FIO0PIN_BITS.b2 = 1; 0x00000134 E59F0034 LDR R0,[PC,#0x0034] 0x00000138 E5901014 LDR R1,[R0,#0x0014] 0x0000013C E3811004 ORR R1,R1,#0x00000004 0x00000140 E5801014 STR R1,[R0,#0x0014] 126: }
Since I'm on vacation right now, I tested the above with the evaluation version of MDK-ARM 3.70.
"True, but compilers have improved over the years and every once in a while we should revisit our findings from the past."
Correct. But note that I did not write about a specific compiler. I did write "But with some compilers, the compiler will produce worse code than if you manually mask the bits." That should be read as: Do not assume that the compiler you have will produce good code for bit fields - check first!
Compound literals helps assign all field values in a single assign. Just note that you may find C++ compilers that will not support it. ARMCC and gnuarm does support it also for C++ sources.
But you seem to have brought up compound literals as a response to my sentence "when your assign clears or sets all bits of the bit field". I wasn't talking about the assign of all fields in a container, but a set or clear of all bits in a single field. When setting all bits, no mask operation is needed. When clearing all fields, no or is needed.
"I was talking about transparency."
Per, I think people will benefit a lot from your presence here if you stop talking.
ciao.
> I was thinking about that junior developer who gets the support of the > software a couple of years from now. He sees assigns to > <container>.<field> and decides to make some modification, adding one > more such assign. But what isn't visible when reading the *.c files, > is that the assign to the field did not just write to the named field, > but did perform one read and one write of the full container.
We were talking about different things then. Sorry. I was referring to the rules of access to volatile bit field structs with multiple, perhaps overlapping (yuck!) containers.
> If mapping such a regsiter to bit fields, the expression > > if (com1.status.rx_overrun) ... > > > would automagically also clear any reported framing, parity, > ... errors that may have been detected.
But since you carefully read the data sheet before starting to write code I am sure that you copied the entire register into a local variable before accessing its contents :^)
Seriously, I see the issue, but I fail to see in which way any other approach would be safer. Does something like
if (com1.status & COM_STATUX_RX_OVERRUN) ...
make a difference here?
> When compiled for a Cortex-M3 processor (Thumb2 instruction set) with > -O0, the following code is generated (b2 is a one-bit field): > [...] > > Switching from a Cortex-M3 processor to a processor with full 32-bit > ARM instructions, the compiler manages without the clear even ast > -O0:
With a CM3 target I can reproduce this. Strange indeed. Thanks for the additional info.
> I wasn't talking about the assign of all fields in a container, but > a set or clear of all bits in a single field. When setting all bits, > no mask operation is needed. When clearing all fields, no or is > needed.
Understand now what you meant but I don't seem to get it. If you modify all bits in the field, you still have to mask the other bits in the container, no? Could you give an example, please?
Thanks Marcus http://www.doulos.com/arm/
"If you modify all bits in the field, you still have to mask the other bits in the container, no? Could you give an example, please?"
Normally yes, but there are two special cases.
When I want to set all bits in one field to one, the compiler can directly do the "or" operation, without first masking any other fields.
When I want to clear all bits in one field to zero, the compiler don't need to perform any "or" operation - it is enough to just do the "and not" to keep everything but this field.
I have seen compilers that have missed this optimization for fields bigger than one bit. The ARM compiler managing to do a clear+set for a one-bit field was a first. But it was only with zero optimization so it isn't something I would see as a problem.
But as you have already noted, such optimization failures can be corrected in the next version of the compiler.
Another thing that can sometimes be funny with some compilers is if you use signed fields. I have seen compilers generating very interesting code then, because of the need of sign-extend when you assign. This is normally a problem with microcontrollers, since you would normally only map unsigned fields to SFR.
About the example I showed earlier. Yes, any competent programmer should read out the register value into a temporary variable before processing the bits. And I do expect people who writes drivers/ISR to have read the relevant parts of the datasheet. But that doesn't always happens when a software gets old and the responsibility moved to some new guy/gal just out from school and where just some very minor changes are expected.
The previous example whas a bit "odd" in that few processors perform auto-acknowledge for a interrupt or status register. Auto-acknowledge is normally reserved for the RX register of a communication port. But what is quite common is that you have an interrupt register where each bit flags an interrupt source, and you then write a one to that bit to acknowledge. An example is the timer interrupt register in a NXP LPC17xx (Cortex-M3) or LPC23xx (ARM7) chip.
It is so very easy to write code like the following:
void handle_timer0_irq() { if (t0ir.mr0) { // Handle match 0 t0ir.mr0 = 1; // Acknowledge MR0 (###) } if (t0ir.mr1) { // Handle match 1 t0ir.mr1 = 1; // Acknowledge MR1 (###) } //... if (t0ir.cr0) { // Handle capture channel 0 t0ir.cr0 = 1; // Acknowledge CR0 (###) } if (t0ir.cr1) { // Handle capture channel 1 t0ir.cr1 = 1; // Acknowledge CR0 (###) } }
The application may normally expect only one interrupt flag at a time, but it isn't impossible that you may have multiple timer events so close together that several is trigged before you enter the ISR, or that one more event happens while in the above code.
Each of the ### represents an acknowledge of one (1) timer event, but since we are talking about bit fields, the ARM ABI requires a read and a write of the full container for every field assign.
So when the developer thinks that he acknowledges one timer event, the ARM will read the T0IR register and potentially pick up a number of set fields that will be or:ed with the bit the programmer intended to acknowledge. The net result is a potential loss of some timer events.
If using the more visible alternative, using standard or/and code, the above would have looked like:
enum { TxIR_MR0 = 1u << 0, TxIR_MR1 = 1u << 1, TxIR_MR2 = 1u << 2, TxIR_MR3 = 1u << 3, TxIR_CR0 = 1u << 4, TxIR_CR1 = 1u << 5, ... }; void handle_timer0_irq() { if (t0ir & TxIR_MR0) { // Handle match 0 t0ir |= TxIR_MR0; // Safe acknowledge of MR0 } if (t0ir & TxIR_MR1) { // Handle match 1 t0ir |= TxIR_MR1; // Safe acknowledge of MR1 } //... if (t0ir & TxIR_CR0) { // Handle capture channel 0 t0ir |= TxIR_CR0; // Safe acknowledge of CR0 } if (t0ir.cr1) { // Handle capture channel 1 t0ir |= TxIR_CR1; // Safe acknowledge of CR1 } }
In the end, there is nothing wrong with bit fields. It is just that you always have to think about the hidden "bonus" you may get.
If you are using bit fields, you may find that you need to spend more time adding extra comments than what you save from the neater code.
Of course we have moved far off topic by now. The OP asked about a data structure and not HW registers.
> It is so very easy to write code like the following: > > [...] > > So when the developer thinks that he acknowledges one timer event, > the ARM will read the T0IR register and potentially pick up a number > of set fields that will be or:ed with the bit the programmer > intended to acknowledge. The net result is a potential loss of some > timer events.
Yes that is true.
> If using the more visible alternative, using standard or/and code, > the above would have looked like: > > [...]
Hmm, and in which way does the second example differ from the first one? Both ISR read the control register multiple times and may loose timer events. In fact I compiled both versions and with the exception of a single instruction the code is identical as expected. I fail to see the advantage of the second solution.
Please provide a full minimal example that demonstrates what you were thinking of.
And... getting back to the original question, could I have used the __packed attribute to make the original code work like I expected it to (i.e., to shrink the number of bytes required to implement the ioflags variables to the min size possible)?