HAL geniuses, how does this work?

[Tesla DeLorean]

Posted on June 16, 2017 at 23:56

From the STM32F412G-DISCO demo

uint32_t HAL_GetTick(void)

{ return (DWT->CYCCNT / (HAL_RCC_GetHCLKFreq() / 1000)) ;}

How exactly does this wrap properly within the 32-bit number space? Answer: IT DOESN'T

[Tesla DeLorean]

Posted on June 17, 2017 at 00:13

Ok, so how is it that as a casual user of the HAL I can see the horrific consequences of this code in a blink of an eye?

Observe the delta in

ABS( 

(0x00000000 

/ (HAL_RCC_GetHCLKFreq() / 1000)) - (0xFFFFFFFF 

/ (HAL_RCC_GetHCLKFreq() / 1000))  )

CLUE: It is a number significantly larger than ONE

This will break code like this, which is completely valid for 32-bit number space,

__weak void HAL_Delay(__IO uint32_t Delay)

{ uint32_t tickstart = HAL_GetTick(); uint32_t wait = Delay; /* Add a period to guarantee minimum wait */ if (wait < HAL_MAX_DELAY) { wait++; } while((HAL_GetTick() - tickstart) < wait) { }}

and the thousands of other timeout use cases in the entire HAL

Then we have other horrific code like this:

/* Got awaited press state */

/* Record in 'TimeStart' the time of awaited touch event for anti-rebound calculation */ /* The state should persist for a minimum sufficient time */ TimeStart = HAL_GetTick();

/* Is state of the touch changing ? */

/* Second level while loop entry */ do { /* New sense of touch state from touch IC : to evaluate if state was stable */ status = BSP_TS_GetState(&TS_State); if(status == TS_OK) { /* Is there a state change compared since having found the awaited state ? */ if (((Pressed == 0) && ((TS_State.touchDetected == 1) || (TS_State.touchDetected == 2))) || ((Pressed == 1) && ((TS_State.touchDetected == 0)))) { /* Too rapid state change => anti-rebound management : restart first touch search */ exitSecondLevelWhileLoopReq = 1; /* exit request from second level while loop */ } else if ((HAL_GetTick() - 100) > TimeStart) { /* State have not changed for the timeout duration (stable touch for 100 ms) */ /* This means the touch state is stable : can exit function */

Again, someone really doesn't know how to use HAL_GetTick, or basic 32-bit unsigned number space math.

if ((HAL_GetTick() - 100) > TimeStart) // NO NO NO NO NO !!!!!!

[Tesla DeLorean]

Posted on June 17, 2017 at 03:46

This is a more robust solution, one where you don't call functions repeatedly and do division, will all help significantly

For Core Cycle Counter
uint32_t HAL_GetTick(void)
{
 return(DWT->CYCCNT);
}
__weak void HAL_Delay(uint32_t Delay) // In milliseconds, @ 200 MHz stay below 474 seconds
{
 uint32_t tickstart = HAL_GetTick();
 uint32_t wait = Delay * (HAL_RCC_GetHCLKFreq() / 1000); // ms to core ticks
 while((HAL_GetTick() - tickstart) < wait);
}

For 32-bit TIM2 clocking at 1 MHz
uint32_t HAL_GetTick(void)
{
 return(TIM2->CNT);
}
__weak void HAL_Delay(uint32_t Delay) // In milliseconds, @ 1MHz stay below 4967 seconds
{
 uint32_t tickstart = HAL_GetTick();
 uint32_t wait = Delay * 1000; // ms to microseconds
 while((HAL_GetTick() - tickstart) < wait);
}
�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?

And for the other Timeout cases, you should provide a SCALING macro that converts for millisecond units to the units of the Ticker.

[Zt Liu]

Posted on June 17, 2017 at 05:14

This is certainly not just a sleep of pen!!

But I don't understand why they don't want to use uwTick,

as it's already incremented every time in the SysTick_Handler()??

[S.Ma]

Posted on June 17, 2017 at 16:27

As a general practice, divide last, and name the variables with their physical unit and if necessary their magnifying number.

Then check for the min/max values to make sure there is no overflow.

Example : count_lsb or delay_ms_x1000 ==> this is a way to keep some precision in further post calculation...

[Tesla DeLorean]

Posted on June 17, 2017 at 19:17

Here is a real world example of a woman winning $FFFFFFEC cents

http://www.dailymail.co.uk/news/article-4612826/Woman-told-43million-slot-machine-win-glitch-sues.html

 

[Tesla DeLorean]

Posted on June 17, 2017 at 19:12

Yes, but what to interrupts rely on? And how do wait loops work when a higher priority interrupt is sitting in them?

There is no real control here on what functions are thread/interrupt safe, where timeouts become infinite, etc., whether locks and loops are safe in callbacks.  A hardware count is at least immune to that.

The HAL is very ambitious, it's flawed by the number of unqualified people working on it, and the acceptance of that code by people who should know better.

The problem with example code, and that emitted by CubeMX, is that it will get into everything, and result in huge amounts of latent failure.

[Tesla DeLorean]

Posted on June 17, 2017 at 19:15

Frequency here has the benefit of almost always being MHz. I'd rather compiler compute constants be used here than use 64-bit math.

[Dvorak.Peter]

Posted on June 18, 2017 at 20:41

/external-link.jspa?url=http%3A%2F%2Fwww.dailymail.co.uk%2Fnews%2Farticle-4612826%2FWoman-told-43million-slot-machine-win-glitch-sues.html

Could have been worse.

She might have 'won' -$5 million.

Peter

[AvaTar]

Posted on June 19, 2017 at 11:26

The HAL is very ambitious, it's flawed by the number of unqualified people working on it, and the acceptance of that code by people who should know better.

Hmmmmm, at least in theory.

But in my experience, very few good developers want a management job (project management) which involves making such decisions.

Most project management people I met were more driven by ambition, and thus preferred unquestioning greenhorns over dissenting professionals ...