AVR ISR Rotary Encoder difficulties

[Mastrofski]

Admittedly, it's been a while since I've done any microcontroller work, but I'm running into an issue with a project I'm working on. I'm simply trying to read a rotary encoder value through a Pin Change interrupt. I borrowed the execution of the code from this open source power supply. Where he's using object oriented programming to store the values, I'm just using plain ole C. I'd think the premise should be the same, but I'm not having any luck. The code of interest is as follows:

Code: [Select]

//INTERRUPT SERVICE ROUTINES
ISR(PCINT2_vect){ //PCINT2 register contains PCINT18 and PCINT19
//Channel A on PCINT18, Channel B on PCINT19

//Determine Change
static uint8_t encoderVal = 0x00; //Variable to store Encoder Value, init at 0x00

//Shift previous value to upper half of nibble
//Complete Nibble by Or-ing Rotary Encoder
//Mask off upper nibble of byte
encoderVal = ((encoderVal << 2) | (PIND & ((1<<PIND3)|(1<<PIND2)))) & 0x0F;

switch(encoderVal){
case 0x01: //prev 00, current 01
//Fall Through
case 0x07: //prev 01, current 11
//Fall Through
case 0x08: //prev 10, current 00
//Fall Through
case 0x0E: //prev 11, current 10
voltageSet--; //CCW Movement, Decrement
PORTD ^= (1<<PORTD6); //TOGGLE GREEN LED -- TROUBLESHOOTING
break;
case 0x02: //prev 00, current 10
//Fall Through
case 0x04: //prev 01, current 00
//Fall Through
case 0x0B: //prev 10, current 00
//Fall Through
case 0x0D: //prev 11, current 01
voltageSet++; //CW Movement, Increment
PORTD ^= (1<<PORTD7); //TOGGLE RED LED -- TROUBLESHOOTING
break;
}// End Switch Case

}//End ISR
As it stands, the troubleshooting LEDs are toggling, as would be expected, but both of them are going on in regardless of which direction the encoder is turned, just out of phase, which is leading me to believe that there's something wrong with the logic of my switch case. I'm thinking that perhaps declaring the variable inside the ISR might be causing some issue(again, it's been a while since I've worked with Microcontrollers), but playing with the variable type and scope haven't yielded the results I'm looking for, nor would it explain both of the LEDs turning on out of phase. Channel A of the encoder is on PIND2, and channel B is on PIND3. I have verified that the Interrupt settings are accurate(as could be assumed given that the LEDs are turning on).

I'm hoping that one of you fine people might be able to lend me an extra set of eyes and shed some insight as to where I'm going wrong here.

Thanks

[rs20]

There are countless threads on this forum where the general consensus agrees that interrupts are a horrible way to deal with rotary encoders (example). Polling at 500 Hz to 1kHz is, in the typical case, vastly easier, more efficient, easier to debounce, etc etc.

You're probably observing switch bounce. With a rotary encoder, switch bounce appears (in the easiest case) like this: Whenever a user clicks the rotary encoder 1 click clockwise, the encoder appears to generate a storm of clicks both clockwise and anticlockwise -- with (hopefully) one more clockwise click than an anti-clockwise.

Polling goes a long way to fixing this, by simply being blind to most bounces. You can also implement some lightweight debouncing on the inputs, and/or can do debouncing-style simplification on the outputs from your state machine. See the linked thread for more details.

[dannyf]

I prefer the state machine approach. Essentially, you use the pins previous state plus their current state to determine rotation, through a look up table.

It is amazingly resistance to bounces.

Interrupts are the way to go. You would simply call the routine from within the isr.

[rs20]

I prefer the state machine approach. Essentially, you use the pins previous state plus their current state to determine rotation, through a look up table.

It is amazingly resistance to bounces.

The quoted code (unless there's a bug in it) is totally equivalent to a state machine.

Interrupts are the way to go. You would simply call the routine from within the isr.

How do you deal with the input bounce-changing between the pin change interrupt and the reading of PIND?

I encourage you to participate on the thread I linked, where everyone rapidly agrees interrupts are an inferior solution, and most of the debate was to do with the details of debouncing.

[rs20]

BTW, I think the real bug you're encountering is here:

Code: [Select]

encoderVal = ((encoderVal << 2) | (PIND & ((1<<PIND3)|(1<<PIND2)))) & 0x0F;
The masked PIND values are located at bits 2 and 3, but you seem to be assuming they'll be down in bits 0 and 1. This means that the encoder perpetually thinks the "previous state" is 00.

Corrected code:

Code: [Select]

encoderVal = ((encoderVal << 2) | ((PIND & ((1<<PIND3)|(1<<PIND2))) >> 2)) & 0x0F;
I also encourage you to use, e.g., 0b0111 instead of 0x07 for readability.

But yeah, interrupts are the wrong way to go because PIND will not necessarily reflect the state change that caused the interrupt.

[dannyf]

A little bit more detail on what I wrote earlier.

1) I used the previous pin state + current pin state to detect rotation. The convention I followed is ENC_A..ENC_B. For example, a clock-wise rotation off of 00->10 (ENC_A channel goes high while ENC_B stays low) should yield +1, indicating clock-wise rotation. Like wise, a counter-clock-wise rotation off of 11->10 (ENC_B goes low while ENC_A stays high) should yield -1, indicating CCW rotation.

2) the four bits of encoder pin states, as indicated above, can be used to index the return value, through a look-up table.

So my routine would look like this:

Code: [Select]

//encoder states.
//coding convention: ENC_A previous..ENC_B previous..ENC_A..ENC_B
const int8_t enc_states={0, -1, 1, 0, 1, 0, 0, -1, -1, 0, 0, 1, 0, 1, -1, 0};

int8_t enc_read(void) {
  static uint8_t enc_ab=0;  //encoder stats, lowest 4 bits effective
  enc_ab = (enc_ab << 2) | (IO_GET(ENC_PORT, ENC_A)?0x02:0) | (IO_GET(ENC_PORT, ENC_A)?0x01:0);
  return enc_states[enc_ab & 0x0f]; //index through the lowest 4 bits
}

You can actually write it into a macro if you care about minimizing the overhead.

Once your isr is set-up, you can simply call enc_read() to return / update the encoder reads.

I have used this approach on motors speed sensors. It can easily respond up to 20Khz+ on a 1MIPS mcu so speed is never a problem.

The only thing I would caution you is hardware wiring. If you intend to use a capacitor to debounce, make sure that there is a serial resistor there. Or you run the risk of resetting the mcu. Here is a case where a low quality switch (with high on resistance) is actually helpful.

[dannyf]

Code: [Select]

  encoderVal = ((encoderVal << 2) | (PIND & ((1<<PIND3)|(1<<PIND2)))) & 0x0F;

Change that to:

Code: [Select]

  encoderVal = ((encoderVal << 2) | ((PIND & (1<<PIND3))?0x02:0) | ((PIND) & (1<<PIND2))?0x01:0)) & 0x0F;

Check for mismatched brackets.

I would also replace PIND, PIND3, PIND2 with macros so porting the code and reading the code are easier. 0x02 and 0x01 may be switched to fit your convention.

You can use ~PIND instead if the switch is active high.

[jpittner]

I have no problems with interrupt driven rotcoder processing. I use internal pullups and 10-22nF capacitors to GND for debouncing, no need for software debounce. Using ATmega128:
        PE6 (left)
        PE7 (right)
        PE4 (button press)

//pull-up input pins
cbi(DDRE,PE4);
cbi(DDRE,PE6);
cbi(DDRE,PE7);
sbi(PORTE,PE4);
sbi(PORTE,PE6);
sbi(PORTE,PE7);

//and allow any-edge interrupts on INT4,6,7
sbi(EICRB,ISC40); cbi(EICRB,ISC41);
sbi(EICRB,ISC60); cbi(EICRB,ISC61);
sbi(EICRB,ISC70); cbi(EICRB,ISC71);
sbi(EIMSK,INT4);
sbi(EIMSK,INT6);
sbi(EIMSK,INT7);

volatile static int8_t rotcount=0;
void rotcoder(uint8_t interrupt, uint8_t pins)
{
if(interrupt)
        {
        if(pins==0 || pins==3) ++rotcount;
        else --rotcount;
        }
else
        {
        if(pins==0 || pins==3) --rotcount;
        else ++rotcount;
        }
if(rotcount==4) {process_rotation(1); rotcount=0;}
if(rotcount== -4) {process_rotation(0); rotcount=0;}
}

INTERRUPT(SIG_INTERRUPT6)
{
rotcoder(0,(PINE>>6)&3);
}

INTERRUPT(SIG_INTERRUPT7)
{
rotcoder(1,(PINE>>6)&3);
}

For a complete open source example see http://www.pittnerovi.com/jiri/hobby/electronics/thermostat/

[Mastrofski]

Thanks everyone for the help thus far, from a design standpoint, I tend to stray away from methods I don't fully understand, so sometimes there are best practices I don't follow because there's a kuldge-y way that I understand. From a troubleshooting standpoint, it works for me, but from a design standpoint, sometimes I've got some weirdness going on.

BTW, I think the real bug you're encountering is here:

Code: [Select]

encoderVal = ((encoderVal << 2) | (PIND & ((1<<PIND3)|(1<<PIND2)))) & 0x0F;
The masked PIND values are located at bits 2 and 3, but you seem to be assuming they'll be down in bits 0 and 1. This means that the encoder perpetually thinks the "previous state" is 00.

Corrected code:

Code: [Select]

encoderVal = ((encoderVal << 2) | ((PIND & ((1<<PIND3)|(1<<PIND2))) >> 2)) & 0x0F;
I also encourage you to use, e.g., 0b0111 instead of 0x07 for readability.

But yeah, interrupts are the wrong way to go because PIND will not necessarily reflect the state change that caused the interrupt.

That seemed to fix the bulk of my problems, although I'm still getting an LED to light occasionally when it shouldn't, I'm sure this speaks to the bouncing issue. I'll look into the thread you linked. Regarding polling, is that just the terminology for checking the encoder in your main loop? I've heard that term used quite a bit and never really got what the difference.

Code: [Select]

  encoderVal = ((encoderVal << 2) | (PIND & ((1<<PIND3)|(1<<PIND2)))) & 0x0F;

Change that to:

Code: [Select]

  encoderVal = ((encoderVal << 2) | ((PIND & (1<<PIND3))?0x02:0) | ((PIND) & (1<<PIND2))?0x01:0)) & 0x0F;

Check for mismatched brackets.

I would also replace PIND, PIND3, PIND2 with macros so porting the code and reading the code are easier. 0x02 and 0x01 may be switched to fit your convention.

You can use ~PIND instead if the switch is active high.

I'm unfamiliar with that ?0x01:0 terminology, specifically the question mark. Is there a reference to read that goes over that? And I thought I had all the hardware debouncing taken care of, but I'm starting to think that that may not be the case(cutoff frequency of 800Hz). I'll be getting a real scope this week though, so maybe I can look into settling the bounce in hardware.

[poorchava]

I generally use rc filtering and a Schmitt trigger to deal with encoders if I don't have dedicated encoser HW inside the uC.

Sent from my HTC One M8s using Tapatalk.

[dannyf]

Code: [Select]

  encoderVal = ((encoderVal << 2) | ((PIND & ((1<<PIND3)|(1<<PIND2))) >> 2)) & 0x0F;

The issue with that kind of code, aside from fixed port and pin assignments, is that it requires 1) consecutive placement of those two pins; and 2) they have to be on pin 2 and pin 3.

If you were to change port / pin assignment, or you wanted to put the encoder on for example pin 6 and pin 1, it no longer works.

The "?" is part of C. A?B:C returns B if A is true, C otherwise. so it is a short-hand version of a "if... else" statement.

[rs20]

That seemed to fix the bulk of my problems, although I'm still getting an LED to light occasionally when it shouldn't, I'm sure this speaks to the bouncing issue. I'll look into the thread you linked. Regarding polling, is that just the terminology for checking the encoder in your main loop? I've heard that term used quite a bit and never really got what the difference.

Polling just means checking the encoder on any regular basis -- the main loop isn't the best place necessarily, as you'll then be blind during any long-running processes sharing that main loop. If you have a timer peripheral spare, it's preferable to set that up to fire an interrupt every 500 Hz or 1 kHz, that way you main loop doesn't need to be written so carefully. (When I say interrupts are bad, I mean that pin change interrupts are bad. But evidently there's more debate on this topic than I realised...)

Also, using hardware/passive components that cost actual money to do debouncing seems strange to me, but again, that seems to be just me...

[rstofer]

unfamiliar with that ?0x01:0 terminology, specifically the question mark. Is there a reference to read that goes over that? And I thought I had all the hardware debouncing taken care of, but I'm starting to think that that may not be the case(cutoff frequency of 800Hz). I'll be getting a real scope this week though, so maybe I can look into settling the bounce in hardware.

The ? mark is a C conditional operator

http://www.c4learn.com/c-programming/c-conditional-operators/

... ((PIND) & (1<<PIND2))?0x01:0))

We AND PIND with 1 shifted left PIND2 times.  If the result is non-zero, we return 0x01 else 0 which is then used in the '|' expression.

More generally:

<conditional expression> ? <statement to execute if non-zero> : <statement to execute if zero>

I have never used it this way but the following statement actually works:

   digitalRead(LED) ? Serial.print("LED = 1") : Serial.print("LED = 0");

The typical blinking LED loop can be shortened:

void loop()
{
   digitalWrite(LED, digitalRead(LED) ? LOW : HIGH);
   delay(200);
}

Not that it is an efficient way to write the loop but it demonstrates something...

[CJay]

I've experimented with rotary encoders before and have my C code for a PIC controlled DDS sig gen somewhere, shouldn't be too difficult to adapt but I think the code here is probably a good margin better than mine already.

One thing I have experienced and later found others have too, if you use a capacitor to 'filter' bounces, you need to make sure it has a discharge path or you may see problems with the encoder not being read reliably no matter what code you write.

Discharging the capacitor through the switch is not a good idea, in my experience it made matters worse (and could potentially shorten the switch life)

[dannyf]

As to bounces, the code I posted earlier is naturally immune to bounces - it will see bounces aas invalide States. You can verify that by walking thee code trough any bounce sequence. The beauty of a state machine.

I often run my eencoders without any RC network at all.

[CM800]

Why not change to ARM, XMEGA AVR or another chip with a built in quadrature decoder input?
It's so much easier and can often take in pulses up to the MHz.

I managed to have great sucess with an Arduino Due.

[rstofer]

I'm kind of sorry to hear that an interrupt driven state machine won't work.  It seems I have been hanging on to this code for quite a while (6+ years) and I sure thought it worked.  The state machine keeps track of all 4 transitions and counts up/down on each transition.

Notice that it also keeps track of which edge of each signal should happen next and interrupts on that particular edge (hence EICRB gets toggled) and it also deals with the initial condition of each signal which is important for wheel encoders and not so much for detent encoders:

Code: [Select]

#include <avr/io.h>
#include <avr/interrupt.h>

#include <stdio.h>
#include <ctype.h>

#include "defines.h"

volatile int8_t  RightWheelState = 0;
volatile int32_t RightWheelCount = 0;
volatile int8_t  LeftWheelState  = 0;
volatile int32_t LeftWheelCount  = 0;

volatile uint8_t urx;
volatile uint8_t urx_recv;

/* Quadrature Encoder Scheme
 *
 *  There are two pairs of quadrature signals, one for each wheel.
 *  The right wheel is on EXINT4 (PE4) and EXINT5 (PE5)
 *  The left  wheel is on EXINT6 (PE6) and EXINT7 (PE7)
 *  Each encoder has two outputs A & B.  For convenience, A is considered the MSB
 *  while B is considered the LSB.
 */

const int32_t EncoderB[] = {+1,-1,-1,+1};
const int32_t EncoderA[] = {-1,+1,+1,-1};

SIGNAL(SIG_INTERRUPT4) // encoder B for right wheel
{
EICRB           ^= 0x01; // toggle the edge bit
RightWheelState ^= 0x01;
RightWheelCount += EncoderB[RightWheelState];
}

SIGNAL(SIG_INTERRUPT5) // encoder A for right wheel
{
EICRB         ^= 0x04;
RightWheelState ^= 0x02;
RightWheelCount += EncoderA[RightWheelState];
}

SIGNAL(SIG_INTERRUPT6) // encoder B for left wheel
{
EICRB          ^= 0x10;
LeftWheelState ^= 0x01;
LeftWheelCount += EncoderB[LeftWheelState];
}

SIGNAL(SIG_INTERRUPT7) // encoder A for left wheel
{
EICRB          ^= 0x40;
LeftWheelState ^= 0x02;
LeftWheelCount += EncoderA[LeftWheelState];
}

void IOInit(void)
{
UBRR0H = 0;
UBRR0L = BAUD;
UCSR0B = _BV(RXEN) | _BV(TXEN) | _BV(RXCIE);
}

static int uart_putchar(char c, FILE * stream)
{
if (c == '\n')
uart_putchar('\r', stream);
while (! (UCSR0A & _BV(UDRE)))
;
UDR0 = c;
return c;
}

SIGNAL(SIG_UART0_RECV)
{
uint8_t s;

s = UCSR0A;
urx = UDR0;
if (bit_is_clear(s, FE))
urx_recv = 1;
}

void QuadInit(void)
{
LeftWheelState  = (PORTE & 0xC0) >> 6;
RightWheelState = (PORTE & 0x30) >> 4;
EICRB = 0xff; // enable rising edge triggering on INT4..7
EICRB ^= (LeftWheelState  & 0x02) << 5; // if a signal is already set
EICRB ^= (LeftWheelState  & 0x01) << 4; // set the interrupt to falling edge
EICRB ^= (RightWheelState & 0x02) << 1;
EICRB ^= (RightWheelState & 0x01);
EIMSK = 0xf0; // enable these 4 interrupts
}

int main(void)
{
IOInit();
QuadInit();
sei();

fdevopen(uart_putchar, NULL);
printf("Hello World\n");

while (1) {
printf("LWS =%2d LWC =%6ld RWS =%2d RWC =%6ld\n",
LeftWheelState, LeftWheelCount, RightWheelState, RightWheelCount);
}
}



[Mastrofski]

Again everyone, thank you for the help, you guys certainly have given me a lot to think about, and I think I'll be able to sort out my issues with this.

That seemed to fix the bulk of my problems, although I'm still getting an LED to light occasionally when it shouldn't, I'm sure this speaks to the bouncing issue. I'll look into the thread you linked. Regarding polling, is that just the terminology for checking the encoder in your main loop? I've heard that term used quite a bit and never really got what the difference.

Polling just means checking the encoder on any regular basis -- the main loop isn't the best place necessarily, as you'll then be blind during any long-running processes sharing that main loop. If you have a timer peripheral spare, it's preferable to set that up to fire an interrupt every 500 Hz or 1 kHz, that way you main loop doesn't need to be written so carefully. (When I say interrupts are bad, I mean that pin change interrupts are bad. But evidently there's more debate on this topic than I realised...)

Also, using hardware/passive components that cost actual money to do debouncing seems strange to me, but again, that seems to be just me...

Thanks for clearing up the polling issue! I can spare the timer overflow, so maybe I'll look into that as a method. I like the pin change interrupt though because it intuitively makes more sense to me. A pin has changed, we need to go check it. I'm also not the best at optimizing code, so I prefer to do as much heavy lifting with the hardware to compensate for inefficiencies in the code(it's certainly easier to put a probe on a physical component!) just my two cents on why I went this route initially instead of jumping straight for software debouncing.

As to bounces, the code I posted earlier is naturally immune to bounces - it will see bounces aas invalide States. You can verify that by walking thee code trough any bounce sequence. The beauty of a state machine.

I often run my eencoders without any RC network at all.

As I mentioned before, I tend to stick with things within my comfort zone, and the only exposure to state machines I had was with VHDL in school. I'll be sure to look over your code and try to make some sense of it.

Why not change to ARM, XMEGA AVR or another chip with a built in quadrature decoder input?
It's so much easier and can often take in pulses up to the MHz.

I managed to have great sucess with an Arduino Due.

2 reasons in particular.

1) I was completely unaware that those even existed.
2) The people using the end result will be much more comfortable soldering through-hole components as opposed to surface mount ones, and I don't think any SAM or XMEGA platforms are offered in a through-hole package.

[bingo600]

Try one of these examples from Peter Danegger , use google xlate for the text (German)

Wikki
https://www.mikrocontroller.net/articles/Drehgeber#Beispielcode_in_C

https://www.mikrocontroller.net/wikifiles/4/41/Encoder.zip


Thread 2
https://www.mikrocontroller.net/topic/112603

Thread 1
https://www.mikrocontroller.net/topic/drehgeber-auslesen?page=1

Sometimes it pays to start at the bottom to see if Peter has made an update (newer code) , even though it's rare

/Bingo

[dannyf]

Thread 1
https://www.mikrocontroller.net/topic/drehgeber-auslesen?page=1

That's the state machine + look-up table approach I mentioned earlier. and I would argue that his code is far less modular and far less portable than mine.

[Mastrofski]

Thread 1
https://www.mikrocontroller.net/topic/drehgeber-auslesen?page=1

That's the state machine + look-up table approach I mentioned earlier. and I would argue that his code is far less modular and far less portable than mine.

Your code looks solid, although I'm unfamiliar with the IO_GET function. I'm assuming that it just reads the pins, but is that a standard function that Atmel provides? If so, is there documentation for it available?

[dannyf]

On a side note, I wrote up a page on the implementation of a generic (aka hardware independent) software debouncer here: https://dannyelectronics.wordpress.com/2016/06/05/a-generic-software-debouncer/

It is built on the well known Kuhn debouncer, except that the hardware-specific portion of the implementation is done through a layer of call back functions.

I will put up a separate post about it.