Measuring a full wave cycle using pulseIn

I am using an Arduino Uno and an Adafruit Motor Shield v2.3 as a tachometer for my ATV. I have built a simple circuit that sends a LOW pulse when the spark plug fires. I have a working program, but the refresh rate is very slow at idle. This is because I have two successive pulseIn commands that measure the HIGH and LOW durations of the signal. After the first pulseIn(HIGH) command is finished, the signal is LOW. The next pulseIn(LOW) command has to wait for the signal to go HIGH again before it can time a LOW pulse. If I can eliminate this waiting, I can double the refresh rate. I need help to do this. Here is my program as it is now:

#include <Servo.h>
#include <Wire.h>
#include <Adafruit_MotorShield.h>
#include "utility/Adafruit_PWMServoDriver.h"
Adafruit_MotorShield AFMS = Adafruit_MotorShield();
Servo servo1;

int p2 = 2;
volatile double duration;
int pos;
volatile unsigned long duration1;
volatile unsigned long duration2;

void setup() {
  Serial.begin(9600);
  servo1.attach(10);
  pinMode(p2, INPUT);
  servo1.write(180);
  delay(50);
}

void loop() {
    refresh:
  duration1 = pulseIn(p2, HIGH, 600000);
  duration2 = pulseIn(p2, LOW, 600000);
  duration = duration1 + duration2;
  duration = duration / 1000000;
  duration = 1 / duration;
  duration = 120 * duration;
  if (duration > 9000) {
  servo1.write(0);
  Serial.println("Overrev");
  Serial.println(duration);
    goto refresh;
}
if (duration < 750) {
servo1.write(180);
Serial.println("Stall");
Serial.println(duration);
delay(10);
goto refresh;
}
  pos = map(duration, 750, 9000, 170, 0);
  Serial.println(duration);
  servo1.write(pos);
}

I have been looking into the attachInterrupt command but I don't quite know how I would use it for this.

What the hell is with the gotos? Don't you realize that loop() loops? Get rid of them!

You could have the signal come into an interrupt pin, and attach the interrupt handler using CHANGE, so the ISR is called when the signal changes from LOW to HIGH and when it goes from HIGH to LOW. In the ISR, read the current state to determine what the change was.

The goto's are there to skip the last few lines in the loop when the speed is out of range. It skips writing to the servo and rechecks the engine's speed.

I see what you mean about the attachInterrupt command with the CHANGE mode, but how do I time the pulse with it?

electrodog321:
The goto's are there to skip the last few lines in the loop when the speed is out of range. It skips writing to the servo and rechecks the engine's speed.

I see what you mean about the attachInterrupt command with the CHANGE mode, but how do I time the pulse with it?

Then put that last bit of code in an else to that if right before it. There's never any good reason to use goto.

void loop() {
  duration1 = pulseIn(p2, HIGH, 600000);
  duration2 = pulseIn(p2, LOW, 600000);
  duration = duration1 + duration2;
  duration = duration / 1000000;
  duration = 1 / duration;
  duration = 120 * duration;
  if (duration > 9000) {
    servo1.write(0);
    Serial.println("Overrev");
    Serial.println(duration);
  }
  else if (duration < 750) {
    servo1.write(180);
    Serial.println("Stall");
    Serial.println(duration);
    delay(10);
  }
  else {
    pos = map(duration, 750, 9000, 170, 0);
    Serial.println(duration);
    servo1.write(pos);
  }
}

Delta_G:
Then put that last bit of code in an else to that if right before it. There's never any good reason to use goto.

void loop() {

duration1 = pulseIn(p2, HIGH, 600000);
  duration2 = pulseIn(p2, LOW, 600000);
  duration = duration1 + duration2;
  duration = duration / 1000000;
  duration = 1 / duration;
  duration = 120 * duration;
  if (duration > 9000) {
    servo1.write(0);
    Serial.println("Overrev");
    Serial.println(duration);
  }
  else if (duration < 750) {
    servo1.write(180);
    Serial.println("Stall");
    Serial.println(duration);
    delay(10);
  }
  else {
    pos = map(duration, 750, 9000, 170, 0);
    Serial.println(duration);
    servo1.write(pos);
  }
}

Thank you for that edit. But why is it forbidden to use a goto command in Arduino? Does it slow down the program or take up lots of space? My first programing language was Batch so I am used to using goto's.

It makes for what we call spaghetti code. That is, it makes the code hard to follow. Even in other languages where it is often used it makes it confusing trying to trace down where the code goes. C++ has all the constructs needed to avoid it so there's no good reason to use it.

Delta_G:
It makes for what we call spaghetti code. That is, it makes the code hard to follow. Even in other languages where it is often used it makes it confusing trying to trace down where the code goes. C++ has all the constructs needed to avoid it so there's no good reason to use it.

Good tip. Thank you.

but how do I time the pulse with it?

You can tell, in the ISR, if the change is to HIGH. If it is, record that time. If not, the change must be to LOW, so you can record that time. If the idea is to determine the HIGH to HIGH time, use RISING, instead, and just determine the time between this call to the ISR and the last call to the ISR.

PaulS:
You can tell, in the ISR, if the change is to HIGH. If it is, record that time. If not, the change must be to LOW, so you can record that time. If the idea is to determine the HIGH to HIGH time, use RISING, instead, and just determine the time between this call to the ISR and the last call to the ISR.

Okay, but can I see some code to do this? I want to time a full cycle of the wave. (Rising edge to rising edge or falling edge to falling edge.)

electrodog321:
The goto's are there to skip the last few lines in the loop when the speed is out of range. It skips writing to the servo and rechecks the engine's speed.

use the continue statement for that.

I want to time a full cycle of the wave. (Rising edge to rising edge or falling edge to falling edge.)

The necessary snippets:

    attachInterrupt(intPin, measureCycleLength, RISING);

unsigned long interruptTime = 0;
unsigned long cycleTime = 0;

void measureCycleLength()
{
   unsigned long now = millis();
   if(interruptTime > 0)
   {
      cycleTime = now - interruptTime;
   }

   interruptTime = now;
}

pulseIn() counts the HIGH or LOW time in milliseconds. You can determine one from the other. I avoided using interrupts using pulseIn and some creative code when I built a digital speedometer for my truck. It is accurate and works quite well on a 20x4 lcd and is very responsive. You don't have to worry about interrupts. I use the same speedo input to run my electronic transmission controller built using a Mega 2560. Works great even with all it has going on controlling the trans control.

I counted the LOW time using pulseIn. Then calculated the frequency from that and using a few more calculations devised road speed.

Just my 2 cents

Bill

Billdefish:
pulseIn() counts the HIGH or LOW time in milliseconds. You can determine one from the other.

Provided the duty cycle is fixed and known.

Aarg, I assume if the OP is going this far, he/she should know those variables, I did when I started on my speedo. And if the OP doesn't know, this should prompt them to find out. :slight_smile:

Bill

MarkT:
use the continue statement for that.

I like sound of that, but when I use it gives me an error that it is not in a loop. Is it supposed to work for the void loop() loop?

aarg:
Provided the duty cycle is fixed and known.

The duty cycle is not fixed. The engine can control the spark timing and duration for a better burn. I need to measure both the HIGH and LOW components of the wave.

Billdefish:
Aarg, I assume if the OP is going this far, he/she should know those variables, I did when I started on my speedo. And if the OP doesn't know, this should prompt them to find out. :slight_smile:

Bill

I tried that but it was very inconsistent because the engine can adjust the spark duration for a cleaner burn. I need to measure both the HIGH and LOW components of the waveform.

PaulS:
The necessary snippets:

    attachInterrupt(intPin, measureCycleLength, RISING);

unsigned long interruptTime = 0;
unsigned long cycleTime = 0;

void measureCycleLength()
{
  unsigned long now = millis();
  if(interruptTime > 0)
  {
      cycleTime = now - interruptTime;
  }

interruptTime = now;
}

You are very close, PaulS. But I can't get the ISR to send the cycleTime variable back to the void loop() portion of the program with the math functions. I also can't seem to get the servo to react to it's command when the math functions and the servo command are in the ISR.

Any suggestions?

I need to measure both the HIGH and LOW components of the waveform.

This conflicts with what you said before.

You need to use CHANGE, to do this, and determine if the change is to HIGH (the end of the LOW phase and the start of the HIGH phase) or of the change is to LOW (the end of the HIGH phase and the start of the LOW phase.

What you do when each change happens is very similar to what you do catching only the RISING edges, except that you use a total of 4 time variables - to capture the start and end of each phase. Your turn to try.

But I can't get the ISR to send the cycleTime variable back to the void loop() portion of the program

It is (supposed to be) global, so there is no need to send it anywhere. It should be volatile, so that loop() knows it might have changed in an interrupt.