user31442: I am reading the period of pulses from a 600ppturn shaft encoder, using the FreqPeriod library, by Martin Nawrath.
This works using counter1 and the comparator on pin6 and pin7 on the Arduino Uno.

The period duration for 1-2500Hz is being used in a midi musical instrument to give a speed to volume and a note change at a fixed speed.
This has worked well on the Uno but now I want to use a Mega (which does not have both comparator pins).
So I have tried using the Nick Gammon interrupt sketch. This has produced very erratic results returning both positive and many negative numbers (shown by using "volatile long period;" in the monitor.
Can you tell me what is happening and would it be better to use a counter instead of interrupts?
Hope I have given enough information to be getting on with, many thanks.
// Camera shutter speed timer
// Author: Nick Gammon
// Date: 15th January 2012

volatile boolean started;
volatile unsigned long startTime;
volatile unsigned long endTime;
volatile unsigned long period;
volatile unsigned long lfrq;

// interrupt service routine
void isr () {
    if (started)
        endTime = micros ();
    else
        startTime = micros ();
    started = !started;
} // end of Pulse Width

void setup () {
    digitalWrite (2, HIGH); // pull-up
    Serial.begin (9600);
    Serial.println (" Pulse Width test = ");
    attachInterrupt (digitalPinToInterrupt (2), isr, CHANGE); //CHANGE RISING
} // end of setup

void loop () {
    if (endTime) {
        period = (endTime - startTime) * 32 ; // pulse width?
        Serial.print (" \n  ");
        Serial.print ( " period =  ") ;
        Serial.print (period) ;
        lfrq = 16000000.0 / period;
        Serial.print(" Hz = "); // one rev per sec = 26666 micros period
        Serial.print( lfrq );
        endTime = 0;
    }
} // end of loop


user7508: 
Can you tell me what is happening [...]?

Not really, but here is a wild guess: your input signal is bouncy, the
ISR fires too often, your loop cannot keep up with the rate, and you end
up accessing endTime in the ISR and loop simultaneously.
If you have a scope, look at your encoder signal. Is it clean?

would it be better to use a counter instead of interrupts?

Using the input capture feature of a timer will give you the best
possible accuracy, as you will have single-cycle resolution. In
contrast, micros() has only 4 µs resolution (64 times worse) and
the ISR may be delayed by up to ≈ 16 µs by the TIMER0_OVF
ISR.
You can use the FreqPeriod library you linked to as an inspiration for
writing your own timer-based code, but it will require some changes:

As pointed out by Nick Gammon in a comment, you don't need the analog
comparator, nor the conditioning RC network, as the encoder's outputs
are already digital. Or you may use a low-pass filter if the encoder
is bouncy.
You will use the input capture pins of either timer 4 (digital pin 49)
or timer 5 (digital pin 48). The other input capture pins are not
available on the Mega's pin headers.
You will replace ISR(ANALOG_COMP_vect) by either ISR(TIMER4_CAPT)
or ISR(TIMER5_CAPT) (the relevant “capture event” ISR). No need to
adjust a comparator threshold in the ISR.
In the begin() method, you will leave the analog comparator alone,
and you will enable the capture event interrupt.

Obviously you will have to study the
datasheet
before start coding.

user30005: 
Could this be so?

because the code is faulty: there is no assurance of atomicity between startTime and endTime. ie. the interrupts could have arrived, multiple times, while the if loop is executing.
two ways to fix it:

calculate period in the isr; or
clear endTime once the clock starts in the isr.


user33871: While you are doing all those things with those volatile variables in the loop function, the interrupt can still be firing.  If it fires in the middle of a read of startTime or endTime then you get might get part of the variable holding old value and part holding new value and when you put it together it makes garbage.  
You need to do a cli() at the beginning of loop to turn off interrupts and quickly make local copies of all those variables and then sei() to turn interrupts back on.  Now you can safely work with the copies you made.  

31442: <p>I am reading the period of pulses from a 600ppturn shaft encoder, using the <a href="http://interface.khm.de/index.php/lab/interfaces-advanced/frequency-measurement-library/" rel="nofollow noreferrer">FreqPeriod library</a>, by Martin Nawrath.</p>

<p>This works using counter1 and the comparator on pin6 and pin7 on the Arduino Uno.</p>

<p><a href="https://i.stack.imgur.com/jNuNi.jpg" rel="nofollow noreferrer"><img src="https://i.stack.imgur.com/jNuNi.jpg" alt=" Counter and Capture unit"></a></p>

<p>The period duration for 1-2500Hz is being used in a midi musical instrument to give a speed to volume and a note change at a fixed speed.
This has worked well on the Uno but now I want to use a Mega (which does not have both comparator pins).</p>

<p>So I have tried using the Nick Gammon interrupt sketch. This has produced very erratic results returning both positive and many negative numbers (shown by using "volatile long period;" in the monitor.</p>

<p>Can you tell me what is happening and would it be better to use a counter instead of interrupts?</p>

<p>Hope I have given enough information to be getting on with, many thanks.</p>

<pre class="lang-c++ prettyprint-override"><code>// Camera shutter speed timer
// Author: Nick Gammon
// Date: 15th January 2012

volatile boolean started;
volatile unsigned long startTime;
volatile unsigned long endTime;
volatile unsigned long period;
volatile unsigned long lfrq;

// interrupt service routine
void isr () {
    if (started)
        endTime = micros ();
    else
        startTime = micros ();
    started = !started;
} // end of Pulse Width

void setup () {
    digitalWrite (2, HIGH); // pull-up
    Serial.begin (9600);
    Serial.println (" Pulse Width test = ");
    attachInterrupt (digitalPinToInterrupt (2), isr, CHANGE); //CHANGE RISING
} // end of setup

void loop () {
    if (endTime) {
        period = (endTime - startTime) * 32 ; // pulse width?
        Serial.print (" \n  ");
        Serial.print ( " period =  ") ;
        Serial.print (period) ;
        lfrq = 16000000.0 / period;
        Serial.print(" Hz = "); // one rev per sec = 26666 micros period
        Serial.print( lfrq );
        endTime = 0;
    }
} // end of loop
</code></pre>
 7508: <blockquote>
  <p>Can you tell me what is happening [...]?</p>
</blockquote>

<p>Not really, but here is a wild guess: your input signal is bouncy, the
ISR fires too often, your loop cannot keep up with the rate, and you end
up accessing <code>endTime</code> in the ISR and loop simultaneously.</p>

<p>If you have a scope, look at your encoder signal. Is it clean?</p>

<blockquote>
  <p>would it be better to use a counter instead of interrupts?</p>
</blockquote>

<p>Using the input capture feature of a timer will give you the best
possible accuracy, as you will have single-cycle resolution. In
contrast, <code>micros()</code> has only 4&nbsp;µs resolution (64 times worse) and
the ISR may be delayed by up to ≈&nbsp;16&nbsp;µs by the <code>TIMER0_OVF</code>
ISR.</p>

<p>You can use the FreqPeriod library you linked to as an inspiration for
writing your own timer-based code, but it will require some changes:</p>

<ul>
<li><p>As pointed out by Nick Gammon in a comment, you don't need the analog
comparator, nor the conditioning RC network, as the encoder's outputs
are already digital. Or you may use a low-pass filter if the encoder
is bouncy.</p></li>
<li><p>You will use the input capture pins of either timer 4 (digital pin 49)
or timer 5 (digital pin 48). The other input capture pins are not
available on the Mega's pin headers.</p></li>
<li><p>You will replace <code>ISR(ANALOG_COMP_vect)</code> by either <code>ISR(TIMER4_CAPT)</code>
or <code>ISR(TIMER5_CAPT)</code> (the relevant “capture event” ISR). No need to
adjust a comparator threshold in the ISR.</p></li>
<li><p>In the <code>begin()</code> method, you will leave the analog comparator alone,
and you will enable the capture event interrupt.</p></li>
</ul>

<p>Obviously you will have to study <a href="http://www.atmel.com/Images/Atmel-2549-8-bit-AVR-Microcontroller-ATmega640-1280-1281-2560-2561_datasheet.pdf" rel="nofollow noreferrer">the
datasheet</a>
before start coding.</p>
 30005: <blockquote>
  <p>Could this be so?</p>
</blockquote>

<p>because the code is faulty: there is no assurance of atomicity between startTime and endTime. ie. the interrupts could have arrived, multiple times, while the if loop is executing.</p>

<p>two ways to fix it:</p>

<ol>
<li><p>calculate period in the isr; or</p></li>
<li><p>clear endTime once the clock starts in the isr.</p></li>
</ol>
 33871: <p>While you are doing all those things with those volatile variables in the loop function, the interrupt can still be firing.  If it fires in the middle of a read of startTime or endTime then you get might get part of the variable holding old value and part holding new value and when you put it together it makes garbage.  </p>

<p>You need to do a cli() at the beginning of loop to turn off interrupts and quickly make local copies of all those variables and then sei() to turn interrupts back on.  Now you can safely work with the copies you made.  </p>
