Use of bit fields to manipulate registers.

[sglow]

I have recently been asked to review two pull requests in which bit field structures were being used to manipulate hardware register values. I have some technical concerns about this approach which are preventing me from approving those PRs. I'd like to use this issue as a place where this technique can be debated among the key players of the software team.

I'm attempting to tag all the main controller developers here. Please add anyone who I've missed. @jkff @jlebar @martukas @Miceuz @inceptionev @asmodai27

Background

Bit fields are a C/C++ language feature which allow members of a structure to be defined with a specific bit size. For example, you could have one structure member which is 3 bits in size and therefore only takes values from 0 to 7, and another which is only a single bit and therefore can only take values of 0 and 1.

For example:

struct bitfield{    
   int  foo : 2;  // This structure member will be two bits long
   int bar: 1;    // This member will be one bit long
}

Registers are memory mapped locations on in the processor address space that are used to control hardware modules such as UARTs, DMA, etc. Unlike normal memory locations, writing to or reading from registers has side effects. The values you write effect how the hardware works.

Very often a register will consist of a bunch of different settings of variable bit length. For example, the first UART control register (page 1238 of the reference manual) is a 32-bit register which includes 19 single bit settings, two settings that are each 5 bits long, and three reserved bits. To set up the UART you would write a value to that register which adjusts each bit as necessary depending on it's use.

Advantages of using bit-fields

Bit-fields would seem like an ideal way to manipulate register values because they allow you to give names to individual register values and set or clear them independently. The UART control register described above could be defined as a bit field structure with 19 single bit sized fields, two 5-bit sized fields, and one 3-bit reserved field. The resulting code to manipulate that register value is very easy to read and document.

The problems

• Bit fields by their nature require the compiler to generate read/modify/write sequences when manipulating the bit field. For example, when a bit-field that's only one bit long is set to 1 the compiler needs to read the memory unit (byte, short or int) containing the bit field, set just that bit in the value that was read, and write the updated value back to memory. This sort of read/modify/write operation is fine for normal memory access, but can be problematic for hardware registers because both reads and writes of registers can have side effects. For example, some registers provide status information in their bit contents. To clear an individual status bit you would need to write a 1 to that bit location. In this case, writing a 0 to that location has no effect. The way to clear such a status bit would be to write a value to the register with only that one bit location set. If a bit field is used, the compiler will read the entire register value and write back that same value with one additional bit set. This can cause other similar status bits in the register to be silently cleared due to the read/modify/write nature of the access. Not every register has side effects like this, most can be safely manipulated by bit-field accesses, but I'm concerned about standardizing on a method of accessing registers that can't be uniformly applied. The types of bugs that the incorrect use of bit-fields will cause can be very difficult to find.

• The C/C++ language does not give any guarantees as to how a bit field will be ordered in memory, that's implementation defined. Using bit-field structures to manipulate registers only works if the bits are ordered in a way that's consistent with how the register is laid out. If we, or anyone in the future using this code base, were to change compilers it's possible that the bit-fields would be ordered differently causing the code to fail.

• Many registers require a specific sized memory access to work properly. For example, it may only be possible to perform a full 32-bit read or write to a register value, reading or writing a single byte at a time could cause undefined results. The C++ language doesn't give any guarantees as to what type of memory access a bit-field manipulation will generate in the compiled code. gcc does have a command line option which seems to resolve this issue, but this is another non-standard convention that could prevent the use of this code base on any other compiler.

Discussion

I'd like to use this issue as a place to discuss the merits of using bit-fields to manipulate register values. I agree that it makes the code more readable, but am concerned that it could lead to some very difficult to find problems.

If the group collectively decides that this is the right approach, then I won't use it as a reason to reject a PR. I would however like to make sure we have a consensus on this subject before we start whole sale modifications to the HAL.

[sglow]

There's some additional discussion that I've already had with @jlebar and @Miceuz related to this issue on PR #322

[jlebar]

The compiler flag -fstrict-volatile-bitfields ensures this is safe. Here's what the GCC manual says.

accesses to volatile bit-fields [will] use a single access of the width of the field’s type, aligned to a natural alignment if possible. For example, targets with memory-mapped peripheral registers might require all such accesses to be 16 bits wide; with this flag you can declare all peripheral bit-fields as unsigned short (assuming short is 16 bits on these targets) to force GCC to use 16-bit accesses instead of, perhaps, a more efficient 32-bit access.

https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#Code-Gen-Options. It even talks about exactly our use-case.

You can also try it out yourself. The flag is on by default for whatever triple godbolt is using, but you can disable it with -fno-strict-volatile-bitfields and observe that the compiler generates what we don't want. https://gcc.godbolt.org/z/qnP6iM

It's true that clang does not seem to have this flag. It may be that clang does the right thing by default; I haven't yet been able to coerce godbolt into compiling for ARM. I don't mind locked in to gcc. If we have to change it later, we can.

[jlebar]

I'm concerned about standardizing on a method of accessing registers that can't be uniformly applied.

Can you give a specific example of such a register?

[jlebar]

The C/C++ language does not give any guarantees as to how a bit field will be ordered in memory, that's implementation defined.

"Implementation-defined" does not mean "undefined". The behavior of lots of things that we rely upon is implementation-defined. For example, iirc the behavior of static_cast is under some circumstances implementation-defined. This can be OK, if you know what your implementation is going to do.

[sglow]

Can you give a specific example of such a register?

One particularly nasty register that sticks in my mind is part of the USB module used on this chip. I know we aren't using that module and probably won't in the future, but this illustrates the types of problems we can get into.

The USB endpoint register (described on page 1544 of the reference manual) includes some bits that are rc_w0 which means you can read them and clear them by writing 0, writing 1 has no effect on this bit, so any time you write to it (regardless of what you read) you need to write 1 unless you want to clear it.

In the same register are some bits that are of type t (for toggle). Any time you write 1 to those bits you will toggle their value, writing 0 has no effect. There are also normal read/write bits in the same register.

I don't think there's any way we could use a bit-field based approach on a register like that. If one of the toggle type bits was a 1 (which it is about half the time), then any time we wrote any value to any other bit in the register we would cause it to toggle because of the read/modify/write nature of the bit.

[jlebar]

I see; the manual is pretty explicit about this footgun.

Read-modify-write cycles on these registers should be avoided because between the read and the write operations some bits could be set by the hardware and the next write would modify them before the CPU has the time to detect the change. For this purpose, all bits affected by this problem have an ‘invariant’ value that must be used whenever their modification is not required. It is recommended to modify these registers with a load instruction where all the bits, which can be modified only by the hardware, are written with their ‘invariant’ value.

https://www.st.com/resource/en/reference_manual/dm00151940-stm32l41xxx42xxx43xxx44xxx45xxx46xxx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf

Your worry is that we'll accidentally apply the bitfield technique to such a register and shoot ourselves in the foot that way?

[sglow]

Exactly. This register is a particularly egregious example which is why it came to mind, but it's not uncommon for registers to have status bits that need to be written with a 1 value to clear. You couldn't use a bit-field safely on such a register because any time you write to any other bit in the register you could accidentally clear the status bit without knowing it.

[jlebar]

it's not uncommon for registers to have status bits that need to be written with a 1 value to clear

And these registers where it's unsafe to read-modify-write, it's not as clear in the documentation?

[sglow]

I'm not really sure how explicit they are about pointing that out in the manual. I'm not surprised that they did it for this one since it was such a nasty register. That's why it stuck in my mind as well.

[jlebar]

I'm not really sure how explicit they are about pointing that out in the manual.

If they were explicit, would this allay your concern?

[sglow]

Looks like they aren't explicit in general. I just went looking at a couple of the peripherals that we do use and found a problem register in the A/D. The ADC interrupt and status register (ADC_ISR) (page 450). Each bit gives a status that's set by hardware and you write 1 to clear them. Using bit-fields on that register will lead to race conditions because setting any bit in the register will also clear any other one that's set. I don't see any warning about this on that register though.

[jlebar]

You're talking about e.g.

AWD3: Analog watchdog 3 flag This bit is set by hardware when the converted voltage crosses the values programmed in the fields LT3[7:0] and HT3[7:0] of ADC_TR3 register. It is cleared by software writing 1 to it.

?

[sglow]

It's true for all 10 bits in that register.

[jlebar]

Sure. They all say the same thing: "This bit is set by hardware ... it is cleared by software writing 1 to it".

[jlebar]

You are worried that is not explicit enough?

[sglow]

I'm pointing out that it's not safe to use bit-fields on some types of registers. If we do decide to switch to bit-field type access for registers, we can't do it universally.
We'll also be making the code less portable to different compilers, but that's probably not a huge concern.

[jlebar]

If we do decide to switch to bit-field type access for registers, we can't do it universally.

10-4; we cannot do it universally.

Right now we don't have a consensus opinion; we just have two people (me and Albertas) saying we think this is a good idea, versus one person (you) saying you'd prefer that we didn't do it.

One thing we could do is wait for a consensus; that seemed to be what you were asking for in the last two sentences of https://github.com/RespiraWorks/VentilatorSoftware/issues/332#issue-620124987. I am wondering if there is a way you'd feel comfortable with that would let us continue to hear your concerns while also not blocking development.

It is sounding like your concern at this point is mostly about the future -- will we apply this approach to some register where it's unsafe, will a different compiler respect -fstrict-volatile-bitfields in the same way or even lay out bitfields the same way, among other concerns. It is sounding like you don't think that the code in these PRs is incorrect as-is.

How would you feel about the following? Suppose we move forward with the two PRs while we await a consensus on this topic. If the consensus turns out to be, we do not want to use bitfields, I volunteer to get rid of them.

[sglow]

Yup, I'm just trying to get a discussion going on this. This seemed like a good place to do it. I was hoping some of the other developers would weigh in. I'll look at the PRs tonight and won't let the use of bit-fields prevent approval. Like you said, we can always clean up later if necessary

[sglow]

OK, I'm closing this due to lack of interest. We'll continue to use bit-fields in registers and just try to be careful not to shoot ourselves in the foot because of it.