[ARM] [Sam] Uh is it a good idea to map registers to structs

[Siwastaja]

You can't access a peripheral register as an initializer for a global or static variable. Their initializers must be compile time constants. Think about it: when would the compiler run the code needed to get that value? For a local variable, that's OK, then the compiler emits the code to evaluate the value at that point.

If you want to "initialize" a global (or static) variable like that, just define it without initialization:
uint16_t EEPROM_START;

and then wherever you want to write the value at the NVMP register to it, do it:
EEPROM_START = NVMCTRL->PARAM.bit.NVMP;

If this information is available right at the boot, just do it beginning of main(), for example.

Am I right guessing this NVMP is some information field so that you use it instead of hard-coded number so you can port the code to different MCUs, with different value of NVMP?

[Simon]

Am I right guessing this NVMP is some information field so that you use it instead of hard-coded number so you can port the code to different MCUs, with different value of NVMP?

Yes

Although as these numbers are defined in the header for each variant I just need to use the define.

[Nominal Animal]

I definitely missed the fact that the structure inside the union wasn't anonymous, since I habitually use anonymous members in such cases.  Apologies.

uint16_t EEPROM_START = (*NVMCTRL).PARAM.bit.NVMP;

What is this expression supposed to achieve?

Right now it says "assign the current value of the bitfield NVMP to variable EEPROM_START".
If you put that in the global scope, the compiler cannot tell exactly when it is supposed to do that assignment.
If the right side was a compile-time constant, then it would not matter; so, for sanity, only constant expressions are allowed as initializers for file scope variables.

If instead you wanted to set EEPROM_START address to the address of the NVMP member, something like
    uint16_t  EEPROM_START = (uint16_t)&(NVMCTRL->PARAM.bit.NVMP);
but that will not work, because NVMP is a bitfield, and you can't take the address of a bitfield in C.  Instead, you'll want to use
    uint16_t  EEPROM_START = (uint16_t)&(NVMCTRL->PARAM.bit);
or even
    uint16_t  EEPROM_START = (uint16_t)&(NVMCTRL->PARAM);

[Simon]

So I'll just use the simple defines. I want the code to be easy to write and easy to read. As there is a header file per device variant that uses the same define names it will serve the same purpose. I just type REG_peripheral name and keep following the suggestions until I get the register I want. I can do bit masks.

[Siwastaja]

Classical bit masks are fine, you don't need to use bitfields, and that union gives you the choice. The idea of bitfields is readability (and obviously writability), but there are a few catches, like the implementation defined ordering (which is no issue if you always use GCC and the same platform), and the performance issues when accessing volatile registers, but that isn't because of the bitfields per se, it's just that bitfields make the problem much more severe.

Even with bitmasks, you hit the performance issues: if you want to set a bit and clear another, you normally do:
reg &= ~(1UL << 5);
reg |= 1UL<<3;

But this results in two read-modify-write cycles for volatile reg, while what you usually want, but don't bother writing, is:
tmp = reg; // tmp is NOT qualified volatile
tmp &= ~(1UL <<5);
tmp |= 1UL<<3;
reg = tmp;

If only C had some mechanism to say, hey, you can temporarily ignore the volatile qualifiers, let the compiler combine and reorder operations, and then bang, end that region with some special keyword to assign the results to the memory locations. That would save hundreds of bytes of initialization code space right away.

[SiliconWizard]

and the performance issues when accessing volatile registers, but that isn't because of the bitfields per se, it's just that bitfields make the problem much more severe.

You're right! Performance is usually not an issue when accessing non-volatile variables, because the optimizer will group accesses and it won't make a difference with using bit masks. But if using a volatile-qualified object, that's completely different. *Every* access of a bitfield will lead to an actual memory access. This can be highly inefficient.

Now it's all in the way you use the bit fields. As you showed, you can also have performance issues if you use bit masks in a number of separate assignments (with volatile objects).
In both cases, one way of circumventing it is to use temporary non-volatile variables and assign (or read from) only ONCE to the volatile object. Not as elegant, of course.

[Siwastaja]

You're right!

I'm always right, even when I'm not!

Anyway, this is a good example how C is not "portable assembler". The solution which looks more elegant, shorter, and "right", produces the unwanted result, which is longer, and doing more than necessary. While the code that produces just the minimum elegant output, looks longer and ugly.

This limitation is because the actual machine (and thus, assembly language) has the concept of type per every operation, while C has type assigned to each variable, and accesses happen automatically based on that stored type. Usually the type stays consistent which makes programming easier, but when it does not, it results in quite confusing casting orgies, or usage of temporary variables like in my example, because you can't cast the volatile away in that example. And all you wanted is to tell compiler, "modify the peripheral register like this", and the only tools you have is the choice between "access the peripheral register always when its name appear in code" or "don't access the peripheral register at all", and you need to work around that type limitation.

But I have to clarify this is all pretty acceptable because C at least is quite well standardized and not massively complicated, so you just have to learn how it works. This is at least possible, and it's not a moving target.

[NorthGuy]

... performance issues when accessing volatile registers, but that isn't because of the bitfields per se, it's just that bitfields make the problem much more severe.

I don't think bitfields are less efficient than masks. Bitfield operations are likely to be implemented through masks by the compiler, so it is likely to be exactly the same as writing down the masks. If you want to reduce reads you can always create a temprary bitfielded variable the same as you did with masks.

Moreover, many CPUs have bitfield manipulation instructions, so using bitfields may be more efficient than using masks, especially with multi-bit bitfields.

There are some cases where masks must be used, for example when different parts of the same hardware register are accessed from both the main code and from the interrupt. Other than that, there's nothing wrong with using bitfields.

[Simon]

It's Microchip, the manufacturer that supply the header files, full of structured and bit fielded registers, I assume they, cough, know what they are doing. i also assume they only warrant it to work with "their" compiler that even though it is the GCC-ARM one they may check each update to make sure that bitfield packing has not been broken before redistributing it. I'll just use the register defines: KISS

[westfw]

I don't also like not-invented-here naming like __IO

That's part of the ARM CMSIS spec.  They use it to distinguish read/write registers vs read-only or write-only registers.
https://www.keil.com/pack/doc/CMSIS/Core/html/group__peripheral__gr.htmlI don't know if there are any compilers that can actually make use of such information, but I guess it makes the .h file slightly more self-documenting, which I guess is good.  (But as per an ongoing avr-freaks discussion, we also sort of need a tag of some kind for registers whose read value and write value are different.)

(Of course, if we're talking about NVMCTRL->PARAM, it's actually a read-only register, even though the .h files have it tagged as __IO.  Sigh.)

I assume [Microchip], cough, know what they are doing.

You didn't look at the changes in the new (MPLABX) packs, did you?  All the bitfields are gone.

Code: [Select]

#define NVMCTRL_PARAM_NVMP_Pos                _U_(0)                                               /**< (NVMCTRL_PARAM) NVM Pages Position */
#define NVMCTRL_PARAM_NVMP_Msk                (_U_(0xFFFF) << NVMCTRL_PARAM_NVMP_Pos)              /**< (NVMCTRL_PARAM) NVM Pages Mask */
#define NVMCTRL_PARAM_NVMP(value)             (NVMCTRL_PARAM_NVMP_Msk & ((value) << NVMCTRL_PARAM_NVMP_Pos))
   :

typedef struct
{  /* Non-Volatile Memory Controller */
  __IO  uint16_t                       NVMCTRL_CTRLA;      /**< Offset: 0x00 (R/W  16) Control A */
  __I   uint8_t                        Reserved1[0x02];
  __IO  uint32_t                       NVMCTRL_CTRLB;      /**< Offset: 0x04 (R/W  32) Control B */
  __IO  uint32_t                       NVMCTRL_PARAM;      /**< Offset: 0x08 (R/W  32) NVM Parameter */
  __IO  uint8_t                        NVMCTRL_INTENCLR;   /**< Offset: 0x0C (R/W  8) Interrupt Enable Clear */
   :
} nvmctrl_registers_t;
(Heh.  Still labeled as __IO, though.)

If only C had some mechanism to say, hey, you can temporarily ignore the volatile qualifiers, let the compiler combine and reorder operations, and then bang, end that region with some special keyword to assign the results to the memory locations.

Code: [Select]

void myParamInit() {
  NVMCTRL_PARAM_Type tmpParam = NVMCTRL->PARAM;
  tmpParam.bit.NVMP = foo;
  tmpParam.bit.PSZ = bar;
  tmpParam.bit.RWEEO = baz;
  NVMCTRL->PARAM = tmpParam;
}
Or something like that.  It's a neat use of the way that Atmel defined all the sub-types.  Last time I looked, ASF libraries were full of code like this (but I can't find an example now. :-( )
(of course, you need to realize that this is useful.)

[westfw]

The purist would say: don't do it, it's potentially dangerous and non-portable.

Yawn.  I've been doing this for 30+ years now, and the benefits far outweigh the "dangers."

You need to use a compiler that allows defining alignment

I'm not sure how alignment comes into it.  Presumably the addresses of the IO registers are aligned as they need to be.

You do need a compiler that will "pack" the structures, and you need to be away of (CPU-dependent) bitfield ordering (IFF you use bitfields.)   But any compiler vendor who does something really strange with structure packing (without a workaround) is going to be laughed out of the market.  And if we're talking about IO registers, they're already highly not-portable.

(Now, mapping structures onto something like networking packets (or protocol headers within a packet) is somewhat more dangerous.  THEY can have all sorts of alignment issues.)

[Nominal Animal]

Code: [Select]

void myParamInit() {
  NVMCTRL_PARAM_Type tmpParam = NVMCTRL->PARAM;
  tmpParam.bit.NVMP = foo;
  tmpParam.bit.PSZ = bar;
  tmpParam.bit.RWEEO = baz;
  NVMCTRL->PARAM = tmpParam;
}
Or something like that.  It's a neat use of the way that Atmel defined all the sub-types.

One non-struct approach would be something like

Code: [Select]

/* Update Param with new values.
   Use negative to indicate no change in corresponding value, or
   zero or positive to set the new value.
*/
void myParamSet(int foo, int bar, int baz) { /* ... omitted ... */ }where the omitted function body reads the original register(s), modifies them according to the parameters – noting that this assumes valid values are nonnegative and at least two bits smaller than a full unsigned int – and then updates the registers.

For example,
    myParamSet(0, -1, -1);
would only clear foo to zero, leaving bar and baz unchanged.

This is particularly useful in the case when the registers being modified are normally locked using some kind of protection register, as it can hide such details.  It also ensures a single read-modify-write cycle.

This does not work that well when the bit fields in the register are used in completely different contexts.  That is relatively rare, though.
This is also not that useful when there are too many fields (function parameters), unless the function is static and gets inlined (in which case the parameters do not need to be passed on the stack).

Note: I am not saying this is better.  I am just showing what one of the alternatives looks like.

[SiliconWizard]

But I have to clarify this is all pretty acceptable because C at least is quite well standardized and not massively complicated, so you just have to learn how it works. This is at least possible, and it's not a moving target.

Oh yep. Sure. We're certainly even splitting hairs here. That potential "inefficiency" will matter only if you're after saving a few cycles. If so, it's good to know what is efficient and what isn't, but even so, in this case, you sometimes have to resort to assembly. Otherwise, whatever approach you find more readable and more comfortable with is usually the way to go.

What is important to know is that with volatile-qualified objects, the compiler can't optimize accesses and must honour all of them, in the order they appear. It has implications beyond efficiency. In some cases, for peripheral registers for instance, setting or clearing some bits *in a certain order* actually matters, so you wouldn't want the compiler to group accesses behind your back.

[Simon]

This is low level setup code that has to know the addresses of the registers. In the past I wrote my own, now I have delved into the files and found them defined in a clear enough way I am happy to not bother reinventing that wheel. But the structures and bit fields are going to be more pain than they are worth. I believe most of the desired bit field masks are already defined so meh.

[Simon]

While we are on the subject of documentation, attached is the memory organization. It's nonsense to me.

Surely it should be, ( "row n" * "page bytes" * 4 ) + ( "page n"  * "page bytes"  ) + base address of page/row 0.

Given

#define FLASH_USER_PAGE_ADDR  _UL_(0x00800000) /**< FLASH_USER_PAGE base address */
#define FLASH_PAGE_SIZE       64

So
row 0 page 0 starts at 0*64*4 + 0*64 + 0x00800000 = 0x00800000
row 0 page 1 starts at 0*64*4 + 1*64 + 0x00800000 = 0x00800040
row 0 page 2 starts at 0*64*4 + 2*64 + 0x00800000 = 0x00800080
row 0 page 3 starts at 0*64*4 + 3*64 + 0x00800000 = 0x008000C0

row 1 page 0 starts at 1*64*4 + 0*64 + 0x00800000 = 0x00800100
row 1 page 1 starts at 1*64*4 + 1*64 + 0x00800000 = 0x00800140

....................................................

[Nominal Animal]

Surely it should be, ( "row n" * "page bytes" * 4 ) + ( "page n"  * "page bytes"  ) + base address of page/row 0.

That's how I read it, too.

In other words, let P be the size of each page in bytes, B be the base address, r be the row number one wants to access, and p the page number (0 to 3) one wants, the page address is
    B + P * (4 * r + p)
which also matches your example addresses.

[Simon]

Well that is not how I read their diagram but that is how I read the text and how I would implement it.

[NorthGuy]

What is important to know is that with volatile-qualified objects, the compiler can't optimize accesses and must honour all of them, in the order they appear. It has implications beyond efficiency. In some cases, for peripheral registers for instance, setting or clearing some bits *in a certain order* actually matters, so you wouldn't want the compiler to group accesses behind your back.

These are general consideration which appy whether you use structs or not. The difference in using structs (as opposed to accessing individual registers) is that they provide you a pointer to the hardware module with all the attached benefits.

The pointer to the struct is a reference to the hardware module. Therefore, if your MCU has 5 moduli of the same kind, your code can access any of them without knowing which one of the modules it is accessing. This gives you an opportunity to switch to a different module if needed - either within your project, or in the next project, or even dynamically at the run time. You can even use dynamic allocation - such as allocating DMA moduli on demand.

IMHO, this is a huge benefit. And on the load/store architecture there's almost no penalty in terms of efficiency.

[Simon]

The pointer to the struct is a reference to the hardware module. Therefore, if your MCU has 5 moduli of the same kind, your code can access any of them without knowing which one of the modules it is accessing. This gives you an opportunity to switch to a different module if needed - either within your project, or in the next project, or even dynamically at the run time. You can even use dynamic allocation - such as allocating DMA moduli on demand.

The other way is what I do. I define each address:

Peripheral base address
Offset between instances
Each register of an instance

This makes it very easy to write code that can access a giver register of any instance of that peripheral so you write setup code or common functions once.

void do_thing_that_wants_to_write_to_a_register(instance)
{
        "pointery stuff" base address + instance offset * instance + register = thing ;
}

[Siwastaja]

The pointer to the struct is a reference to the hardware module. Therefore, if your MCU has 5 moduli of the same kind, your code can access any of them without knowing which one of the modules it is accessing. This gives you an opportunity to switch to a different module if needed - either within your project, or in the next project, or even dynamically at the run time. You can even use dynamic allocation - such as allocating DMA moduli on demand.

Manufacturers do love to ruin that fun by making every instantiation of the peripheral slightly different. Different field widths in timers, different FIFO sizes, some instances lack some of the features. Or just have completely different peripherals which really do the same thing. Think about STM32H7 series, they have three (3!) different DMAs: MDMA, DMA and BDMA, and specifically some peripherals can only use BDMA, and some can only use DMA. Besides, DMA and BDMA are very similar and both are just design reuse from older ST devices (BDMA is the F0 series DMA renamed, DMA from higher-up devices), so why not have DMA1, DMA2, DMA3 all of the same design, instead of DMA1, DMA2 and BDMA. Maybe they saved 100 transistors. Probably not even that.

Some manufacturers indeed love making programmer's job as difficult as possible. On the other hand, now I'm writing firmware for Nordic Semiconductor devices and I struggle in the opposite way, the peripheral design is just so clean and easy to use I need to concentrate in the actual application code, and the mental quality bar is now set higher. With STM32 for example, significant part of effort goes to working around broken mess ST gave us to handle, and code that works around it simply can't look beautiful.

I can see my workflow totally changed: with ST devices it's "read documentation for hours, write code, test, doesn't work, read more, modify, test, find HW bugs, work around, test, read more, modify, test, modify, need more coffee, it seems to work, repeat 5 times, verify it actually works". With nRF parts it's "read documentation quickly, write code SLOOWLY and painfully because you need to actually concentrate, test -> works perfectly the first time".

[cv007]

Using structs is a very good idea

It is ideal so you can let the compiler deal with bit manipulation, but the downside is you usually have to create them yourself. Not so bad for 8bit, but gets worse when your 32bit mcu has a lot of peripherals. You still have to keep in mind what is happening behind the scenes as the code gives the appearance something simple is happening as it looks like simple assignment. In the end your code should be 'hidden' by higher level functions, so these low level bits of code are in many instances a one time creation so the bigger benefit is in any necessary reading of the code later (don't have to look over a bunch of bit manipulation code).

One upside to creating your own structs, is you can get a complete 'driver' in a single header where all relevant information is sitting in one header file so there is no need to be hunting down headers. You can also choose to do your own thing on an as-needed basis, but then you still have to deal with manufacturer headers for anything else (learn to use what they provide for you in any case).

A simple example of a gpio 'driver' for an nrf52-
https://godbolt.org/z/7ec4YsK68
This is a little more advanced (and in c++), but shows you can also get a pin specific register layout where there is no 'manual' bit manipulation in any code. This piece of code can live in a header, can be used for any nrf52 by having an mcu specific PIN enum. In this case, one can even create it in the online compiler since it does not depend on the manufacturer headers. The downside in this case is the use of templates, which is fine but your code gets 'template infected' and can get to a point where its just too much. Without templates and without c++ you can still get to the same destination and since you would be using the functions it would look the same on the outside.

A compromise is to use the headers provided to you, creating the functions you will use for the most part-
https://github.com/cv007/NUCLEO_G031K8/blob/main/Gpio.hpp
You end up doing the bit manipulation yourself, but in the end it gets used the same as the nrf52 code above- you use functions to do things and you are not constantly having to directly deal with registers. In this case the manufacturer header is a single file, and in the case of your sam you end up with a couple folders plus a file or two but is still manageable.
edit-- sorry, the above version I created the register struct, this is the one that uses the st headers, -
https://github.com/cv007/NUCLEO32_G031K8_B/blob/main/Gpio.hpp


Learn to use the provided headers in any case, since you will most likely have to use them at some point anyway.


edit- there is also a mistake in the nrf52 example, which is the type of mistake one can make using these structs/bitfields (last function is wrong). Corrected-
https://godbolt.org/z/PhndsPWYo

[westfw]

Learn to use the provided headers

This is a very important point that I don't think anyone else has mentioned yet.  If  you're "professional", it is likely that any prospective employer will want you to be familiar with The Vendor's Standard Way of Doing Things.  No one wants a programmer that generates code that looks weird to everyone else on the project, or whoever your successor is likely to be.  Even in the OSSW world, everyone who ever looks at your code will thank you if it looks like "standard Atmel-style ARM code", and doesn't use custom-named register definitions extracted from the .SVD XML and a bunch of custom macros.
(Most ARM vendors, including Atmel/Microchip have both structure-oriented and individual register definitions, though.  So yiou can be a hold-out for a while.)

you usually have to create them [(structs_] yourself.

Not so, any more.  ARM specifically recommends using structures for peripherals (the part of CMSIS that most vendors actually implement.)  It's been a long time since I've seen a 32bit microcontroller that hasn't had vendor-provided structure-based definitions for their on-chip peripherals.  (The "fun" part is when you want to use something like that for asm code...)

Use of bitfields is convenient, but less common and more dangerous.

Anonymous unions are nice, but most vendors seem to be reluctant to use any C features from any version later than C99.
Having a structure member "ctrl" is nice from  a typing. reading, and logic point of view, but sort-of sucky if you're trying to find all the places in your code that modified the uart ctrl register, since other peripherals likely have a register named "ctrl" as well.

Having an IDE with context-sensitive completion and help is nice, but "uart->c" and "uartreg_c" probably act similarly.

Code like:

Code: [Select]

   uart_t *u = &USART1;
   u->brgen = BAUD2DIV(9600);
   u->ctrla = foo;
   u->ctrb = bar;Is more likely to get optimized effectively than

Code: [Select]

   USART1_brgen = BAUD2DIV(9600);
   USART1_ctrla = foo;
   USART1_ctrb = bar;(But see also https://embdev.net/topic/426508 )

[cv007]

Not so, any more.

I meant bitfields inside the struct.

Use of bitfields is convenient, but less common and more dangerous.

I showed a case where a mistake can be made for some of the 'odd' registers (typically the write 1 to do something type), but I'm not sure that is any worse than sorting out a flawed bit manipulation statement that may be several lines of code (if using the defines given for bit positions, with their long-winded names). Either one can be easily/quickly fixed as it won't be your first mistake of this type, plus you typically test what you just wrote so your problem code is usually right in front of you. I'm not sure what other dangers are lurking, but have not seen the compiler doing anything other than what is expected with these structs that have bitfields (I only use gcc, various mcu's, all using bitfields).

Here is the nrf52 example with the helpful bitfields and register struct template removed (plain struct)-
https://godbolt.org/z/15rWsh8M7
Produces the exact same code, but the functions are both harder to create and read.

[bson]

Volatile structs with bit field members makes for poor optimization; every single struct member access has to be kept as-is since it's an implied optimization memory barrier, and they can't be collected by the compiler into combined bit mask operations.  I think it's a much better idea to simply have volatile unsigned integers of the right size and do the masking yourself.  Usually you can clear multiple fields and update them in a single operation.

The exception is for non-volatile binary bit fields, like protocol headers.  In this case the compiler can freely optimize access.

[cv007]

Volatile structs with bit field members makes for poor optimization;

You still have the choice to not use them, assuming a union was created so you can also access the whole register. There may be peripheral registers that have a number of bitfield 'groups' that need to be set in a single function, you can adjust as needed and are not required to use the bitfields.

In the nrf52 example, the pin properties are in a single register and there are functions that deal with individual pin properties that make use of the bitfields. In addition, there is an init function that 'accumulates' the property values (provided by enum values in any order) through the private init_ function templates that end up setting all the pin properties in a single write (compiler sorts it all out at compile time, also notice bitfields are also in use in these functions).