Help me understand some assembly language to modify this encoder library

Hello to all :slight_smile:

i was working with the encoder library for the first time and i noticed that probablky due to the steps of my encoder, the position that i get jumps 4 units at a time. One option is to fix this mechanically but as this is my only encoder for now i want to solve it software wise.

i tried to fix it after just by sum one to the variable i want to change when the position changes by 4 units but that makes my code too slugish…

so, as i was looking to learn some assembly maybe i could modify the library.

sadly i dont know where to start, i think i understand some lines but im not really sure.

so if maybe could point me in the right direction i would be so grateful :slight_smile: :slight_smile: :slight_smile:

here is the relevant (i guess) part of the .h file.

i’m attaching the complete file if needed.

thanks in advance :slight_smile:

/* Encoder Library, for measuring quadrature encoded signals
 * http://www.pjrc.com/teensy/td_libs_Encoder.html
 * Copyright (c) 2011,2013 PJRC.COM, LLC - Paul Stoffregen <paul@pjrc.com>
 *
 * Version 1.2 - fix -2 bug in C-only code
 * Version 1.1 - expand to support boards with up to 60 interrupts
 * Version 1.0 - initial release
 * 
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to deal
 * in the Software without restriction, including without limitation the rights
 * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 * copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 * 
 * The above copyright notice and this permission notice shall be included in
 * all copies or substantial portions of the Software.
 * 
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 * THE SOFTWARE.
 */


#ifndef Encoder_h_
#define Encoder_h_

#if defined(ARDUINO) && ARDUINO >= 100
#include "Arduino.h"
#elif defined(WIRING)
#include "Wiring.h"
#else
#include "WProgram.h"
#include "pins_arduino.h"
#endif

#include "utility/direct_pin_read.h"

#if defined(ENCODER_USE_INTERRUPTS) || !defined(ENCODER_DO_NOT_USE_INTERRUPTS)
#define ENCODER_USE_INTERRUPTS
#define ENCODER_ARGLIST_SIZE CORE_NUM_INTERRUPT
#include "utility/interrupt_pins.h"
#ifdef ENCODER_OPTIMIZE_INTERRUPTS
#include "utility/interrupt_config.h"
#endif
#else
#define ENCODER_ARGLIST_SIZE 0
#endif



// All the data needed by interrupts is consolidated into this ugly struct
// to facilitate assembly language optimizing of the speed critical update.
// The assembly code uses auto-incrementing addressing modes, so the struct
// must remain in exactly this order.
typedef struct {
 volatile IO_REG_TYPE * pin1_register;
 volatile IO_REG_TYPE * pin2_register;
 IO_REG_TYPE            pin1_bitmask;
 IO_REG_TYPE            pin2_bitmask;
 uint8_t                state;
 int32_t                position;
} Encoder_internal_state_t;

class Encoder
{
public:
 Encoder(uint8_t pin1, uint8_t pin2) {
 #ifdef INPUT_PULLUP
 pinMode(pin1, INPUT_PULLUP);
 pinMode(pin2, INPUT_PULLUP);
 #else
 pinMode(pin1, INPUT);
 digitalWrite(pin1, HIGH);
 pinMode(pin2, INPUT);
 digitalWrite(pin2, HIGH);
 #endif
 encoder.pin1_register = PIN_TO_BASEREG(pin1);
 encoder.pin1_bitmask = PIN_TO_BITMASK(pin1);
 encoder.pin2_register = PIN_TO_BASEREG(pin2);
 encoder.pin2_bitmask = PIN_TO_BITMASK(pin2);
 encoder.position = 0;
 // allow time for a passive R-C filter to charge
 // through the pullup resistors, before reading
 // the initial state
 delayMicroseconds(2000);
 uint8_t s = 0;
 if (DIRECT_PIN_READ(encoder.pin1_register, encoder.pin1_bitmask)) s |= 1;
 if (DIRECT_PIN_READ(encoder.pin2_register, encoder.pin2_bitmask)) s |= 2;
 encoder.state = s;
#ifdef ENCODER_USE_INTERRUPTS
 interrupts_in_use = attach_interrupt(pin1, &encoder);
 interrupts_in_use += attach_interrupt(pin2, &encoder);
#endif
 //update_finishup();  // to force linker to include the code (does not work)
 }


#ifdef ENCODER_USE_INTERRUPTS
 inline int32_t read() {
 if (interrupts_in_use < 2) {
 noInterrupts();
 update(&encoder);
 } else {
 noInterrupts();
 }
 int32_t ret = encoder.position;
 interrupts();
 return ret;
 }
 inline void write(int32_t p) {
 noInterrupts();
 encoder.position = p;
 interrupts();
 }
#else
 inline int32_t read() {
 update(&encoder);
 return encoder.position;
 }
 inline void write(int32_t p) {
 encoder.position = p;
 }
#endif
private:
 Encoder_internal_state_t encoder;
#ifdef ENCODER_USE_INTERRUPTS
 uint8_t interrupts_in_use;
#endif
public:
 static Encoder_internal_state_t * interruptArgs[ENCODER_ARGLIST_SIZE];


 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);
 }
*/

public:
 // update() is not meant to be called from outside Encoder,
 // but it is public to allow static interrupt routines.
 // DO NOT call update() directly from sketches.
 static void update(Encoder_internal_state_t *arg) {
#if defined(__AVR__)
 // The compiler believes this is just 1 line of code, so
 // it will inline this function into each interrupt
 // handler.  That's a tiny bit faster, but grows the code.
 // Especially when used with ENCODER_OPTIMIZE_INTERRUPTS,
 // the inline nature allows the ISR prologue and epilogue
 // to only save/restore necessary registers, for very nice
 // speed increase.
 asm volatile (
 "ld r30, X+" "\n\t"
 "ld r31, X+" "\n\t"
 "ld r24, Z" "\n\t" // r24 = pin1 input
 "ld r30, X+" "\n\t"
 "ld r31, X+" "\n\t"
 "ld r25, Z" "\n\t"  // r25 = pin2 input
 "ld r30, X+" "\n\t"  // r30 = pin1 mask
 "ld r31, X+" "\n\t" // r31 = pin2 mask
 "ld r22, X" "\n\t" // r22 = state
 "andi r22, 3" "\n\t"
 "and r24, r30" "\n\t"
 "breq L%=1" "\n\t" // if (pin1)
 "ori r22, 4" "\n\t" // state |= 4
 "L%=1:" "and r25, r31" "\n\t"
 "breq L%=2" "\n\t" // if (pin2)
 "ori r22, 8" "\n\t" // state |= 8
 "L%=2:" "ldi r30, lo8(pm(L%=table))" "\n\t"
 "ldi r31, hi8(pm(L%=table))" "\n\t"
 "add r30, r22" "\n\t"
 "adc r31, __zero_reg__" "\n\t"
 "asr r22" "\n\t"
 "asr r22" "\n\t"
 "st X+, r22" "\n\t"  // store new state
 "ld r22, X+" "\n\t"
 "ld r23, X+" "\n\t"
 "ld r24, X+" "\n\t"
 "ld r25, X+" "\n\t"
 "ijmp" "\n\t" // jumps to update_finishup()
 // TODO move this table to another static function,
 // so it doesn't get needlessly duplicated.  Easier
 // said than done, due to linker issues and inlining
 "L%=table:" "\n\t"
 "rjmp L%=end" "\n\t" // 0
 "rjmp L%=plus1" "\n\t" // 1
 "rjmp L%=minus1" "\n\t" // 2
 "rjmp L%=plus2" "\n\t" // 3
 "rjmp L%=minus1" "\n\t" // 4
 "rjmp L%=end" "\n\t" // 5
 "rjmp L%=minus2" "\n\t" // 6
 "rjmp L%=plus1" "\n\t" // 7
 "rjmp L%=plus1" "\n\t" // 8
 "rjmp L%=minus2" "\n\t" // 9
 "rjmp L%=end" "\n\t" // 10
 "rjmp L%=minus1" "\n\t" // 11
 "rjmp L%=plus2" "\n\t" // 12
 "rjmp L%=minus1" "\n\t" // 13
 "rjmp L%=plus1" "\n\t" // 14
 "rjmp L%=end" "\n\t" // 15
 "L%=minus2:" "\n\t"
 "subi r22, 2" "\n\t"
 "sbci r23, 0" "\n\t"
 "sbci r24, 0" "\n\t"
 "sbci r25, 0" "\n\t"
 "rjmp L%=store" "\n\t"
 "L%=minus1:" "\n\t"
 "subi r22, 1" "\n\t"
 "sbci r23, 0" "\n\t"
 "sbci r24, 0" "\n\t"
 "sbci r25, 0" "\n\t"
 "rjmp L%=store" "\n\t"
 "L%=plus2:" "\n\t"
 "subi r22, 254" "\n\t"
 "rjmp L%=z" "\n\t"
 "L%=plus1:" "\n\t"
 "subi r22, 255" "\n\t"
 "L%=z:" "sbci r23, 255" "\n\t"
 "sbci r24, 255" "\n\t"
 "sbci r25, 255" "\n\t"
 "L%=store:" "\n\t"
 "st -X, r25" "\n\t"
 "st -X, r24" "\n\t"
 "st -X, r23" "\n\t"
 "st -X, r22" "\n\t"
 "L%=end:" "\n"
 : : "x" (arg) : "r22", "r23", "r24", "r25", "r30", "r31");
#else
 uint8_t p1val = DIRECT_PIN_READ(arg->pin1_register, arg->pin1_bitmask);
 uint8_t p2val = DIRECT_PIN_READ(arg->pin2_register, arg->pin2_bitmask);
 uint8_t state = arg->state & 3;
 if (p1val) state |= 4;
 if (p2val) state |= 8;
 arg->state = (state >> 1);
 switch (state) {
 case 1: case 7: case 8: case 14:
 arg->position++;
 return;
 case 2: case 4: case 11: case 13:
 arg->position--;
 return;
 case 3: case 12:
 arg->position += 2;
 return;
 case 6: case 9:
 arg->position -= 2;
 return;
 }
#endif
 }

Encoder.h (25.5 KB)

With all due respect to Mr. Paul Stoffregen, this is a very convoluted library necessitated by trying to be 'one-size-fits-all'. Clearly a case where this doesn't apply. When dealing with pin change interrupts it's better to get your hands dirty, irrespective of the gaggle of naysayers, and code directly. Study up on pin change interrupts and quadrature encoders. Understand why they are called quadrature encoders and the relationship between the sequences of FOUR signals (1-0-2-3 and 2-0-1-3) and the direction of rotation.

Put together your own function and let's have a look at it. My entire ISR for a quadrature rotary encoder that counts up and down with rollover is 9 lines of code. The interrupt setup is 2 statements and I use 7 byte variables. It can be done and it's not that tough.

dkw

I can't show you code for this as it was developed under contract so I don't really own it but the doesn't prevent me from discussing the logic (I think?) and you don't need assembly to make it small and fast.

Quadrature encoders derive their name from the four possible states that they can assume, if only momentarily. They have two outputs, typically named A and B, each can be logic high or low thus giving us the four combinations (11, 10, 01, 00). Internally, these encoders are likely to have two disks, one for each A and B, that are slightly out of phase. Using mechanical or optical sensors, as the encoder is rotated so the signals vary on each output with a delay that represents the phase difference. In one direction, A changes prior to B and the reverse in the opposite direction. The only stable state, which is commonly referred to as detente, is when the encoder is at one of its rest positions and both signals are at their respective rest values. For this discussion we'll assume signals at rest to be at a logic 1, hence the detente value would be 0b11 or 3. As the encoder leaves detente, first one signal will drop to zero, giving us 0b10 or 0b01 (2 or 1). Next we'll pass through a phase where both signals are zero and then through the reverse state from the start before returning to detente.

The two sequences can therefore be described (and tested for) as starting at detente 3, passing through 102 or 201 depending on direction and returning to detente 3. Your four interrupt hits are then 1-0-2-3 or 2-0-1-3.

Each time the interrupt fires, it will reflect a state change on one of the two pins. If you read the port input pins, masking those that map to your A and B signals, you'll get 0, 1, 2 or 3, Keeping track of the sequence will tell you which direction the encoder is being rotated.

dkw

Here's a bit of code for using two rotary encoders with a PCINT...

Also, as an aside - quadrature encoders generally only have one wheel - they just have two sensors, located such that the output of the two sensors are out of phase with eachother.

DKWatson:
I can't show you code for this as it was developed under contract so I don't really own it but the doesn't prevent me from discussing the logic (I think?) and you don't need assembly to make it small and fast.

Quadrature encoders derive their name from the four possible states that they can assume, if only momentarily. They have two outputs, typically named A and B, each can be logic high or low thus giving us the four combinations (11, 10, 01, 00). Internally, these encoders are likely to have two disks, one for each A and B, that are slightly out of phase. Using mechanical or optical sensors, as the encoder is rotated so the signals vary on each output with a delay that represents the phase difference. In one direction, A changes prior to B and the reverse in the opposite direction. The only stable state, which is commonly referred to as detente, is when the encoder is at one of its rest positions and both signals are at their respective rest values. For this discussion we'll assume signals at rest to be at a logic 1, hence the detente value would be 0b11 or 3. As the encoder leaves detente, first one signal will drop to zero, giving us 0b10 or 0b01 (2 or 1). Next we'll pass through a phase where both signals are zero and then through the reverse state from the start before returning to detente.

The two sequences can therefore be described (and tested for) as starting at detente 3, passing through 102 or 201 depending on direction and returning to detente 3. Your four interrupt hits are then 1-0-2-3 or 2-0-1-3.

Each time the interrupt fires, it will reflect a state change on one of the two pins. If you read the port input pins, masking those that map to your A and B signals, you'll get 0, 1, 2 or 3, Keeping track of the sequence will tell you which direction the encoder is being rotated.

dkw

thank you so much for your time :slight_smile: :slight_smile: great explanation, all the phases are very clear now, i already have an idea on how to do it.

I thought i could get away without coding this myself considering encoders are so ubiqutous in electronics and it would be rather useless but it seems its not the case and its worthwhile to dig into it. well thinking it twice, it will be very usefull for working with motors or implementing encoders that are less common

thanks again, really useful :slight_smile:

DrAzzy:
UsefulArduinoPosts/Encoder_with_PCINT.md at master · SpenceKonde/UsefulArduinoPosts · GitHub

Here’s a bit of code for using two rotary encoders with a PCINT…

Also, as an aside - quadrature encoders generally only have one wheel - they just have two sensors, located such that the output of the two sensors are out of phase with eachother.

thanks for your answer, i’ll try it right away

Let us know how you get on.

dkw

i was working with the encoder library for the first time and i noticed that probablky due to the steps of my encoder, the position that i get jumps 4 units at a time. One option is to fix this mechanically but as this is my only encoder for now i want to solve it software wise.

i tried to fix it after just by sum one to the variable i want to change when the position changes by 4 units but that makes my code too slugish...

Can you please explain how a simple divide by four makes the code too sluggish?

Does your encoder have detent positions, and there are four counts between detents? This is common.

DKWatson:
Let us know how you get on.

dkw

This what i ended up with it does the job without a problem.

thanks to all for the help. hope its clear enough for anybody.

/*
 * Rotary Encoder implementation. it uses one interrupt pin to know when to take a measurement and compare the states of each of the encoder Outputs and acts accordingly to a quadrature
 */

unsigned long currentTime; // will keep track of actual time while running

unsigned long lastTime;  // stores the time of the last calculation

const int pin_A = 2;  // This pin MUST be one with interrupt available

const int pin_B = 12;  

unsigned char enc_A;  //current state of Output A

unsigned char enc_B;   //current state of Output B

unsigned char encoder_A_prev=0; // storage of the previous state of Output A

int tmax = 80; // variable we want to change (maximum temperature in this example)

int t_change =1; // change rate


void setup()  {
  // start up...
  
  pinMode(pin_A, INPUT_PULLUP);
  
  pinMode(pin_B, INPUT_PULLUP);
  
  currentTime = millis();
  
  lastTime = currentTime;
   
  attachInterrupt(0,trigg_A,CHANGE); // when ever Outpu A changes of state we will compare 
  
   
  Serial.begin(115200); //DEBUG
  
 
} 

void loop(){
  
}


void trigg_A(){
  cli();
  
  currentTime = millis();
  if(currentTime >= (lastTime + 2)){
      
    enc_A = digitalRead(pin_A);    // Read encoder pins
    enc_B = digitalRead(pin_B);   
    if((!encoder_A) && (encoder_A_prev)){
      // A has gone from high to low 
      if(encoder_B) {
 
        if(tmax + t_change <= 100) tmax += t_change;   //if !(maximum value) then increment
        Serial.println(tmax);        //DEBUG
             
      }   
      else {
        //antii clockwise direction
        if(tmax - t_change >= 40) tmax -= t_change; // if !(minimum value) then decrease 
        Serial.println(tmax);     //DEBUG
                 
      }   

    }   
    encoder_A_prev = enc_A;     // Store value of A for next time    
    
    
       
    
    lastTime = currentTime;  // Updates loopTime
    
    sei();
  }
  
  
}

cattledog:
Can you please explain how a simple divide by four makes the code too sluggish?

i deleted the code where i did this but i just added an if for when the position of the encoder has changed 4 units, then add just 1 to the variable i want to change

cattledog:
Does your encoder have detent positions, and there are four counts between detents? This is common.

yep it has, i guess that was the issue, if the detent rest position wasnt there the encoder could efectively stop within any steps (or state change).

with the code i posted above i fixed that by only triggering at the change of only one output of the encoder

You did remarkably well. Just a hint though, you can get rid of the millis() and the time check by introducing a state that represents detente and then testing only the progression leading to that state. What this means is that if there is a roll past detente (which occurs frequently) before settling back to the rest position, the progression will not reflect rotation, you will not see 1-0-2-3 or 2-0-1-3 but 3-1-3 or 3-0-3 so you just ignore it.