-2

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)

  • A good starting place may be: "'isr_rpm' was not declared in this scope" That should be fixable. Then .. – Russell McMahon Jul 5 '15 at 4:46
  • Another thing you might consider is losing the extraneous spaces in your include filenames. <[space]Wire.h[space]> is not the same as <Wire.h> – Majenko Jul 5 '15 at 9:50
  • 1
    Another thing to watch out for is making sure you balance your brackets properly. You seem to have an extra one there. Fix your indenting and you'll soon spot it. – Majenko Jul 5 '15 at 9:52
  • 1
    @EdgarBonet The standard avr-gcc. The compiler has no concept of what % is. It just knows that to do a % operation it has to call the libc function that performs that operation. It can't know that, just because the second argument is a 2 that it'd be better done with a completely different set of operations since it doesn't know what % does. Just like if you call "sin(0)" it can't know that it'd be better to substitute that with "0" since it doesn't know what sin() does. It's just a function, which is all % is. To the compiler it's the same as %(counter1, 2) – Majenko Jul 5 '15 at 10:38
  • 2
    @KeyPrice Since your last edit you have made some fundamental errors with your program. Most notably with your ISR1() function. Fix your indenting so you can see where the error lies. Good indenting makes for easy to understand programs. You cannot understand your program because you have a very broken layout. Start with fixing the layout and you will see your errors as plain as day. If you can't fix your layout then use an autoformatter to do it for you. – Majenko Jul 5 '15 at 18:24
3

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.

  • Thanks guys so much, I have been able to fix about 90% of it and I think I'm down to just a few little brackets and indents and such. after having about a pot of coffee and reading some more reference material along with all of your comments I chose to stick with the actual ISR(). Majenko thanks for mentioning the curly brackets it helped a lot and RSM i could not have made it through the morning with out the coffee and your guidance. as soon as i figure out how the formatting on these posts go i will put up the new and improved code. – Key Price Jul 5 '15 at 16:47
  • @KeyPrice - I am sure I helped but I should have possibly been more exact about the use of an ISR as I think that has lead to the completely new code problems. I would review the annotated code by Majenko and compare that with my notes. Also if you have your original code also look at that as that had some formatting errors mainly and the ISR would have been fine if you put the vector in those brackets. Have she more coffee and read through the ISR() link in my post. Also try avrfreaks.net/forum/tut-newbies-guide-avr-interrupts?page=all. It is a good tutorial. – RSM Jul 5 '15 at 18:58
1

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.