rotary encoder overflow prevention

Question

I am in the process of writing code for an AVR Atmega328p microcontroller. The microcontroller is supposed to read the encoder and increment or decrement r23 based on the rotation of the encoder. Unfortunately, at this time, the output only decreases until it hits 0 then starts at 255, irrespective of the direction that I turn the encoder.

My code is fairly straightforward and is based on a table lookup value that combines the previous state and current state of the encoder. If the previous state and the current state don't combine to create a valid turn, an error is returned and the code does nothing. If a valid state change takes place, a 1 or -1 is added to r23 via r24.

I do not have a problem getting the microcontroller to read the encoder, but I cannot figure out how to prevent r23 from overflowing. My problem is when I hit 255 and add 1, the register overflows and goes to 0. I do not want the register to go to zero; I want it to stay at 255 until I rotate the encoder in the opposite direction. I have the same problem with 0. If the register is at 0 and I add -1, I do not want the register to go to 255, I want it to stay at 0 until I rotate it in the opposite direction.

I do not have a problem with thinking outside the box. If you have a solution or idea, please feel free to post it.

;**** A P P L I C A T I O N   N O T E  *************************************
;*
;* Title:       
;* Version:     
;* Last updated:    
;* Target:  AVR Dragon  
;*
;*
;* DESCRIPTION
;*
;*.device   ATmega328P @ 1M  clock speed
;* 
;* This is a simple program to test an optical encoder
;***************************************************************************

.include "m328Pdef.inc"
.org 0x0000
    jmp RESET   ;Reset Handle

.org 0x0008
    jmp Interrupt1 ; PCINT1 Handler

enc_states:
.db 0,-1,1,0,1,0,0,-1,-1,0,0,1,0,1,-1,0 

RESET:

;Setup stack pointer
    ldi temp, low(RAMEND)   
    out SPL, temp
    ldi temp, high(RAMEND)
    out SPH, temp

//Set Port B pins to output
    ser     temp            ; Set Register Rd <-- 0xff (output)
    out     DDRB,temp       ; set all PORTB bits as output

//Clear analog input pins and enable pull ups on Pin 0 and 1 (Port C)
    clr     temp
    out     DDRC, temp      ;all pins input
    ldi     temp, (1<<PC1)|(1<<PC0)
    out     PORTC,temp      ;Enable pullups on Pin 0 and 1

//Set Port D pins to output
    ser     temp            ; Set Register Rd <-- 0xff
    out     DDRD,temp       ; set all PORTD bits as output

//Enable encoder pins interrupt sources (Encoder 1)
    ldi     temp, (1<<PCINT9)|(1<<PCINT8)
    sts     PCMSK1, temp

//Enable encoder pins interrupt sources (Encoder 2)
//  ldi     temp, (1<<PCINT11)|(1<<PCINT10)
//  sts     PCMSK1, temp

//Enable pin change interrupts
    ldi     temp, (1<<PCIE1)
    sts     PCICR, temp

//Enable global interrupts
    sei

//Lookup table initial value
    ldi     ZL, 0x00   ;lookup table index and initial state

.def temp   = r16
    clr     r25
    clr     r24
    clr     r23

loop:
    out PORTB, r23
    jmp loop

Interrupt1:
// Push SREG, etc
    in      r25, PORTC  ;encoder value from PORTC

    ldi   ZH, High(enc_states*2)   ; setup Z pointer hi
    ldi   ZL, Low (enc_states*2)   ; setup Z pointer lo
    rol     r22     ;remember previous state and shift left twice
    rol     r22
    cbr     r25, 0xFC  ;clear encoder bits 7:2
    mov     r21,r25
    or      r25, r22   ;combine encoder bits with old bits
    cbr     r25, 0xF0  ;clear bits 7:4 for table lookup
    mov     r22, r25   ;save table lookup value
    mov     ZL, r25    ;load index value into table
    lpm     r24, z      ;get result
    add     r23,r24

// Pop SREG, etc.
    reti

Answer 1

'Saturation' can be done quite simply by leveraging the carry flag, like:

  mov __tmp_reg__, r23
  add __tmp_reg__, r24   ; do the addition

  brcs saturated          ; if the carry flag is set we have an overflow and should discard the result

    mov r23, __tmp_reg__ ; there was no overflow so we store the result

  saturated:          

Replace the add r23,r24 at the end of your ISR with the above code and you should be fine. (Obviously, you may need to change __tmp_reg__ to some register you can use as a temporary storage.)

Taking into account that r24 may be either positive, negative, or zero, all cases can be handled correctly by extending the above principle a little:

  mov __tmp_reg__, r23

  tst r24 

  breq doreturn  ; if r24 == 0 we have nothing to do and may just return

  brmi subtract  ; if r24 is 'negative' we need to subtract

    add __tmp_reg__, r24  ; if r24 is not negative we just add
    rjmp store            ; and go to where the result may be stored

  subtract:

    neg r24  ; r24 := -r24
    sub __tmp_reg__, r24   ; do the subtraction

  store:

  brcs doreturn          ; if the carry flag is set we have an overflow and should discard the result

    mov r23, __tmp_reg__ ; there was no overflow so we store the result

  doreturn:

Taking a closer look at your code it seems to me there is another 'glitch' in it when the calculation of the Z pointer is done:

ldi   ZH, High(enc_states*2)   ; setup Z pointer hi
ldi   ZL, Low (enc_states*2)   ; setup Z pointer lo

and

mov     ZL, r25    ;load index value into table

look problematic: The lower part of the Z address is just ignored and overwritten with the index value. This will cause trouble if and when Low (enc_states*2) does not happen to be 0; you probably need to do add ZL, r25 and adc ZH, __zero_reg__ (16-bit addition) instead of mov ZL, r25.

Another thought to maybe reduce the complexity of your routine:

The output of an incremental rotary encoder can be interpreted like some kind of synchronous serial data: One output (say 'A') represents the 'clock' signal, while the other output ('B') represents the 'data' signal.

You may freely choose which output is used for which signal, and which 'clock' polarity you select. The algorithm is then rather simple:

1. (Debounce and) detect a rising (or falling, you choose) transition on the 'clock' signal
2. As soon as the 'clock' edge is detected, just read the level of the 'data' signal - no debouncing needed here
3. The single 'data' bit read then directly indicates the direction the encoder has just made a step in: '0' or '1', one direction or the other.

In pseudo-code this can look like:

bit lastClockState;

void readEncoder() {

    bit currentClockState = readClockPin();

    if ( lastClockState == 1 && currentClockState == 0 ) {
        // A (falling) edge was detected...

        // Get the direction of the rotation:
        bit direction = readDataPin();

        if ( direction == 1 ) {
            value++;
        } else {
            value--;
        }
    }

    lastClockState = currentClockState; // Update the lastClockState for the next iteration

}

Let this execute, for example, every 10ms and you already have some minimalistic de-bouncing 'for free'.

(By the way, to summarize the lesson learned by many others before: Don't try to use some external/pin-change interrupt to detect (un-debounced) signal transitions produced by any mechanical switch, or mechanical encoder for that matter. The bouncing nature of the mechanical elements will make sure this will never work as expected.)

Answer 2

You just need to add some tests for the boundary cases. I haven't done any assembly for years and this code might have nothing to do with your architecture, but it's something like:

    lpm     r24, z      ;get result
    cmp     r23, 0
    je      rot_lo      ; if val is 0
    cmp     r23, 255
    je      rot_hi      ; if val is 255
    jmp     rot_add

rot_lo:
    cmp     r24, 0
    jl      rot_ret     ; don't add if delta less than 0
    jmp     rot_add

rot_hi:
    cmp     r24, 0
    jg      rot_ret     ; don't add if delta greater than 0
    jmp     rot_add     ; (or just fall through here)

rot_add:
    add     r23, r24

rot_ret:
    reti