Interrupt Driven Rotary Encoder help needed

Go To Last Post
10 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi gurus - Firstly apologies for the long post but...
I'm moving from a polled rotary encoder to an interrupt driven model for some code I'm rewriting from a PIC based project I did years ago.

platform = AT Mega 32 @ 14.7MHz
I'm trying to trigger an interrupt on the folling edge of INT2 on Port B (Pin 2). From what I recall, at the time an edge falls on one pin of the rotary encoder, the other pin is either high or low depending on the direction turned.
My code 'seems' to work, but I thought I was having debouce problems so I added some code (included) but the results are the same - very erratic behavior i.e. not consistently incrementing or decrementing a value based on direction of turn

Here's a partial of my Encoder setup routine
:roll:

#include 
#include 
#include 
#include "../TP_Includes/TP_Delay.h"

#define Encoder_Pin_1	2   // The pins the encoder is on (INT2)
#define Encoder_Pin_2	3  // Other Pin
#define Encoder_Input_Port	PINB	//The port the encoder is on
#define Encoder_Input_Port_DDR	DDRB	//the port direction register - 

volatile static int Encoder; //volatile variable that's update in ISR

void Encoder_Init(void) // Initialise Encoder 
{
	Encoder=0;
	Encoder_Input_Port_DDR &= ~((1<<Encoder_Pin_1) | (1<<Encoder_Pin_2));	//set the pins on the encoder port for input
	MCUCSR &= ~(1<<ISC2); 	//Clear the ISC2 bit to 0 (falling edge trigger on INT2) in the MCUCSR
	GICR |= (1<<INT2); 	//Enable the INT 2 interrupt
}

ISR(INT2_vect)	// Interrupt Service Routine for INT on encoder
{
	_delay_us(100);  // Debounce Delay  **Same problem with this line removed
if (!(Encoder_Input_Port & (1<<Encoder_Pin_1)))	// Check if Port bit is low still = valid turn of encoder  **Same problem with this line removed
	{
		if (Encoder_Input_Port & (1<<Encoder_Pin_2)) Encoder=1; else Encoder=-1;			// If the other pin is high rotate is one way otherwise rotate is the other way
	}
		
}

somwhere else in the code

...
int a;
...
sei();
....
Encoder_Init();

there is a loop with this line in it
 a+=Encoder;

the value of a mostly seems to change based on the direction of the turn - but sometimes it just stays the same or goes in the wrong direction

any ideas?

Thanks

Tom

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your code seems okay. One observation: Because of the lower-edge trigger on one line only, you will only trigger every 4 'ticks' or possible encoder positions. If your encoder is a detent type, every 4th detent position will result in a trigger. You could make both lines trigger on both edges... a bit more complicated but 4x the resolution.

As for the funny operation, is this a switch-type encoder, or a powered-type encoder? If its a switch-type you may be getting mucho switch bounce and need bigger delays--up to 50mS. If its a powered digital type, try a bypass cap right at the encoder. 0.1uF across the power and ground pins should do it.

Tim Ressel
Portland, OR
timr@earthling.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks Madhun - yes it's a 4 ticks per detent encoder and a cheap one at that - I'll try with a higher debounce value first (easiest :-) )

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I can't speak about encoders, but this concerns me a bit:

volatile static int Encoder;

Since this is 16 bits, main()'s fetch of this value is non-atomic, so it
could see 0x0001, 0x00FF, 0xFF01, or 0xFFFF if it were interrupted.
You could surround the fetch with CLI/SEI, but since both -1 and 1
are expressible in 8 bits, the simplest fix would be to declare it:

volatile static signed char Encoder;
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Good point McKenny - I'll give it a whirl...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Tried both suggestions (messing around with debounce time and volatile static signed char)
No luck yet - have any of you freakers used an encoder with interrups - I can go back to polling, but the interrupt method seems so much more elegant

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Try something like this... (source of mine from a previous project)

  .
  .
  .
#include 
  .
  .
  .
/*************************************************************
 * Global Variables
 *************************************************************/
bit SwitchA, SwitchB;
bit EncoderCCW, EncoderCW;
  .
  .
  .
/*************************************************************
 * Task:        user_interface_task()
 *
 * Description:  Checks both pushbuttons and rotary encoder. Sets global bit(s) if
 *                    activity detected. 
 *
 * Globals:     SwitchA
 *              SwitchB
 *              EncoderCW
 *              EncoderCCW
 *
 *************************************************************/
void user_interface_task(void)
{
     unsigned char responseTask;
     unsigned char current, save;
     unsigned char sav, enc;
     unsigned char command;
     unsigned char CWcount, CCWcount;
     unsigned char CWsave, CCWsave;

     current = save = PINF & 0x0f;
     enc = SwitchA = SwitchB = EncoderCCW = EncoderCW = 0;
     responseTask = NO_TASK;
     CWcount = CCWcount = 0;


    /*
     * Check the switches and rotary encoder for activity every 4 ticks
     */
     while (1)
     {
          PR_Wait_Semaphore(4);
          command = PR_Query_Semaphore();
          switch (command)
          {
          case UI_REG_RESP_CMD:
               responseTask = uiResponseTask;
               break;
          case WAIT_TIMEOUT:
               if ((PINF & 0x0f) != save)
                    PR_Wait_Ticks(10);
               current = PINF & 0x0f;
               if (current != save)
               {
                    /*
                     * If switch pressed, respective global bit variable = 1.
                     */
                    SwitchA = (~current >> 3) & 1;
                    SwitchB = (~current >> 2) & 1;

                    /*
                     * Set the global CW bit if rotary encoder was rotated CW.
                     * Set the global CCW bit if rotary encoder was rotated CCW.
                     * Clear global CW and CCW bits if no change in rotary
                     * encoder position.
                     */
                    enc = gray2binc(current & 3);
                    sav = gray2binc(save & 3);
                    if (((enc - sav) == 1) || ((enc == 0) && (sav == 3)))
                    {
                         EncoderCW = 1;
                         EncoderCCW = 0;
                    }
                    else if (((sav - enc) == 1) || ((sav == 0) && (enc == 3)))
                    {
                         EncoderCW = 0;
                         EncoderCCW = 1;
                    }
                    else
                    {
                         EncoderCW = 0;
                         EncoderCCW = 0;
                    }

                    save = current;

                    /*
                     * Send semaphore to the registered task.
                     */
                    if ((SwitchA || SwitchB || (EncoderCW) || (EncoderCCW)) && (responseTask != NO_TASK))
                    {
                         CWcount = CCWcount = 0;
                         PR_Send_Semaphore(responseTask, UI_EVENT);
                    }

               }
               break;
          default:
               while (1) {}
               break;
          }
     }
}

  .
  .
  .

The above could/should be in/called from your "main" loop (when not using PR_RTX)
Other code in main loop (or other tasks, if using PR_RTX) should test for state of EncoderCCW and EncoderCW and execute/branch accordingly. If no encoder motion, both equal 0.

The gray2binc(unsigned char n) is a CodeVision library function that converts the number n from Gray code representation to its binary equivalent. I don't have the source, of course, but it should not be too difficult to figure. Google gray2bin.

The bottom line is, not only is it not necessary, it is a very bad idea to connect rotary encoder outputs to uC interrupt input pins! Don't feel bad, I think this is a mistake that everyone makes when first connecting a rotary encoder to a uC! (I did too...)

Ignore the PR_RTX stuff above. All that's really happening, is that the switches and rotary encoder inputs are sampled periodically and global variables are set/reset to indicate button presses and/or rotary encoder movement.

Enjoy!

Tom

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks! - I have the code working fine on a polled basis (not listed here) - was trying to get it working with interrupts (you can wake the uC with an interrups :-) )
Curious why you think using interrups is a bad idea?

T

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
Curious why you think using interrups is a bad idea?

Jack explains it well here in 7th paragraph
http://www.embedded.com/showArti...

Other interesting reads:
http://www.embedded.com/showArti...
http://www.embedded.com/showArti...

Note: You can still achieve the feel of moving through a menu, etc faster or slower depending on encoder rotation rate even while polling at a fixed rate. This provides a nice user interface feel.

Quote:
was trying to get it working with interrupts (you can wake the uC with an interrups

EDIT:
I don't know what your I/O situation is. Here's an idea: Connect one of the encoder outputs to an input port that has a comparator input alternate function. Before sleeping, switch the input pin to be comparator input with interrupt. After you wake up, reconfigure the pin to input port function. The comparator input provides hysteresis and is perfect for this application.

Tom

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Tom

Thanks for the links - really interesting reading (especially the first one).
I guess I'm going back to polled code (Want to avoid external circuitry) and will use some of the tidbits in the article.

Tom