Skip to main content
Tesla DeLorean
Guru
June 16, 2017
Solved

HAL geniuses, how does this work?

  • June 16, 2017
  • 12 replies
  • 2601 views
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

    This topic has been closed for replies.
    Best answer by Amel NASRI
    Posted on June 23, 2017 at 12:09

    Hi

    ,

    We deeply reviewed all your comments and here our feedback:

    First to precise: the points you raised in this discussion are valid for F412G demo, some BSP examples and some applications based on the usage of the Touch screen calibration module.

    They are not valid for the Cube HAL and Middleware drivers.

    We recognize that the following statement should not be used:

    uint32_t HAL_GetTick(void){ return (DWT->CYCCNT / (HAL_RCC_GetHCLKFreq() / 1000)) ;}�?�?�?�?�?�?�?�?

    In coming Cube firmware package versions, this implementation will be replaced by the usage of a time base based on the TIMERs or the native time base GetTick() (declared as a weak function in the hal.c file). Your proposal to use the following implementation is correct :

    uint32_t HAL_GetTick(void){ return(DWT->CYCCNT);}�?�?�?�?�?�?�?�?

    But this is not recommended as there is a high risk of multi rollover condition as the DWT->CYCCNT overflow occurs every almost 86 seconds at 50MHz.

    As DWT is not available on M0/M0+ cores, it is decided to remove the implementation of GetTick based on the DWT and replace it by TIMER or native time base as previously said in the next releases of the demos.

    The default HAL_Delay implementation used for the Cube today is the following :

    __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) { }}�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?

    So the minimum 1 ms is ensured.

    All the timeout and delay used in the Cube drivers are based on the HAL_GetTick() using the following comparison:

    HAL_GetTick() - tickstart) < __TIME__�?

    The GetTick() is always returning the uwTick (uint32_t) which is incremented by a Time base ISR occurring each 1ms, and this is completely valid for 32-bit number space.

    However, the following statement is used in a special case (touch screen calibration after the Reset, so

    the TimeStart is often 0 or 1 ms

    :(

    if ((HAL_GetTick() - 100) > TimeStart)

    Even if there is no issue in this particular case, I agree with you that this has to be updated to avoid confusion.

    Please note also that such statement is not used by the HAL drivers neither the applications. Finally, I would like to highlight that the time-base functions implemented in the Cube drivers are all weak, so you have always the possibility to over-write them when needed.

    Hope this reply covers all the technical discussed points, and thanks for your insights.

    -Amel

    12 replies

    Tesla DeLorean
    Guru
    June 16, 2017
    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 !!!!!!

    Tips, Buy me a coffee, or three.. PayPal Venmo (See Profile) Up vote any posts that you find helpful, it shows what's working..
    Zt Liu
    Senior III
    June 17, 2017
    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()??

    Tesla DeLorean
    Guru
    June 17, 2017
    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.

    Tips, Buy me a coffee, or three.. PayPal Venmo (See Profile) Up vote any posts that you find helpful, it shows what's working..
    Tesla DeLorean
    Guru
    June 17, 2017
    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.

    Tips, Buy me a coffee, or three.. PayPal Venmo (See Profile) Up vote any posts that you find helpful, it shows what's working..
    S.Ma
    Principal
    June 17, 2017
    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
    Guru
    June 17, 2017
    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.

    Tips, Buy me a coffee, or three.. PayPal Venmo (See Profile) Up vote any posts that you find helpful, it shows what's working..
    Tesla DeLorean
    Guru
    June 17, 2017
    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

     
    Tips, Buy me a coffee, or three.. PayPal Venmo (See Profile) Up vote any posts that you find helpful, it shows what's working..
    Dvorak.Peter
    Associate II
    June 18, 2017
    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

    Oliver Sedlacek
    Senior
    June 22, 2017
    Posted on June 22, 2017 at 11:59

    When something is really badly broken it becomes quite hard to even have a meaningful discussion about it. In my experience all time representations must be signed to make time calculations work at all.

    Amel NASRI
    Amel NASRIBest answer
    ST Technical Moderator
    June 23, 2017
    Posted on June 23, 2017 at 12:09

    Hi

    ,

    We deeply reviewed all your comments and here our feedback:

    First to precise: the points you raised in this discussion are valid for F412G demo, some BSP examples and some applications based on the usage of the Touch screen calibration module.

    They are not valid for the Cube HAL and Middleware drivers.

    We recognize that the following statement should not be used:

    uint32_t HAL_GetTick(void){ return (DWT->CYCCNT / (HAL_RCC_GetHCLKFreq() / 1000)) ;}�?�?�?�?�?�?�?�?

    In coming Cube firmware package versions, this implementation will be replaced by the usage of a time base based on the TIMERs or the native time base GetTick() (declared as a weak function in the hal.c file). Your proposal to use the following implementation is correct :

    uint32_t HAL_GetTick(void){ return(DWT->CYCCNT);}�?�?�?�?�?�?�?�?

    But this is not recommended as there is a high risk of multi rollover condition as the DWT->CYCCNT overflow occurs every almost 86 seconds at 50MHz.

    As DWT is not available on M0/M0+ cores, it is decided to remove the implementation of GetTick based on the DWT and replace it by TIMER or native time base as previously said in the next releases of the demos.

    The default HAL_Delay implementation used for the Cube today is the following :

    __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) { }}�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?

    So the minimum 1 ms is ensured.

    All the timeout and delay used in the Cube drivers are based on the HAL_GetTick() using the following comparison:

    HAL_GetTick() - tickstart) < __TIME__�?

    The GetTick() is always returning the uwTick (uint32_t) which is incremented by a Time base ISR occurring each 1ms, and this is completely valid for 32-bit number space.

    However, the following statement is used in a special case (touch screen calibration after the Reset, so

    the TimeStart is often 0 or 1 ms

    :(

    if ((HAL_GetTick() - 100) > TimeStart)

    Even if there is no issue in this particular case, I agree with you that this has to be updated to avoid confusion.

    Please note also that such statement is not used by the HAL drivers neither the applications. Finally, I would like to highlight that the time-base functions implemented in the Cube drivers are all weak, so you have always the possibility to over-write them when needed.

    Hope this reply covers all the technical discussed points, and thanks for your insights.

    -Amel

    To give better visibility on the answered topics, please click on "Best Answer" on the reply which solved your issue or answered your question.
    dcode31
    Associate
    June 29, 2017
    Posted on June 29, 2017 at 17:39

    st.mcu

    ‌ good to know that ST is thoughtfully developing the HAL.