0

Want to unify rps reading in one function. However, it shows incorrect values. when it is reading for one it works fine. Following is the code. Appreciate for your help.

float rps[4] = {0.00, 0.00, 0.00, 0.00};
long int PRINT_PERIOD = 1000;
long int lastTimePrinted = 0;

void setup() {
    Serial.begin(9600);
    DDRD &= ~_BV(7); // pinMode(7, INPUT);     // rps_1
    DDRB &= ~_BV(0); // pinMode(8, INPUT);     // rps_2
    DDRC &= ~_BV(1); // pinMode(A1, INPUT);    // rps_3
    DDRD &= ~_BV(2); // pinMode(2, INPUT);     // rps_4
}

float rps_out(int k) {
    static int lastReedState;
    static unsigned long lastTransition;
    static float rps_result;
    int reedState;
    unsigned long now;
    unsigned long revolutionTime;

    if (k == 1)    reedState = (PIND & _BV (7)) == 0; // digitalRead(7);
    else if (k==2) reedState = (PINB & _BV (0)) == 0; // digitalRead(8);
    else if (k==3) reedState = (PINC & _BV (1)) == 0; // digitalRead(A1);
    else if (k==4) reedState = (PIND & _BV (2)) == 0; // digitalRead(2);

    // HIGH LOW measuring the low period
    // LOW HIGH measuring the high period
    // On a rising transition of the reed switch:
    if (reedState == HIGH && lastReedState == LOW) {

        // Compute time since last valid transition.
        now = micros();
        revolutionTime = now - lastTransition;    // measuring the High period
         rps_result = 1.00 / (12.00*revolutionTime) *1000000.00;  // 12 is rotary encoders resolution 
        // Remember this transition.
        lastTransition = now;
    }

    // Remember last state.
    lastReedState = reedState;

    return rps_result;
}

void loop() {
    rps[0] = rps_out(1);
    rps[1] = rps_out(2);
    rps[2] = rps_out(3);
    rps[3] = rps_out(4);

    if (millis() - lastTimePrinted >= PRINT_PERIOD) {
        Serial.println(rps[0]);
        Serial.print(',');
        Serial.println(rps[1]);
        Serial.print(',');
        Serial.print(rps[2]);
        Serial.print(',');
        Serial.println(rps[3]);
        lastTimePrinted = millis();
    }
}

worked code except for rps[3] at the beginning. it shows rps[3] = 672.04

float rps[4];
long int PRINT_PERIOD = 100;
long int lastTimePrinted = 0;
int lastReedState[4];
unsigned long lastTransition[4];
int reedState[4];
unsigned long now[4]; 
float rps_result[4];   
unsigned long revolutionTime[4];

void setup(){

 Serial.begin(9600);

 DDRD &= ~_BV (7); // pinMode (7, INPUT);       // rps_1
 DDRB &= ~_BV (0); // pinMode (8, INPUT);       // rps_2
 DDRC &= ~_BV (1); // pinMode (A1, INPUT);      //  rps_3
 DDRD &= ~_BV (2); // pinMode (2, INPUT);       //  rps_4


}


float rps_out(int k){

    //static int lastReedState;
    //static unsigned long lastTransition;
    //static float rps_result; 
    //float rps_result;
    //int reedState;
    //unsigned long now;    
    //unsigned long revolutionTime;

   if ( k == 0 )     reedState[0] = (PIND & _BV (7)) == 0; // digitalRead (7);
   else if ( k==1 )  reedState[1] = (PINB & _BV (0)) == 0; // digitalRead (8);
   else if ( k==2 )  reedState[2] = (PINC & _BV (1)) == 0; // digitalRead (A1);
   else if ( k==3 )  reedState[3] = (PIND & _BV (2)) == 0; // digitalRead (2);

    // On a rising transition of the reed switch:
    if (reedState[k] == HIGH && lastReedState[k] == LOW) {                   // HIGH  LOW   measuring the low period    // LOW HIGH measuring the high period 
        // Compute time since last valid transition.
        //unsigned long now = micros();
        //unsigned long revolutionTime = now - lastTransition;       // measuring the High period
         now[k] = micros();
         revolutionTime[k] = now[k] - lastTransition[k];       // measuring the High period
        // Compute the rps

        if(revolutionTime[k] == 0 ) rps_result[k] = 0.00;     // avoid deviding by zero


        else rps_result[k] = 1.00 / (12.00*revolutionTime[k]) *1000000.00;  // 12 is rotary encoders resolution 

        // Remember this transition.
        lastTransition[k] = now[k];   

    }

    // Remember last state.
    lastReedState[k] = reedState[k];


    return rps_result[k]; 

}


void loop() {

   rps[0] = rps_out(0);
   rps[1] = rps_out(1);
   rps[2] = rps_out(2);
   rps[3] = rps_out(3);

    if (millis() - lastTimePrinted >= PRINT_PERIOD) {   

     Serial.print(rps[0]);
     Serial.print(',');
     Serial.print(rps[1]); 
     Serial.print(',');
     Serial.print(rps[2]);
     Serial.print(',');
     Serial.println(rps[3]);

      lastTimePrinted = millis();

     }

}
8
  • 1
    Remember your question Getting different results from function? Same problem here: rps_out() has state (its static variables), and the state should be stored independently for each channel. Same solutions as in the answers you already got. Oh, and you forgot to actually compute rps_result. – Edgar Bonet Dec 9 '16 at 10:15
  • ok, I added rps_result calculation. – zalt Dec 11 '16 at 23:29
  • So you mean I have to make rps_result as an array? – zalt Dec 11 '16 at 23:30
  • @edgar could you please explain in detail which one should be as global array? – zalt Dec 12 '16 at 0:26
  • I have done it, except for the last one rps[3]. it is supposed to be rps[3].=0.00 at the beginning. However, it shows rps[3] = 672.04. What is wrong? – zalt Dec 12 '16 at 5:03
1

Your problem is that you have one set of (static) state information that you are trying to use for all your inputs. That's never going to work. You need separate state information for each and every input.

There's numerous ways of doing that:

  1. The traditional C library "re-entrant" style. This involves passing pointers to any state variables into your function. The caller is responsible for creating and managing these variables and passing the pointers when the function is called. This is messy but pretty much completely portable across systems since it doesn't rely on any C++ facilities.
  2. Similar to option 1 but grouping your different state variables into a struct to make things cleaner.
  3. Global arrays - have a global array (maybe of structs as in option 2) to store the state information and pass the slice number to your function.
  4. The OOP (C++) way - create a class with all the state variables and functions within it then instantiate a new object for each input. This is by far the cleanest and most self contained way but does require knowledge of creating and using classes.

Of the four options I would most likely choose option 4:

class RPS {
    private:
        uint8_t _pin;
        uint8_t _lastReedState;
        uint32_t _lastTransitionTime;
        uint32_t _rps;

    public:
        RPS(uint8_t pin) : _pin(pin) {}
        void begin() {
            pinMode(_pin, INPUT);
            _lastReedState = digitalRead(_pin);
            _lastTransitionTime = millis();
            _rps = 0;
        }
        void calc() {
            uint8_t reedState = digitalRead(_pin);
            if (reedState != _lastReedState) {
                uint32_t now = millis();
                _lastReedState = reedState;
                if (reedState == HIGH) {
                    uint32_t diff = now - _lastTransitionTime;
                    if (diff == 0) {
                        _rps = 0; // Avoid divide by zero
                    } else {
                        _rps = 1000 / diff;
                    }
                }
                _lastTransitionTime = now;
            }
        }
        uint32_t get() {
            return _rps;
        }
};

RPS rps1(1);
RPS rps2(2);

#define PRINT_PERIOD 1000

void setup() {
    rps1.begin();
    rps2.begin();
}
void loop() {
    static uint32_t lastTimePrinted = 0;

    rps1.calc();
    rps2.calc();

    if (millis() - lastTimePrinted >= PRINT_PERIOD) {
        lastTimePrinted = millis();
        Serial.print("RPS 1: ");
        Serial.println(rps1.get());
        Serial.print("RPS 2: ");
        Serial.println(rps2.get());
    }
}

(note: Untested. It may not even compile)

3
  • Your get() method has to be called as often as possible in order to get an accurate reading of the transition time, but your loop() only calls it one per second. You may want to split this method into update() (takes the reading, to be called often) and read() (just returns _rps). You may also want to use micros() instead of millis(), for better resolution. – Edgar Bonet Dec 9 '16 at 11:24
  • Good point. Fixing now. – Majenko Dec 9 '16 at 11:25
  • As to millis/micros, that's up to the end user to choose. This just gives a framework for them to build on to demonstrate the concept. If you feel the need to use microseconds then you won't be wanting to use micros() since it is only a rough guestimate at the best of times. Using IC would be preferable. – Majenko Dec 9 '16 at 11:26

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.