Discussion on PolledTimeout

[d-a-v]

Following

1. 9a2ed27#r33052405 and
2. 9a2ed27#r33052437

Regarding 2:

(@TD-er)
I guess we have to try then what the compiler makes of it.
As far as I know (current - _start) will give a high value if _start > current.
So then the division should also be a high number.
If the compiler should make it a signed integer and then casting it to uint, then we have other issues.

I made some test with gcc and it seems OK.
However, I totally agree it is not clear enough.
To ensure we stay within (unsigned) timeType bounds (with implicit modulo over range), I would propose to change

unsigned long n = (current - _start) / _timeout

to

unsigned long n = ((timeType)(current - _start)) / _timeout

Also, regarding 1)

return (!_neverExpires) && ((internalUnit - _start) >= _timeout);

to

return (!_neverExpires) && (((timeType)(internalUnit - _start)) >= _timeout);

This is supposed to be already implicit but it becomes clear and explicit this way, what do you think ?

and finally to add somewhere (a proposal from @devyte)

static_assert(std::is_unsigned<timeType>::value == true);

[devyte]

Sorry for being away for so long, migrating to a new phone was rather painful.
Ok, i see that the discussion about the calculation has gone off the rails a bit. Let's see if I can unconfuse some of it.

First and foremost: all variables that hold time values are of type timeType, with the sole exception of "n", which is forcibly unsigned long. In the case of millis(), timeType is unsigned long, hence the same type.
When there is a minus operation, and the result would intuitively be negative, then with unsigned types what happens is an underflow (i.e.: rollaround), which means the result is always positive. It is precisely this that most users get wrong when trying to implement reinvent the calculation wheel.
So here:

unsigned long n = (current - _start) / _timeout; 

The minus op is always >=0 , then the division is always >=0, and n is always >=0.

The reason for the existence of "n" is to account for the case when polling skips over more than "timeout" time, in which case the _start has to be advanced a multiple of _timeout instead of just one _timeout.
Example: timeout is 10ms, say we poll about every 2ms
2 4 6 8 10 => timeout!
Abnormal case: say in between there's a long operation doing something:
2 4 6 8 .............................48 => timeout! Here we skipped over more than 10ms, hence n should be 4.

Notice that the calculation has zero drift in the time calculation, but allows for jitter in the poll moment, and also allows skipping multiples of timeout without winding. This is particularly important for periodic.

With normal use (construct, then test), the calculation is always correct if the time elapsed between checks is less than timsSource::timeMax (in truth it's less strict than that, but timeMax is defined the way it is because Reasons).
With manual use, i.e.: use of checkExpired(), the same holds true, but then the user is responsible of resetting. The only reason this api is exposed is the new mdns responder, which has certain quirks in the requirements, and I deemed it better to expose this api and have a single polledTimeout class vs. having two polledTimeout classes with different behavior.

About this:
`unsigned long n = ((timeType)(current - _start)) / _timeout``

current is of type timeType
_start is of type timeType
_timeout is of type timeType
=>
the minus op is also of type timeType, as is the result of the division. This means that casting within the expression doesn't make sense: it would case from timeType to timeType, which just reduces readability.

TBH, I don't remember why n is unsigned long. In theory, if timeType is constrained to unsigned types, then I guess n could also be of type timeType.

About this:
return (!_neverExpires) && ((internalUnit - _start) >= _timeout);

The same applies as above:
internalUnit is of type timeType
_start is of type timeType
_timeout is of type timeType
=>
the lhs minus op is of type timeType, and the >= comparison is always within the same types.
Therefore, casting doesn't make sense, because it casts from timeType to timeType.

About this:
static_assert(std::is_unsigned<timeType>::value == true);

The entire code assumes that the time() function in the TimeSource policy class returns an unsigned type. That assumption isn't enforced anywhere, and if it is not met, then the calculation code has to be quite different. This is why I proposed to enforce always meeting the assumption with the above assert, to make sure that the code won't compile if anyone ever implements a new TimeSource::time() that returns a signed type.

I'm closing this, because I don't see an issue, but I'll be happy to address further questions.

[d-a-v]

@devyte are the proposed type-cast relevant for reader's proper understanding ?
Are we reopening this issue to track the static_assert( pending addition ?

[devyte]

are the proposed type-cast relevant for reader's proper understanding ?

In general, casting should be done when there is a type change involved, to clarify in an explicit way what is being done, and i don't think that's the case here.
Also, i find the calculations to be easier to understand without the casts. I think this is a matter of opinion, though.

Are we reopening this issue to track the static_assert pending addition ?

You're right! I had thought to just use a pr, but this is where the discussion is. Reopening.