Special 3 pin + ground encoder. Code works (sometimes?) Help?

I am trying to make a volume dial on some existing hardware work by outputting the up or down command to serial. The encoder is 3 pins and one ground pin. The encoder pins are sequenced (moved over once per click).

example rotate right and each line is a click…

100
010
001
100

example rotate left and each line is a click…

100
001
010
100
001

Here is the code. Maybe someone can show me a better way or help me understand why this code works but sometimes it don’t. The CPU is still running however. It does not crash or halt the CPU. I have also tried adding 50k pullup resistors to the inputs. Still works 80% of the time. 20% of the time I get nothing at all.

/*
Volume Encoder Pins are: D8,D9, D10

They are pulled to ground (sequenced) when knob is rotated.

*/

// Define volume pins
const int buttonPinA = 8;
const int buttonPinB = 9;
const int buttonPinC = 10;

// Define volume pin states
int buttonStateA = 0; 
int buttonStateB = 0; 
int buttonStateC = 0; 

// Define current volume pin states
int oldValA = 0;
int oldValB = 0;
int oldValC = 0;

// volume changed flag
boolean volChangeFlag = false;

void setup()
{
  Serial.begin(9600);
// Set volume inputs
  pinMode(buttonPinA, INPUT_PULLUP);
  pinMode(buttonPinB, INPUT_PULLUP);
  pinMode(buttonPinC, INPUT_PULLUP);

// Set current values
  oldValA = !digitalRead(buttonPinA);
  oldValB = !digitalRead(buttonPinB);
  oldValC = !digitalRead(buttonPinC);
  Serial.println("Ready");
}


void loop()
{
  volKnobCheck();
  test1();
  volKnobSet();
}

void volKnobCheck()
{
  buttonStateA = !digitalRead(buttonPinA);
  buttonStateB = !digitalRead(buttonPinB);
  buttonStateC = !digitalRead(buttonPinC);
  
  if(buttonStateA != oldValA) {
    zeroTest();
  }
  if(buttonStateB != oldValB) {
    zeroTest();
  }
  if(buttonStateC != oldValC) {
    zeroTest();
  }
}

void zeroTest() // removes the "000" read inbetween clicks
{
  if(buttonStateA >0 || buttonStateB >0 ||  buttonStateC >0){
    volChangeFlag = true;
  }
}

void test1()
{
  if(volChangeFlag == true){
    Serial.print(buttonStateA);
    Serial.print(buttonStateB);
    Serial.print(buttonStateC);
    Serial.print("    ");
    Serial.print(oldValA);
    Serial.print(oldValB);
    Serial.println(oldValC);
    //resetOldVal();
  }
}

void volKnobSet()
{
  if(volChangeFlag == true){
    if(buttonStateB == 1 && oldValC == 1){
      Serial.print("<volume:up>");
      resetOldVal();
    }
    if(buttonStateA == 1 && oldValB == 1){
      Serial.print("<volume:up>");
      resetOldVal();
    }
    if(buttonStateC == 1 && oldValA == 1){
      Serial.print("<volume:up>");
      resetOldVal();
    }
    
    if(buttonStateC == 1 && oldValB == 1){
      Serial.print("<volume:dn>");
      resetOldVal();
    }
    if(buttonStateB == 1 && oldValA == 1){
      Serial.print("<volume:dn>");
      resetOldVal();
    }
    if(buttonStateA == 1 && oldValC == 1){
      Serial.print("<volume:dn>");
      resetOldVal();
    }
  }
}

void resetOldVal()
{
  oldValA = buttonStateA;
  oldValB = buttonStateB;
  oldValC = buttonStateC;
  volChangeFlag = false;
}
  if(buttonStateA != oldValA) {

It's really hard to see a relationship between these variables. Now, if you used names like currStateA and prevStateA, the relationship would be easy to see.

  buttonStateA = !digitalRead(buttonPinA);
  buttonStateB = !digitalRead(buttonPinB);
  buttonStateC = !digitalRead(buttonPinC);

The digitalRead() function does not return a boolean. So, apply the not operator is not logical. It's also not necessary. There is no need to invert the return value. LOW is just as good a value as HIGH.

  if(buttonStateA != oldValA) {
    zeroTest();
  }
  if(buttonStateB != oldValB) {
    zeroTest();
  }
  if(buttonStateC != oldValC) {
    zeroTest();
  }

If all three values change, is it REALLY necessary to call zeroTest() three times? An if/else if structure seems more appropriate.

void test1()
{
  if(volChangeFlag == true){
    Serial.print(buttonStateA);
    Serial.print(buttonStateB);
    Serial.print(buttonStateC);
    Serial.print("    ");
    Serial.print(oldValA);
    Serial.print(oldValB);
    Serial.println(oldValC);
    //resetOldVal();
  }
}

during which time you'll be missing encoder state changes.

Calling the resetOldVal() function should NOT be conditional.

Some help HERE: and HERE:

I fixed the code using your suggestions however this part I do not understand…

PaulS:

  buttonStateA = !digitalRead(buttonPinA);

buttonStateB = !digitalRead(buttonPinB);
  buttonStateC = !digitalRead(buttonPinC);



The digitalRead() function does not return a boolean. So, apply the not operator is not logical. It's also not necessary. There is no need to invert the return value. LOW is just as good a value as HIGH.

I need the inputs to be inverted since they are pulled high and I need them to equal zero when high. Can you show me a better way by editing my code? Also the “test1” function is just for debugging. It will be removed.

/*
Volume Encoder Pins are: D8,D9, D10

They are pulled to ground (sequenced) when knob is rotated.

*/

// Define volume pins
const int buttonPinA = 8;
const int buttonPinB = 9;
const int buttonPinC = 10;

// Define volume pin states
int currStateA = 0; 
int currStateB = 0; 
int currStateC = 0; 

// Define current volume pin states
int prevStateA = 0;
int prevStateB = 0;
int prevStateC = 0;

// volume changed flag
boolean volChangeFlag = false;

void setup()
{
  Serial.begin(9600);
// Set volume inputs
  pinMode(buttonPinA, INPUT_PULLUP);
  pinMode(buttonPinB, INPUT_PULLUP);
  pinMode(buttonPinC, INPUT_PULLUP);

// Set current values
  prevStateA = !digitalRead(buttonPinA);
  prevStateB = !digitalRead(buttonPinB);
  prevStateC = !digitalRead(buttonPinC);
  Serial.println("Ready");
}


void loop()
{
  volKnobCheck();
  test1();
  volKnobSet();
}

void volKnobCheck()
{
  currStateA = !digitalRead(buttonPinA);
  currStateB = !digitalRead(buttonPinB);
  currStateC = !digitalRead(buttonPinC);
  
  if(currStateA != prevStateA) {
    zeroTest();
  }
  else if(currStateB != prevStateB) {
    zeroTest();
  }
  else if(currStateC != prevStateC) {
    zeroTest();
  }
}

void zeroTest() // removes the "000" read inbetween clicks
{
  if(currStateA > 0 || currStateB > 0 ||  currStateC > 0){
    volChangeFlag = true;
  }
}

void test1()
{
  if(volChangeFlag == true){
    Serial.print(currStateA);
    Serial.print(currStateB);
    Serial.print(currStateC);
    Serial.print("    ");
    Serial.print(prevStateA);
    Serial.print(prevStateB);
    Serial.println(prevStateC);
    //resetOldVal();
  }
}

void volKnobSet()
{
  if(volChangeFlag == true){
    if(currStateB == 1 && prevStateC == 1){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    if(currStateA == 1 && prevStateB == 1){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    if(currStateC == 1 && prevStateA == 1){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    
    if(currStateC == 1 && prevStateB == 1){
      Serial.print("<click:4150>");
      resetOldVal();
    }
    if(currStateB == 1 && prevStateA == 1){
      Serial.print("<click:4150>");
      resetOldVal();
    }
    if(currStateA == 1 && prevStateC == 1){
      Serial.print("<click:4150>");
      resetOldVal();
    }
  }
}

void resetOldVal()
{
  prevStateA = currStateA;
  prevStateB = currStateB;
  prevStateC = currStateC;
  volChangeFlag = false;
}

terryking228:
Some help HERE: and HERE:

terryking228, thank you for your help. I have looked at those and this encoder is different.

I need the inputs to be inverted since they are pulled high

No you don't. There is nothing stopping you using the LOW state to indicate that the button has been pressed. In fact it is the normal way of doing things.

So doing it like this correct?

/*
Volume Encoder Pins are: D8,D9, D10

They are pulled to ground (sequenced) when knob is rotated.

*/

// Define volume pins
const int buttonPinA = 8;
const int buttonPinB = 9;
const int buttonPinC = 10;

// Define volume pin states
int currStateA = 1; 
int currStateB = 1; 
int currStateC = 1; 

// Define current volume pin states
int prevStateA = 1;
int prevStateB = 1;
int prevStateC = 1;

// volume changed flag
boolean volChangeFlag = false;

void setup()
{
  Serial.begin(9600);
// Set volume inputs
  pinMode(buttonPinA, INPUT_PULLUP);
  pinMode(buttonPinB, INPUT_PULLUP);
  pinMode(buttonPinC, INPUT_PULLUP);

// Set current values
  prevStateA = digitalRead(buttonPinA);
  prevStateB = digitalRead(buttonPinB);
  prevStateC = digitalRead(buttonPinC);
  Serial.println("Ready");
}


void loop()
{
  volKnobCheck();
  test1();
  volKnobSet();
}

void volKnobCheck()
{
  currStateA = digitalRead(buttonPinA);
  currStateB = digitalRead(buttonPinB);
  currStateC = digitalRead(buttonPinC);
  
  if(currStateA != prevStateA) {
    zeroTest();
  }
  else if(currStateB != prevStateB) {
    zeroTest();
  }
  else if(currStateC != prevStateC) {
    zeroTest();
  }
}

void zeroTest() // removes the "000" read inbetween clicks
{
  if(currStateA < 1 || currStateB < 1 ||  currStateC < 1){
    volChangeFlag = true;
  }
}

void test1()
{
  if(volChangeFlag == true){
    Serial.print(currStateA);
    Serial.print(currStateB);
    Serial.print(currStateC);
    Serial.print("    ");
    Serial.print(prevStateA);
    Serial.print(prevStateB);
    Serial.println(prevStateC);
    //resetOldVal();
  }
}

void volKnobSet()
{
  if(volChangeFlag == true){
    if(currStateB == 0 && prevStateC == 0){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    if(currStateA == 0 && prevStateB == 0){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    if(currStateC == 0 && prevStateA == 0){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    
    if(currStateC == 0 && prevStateB == 0){
      Serial.print("<click:4150>");
      resetOldVal();
    }
    if(currStateB == 0 && prevStateA == 0){
      Serial.print("<click:4150>");
      resetOldVal();
    }
    if(currStateA == 0 && prevStateC == 0){
      Serial.print("<click:4150>");
      resetOldVal();
    }
  }
}

void resetOldVal()
{
  prevStateA = currStateA;
  prevStateB = currStateB;
  prevStateC = currStateC;
  volChangeFlag = false;
}

So doing it like this correct?

Not sure what you mean. The code is a complete nonsense as others have said. It just seems to be doing the same thing over and over and is very convoluted for what it does.

Nevertheless this bit:-

if(currStateA < 1 || currStateB < 1 ||  currStateC < 1)

will work although I would have put:-

if(currStateA == LOW || currStateB == LOW ||  currStateC == LOW)

It is much clearer what you are doing.

Grumpy_Mike:

So doing it like this correct?

Not sure what you mean. The code is a complete nonsense as others have said. It just seems to be doing the same thing over and over and is very convoluted for what it does.

I have taken all suggestions and implemented them. Maybe you can show me a better way and help me learn?

Mike, is this better or am I still on the wrong path here?

/*
Volume Encoder Pins are: D8,D9, D10

They are pulled to ground (sequenced) when knob is rotated.

*/

// Define volume pins
const int buttonPinA = 8;
const int buttonPinB = 9;
const int buttonPinC = 10;

// Define volume pin states
boolean currStateA = HIGH; 
boolean currStateB = HIGH; 
boolean currStateC = HIGH; 

// Define current volume pin states
boolean prevStateA = HIGH;
boolean prevStateB = HIGH;
boolean prevStateC = HIGH;

// volume changed flag
boolean volChangeFlag = false;

void setup()
{
  Serial.begin(9600);
// Set volume inputs
  pinMode(buttonPinA, INPUT_PULLUP);
  pinMode(buttonPinB, INPUT_PULLUP);
  pinMode(buttonPinC, INPUT_PULLUP);

// Set current values
  prevStateA = digitalRead(buttonPinA);
  prevStateB = digitalRead(buttonPinB);
  prevStateC = digitalRead(buttonPinC);
}


void loop()
{
  volKnobCheck();
  volKnobSet();
}

void volKnobCheck()
{
  currStateA = digitalRead(buttonPinA);
  currStateB = digitalRead(buttonPinB);
  currStateC = digitalRead(buttonPinC);
  
  if(currStateA != prevStateA || currStateB != prevStateB || currStateC != prevStateC) {
    if(currStateA == LOW || currStateB == LOW || currStateC == LOW){
      volChangeFlag = true;
    }
  }
}

void volKnobSet()
{
  if(volChangeFlag == true){
    if(currStateB == LOW && prevStateC == LOW  ||  currStateA == LOW && prevStateB == LOW || currStateC == LOW && prevStateA == LOW){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    else if(currStateC == LOW && prevStateB == LOW  || currStateB == LOW && prevStateA == LOW || currStateA == LOW && prevStateC == LOW){
      Serial.print("<click:4150>");
      resetOldVal();
    }
  }
}

void resetOldVal()
{
  prevStateA = currStateA;
  prevStateB = currStateB;
  prevStateC = currStateC;
  volChangeFlag = false;
}

You need to separate the "deal with the new encoder value" from the "get the current encoder value" part of the code. The mixed up code is just too hard to follow.

There is also too much use of global variables. volKnobCheck() (dumb name; knobs are for opening drawers) should return the current encoder value. loop() should compare that to the previous encoder value, to determine whether or not to call volKnobSet() (another dumb name; you are not setting the knob), and with what value (+1 for up; -1 for down).

PaulS, Maybe you can show me an example? I am not fully understanding this. The reason I don’t return a value is because the loop is triggered on a change. Then the loop runs till it finds that change and outputs the direction of the “click”. Maybe a while loop instead of another function looks better here? The output is a simple serial up or down button push for a car PC. No increment needed here.

I changed my function name to “readVolControl” and “outputVolChange”

The reason I don't return a value is because the loop is triggered on a change.

What loop is? loop() isn't.

This loops (called via the main loop) till the flag is cleared. The only was to clear the flag is when the change is sent via serial.

void outputVolChange()
{
  if(volChangeFlag == true){
    if(currStateB == LOW && prevStateC == LOW  ||  currStateA == LOW && prevStateB == LOW || currStateC == LOW && prevStateA == LOW){
      Serial.print("<click:4100>");
      resetOldVal();
    }
    else if(currStateC == LOW && prevStateB == LOW  || currStateB == LOW && prevStateA == LOW || currStateA == LOW && prevStateC == LOW){
      Serial.print("<click:4150>");
      resetOldVal();
    }
  }
}