coding problems

Question

I'm working on a Arduino controlled solenoid motor project that I found on Instructables. I have been able to piece together what I think is most of the code but I'm getting some syntax errors and was wondering if I could get some help? Here is the code and the errors.

   // Including Libraries
#include <Wire.h>                                   
#include <LiquidCrystal_I2C.h>                     
#include <math.h>                                   

LiquidCrystal_I2C mydisplay(0x27,16,2);             

// Define Input/Output Pins
#define solenoidpin 9                               
#define potpin 1                                    
#define ignitioninputpin 2                          
///#define interruptnumber 3                          
#define isr_rpm(INT0_vect)

// Defineing Variables
 int previousstate;                                  
 byte buttonstate;                                  
 int counter1;                                      
 int start;                                         
 int finish;                                        
 int elapsed;                                        
 int RPM;                                            

// Main setup
void setup(){ 
  pinMode(ignitioninputpin,INPUT);                  
  digitalWrite(ignitioninputpin,HIGH);              
////  attachInterrupt(3,isr_rpm, FALLING);              
  counter1=1;                                       

  mydisplay.init();                                 
  mydisplay.backlight();                            
  mydisplay.setCursor(0,1);                        
  mydisplay.print(" Soleniod Motor ");             

  previousstate=HIGH;                               
  Serial.begin(19200);                              
  buttonstate = digitalRead(ignitioninputpin);      
}

// Main program loop
void loop() {
  buttonstate = digitalRead(ignitioninputpin);      
  Serial.println(buttonstate);                      
  Serial.write("previousstate");                    
  Serial.println(previousstate);                    
  Serial.println(counter1);                         
  float elapsed=start=finish;                       
  float RPM= (6000/elapsed);                        
{

if  (buttonstate==LOW && previousstate==HIGH && (counter1%2)==0){    
    digitalWrite(solenoidpin,HIGH);
  }

if  (buttonstate==HIGH && previousstate==LOW && (counter1%2)==0){
    digitalWrite(solenoidpin,HIGH);                                 
  }

if  (buttonstate==LOW && previousstate==HIGH && (counter1%2)==0){   
    digitalWrite(solenoidpin,LOW);
  }

if  ((counter1%2)==0){                                            for rpm calcs
    finish=start;                                                  
    start=millis();
  }

if  ((counter1%6)==0){                                             
    mydisplay.setCursor(0,0);                                     
    mydisplay.print(" RPM ");                                     
    mydisplay.print(RPM);                                          
    mydisplay.setCursor(0,1);                                      
    mydisplay.print(" Solenoid Motor ");                           
  }

void ISR1();
  buttonstate=LOW;                                                
  counter1=counter1+1;                                            
  previousstate=HIGH;                                              
  }
 }

Error messages:

Binary sketch size: 7,594 bytes (of a 32,256 byte maximum)

Answer 1

Welcome to Arduino.SE. I am not going to rewrite the code for you but from this and other answers and comments you should be able to correct the code for yourself from constructive criticism.

To start at the beginning. You have defined interruptpinnumber but you don't use it in the attachInterrupt function as you declare interrupt pin 1(digital pin 3) and not 0 as the interrupt pin, this will cause problems in your code.

Next, for your timing variables, they need to be unsigned long as the millis() function will return unsigned long. There is also no need to declare them volatile as you are not using them in the ISR routine.

On the topic of the ISR, you have used the arduino attachInterrupt() and you have used the actual ISR() routine from the AVR libraries, also without including it. In the ISR you need to give it a vector which is not there and that is going to give errors later on. You also do not need the ISR routine if you have used the function from Arduino. You need to ensure that what you declare as the routine( that being isr_rpm) in the attachInterrupt() function to be executed when the interrupt occurs, actually exists.

Also there are some basic arithmetic issues with the elapsed time calculation, that is if you were trying to determine the difference.

As @Majenko also pointed out and I was going to too, that the use of a bitwise AND, & in your if() statements might not be a problem but best to learn now that you should use logical AND for comparison &&.

There is also a redundant if statement, that I will leave to you to find as it is your code and you can determine if it is of use or not. Also you need to keep track of you braces as this will cause the errors you are seeing too.

Edit

I think I need to elaborate on a few things as I think you got confused a bit more.

Concerning the whole arduino attachInterrupt function, the isr_rpm() needs to be a function and not a define, I think this caused it

In the ISR you need to give it a vector which is not there and that is going to give errors later on. You also do not need the ISR routine if you have used the function from Arduino. You need to ensure that what you declare as the routine( that being isr_rpm) in the attachInterrupt() function to be executed when the interrupt occurs, actually exists.

A function of that would be:

void isr_rpm(){

   //code

}

As @Majenko has kindly done you a favor and annotated your code, look at the stuff related to the interrupt routine (and other stuff) and compare it with the below avrfreaks tutorial, and the original code if you have it, the problem to begin with was that you had too many braces:

void ISR(){
      buttonState = LOW;
      counter1 = counter1 + 1;
      previousState = HIGH;
    }
    }

it should be:

ISR(INT0_vect){
      buttonState = LOW;
      counter1 = counter1 + 1;
      previousState = HIGH;
    }

There is no need to give your own special name to ISR either, this is from the newer code.

AVRfreaks tutorial concerning interrupts and the ISR stuff it will give you an explanation on the volatile stuff too.

I cannot stress the point more but, look through your code that Majenko has annotated and see where you can fix the code.

Answer 2

Ok, I swore I wasn't going to do this, but I have re-formatted your latest code and I am going to go through and annotate it.

// Including Libraries
#include <Wire.h>
#include <LiquidCrystal_I2C.h>
#include <math.h>  /// << WHY?!  You're not using it!

LiquidCrystal_I2C mydisplay(0x27, 16, 2);

// Define Input/Output Pins
#define solenoidpin 9
#define potpin 1
#define ignitioninputpin 2
///#define interruptnumber 3
#define isr_rpm(INT0_vect)  /// << What is this hoping to achieve?

// Defineing Variables
int previousstate;
byte buttonstate;
int counter1;
int start;  /// 
int finish; /// << These should be unsigned long
int elapsed;///
int RPM;

// Main setup
void setup() {
    pinMode(ignitioninputpin, INPUT);
    digitalWrite(ignitioninputpin, HIGH);
    ////  attachInterrupt(3,isr_rpm, FALLING);  /// << This will never work with that #define I pointed out...
    counter1 = 1;
    mydisplay.init();
    mydisplay.backlight();
    mydisplay.setCursor(0, 1);
    mydisplay.print(" Soleniod Motor ");
    previousstate = HIGH;
    Serial.begin(19200);
    buttonstate = digitalRead(ignitioninputpin);
}

// Main program loop
void loop() {
    buttonstate = digitalRead(ignitioninputpin);
    Serial.println(buttonstate);
    Serial.write("previousstate");
    Serial.println(previousstate);
    Serial.println(counter1);
    float elapsed = start = finish;  /// << You already defined these as ints in the global
    float RPM = (6000 / elapsed);    /// << scope. Why are you defining them again as floats?!  Also,
                                     /// << elapsed = start = finish?!?!  I think you have an = instead of a -
    { /// << Why are you starting a new block here???
        if (buttonstate == LOW && previousstate == HIGH && (counter1 % 2) == 0) {  /// << counter1 % 2 is very inefficient since counter1 is signed.
            digitalWrite(solenoidpin, HIGH);
        }

        if (buttonstate == HIGH && previousstate == LOW && (counter1 % 2) == 0) { /// << previousstate never changes from HIGH - this will NEVER run!
            digitalWrite(solenoidpin, HIGH);
        }

        if (buttonstate == LOW && previousstate == HIGH && (counter1 % 2) == 0) {  /// << You have already checked this state above with a different outcome.  WHY?!?!
            digitalWrite(solenoidpin, LOW);
        }

        if ((counter1 % 2) == 0) {
            for rpm calcs /// << This isn't even C!!!
            finish = start;

        start = millis();
        }

        if ((counter1 % 6) == 0) {
            mydisplay.setCursor(0, 0);
            mydisplay.print(" RPM ");
            mydisplay.print(RPM);
            mydisplay.setCursor(0, 1);
            mydisplay.print(" Solenoid Motor ");
        }

        void ISR1(); /// << This is a function prototype, not a function definition.
        buttonstate = LOW;       /// << I guess these should be inside
        counter1 = counter1 + 1; /// << a function called ISR1() but that
        previousstate = HIGH;    /// << function hasn't been written.
    }
}

You're trying to mess around writing low-level interrupt routines without having a clue how to work with low level interrupt routines. Go back and read the manuals on how to do external interrupts with Arduino. It's very very simple.

There are things about your program that just make me thing "What?!?!" and things that are just plain missing.