A simple timeout function

Question

I came across a timeout function in a product line code which get me really confused:

int TestTimeOut(unsigned long Timed_Val1, unsigned long Timed_Val2)
{
    Timed_Val2 = Timed_Val1 + (Timed_Val2 * 200);
    if (((Timed_Val1 > Timed_Val2) && (sys_msec < Timed_Val1) && (sys_msec > Timed_Val2)) || ((Timed_Val1 < Timed_Val2) && ((sys_msec < Timed_Val1) || (sys_msec > Timed_Val2))))
        return TRUE;
    return FALSE;
}

And here is how it's used:

unsigned long Timeout = sys_msec;
#define MAX_TIMEOUT   15L

    while (!com_eot(1))  //to check if some transmission in progress in COM1
        if (TestTimeOut(Timeout, MAX_TIMEOUT))
            return FALSE;
    return TRUE;

How does it work? I am totally confused by the 3 lines in TestTimeOut().

Answer 1

The reason for the somewhat complex check is because of the possibility of integer rollover. If that happens, then it needs the two parts of the check. A specific example might help. If, for example, a long is 32-bits on this system, and the initial value of Timed_Val1 is 2^32-100 = 4294967196, then Timed_Val2 would be computed as 2900. So it is that type of situation that requires this part of the check:

if (((Timed_Val1 > Timed_Val2) && (sys_msec < Timed_Val1) && (sys_msec > Timed_Val2)) || 

In that situation, the timeout occurs when sys_msec is between val1 and val2. It needs to be greater than 2900 and less than 4294967196.

The other half of the condition is the "normal" situation when there is no rollover in the computation of Timed_Val2:

((Timed_Val1 < Timed_Val2) && ((sys_msec < Timed_Val1) || (sys_msec > Timed_Val2))))

In that case, the timeout occurs when sys_msec is greater than val2 OR when it has rolled over and is, thus, less than val1.

The chosen variable names are definitely poor, though. It would make sense to rename them.

Answer 2

First I refactor out a and b local variables.

int TestTimeOut(unsigned long Timed_Val1, unsigned long Timed_Val2)
{
    Timed_Val2 = Timed_Val1 + (Timed_Val2 * 200);
    const int a = (Timed_Val1 > Timed_Val2) &&  (sys_msec < Timed_Val1) && (sys_msec > Timed_Val2);
    const int b = (Timed_Val1 < Timed_Val2) && ((sys_msec < Timed_Val1) || (sys_msec > Timed_Val2));
    return a || b;
}

Now this is interesting, Timed_Val2 is based on Timed_Val1, they are both unsigned so Timed_Val2 is always >= Timed_Val1. At first I could not see no way a can be true, but as Mark Wilkins points out, it could if it wraps around.

There's also only 1 case where they are equal however, that is when Timed_Val2==0 I'm going to extract that out as a special case to help readability. I can then factor out the two >/< statements into an if.

int TestTimeOut(unsigned long Timed_Val1, unsigned long Timed_Val2)
{
    if (Timed_Val2==0) return FALSE;

    {
      Timed_Val2 = Timed_Val1 + (Timed_Val2 * 200);

      if (Timed_Val1 > Timed_Val2)
      { //this happens when it wraps around past 2^32
        return (sys_msec < Timed_Val1) && (sys_msec > Timed_Val2);
      }
      else
      {
        return (sys_msec < Timed_Val1) || (sys_msec > Timed_Val2);
      }
    }
}

So I would say this returns true iff (if and only if) sys_msec is before Timed_Val1 or after Timed_Val1 + Timed_Val2 * 0.2 seconds.

As a final stage, now I would rename the variables and comment it.

//Returns true iff time is before startTime_msec or after timeoutPeriods of 0.2 seconds
//startTime_msec - millisecond value compariable to sys_msec
//timeoutPeriods - the number of timeout periods of 0.2 seconds each
int TestTimeOut(const unsigned long startTime_msec, const unsigned long timeoutPeriods)
{
    if (timeoutPeriods==0) return FALSE;

    {
      const unsigned long maxTime_msec = startTime_msec + (timeoutPeriods * 200);
      if (startTime_msec > maxTime_msec)
      { //this happens when it wraps around past 2^32
        return (sys_msec < startTime_msec) && (sys_msec > maxTime_msec);
      }
      else
      {
        return (sys_msec < startTime_msec) || (sys_msec > maxTime_msec);
      }          
    }
}

That's not to say there's not a better way of doing this, but at least it's readable now.