Needing two clicks to see one fire on encoder

Status
Not open for further replies.

rfresh737

Well-known member
I got my code working for a single rotary encoder, using interrupts.

I added a dual rotary encoder to my breadboard and have only one knob wired up at this point. It works like the single rotary encoder as I just edited the two A B pin values for the dual -- but -- my serial monitor shows a 'fire' on every other knob click instead of each knob click.

The single rotary encoder fired on each knob click but the dual rotary encoder won't do that. I must be missing something but can't think of what it might be.

Thanks for any help.

Sketch is attached.
 

Attachments

  • T3.5_Encoder_ISR1.ino
    2.7 KB · Views: 59
Might help to know which dual rotary encoder you are using, to maybe see the specs to know what the two pins do when you turn knob...
 
Code:
    if (digitalRead(PinB) == LOW)
    {
      virtualPosition-- ; // Could be -5 or -10
    }
    else
    {
      virtualPosition++ ; // Could be +5 or +10
    }

Your decode algorithm only decodes 1 encoder state out of the possible 4 states, does it not?

The encoder will move through states 00, 01, 11, 10, 00, 01 and so forth. Assuming that is pinB pinA, you are getting an interrupt when A goes low. Traversing up through those numbers, you get an interrupt when the code changes from 11 to 10 and that is decoded as ++. Going down through those numbers, you get an interrupt when the code change from 01 to 00 and that is decoded as --. You are missing 3 out of 4 gray code changes.
 
>Your decode algorithm only decodes 1 encoder state out of the possible 4 states, does it not?

Yes. Let me have a look at an encoder diagram and see if I can add 4 checks and figure this out.

I'm still waiting for the spec sheet from Propwash who sells these encoders.
 
You may want to use the encoder library or look at the code in encoder.h to see how they fully decode the states. Here are some of the comments in that code. The actual code appears to be hand optimized assembly code.

Code:
//                           _______         _______       
//               Pin1 ______|       |_______|       |______ Pin1
// negative <---         _______         _______         __      --> positive
//               Pin2 __|       |_______|       |_______|   Pin2

		//	new	new	old	old
		//	pin2	pin1	pin2	pin1	Result
		//	----	----	----	----	------
		//	0	0	0	0	no movement
		//	0	0	0	1	+1
		//	0	0	1	0	-1
		//	0	0	1	1	+2  (assume pin1 edges only)
		//	0	1	0	0	-1
		//	0	1	0	1	no movement
		//	0	1	1	0	-2  (assume pin1 edges only)
		//	0	1	1	1	+1
		//	1	0	0	0	+1
		//	1	0	0	1	-2  (assume pin1 edges only)
		//	1	0	1	0	no movement
		//	1	0	1	1	-1
		//	1	1	0	0	+2  (assume pin1 edges only)
		//	1	1	0	1	-1
		//	1	1	1	0	+1
		//	1	1	1	1	no movement
/*
	// Simple, easy-to-read "documentation" version :-)
	//
	void update(void) {
		uint8_t s = state & 3;
		if (digitalRead(pin1)) s |= 4;
		if (digitalRead(pin2)) s |= 8;
		switch (s) {
			case 0: case 5: case 10: case 15:
				break;
			case 1: case 7: case 8: case 14:
				position++; break;
			case 2: case 4: case 11: case 13:
				position--; break;
			case 3: case 12:
				position += 2; break;
			default:
				position -= 2; break;
		}
		state = (s >> 2);
	}
*/
 
@rcarr I'm still looking at your post and trying to understand it.

I have made some progress. My monitor output for turning the encoder cw (at this point) is:

Code:
Start
A low
B high
----one cw click ----
A high
B low
----one cw click ----
A low
B high
----one cw click ----
A low
B low
----one cw click ----
A low
B high
----one cw click ----
A high
B low
----one cw click ----
A low
B high
----one cw click ----
A high
B low
----one cw click ----

So, I'm now getting a response for each click (that's the progress) but the debug output doesn't give me anything that is consistent, so I'm not yet able to detect a cw vs ccw turn. I suspect I have to add those 4 pinA/pinB conditions before this will work properly.

Code:
// Used for generating interrupts using CLK signal
const int PinA = 6; 

// Used for reading DT signal
const int PinB = 7;  

// Used for the push button switch
const int PinSW = 5;  

// Simple PWM LED pin
//#define PinLED 11

// Keep track of last rotary value
int lastCount = 50;

// Updated by the ISR (Interrupt Service Routine)
volatile static int virtualPosition = 50;
volatile unsigned long lastInterruptTime = 0;
  
// ------------------------------------------------------------------
// INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT 
// ------------------------------------------------------------------
void isrPinA()
{
  static unsigned long lastInterruptTime = 0;
  unsigned long interruptTime = millis();

  // If interrupts come faster than 5ms, assume it's a bounce and ignore
  if (interruptTime - lastInterruptTime > 20)
  {
    if (digitalRead(PinA) == LOW)
    {
      virtualPosition++;
      Serial.println("A low");
      if (digitalRead(PinB) == LOW)
      {
        Serial.println("B low");
      }
      else
      {
        Serial.println("B high");
      }
    }
    else
    {
      virtualPosition--;
      Serial.println("A high");
      if (digitalRead(PinB) == LOW)
      {
        Serial.println("B low");
      }
      else
      {
        Serial.println("B high");
      }
    }
    // Restrict value from 0 to +100
    //virtualPosition = min(100, max(0, virtualPosition));
  }
    // Keep track of when we were here last (no more than every 5ms)
    lastInterruptTime = interruptTime;
}

// ------------------------------------------------------------------
// SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    
// ------------------------------------------------------------------
void setup()
{
  // Just whilst we debug, view output on serial monitor
  //Serial.begin(9600);
  //Serial.begin(38400);
  Serial.begin(115200);
    
  // Rotary pulses are INPUTs
  pinMode(PinA, INPUT_PULLUP);
  pinMode(PinB, INPUT_PULLUP);

  // Switch is floating so use the in-built PULLUP so we don't need a resistor
  pinMode(PinSW, INPUT_PULLUP);

  // Attach the routine to service the interrupts
  attachInterrupt(digitalPinToInterrupt(PinA), isrPinA, CHANGE);
  
  // Ready to go!
  Serial.println("Start");
}

// ------------------------------------------------------------------
// MAIN LOOP     MAIN LOOP     MAIN LOOP     MAIN LOOP     MAIN LOOP
// ------------------------------------------------------------------
void loop()
{
  // Is someone pressing the rotary switch?
  if ((!digitalRead(PinSW)))
  {
    virtualPosition = 50;
    while (!digitalRead(PinSW))
    {
      delayMicroseconds(10000);      
    }
    Serial.println("Reset");
  }

  // If the current rotary switch position has changed then update everything
  if (virtualPosition != lastCount)
  {
    // Our LED gets brighter or dimmer
    //analogWrite(PinLED, virtualPosition);

    // Write out to serial monitor the value and direction
    //Serial.print(virtualPosition > lastCount ? "Up  :" : "Down:");
    //Serial.println(virtualPosition);
    Serial.println("----one cw click ----");
    
    // Keep track of this new value
    lastCount = virtualPosition ;
  }
}
 
Propwash finally got their dual rotary encoder spec to me.

Thank you for any help.

encoder-phase.jpg
 
Good luck.

Just for general interest, the example from the library code I posted uses a switch/case to decode the 16 states that one gets when considering the current position and the previous position of the encoder. There is another way to decode the direction the shaft is turning and that is to xor the old pin1 state with the new pin2 state. This is a much simpler algorithm, but not as easy to understand when looking at the code 6 months after you wrote it.

Code:
//                           _______         _______       
//               Pin1 ______|       |_______|       |______ Pin1
// negative <---         _______         _______         __      --> positive
//               Pin2 __|       |_______|       |_______|   Pin2

		//	new	new	old	old
		//	pin2	pin1	pin2	pin1	Result                  New_pin2 xor Old_pin1
		//	----	----	----	----	------
		//	0	0	0	0	no movement
		//	0	0	0	1	+1                               1
		//	0	0	1	0	-1                               0
		//	0	0	1	1	+2  (assume pin1 edges only)     1
		//	0	1	0	0	-1                               0
		//	0	1	0	1	no movement
		//	0	1	1	0	-2  (assume pin1 edges only)     0
		//	0	1	1	1	+1                               1
		//	1	0	0	0	+1                               1
		//	1	0	0	1	-2  (assume pin1 edges only)     0
		//	1	0	1	0	no movement
		//	1	0	1	1	-1                               0
		//	1	1	0	0	+2  (assume pin1 edges only)     1
		//	1	1	0	1	-1                               0
		//	1	1	1	0	+1                               1
		//	1	1	1	1	no movement

(edit: I seem to have a problem with the fixed font and proportional font in the table, but hope you get the idea )
(edit2: attempted to fix using notepad )
 
I really don't follow your 16 states example. Sorry. I do understand the top part of it showing the pin1 and pin2 sign waves being created, but the new pin/old pin part I don't understand that (yet).

What I see in the Propwash encoder spec sheet is 4 states (T1..T4).

For CW:

T1 is pinA low/pinB high
T2 is pinA low/pinB low
T3 is pinA high/pinB low
T4 is pinA high/pinB high

So...just focusing on CW for the moment, wouldn't I just check those 4 states to determine a CW turn with each detent click?
 
I'm working on CW direction only at this time.

I have pinA set for two interrupts: to interrupt on FALLING and to interrupt on RISING. The thought being that pinA FALLING with pinB HIGH == CW and pinA RISING with pinB LOW == CW.

But in my serial window, all I see is pinA RISING. Why isn't pinA FALLING registering?

Code:
Uploading to I/O board
Opening port
Port open
A rising B low
A rising B low
A rising B high
A rising B low
A rising B low
A rising B low
A rising B low
A rising B high
A rising B low
A rising B high
A rising B low
A rising B high
A rising B low
A rising B low
A rising B low

Code:
const int PinA = 6; 
const int PinB = 7;  
const int PinSW = 8;  

volatile static int virtualPosition = 50;
volatile unsigned long lastInterruptTime = 0;

// ------------------------------------------------------------------
// SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    
// ------------------------------------------------------------------
void setup()
{
  Serial.begin(115200);
  pinMode(PinA, INPUT_PULLUP);
  pinMode(PinB, INPUT_PULLUP);
  pinMode(PinSW, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(PinA), isrPinAFalling, FALLING);
  attachInterrupt(digitalPinToInterrupt(PinA), isrPinARising, RISING);
}

// ------------------------------------------------------------------
// LOOP     LOOP     LOOP     LOOP     LOOP     LOOP     LOOP
// ------------------------------------------------------------------
void loop()
{
  // Is someone pressing the rotary switch?
  //if ((!digitalRead(PinSW)))
  //{
  //  virtualPosition = 50;
  //  while (!digitalRead(PinSW))
  //  {
  //    delayMicroseconds(3000);      
  //  }
  //  Serial.println("Reset");
  //}
}

// ------------------------------------------------------------------
// INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT 
// ------------------------------------------------------------------
void isrPinAFalling()
{
	static unsigned long lastInterruptTime = 0;
	unsigned long interruptTime = millis();
	if (interruptTime - lastInterruptTime > 5)
	{
			Serial.print("A falling ");
			if (digitalRead(PinB) == HIGH)
			{
				Serial.println("B high");
			}
			if (digitalRead(PinB) == LOW)
			{
				Serial.println("B low");
			}
	}
	lastInterruptTime = interruptTime;
}
void isrPinARising()
{
	static unsigned long lastInterruptTime = 0;
	unsigned long interruptTime = millis();
	if (interruptTime - lastInterruptTime > 5)
	{
		Serial.print("A rising ");
		if (digitalRead(PinB) == HIGH)
		{
			Serial.println("B high");
		}
		if (digitalRead(PinB) == LOW)
		{
			Serial.println("B low");
		}
	}
	lastInterruptTime = interruptTime;
}
 
In general, you do not want to do anything complicated in an interrupt function. You don't want to use delay, you don't want to call Serial, etc. The best thing to do is set a flag in the interrupt function, and then do the processing in the main loop code. Be sure to use the volatile keyword on the flag so that the compiler has to assume the flag will be set elsewhere (i.e. in the interrupt function).

Now there are places where you want to do complicated stuff (mostly things like issuing new DMA/communication requests when a buffer empties).
 
OK, I put just a flag into the ISR's and do the main work in the loop().

However, my serial window still shows only the Rising ISR activity.

Code:
A rising B high 152
A rising B low 153
A rising B high 154
A rising B low 155
A rising B high 156
A rising B high 157
A rising B low 158
A rising B high 159
A rising B low 160
A rising B high 161
A rising B high 162
A rising B high 163
A rising B high 164
A rising B high 165
A rising B low 166
A rising B low 167
A rising B high 168
A rising B low 169
A rising B high 170
A rising B low 171
A rising B high 172
A rising B high 173

Code:
const int PinA = 6; 
const int PinB = 7;  
const int PinSW = 8;  

long virtualPosition = 0;
volatile static int pinARising = 0;
volatile static int pinAFallinging = 0;

unsigned long lastInterruptTime = 0;

// ------------------------------------------------------------------
// SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    
// ------------------------------------------------------------------
void setup()
{
  Serial.begin(115200);
  pinMode(PinA, INPUT_PULLUP);
  pinMode(PinB, INPUT_PULLUP);
  pinMode(PinSW, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(PinA), isrPinAFalling, FALLING);
  attachInterrupt(digitalPinToInterrupt(PinA), isrPinARising, RISING);
}

// ------------------------------------------------------------------
// LOOP     LOOP     LOOP     LOOP     LOOP     LOOP     LOOP
// ------------------------------------------------------------------
void loop()
{
	unsigned long interruptTime = millis();
	if (pinAFallinging == 1)
	{
		if (interruptTime - lastInterruptTime > 5)
		{
			virtualPosition++;
			Serial.print("A falling ");
			if (digitalRead(PinB) == HIGH)
			{
				Serial.print("B high ");
			}
			if (digitalRead(PinB) == LOW)
			{
				Serial.print("B low ");
			}
			Serial.println(virtualPosition);
		}
		pinAFallinging = 0;
	}
	if (pinARising == 1)
	{
		if (interruptTime - lastInterruptTime > 5)
		{
			virtualPosition++;
			Serial.print("A rising ");
			if (digitalRead(PinB) == HIGH)
			{
				Serial.print("B high ");
			}
			if (digitalRead(PinB) == LOW)
			{
				Serial.print("B low ");
			}
			Serial.println(virtualPosition);
		}
		pinARising = 0;
	}
	lastInterruptTime = interruptTime;
	delay(20);
  // Is someone pressing the rotary switch?
  //if ((!digitalRead(PinSW)))
  //{
  //  virtualPosition = 50;
  //  while (!digitalRead(PinSW))
  //  {
  //    delayMicroseconds(3000);      
  //  }
  //  Serial.println("Reset");
  //}
}

// ------------------------------------------------------------------
// INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT 
// ------------------------------------------------------------------
void isrPinAFalling()
{
	pinAFallinging = 1;
}
void isrPinARising()
{
	pinARising = 1;
}
 
Not sure the same PinA can hold two _isr()'s - Falling and Rising - only the Rising is left after the second call.

Would have to use CHANGE and then read the current state perhaps. Of course depending on the change source it could have Bounce - could infer any change means it toggled from what it was.
 
I switched to using one ISR on PinA and one ISR on PinB.

The output windows shows mostly zero's but at least there is a response with each detent click.

encoderValue doesn't seem to change from 0.

Code:
const int PinA = 6; 
const int PinB = 7;  
const int PinSW = 8;  

volatile int lastEncoded = 0;
volatile long encoderValue = 0;
long lastencoderValue = 0;
int lastMSB = 0;
int lastLSB = 0;
int pinABInterrupted = 0;

// ------------------------------------------------------------------
// SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    SETUP    
// ------------------------------------------------------------------
void setup()
{
  Serial.begin(115200);
  pinMode(PinA, INPUT_PULLUP);
  pinMode(PinB, INPUT_PULLUP);
  pinMode(PinSW, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(PinA), updateEncoder, CHANGE);
  attachInterrupt(digitalPinToInterrupt(PinB), updateEncoder, CHANGE);
}

// ------------------------------------------------------------------
// LOOP     LOOP     LOOP     LOOP     LOOP     LOOP     LOOP
// ------------------------------------------------------------------
void loop()
{
	if (pinABInterrupted == 1)
	{
		int MSB = digitalRead(PinA); //MSB = most significant bit
		int LSB = digitalRead(PinB); //LSB = least significant bit

		int encoded = (MSB << 1) | LSB; //converting the 2 pin value to single number
		int sum = (lastEncoded << 2) | encoded; //adding it to the previous encoded value

		if (sum == 0b1101 || sum == 0b0100 || sum == 0b0010 || sum == 0b1011) encoderValue++;
		if (sum == 0b1110 || sum == 0b0111 || sum == 0b0001 || sum == 0b1000) encoderValue--;

		lastEncoded = encoded; //store this value for next time
		Serial.println(encoderValue / 4);
		pinABInterrupted = 0;
	}
	delay(50);
}

// ------------------------------------------------------------------
// INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT     INTERRUPT 
// ------------------------------------------------------------------
void updateEncoder()
{
	pinABInterrupted = 1;
}
 
Status
Not open for further replies.
Back
Top