Interrupt driven encoder with unexpected behaviour

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

Hi all,

 

I’m using an Atmega328p with a quadrature encoder to make a DC motor move to a setpoint value. I’m using interrupts to make a counter go up each time I have a falling edge of sensor A AND sensor B is high. I make the counter go down in the ISR each time I have a rising edge of sensor A AND sensor B is high. In my main I then run a while loop which checks whether the counter is below my setpoint value. It escapes the while loop when the count is greater than or equal to the setpoint, after which the motor gets turned off. The code is however not behaving as I expected:

  • If setpoint <= 255, everything is normal
  • If setpoint is in the interval  [256, 511] my motor either goes to setpoint 256 (which is wrong, except if setpoint = 256) or to the specified setpoint (which is correct).
  • If setpoint is in the interval [512, 767] my motor either goes to setpoint 512 (which is wrong, except if setpoint = 512) or to the specified setpoint (which is correct).
  • If setpoint is in the interval [768, 1023] my motor either goes to setpoint 768 (which is wrong, except if setpoint = 768) or to the specified setpoint (which is correct).
  • Etc…

 

I think that the weird behaviour occurs when the count is 255, 511, 767, … (multiples of 256) AND the ISR gets called during the evaluation of ‘count < setpoint’. This would explain why it only happens occasionally, and works fine in the other cases. When I move the evaluation of 'count < setpoint’ into my ISR everything is fine.

 

I posted both the working interrupt code, and the interrupt code that behaves weirdly. Does anyone have an idea as to why it behaves this way?

 

Thanks in advance!

Simon

 

Code that behaves weirdly:

#define F_CPU 1000000UL

#include <util/delay.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include "lcd.h"

volatile int count;
int setpoint;

int main(void)
{
        lcd_initialize();
        DDRD &= ~(1 << PIND2); // Sensor A
        DDRD &= ~(1 << PIND3); // Sensor B
        DDRD |= (1 << PIND7); // Transistor base which turns motor on/off

        EIMSK |= (1 << INT1); // Enable interrupts for INT1

        EICRA |= (1 << ISC10); // Interrupts each logical change INT1

        sei(); // Enable global interrupts

        count = 0;
        setpoint = 300;

        while(1)
        {
            PORTD |= (1 << PIND7); // Turn motor on
            while(count < setpoint){};

            PORTD &= ~(1 << PIND7); // Turn motor off
            lcd_print_int(count);
            break;
        }        
        return 0;
}

ISR(INT1_vect)
{
    /* If signal A is HIGH (rising edge) and B is HIGH */
    if ((PIND & (1 << PIND3)) && (PIND & (1 << PIND2))) {
        count++;
    }
    /* If signal A is LOW (falling edge) and B is HIGH */
    else if (~(PIND | ~(1 << PIND3)) && (PIND & (1 << PIND2))) {
        count--;   
    }
}

 

Working code:

#define F_CPU 1000000UL

#include <util/delay.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include "lcd.h"

volatile int count;
volatile unsigned char expression; 
int setpoint;

int main(void)
{
        lcd_initialize();
        DDRD &= ~(1 << PIND2); // Sensor A
        DDRD &= ~(1 << PIND3); // Sensor B
        DDRD |= (1 << PIND7); // Transistor base which turns motor on/off

        EIMSK |= (1 << INT1); // Enable interrupts for INT1

        EICRA |= (1 << ISC10); // Interrupts each logical change INT1

        sei(); // Enable global interrupts

        count = 0;
        setpoint = 300;
        expression = count < setpoint;

        while(1)
        {
            PORTD |= (1 << PIND7); // Turn motor on
            while(expression){};

            PORTD &= ~(1 << PIND7); // Turn motor off
            lcd_print_int(count);

            break;
        }        
        return 0;
}

ISR(INT1_vect)
{
    /* If signal A is HIGH (rising edge) and B is HIGH */
    if ((PIND & (1 << PIND3)) && (PIND & (1 << PIND2))) {
        count++;
        expression = count < setpoint;
    }
    /* If signal A is LOW (falling edge) and B is HIGH */
    else if (~(PIND | ~(1 << PIND3)) && (PIND & (1 << PIND2))) {
        count--;   
        expression = count < setpoint;
    }
}

 

Last Edited: Tue. Dec 2, 2014 - 01:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have a comment or two. Unexpected behavior might occur when one returns from main. Where does it return to? 0x0000? Thats a restart. Returns to 0xffff and walks thru 64K of those until it gets to zero? Thats a restart. 2nd point, there are 3 modes you can read the encoder in: watch one edge on one input, watch either edge on one input, watch either edge on both inputs. Called quadrature mode 1,2 or 4. If the encoder has a detent, I betcha one input changes right in the detent, the other changes in the MIDDLE of the detent. Dont use the input that sits in the detent for the edge change interrupt. Finally, I think there are a couple more init instructions needed to turn on the falling edge detection, so you might be getting an interrupt on a hi level or a lo level. Over.

 

Imagecraft compiler user

Last Edited: Tue. Dec 2, 2014 - 01:27 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I agree that I shouldn’t return from main, I just added that so that my motor wouldn’t keep spinning loop after loop. But the problem occurs before exiting the main loop.

 

As for the initialization, I used the Atmega's datasheet. I want to detect every logical change on INT1 and not only falling edge. Page 71 mentions that if I want 'Any logical change on INT1 generates an interrupt request.’ then I only have to set ISC10 to 1 in the EICRA register.

 

I've tried both signals for the rising/falling edge detection and unfortunately that did not resolve the issue. But out of curiosity, what is the impact of using the input that sits in the detent for the edge interrupt?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
else if (~(PIND | ~(1 << PIND3))

That expression is confusing to me.  Won't it almost always be true?

 

ISR(INT1_vect)
{
    /* If signal A is HIGH (rising edge) and B is HIGH */
    if ((PIND & (1 << PIND3)) && (PIND & (1 << PIND2))) {
        count++;
        expression = count < setpoint;
    }
    /* If signal A is LOW (falling edge) and B is HIGH */
    else if (~(PIND | ~(1 << PIND3)) && (PIND & (1 << PIND2))) {
        count--;   
        expression = count < setpoint;
    }
}

In my encoder apps, I simplify the logic a bit to only test once.  Excerpt below (using CodeVision's "dot" syntax).  Discussed here https://www.avrfreaks.net/comment... and code repasted from the following post

 

// **************************************************************************
// *
// *		P I N _ C H A N G E _ I S R 2
// *
// **************************************************************************
//

// Pin change 16-23 interrupt service routine
//	Used for a pin change interrupt on PCINT18 (PIND.2) for recognizing
//	encoder pulses (Hall switch channel A).
interrupt [HALLA_INT] void pin_change_isr2(void)
{
// An edge on HALLA was detected.  We use both edges (x2 quadrature)
//	as well as both edges on HALLB giving x4 quadrature
	if (!HALLA)
		{
		// A falling edge.

		if (!HALLB)
			{
			// Positive direction (arbitrary)
			position += rotation;
			}
		else
			{
			// Negative direction (arbitrary)
			position -= rotation;
			}

		}
	else
		{
		// A falling edge.

		if (HALLB)
			{
			// Positive direction (arbitrary)
			position += rotation;
			}
		else
			{
			// Negative direction (arbitrary)
			position -= rotation;
			}

		}
}

I've done several production encoder apps using the above.  Note (as I mentioned) there is >>one<< initial check for rising/falling, then >>one<< test of the "other" pin.  Depending on the particular layout, I use either pin-change or external int configured for both edges.

 

[Rereading the linked thread, the whole thread and the links therein should be very applicable to what you are doing.]

 

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
~(PIND | ~(1 << PIND3)

is the same as

~PIND & (1<<PIND3)

which is 1 if pin3 is low, and 0 if pin3 is high. 

 

I use x1 quadrature since I only use rising and falling edge of sensor A, whereas you use rising/falling edge for both sensors. Also, I don’t think my ISR is wrong since the first and second code sample I posted both share the same ISR and the second sample does work. I’m mainly trying to figure out why the first approach does not.

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

theusch wrote:

else if (~(PIND | ~(1 << PIND3))

That expression is confusing to me.  Won't it almost always be true?

 

This can be rewritten as

else if  (~PIND & 1<<PIND3)

which I believe is true when PIND3 is 0.

 

The following code can be simplified.

ISR(INT1_vect)
{
    /* If signal A is HIGH (rising edge) and B is HIGH */
    if ((PIND & (1 << PIND3)) && (PIND & (1 << PIND2))) {
        count++;
        expression = count < setpoint;
    }
    /* If signal A is LOW (falling edge) and B is HIGH */
    else if (~(PIND | ~(1 << PIND3)) && (PIND & (1 << PIND2))) {
        count--;   
        expression = count < setpoint;
    }
}

It is the same as

ISR(INT1_vect)
{
    /* If signal B is HIGH */
    if (PIND & (1 << PIND2)) {
        /* If signal A is HIGH (rising edge) */
        if (PIND & 1<<PIND3) {
            count++;
            expression = count < setpoint;
        }
        /* If signal A is LOW (falling edge)*/
        else {
            count--;   
            expression = count < setpoint;
        }
    }
}

 

I don't think it is generally good evaluate

(PIND & (1 << PIND2)

twice in too different if statements because the value could change in between (doubt it will, but still) and you probably expect it to be the same. Anyway I doubt this will solve your problem because your code seems to be correct, it's just a simplification. Unfortunately If it doesn't work I can't give any clarification for this odd behavior.

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

Yes, you are right. The code you wrote is indeed simpler and does the same thing (except that I check the encoder's values twice which I think might indeed be bad). Didn't fix my problem though unfortunately.

Last Edited: Tue. Dec 2, 2014 - 04:44 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I use x1 quadrature since I only use rising and falling edge of sensor A...

 Now, I certainly could be wrong on this.  In my mind:

 

x1 -- one edge on one channel

x2 -- both edges on one channel

x4 -- both edges on both channels

 

~(PIND | ~(1 << PIND3)

is the same as

~PIND & (1<<PIND3)

which is 1 if pin3 is low, and 0 if pin3 is high. 

Lemme grind on that...

 

If PIND is e.g. 0xf0, then that is ORed with ~0x08 which is 0xf7, then the result is 0xf7. The complement of that is 0x08 which is true.

If PIND is e.g. 0xf8, then that is ORed with ~0x08 which is 0xf7, then the result is 0xff. The complement of that is 0x00 which is false.

 

I tried a couple other PIND values also, and now I'm a believer. ;)

 

I’m mainly trying to figure out why the first approach does not.

Aaah--I see it.  And you were on the right track when you talked of edge conditions...  The read of multi-byte "count" must be atomic.

 

Change

            while(count < setpoint){};

to something like

unsigned int work;
...
            do
            {
                cli();
                work = count;
                sei();
                
            }
            while(work < setpoint);

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

Last Edited: Tue. Dec 2, 2014 - 04:49 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

BTW, I still stand behind "only test the value of PD3 once".

 

In a real app with other things going on, another ISR might delay the servicing of the encoder edge ISR.  When you test the value a second time, it might have changed.  Only important when approaching max rate.  If you follow my code, you see that edge detection and direction tests are done ASAP to minimize racing.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Just for the sake of discussion-- With my CodeVision compiler and the source program and generated code below, the second test with the complements generates more ISR cycles and thus limits max rate (ignoring the second read for the moment).  We want to be skinny, right?  ;)  Your toolchain might be different.

 

#include <io.h>
register unsigned int count;
// External Interrupt 0 service routine
interrupt [EXT_INT0] void ext_int0_isr(void)
{
    /* If signal A is HIGH (rising edge) and B is HIGH */
    if ((PIND & (1 << PIND3)) && (PIND & (1 << PIND2))) {
        count++;
    }
    /* If signal A is LOW (falling edge) and B is HIGH */
    else if (~(PIND | ~(1 << PIND3)) && (PIND & (1 << PIND2))) {
        count--;   
    }

}
// External Interrupt 1 service routine
interrupt [EXT_INT1] void ext_int1_isr(void)
{
    if (PIND & (1 << PIND3))
        {
        // Rising
        if (PIND & (1 << PIND2))
            {
            // Forward
            count++;
            }
        else
            {
            // Reverse
            count--;
            } 
        }
    else
        {
        // Falling
        if (PIND & (1 << PIND2))
            {
            // Reverse
            count--;
            }
        else
            {
            // Forward
            count++;
            } 
        }

}

void main(void)
{
for( ;; ) {
      
    }  
}  

 

                 	.CSEG
                 _ext_int0_isr:
                 ; .FSTART _ext_int0_isr
00003d 93ea      	ST   -Y,R30
00003e 93fa      	ST   -Y,R31
00003f b7ef      	IN   R30,SREG
000040 93ea      	ST   -Y,R30
                 ; 0000 0006     /* If signal A is HIGH (rising edge) and B is HIGH */
                 ; 0000 0007     if ((PIND & (1 << PIND3)) && (PIND & (1 << PIND2))) {
000041 9b4b      	SBIS 0x9,3
000042 c002      	RJMP _0x4
000043 994a      	SBIC 0x9,2
000044 c001      	RJMP _0x5
                 _0x4:
000045 c004      	RJMP _0x3
                 _0x5:
                 ; 0000 0008         count++;
000046 01f2      	MOVW R30,R4
000047 9631      	ADIW R30,1
000048 012f      	MOVW R4,R30
                 ; 0000 0009     }
                 ; 0000 000A     /* If signal A is LOW (falling edge) and B is HIGH */
                 ; 0000 000B     else if (~(PIND | ~(1 << PIND3)) && (PIND & (1 << PIND2))) {
000049 c00c      	RJMP _0x6
                 _0x3:
00004a b1e9      	IN   R30,0x9
00004b 6fe7      	ORI  R30,LOW(0xF7)
00004c 95e0      	COM  R30
00004d 30e0      	CPI  R30,0
00004e f011      	BREQ _0x8
00004f 994a      	SBIC 0x9,2
000050 c001      	RJMP _0x9
                 _0x8:
000051 c004      	RJMP _0x7
                 _0x9:
                 ; 0000 000C         count--;
000052 01f2      	MOVW R30,R4
000053 9731      	SBIW R30,1
000054 012f      	MOVW R4,R30
000055 9631      	ADIW R30,1
                 ; 0000 000D     }
                 ; 0000 000E 
                 ; 0000 000F }
                 _0x7:
                 _0x6:
000056 91e9      	LD   R30,Y+
000057 bfef      	OUT  SREG,R30
000058 91f9      	LD   R31,Y+
000059 91e9      	LD   R30,Y+
00005a 9518      	RETI
                 ; .FEND
                 ;// External Interrupt 1 service routine
                 ;interrupt [EXT_INT1] void ext_int1_isr(void)
                 ; 0000 0012 {
                 _ext_int1_isr:
                 ; .FSTART _ext_int1_isr
00005b 93ea      	ST   -Y,R30
00005c 93fa      	ST   -Y,R31
00005d b7ef      	IN   R30,SREG
00005e 93ea      	ST   -Y,R30
                 ; 0000 0013     if (PIND & (1 << PIND3))
00005f 9b4b      	SBIS 0x9,3
000060 c009      	RJMP _0xA
                 ; 0000 0014         {
                 ; 0000 0015         // Rising
                 ; 0000 0016         if (PIND & (1 << PIND2))
000061 9b4a      	SBIS 0x9,2
000062 c003      	RJMP _0xB
                 ; 0000 0017             {
                 ; 0000 0018             // Forward
                 ; 0000 0019             count++;
000063 01f2      	MOVW R30,R4
000064 9631      	ADIW R30,1
000065 c002      	RJMP _0x14
                 ; 0000 001A             }
                 ; 0000 001B         else
                 _0xB:
                 ; 0000 001C             {
                 ; 0000 001D             // Reverse
                 ; 0000 001E             count--;
000066 01f2      	MOVW R30,R4
000067 9731      	SBIW R30,1
                 _0x14:
000068 012f      	MOVW R4,R30
                 ; 0000 001F             }
                 ; 0000 0020         }
                 ; 0000 0021     else
000069 c008      	RJMP _0xD
                 _0xA:
                 ; 0000 0022         {
                 ; 0000 0023         // Falling
                 ; 0000 0024         if (PIND & (1 << PIND2))
00006a 9b4a      	SBIS 0x9,2
00006b c003      	RJMP _0xE
                 ; 0000 0025             {
                 ; 0000 0026             // Reverse
                 ; 0000 0027             count--;
00006c 01f2      	MOVW R30,R4
00006d 9731      	SBIW R30,1
00006e c002      	RJMP _0x15
                 ; 0000 0028             }
                 ; 0000 0029         else
                 _0xE:
                 ; 0000 002A             {
                 ; 0000 002B             // Forward
                 ; 0000 002C             count++;
00006f 01f2      	MOVW R30,R4
000070 9631      	ADIW R30,1
                 _0x15:
000071 012f      	MOVW R4,R30
                 ; 0000 002D             }
                 ; 0000 002E         }
                 _0xD:
                 ; 0000 002F 
                 ; 0000 0030 }
000072 91e9      	LD   R30,Y+
000073 bfef      	OUT  SREG,R30
000074 91f9      	LD   R31,Y+
000075 91e9      	LD   R30,Y+
000076 9518      	RETI
                 ; .FEND

 

 

I

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

You asked why one shouldnt test the edge of the channel sitting in the detent... If its a knob that a human touches, it will interrupt up or down if you breathe on the knob. You want the interrupt to hit right in the middle of the click from one detent to the next. Make sense? You can tell with a meter.

 

Imagecraft compiler user

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

@teutsch: The atomic read indeed fixed the issue where the motor would stop at 256, 512, ... instead of at the setpoint value. But this means that interrupts that would otherwise be triggered will be ignored during the atomic read and my some of the encoder counts will be missed?

 

@bobgardner: yes makes sense, I get what you meant now

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

But this means that interrupts that would otherwise be triggered will be ignored during the atomic read and my some of the encoder counts will be missed?

No.  What does the datasheet say?

 

 

There are basically two types of interrupts. The first type is triggered by an event that sets the Interrupt Flag. For these interrupts, the Program Counter is vectored to the actual Interrupt Vector in order to execute the interrupt handling routine, and hardware clears the corresponding Interrupt Flag. Interrupt Flags can also be cleared by writing a logic one to the flag bit position(s)
to be cleared. If an interrupt condition occurs while the corresponding interrupt enable bit is cleared, the Interrupt Flag will be set and remembered until the interrupt is enabled, or the flag is cleared by software. Similarly, if one or more interrupt conditions occur while the Global Interrupt Enable bit is cleared, the corresponding Interrupt Flag(s) will be set and remembered until the Global Interrupt Enable bit is set, and will then be executed by order of priority.

 

I think there is a Tutorials article on this as well.

 

It is interesting that your question is concerned with a microsecond (or a few at your stated 1MHz), yet your code with the double-read has several/many microseconds between reads of the signal lines...

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Thanks that's very helpful, hadn't seen that part of the datasheet yet!

 

And you are right that the ISR code that I posted initially was not very efficient. I now use the code that MuTeD posted and I don't think this can be optimized any further:

ISR(INT1_vect)
{
    /* If signal B is HIGH */
    if (PIND & (1 << PIND2)) {
        /* If signal A is HIGH (rising edge) */
        if (PIND & 1<<PIND3) {
            count++;
        }
        /* If signal A is LOW (falling edge)*/
        else {
            count--;   
        }
    }
}

Thanks again for all your posts, you've helped me quite a bit!

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

and I don't think this can be optimized any further:

 Well, let's think about that for a moment...

 

You said Mega328, which has INT1 on PD3.  You said "x1 quadrature", which to me means one signal, one edge.

In my experience and the posted code above, I look at the "primary" signal first to determine rising/falling edge of the "primary" signal, then look at the "secondary" signal to determine direction.

 

It appears to me you are looking at the "secondary" signal (PD2) first, and then take action depending on the sense of the "primary" signal.

 

-- You said x1, one edge.  So, configure INT1 to only fire on one edge.  That means your ISR ends up just looking at the secondary/PD2 signal; a single if/else.

 

-- If indeed you have only configured for one edge, then you can only count in one direction, right?

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

The problem with only using a rising or only a falling edge interrupt is that the interrupts don’t happen at the exact same location when moving in one direction as opposed to the other (rising becomes falling and vice versa). This means that there is a small area where you can "oscillate" the encoder and make the count go infinitely large or infinitely low.

Last Edited: Tue. Dec 2, 2014 - 09:44 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The problem with only using a rising or only a falling edge interrupt is that the interrupts don’t happen at the exact same location when moving in one direction as opposed to the other (rising becomes falling and vice versa). This means that there is a small area where you can "oscillate" the encoder and make the count go infinitely large or infinitely low.

There shouldn't be a problem.  We test with a pendulum, which over the minutes/hours of decay time should stop and reverse at every conceivable part of the waveform.  If your signals aren't fairly well centered and distort when reversing direction, then the signal conditioning isn't correct.  Again, there should be no small area and no problem oscillation.

 

And yet again, doesn't x1 only use one edge?  Or do you really mean x2? 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.