1
\$\begingroup\$

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); }
\$\endgroup\$
9
  • \$\begingroup\$ Are you trying to bit-bang encoder? \$\endgroup\$ Commented Apr 1 at 21:18
  • \$\begingroup\$ 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/… \$\endgroup\$ Commented Apr 1 at 21:57
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Apr 1 at 22:48
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Apr 1 at 22:52
  • \$\begingroup\$ Do you have a scope? Scope A & B while you turn the knob until you understand how they work. \$\endgroup\$ Commented Apr 2 at 1:39

1 Answer 1

Reset to default
1
\$\begingroup\$

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.

\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.