# How to unify all RPS reading in one function?

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();

}

}
``````
• 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`. Commented Dec 9, 2016 at 10:15
• ok, I added rps_result calculation.
– zalt
Commented Dec 11, 2016 at 23:29
• So you mean I have to make rps_result as an array?
– zalt
Commented Dec 11, 2016 at 23:30
• @edgar could you please explain in detail which one should be as global array?
– zalt
Commented Dec 12, 2016 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
Commented Dec 12, 2016 at 5:03

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);
_lastTransitionTime = millis();
_rps = 0;
}
void calc() {
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)

• 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. Commented Dec 9, 2016 at 11:24
• Good point. Fixing now. Commented Dec 9, 2016 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. Commented Dec 9, 2016 at 11:26