STMicroelectronics / STM32CubeG0

STM32Cube MCU Full Package for the STM32G0 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
Other
165 stars 75 forks source link

Consistent RCC definitions #3

Closed salkinium closed 3 years ago

salkinium commented 5 years ago

Describe the set-up

Describe the bug

There are a few definitions missing that are available on all other devices with the same RCC implementation. This is annoying when you need to maintain a family independent RCC HAL.

How To Reproduce

Try to use RCC_CFGR_HPRE_DIV1 or RCC_CFGR_PPRE_DIV1 or RCC_CFGR_SW_HSI in any code that includes the STM32G0 header.

Additional context

Patches:

The specific use of these missing defines in our code.

CCASTM commented 5 years ago

Hello Niklas, Thank you for your remark. We will analyze it and come back to you. Regards Christophe

ALABSTM commented 5 years ago

Hi Niklas,

After reporting this issue to the technical committee the decision taken is to remove these definitions from the CMSIS. In fact, they are not the standard bit definitions recommended by Arm to be in the CMSIS.

Moreover as these constants are defined in the CMSIS, they are strictly meant to be used by the HAL and LL drivers, not the user-application code. Instead the user-application code is meant to use the following redefinitions present in hal_rcc.h and ll_rcc.h:

Actually, what you found on G0 and F0 is the fix that has been applied on these two firmwares. It is how things should be. The same fix still needs to be applied on the other firmwares.

These constants shall have not been defined in the CMSIS file. We do apologize for the inconvenience incurred.

With regards

ALABSTM commented 5 years ago

ST Internal Reference: 84151

salkinium commented 5 years ago

My understanding this from a technical point is that you as a vendor specify the CMSIS SVD and then use a converter (officially SVDConv.exe, but your headers seem manually edited too) to create the CMSIS header files in C.

The CMSIS SVD format explicitly describes enumerated value levels. (Ironically the example they use is of clock sources ;-) They are supposed to be used exactly for specifing the meaning of the bit fields.

Looking at your CMSIS-SVD files for the series, I find no such value level enumerations for the register RCC->CFGR (or any other register for that matter):

            <field>
              <name>HPRE</name>
              <description>AHB prescaler</description>
              <bitOffset>8</bitOffset>
              <bitWidth>4</bitWidth>
              <access>read-write</access>
            </field>

I've looked again at the header files, and indeed, they are completely consistent in that regard, there are only _Msk and _Pos values, and they reflect what is defined in the SVD file.

I think it's fine to be consistent about where you define what, however, PLEASE give use a machine-readable list of enumerations. The CMSIS-SVD files provide you the perfect mechanism to do so.

I hope you understand that there is considerable research being done for making embedded software more safe and secure, incl. by the Ada and Rust community. They rely on the SVD files to generate the appropriate language definition to access the registers and the lack of enumeration values is a significant burden.

There is a giant manual effort to fix and add the missing definitions by the Rust community: https://github.com/stm32-rs/stm32-rs

Note in particular the big set of patches of your SVD files: https://github.com/stm32-rs/stm32-rs/tree/master/peripherals

You should incorporate them into your own, so that YOU benefit from these corrections too. (This isn't just a one-way street, your own engineers may also be using these wrong definitions).

Please relay this to the technical commitee, so they can understand why this information is important and how you can improve your own software. This is important to a whole lot of embedded developers.