Timing between rising and falling interrupt on the same pin

I am trying to measure the time between a rising and falling interrupt but instead of getting the time between them, I get the millis timer counting up plus the time between the two interrupts added on.

The resulting output in the serial monitor looks like this:

3505
3505
3505
3505
3505
3505
3505
3505
3553
3553
3553
3553
3553
3553
3553
3599
3599
3599
3599
3599
3599
3599

volatile unsigned long motortime1 = 0;
volatile unsigned long motortime2 = 0;
volatile int frametime = 0;

void setup()
{
Serial.begin(9600);
attachInterrupt(digitalPinToInterrupt(2), incframe, RISING);
attachInterrupt(digitalPinToInterrupt(2), motorspeedtime, FALLING);

}

void loop() 
{
  
Serial.println(frametime);

}

void incframe()
{
  //frames = frames + 1;
  unsigned long motortime1 = millis();

}

void motorspeedtime()
{
  unsigned long motortime2 = millis();
  frametime = motortime2 - motortime1;
}

You can only attach one interrupt to an external interrupt pin.

Attach one that triggers on CHANGE and check the state in the ISR.

I can't do it on change because this routine is part of a much larger project which controls a motor for a motion picture camera. The camera has a spinning mirror shutter in it to which I have attached a sensor and the blades on the shutter occupy a smaller angle than the gaps so using change would mess up my frame counting code which for simplicity and readability, I have not included here.

If you are using the external interrupts for another purpose, you can still attach a pin change to it, the external will have a higher priority. Form within the external ISR you can mark time and enable the PCI, then when the level changes the other interrupt will fire.

RB358:
I can’t do it on change because this routine is part of a much larger project which controls a motor for a motion picture camera. The camera has a spinning mirror shutter in it to which I have attached a sensor and the blades on the shutter occupy a smaller angle than the gaps so using change would mess up my frame counting code which for simplicity and readability, I have not included here.

I see no valid argument there.

Currently you have two routines, one for RISING, one for FALLING.

After the change you have one CHANGE ISR that calls the previous routines based on the pin level.

Whandall: I see no valid argument there.

Currently you have two routines, one for RISING, one for FALLING.

After the change you have one CHANGE ISR that calls the previous routines based on the pin level.

Right. And, when you read the pin level at the beginning of the ISR, do it directly through the port registers. It will be considerably faster than using digitalRead().

RB358: I can't do it on change because this routine is part of a much larger project which controls a motor for a motion picture camera. The camera has a spinning mirror shutter in it to which I have attached a sensor and the blades on the shutter occupy a smaller angle than the gaps so using change would mess up my frame counting code which for simplicity and readability, I have not included here.

If you can't do it on change, you certainly can't do it on both rising and falling, that's exactly the same thing (if it were possible to have two separate ISRs).

I reckon you can easily do it on change - you have to check what its changed to.

gfvalvo: Right. And, when you read the pin level at the beginning of the ISR, do it directly through the port registers. It will be considerably faster than using digitalRead().

What sort of speeds for the two methods are we talking here? Any chance of a code snippet to show what the port reading looks like? Thanks.

https://www.arduino.cc/en/Reference/PortManipulation

Thanks.
Here is the speed improvement I got:

timms = micros();
for (int i = 0; i < 1000; i++) {
int Aread= digitalRead(pinA);
int Bread= digitalRead(pinB);
}
elapsed = micros()-timms;
Serial.println("-------------------------------------------");
Serial.println("1000 x2 pin reads " + String(elapsed) + " us ");

// 8988 us

timms = micros();
for (int i = 0; i < 1000; i++) {
Portread = PINB;
byte A = Portread & 0b00010000; // pin B4
byte B = Portread & 0b00100000; // pin B5
}
elapsed = micros()-timms;
Serial.println("-------------------------------------------");
Serial.println("1000 Port Reads and PinEval " + String(elapsed) + " us ");

// 452 us

I have been working on this issue some more and I almost have it solved. The output on the serial monitor is now

43 -42 42 -43 42 -42 42 -42 42 -43 42

This is really close to what I want and the only way for me to stop getting a negative number the next time the shutter passes the sensor is to set calctime to 1 when case 1 in the interrupt code is true rather than setting it to 1 right at the start of the interrupt. This results in only positive numbers but at half the interval as I expected. How do I get only positive numbers each time the interrupt triggers?

volatile unsigned long prevframetime = 0;//stores the first reading of millis when the interrupt is triggered
volatile unsigned long newframetime = 0;//stores the second reading of millis when the interrupt is triggered after the first time
int frametime = 0; // stores the difference between prev and newframetime
volatile bool count = 0;
volatile bool calctime = 0; //when this is true, the resulting time is calculated and displayed
//volatile int frames = 0;

void setup()
{
Serial.begin(9600);
attachInterrupt(digitalPinToInterrupt(2), incframe, RISING);
}

void loop() 
{
  if (calctime == 1)
  {
    frametime = newframetime - prevframetime;
    Serial.println(frametime);    
    calctime = 0;
  }
}

void incframe()
{
  calctime = 1;// setting this variable true here results in alternating positive and negative time readings
  switch (count)
    {
    case 0:
      count = 1;
      prevframetime = millis();
      break;
    case 1:
    //calctime = 1; setting this variable true here results in only positive time readings but at half the rate. I did expect this
      count = 0;
      newframetime = millis();
    }
}

How do I get only positive numbers each time the interrupt triggers?

You already only get positive numbers. The value reported by millis() is NEVER negative.

What you need to do is subtract A from B in one case and subtract B from A in the other case. Or, just use abs(A - B).

frametime = newframetime - prevframetime;

Don’t do that. The interrupt might occur in between reading those two variables. Even worse, it might occur between reading the two bytes of a single int variable. Always disable interrupts to grab a “safe copy” of the volatile variables. If that overlaps the interrupt then it will be delayed by a few nanoseconds and will run just after you re-enable interrupts.

I also note that you did not change to CHANGE.

As I’ve said before, I can’t use change because it would mess up the timing. The shutter of the camera spins and it has two blades and two gaps. The blades interrupt a photodiode and LED sensor and they occupy a smaller angle than the gaps do so if I used change, each time the interrupt is triggered, a different time would be returned as the state changes. If the camera had equal spacing on the shutter, using change would make perfect sense. The program is now doing what I want it to but I am getting strange results on the serial output for the frametime variable. Sometimes it jumps to zero or a much lower value, but this doesn’t affect the speed of the motor. I think the strange values are due to what MorganS suggested so I tried disabling and enabling interrupts within the main loop and within calculate frame time but this made no difference. Here is what the serial monitor shows.

50
50
49
50
50
50
51
29
22
0
50
51
51
53
50

volatile unsigned long prevframetime = 0;//stores the first reading of millis when the interrupt is triggered
volatile unsigned long newframetime = 0;//stores the second reading of millis when the interrupt is triggered after the first time
int frametime = 0; // stores the difference between prev and newframetime
int setspeed = 50;
int val = 120;
volatile bool count = 0;
volatile bool calctime = 0; //when this is true, the resulting time is calculated and displayed
//volatile int frames = 0;
void setup()
{
Serial.begin(9600);
attachInterrupt(digitalPinToInterrupt(2), incframe, RISING);
}

void loop() 
{
  if (calctime == 1) 
  {   
    frametime = Calculate_Frame_Time();  
    Set_Motor_Speed(frametime,setspeed);
    Serial.println(frametime);   
  }
  analogWrite(3,val);
}

void incframe()
{
  calctime = 1;
  switch (count)
    {
    case 0:
      count = 1;
      prevframetime = millis();
      break;
    case 1:
      count = 0;
      newframetime = millis();
      break;
    }
}

int Calculate_Frame_Time()
{
  noInterrupts();
  int result;
     if (count == 0)
    {
      result = newframetime - prevframetime;   
      calctime = 0;
    }
    if (count == 1)
    {
      result = prevframetime - newframetime;  
      calctime = 0;
    }
    interrupts();
    return result;   
}

void Set_Motor_Speed(int a, int b)//a frametime b setspeed
{
  if (a > b && val < 255)
    {
      val +=1;
    }
  if (a < b && val > 0)
    {
      val -=1;
    }
    //analogWrite(3,val);
    //Serial.println(val);
}

RB358: As I've said before, I can't use change because it would mess up the timing. The shutter of the camera spins and it has two blades and two gaps. The blades interrupt a photodiode and LED sensor and they occupy a smaller angle than the gaps do so if I used change, each time the interrupt is triggered, a different time would be returned as the state changes.

And, as was explained to you in Reply #4, that's a totally incorrect line of reasoning. Reread Reply #5 also.

It seems you wouldn't understand unless you had the camera in front of you. I have explained numerous times how change can't work for this project.

Oh, for God's sake:

void setup()
{
Serial.begin(9600);
attachInterrupt(digitalPinToInterrupt(2), incframe, CHANGE);
}

void loop() {
  // Do loop stuff
}

void incframe() {
  uint8_t pinLevel;

  pinLevel = digitalRead(2);
  if (pinLevel) {
    processRisingEdge();    // must be a rising edge
  } else {
    processFallingEdge();   // must be a rising edge
  }
}

void processRisingEdge() {
  // Do rising edge stuff
}

void processFallingEdge() {
  // Do falling edge stuff
}

Maybe apply some gating? That means only accept certain inputs if they pass a test like "not less than 1/30th of a second." Pulses arriving faster than this either have some other meaning (trailing edge of the shutter) or they should be ignored.

Without more details it is difficult to say more.