Very incorrect timeout handling in pulseInLong

[igendel]

The three While loops in pulseInLong (at wiring_pulse.c) increase "numloops" from 0 to "maxloops" as a way to check and enforce the timeout parameter. However, "maxloops" is defined in clock cycles, but each loop run takes much more than a single clock cycle.

This results in timeouts that are actually over 20 times longer than the user requests.

Since this function uses micros() anyway for timing the pulse, I suggest replacing the numloops/maxloops method with checks against micros(). For instance:

// After function header and pin definitions...

unsigned long maxMicros = micros() + timeout;

// wait for any previous pulse to end
    while ((*portInputRegister(port) & bit) == stateMask)
        if (micros() > maxMicros)
            return 0;

// etc.

[facchinm]

Woa! I totally missed it 😄 Fixed by c7b2743
Thank you for pointing out this bug!

[matthijskooijman]

The fix does not handle micros() overflow properly. If maxMicros = micros() + timeout overflows, then micros() > maxMicros will return true immediately.

Instead, try: startMicros = micros(); and if (micros() - startMicros > timeout), since then the overflow will happen correctly.

[igendel]

True, matthijskooijman. I suggested my solution off the top of my head... the possibility of overflow should be taken into account as well. Thanks!

[facchinm]

@matthijskooijman , you are totally correct.
While applying your patch (facchinm@480cd22), however, I encountered another issue.
Flash occupation increases by 72 bytes on AVR... Any cleaver idea on why this happens before starting a deeper analysis?

[matthijskooijman]

Nothing springs to mind just now, but it does sound like something to investigate. If you want to investigate, have a look at this script I created to diff .elf file symbol tables (there is also a script to diff ASM output and reducing the amount of junk from differing addresses): https://github.com/matthijskooijman/scripts/blob/master/Arduino/diff-avr-sym

[facchinm]

After a loooooong (way longer than a pulse 😄 ) analysis it turned out that the three 32bit arguments involved in the subtraction lead the compiler to allocate these variables in the stack instead than using registers, thus the +72 byte.

Strangely, optimizing the function with -O1 the size increases by only 24 bytes.

No math guru over the web came out with a better implementation so I'm merging @matthijskooijman's one with a note about this strange behaviour

[matthijskooijman]

Ah, that makes sense - with my suggestion, the program needs to keep both track of startMicros and timeout, while the previous code didn't need timeout any more after the initial calculation of maxMicros, which needs more variables. Not sure why O1 doesn't have this problem, perhaps it does less reordering of the code or something. Oh well.