CORE: _FLD2VAL() macro, brackets around passed parameter 'value' about cmsis_5

[GuentherMartin]

The macros will be changed to:

#define _VAL2FLD(field, value) (((uint32_t)(value) << field ## _Pos) & field ## _Msk)

#define _FLD2VAL(field, value) (((uint32_t)(value) & field ## _Msk) >> field ## _Pos)

[franzhollerer-Artesyn]

I am unsure if the _FLD2VAL() macro needs the cast. The intended use is:

id = _FLD2VAL(SCB_CPUID_REVISION, SCB->CPUID);

The register SCB->CPUID passed as 'value' has already an unsigned type. The macro could also be used for registers of type uint16_t, therefore I am unsure if the cast to uint32_t is counterproductive in this case or not.

The use of _VAL2FLD() is different. One of its intended use is to pass a literal constant as 'value' to the macro.

SCB->CPUID = _VAL2FLD(SCB_CPUID_REVISION, 0x3) | _VAL2FLD(SCB_CPUID_VARIANT, 0x3);

The literal constant defaults to int. The integer conversion then may result in a change of sign if the value is not casted to uint32_t.

Being said, I don't see a problem adding the uint32_t cast to the _FLD2VAL() macro. I only want to point out that the conditions are different. But I guess you are aware of this.

[franzhollerer-Artesyn]

I think there is a typo in the example for the _FLD2VAL() macro given here:

http://www.keil.com/pack/doc/CMSIS/Core/html/group__peripheral__gr.html#ga139b6e261c981f014f386927ca4a8444

id = = _FLD2VAL(SCB_CPUID_REVISION, SCB->CPUID);

should be changed to

id = _FLD2VAL(SCB_CPUID_REVISION, SCB->CPUID);

[GuentherMartin]

The macros will be changed to:
#define _VAL2FLD(field, value) (((value) << field ## _Pos) & field ## _Msk)
#define _FLD2VAL(field, value) (((value) & field ## _Msk) >> field ## _Pos)

A note will be added which says that value must be of type unsigned.
\note type of value must be unsigned.

Adding a cast (uint32_t) to parameter value within the macros will result in ineffective code if the register type is not uint32_t. To avoid warnings when using the macros cast the parameter value to unsigned if necessary.

x_32u = _VAL2FLD(POS31, 1);
will cause a warning about sign conversion.

x_32u = _VAL2FLD(POS31, 1u);
will not cause a warning.

[franzhollerer-Artesyn]

x_32u = _VAL2FLD(POS31, 1u);

Yes, that's correct. This will not cause the warning. But this is not intuitive and it's cumbersome for the user.

For the _VAL2FLD the left side term will mostly always be an unsigned type. Thus it is much more convenient for the user that the passed value is cast to an uint32_t before shifting it. There should not be a need to care about the 'u' suffix.

To emphasize this statement I want to point you to the original example part of the CMSIS documentation:

SCB->CPUID = _VAL2FLD(SCB_CPUID_REVISION, 0x3) | _VAL2FLD(SCB_CPUID_VARIANT, 0x3);

This example also misses the 'U' suffix. And this is ok. I think this is the natural way someone wants to use this macro.

Thus please go with

#define _VAL2FLD(field, value) (((uint32_t)(value) << field ## _Pos) & field ## _Msk)

After thinking about it I regret that I brought up the concerns about the _FLD2VAL() macro. From the point of the C standard the behavior of a right shift of a singed value is implementation-defined. With this argument, a cast to uint32_t is the better choice here as well.

#define _FLD2VAL(field, value) (((uint32_t)(value) & field ## _Msk) >> field ## _Pos)

I am sorry that I caused confusion.

[franzhollerer-Artesyn]

To give an example.

#define FOO_Pos 31
#define FOO_Msk (1U << FOO_Pos)

int bar = FOO_Msk | ......;  // just some initialization to show the problem
int val;

val = _FLD2VAL(FOO, bar);  // might get 1 or -1, depending on the compiler

Following the C standard ISO/IEC 9899:1999:

The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type
or if E1 has a signed type and a nonnegative value, the value of the result is the integral
part of the quotient of E1 divided by the quantity, 2 raised to the power E2. If E1 has a
signed type and a negative value, the resulting value is implementation-defined.

The variable val might get 1 or -1, depending on the compiler. It is better to avoid the sign extension and make sure that the result is always 1.

Thus please go on with the original plan and change the macros to

 #define _VAL2FLD(field, value) (((uint32_t)(value) << field ## _Pos) & field ## _Msk) 
 #define _FLD2VAL(field, value) (((uint32_t)(value) & field ## _Msk) >> field ## _Pos)

It may be a good idea to update the macro documentation accordingly:

/**
  \brief   Mask and shift a bit field value for use in a register bit range.
  \param[in] field  Name of the register bit field.
  \param[in] value  Value of the bit field. This parameter is interpreted as an uint32_t type.
  \return           Masked and shifted value.

  Example:
  \code
  SCB->CPUID = _VAL2FLD(SCB_CPUID_REVISION, 0x3) | _VAL2FLD(SCB_CPUID_VARIANT, 0x3);
   \endcode
*/
#define _VAL2FLD(field, value) (((uint32_t)(value) << field ## _Pos) & field ## _Msk)

/**
  \brief     Mask and shift a register value to extract a bit filed value.
  \param[in] field  Name of the register bit field.
  \param[in] value  Value of register. This parameter is interpreted as an uint32_t type.
  \return           Masked and shifted bit field value.

  Exampe
  \code
  id = = _FLD2VAL(SCB_CPUID_REVISION, SCB->CPUID);
  \endcode
*/
#define _FLD2VAL(field, value) (((uint32_t)(value) & field ## _Msk) >> field ## _Pos)

[GuentherMartin]

So finally I will change the macros and the description to your proposal.
Thank you for your contribution.

[GuentherMartin]

Included in CMSIS-Core.

[ReinhardKeil]

Closing as there is not further reply