There is a potential issue when xTimeInMs is a very large value, and configTICK_RATE_HZ is a maximum of 1000.
The result of xTimeInMs * configTICK_RATE_HZ may overflow, since both are type-casted to TickType_t, and the final result of pdMS_TO_TICKS() will be wrong.
My fix in which my TickType_t is unsigned long is as below:
But there is a downside: some platforms do not implement 64-bit calculations or at a high cost. On a 8-bit MCU, a 64-bit calculation can be very expensive. In other situations, people don’t like to link in a 64-bits library to safe space in flash.
When this is not a problem then go ahead.
Another problem that has been mentioned with pdMS_TO_TICKS is that it may return 0. When you want a task to sleep (block), you might want a minimum of 2 clock ticks, to get at least a single clock tick.
I agree with Hein that we cannot, by default, use 64-bit calculations. Perhaps the best thing would be to leave the default calculation as it is, but then to allow the macro to be overridden in FreeRTOSConfig.h? So the definition becomes:
The user could then define it to whatever they like.
Also remember that the macro is not used by the FreeRTOS code, it is only there for the convenience of application writers, who might choose not to use it at all. It is also used a lot in the demo code.
One modification that I would also suggest is you should round UP the result, not down as default in C math. You can do this by adding 999 before dividing by 1000. That way a 1 ms delay on a 100 Hz tick system gives you a delay of 1 tick, not 0.
There also is the issue that many miss that an 1 tick delay is a delay to the next tick, which might happen almost immediately. If you really need a delay for at least a specified period, you need to add 1 to the number of tick to make sure you delay long enough. A delay of 1 give no minimum delay and is more like a yield, except it lets lower priority tasks execute also.
Just ran into the overflow problem on this macro, as I’m using just 16-bit ticks, needing only a 10 second (10,000 tick) horizon for everything except the hourmeter:
The problem with re-writing it really boils down to implementation details: If I use a constant argument that is computed at compile time, or even just once at startup, it doesn’t matter whether 64 bit types are implemented on the target or not. Modern, fast targets wouldn’t care if it was a run-time function, but I’m really not at all CPU-power conscious.
The “proper” size of the arguments in the calculation (defined by expectations as a naive user) would need to be sizeof(argument)+sizeof(TickType_t), which we can conservatively decide should be
2 * sizeof(TickType_t). Good luck properly expressing that in the C programming language, especially in a portable, simple way – you’d need some decent typedef magic.
A warning in the examples is probably best, as if the definition had been partially “fixed”,say, with uint32 casts, it just hides the problem until much later.
For myself, I’m modifying projdefs.h and putting my definition (with a warning) in FreeRTOSConfig.h.
You should not need to edit any of the core FreeRTOS source files, just define pdMS_TO_TICKS() however you want in FreeRTOSConfig.h. The default definition appears as below, so will only be used if pdMS_TO_TICKS is not defined elsewhere.