Arduino Code Help

[klh2000]

I made a thread for this quite a while ago, but if felt that making a new one was best.

 

So, I'm having a problem with the code:

 

Spoiler

//-----------------------------------------------

 volatile byte half_revolutions;

 unsigned int rpm;

 unsigned long timeold;

 void setup()
 {
   Serial.begin(9600);
   attachInterrupt(0, rpm_fun, RISING);
 

   half_revolutions = 0;
   rpm = 0;
   timeold = 0;
 }

 void loop()
 {
   if (half_revolutions >= 8) {
     //Update RPM every 20 counts, increase this for better RPM resolution,
     //decrease for faster update
     rpm = 30*1000/(millis() - timeold)*half_revolutions;
     timeold = millis();
     half_revolutions = 0;
     Serial.println(rpm,DEC);
   }

{
   if (rpm >= 500);
    digitalWrite(7, HIGH);
}

 }

 void rpm_fun()
 {
   half_revolutions++;
   //Each rotation, this interrupt function is run twice
 }

//-----------------------------------------------

The code in the above spoiler works fine, when the detected rpm of a motor goes over 500, pin 7 goes high. I want to make pin 7 go low when the rpm goes below 500.

So I thought to try adding this:

Spoiler

{
  else (rpm <=500);

    digitalWrite(7, LOW);
 }

But, when I try to compile it I get this:

 

 

Any help would be greatly appreciated!

 

[Hackentosher]

I think that means you have to define rpm_fun before it is called to. I'm not sure though, I have no idea what I'm doIng in Arduino and I'm getting the same damn issue in my own arduino project.

[klh2000]

19 minutes ago, Hackentosher said:

I think that means you have to define rpm_fun before it is called to. I'm not sure though, I have no idea what I'm doIng in Arduino and I'm getting the same damn issue in my own arduino project.

That's what I thought, but I don't see why it only breaks when I put the else in. I wish this was just as easy as vb.net

[Hackentosher]

Just now, klh2000 said:

That's what I thought, but I don't see why it only breaks when I put the else in. I wish this was just as easy as vb.net

Yeah unfortunately there is no idiot proof guide to arduino like there is with computers.

[werto165]

digitalWrite(7, LOW ); at the start of the loop{}? or throw a static in front of that rpm_fun() ? 

[Factory Factory]

That's some weird formatting for C.

 

Do me a favor, try this as your loop():

 

void loop()
{
	if (half_revolutions >= 8) {
		//Update RPM every 20 counts, increase this for better RPM resolution,
		//decrease for faster update
		rpm = 30*1000/(millis() - timeold)*half_revolutions;
		timeold = millis();
		half_revolutions = 0;
		Serial.println(rpm,DEC);
	}
	if (rpm >= 500) { digitalWrite(7, HIGH); }
	else { digitalWrite(7, LOW); }
}

void rpm_fun()
{
	half_revolutions++;
	//Each rotation, this interrupt function is run twice
}

I've never seen arbitrary brackets placed the way you've put them. I'm pretty sure that you're not writing valid C code there. This in particular:

 

{
  else (rpm <=500);

    digitalWrite(7, LOW);
 }

Else is a comment-less statement in C. You can do "else if (condition)," but else just executes the next statement. So the compiler then just executes a check, gets a boolean value (which it does nothing with), and writes 7 LOW. It doesn't actually do that conditionally, it just does that. Perhaps it's screwing itself up there and interpreting what happens after incorrectly. Since it's not valid code, it could lead to undefined behavior.

 

Similarly your if statement isn't operating conditionally. It's just going "If true, then semicolon (statement ends)." Computer shrugs, moves on, and the next statement is executed, writing high.

 

If my version still doesn't work, add a declaration up above setup(). That is, add the line:

 

void rpm_fun();

... at the top above setup().

 

I don't know if the Arduino IDE requires it, but generally speaking C and C++ programs don't understand any function written after "main()" unless it has been declared, so it can't hurt.

[klh2000]

53 minutes ago, Factory Factory said:

That's some weird formatting for C.

 

Do me a favor, try this as your loop():

 

void loop()
{
	if (half_revolutions >= 8) {
		//Update RPM every 20 counts, increase this for better RPM resolution,
		//decrease for faster update
		rpm = 30*1000/(millis() - timeold)*half_revolutions;
		timeold = millis();
		half_revolutions = 0;
		Serial.println(rpm,DEC);
	}
	if (rpm >= 500) { digitalWrite(7, HIGH); }
	else { digitalWrite(7, LOW); }
}

void rpm_fun()
{
	half_revolutions++;
	//Each rotation, this interrupt function is run twice
}

I've never seen arbitrary brackets placed the way you've put them. I'm pretty sure that you're not writing valid C code there. This in particular:

 

{
  else (rpm <=500);

    digitalWrite(7, LOW);
 }

Else is a comment-less statement in C. You can do "else if (condition)," but else just executes the next statement. So the compiler then just executes a check, gets a boolean value (which it does nothing with), and writes 7 LOW. It doesn't actually do that conditionally, it just does that. Perhaps it's screwing itself up there and interpreting what happens after incorrectly. Since it's not valid code, it could lead to undefined behavior.

 

Similarly your if statement isn't operating conditionally. It's just going "If true, then semicolon (statement ends)." Computer shrugs, moves on, and the next statement is executed, writing high.

 

If my version still doesn't work, add a declaration up above setup(). That is, add the line:

 

void rpm_fun();

... at the top above setup().

 

I don't know if the Arduino IDE requires it, but generally speaking C and C++ programs don't understand any function written after "main()" unless it has been declared, so it can't hurt.

Ok, I will try that when I get home later.

[Unimportant]

void loop()
{
   if (half_revolutions >= 8) {
     //Update RPM every 20 counts, increase this for better RPM resolution,
     //decrease for faster update
     rpm = 30*1000/(millis() - timeold)*half_revolutions;
     timeold = millis();
     half_revolutions = 0;
     Serial.println(rpm,DEC);
   }

   {
   	if (rpm >= 500)
    		digitalWrite(7, HIGH);
  	else
          	digitalWrite(7, LOW);
   }
} 

 

[straight_stewie]

22 hours ago, klh2000 said:

{
  else (rpm <=500);

    digitalWrite(7, LOW);
 }

Should be what I've shown below, if you want to be explicit. Or if you want to be implicit about it, you can do what @Unimportant suggested. IMO explicit is always better than implicit.

 

{
	if (rpm >= 500);
    	// Your stuff here
        
    else (rpm < 500);
    	// Your stuff here
        
 }

 

When you run your statement and the RPM is at 500, both the if and the else statement will be true, which is logically impossible. If else is defined as Either this is true, or That is true, but not both.

 

Now, to fix your real problem: The simplest way to fix that (assuming that rpm_fun is only ever going to be half_revolutions++) you can just write the increment function right where you call it, being careful to make sure to use prefix incrementation instead of post fix incrementation. So the code would then look like:

 

 volatile byte half_revolutions;
 unsigned int rpm;
 unsigned long timeold;
 void setup()
 {
   Serial.begin(9600);
	// This line is where I made the change.
   attachInterrupt(0, ++half_revolutions, RISING);
 
   half_revolutions = 0;
   rpm = 0;
   timeold = 0;
 }
 void loop()
 {
   if (half_revolutions >= 8) {
     //Update RPM every 20 counts, increase this for better RPM resolution,
     //decrease for faster update
     rpm = 30*1000/(millis() - timeold)*half_revolutions;
     timeold = millis();
     half_revolutions = 0;
     Serial.println(rpm,DEC);
   }
{
   if (rpm >= 500);
    digitalWrite(7, HIGH);
}
 }

 

Beyond that, I'm not really sure why rpm_fun exists? It seems that you were only calling it in the one place, during setup (before any sensors are read or anything) so wouldn't you just be able to set it to a constant? Why are you incrementing half_revolutions right before setting half_revolutions to 0 anyway?

[klh2000]

On 9/21/2016 at 1:34 PM, straight_stewie said:

snip

 

{
	if (rpm >= 500);
    	// Your stuff here
        
    else (rpm < 500);
    	// Your stuff here
        
 }

 

 volatile byte half_revolutions;
 unsigned int rpm;
 unsigned long timeold;
 void setup()
 {
   Serial.begin(9600);
	// This line is where I made the change.
   attachInterrupt(0, ++half_revolutions, RISING);
 
   half_revolutions = 0;
   rpm = 0;
   timeold = 0;
 }
 void loop()
 {
   if (half_revolutions >= 8) {
     //Update RPM every 20 counts, increase this for better RPM resolution,
     //decrease for faster update
     rpm = 30*1000/(millis() - timeold)*half_revolutions;
     timeold = millis();
     half_revolutions = 0;
     Serial.println(rpm,DEC);
   }
{
   if (rpm >= 500);
    digitalWrite(7, HIGH);
}
 }

 

On 9/21/2016 at 0:50 PM, Unimportant said:

void loop()
{
   if (half_revolutions >= 8) {
     //Update RPM every 20 counts, increase this for better RPM resolution,
     //decrease for faster update
     rpm = 30*1000/(millis() - timeold)*half_revolutions;
     timeold = millis();
     half_revolutions = 0;
     Serial.println(rpm,DEC);
   }

   {
   	if (rpm >= 500)
    		digitalWrite(7, HIGH);
  	else
          	digitalWrite(7, LOW);
   }
} 

 

 

On 9/21/2016 at 6:01 AM, Factory Factory said:

snip

void loop()
{
	if (half_revolutions >= 8) {
		//Update RPM every 20 counts, increase this for better RPM resolution,
		//decrease for faster update
		rpm = 30*1000/(millis() - timeold)*half_revolutions;
		timeold = millis();
		half_revolutions = 0;
		Serial.println(rpm,DEC);
	}
	if (rpm >= 500) { digitalWrite(7, HIGH); }
	else { digitalWrite(7, LOW); }
}

void rpm_fun()
{
	half_revolutions++;
	//Each rotation, this interrupt function is run twice
}

 

void rpm_fun();

 

 

 

First of all, Thank you for your help everyone. I have got it working except one small thing.

I am using a relay, so inductive spikes from the coil are causing problems with the rpm input. I have tried using diodes on the input, and across the coil and bypass caps on the input, but it is still picking up the noise.

This might be because I'm using the digital pins for both the input and the output (different pins though). Is there a way to change the input from digital pin 2, to one of the analog input pins? The hall effect sensor just puts out 5 volts when the magnet goes by to sense the rpm, so it would just have to count the 5 volt pulses.

 

[straight_stewie]

10 hours ago, klh2000 said:

First of all, Thank you for your help everyone. I have got it working except one small thing.

What's the finished code look like?

 

10 hours ago, klh2000 said:

This might be because I'm using the digital pins for both the input and the output (different pins though). Is there a way to change the input from digital pin 2, to one of the analog input pins? The hall effect sensor just puts out 5 volts when the magnet goes by to sense the rpm, so it would just have to count the 5 volt pulses.

If the hall effect sensor truly only output 5 or 0 volts then a digital pin would be just fine. Can we please get a graph of the sensor output as well as a full schematic of your circuit?

[Factory Factory]

Saw this one on a Big Clive video today: try wiring in a common mode choke. One end goes between positive and hall effect sensor, and one end goes between ground and hall effect sensor. The signal inside the circuit should be preserved, but stray capacitance from outside the circuit should be rejected by the choke.

 

Alternatively, or if that doesn't work, you could switch to a solid state relay, or you could decouple the relay using an optoisolator to trigger a transistor that triggers the relay (along with decoupling capacitors on the power rail).

[Factory Factory]

Also, make sure your clamping diode is set up correctly. https://en.wikipedia.org/wiki/Flyback_diode

 

 

L in your case is the switching coil, and R would be a small-value resistor (like 0.01 Ohm), and pretend the switch t0 doesn't exist.