Rotary Encoder Code Double Counting

Question

I have successfully make my C code on Atmega328 to read my 2 rotary encoders. (More details here)

I am having an issue with the encoder always incrementing by 2 sometimes 4. I'm not sure where the double counting is coming from. I have the code skip polling the encoder pins for N timer cycles it doesn't appear to help.

What can I do to prevent double counts?

Attached is the relevant code.

PIN POLING TIMER

#define ENCODER_READS_PER_LCD_UPDATE 30 // Adjust as needed
volatile uint8_t step = 0;
volatile uint8_t encoder_read_count = 0; // Count encoder reads
ISR(TIMER1_COMPA_vect) {
 switch (step) {
 case 0: // A LOW for 136.5μs
 //sbi(PORTB, BUTTON_SINK);
 cbi(PORTB, ENCODER_A);
 OCR1A = 57; // Restart with 136.5μs
 break;
 case 1: // A HIGH, B LOW for 94.2μs
 sbi(PORTB, ENCODER_A);
 cbi(PORTB, ENCODER_B);
 OCR1A = 47; // 94.2μs / 2μs = ~47 ticks (next step)
 break;
 case 2: // B HIGH, SW LOW for 347μs
 sbi(PORTB, ENCODER_B);
 _delay_us(20);
 cbi(PORTB, BUTTON_SINK);
 OCR1A = 174; // 347μs / 2μs = ~174 ticks (next step)
 //step = -1; // Reset step counter
 break;
 case 3: // SW HIGH, Reset cycle
 sbi(PORTB, BUTTON_SINK);
 encoder_read_count++;
 if(incremented > 0){
 incremented--;
 }
 sbi(ADCSRA, ADSC); // Grab ADC data
 OCR1A = 17; // Restart with 13.1μs
 step = -1; // Reset step counter
 break;
 }
 step++;
}
void init_timer() {
 TCCR1B |= (1 << WGM12); // CTC mode
 TCCR1B |= (1 << CS11); // Prescaler 8 (4MHz / 8 = 500kHz, 2μs per tick)
 OCR1A = 67; // First period: 136.5μs / 2μs = ~68 ticks (rounded)
 TIMSK1 |= (1 << OCIE1A); // Enable Timer1 Compare Match A interrupt
}

Check Encoder:

// check the rotary encoders
static unsigned char check_encoders(void){
 //if(encoder_count < 10){
 uint8_t store = 0;
 if(!incremented){
 cli();
 TCCR1B &= ~(1 << CS11); // Stop Timer1
 switch(step){
 case 1:
 // Fall through to case 1
 case 2:
 if (read_encoder(VOLTAGE_C, &(set_val[1]), step, &incremented)){
 if(set_val[1]>U_MAX){
 set_val[1]=U_MAX;
 }
 store++;
 }
 if (read_encoder(CURRENT_C, &(set_val[0]), step, &incremented)){
 if(set_val[0]>I_MAX){
 set_val[0]=I_MAX;
 }
 //_delay_us(2);
 store++;
 }
 if(incremented > 1){
 incremented = ENCODER_READS_PER_LCD_UPDATE;
 }
 
 break;
 case 3:
 //cli();
 //TCCR1B &= ~(1 << CS11); // Stop Timer1
 store = check_store_button();
 if (store == 1){
 store_permanent_V();
 //TCCR1B |= (1 << CS11); // Restart Timer1
 //sei();
 store = 3;
 }
 if (store == 2){
 store_permanent_I();
 //TCCR1B |= (1 << CS11); // Restart Timer1
 //sei();
 store = 4;
 }
 //TCCR1B |= (1 << CS11); // Restart Timer1
 //sei();
 break;
 default:
 break;
 }
 TCCR1B |= (1 << CS11); // Restart Timer1
 sei();
 }
 //}
 //encoder_count = (encoder_count + 1)%20;
 return store;
}

Gray Code Section:

volatile int voltage_setpoint = 0; // Stores voltage value
volatile int current_limit = 0; // Stores current limit
void init_encoders() {
 cbi(DDRB,PINB5); // input line
 cbi(DDRB,PINB3); // input line
 
 // Enable pull-ups for encoder C pins
 sbi(PORTB, VOLTAGE_C);
 sbi(PORTB, CURRENT_C);
 // Set PB0, PB1, PB2 high initially (to avoid sinking current)
 sbi(PORTB, ENCODER_A);
 sbi(PORTB, ENCODER_B);
 sbi(PORTB, BUTTON_SINK);
}
static uint8_t lastStateV = 0b11; // Start at '11' (both high)
static uint8_t lastStateC = 0b11; // Start at '11' (both high)
static uint8_t stateV = 0;
static uint8_t stateC = 0;
int16_t read_encoder(uint8_t encoder_c, volatile int *value, volatile int8_t step, volatile uint8_t *incremented) {
 uint8_t* lastState = &lastStateV;
 uint8_t* state = &stateV;
 if(encoder_c == CURRENT_C){
 lastState = &lastStateC;
 state = &stateC;
 }
 
 switch(step){
 case 1:
 if(bit_is_clear(PINB, encoder_c)){
 *state |= (1<<1);
 }
 break;
 case 2:
 if(bit_is_clear(PINB, encoder_c)){
 *state |= (1<<0);
 }
 // Gray code decoding
 //if(state == *lastState){ 
 //return 0;
 //}
 
 if(*state != *lastState){
 if ((*lastState == 0b11 && *state == 0b10) || (*lastState == 0b10 && *state == 0b00) ||
 (*lastState == 0b00 && *state == 0b01) || (*lastState == 0b01 && *state == 0b11)) {
 (*value)--; // CounterClockwise rotation
 if (*value < 0) *value = 0; // Prevent negative values
 (*incremented)++;
 } else if ((*lastState == 0b11 && *state == 0b01) || (*lastState == 0b01 && *state == 0b00) ||
 (*lastState == 0b00 && *state == 0b10) || (*lastState == 0b10 && *state == 0b11)) {
 (*value)++; // Clockwise rotation
 (*incremented)++;
 }
 *lastState = *state;
 *state = 0;
 return 1;
 }
 *lastState = *state;
 *state = 0;
 break;
 default:
 return 0;
 }
 return 0;
}

Main loop:

int main(void)
{
 char out_buf[20+1];
 uint8_t i=0;
 uint8_t ilimit=0;
 measured_val[0]=0;
 measured_val[1]=0;
 init_dac();
 lcd_init();
 init_encoders();
 init_timer();
 set_val[0]=15;set_val[1]=50; // 150mA and 5V
 if (eeprom_read_byte((uint8_t *)0x0) == 19){
 // ok magic number matches accept values
 set_val[1]=eeprom_read_word((uint16_t *)0x04);
 set_val[0]=eeprom_read_word((uint16_t *)0x02);
 }
 // I2C also called TWI, slave address=3
 // i2c_init(3,1,0);
 sei();
 // i2c_send_data("on");
 init_analog();
 int16_t prev_adc_i = 0;
 int16_t prev_adc_v = 0;
 int16_t prev_set_i = 0;
 int16_t prev_set_v = 0;
 uint8_t update_values = 0;
 while (1) {
 update_values = 0;
 i++;
 // due to electrical interference we can get some
 // garbage onto the display especially if the power supply
 // source is not stable enough. We can remedy it a bit in
 // software with an occasional reset:
 if (i>=2000){ // not every round to avoid flicker
 cli();
 TCCR1B &= ~(1 << CS11); // Stop Timer1
 sbi(PORTB, ENCODER_A);
 sbi(PORTB, ENCODER_B);
 sbi(PORTB, BUTTON_SINK);
 lcd_reset();
 i=0;
 update_values = 1;
 }
 // current
 // voltage
 if(encoder_read_count >= ENCODER_READS_PER_LCD_UPDATE || update_values){
 
 cli();
 TCCR1B &= ~(1 << CS11); // Stop Timer1
 sbi(PORTB, ENCODER_A);
 sbi(PORTB, ENCODER_B);
 sbi(PORTB, BUTTON_SINK);
 
 measured_val[0]=adc_i_to_disp(getanalogresult(0));
 set_val_adcUnits[0]=disp_i_to_adc(set_val[0]);
 set_target_adc_val(0,set_val_adcUnits[0]);
 // voltage
 measured_val[1]=adc_u_to_disp(getanalogresult(1),measured_val[0]);
 set_val_adcUnits[1]=disp_u_to_adc(set_val[1])+disp_i_to_u_adc_offset(measured_val[0]);
 set_target_adc_val(1,set_val_adcUnits[1]);
 ilimit=is_current_limit();
 
 lcd_home();
 lcd_puts("SET:");
 int_to_ascii(set_val[1],out_buf,1,1);
 lcd_puts(out_buf);
 lcd_puts("V ");
 int_to_ascii(set_val[0],out_buf,2,0);
 lcd_puts(out_buf);
 lcd_putc('A');
 //if (!ilimit){
 // put a marker to show which value is currently limiting
 //lcd_puts("<-");
 //}else{
 //lcd_puts(" ");
 //} 
 
 // check_i2c_interface();
 // current
 
 lcd_gotoxy(0,1);
 if(ilimit){
 lcd_puts("<OL>");
 } else if(set_val[1] == 0){
 lcd_puts("OFF!");
 } else {
 lcd_puts(" ");
 }
 int_to_ascii(measured_val[1],out_buf,1,1);
 lcd_puts(out_buf);
 lcd_puts("V ");
 int_to_ascii(measured_val[0],out_buf,2,0);
 lcd_puts(out_buf);
 lcd_putc('A');
 if (ilimit){
 // put a marker to show which value is currently limiting
 lcd_putc('!');
 } else {
 lcd_putc(' ');
 }
 encoder_read_count = 0;
 //incremented = 0;
 sbi(PORTB, ENCODER_A);
 sbi(PORTB, ENCODER_B);
 sbi(PORTB, BUTTON_SINK);
 TCCR1B |= (1 << CS11); // Restart Timer1
 sei();
 }
 //dbg
 //int_to_ascii(is_dacval(),out_buf,0,0);
 //lcd_puts(out_buf);
 // check_i2c_interface();
 // the buttons must be responsive but they must not
 // scroll too fast if pressed permanently
 if (check_encoders()==0 && step > 2){
 // no buttons pressed
 //delay_ms(100);
 bpress=0;
 // check_i2c_interface();
 //check_encoders();
 //delay_ms(150);
 } else {
 // button press
 if (bpress > 11){
 // somebody pressed permanetly the button=>scroll fast
 //delay_ms(10);
 // check_i2c_interface();
 //delay_ms(30);
 } else {
 bpress++;
 //delay_ms(100);
 // check_i2c_interface();
 //delay_ms(100);
 }
 }
 prev_adc_i = measured_val[0];
 prev_adc_v = measured_val[1];
 prev_set_i = set_val[0];
 prev_set_v = set_val[1];
 }
 return(0);
}

• TQQQ: Are you trying to bit-bang encoder?
• Mattman944: Are you using the cheap mechanical encoders with detents? I sometimes buy cheap stuff from Amazon with poor documentation. I bought cheap encoders and characterized them, they didn't behave as I expected, there are two edges per detent. electronics.stackexchange.com/questions/632802/…
• Antone Bajor: @Mattman944 They are specifically the this Bourns model so not cheap amazon. The page says "24 Pulses Per Revolution" I have no idea what that means since it's a passive component.
• Antone Bajor: @TQQQ Yes I'm using the atmegas timer to pull the A, B, and Push buttons Pins low in a timed sequence. Then in the software loop I'm reading of the C pin has been pulled low. The A, B and Push pins are shared with the LCD data lines so there has to be some enabling and disabling of the timer to prevent everything from interfering.
• Mattman944: Do you have a scope? Scope A & B while you turn the knob until you understand how they work.

Answer 1

The solution was quite simple and my modified code was already 90% there. The read_encoder code needed to keep track of rotational ticks or pulses.

Since I was always seeing multiples of 2 in my voltage or current adjustment I kept track of good encoder reads and only allowed incrementing or decrementing once it counted ticks > 1 or ticks < 1.

static uint8_t lastStateV = 0b11; // Start at '11' (both high)
static uint8_t lastStateC = 0b11; // Start at '11' (both high)
static uint8_t stateV = 0;
static uint8_t stateC = 0;
static int8_t ticksV = 0;
static int8_t ticksC = 0;
int16_t read_encoder(uint8_t encoder_c, volatile int *value, volatile int8_t step, volatile uint8_t *incremented) {
 uint8_t* lastState = &lastStateV;
 uint8_t* state = &stateV;
 int8_t* ticks = &ticksV;
 
 if(encoder_c == CURRENT_C){
 lastState = &lastStateC;
 state = &stateC;
 ticks = &ticksC;
 }
 
 switch(step){
 case 1:
 if(bit_is_clear(PINB, encoder_c)){
 *state |= (1<<1);
 }
 break;
 case 2:
 if(bit_is_clear(PINB, encoder_c)){
 *state |= (1<<0);
 }
 // Gray code decoding
 //if(state == *lastState){ 
 //return 0;
 //}
 
 if(*state != *lastState){
 if ((*lastState == 0b11 && *state == 0b10) || (*lastState == 0b10 && *state == 0b00) ||
 (*lastState == 0b00 && *state == 0b01) || (*lastState == 0b01 && *state == 0b11)) {
 (*ticks)--;
 if((*ticks) < -1){
 (*value)--; // CounterClockwise rotation
 if (*value < 0) *value = 0; // Prevent negative values
 (*incremented)++;
 (*ticks) = 0;
 }
 *lastState = *state;
 } else if ((*lastState == 0b11 && *state == 0b01) || (*lastState == 0b01 && *state == 0b00) ||
 (*lastState == 0b00 && *state == 0b10) || (*lastState == 0b10 && *state == 0b11)) {
 (*ticks)++;
 if((*ticks > 1)){
 (*value)++; // Clockwise rotation
 (*incremented)++;
 (*ticks) = 0;
 }
 *lastState = *state;
 } else {
 *lastState = *state;
 *state = 0;
 }
 return 1;
 }
 *lastState = *state;
 *state = 0;
 break;
 default:
 return 0;
 }
 return 0;
}

I still haven't figured out why I always get increments of 2 perhaps the settling of encoder states is triggering a true state, but this solution is very reliable.