PulseIn() overflows due to math expression -> can be simplified ??

[GoogleCodeExporter]

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

Original issue reported on code.google.com by rob.till...@gmail.com on 9 Oct 2011 at 3:33

[GoogleCodeExporter]

-- 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

Original comment by rob.till...@gmail.com on 10 Oct 2011 at 8:39

[GoogleCodeExporter]

Sounds like something we should fix.

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

Original comment by dmel...@gmail.com on 11 Oct 2011 at 3:34

[GoogleCodeExporter]

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)

Original comment by rob.till...@gmail.com on 11 Oct 2011 at 8:50

[GoogleCodeExporter]

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

Original comment by rob.till...@gmail.com on 13 Nov 2011 at 9:21

[GoogleCodeExporter]

https://github.com/arduino/Arduino/commit/f520bb505134893b36182085fc1bfdc301d5bf
89

Original comment by dmel...@gmail.com on 30 Dec 2011 at 11:08

• Changed state: Fixed
• Added labels: Component-Core, Milestone-1.0.1