HAL_Delay adds 1ms extra (1.4.0 version)

[sbp-dev]

I programmed a stepper motor controller with step interval 1ms using HAL_Delay(1). After updating my CubeMX library few days back, I realized every motion is taking almost twice as long. Upon inspecting, I found this in STM32CubeL1/Drivers/STM32L1xx_HAL_Driver/Src/stm32l1xx_hal.c

__weak void HAL_Delay(uint32_t Delay)
{
  uint32_t tickstart = HAL_GetTick();
  uint32_t wait = Delay;

  /* Add a period to guaranty minimum wait */
  if (wait < HAL_MAX_DELAY)
  {
    wait += (uint32_t)(uwTickFreq);
  }

  while((HAL_GetTick() - tickstart) < wait)
  {
  }
}

Any uint32_t value will be < HAL_MAX_DELAY (=2^32-1) so the if condition will always be true. So essentially, we are unconditionally adding uwTickFreq (=1 for default clock settings) to any Delay value. This baffles me! My idea of "guaranty minimum wait" would be to check explicitly for Delay==0 and add 1 if true, something like:

   /* Add a period to guaranty minimum wait */
  if (Delay == 0)
      Delay =1;
  while((HAL_GetTick() - tickstart) < Delay)
  {
  }

The present implementation unfortunately, is unfaithful to what the API is intuitively meant to do. Please clarify if I've missed something. Thanks.

[jrahlf]

You cannot do that due to the quantization of the underlying time. Imagine that you call HAL_Delay(1) and the current physical time is 0.99. Then it will stop spinning after 0.01ms, because tickstart gets initialized as 0 (due to integer quantization). Also, why would you add an extra delay of 1ms for the special case of 0, that is the only case where you don't need to add an extra delay, lol.

What you likely need is a microscond delay function, which unfortunately ST does not want to add (already asked for it here)

[sbp-dev]

@jrahlf , I know what you mean. On one hand we have a possibility of smaller than required delay (jitter), on the other hand we are guaranteeing that the delay produced is always larger than what is required. I don't think the latter is justified, especially since its not at all obvious to application developers without digging into the code. The jitter issue is not really addressed either, we're just adding an offset of 1 to it!

Also, why would you add an extra delay of 1ms for the special case of 0

I wouldn't really :) In fact that it how the implementation was till v1.3.0. I was just trying to suggest a alternate that doesn't affect every other delay value. My interpretation of "Add a period to guaranty minimum wait" was to make sure that whenever HAL_Delay is called, ensure a minimum wait is provided.

[atsju]

I agree with explanation of @jrahlf. If you are bothered about waiting 1ms too long you should use microsecond delay. I would add that making the change you request could break many applications for current users that know (and use the fact) that delay is minimum meaning 1ms request is between 1.000ms and 1.999ms. Maybe some documentation could be added by ST but how the code behaves shall not be modified. There is no "correct" way of handling the rounding so here the best option is just to keep history.

[ASELSTM]

Hi @sbp-dev,

Thank you for your contribution. This has been communicated to the development team. We will come back to you as soon as we get their feedback.

Thank you again for your contribution.

With regards,

[ASELSTM]

ST Internal Reference: 68380

[sbp-dev]

@atsju

There is no "correct" way of handling the rounding so here the best option is just to keep history.

I agree with the first part, its a matter of preference. You maybe okay with HAL_Delay(x) providing a delay of <=x+1, but I prefer it to be <=x since that is how the default delay APIs work for all the other platforms that I've worked with, including STM32Cube, till v1.3. Regarding keeping it history, I disagree. Of course there is nothing I can do if this issue is not taken up, I'd like to make my case here nonetheless :)

I would add that making the change you request could break many applications for current users

What about all the applications like mine that broke after updating the Cube library? If you agree that codes can break due to my request for reversal, you must also concede that codes were broken when this change was introduced in the first place. But if you propose that someone using HAL_Delay shouldn't bother about 1ms extra (they should be using microsecond delays in such cases), then I'd argue that they shouldn't be bothered about 1ms less either, so why do you think such codes will break?

On another note, imagine if I want to set UART baud rate to 9600, but my controller API implements 9601 or 9599 under the hood. For all practical purposes it'll work just fine, but is it faithful? There will always be sub millisecond jitters in all three cases, and adding the offsets doesn't really solve it so why not let it be as it is!

[atsju]

What about all the applications like mine that broke after updating the Cube library ?

I agree this one is a valid point, I didn't knew that behavior already changed. This is exactly what should never happen (of course there are exceptions but here I think it shouldn't have changed). And as I said it's not your solution that is good or bad but the fact of changing behavior from something unprecise to an other (exact same) unprecise solution.

I gave my point of view and I know you have some solid points. My purpose was not to start an argument just to ensure that ST support has a feedback for customer with different usage and opinion. It's even possible that the last change (breaking your code) was done after a customer request with single point of view.

[ASELSTM]

Hi @sbp-dev,

Would you please try to test with the latest version of the STM32CubeL1.

With regards,

[ASELSTM]

Hi,

Please allow me to close this thread. If you have any updates you can reopen it at any time. Thank you for your comprehension.

With regards,