Pages: [1]   Go Down
Author Topic: Serial data always -1 when nothing happening  (Read 999 times)
0 Members and 1 Guest are viewing this topic.
Offline Offline
Newbie
*
Karma: 0
Posts: 13
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

You guys have really helped me out so I firgured I would ask another question about another problem. Same project, different end.

I am counting the revolutions of a shaft per min which works fine but even while the shaft is not moving the serial still prints the last rpm that is registered before stopping. Also even if nothing is moving and I start the program the serial sends out -1 when it should be 0. Im sure this is an easy fix and I'm over looking something I know you pros will see probably immediately.

Code:
const int SENSOR_0 = 2;      //Moved to interrupt pin
const int SENSOR_1 = 3;      //Moved to interrupt pin
const int pulsesPerRev = 2;  //Number of magnets on the shaft
const long sendingInterval = 1000;    //ms between serial messages

long averageRpm_0 = 0;      
long averageRpm_1 = 0;
long previousMillisSend = 0;

unsigned long currentMillis = 0;
volatile long pulseDuration_0 = 0;
volatile long pulseDuration_1 = 0;
volatile long previousPulseTime_0 = 0;
volatile long previousPulseTime_1 = 0;

void setup()
{
  pinMode(SENSOR_0, INPUT);
  digitalWrite(SENSOR_0, HIGH);    //Set pullup resistor on input pin
  attachInterrupt(0, sensorISR_0, RISING);
      
  pinMode(SENSOR_1, INPUT);
  digitalWrite(SENSOR_1, HIGH);    //Set pullup resistor on input pin
  attachInterrupt(1, sensorISR_1, RISING);
              
  Serial.begin( 57600 );
}


void loop()
{
  currentMillis = millis();
  
  if(currentMillis - previousMillisSend > sendingInterval){    //Send rpm update to Adobe Air program
     previousMillisSend = currentMillis;
        
        
    averageRpm_0 = 1000 * 60 / pulsesPerRev / pulseDuration_0;
    averageRpm_1 = 1000 * 60 / pulsesPerRev / pulseDuration_1;
    
    
    //**counts to rpm math**        
     Serial.print(averageRpm_0, DEC);
     Serial.print("\t");
     Serial.print(averageRpm_1, DEC);
     Serial.println();
  }

}

void sensorISR_0()
{
   pulseDuration_0 = currentMillis - previousPulseTime_0;
   previousPulseTime_0 = currentMillis;
}

void sensorISR_1()
{
   pulseDuration_1 = currentMillis - previousPulseTime_1;
   previousPulseTime_1 = currentMillis;
}
« Last Edit: November 01, 2012, 10:27:24 am by 89ls1 » Logged

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 601
Posts: 48543
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Also even if nothing is moving and I start the program the serial sends out -1 when it should be 0.
Are you sure? -1 means that there is no data to read.

Make averageRpm_0 and averageRpm_1 unsigned to assure that they never go negative.
Logged

NL
Offline Offline
Newbie
*
Karma: 1
Posts: 30
Tech Tinkerer
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Do you get actual/meaningfull values when you print pulseDuration_0 & pulseDuration_1. The average beeing always -1 sounds like a overflow because of divide_by_0 or something like that.

Hope this helps,
Guido
Logged

Switzerland
Offline Offline
Faraday Member
**
Karma: 108
Posts: 5144
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
   averageRpm_0 = 1000 * 60 / pulsesPerRev / pulseDuration_0;

You're dividing by a value which you haven't tested against zero. A division by zero is not defined and your -1 may result from there. Also you're not resetting the pulseDuration_X variables, so they hold the last value till the next edge is reached.
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 13
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Hows the indenting looking this time around Paul?

Ok, made the sketch zero out pulseDuration_X is there has not been any pulses for greater than 5sec.

Code:
const int SENSOR_0 = 2;      //Moved to interrupt pin
const int SENSOR_1 = 3;      //Moved to interrupt pin
const int pulsesPerRev = 2;  //Number of magnets on the shaft
const long sendingInterval = 1000;    //ms between serial messages

long averageRpm_0 = 0;      
long averageRpm_1 = 0;
long previousMillisSend = 0;

unsigned long currentMillis = 0;
volatile long pulseDuration_0 = 0;
volatile long pulseDuration_1 = 0;
volatile long previousPulseTime_0 = 0;
volatile long previousPulseTime_1 = 0;

void setup()
{
  pinMode(SENSOR_0, INPUT);
  digitalWrite(SENSOR_0, HIGH);    //Set pullup resistor on input pin
  attachInterrupt(0, sensorISR_0, RISING);
      
  pinMode(SENSOR_1, INPUT);
  digitalWrite(SENSOR_1, HIGH);    //Set pullup resistor on input pin
  attachInterrupt(1, sensorISR_1, RISING);
              
  Serial.begin(57600);
}


void loop()
{
  currentMillis = millis();
  
  if(currentMillis - previousMillisSend > sendingInterval){    //Send rpm update to AIR program
     previousMillisSend = currentMillis;
        
        
    averageRpm_0 = (1000 * 60 / pulsesPerRev / pulseDuration_0)*(-1);
    averageRpm_1 = (1000 * 60 / pulsesPerRev / pulseDuration_1)*(-1);
    
    
    //**counts to rpm math**        
     Serial.print(averageRpm_0, DEC);
     Serial.print("\t");
     Serial.print(averageRpm_1, DEC);
     Serial.println();
    
   if(currentMillis - previousPulseTime_0 > 5000)
   {
     pulseDuration_0=0;
   }
   if(currentMillis - previousPulseTime_1 > 5000)
   {
     pulseDuration_1=0;
   }  
  }
}

void sensorISR_0()
{
   pulseDuration_0 = currentMillis - previousPulseTime_0;
   previousPulseTime_0 = currentMillis;
}

void sensorISR_1()
{
   pulseDuration_1 = currentMillis - previousPulseTime_1;
   previousPulseTime_1 = currentMillis;
}

Now if I spin the shaft it gives the proper values as well as stops sending crap when the shaft stops moving.
At little nit pick problem still is...
If the shaft slows and stops on its own, the program stops reading signal and zeros out the rpm reading. (As it should)
BUT... if the shaft is spinning and I bring it to a sudden stop then the program will continue to register a floating rpm value and does not zero out until I spin the shaft just enough to get a pulse, the it zeros out.

Hope I am being clear on what I'm trying to explain.
« Last Edit: November 01, 2012, 12:32:19 pm by 89ls1 » Logged

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 601
Posts: 48543
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
unsigned long currentMillis = 0;

void loop()
{
  currentMillis = millis();

void sensorISR_0()
{
   pulseDuration_0 = currentMillis - previousPulseTime_0;

void sensorISR_1()
{
   pulseDuration_1 = currentMillis - previousPulseTime_1;
Why isn't currentMillis volatile? More importantly, why isn't currentMillis WHEN the interrupt occurred?

Code:
    averageRpm_0 = (1000 * 60 / pulsesPerRev / pulseDuration_0)*(-1);
    averageRpm_1 = (1000 * 60 / pulsesPerRev / pulseDuration_1)*(-1);
Why are you multiplying by -1?

pulseDuration_N should always be positive, unless you've got a way-back machine hidden there somewhere. The other values can not be negative, either.
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 13
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

currentMillis is not volatile because the value is not being changed in the ISR. I was under the impression it only needed to be volatile if the variable was changed in a ISR. Maybe I should look up the definition of volatile and when to use it.

And why currentMillis is not in the ISR is because I don't know exactly what I'm doing..Learning as I go...

But from what I read in the reference section (http://www.arduino.cc/en/Reference/AttachInterrupt) It says: "Inside the attached function, delay() won't work and the value returned by millis() will not increment."
So I did not put currentMillis into the ISR due to that, maybe I am misunderstanding that comment. And I am using the currentMillis for a timer to send the serial data. If I move that variable to the ISR then will my sending interval timer even function properly since currentMillis would only be updated when an interrupt is triggered?

Now that you have brought up that question, it makes me think that just cause it won't increment, it will still give me the "exact" time that the interrupt was tripped.

Code:
averageRpm_0 = (1000 * 60 / pulsesPerRev / pulseDuration_0)*(-1);
 averageRpm_1 = (1000 * 60 / pulsesPerRev / pulseDuration_1)*(-1);
This is being multiplied by -1 so that my rpm comes out positive, otherwise I get a negative RPM reading. I could just take abs() but multiplying by -1 was so easy.
« Last Edit: November 01, 2012, 02:00:19 pm by 89ls1 » Logged

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 601
Posts: 48543
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
It says: "Inside the attached function, delay() won't work and the value returned by millis() will not increment."
This means that if your ISR takes a long time, the time returned by millis() at the start and the time returned by millis() at the end will be the same. It doesn't mean that you can't call millis() in the ISR.

Doing so would assure that the time associated with the last pulse was always correct, and currentMillis would not be needed as a global or volatile variable.
« Last Edit: November 01, 2012, 01:59:47 pm by PaulS » Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 13
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Thanks for clearing that up for me about the millis() in an ISR.

I moved currentMillis to the ISR's and added a currentMillisT variable for my sending interval timer. Also made currentMillis a volatile long and made the new currentMillisT an unsigned long.

Also got rid of the *-1 and replaced it with abs.
Code:
averageRpm_0 = abs(1000 * 60 / pulsesPerRev / pulseDuration_0);
averageRpm_1 = abs(1000 * 60 / pulsesPerRev / pulseDuration_1);
To make things look better. Have to get into the practice of proper coding, not make shift patching..

Do yall think there is a better way (I know there is, just don't know how to do it.) to implement the counting of the RPMs and to make it say 0 RPM when the shaft is not moving? It still has a small bug where if the shaft comes to a sudden stop it will keep reading an RPM although nothing is there.

UPDATE::

After testing my changes I just got random results. Here is the complete change
Code:
const int SENSOR_0 = 2;      //Moved to interrupt pin
const int SENSOR_1 = 3;      //Moved to interrupt pin
const int pulsesPerRev = 2;  //Number of magnets on the shaft
const long sendingInterval = 1000;    //ms between serial messages

unsigned long averageRpm_0 = 0;       
unsigned long averageRpm_1 = 0;
long previousMillisSend = 0;
unsigned long currentMillisT = 0;
volatile long currentMillis = 0;
volatile long currentMillis1 = 0;
volatile long pulseDuration_0 = 0;
volatile long pulseDuration_1 = 0;
volatile long previousPulseTime_0 = 0;
volatile long previousPulseTime_1 = 0;

void setup()
{
  pinMode(SENSOR_0, INPUT);
  digitalWrite(SENSOR_0, HIGH);    //Set pullup resistor on input pin
  attachInterrupt(0, sensorISR_0, RISING);
       
  pinMode(SENSOR_1, INPUT);
  digitalWrite(SENSOR_1, HIGH);    //Set pullup resistor on input pin
  attachInterrupt(1, sensorISR_1, RISING);
             
  Serial.begin(57600);
}


void loop()
{
  currentMillisT = millis();
   
  if(currentMillisT - previousMillisSend > sendingInterval){    //Send rpm update to AIR program
     previousMillisSend = currentMillisT;
         
         
    averageRpm_0 = abs(1000 * 60 / pulsesPerRev / pulseDuration_0);
    averageRpm_1 = abs(1000 * 60 / pulsesPerRev / pulseDuration_1);
       
    //**counts to rpm math**       
     Serial.print(averageRpm_0, DEC);
     Serial.print("\t");
     Serial.print(averageRpm_1, DEC);
     Serial.println();
     Serial.flush();
   
   if(currentMillisT - previousPulseTime_0 > 5000)
   {
     pulseDuration_0=0;
   }
   if(currentMillisT - previousPulseTime_1 > 5000)
   {
     pulseDuration_1=0;
   }   
  }
}

void sensorISR_0()
{
   currentMillis = millis();
   pulseDuration_0 = currentMillis - previousPulseTime_0;
   previousPulseTime_0 = currentMillis;
}

void sensorISR_1()
{
   currentMillis1 = millis();
   pulseDuration_1 = currentMillis1 - previousPulseTime_1;
   previousPulseTime_1 = currentMillis1;
}
« Last Edit: November 01, 2012, 02:56:40 pm by 89ls1 » Logged

Pittsburgh, PA, USA
Offline Offline
Faraday Member
**
Karma: 93
Posts: 4750
I learn a bit every time I visit the forum.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

millis() returns an unsigned long. All your time operations should be unsigned, with unsigned math you can subtract a bigger number from a smaller number and get a correct positive result.

It's like with a 12 hour clock, present time 3 - start time 10 = 5 hours difference. We don't care that it is -5 hours since we are looking at hours -ago- just by subtracting start from present. Unsigned math takes care of that for us; so in base 12 (12 hour clock), 10 - 3 = 7, 3 - 10 = 5.

With 32 bits you have base 4,294,967,296. That many millseconds takes over 47 days to roll over but when it does, signed math will fail without extra code that unsigned math doesn't need.

You want, I'll post a sketch made to experiment with that shows how the bits come out.

Logged

I find it harder to express logic in English than in Code.
Sometimes an example says more than many times as many words.

Offline Offline
Full Member
***
Karma: 3
Posts: 106
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

1000 is an int.
1000 * 60 will cause 60 to be treated as an int.
1000*60 is 60000 which is too large for an int (should be less than or equal to 32767) and will appear as a negative int.
You need to perform your arithmetic in a different order to ensure that you do not overflow.

Multiplying and dividing positive numbers and getting a negative result should be a clear sign (pun intended) that something is wrong.  Multiplying by -1 or taking the absolute value is not the correct approach.
Logged

Pittsburgh, PA, USA
Offline Offline
Faraday Member
**
Karma: 93
Posts: 4750
I learn a bit every time I visit the forum.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Multiplying by -1 or taking the absolute value is not the correct approach.

Too true, making a bug to fix a bug is almost common practice for beginners.
Logged

I find it harder to express logic in English than in Code.
Sometimes an example says more than many times as many words.

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 474
Posts: 18696
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

I was under the impression it only needed to be volatile if the variable was changed in a ISR.

Not quite. If the variable is changed outside an ISR, the compiler might just keep it in a register (not put it back into the actual variable). Thus, the ISR won't notice the change.
Logged

Pages: [1]   Go Up
Jump to: