Use of C language bitfields for device register I/O in STM32CubeC5

[mtc]

I noticed that stm32c5xx_hal_fdcan.c uses C language bitfields to access device registers. Since bitfields are compiler specific, every knowledgeable embedded software engineer knows that using bitfields to access device registers is non-standard and not to be used. I suggest that using bitfields to access device registers be changed. You can refer to STM32CubeH7 for correctly accessing device registers.

 

BTW. I tried to open a support issue in the on-line support, but received the following error:

• Case_trigger: execution of AfterInsert caused by: System.DmlException: Update failed. First exception on row 0 with id 0033W00000YERuSQAX; first error: FIELD_CUSTOM_VALIDATION_EXCEPTION, Please specify a valid Zip Code: [] Class.CaseUtility.addInfoToContactAndLead: line 231, column 1 Trigger.Case_trigger: line 38, column 1

[TDK]

I noticed that stm32c5xx_hal_fdcan.c uses C language bitfields to access device registers.

Where? I don’t see it. Could be missing it. The file is 4800 lines, so some specifics would help here.

[mtc]

The following structures use bitfields: hal_fdcan_tx_header_t, hal_fdcan_rx_header_t, hal_fdcan_tx_evt_fifo_header_t

In routine FDCAN_CopyMessageToRAM(), lines 4419, 4420.

In routine HAL_FDCAN_GetReceivedMessage(), line 3194

In routine HAL_FDCAN_GetTxEvent(), line 3044

 

[TDK]

I tend to agree with you here, though it’s not an issue in practice. HAL is built for specific set of toolchains that all behave consistently.

It’s interesting that they moved from the structure in HAL to the HAL2/C5 structure. One of the common complaints of HAL was the size of the library. This was one way to cut down the memory footprint. Can’t have everything.

[Pavel A.]

> Since bitfields are compiler specific, every knowledgeable embedded software engineer knows that using bitfields to access device registers is non-standard and not to be used

True, but as the things change, this statement became less solid. All compilers that matter these days have options to ensure the desired allocation of bit fields.

A static assert or runtime check can be used to verify that a struct with bit fields is compiled correctly.

Of course if you have a legacy toolchain or a specific requirement, there’s a problem.

 

[mtc]

There will always be a problem with using C language bitfields for device register I/O. And since there are well-known, efficient and language standard techniques for dealing with this problem, there is no good argument for using bit fields. In this specific case, defining the data structure as a uint64_t and defining the enums as the actual bit values that can be ORd together, is a standard and efficient.technique.

Leaving the bitfield design, just propagates bad design decisions. It should be changed.

If anyone knows how to raise this with STM, please do. Or let me know how and I’ll raise it.

[Pavel A.]

You can open an issue on github for the STM32C5 HAL.

 

[gbm]

What exactly might be compiler-specific in bit field handling? The standard does not leave too much place for improvisation and modern compilers tend to be quite compatible with each other. Practically the only dangerous aspect is endiannes but lets be realistic - ARM-M and RISC-V are both little-endian. If the base type is correctly defined (volatile uint32_t for typical register access) there should be no dangers.

In my code I usually define unions with bit field based structure and word member, then I use bit fields for assembling the data and word register access for updating the SFR. This works flawlessly.

[Andrew Neil]

What exactly might be compiler-specific in bit field handling?

From K&R:

“Almost everything about fields is implementation-dependent. Whether a field may overlap a word boundary is implementation-defined. Fields need not be names; unnamed fields (a colon and width only) are used for padding. The special width 0 may be used to force alignment at the next word boundary.”

[gbm]

K&R is over 40 years old. Hey, today it’s C23! ;) The field must fit in a single base type item - this is quite strict. So for example struct {uint8_t r:5, g:6, b:5;} takes 3 bytes, while struct {uint6_t r:5, g:6, b:5;} takes one 16-bit word. The standard is quite precise here since C99.

[Andrew Neil]

https://stackoverflow.com/questions/75283038/bit-fields-in-c-c-what-is-guaranteed-what-is-implementation-defined

[gbm]

Lets be practical. Assuming we agree on the endianness, it’s hard to think of a reasonably written code that would result in an unpredictable behavior here. ABI specifies whatever is not specified in the language standard (including the endiannes, sizes, alignments etc.) and the compiler must obey ABI rules. So, the portability of code using bit fields should not be any worse than the portability of code not using bit fields.

[Andrew Neil]

Agreed - as ​@TDK and ​@Pavel A. said earlier, it’s moot as the STM32 HAL is targetting a specific, known set of compilers for just one known target architecture.

[Saket_Om]

Hello ​@mtc 

Thank you for raising this point. These bitfields are not MMIO definitions for FDCAN peripheral registers. They model the 64-bit FDCAN message-RAM header, with field widths and ordering deliberately matching the exact two-word R0/R1 structure defined in the STM32C5 reference manual.

 

The union provides two views of the same object: b gives convenient named-field access, while d64 provides the compact 64-bit representation. The driver transfers the header to and from message RAM as two 32-bit words. FDCAN peripheral registers are accessed separately through FDCAN_GlobalTypeDef and the dedicated register-access macros.

 

This HAL2 representation is intentional and was selected to reduce the header footprint while preserving a convenient API. Its layout relies on the ABI of the supported STM32 toolchains and target architecture.

[mtc]

My mistake for assuming the bit-fields were used for accessing device registers. They are used for accessing Rx/Tx message buffer contents. My point still stands: using bit fields to access device registers or parse a message buffer is problematic since the layout of the fields is non-standard. Given that there are well-known, standard and efficient techniques to address this issue, your use of C language bitfields should be changed.

The argument that you only support a select hardware architecture and specific compilers is not relevant because things change.

The well-known, standard and efficient techniques are both hardware and compiler independent and are as efficient or even more so than C bit-fields. The efficiency is in both generated object code and the amount of source code. For example:

uint32_t v = FLD_0 | FLD_1 | FLD_2;

vs

struct flds_t v;

v.fld_0 = FLD_0;

v.fld_1 = FLD_1;

v.fld_2 = FLD_2;

[TDK]

The two examples you show are only equivalent if you are setting all fields which is not how the structure is being used. If you actually implement the same functionality “v = FLD_0” now requires a mask and is neither more efficient nor more clear than “v.fld_0 = FLD_0”. This is even ignoring the argument that you can just use the 64-bit member of the union to do exactly what you’re showing--assign all fields in one statement.

They’re not going to change the library and break existing working code just because you believe another way of doing it is better. Especially as you’ve been told it was intentionally designed that way.