pulseIn contains invalid assumptions about compilation on newer tool chains

[ashirley]

The pulseIn function in Arduino/hardware/arduino/cores/arduino/wiring_pulse.c assumes that the loop compiles into 21 clock cycles (note the documentation disagrees and assumes 20). This seems to be true with avr-gcc 4.3.2 but not with avr-gcc 4.8.2.

avr-gcc 4.3.2 produces the following for the loop:

    1528:   0f c0           rjmp    .+30            ; 0x1548 <pulseIn+0xde> [2 cycles]
; loop starts here
    152a:   da 01           movw    r26, r20        ; [1 cycle]
    152c:   c9 01           movw    r24, r18        ; [1 cycle]
    152e:   8e 0d           add     r24, r14        ; [1 cycle]
    1530:   9f 1d           adc     r25, r15        ; [1 cycle]
    1532:   a0 1f           adc     r26, r16        ; [1 cycle]
    1534:   b1 1f           adc     r27, r17        ; [1 cycle]
    1536:   8a 15           cp      r24, r10        ; [1 cycle]
    1538:   9b 05           cpc     r25, r11        ; [1 cycle]
    153a:   ac 05           cpc     r26, r12        ; [1 cycle]
    153c:   bd 05           cpc     r27, r13        ; [1 cycle]
    153e:   f1 f0           breq    .+60            ; 0x157c <pulseIn+0x112> [1 cycle - usually not taken]
    1540:   2f 5f           subi    r18, 0xFF       ; 255 [1 cycle]
    1542:   3f 4f           sbci    r19, 0xFF       ; 255 [1 cycle]
    1544:   4f 4f           sbci    r20, 0xFF       ; 255 [1 cycle]
    1546:   5f 4f           sbci    r21, 0xFF       ; 255 [1 cycle]
    1548:   88 81           ld      r24, Y          ; [2 cycles]
    154a:   88 21           and     r24, r8         ; [1 cycle]
    154c:   89 15           cp      r24, r9         ; [1 cycle]
    154e:   69 f3           breq    .-38            ; 0x152a <pulseIn+0xc0> [2 cycles as usually taken]

which by my count is 21 cycles long which matches the assumption in the code.

avr-gcc 4.3.2 produces the following for the loop:

    12b4:   68 81           ld      r22, Y          ; [2 cycles]
    12b6:   60 23           and     r22, r16        ; [1 cycle]
    12b8:   61 13           cpse    r22, r17        ; [2 cycles - usually skips next instruction]
    12ba:   0a c0           rjmp    .+20            ; 0x12d0 <pulseIn+0xd6> [usually skipped]
    12bc:   28 17           cp      r18, r24        ; [1 cycle]
    12be:   39 07           cpc     r19, r25        ; [1 cycle]
    12c0:   4a 07           cpc     r20, r26        ; [1 cycle]
    12c2:   5b 07           cpc     r21, r27        ; [1 cycle]
    12c4:   a9 f0           breq    .+42            ; 0x12f0 <pulseIn+0xf6> [1 cycle - usually not taken]
    12c6:   2f 5f           subi    r18, 0xFF       ; 255 [1 cycle]
    12c8:   3f 4f           sbci    r19, 0xFF       ; 255 [1 cycle]
    12ca:   4f 4f           sbci    r20, 0xFF       ; 255 [1 cycle]
    12cc:   5f 4f           sbci    r21, 0xFF       ; 255 [1 cycle]
    12ce:   f2 cf           rjmp    .-28            ; 0x12b4 <pulseIn+0xba> [2 cycles]

Which by my count is only 16. This would account for pulse readings being around 30% higher than expected (as we are seeing and as is reported at http://forum.arduino.cc/index.php/topic,247507.0.html )

Note that in the process of raising this bug (but after I had done all the investigation!) I found the following thread discussing possible fixes: https://groups.google.com/a/arduino.cc/forum/#!topic/developers/5Po5BuY7UhA

This thread points out that this implementation is also very fragile in the face of interrupts.

[matthijskooijman]

So, there's two issues with the current implementation:

1. It makes assumptions about the code generated by hardcoding the formula
2. Interrupts completely destroy the counting
3. could be solved by handcoding the loop in assembly, which should be a reasonable approach. This still leaves 2. In the mailling list, it was suggested that 2. is only a problem for longer pulses, but it's just more likely to occur in a longer pulse. If an interrupt happens during a short pulse, the pulse length will be way off (an interrupt easily takes up to a dozen us IIRC, which is huge on a 2 or 3 us pulse).

It seems that using micros() for the timing solves 1. and 2., but 2. is still not entirely solved (when an interrupt happens just before the end of the pulse, its length will be too long by the interrupt duration). Also, people think the loss of granularity to 4us is too much. Using a separate timer could solve the granularity issue, but still leaves the interrupt-at-the-end issue.

Disabling interrupts during pulseIn would perfectly solve the interrupt issues, but disabling interrupts for more than a few us will likely break all sorts of other stuff...

I think the summary is that the best solution depends on:

• The expected pulse duration (and time until pulse)
• The accuracy requirements
• (Other) interrupt latency requirements

Hard to it correct for everybody.

Having said that - currently pulseIn returns wrong results in all cases, which is something that should certainly be fixed...

[facchinm]

The current implementation is obviously broken and I think it should be fixed using (optimized) compiler-agnostic asm code. This should fix the backwards-compatibility problem. In a long term view it could be useful to provide a function like

unsigned long pulseInLen(uint8_t pin, uint8_t state, unsigned long timeout, unsigned long expected_length)

where different implementations are chosen at runtime based on the expected lenght of the pulse.
The additional check will switch between asm and micros() implementations, also deciding if it is safe to disable interrupts.

[damellis]

Currently, it's up to the user whether to prioritize pulseIn() accuracy (by disabling interrupts before calling the function) or interrupts. Would we want to remove that choice from the user by automatically disabling interrupts inside of pulseIn()?

Of course, it would be good to fix the incorrect assumptions about the generated assembly code / loop duration.

[matthijskooijman]

@damellis, that's not entirely true - using a micros() based approach instead of a instruction-timing approach also makes the timing less sensitive to error caused by interrupts, without having to disable interrupts at all.

I agree that pulseIn probably shouldn't go disabling interrupts - if the user thinks that's needed, he should probably do it himself.

[facchinm]

@damellis the rationale behind my proposal was fixing the bug only in the original function, leaving the usual freedom to the user (whether disabling or leaving irq, etc...), plus giving another helper function for new projects that can automatically chose the "best" implementation based on the expected length of the pulse

[facchinm]

I just posted a pull request on #2495
If you could test it in your environment it would be much appreciated

[cmaglie]

Closing this one, please follow up on #2495