pulseInLong function incorrect timeout

[igendel]

Hello all,

While browsing through the Arduino source code, I noticed what seems to me a serious issue with the way pulseInLong (in wiring_pulse.c) treats the timeout parameter.

Here's an example from the code:

    unsigned long numloops = 0;
    unsigned long maxloops = microsecondsToClockCycles(timeout);

    // wait for any previous pulse to end
    while ((*portInputRegister(port) & bit) == stateMask)
        if (numloops++ == maxloops)
            return 0;

This, I believe, assumes that each pass of the while loop takes exactly one cycle - and that's obviously incorrect. My crude measurements of pulseInLong running time also indicate to the same (runtime too long, length proportional to value of timeout).

Since it may be hard to estimate the exact cycle-time of the loop for each MCU and compiler, perhaps it's best to use micros() for timing out in these pre-measurement loops, as well as in the pulse measurement itself. Would that be a good solution?

[Cristian Maglie]

Il 20/09/2015 02:29, igendel ha scritto:
> Here's an example from the code:
>
> |
> unsignedlongnumloops =0;
> unsignedlongmaxloops =microsecondsToClockCycles(timeout);
>
> // wait for any previous pulse to end
> while((*portInputRegister(port)&bit)==stateMask)
> if(numloops++==maxloops)
> return0;
> |

you're browsing a very old version, please look at the latest here:

https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/wiring_pulse.c#L33

unsigned long pulseIn(uint8_t pin, uint8_t state, unsigned long timeout)
{
// cache the port and bit of the pin in order to speed up the
// pulse width measuring loop and achieve finer resolution. calling
// digitalRead() instead yields much coarser resolution.
uint8_t bit = digitalPinToBitMask(pin);
uint8_t port = digitalPinToPort(pin);
uint8_t stateMask = (state ? bit : 0);

// convert the timeout from microseconds to a number of times through
// the initial loop; it takes approximately 16 clock cycles per iteration
unsigned long maxloops = microsecondsToClockCycles(timeout)/16;

unsigned long width = countPulseASM(portInputRegister(port), bit,
stateMask, maxloops);

// prevent clockCyclesToMicroseconds to return bogus values if
countPulseASM timed out
if (width)
return clockCyclesToMicroseconds(width * 16 + 16);
else
return 0;
}

as you can see maxloops is now divided by 16 and the busy-loop is
written in asm to keep the loops duration constant and independent from
compiler optimizations.

C

[Cristian Maglie]

Il 21/09/2015 10:32, Cristian Maglie ha scritto:
> Il 20/09/2015 02:29, igendel ha scritto:
>> Here's an example from the code:
>>
>> |
>> unsignedlongnumloops =0;
>> unsignedlongmaxloops =microsecondsToClockCycles(timeout);
>>
>> // wait for any previous pulse to end
>> while((*portInputRegister(port)&bit)==stateMask)
>> if(numloops++==maxloops)
>> return0;
>> |

Sorry, just realized that you're talking about pulseInLong and not pulseIn!

you're right, thank you for the notification!

C

[igendel]

On Monday, September 21, 2015 at 11:32:46 AM UTC+3, c.maglie wrote:

you're browsing a very old version, please look at the latest here:

Hi,

The code is from pulseInLong that came with the official Arduino 1.6.5, not for pulseIn.
Anyway, since this post on mine did not appear here for almost a day, I figured there might have been a technical error in and posted it as a GitHub issue; it was quickly resolved there, so please consider the matter closed.

Thanks!