Return to Answer

+ comments on updated code.

Source Link

edited Feb 9 at 19:24

Edgar Bonet

edited Feb 9 at 19:24

Edgar Bonet

45.1k
4
42
81

Edit: Regarding your updated code, I suggest a few more changes:

As noticed by 6v6gt, do not rely on calcPulse() setting newPulse to false, as this makes your program too dependent on timings. Instead, set it to false in loop(), right after toggling the variable cameraEnable (which should be named "cameraEnabled"). I may be repeating myself here.
Computing the pulse width should not be conditioned on newPulse == false. There is no reason to do so: every pulse should be measured.
Do not use startPeriod in a boolean context: the value 0 is a perfectly valid return value for micros().

Here is a version of calcPulse() with all unnecessary stuff removed:

void calcPulse() {
 unsigned long now = micros();
 if (digitalRead(pulsePin) == HIGH) { // pulse starts
 pulseStartTime = now;
 } else { // pulse ends
 unsigned long pulseLength = now - pulseStartTime;
 if (pulseLength > 1500) {
 newPulse = true;
 }
 }
}

I just noticed that the newPulse variable is not actually needed: the interrupt handler could just take care of toggling cameraEnabled. This should also implify loop():

unsigned long pulseStartTime;
volatile bool cameraEnabled = false; // is the camera firing?
// Interrupt handler for CHANGE in pulsePin.
void calcPulse() {
 unsigned long now = micros();
 if (digitalRead(pulsePin) == HIGH) { // pulse starts
 pulseStartTime = now;
 } else { // pulse ends
 unsigned long pulseLength = now - pulseStartTime;
 if (pulseLength > 1500) {
 cameraEnabled = !cameraEnabled;
 }
 }
}
void loop() {
 if (cameraEnabled) {
 // Fire camera.
 digitalWrite(LED_PIN, HIGH);
 delay(500);
 digitalWrite(LED_PIN, LOW);
 delay(500);
 }
}

Edit: Regarding your updated code, I suggest a few more changes:

As noticed by 6v6gt, do not rely on calcPulse() setting newPulse to false, as this makes your program too dependent on timings. Instead, set it to false in loop(), right after toggling the variable cameraEnable (which should be named "cameraEnabled"). I may be repeating myself here.
Computing the pulse width should not be conditioned on newPulse == false. There is no reason to do so: every pulse should be measured.
Do not use startPeriod in a boolean context: the value 0 is a perfectly valid return value for micros().

Here is a version of calcPulse() with all unnecessary stuff removed:

void calcPulse() {
 unsigned long now = micros();
 if (digitalRead(pulsePin) == HIGH) { // pulse starts
 pulseStartTime = now;
 } else { // pulse ends
 unsigned long pulseLength = now - pulseStartTime;
 if (pulseLength > 1500) {
 newPulse = true;
 }
 }
}

I just noticed that the newPulse variable is not actually needed: the interrupt handler could just take care of toggling cameraEnabled. This should also implify loop():

unsigned long pulseStartTime;
volatile bool cameraEnabled = false; // is the camera firing?
// Interrupt handler for CHANGE in pulsePin.
void calcPulse() {
 unsigned long now = micros();
 if (digitalRead(pulsePin) == HIGH) { // pulse starts
 pulseStartTime = now;
 } else { // pulse ends
 unsigned long pulseLength = now - pulseStartTime;
 if (pulseLength > 1500) {
 cameraEnabled = !cameraEnabled;
 }
 }
}
void loop() {
 if (cameraEnabled) {
 // Fire camera.
 digitalWrite(LED_PIN, HIGH);
 delay(500);
 digitalWrite(LED_PIN, LOW);
 delay(500);
 }
}

Source Link

answered Feb 7 at 8:39

Edgar Bonet

answered Feb 7 at 8:39

Edgar Bonet

45.1k
4
42
81

There are a few issues with this code. I am not sure about how exactly they combine to give the behavior you experience. Anyhow, here are some points I suggest you fix:

The constant pin is poorly named, as there are three constants identifying pin numbers. Maybe PWMinput would be more explicit. Same for the variable toggle: cameraActive would be clearer.
Most variables could be local to the only function using them, which would make the code more readable.
The third argument of attachInterrupt() should be CHANGE, FALLING or RISING. The value of HIGH is identical to CHANGE, which is probably not what you want.
No need to call sei(): the Arduino core has already taken care of that.
As noticed by 6v6gt, do not rely on calcPulse() setting newPulse to false, as this makes your program too dependent on timings. Instead, set it to false in loop(), right after toggling the variable toggle.
The function calcPulse() blocks for the whole duration of the pulse, which you should never do in interrupt context: this will block the timer interrupt used by the Arduino core to track time and make millis() and delay() work. The interrupt service routine should fire on both pulse edges and only set startPeriod (on the rising edge) or newPulse (on the falling edge) and return right away.

At a higher level, I wonder how you plan to send the pulse to the ATtiny. If you are using an RC remote, will you be able to set the control above neutral for only the duration of a single pulse (20 ms)? If the remote sends multiple consecutive pulses longer than 1.5 ms, your ATtiny may not do what you expect.