RobTillaart / rotaryDecoder

Arduino library for a PCF8574 based rotary decoder - supports 4 rotary encoders.
MIT License
12 stars 4 forks source link

rotating accuracy issues // 1 detent on KY-040 = +4 C/-4 CC #12

Closed quiknick closed 4 months ago

quiknick commented 4 months ago

Using KY-040 rotary encoders with this library causes a single detent clockwise or counter-clockwise to advance +4 or -4 respectively. Need to correct the accuracy other wise this library is not useful.

KY-040 datasheet

RobTillaart commented 4 months ago

Thanks for the issue, will look into the datasheet if it can be supported. Might take a few days.

quiknick commented 4 months ago

Clockwise 1 detent = 4 value changes

update State: 251
    0   29  0   0
update State: 243
    0   30  0   0
update State: 247
    0   31  0   0
update State: 255
    0   32  0   0

An then sometimes you'll get a case where a single update state provides the same value 2 or more times like this

update State: 243
    0   32  0   0
    0   32  0   0
    0   32  0   0
    0   32  0   0
RobTillaart commented 4 months ago

Can you share the code you used? Do you use interrupt or polling?

quiknick commented 4 months ago

I used interrupt example. no changes. only added serial.print in .cpp file for update

quiknick commented 4 months ago

polling symptoms identical.

quiknick commented 4 months ago

so the KY-040 goes like this

Clockwise

Position Bit1 Bit2
Step1     0      0
 1/4        1      0
 1/2        1      1
 3/4        0      1
Step2     0      0

Counter-Clockwise

Step1     0      0
 1/4        0      1
 1/2        1      1
 3/4        1      0
Step2     0      0

From this table, we can see that when moving from one 'click' to the next, there are 4 changes in the output code.

From an initial 0 - 0, Bit1 goes high, Bit0 stays low. Then both bits are high, halfway through the step. Then Bit1 goes low, but Bit2 stays high. Finally at the end of the step, both bits return to 0. Detecting the direction is easy - the table simply goes in the other direction (read up instead of down).

RobTillaart commented 4 months ago

So the KY-040 generates 4 pulses for one step.

Would it be an option just to divide the values by 4?

RobTillaart commented 4 months ago

@quiknick I might add the following functions to the API. Would that solve your problem?

void setPulsesPerStep(uint8_t pps)
{
  _pps = pps;
}

uint8_t getPulsesPerStep()
{
  return _pps;
}

int32_t getPulses(uint8_t re)
{
  return getValue(re) / _pps;
}

bool setPulses(uint8_t re, int32_t pulses)
{
  return setValue(re, pulses * _pps);
}
RobTillaart commented 4 months ago

@quiknick

The datasheet does not mention this factor 4 between pulses and steps / detent. It only states 20 pulses per 360° (full rotation) Do you have information, datasheet or link that shows this?

RobTillaart commented 4 months ago

@quiknick I might add the following functions to the API. Would that solve your problem?

void setPulsesPerStep(uint8_t pps)
uint8_t getPulsesPerStep()
int32_t getPulses(uint8_t re)
bool setPulses(uint8_t re, int32_t pulses)

Maybe the API extension should have a more generic conversion, by making the conversion factor a float.

void setScaling(float factor = 1.0)
{
  _factor = factor;
}

float getScaling()
{
  return _factor;
}

int32_t getPulses(uint8_t re)
{
  return getValue(re) / _factor;
}

bool setPulses(uint8_t re, int32_t pulses)
{
  return setValue(re, pulses * _factor);
}

This would allow to solve any factor needed, including non-integer factors (yes it might have rounding / truncate effects) Think of measuring the RPM through a gear with a RE.

Four RE per PCF8574 would mean store 4 factors even if not used which is quite a waste (AVR).. As the conversion code can easily be added in the user code I think it has no place in the library.

RobTillaart commented 4 months ago

Using KY-040 rotary encoders with this library causes a single detent clockwise or counter-clockwise to advance +4 or -4 respectively. Need to correct the accuracy other wise this library is not useful.

After giving this problem some thoughts it won't be solved in the library. The user can fix this very easily by dividing the getValue() by 4 resulting in +1 or -1. There is imho no reason to add a dedicated fix for this.

RobTillaart commented 4 months ago

Alternative solution (footnote for myself) create a derived class KY-040, which does this division by 4. It would assume every RE has the factor 4 correction. However other RE's might have another correction factor 2 or any value (as stated above).

quiknick commented 4 months ago

getvalue(i)/4 does not solve the inherent problem of this library when dealing with rotary encoders that may send multiple pulses per rotation. In this case there are 4 pulses. Divide by 4 does get the correct value, but the library still causes that value to repeat up to 4 or more times because decoder.update(); is still called. I have decided to use something else for my use case. Thank you.

RobTillaart commented 4 months ago

@quiknick Can you share what you used instead? I am curious to how a matching solution looks like, and might learn from it.

quiknick commented 4 months ago

I have tried all these below over the past 6 months. each have their pros and cons. By themselves some are very good, but within the complex use case (12 rotary encoders via 2 encs on ESP32 10 encs on 2x MCP23017 or 2x PCF8575, or 3 PCF8574) I am doing, there seems to be not enough processing power and may need another esp32 even though I am using both cores. I have exclusively switched to MCP23017 at this point now with 4 on my prototype, 2 of which cover the Rotary Encoders and buttons. At any rate here are the libraries I have used:

Currently using: https://github.com/ruiseixasm/Versatile_RotaryEncoder This one is really good as it includes rotating while pushing in the encoder, as well as button functions press, doublepress, hold, hold while rotating etc. The only thing lacking for my case would be acceleration/ speed of rotation such as if I rotate faster vs slower indicate ticks or a value to determine speed so I can utilize code to do +1 +5 +10 value. Performance seems to be working well with my use case w/ 1 encoder so far. will get all 11 set up to see how it performs.

https://github.com/MajicDesigns/MD_REncoder This one fit my use case perfectly, including acceleration, and worked perfectly in my demo, but in my use case performance wasn't very good.

https://github.com/neroroxxx/RoxMux This one fit my use case perfectly, including acceleration, and worked perfectly in my demo, but in my use case performance wasn't very good.

I have also used these other methods w/o libraries as well, but rotating quickly had performance issues in my use case.

void read_encoders() {
  // Encoder interrupt routine for both pins. Updates counter
  // if they are valid and have rotated a full indent

  static uint8_t old_EXPR   = 3;  // Lookup table index
  static int8_t enc_EXPR    = 0;  // Encoder value
  static const int8_t enc_states[]  = {0,-1,1,0,1,0,0,-1,-1,0,0,1,0,1,-1,0}; // Lookup table

  old_EXPR   <<=2;

  //esp32
  //if (digitalRead(7)) old_EXPR |= 0x01; // Add current state of pin A
  //if (digitalRead(8)) old_EXPR |= 0x02; // Add current state of pin B 
  //mcp23017
  if (mcp.read1(0)) old_EXPR |= 0x01; // Add current state of pin A
  if (mcp.read1(1)) old_EXPR |= 0x02; // Add current state of pin B
  enc_EXPR += enc_states[( old_EXPR & 0x0f )];

    // Update counter if encoder has rotated a full indent, that is at least 4 steps
    if( enc_EXPR > 2 ) {                                      // Four steps forward;
      Serial.println("encoder_cw");
      enc_EXPR = 0;
    }
    else if( enc_EXPR < -2 ) {                                // Four steps backwards
            Serial.println("encoder_ccw");
      enc_EXPR = 0;
    }
}
quiknick commented 4 months ago

also this other non library method but again performance and inconsistency with fast rotating sometimes going back than forward.

#define SDA 21
#define SCL 22
#define SDA1 25 //32
#define SCL1 26 //33

#include <Wire.h>      // Required for I2C communication
#include "MCP23017.h"  // MCP23017_RT library
  MCP23017 mcp20(0x20);
  MCP23017 mcp21(0x21, &Wire1); 
  MCP23017 mcp22(0x22); 
  MCP23017 mcp24(0x24);

volatile byte seqA = 0;
volatile byte seqB = 0;
volatile byte cnt1 = 0;
volatile byte cnt2 = 0;
volatile boolean right = false;
volatile boolean left = false;
 int RotPosition = 0; 
 int rotation;  
 int value;
 boolean LeftRight;

int currentStateCLK;
volatile unsigned int encoder0Pos = 0;

//attachInterrupt(digitalPinToInterrupt(5), ISR_encoderChange, RISING);

void setup() {
  Serial.begin(115200);
  Wire.begin(SDA, SCL, 400000);
  Wire1.begin(SDA1, SCL1, 400000);

    //mcp22
  for (int i = 0; i < 16; i++) {
    mcp22.pinMode1(i, INPUT_PULLUP);
    mcp22.write1(i, HIGH);
  }
  while (!mcp22.begin()) {
    Serial.println("mcp22 23017 Error. \nChecking again in 5 seconds...");
    delay(5000);
  }

  //mcp24
  for (int i = 0; i < 16; i++) {
    mcp24.pinMode1(i, INPUT_PULLUP);
    mcp24.write1(i, HIGH);
  }

  while (!mcp24.begin()) {
    Serial.println("mcp24 23017 Error. \nChecking again in 5 seconds...");
    delay(5000);
  }

  Serial.println("ready");
  pinMode(27, INPUT_PULLUP);
  pinMode(37, INPUT_PULLUP);
  pinMode(38, INPUT_PULLUP);

  // Enable internal pull-up resistors
  digitalWrite(27, HIGH);
  digitalWrite(37, HIGH);
  digitalWrite(38, HIGH);

  //PCICR =  0b00000010; // 1. PCIE1: Pin Change Interrupt Enable 1
  //PCMSK1 = 0b00000111; // Enable Pin Change Interrupt for A0, A1, A2
  rotation = mcp24.read1(CLK); 
      RotPosition = 0;  
      Serial.print("Initilising RotPosition at: ");
      //RotPosition%=100;
      Serial.println(RotPosition);
}

void loop() {

  // MAIN LOOP 
  value = mcp24.read1(CLK);
  if (value != rotation) { // we use the DT pin to find out which way we turning.

    if (mcp24.read1(DT) != value) {  // Clockwise
      RotPosition ++;
      LeftRight = true;
      Serial.print ("Right to :");
    } 
    else { //Counterclockwise
      LeftRight = false;
      RotPosition--;
      Serial.print("Left to: ");
    }

    //RotPosition%=100;
    Serial.println(RotPosition);

  } 
  rotation = value;
}
RobTillaart commented 4 months ago

@quiknick For performance it might be interesting to use the MCP23S17, SPI is faster than I2C

https://github.com/RobTillaart/MCP23S17

quiknick commented 4 months ago

@quiknick For performance it might be interesting to use the MCP23S17, SPI is faster than I2C

https://github.com/RobTillaart/MCP23S17

I think just the polling nature of 4 MCP23017 plus all the BLE and other aspects of my use case is too much for the ESP32. There are also 18 OLEDs via TCA9548A so a lot is going on.

RobTillaart commented 4 months ago

May I ask what the project is you're building?

Polling is slower than using interrupt driven, so there should be something to gain. At least you should divide the load over the two cores of the ESP32. Given your requirements you might need RTOS, and use multiple threads to keep track of all the IO.

Do you need the 12 encoders simultaneously or would it be acceptable to have a "selection switch" and only one RE? Are the 12 encodes for the user, or do you monitor a motor or servo position?

18 OLEDs

you mean 18 displays? indeed a lot, text or graphics?