Go Down

Topic: Serial data always -1 when nothing happening (Read 1 time) previous topic - next topic

PaulS

Code: [Select]
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: [Select]
    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.

89ls1

#6
Nov 01, 2012, 07:52 pm Last Edit: Nov 01, 2012, 08:00 pm by 89ls1 Reason: 1
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: [Select]

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.

PaulS

#7
Nov 01, 2012, 07:58 pm Last Edit: Nov 01, 2012, 07:59 pm by PaulS Reason: 1
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.

89ls1

#8
Nov 01, 2012, 08:36 pm Last Edit: Nov 01, 2012, 08:56 pm by 89ls1 Reason: 1
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: [Select]

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: [Select]

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;
}

GoForSmoke

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.

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

Go Up