Improvement on new interval system Topic is solved

[Thargon]

Hi,

I just had a closer look at the new interval system of the 18.2 release. I can not see the reason, why CH_CFG_TIME_TYPES_SIZE is limited to 16 and 32 bit size, though. Of course, there is the risk of overflows in the conversion functions so that 'time_conv_t' has always double the size than the other types, but is is implemented in a very pessimistic way. I would like to see the possibility to set CH_CFG_TIME_TYPES_SIZE to 64, as there is plenty of headroom (depending on other settings) before overflows may occur.

The worst case appears to be in the US2I conversion functions:

Code: Select all

#define TIME_US2I(usecs)                                                    \
  ((sysinterval_t)((((time_conv_t)(usecs) *                                 \
                     (time_conv_t)CH_CFG_ST_FREQUENCY) +                    \
                    (time_conv_t)999999) / (time_conv_t)1000000))


The critical section here is

Code: Select all

(((time_conv_t)(usecs) * (time_conv_t)CH_CFG_ST_FREQUENCY) + (time_conv_t)999999)


as the result of this calculation must fit into 'time_conv_t'.

Consequently, the maximum value of 'usecs' can be calculated as

Code: Select all

usecs <= ((time_conv_t)-1 - 999999) / (time_conv_t)CH_CFG_ST_FREQUENCY


Now let's assume 64 bit width of 'time_conv_t' and a very high setting of CH_CFG_ST_FREQUENCY:

Code: Select all

chconf.h

// 1us resolution
#define CH_CFG_ST_FREQUENCY 1000000


This results in

Code: Select all

usecs <= ((time_conv_t)-1 - 999999) / (time_conv_t)CH_CFG_ST_FREQUENCY
       = (2^64-1 - 999999) / 1000000
       = 1.84E13


As you can see the result is much bigger than 2^32 (4.29E9) and would be even higher for smaller CH_CFG_ST_FREQUENCY.

I would propose a solution where CH_CFG_TIME_TYPES_SIZE can be set to 64, but the secure time conversion functions check the argument for this maximum value, i.e.

Code: Select all

chDbgAssert(usecs <= ((time_conv_t)-1 - (time_conv_t)999999) / (time_conv_t)CH_CFG_ST_FREQUENCY);


Best regards,
Thomas

[Giovanni]

Hi,

The time_conv_t type is meant to avoid those overflows this is why uint64_t is not supported for time, it would require a 128bits type for time_conv_t.

Giovanni

[Thargon]

Hi,

in general you are completely right and I support your opinion. But in practice we are on a scale where overflows will only occur in rare cases as I mentioned at the end of my post. As I showed, given a 64 bit wide time_conv_t and a CH_CFG_ST_FREQUENCY of 1 MHz, you can still safely calculate timeframes of up to 213.5 days! If you reduce the system tick frequency, this number increases, of course.

However, since the sysinterval_t can not be wider than 32 bit, the maximum timeframe that can be represented (given the 1 MHz frequency) is merely 0.05 days (71.4 minutes), which is quite a waste, imo.

In order to prevent overflows as you mentioned, I already proposed a solution in my original post, by adding an additional assert to the convertion functions. You could just check if the width of sysinterval_t is greater or equal to the width of time_conv_t to enable this assert.

There is nothing that will make ChibiOS unsafe or whatsoever, but this feature would further increase system flexibility.

Thomas