The code of pulsein() ends with

... return clockCyclesToMicroseconds(width * 21 + 16); } and uses this define

define clockCyclesToMicroseconds(a) ( ((a) * 1000L) / (F_CPU / 1000L) )

The multiplication * 1000 + the multiplication *21 together causes a maxvalue for width of (approximately) MAXLONG/21000 above that overflow will occur.

Looking at the math the return statement of PulseIn() can be replaced by the simpler.

... return (width*21+16) / clockCyclesPerMicrosecond() ; }

which overflows less fast, making the PulseIn() function behave better in a larger range. Probably it will be optimized to a shift so it will be faster too.

Regards, Rob

-- update -- The timeout calculation (maxloops) has a similar math overflow.

See following threads for discussion of the problem - http://arduino.cc/forum/index.php/topic,74813.0.html - - http://arduino.cc/forum/index.php/topic,74642.0.html -

fixed version of pulsein() - http://arduino.cc/forum/index.php/topic,74813.msg565196.html#msg565196 -

Rob

Sounds like something we should fix.

Did you test the change to the timeout line? Does it actually make a difference?

Hi

difference?

Yes and yes, the default timeout of 1.000.000 micros overflows, it is easy to test (was the complain on the forum)

Rob

Some math: (hope i did it right as it is rather late ;)

lines changed - unsigned long maxloops = microsecondsToClockCycles(timeout) / 16; + unsigned long maxloops = timeout * clockCyclesPerMicrosecond() / 16;

and

• return clockCyclesToMicroseconds(width * 21 + 16);
  • return (width * 21 + 16) / clockCyclesPerMicrosecond();

(from wiring.h)

define clockCyclesPerMicrosecond() ( F_CPU / 1000000L )

define clockCyclesToMicroseconds(a) ( ((a) * 1000L) / (F_CPU / 1000L) )

define microsecondsToClockCycles(a) ( ((a) * (F_CPU / 1000L)) / 1000L )

old code

maxloops = microsecondsToClockCycles(timeout) / 16; => // timeout defaults 1000000. maxloops = microsecondsToClockCycles(1000000) / 16; => maxloops = ((1000000) * (F_CPU / 1000L)) / 1000L /16; => maxloops = ((1000000) * (16000L)) / 1000L /16; => maxloops = 16.000.000.000 /1000L /16 ---> overflow = > 194.693 loops iso 1.000.000

maximum correct value for timeout = 2^32/16.000 = 268.435 micros. roughly quarter of a second

new code

maxloops = timeout * clockCyclesPerMicrosecond() / 16; // assuming no optimization maxloops = timeout * ( F_CPU / 1000000L ) / 16; maxloops = timeout * (16) / 16;

Note the compiler could optimize this compiletime!, but assume it doesn't then the max correct value for timeout = 2^32/16 = 268.435.456 micros = 268 seconds approx 4.5 minutes (if optimized it would even be 70+ minutes)

same reasoning goes for the return value width: scenario 1: max width = ((2^32/16.000) -16) / 21 = 12.781 scenario 2: max width = ((2^32/16)-16) / 21 = 12.782.640

Which means that by using the proposed change one can measure pulses at least up to 12.5 seconds (on a 16Mhz)

additional: The macros from wiring.h

define clockCyclesPerMicrosecond() ( F_CPU / 1000000L )

define clockCyclesToMicroseconds(a) ( ((a) * 1000L) / (F_CPU / 1000L) )

define microsecondsToClockCycles(a) ( ((a) * (F_CPU / 1000L)) / 1000L )

must be rewritten /simplified to

define clockCyclesToMicroseconds(a) ( (a) / clockCyclesPerMicrosecond() )

define microsecondsToClockCycles(a) ( ((a) * clockCyclesPerMicrosecond() )

This increases their working range, preventing overflow for "relative small" values of a.

By changing the macros the original pulseIn() code would not need to be changed as proposed earlier as the problem would be solved at its root cause.

Sorry that it took so long to get this insight ;)

Rob

https://github.com/arduino/Arduino/commit/f520bb505134893b36182085fc1bfdc301d5bf89