12 Replies Latest reply on Jun 29, 2017 5:51 PM by Dion Damato

    HAL geniuses, how does this work?

    Clive One

      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

        • Re: HAL geniuses, how does this work?
          Clive One

          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 !!!!!!

          1 of 1 people found this helpful
            • Re: HAL geniuses, how does this work?
              Zhitai Liu

              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()??

                • Re: HAL geniuses, how does this work?
                  Clive One

                  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.

                    • Re: HAL geniuses, how does this work?
                      AvaTar

                      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 ...

                • Re: HAL geniuses, how does this work?
                  Clive One

                  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 21.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 4294.967 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.

                  • Re: HAL geniuses, how does this work?
                    Clive One

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

                    Woman told $43million slot machine win was a glitch sues | Daily Mail Online 

                    • Re: HAL geniuses, how does this work?
                      Oliver Sedlacek

                      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.

                      • Re: HAL geniuses, how does this work?
                        Amel N

                        Hi Clive One,

                         

                        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

                        3 of 3 people found this helpful