EM shutter almost working. Still a few things needed...

Hi,

My electromagnetic shutter is almost complete, but lacking a few essential things both in programming and hardware.

First of all, when the input causes the interrupt function, the CAMERA output instantly switches to LOW, yet it should be HIGH until it is told to go LOW.

Second, when interrupted, the program needs to go back to the first line of the loop(), it instead locks into finishing the full delay, not allowing any interruptions.

This is the code below...

#include <avr/io.h>
#include <avr/interrupt.h>

#define INTERRUPTPIN PCINT1
#define PCINT_VECTOR PCINT0_vect
#define DATADIRECTIONPIN DDB1
#define PORTPIN PB2 
#define READPIN PINB2
#define SHUTTER 0
#define CAMERA 2

#define sbi(sfr, bit) (_SFR_BYTE(sfr) |= _BV(bit))
#define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit))


static volatile byte LEDState; 

void setup() {
    cli();
    pinMode(SHUTTER, OUTPUT); 
    pinMode(CAMERA, OUTPUT);
     

    
    LEDState = 0;
    PCMSK |= (1 << INTERRUPTPIN);
    GIMSK |= (1 << PCIE);
    DDRB &= ~(1 << DATADIRECTIONPIN); //cbi(DDRB, DATADIRECTIONPIN);
    PORTB |= (1<< PORTPIN); //cbi(PORTB, PORTPIN);
    sei();

}

void loop() 


{  
digitalWrite(CAMERA, HIGH);
delay(5000);
digitalWrite(CAMERA, LOW);
delay(500);
}




ISR(PCINT_VECTOR)
{

  byte pinState;
  pinState = (PINB >> READPIN)& 1; 
  if (pinState >0) 

{
{
digitalWrite(CAMERA, HIGH);
digitalWrite(SHUTTER, HIGH);
delay(200);
digitalWrite(SHUTTER, LOW);
digitalWrite(CAMERA, LOW);}
delay(500);
}

  
  }

{

Another problem is the reaction to an input is unreliable, sometimes responding, and other times not...

Complete code should be shown.
CTRL-T in the Arduino IDE should be used to auto-format the code.
Interrupt Service Routines (ISRs) should execute quickly. Thus, they should NEVER contain a call to delay(...).
Instead of using delay(...) in the loop function to waste ten seconds of time, a millis() based approach could do the same job and would not need an ISR.
Arduino programmers usually avoid pins 0 and 1 because they are shared with the serial port.

Thanks for replying, I really appreciate it

Here is the complete code, modified from a tutorial I found, so it contains some notes.

//Includes
#include <avr/io.h>
#include <avr/interrupt.h>

#define INTERRUPTPIN PCINT1 //this is PB1 per the schematic
#define PCINT_VECTOR PCINT0_vect  //this step is not necessary
#define DATADIRECTIONPIN DDB1 //Page 64 of data sheet
#define PORTPIN PB2 //Page 64
#define READPIN PINB2 //page 64
#define SHUTTER 0 //PB0
#define CAMERA 2 //PB2

#define sbi(sfr, bit) (_SFR_BYTE(sfr) |= _BV(bit)) //OR
#define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit)) //AND

/*
   Alias for the ISR: "PCINT_VECTOR" (Note: There is only one PCINT ISR.
   PCINT0 in the name for the ISR was confusing to me at first,
   hence the Alias, but it's how the datasheet refers to it)
*/

static volatile byte LEDState; //variable used within ISR must be declared Volatile.

void setup() {
  cli();//disable interrupts during setup
  pinMode(SHUTTER, OUTPUT); //we can use standard arduino style for this as an example
  pinMode(CAMERA, OUTPUT);
  digitalWrite(SHUTTER, LOW); //set the LED to LOW

  LEDState = 0; //we use 0 for Low state and 1 for High
  PCMSK |= (1 << INTERRUPTPIN); //sbi(PCMSK,INTERRUPTPIN) also works but I think this is more clear // tell pin change mask to listen to pin2 /pb3 //SBI
  GIMSK |= (1 << PCIE);   // enable PCINT interrupt in the general interrupt mask //SBI

  DDRB &= ~(1 << DATADIRECTIONPIN); //cbi(DDRB, DATADIRECTIONPIN);//  set up as input  - pin2 clear bit  - set to zero
  PORTB |= (1 << PORTPIN); //cbi(PORTB, PORTPIN);// disable pull-up. hook up pulldown resistor. - set to zero
  sei(); //last line of setup - enable interrupts after setup

}

void loop() {
  digitalWrite(CAMERA, HIGH);
  delay(20000);
  digitalWrite(CAMERA, LOW);
  delay(500);
  // put your main code here, to run repeatedly:
  //If you connect a debounced pushbutton to PB2 and to VCC you can tap the button and the LED will come on
  //tap the button again and the LED will turn off.

}


//this is the interrupt handler
ISR(PCINT_VECTOR)
{
  //Since the PCINTn triggers on both rising and falling edge let's just looks for rising edge
  //i.e. pin goes to 5v
  byte pinState;
  pinState = (PINB >> READPIN) & 1; //PINB is the register to read the state of the pins
  if (pinState > 0) //look at the pin state on the pin PINB register- returns 1 if high
  {
    digitalWrite(CAMERA, HIGH);
    digitalWrite(SHUTTER, HIGH);
    delay(30);
    digitalWrite(SHUTTER, LOW);
    digitalWrite(CAMERA, LOW);
    delay(1000);


  }


}

So you are saying that if I used the millis() I could do away with ISRs altogether? Would I use an if/else instead?

I do need the loop to be able to be interrupted at any time as quickly as possible, but the other sequence to run through entirely, no matter how short the interruption.

Thanks again so much for replying. This thing has had me going round in circles for days!

vaj4088:
Complete code should be shown.
CTRL-T in the Arduino IDE should be used to auto-format the code.
Interrupt Service Routines (ISRs) should execute quickly. Thus, they should NEVER contain a call to delay(...).
Instead of using delay(...) in the loop function to waste ten seconds of time, a millis() based approach could do the same job and would not need an ISR.
Arduino programmers usually avoid pins 0 and 1 because they are shared with the serial port.

I have changed the delay() to millis() and I get "too many arguments to function 'long unsigned int millis()'". I don't know what to do about this, I'm brand new to Arduino...

You cannot just replace the word "delay" with the word "millis" and have your program work. I suggest that you read the documentation for the function millis at millis() - Arduino Reference

and also read Using millis() for timing. A beginners guide

Almost nothing else can occur during delay ("Almost nothing else" means "Nothing except interrupts"). However, if your loop function will check the input pin, then there can be a response nearly as fast (or perhaps even faster than) as using an ISR, and much easier to debug. You would, indeed, be using an if/else instead.

Please revise your program after you read both links that I posted above. Using millis() instead of delay(...) usually means restructuring your program, but your program is small enough that any restructuring should go quickly.

I cannot say how fast this can go (because I do not know all Arduinos and you have not told me what you have or what its clock speed is) but it can go very fast. Microseconds, even.

This code

  pinState = (PINB >> READPIN) & 1; //PINB is the register to read the state of the pins

costs time on many Arduinos because of the shift operator ">>". Because you only compare to zero, it would be better to shift the "1" in the other direction and not shift the value from PINB at all. The compiler should be "smart" enough to calculate this constant once and use it instead of a shift. If the code is sufficient as it stands, perhaps you do not "...need the loop to be able to be interrupted at any time as quickly as possible...".

For some people, "...as quickly as possible..." is femtoseconds. For other people, "...as quickly as possible..." means seconds. What is your requirement?

Thanks again,

I'll see if I can make sense of how to restructure it. So does that mean the delay function can still be used in the "else" code? If I understand correctly it will block everything else and finish what it has been told to do.

My response time would hopefully be 1-5 milliseconds at the most, as this is to detect and respond to small, fast insects flying through the focal area of a camera lens.

Using attiny85 with 8mhz internal clock

Thanks, I'll see how I go with restructuring...

I've been ripping my hair out trying to figure out how to adapt any of this to make the loop run.

For a response time of one millisecond, using the loop function and avoiding the ISR should work very well. Perhaps if you sleep on it there will be less hair ripped out?

You can mix delay(...) and millis() but it is usually not a good idea because it confuses some people. Once you get used to using millis() and keeping track of state, you may wonder why you ever bothered with delay(...).

Figure out your states, such as waiting to be triggered, shutter open, waiting with shutter open, shutter closed, waiting while film advances, etc. Experts use an enum but you can just assign an integer to each one and keep track of the current state by using an int variable. Use a switch/case statement to do the right thing for the current state. Use the loop() function to do the looping that you will need to do.

Use an unsigned long variable to hold the time [millis()] of an interesting event, and use subtraction to determine whether sufficient time has elapsed since the interesting event.

I am tempted to write the code myself but I do not really understand what you are attempting to accomplish.

The order of operations is

loop (2 minutes)

1 ensure shutter output is LOW
2 camera output HIGH
3 keep camera output high for 2 minutes
4 camera output to LOW

If it is interrupted at any time in the loop, the following needs to happen and complete without interruptions:

1 Camera should still be HIGH
2 shutter output to HIGH
3 Hold for 100 milliseconds
4 Shutter output to LOW
5 Camera output to LOW
6 Wait 1 second before returning to loop

This should be simple and it frustrates me to no end that I cannot seem to find a way to make it work, all of the examples in tutorials are of little help as they are just for blinking LEDs - I really have no idea how to adapt it. I have been working on this project for days with nothing to show...

This is the code still missing the millis(). I am still stuck on how to put this function in.

const int CAMERA = 0;
const int SHUTTER = 1;
const int IN = 4;

int val = 0;

void setup()
{
  pinMode(CAMERA, OUTPUT);
  pinMode(SHUTTER, OUTPUT);
  pinMode(IN, INPUT);
}

void loop()
{
  val = digitalRead(IN);
  if (val == 0)
  {
    digitalWrite(SHUTTER, LOW);
    digitalWrite(CAMERA, HIGH);
    //hold for 2 mins
    digitalWrite(CAMERA, LOW);
    //hold for 1 sec


  }
  else

    val = digitalRead(IN);
  if (val == 1)
  {
    {
      digitalWrite(CAMERA, HIGH);
      digitalWrite(SHUTTER, HIGH);
      delay(100);
      digitalWrite(SHUTTER, LOW);
      digitalWrite(CAMERA, LOW);
      delay(1000);

    }
  }

I've kept the delays in some spots as I want it to lock on those tasks. I really don't know what to do, so I have tried this.

const int CAMERA = 0;
const int SHUTTER = 1;
const int IN = 4;

unsigned long interval = 1000;
unsigned long interval2 = 120000;
unsigned long previousMillis = 0; 

int val = 0;

void setup()
{
  pinMode(CAMERA, OUTPUT);
  pinMode(SHUTTER, OUTPUT);
  pinMode(IN, INPUT);
}

void loop()
{
  val = digitalRead(IN);
  if (val == 0)
  {

    digitalWrite(SHUTTER, LOW);
    digitalWrite(CAMERA, HIGH);
    unsigned long currentMillis = millis();
    if ((unsigned long)(currentMillis - previousMillis) >= interval2)

    digitalWrite(CAMERA, LOW);
    delay(1000);
    previousMillis = millis();

  }
  else

    val = digitalRead(IN);
  if (val == 1)
  {
    {
      digitalWrite(CAMERA, HIGH);
      digitalWrite(SHUTTER, HIGH);
      delay(100);
      digitalWrite(SHUTTER, LOW);
      digitalWrite(CAMERA, LOW);
      delay(1000);

    }
  }
}

Not really getting anywhere. >:(

A) Arduino programmers usually avoid pins 0 and 1 because of the complications caused by sharing with the Serial port. Please change the pins that are used for CAMERA and SHUTTER.

B) You have numbered states already. That is great! I would suggest that the numbers go 100, 200, 300, etc. so that additional states can easily be added in-between, but it is not important.

C) Some aspects of your requirements are unclear to me. For example, what happens if the shutter gets triggered after step 4 in the loop when the CAMERA output is LOW but step 1 of the interruption requirements says that CAMERA should still be HIGH? What happens after step 4 in the loop? Step 1? But this would mean that CAMERA would go LOW only for microseconds. Is that your intent?

D) If it were up to me, I would use different numbers in the loop and in the interruption just to avoid confusion, but it is not important.

Thanks again,

I will change the pins.

So the numbers are OK as - is?

I don't want any interruption to the loop when the camera output goes LOW for that second, because if it did trigger, the camera would not be ready.

I will try the code again with different pins and see if it works, but I somehow doubt it.

Thanks

Back to the drawing board. I have made the changes to the pins and called the long 2 minute wait "ARMED".

Uploaded the code and all is does is flash the 2 LEDs every second. If I activate the input, it turns the SHUTTER output LOW and holds the CAMERA input high for a second.

I don't know what is wrong or how to fix it

const int CAMERA = 3;
const int SHUTTER = 4;
const int IN = 2;

unsigned long interval = 1000;
unsigned long ARMED= 120000;
unsigned long previousMillis = 0;

int val = 0;

void setup()
{
  pinMode(CAMERA, OUTPUT);
  pinMode(SHUTTER, OUTPUT);
  pinMode(IN, INPUT);
}

void loop()
{
  val = digitalRead(IN);
  if (val == 0)
  {

    digitalWrite(SHUTTER, LOW);
    digitalWrite(CAMERA, HIGH);
    unsigned long currentMillis = millis();
    if ((unsigned long)(currentMillis - previousMillis) >= ARMED)

    digitalWrite(CAMERA, LOW);
    delay(1000);
    previousMillis = millis();

  }
  else

    val = digitalRead(IN);
  if (val == 1)
  {
    {
      digitalWrite(CAMERA, HIGH);
      digitalWrite(SHUTTER, HIGH);
      delay(100);
      digitalWrite(SHUTTER, LOW);
      digitalWrite(CAMERA, LOW);
      delay(1000);

    }
  }
}

I am sorry that changing the pins did not work out for you.

With the requirements that you have posted and the bit that I have had to glean from the code that you posted and with few decisions to make, this code should practically write itself.

Unfortunately, I have some other priorities that I must attend to, so it may be about two days before I get back to this. I hope to try at that time.

Questions: The only input is called "IN" and its mode is set to INPUT rather than the more common INPUT_PULLUP. What is connected to this pin? Can you provide a schematic?

At the moment I am just running this on a breadboard with LEDs to indicate what each output is doing. I have a schematic in mind but can't test it until the code is doing what it is supposed to do...

LEDs are a good way to check things out, but you still have not explained what is connected to pin 4. Even if it is just a wire or a switch for the test, it needs a pullup or pulldown resistor.

What is currently connected?
What will be connected?
Do you want the shutter triggered when pin 4 goes HIGH or when pin 4 goes LOW?

I have posted some code below which may work for you.

Caveat:
The code compiles for an Uno using the Arduino IDE version 1.8.1. That does not mean that it will compile for an ATTiny, and that does not mean that it will execute correctly.

I salute you for working with an ATTiny. I have never tried to compile for an ATTiny.
An ATTiny does not have a Serial port, so I should not have mentioned pins 0 and 1. Sorry.

I do not have your hardware, so I have not tested this code.
Furthermore, some of my questions were never answered.
Furthermore, the requirements had some ambiguities in them.
Furthermore, the requirements had some contradictions in them.
Furthermore, I may have made mistakes.
This code is a starting point. You may have to adjust it for your purposes, but it may show you how to get from requirements to code.

There are some "else" statements that are empty (except for a comment). They could be omitted but I kept them to make it more clear what is happening.

A "switch/case" statement is just an advanced way of doing a series of "if/else if" statements.

Some of the variables and some of the states should have better names based on what the system does rather than how the system does something, but I did not have more information.

There are some things that I could have done to save space or save time, but I did not want to get too tricky... yet.

Is this code more complex than the original interrupt-based register-based code? Yes, but it will be easier to debug and the use of digitalRead(...), digitalWrite(...), and pinMode(...) makes the code more easily transferred to other types of Arduinos.

Good Luck!

const int CAMERA_PIN = 3;
const int SHUTTER_PIN = 4;
const int IN_PIN = 2;

//  These are times, specified in milliseconds.  Adjust as necessary.

const unsigned long ARMED        = 120000 ;
const unsigned long DISARMED     =    500 ;  //  Not specified, adjust as
                                             //  necessary.
const unsigned long SHUTTER_HOLD =    100 ;
const unsigned long CAMERA_HOLD  =   1000 ;

//  End of times.

/*
   Here is code for defining the states and making use of them,
   but there is a better way using enum that is shown lower down
   in a comment.
*/

const int SET_SHUTTER_LOW  = 100 ;
const int SET_CAMERA_HIGH  = 200 ;
const int WAIT_CAMERA_HIGH = 300 ;
const int SET_CAMERA_LOW   = 400 ;
const int WAIT_CAMERA_LOW  = 500 ;

const int MAKE_CAMERA_HIGH     = 1000 ;
const int MAKE_SHUTTER_HIGH    = 2000 ;
const int TIME_SHUTTER_HIGH    = 3000 ;
const int MAKE_SHUTTER_LOW     = 4000 ;
const int MAKE_CAMERA_LOW      = 5000 ;
const int TIME_CAMERA_LOW      = 6000 ;
const int NO_INTERRUPTION_YET  = 7000 ;

int loopState         = SET_SHUTTER_LOW  ;
int interruptionState = NO_INTERRUPTION_YET ;

/*
   End of code for defining the states and making use of them.
*/


/*
   Here is better code for defining the states and making use of them,
   and it has some advantages, but this code is more advanced and is
   therefore commented out.

  enum LoopStates {
  SET_SHUTTER_LOW,
  SET_CAMERA_HIGH, WAIT_CAMERA_HIGH,
  SET_CAMERA_LOW, WAIT_CAMERA_LOW
  } ;
  enum InterruptionStates {
  MAKE_CAMERA_HIGH, MAKE_SHUTTER_HIGH, TIME_SHUTTER_HIGH,
  MAKE_SHUTTER_LOW, MAKE_CAMERA_LOW, TIME_CAMERA_LOW,
  NO_INTERRUPTION_YET
  };

  LoopStates loopState = SET_SHUTTER_LOW ;
  InterruptionStates interruptionState = NO_INTERRUPTION_YET ;

   End of better code for defining the states and making use of them.

*/

unsigned long loopStartMillis ;
unsigned long interruptionStartMillis ;

void setup()
{
  pinMode(CAMERA_PIN, OUTPUT)  ;
  pinMode(SHUTTER_PIN, OUTPUT) ;
  digitalWrite(CAMERA_PIN, LOW)  ;
  digitalWrite(SHUTTER_PIN, LOW) ;

  pinMode(IN_PIN, INPUT);
  //  I am going to assume that there is a
  //  normally open (NO) pushbutton
  //  switch connected from this pin to the Vcc of the
  //  Arduino, and a pulldown resistor connected from
  //  this pin to ground (GND).
  //  Thus, there is an assumption that when the
  //  pushbutton is NOT pressed, this pin is LOW,
  //  and when the pushbutton is pressed,
  //  this pin is HIGH.

//  Serial.begin(115200) ;

}

void loop()
{
  unsigned long currentMillis = millis() ;  //  This variable is
                                            //  local to this function.
  switch (loopState) {
    case SET_SHUTTER_LOW:
      digitalWrite(SHUTTER_PIN, LOW) ;
      loopState = SET_CAMERA_HIGH ;
      break ;
    case SET_CAMERA_HIGH:
      digitalWrite(CAMERA_PIN, HIGH) ;
      loopStartMillis = currentMillis ;
      loopState = WAIT_CAMERA_HIGH ;
      break ;
    case WAIT_CAMERA_HIGH:
      if (currentMillis - loopStartMillis >= ARMED) {
        loopState = SET_CAMERA_LOW ;
      } else {
        while (triggered()) ;
      }
      break ;
    case SET_CAMERA_LOW:
      digitalWrite(CAMERA_PIN, LOW) ;
      loopStartMillis = currentMillis ;
      loopState = WAIT_CAMERA_LOW ;
      break ;
    case WAIT_CAMERA_LOW:
      if (currentMillis - loopStartMillis >= DISARMED) {
        loopState = SET_SHUTTER_LOW ;
      } else {
        //  Do nothing...  Keep waiting.
      }
      break ;
    default:
//    Serial.print("***** BAD loopState=") ;
//    Serial.println(loopState) ;
      while (true) ;  // Hang here forever, intentionally.
      break ;
  }
}

bool triggered() {
  unsigned long currentMillis = millis() ;  //  This variable is
                                            //  local to this function.
  bool result = false ;
  if ((interruptionState == NO_INTERRUPTION_YET) && (digitalRead(IN_PIN) == HIGH)) {
    interruptionState = MAKE_CAMERA_HIGH ;
    return true ;
  } else {
    switch (interruptionState) {
      case MAKE_CAMERA_HIGH:
        digitalWrite(CAMERA_PIN, HIGH) ; //  Unnecessary but harmless.
        interruptionState = MAKE_SHUTTER_HIGH ;
        result = true ;
        break ;
      case MAKE_SHUTTER_HIGH:
        digitalWrite(SHUTTER_PIN, HIGH) ;
        interruptionStartMillis = currentMillis ;
        interruptionState = TIME_SHUTTER_HIGH ;
        result = true ;
        break ;
      case TIME_SHUTTER_HIGH:
        if (currentMillis - interruptionStartMillis >= SHUTTER_HOLD) {
          interruptionState = MAKE_SHUTTER_LOW ;
        } else {
          //  Do nothing...  Keep waiting.
        }
        result = true ;
        break ;
      case MAKE_SHUTTER_LOW:
        digitalWrite(SHUTTER_PIN, LOW) ;
        interruptionState = MAKE_CAMERA_LOW ;
        result = true ;
        break ;
      case MAKE_CAMERA_LOW:
        digitalWrite(CAMERA_PIN, LOW) ;
        //  Requirements say to do
        //  this but it is
        //  in opposition to what the loop()
        //  function did ;
        interruptionStartMillis = currentMillis ;
        interruptionState = TIME_CAMERA_LOW ;
        result = true ;
        break ;
      case TIME_CAMERA_LOW:
        if (currentMillis - interruptionStartMillis >= CAMERA_HOLD) {
          digitalWrite(CAMERA_PIN, HIGH) ;
          interruptionState = NO_INTERRUPTION_YET ;
        } else {
          //  Do nothing...  Keep waiting.
        }
        result = true ;
        break ;
      case NO_INTERRUPTION_YET:
        result = false ;
        break ;
      default:
//      Serial.print("***** BAD interruptionState=") ;
//      Serial.println(interruptionState) ;
        while (true) ;  // Hang here forever, intentionally.
        break ;
    }
  }
  return result ;
}