Optimizer problem with USBPD

[RomThi]

Hi,

I have an USBPD sink application created with CubeMX that can be found here:
https://github.com/ThiRom/H563NucleoUSBPDSink

 

The project can be build using cmake. If the project is build using Debug configuration, it works. But using Release it shows timeouts on requests. As you can see below during one millisecond the firmware sends 3 retries and a soft reset.  



This problem can be seen with -O2 and -O3 option. It seems that the file usbpd_timersserver.c is not working correctly. I found that function:

uint32_t USBPD_TIM_IsExpired(TIM_identifier Id)
{
 uint32_t _expired = 1u;
 switch (Id)
 {
 case TIM_PORT0_CRC:
 _expired = TIMX_CHANNEL1_GETFLAG(TIMX);
 break;
 case TIM_PORT0_RETRY:
 _expired = TIMX_CHANNEL2_GETFLAG(TIMX);
 break;
 case TIM_PORT1_CRC:
 _expired = TIMX_CHANNEL3_GETFLAG(TIMX);
 break;
 case TIM_PORT1_RETRY:
 _expired = TIMX_CHANNEL4_GETFLAG(TIMX);
 break;
 default:
 break;
 }
 return _expired;
}

 When I change it to this:

uint32_t USBPD_TIM_IsExpired(TIM_identifier Id)
{
 uint32_t _expired = 1u;
 switch (Id)
 {
 case TIM_PORT0_CRC:
 _expired = TIMX_CHANNEL1_GETFLAG(TIMX);
 break;
 case TIM_PORT0_RETRY:
 _expired = 0;
 break;
 case TIM_PORT1_CRC:
 _expired = TIMX_CHANNEL3_GETFLAG(TIMX);
 break;
 case TIM_PORT1_RETRY:
 _expired = TIMX_CHANNEL4_GETFLAG(TIMX);
 break;
 default:
 break;
 }
 return _expired;
}

It works.

Any idea how to fix it correctly?

Best regards

Roman

 

[FBL]

Hi @RomThi 

I think the issue is how the return value _expired is used and how the optimizer sees it, especially inside tight loops. _expired should be either 0 or 1. So I propose the following, 

case TIM_PORT0_RETRY:
 _expired = (TIMX_CHANNEL2_GETFLAG(TIMX) != 0u) ? 1u : 0u;
 break;

_expired initialized to 0 instead of 1 (default should be not expired). Let me know if this does help. 

[RomThi]

Hi,

thx for your fast response. I have tried what you send without any change.

I also tried this:

 case TIM_PORT0_RETRY:
 return ((READ_BIT(TIMx->SR, TIM_SR_CC1IF) == (TIM_SR_CC1IF)) ? 1UL : 0UL);

Still no change.

Best regards,

Roman

[FBL]

@RomThi Still waiting for dev team feedback, 

You can instrument USBPD_TIM_IsExpired as noinline or lowering optimization on this function as a safety measure

__attribute__((noinline, optimize("O0")))
uint32_t USBPD_TIM_IsExpired(TIM_identifier Id)

 Which compiler are you using? The issue is being tracked internally under CDM0060354 for internal reference only.

[gbm]

@FBL Really?...

how about

bool expired = TIMX_CHANNEL2_GETFLAG(TIMX);

 I think it's the high time for ST folks to learn about boolean type finally. ;)

And even without boolean, the expression would be:

expired = READ_BIT(TIMx->SR, TIM_SR_CC1IF) == (TIM_SR_CC1IF);

No ternary conditional is need to convert 0 to 0 or 1 to 1. ;)

[FBL]

Hi  @gbm 

You’re absolutely right that using a boolean is clearer here, and that the comparison already produces a 0/1 semantics without needing a ternary.

My earlier suggestion was trying to make the 0/1 normalization explicit based on the existing uint32_t API and to emphasize that we want strict boolean behavior instead of propagating raw bitmasks.

Thank you for the heads up ! ;)