SoftwareTimer/ms2tick overflow

[thinkOfaNumber]

Operating System

Other (FreeRTOS + arduino)

IDE version

VS Code 1.62.3

Board

custom RAK4630 based on adafruit_feather_nrf52840

BSP version

1.2.0

Sketch

Using ms2tick for ms values over 4,194,303 causes the implementation to overflow, e.g. uint32_t ticks = ms2tick(3 * 60 * 60 * 1000); // 3 hours

The default implementation of ms2tick uses the macro pdMS_TO_TICKS which is defined in projdefs.h as:

#ifndef pdMS_TO_TICKS
    #define pdMS_TO_TICKS( xTimeInMs ) ( ( TickType_t ) ( ( ( TickType_t ) ( xTimeInMs ) * ( TickType_t ) configTICK_RATE_HZ ) / ( TickType_t ) 1000 ) )
#endif

This affects SoftwareTimer::setPeriod which uses xTimerChangePeriod(_handle, ms2tick(ms), 0); and probably other places too.

What happened ?

(3 60 60 * 1000) is multiplied by 1000 which rolls over 32 bits, causing the resulting ticks to be calculated incorrectly: 2,469,265 ticks (or about 40 minutes) instead of 11,059,200 ticks. In any case, it would be good to be able to use long timers for daily/weekly tasks (32 bit ticks should get us approx 48 days if I calculate correctly...)

How to reproduce ?

pretty simple to reproduce: uint32_t ticks = ms2tick(3 * 60 * 60 * 1000); // 3 hours

Or, use software timers and wait for, say, 3 hours.

It would be possible to work around this by defining pdMS_TO_TICKS using uint64_t, however this may not be desirable in all cases as 64bit operations may be slow and unnecessary for most instances of ms2tick:

define pdMS_TO_TICKS( xTimeInMs ) ( ( TickType_t ) ( ( ( TickType_t ) ( xTimeInMs ) * ( uint64_t ) configTICK_RATE_HZ ) / ( TickType_t ) 1000 ) )

It may be better just to provide an implementation of SoftwareTimer that uses long calculations. Not sure what's best here.

Debug Log

No response

Screenshots

No response

[hathach]

Indeed, the TickType_t is defined as 32-bit, this is not a bug, just way this core is ported with freeRTOS. Could you tell us more about your usage. For now we don't have any motivation to change it to 64-bit tick type. You should either break the sleep into short cycle e.g 2 hours instead and has your own 64-bit tick variable in software side.

[thinkOfaNumber]

I agree I don't think changing the tick type to 64 bit is a good idea, but the implementation of pdMS_TO_TICKS doesn't allow the full usage of 32 bit ticks due to the rollover in xTimeInMs * configTICK_RATE_HZ.

Our use case is that we have some real-time measurement tasks that need to be accurate to less than 1ms, but they only need to run once per hour, few hours, day, week, etc. This is to synchronize with other devices using GPS without having any inter-device communication.

I've worked around it for now by using my own calculation, almost identical to the provided pdMS_TO_TICKS. Basically: ms * 1024ULL / 1000UL (note the ULL configTICK_RATE_HZ is enough to cast the intermediate multiplication to a long long preserving the full 32 bits of the resulting TickType_t).

Hope that makes sense, thanks for replying!

[hathach]

I've worked around it for now by using my own calculation, almost identical to the provided pdMS_TO_TICKS. Basically: ms * 1024ULL / 1000UL (note the ULL configTICK_RATE_HZ is enough to cast the intermediate multiplication to a long long preserving the full 32 bits of the resulting TickType_t).

ah I see, casting uint64_t to the intermediate product in pdMS_TO_TICKS. Since you are already testing it in your project. Would you mind submitting an PR to correct this ?

[thinkOfaNumber]

No problem, if you're satisfied a 64bit calculation here is ok by default...

[hathach]

No problem, if you're satisfied a 64bit calculation here is ok by default...

Yup, I am ok with 64-bit cal, it shouldn't be an issue. I don't think it would cause any noticeable impact to the system