Serious issue with CubeMX code generation and HAL Library. Please review.

[MFreu]

I believe I have found a serious code creation bug in STCubeMX concerning PLL initialization on (at least) STM32F767ZI MCUs. Additionally, I take issue with some pre-processor macros used for register manipulation that allowed the code creation bug to become a very serious issue. Please read the detailed description.

We created code using the CubeMX tool and have soon discovered the following symptoms:

With compiler optimizations enabled (-O3), the MCU's timers ran with double frequency after a power-cycle. If you reset (while keeping the power enabled) the timer frequency would be correct.

Analysis showed, during init of the RCC subsystem some register values are set incorrectly. Namely the RCC_DCKFGR1 register, which controls some clock muxes and prescalers for peripherals. Please have a look at this piece of Cube generated init code taken from the SystemClock_Config() function in "main.c"

Exhibit A:

  PeriphClkInitStruct.PeriphClockSelection = RCC_PERIPHCLK_USART1|RCC_PERIPHCLK_SAI1|RCC_PERIPHCLK_SAI2|RCC_PERIPHCLK_CLK48;
  PeriphClkInitStruct.PLLSAI.PLLSAIN = 96;
  PeriphClkInitStruct.PLLSAI.PLLSAIR = 2;
  PeriphClkInitStruct.PLLSAI.PLLSAIQ = 3;
  PeriphClkInitStruct.PLLSAI.PLLSAIP = RCC_PLLSAIP_DIV4;
  PeriphClkInitStruct.PLLSAIDivQ = 1;
  PeriphClkInitStruct.PLLSAIDivR = RCC_PLLSAIDIVR_2;
  PeriphClkInitStruct.Sai1ClockSelection = RCC_SAI1CLKSOURCE_PLLSAI;
  PeriphClkInitStruct.Usart1ClockSelection = RCC_USART1CLKSOURCE_PCLK2;
  PeriphClkInitStruct.Clk48ClockSelection = RCC_CLK48SOURCE_PLL;
  if (HAL_RCCEx_PeriphCLKConfig(&PeriphClkInitStruct) != HAL_OK)
  {
    _Error_Handler(__FILE__, __LINE__);
  }

As you can see, "PeriphClockSelection" includes the flag for "RCC_PERIPHCLK_SAI2" although SAI2 is configured as disabled for both blocks A and B in CubeMX. However, the associated configuration variable is NOT SET before the init struct is used for register init. Since the init struct is stack-allocated, the value of not explicitly set variables of this struct are UNDEFINED. What happens is that the register gets set to "some value" that happens to live in the RAM at the time of init. Register configuration that happens before the SAI2 config may be lost (depending on the state of RAM).

Why is SAI2 clock selection enabled but not configured in code, when it is disabled in Cube?

Now you may ask, how can setting a few bits in the register change other bits? Well that's because of the very bad design of the macro used for register editing.

Exhibit B:

#define SET_BIT(REG, BIT)     ((REG) |= (BIT))
 
#define CLEAR_BIT(REG, BIT)   ((REG) &= ~(BIT))
 
#define READ_BIT(REG, BIT)    ((REG) & (BIT))
 
#define CLEAR_REG(REG)        ((REG) = (0x0))
 
#define WRITE_REG(REG, VAL)   ((REG) = (VAL))
 
#define READ_REG(REG)         ((REG))
 
#define MODIFY_REG(REG, CLEARMASK, SETMASK)  WRITE_REG((REG), (((READ_REG(REG)) & (~(CLEARMASK))) | (SETMASK)))
 
#define POSITION_VAL(VAL)     (__CLZ(__RBIT(VAL)))

Notice how "WRITE_REG" just sets the entire register to "VAL". In "MODIFY_REG" the macro "WRITE_REG" is used as well to set the complete register to the value "SETMASK". This macro is an absolute catastrophe and is used in many places throughout HAL. Why? Because, regardless of the value for "CLEARMASK", the register is always set to "REG | SETMASK".

I am sorry, but how has this macro passed any kind of code review?

It cost us several days to track this bug down. I hope this will be fixed in future versions of CubeMX and the HAL Library.

[Pavel A.]

> regardless of the value for "CLEARMASK", the register is always set to "REG | SETMASK".

In the HAL code, in place of REG parameter are L-values (usually struct->member) so reference to REG expands to reading the memory location. Unless in your codebase "REG" is something like a number (address of register...) it should work as expected.

Regards,

-- pa

[MFreu]

Thank you for your reply. I think I haven't explained very well in my first post, sorry about that.

Please see how these macros are used in HAL to set the register.

The following is an excerpt from the function

"HAL_StatusTypeDef HAL_RCCEx_PeriphCLKConfig( RCC_PeriphCLKInitTypeDef* PeriphClkInit )" in "stm32f7xx_hal_rcc_ex.c".

  /*------------------------------------ SAI2 configuration --------------------------------------*/
  if ( ( ( PeriphClkInit->PeriphClockSelection ) & RCC_PERIPHCLK_SAI2 ) == ( RCC_PERIPHCLK_SAI2 ) )
  {
    /* Check the parameters */
    assert_param( IS_RCC_SAI2CLKSOURCE( PeriphClkInit->Sai2ClockSelection ) );
 
    /* Configure SAI2 Clock source */
    __HAL_RCC_SAI2_CONFIG( PeriphClkInit->Sai2ClockSelection );
 
    /* Enable the PLLI2S when it's used as clock source for SAI */
    if ( PeriphClkInit->Sai2ClockSelection == RCC_SAI2CLKSOURCE_PLLI2S )
    {
      plli2sused = 1;
    }
    /* Enable the PLLSAI when it's used as clock source for SAI */
    if ( PeriphClkInit->Sai2ClockSelection == RCC_SAI2CLKSOURCE_PLLSAI )
    {
      pllsaiused = 1;
    }
  }

__HAL_RCC_SAI2_CONFIG expands like so:

MODIFY_REG(*(0x4002388C), 0x00C00000, (uint32_t)PeriphClkInit->Sai2ClockSelection);
WRITE_REG((*(0x4002388C)), (((READ_REG(*(0x4002388C))) & (~(0x00C00000))) | ((uint32_t)PeriphClkInit->Sai2ClockSelection)));
WRITE_REG((*(0x4002388C)), (((*(0x4002388C)) & (~(0x00C00000))) | ((uint32_t)PeriphClkInit->Sai2ClockSelection)));
(*(0x4002388C) = (((*(0x4002388C)) & (~(0x00C00000))) | ((uint32_t)PeriphClkInit->Sai2ClockSelection));

simplified for easier human parsing:

(*0x4002388C) = ((*0x4002388C) & (~0x00C00000)) | PeriphClkInit->Sai2ClockSelection;

As you can see, PeriphClkInit->Sai2ClockSelection is not masked and since this variable is stack allocated, and in this case not initialized, the behaviour is undefined.

Although the macro itself is valid, it is also very non-defensive. It can have serious side effects. Improving this macro would at least mitigate the original bug in the HAL init code where the SAI2 clock is being configured to uninitialized values, thus affecting bits in this register NOT related to SAI2.

[Pavel A.]

​In this part you are right, using values of uninitialized variables is evil.

But complicating the macro IMHO is a wrong thing. Just fix the initialization and using wrong bits issues. 

-- pa

[Piranha]

The purpose of CLEARMASK is not for "limiting input", but for clearing previous values of a group of bits because OR operation can't clear them.

By the way, I do understand the purpose of MODIFY_REG, POSITION_VAL and well.. even CLEAR_BIT. But what is the purpose of the rest of those trivial macros? To optionally replace them with bit-banding? And even that wouldn't explain the _REG ones (except for MODIFY_REG). Other ideas?

[Pavel A.]

>  But what is the purpose of the rest of those trivial macros? To optionally replace them with bit-banding? 

Maybe. Once I've dealt with a validation tool and had to record any write to registers. For this, ability to redefine (hook) any accessor operation is useful.

-- pa

[Amel NASRI]

Hello @MFreu​ ,

I would like to come back to the initial issue with the wrong CubeMX generated code.

Could you please share the .ioc file that allows to reproduce the issue.

Enabling both USART1 and SAI1 isn't enough to have RCC_PERIPHCLK_SAI2 parameter inserted in PeriphClockSelection.

Thanks

-Amel

[MFreu]

Thank you for your answer, I have new information learned today:

The Cube file was indeed corrupted somehow. I opened the file in a text editor and could find some mentions of the SAI2 peripheral, however without the associated parameters.

Perhaps it happened in the early phases of the project:

During the development of the project we played around in CubeMX to see how we can get all of our needed functions assigned to the MCU. When board layout began, the pinout was fixed and we used this file to create our initial codebase. Now, apparently something happened during the setup of this file that caused some kind of corruption. Unfortunately, it is very hard for me to exactly reproduce the steps that happened, since at this point the project was moving very fast and was not under version control yet. The Cube file was migrated from older versions of Cube at least once, and it also certainly underwent a FW migration during development. Perhaps something happened to the file there?

I have recreated a cube project and transferred all parameters manually. If I generate code with this project, the init is fine and the Cube file also does not contain parts of the SAI2 parameters.

I have to check whether I can upload the Cube file.

[Amel NASRI]

Hello @MFreu​ ,

Thanks for taking the time to share with us these updates.

Should we consider that the problem is resolved?

Thanks,

Amel