Edge detection in a function

Projects Programming
1

Hello All,

For a project I'm building right now. I need a lot of "edge detections". So I thought, lets build a function for it! But now I'm facing trouble with getting the "old" state bit static. for some reason it is overwritten by something. I'm I doing something wrong?

#define UP_PIN            10

static int memBit[9]; // membits to store the button state
       int up;        // var for input button

void setup() {
  pinMode(UP_PIN, INPUT);
  Serial.begin(9600);
}

void loop() {
  up = digitalRead(UP_PIN);
  
// working & tested code:
//  if (up != memBit[0]) {
//    if (up) {
//      Serial.println("ok");
//    }
//  } 
//  memBit[0] = up;

  if (risingEdge(up,&memBit[0])) {
    Serial.println("ok");
  }

}

bool risingEdge(int sig, int *memBit) {
  if (sig != *memBit) {
    if (sig) {
      return true;
    }
  } else {
    return false;
  }
  *memBit = sig;
}

With this i get a lot of "ok" in the serial monitor but not on the rising edge moment.

2

If you've got a global, why are you bothering with a pointer?
72 bits of precious RAM to store 9 bits of data seems wasteful to me.

3

Because I want to use (my guess now) 9 times the edge detection function. And for every button state I need a separate memory location.

Here my try without the pointers.. same result

#define UP_PIN            10

static int memBit[9]; // membits to store the button state
       int up;        // var for input button

void setup() {
  pinMode(UP_PIN, INPUT);
  Serial.begin(9600);
}

void loop() {
  up = digitalRead(UP_PIN);
  
// working & tested code:
//  if (up != memBit[0]) {
//    if (up) {
//      Serial.println("ok");
//    }
//  } 
//  memBit[0] = up;

  if (risingEdge(up,1)) {
    Serial.println("ok");
  }

}

bool risingEdge(int sig, int nr) {
  if (sig != memBit[nr]) {
    if (sig) {
      return true;
    }
  } else {
    return false;
  }
  memBit[nr] = sig;
}
4

MartijnD:
For a project I'm building right now. I need a lot of "edge detections".

What do you mean by "edge detections" ?

Do you just mean you want to check when an I/O pin goes from LOW to HIGH ?

oldVal = newVal;
newVal = digitalRead(pinNum);
if (newVal > oldVal) {
   // it has happened
}

...R

5

Ok... Lets say it this way:

Can anyone tell me why does this code work:

#define UP_PIN 10

static int memBit;
       int up;

void setup() {
  pinMode(UP_PIN, INPUT);
  Serial.begin(9600);
}

void loop() {
  up = digitalRead(UP_PIN); //input read out

// edge detection: 
  if (up != memBit && up) {
    Serial.println("ok");
  } 
  memBit = up; // save last state

}

and this code not:

#define UP_PIN 10

static int memBit;
       int up;

void setup() {
  pinMode(UP_PIN, INPUT);
  Serial.begin(9600);
}

void loop() {
  up = digitalRead(UP_PIN);

  if (risingEdge(up)) {
    Serial.println("ok");
  }

}

bool risingEdge(int sig) {
  if (sig != memBit && sig) {
    return true;
  } else {
    return false;
  }
  memBit = sig; save last state
}

@Robin2: Exectly detect if a input signal goes from LOW to HIGH (rising) or from HIGH to LOW (faling). In daily live I'm a PLC programmer we call this edge detection

___|-------|____

   ^       ^
rising   faling
 edge     edge
6 
  if (sig != memBit && sig) {
    return true;
  } else {
    return false;
  }
  memBit = sig; save last state

either the function will return true or it will return false.
It will never execute "memBit = sig;"

Pete

7

MartijnD:
@Robin2: Exectly detect if a input signal goes from LOW to HIGH (rising) or from HIGH to LOW (faling). In daily live I'm a PLC programmer we call this edge detection

Does the code I suggested work ?

...R

8

for every button state I need a separate memory location.

Actually you don't because either of 2 states can be stored in a single bit so you only need 9 bits for 9 buttons not 9 variables and even if you did they should be bytes not ints.

9

UKHeliBob:
Actually you don't because either of 2 states can be stored in a single bit so you only need 9 bits for 9 buttons not 9 variables and even if you did they should be bytes not ints.

I agree. But if you are not short of memory the programming will be much easier if you store the values as separate byte variables.

Personally I would not waste effort compacting the data into bits unless I had to, or if using bits actually simplified things - as it might do with port manipulation where you read 8 pins at once.

...R

10

<<< I want to use (my guess now) 9 times the edge detection function. >>>

If you only need to detect state changes (any edge) of each input without needing to know if the status is high or low, then using bit math could be useful and actually simplify your code.

Suggestion:
You can use one variable (var) ... unsigned short or word (16 bit).
Then left shift by 1 each of your 9 readings into this variable.
The lower 9 bits will now contain your current readings.
Use an Exclusive NOR operation !^ on var with varPrevious.
The returned result will have bits set for any input that has changed.

11

el_supremo:
either the function will return true or it will return false.
It will never execute "memBit = sig;"

Pete

Thanks Pete! of course, how could I have missed this... :sleeping: after a "return" instruction the current active function immediately ends...

Robin2:
Does the code I suggested work ?..

Sorry I didn't try the code... at first sight I did not see the "real" difference with my code. Now I do thanks for that one! And sorry for ignoring your good sign!

UKHeliBob:
Actually you don't because either of 2 states can be stored in a single bit so you only need 9 bits for 9 buttons not 9 variables and even if you did they should be bytes not ints.

I agree with that but it is almost not possible to return 2 vars. It could be done with a "struct", but then it is not usable in a "if" comparing.

My test code looks now like this:

#define UP_PIN            10
#define DOWN_PIN          11

static int memBit[4];
       int up;        // var for input button
       int down;      // var for input button
  
void setup() {
  pinMode(UP_PIN, INPUT);
  pinMode(DOWN_PIN, INPUT);
  Serial.begin(9600);
}

void loop() {
  up   = digitalRead(UP_PIN);
  down = digitalRead(DOWN_PIN);

  if (risingEdge(up, 0)) {
    Serial.println("rising edge Button Up");
  }
  if (falingEdge(up, 1)) {
    Serial.println("faling edge Button Up");
  }
  
  if (risingEdge(up, 2)) {
    Serial.println("rising edge Button Down");
  }
  if (falingEdge(up, 3)) {
    Serial.println("faling edge Button Down");
  }

}

bool risingEdge(int sig, int nr) {
  if (sig != memBit[nr] && sig) {
    memBit[nr] = sig;
    return true;
  } else {
    memBit[nr] = sig;
    return false;
  }
}

bool falingEdge(int sig, int nr) {
  if (sig != memBit[nr] && !sig) {
    memBit[nr] = sig;
    return true;
  } else {
    memBit[nr] = sig;
    return false;
  }
}

Any suggestions? So I can Learn more :wink:

Thank you all!

12

It could be done with a "struct", but then it is not usable in a "if" comparing.

Why not? You can compare any element of a struct.

13

PaulS:
Why not? You can compare any element of a struct.

that's true, but in my opinion it losses the "ease of use" of my function. Then you'll get something like this:

struct edge {
  bool rising;
  bool faling;
};

void setup() { 
  // what ever comes here 
}

void loop() {
  struct edge tmpVal = edge(button);
  if (tmpVal.rising){
    // do this
  }
}

struct edge edge(int input) {
  struct edge output;
  // the code
  return output;
}
14

If your are only interested in detecting a rising edge, then your rising edge function could take a pin number and a place to store the previous state. That way, the reading of the pin and the "what to do about it" is in one spot:

boolean detectEdge(int pin, boolean *prevousState) {
}

But there's only a few pins, numbered 0-14. It's really no skin off anyone's nose to just hold an array of 14 booleans and use that for all the pins. That way, you are not passing around pointers.

boolean pinState[15]; // 0-14
boolean detectEdge(int pin) {
  boolean oldState = pinState[pin];
  boolean newState = digitalRead(pin);
  pinState[pin] = newState;
  return !oldState && newState;
}

As to jamming all of that into fewer lines with clever C: I think it's do-able, but not worth it. The problem is that && has short-cut evaluation, so one clause of it never gets evaluated; but bit fiddling does not guarrante order of evaluation, so if one clause reads the pin and the other updates, you can't guarrantee that it works.

IOW, in this:

return !pinState[pin] && (pinState[pin] = digitalRead(pin));

if the pin state is high, then the code that sets it to low never gets executed; and in this:

return ~pinState[pin] & (pinState[pin] = digitalRead(pin));

The compiler is free to evaluate the right-hand side before the left, meaning that the function will always return false. I mean - it would probably work, but you can't be sure.

15

If you want to go more heavyweight, your could create a C++ class to bundle together the state and the functionality.

class RisingEdgeDetector {
    int pin;
    boolean previousState;

  public:
    RisingEdgeDetector(int _pin) {
      pin = _pin;
      // initial state.
      // possibly this shouldn't be being done here
      previousState = digitalRead(pin);
    }

    boolean isRising() {
      boolean oldState = previousState;
      boolean newState = digitalRead(pin);
      previousState = newState;
      return !oldState && newState;
    }
};

RisingEdgeDetector greenDetector(9), redDetector(7);

void setup() {
}

void loop() {
  if (greenDetector.isRising()) {
    // flash the green LED, or whatever
  }
}
16

Might need to consider debouncing (could be part of your function), unless your input signals are consistently clean.