FLASH_OB_GetUser() wrongly reports OB_USER_SWAP_BANK bit

[liteyear]

In stm32u5xx_hal_flash_ex.c, version 1.6.2, FLASH_OB_GetUser() is defined as:

/**
* @brief Return the FLASH User Option Byte value.
* @retval The FLASH User Option Bytes values.
* The return value can be a combination of @ref FLASH_OB_USER_BOR_LEVEL,
* @ref FLASH_OB_USER_nRST_STOP, @ref FLASH_OB_USER_nRST_STANDBY,
* @ref FLASH_OB_USER_nRST_SHUTDOWN, @ref FLASH_OB_USER_SRAM_RST,
* @ref FLASH_OB_USER_IWDG_SW, @ref FLASH_OB_USER_IWDG_STOP,
* @ref FLASH_OB_USER_IWDG_STANDBY, @ref FLASH_OB_USER_WWDG_SW,
* @ref OB_USER_SWAP_BANK, @ref FLASH_OB_USER_DUALBANK,
* @ref FLASH_OB_USER_BKPRAM_RST, @ref FLASH_OB_USER_SRAM3_ECC,
* @ref FLASH_OB_USER_SRAM2_ECC, @ref FLASH_OB_USER_SRAM2_RST,
* @ref FLASH_OB_USER_nSWBOOT0, @ref FLASH_OB_USER_nBOOT0,
* @ref FLASH_OB_USER_PA15_PUPEN, @ref FLASH_OB_USER_IO_VDD_HSLV,
* @ref FLASH_OB_USER_IO_VDDIO2_HSLV and @ref OB_USER_TZEN
*/
static uint32_t FLASH_OB_GetUser(void)
{
uint32_t user_config = READ_REG(FLASH->OPTR);
CLEAR_BIT(user_config, FLASH_OPTR_RDP);

return user_config;
}

The reference to OB_USER_SWAP_BANK seems significant, since all the others are prefixed with FLASH_.

This turns out to be misleading, and unpacking the intent is confusing.

OB_USER_SWAP_BANK is 0x200, which does not correspond to a relevant bit in FLASH->OPTR. Like the other return values, I think this should be the defgroup FLASH_OB_USER_SWAP_BANK instead. The options in that group include OB_SWAP_BANK_ENABLE, defined as FLASH_OPTR_SWAP_BANK, which is indeed the SWAP_BANK bit (1<<20) in FLASH->OPTR.

As far as I can tell, OB_USER_SWAP_BANK should only be used as a UserType in FLASH_OB_UserConfig(). Which means the comments on that function, the FLASH_OBProgramInitTypeDef struct, and the FLASH_OB_USER_Type defgroup are all back to front as well. That means all the IS_OB_USER_* defines are misleadingly named as well. Correcting these to make a distinction between the FLASH_OB_USER_* defgroups, and the OB_USER_* UserTypes seems readily achievable and a big improvement in usability.

[Saket_Om · Accepted solution]

Hello  @liteyear 

Thank you for bringing this issue to our attention.

I reported this internally.

Internal ticket number: 215482 (This is an internal tracking number and is not accessible or usable by customers).

[Hai]

Hello  liteyear,

Thank you for your feedback.

The reference @ref OB_USER_SWAP_BANK should be replaced by the corresponding type group @ref FLASH_OB_USER_SWAP_BANK, which includes defined items:

- OB_SWAP_BANK_DISABLE 

- OB_SWAP_BANK_ENABLE.

the following one:

It appears that the original Doxygen tag comment reference was a mistake (highlighted in yellow).

Besides, the FLASH_OB_GetUser() function returns the "user option-bytes type", which is a logical value defined independently of the OPTCR register bit definitions. This value is used to specify the desired option-byte type before configuring it (See listed defines below):

Please feel free to respond and confirm whether this resolves your request.

Best regards,
Haifa.

[Hai]

The same fix should be applied to OB_USER_TZEN by adding the FLASH_ prefix.

[liteyear]

---

@Hai wrote:

Hello  liteyear,

Thank you for your feedback.

The reference @ref OB_USER_SWAP_BANK should be replaced by the corresponding type group @ref FLASH_OB_USER_SWAP_BANK

---

Yes, that's the conclusion I came to.

Besides, the FLASH_OB_GetUser() function returns the "user option-bytes type", which is a logical value defined independently of the OPTCR register bit definitions.

---

I don't think so. That the incorrect assumption I made too, based on the comments. As you can see in the code I included in my post, it literally returns the register value. The definitions in the "FLASH_OB_USER_* groups happen to be the register bit values. The logical values only seem to be used for FLASH_OB_Config().

Thanks for following up.

[Hai]

When replacing @ref OB_USER_SWAP_BANK with @ref FLASH_OB_USER_SWAP_BANK, and @ref OB_USER_TZEN with @ref FLASH_OB_USER_TZEN, the references will match the function's return type, which corresponds to the current value of each option byte.

[liteyear]

Yes. I think we're probably saying the same thing, though I don't know what you mean by "value of each option byte" precisely. It's subtle and there's a few comments that get it back to front. I attempted to list them in my OP.

But I think we agree? FLASH_OB_GetUser() returns the actual register values, which are defined in the FLASH_OB_USER_* groups. The replacements you list will correct that function's comment.

On the other hand, FLASH_OB_Config() takes "logical" values like OB_USER_SWAP_BANK , so the comments I mentioned in the OP are back to front. As far as I can tell, that's the only function that uses those values.

[Hai]

Yes, I completely agree with you, we are aligned.

By "current value of each option-byte", I refer to the presently configured option-bytes for each type.

For example, when retrieving "the swap bank status", you obtain whether it is enabled or disabled based on the bit field value within the OPTCR register which is returned by FLASH_OB_GetUser().

Additionally, as you mentioned, the function FLASH_OB_UserConfig(uint32_t UserType, uint32_t UserConfig) takes as parameters the logical type "UserType" and the corresponding value to be configured "UserConfig".

The fix should be applied to both the static Set and Get functions: