Two Rotary-Encoders

[cstenkamp]

I want to have two encoders in one script. I assumed it would work like this:

#include <Arduino.h>
#include <BasicEncoder.h>
#include <TimerOne.h>

BasicEncoder encoder(8, 9);
BasicEncoder encoder2(10, 11);

void timer_service() {
  encoder.service();
  encoder2.service();
}

void setup() {
  Serial.begin(230400);
  Serial.println("Starting..");
  Timer1.initialize(1000);
  Timer1.attachInterrupt(timer_service);
}

void loop() {
  int encoder_change = encoder.get_change();
  if (encoder_change) {
    Serial.print("Encoder 1: "); Serial.println(encoder.get_count());
  }      
  int encoder2_change = encoder2.get_change();
  if (encoder2_change) {
    Serial.print("Encoder 2: "); Serial.println(encoder2.get_count());
  }      
}

However, when I use this code, both values will change when I turn any of the rotary-encoders, and sometimes they enter a feedback-loop where both of the values will change (equally) endlessly.
I am really bad at Arduino/C, so am I doing something obvious wrong, or is the error somewhere else than on my side here?

[cstenkamp]

Ah, I see that you use a static variable to keep the pin state -

BasicEncoder/BasicEncoder.h

Line 62 in 5b25c66

static int8_t previous_state = state_now;

. I see that you write about doing this in the Readme, but I still do not understand what the benefit of this being a static variable is. Is there any reason against using an instance variable, setting it to -1 before initializing it, and only set it if it's -1? As far as I see, that would do just the same thing but the state wouldn't be shared across instances...?

[micromouseonline]

Thank you.

I can't think why I would have left it like that but you are quite right of course. The static previous_change variable is not good.

You can have multiple encoders if you make these changes. (I am without good internet access just now. I you wanted to make a pull request I can review that or it will be a few days before I can update the repo)

• The sevice method should become:

 void service() {
   int8_t state_now = pin_state();
   state_now ^= state_now >> 1;  // two bit gray-to-binary
   int8_t difference = m_previous_state - state_now;
   // bit 1 has the direction, bit 0 is set if changeed
   if (difference & 1) {
     m_previous_state = state_now;
     int delta = (difference & 2) - 1;
     if (m_reversed) {
       delta = -delta;
     }
     m_change += delta;
     m_steps += delta;
   }
 }

• add a new private property to keep track of changes to give this:

 private:
  int8_t m_pin_a = 0;
  int8_t m_pin_b = 0;
  uint8_t m_pin_active = LOW;
  uint8_t m_steps_per_count = 4;
  bool m_reversed = false;
  int8_t m_previous_state = 0;
  volatile int m_change = 0;
  int m_steps = 0;

• Update the constructor to initialise the state tracker:

BasicEncoder(int8_t pinA, int8_t pinB, uint8_t active_state = LOW, uint8_t steps = 4)
     : m_pin_a(pinA), m_pin_b(pinB), m_pin_active(active_state), m_steps_per_count(steps) {
   pinMode(pinA, INPUT_PULLUP);
   pinMode(pinB, INPUT_PULLUP);
   m_change = 0;
   m_previous_state = pin_state();
 }

I think that is sufficient and I just tested with two encoders on pins (10,11) and (2,3).

[micromouseonline]

I have updated the library now to V1.1.0 which should support multiple encoders. Please re-open this issue if there is a problem or a new issue if that suits you.

Thank you for bringing this up.