I am creating a simple speed controller around the ATMega168 for a small DC motor that includes a feedback loop to maintain, as accurately as possible, a set RPM. I am using a Hall sensor input (4 ticks per turn) as my feedback source, and running fast PWM off of the 16-bit timer.

This leaves the 8-bit timer as the counter to track my RPM. Since the RPM range is from 0 to 2500, a "long" time can pass between ticks; too long for the 8-bit clock to deal with without some external counting. So, I adapted the realtimeclock.c code as follows below.

My question is, how accurate is this approach? I want to be as accurate as possible, and would appreciate insight. For context, the 8-bit method below records RPM about 10% higher than the much more straightforward 16-bit clock RPM counter code provided in the second code block farther below.

Thank you in advance for any input.

Jesse.

============================================================================

8-BIT CLOCK RPM COUNTING CODE:

// adapted from realtimeclock1.c
// for NerdKits with ATmega168

#define F_CPU 14745600

#include <stdio.h>
#include <stdlib.h>

#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/pgmspace.h>
#include <inttypes.h>

#include "../libnerdkits/lcd.h"

// PIN DEFINITIONS:

void realtimeclock_setup() {
    // setup Timer0:
    // CTC (Clear Timer on Compare Match mode)
    // TOP set by OCR0A register
    TCCR0A |= (1<<WGM01);
    // clocked from CLK/1024
    // which is 14745600/1024, or 14400 increments per second
    TCCR0B |= (1<<CS02) | (1<<CS00);
    // set TOP to 143
    // because it counts 0, 1, 2, ... 142, 143, 0, 1, 2 ...
    // so 0 through 143 equals 144 events
    OCR0A = 143;
    // enable interrupt on compare event
    // (14400 / 144 = 100 per second)
    TIMSK0 |= (1<<OCIE0A);
}

// the_time will store the elapsed time
// in hundredths of a second.
// (100 = 1 second)
// 
// note that this will overflow in approximately 248 days!
//
// This variable is marked "volatile" because it is modified
// by an interrupt handler.  Without the "volatile" marking,
// the compiler might just assume that it doesn't change in 
// the flow of any given function (if the compiler doesn't
// see any code in that function modifying it -- sounds 
// reasonable, normally!).
//
// But with "volatile", it will always read it from memory 
// instead of making that assumption.

volatile int32_t the_time;
volatile uint32_t rpm_count = 0;
volatile uint32_t click_count = 0;

SIGNAL(SIG_OUTPUT_COMPARE0A) {
    // when Timer0 gets to its Output Compare value,
    // one one-hundredth of a second has elapsed (0.01 seconds).
    the_time++;
}

ISR(PCINT1_vect){
    rpm_count++;    
    click_count++;
}

void init_hall_sensor(){

    //make PC4 input pin
    //DDRC &= ~(1<<PC4);
    //turn on pullup resistor
    PORTC |= (1<<PC4);

    //Enable PIN Change Interrupt 1 - This enables interrupts on pins
    //PCINT14...8 see p70 of datasheet
    PCICR |= (1<<PCIE1);

    //Set the mask on Pin change interrupt 1 so that only PCINT12 (PC4) triggers
    //the interrupt. see p71 of datasheet
    PCMSK1 |= (1<<PCINT12);
}

int main() {
    realtimeclock_setup();

    int16_t second_count = 0;
    int16_t rps = 0;
    int16_t rpm = 0;

    // init lcd
    lcd_init();
    FILE lcd_stream = FDEV_SETUP_STREAM(lcd_putchar, 0, _FDEV_SETUP_WRITE);
    lcd_home();

    init_hall_sensor();

    // turn on interrupt handler
    sei();

    while(1) {
        lcd_home();

        lcd_line_two();
        fprintf_P(&lcd_stream, PSTR("Click count: %d "), click_count);
        lcd_line_three();
        fprintf_P(&lcd_stream, PSTR("Seconds elapsed: %d"), second_count);
        lcd_line_four();
        fprintf_P(&lcd_stream, PSTR("RPM: %d-----"), rpm);

      if(the_time >= 100){
          the_time = 0;
          second_count++;
          rps = rpm_count / 4;
          rpm_count = 0;
          rpm = rps * 60;
      }
  }

  return 0;
}

====================================================================

16-BIT RPM COUNTING CODE:

// rpm_test.c

#define F_CPU 14745600

#include <stdio.h>
#include <stdlib.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/pgmspace.h>
#include <inttypes.h>
#include "../libnerdkits/delay.h"
#include "../libnerdkits/lcd.h"
#include "../libnerdkits/uart.h"

// PIN DEFINITIONS:
//
// PB4 (PCINT12) - Hall switch input

void realtimeclock_setup() {

    TCCR1B = 5;
    OCR1A = 14400;
    TCCR1B |= _BV(WGM12); // CTC mode
    TIMSK1 = _BV(OCIE1A); // interrupt on OCR1A match

    // setup Timer1:
  // CTC (Clear Timer on Compare Match mode)
  // TOP set by OCR1A register
  // TCCR1A |= (1<<WGM11);
  // clocked from CLK/1024
  // which is 14745600/1024, or 14400 increments per second
  // TCCR1B |= (1<<CS12) | (1<<CS10);
  // set TOP to 144399
  // because it counts 0, 1, 2, ... 142, 143, 0, 1, 2 ...
  // so 0 through 143 equals 144 events
  // OCR1A = 14399;
  // enable interrupt on compare event
  // (14400 / 14400 = 1 per second)
  // TIMSK1 |= (1<<OCIE1A);
}

volatile uint32_t time_count = 0;
volatile int32_t click_count = 0;
volatile uint32_t rpm_count = 0;
volatile uint32_t rps = 0; 
volatile uint32_t rpm = 0;

SIGNAL(SIG_OUTPUT_COMPARE1A) {
    // when Timer1 gets to its Output Compare value,
    // one second has elapsed.
    rps = rpm_count / 4;
    rpm = rps*60;
    rpm_count = 0; 
    time_count++;
}

ISR(PCINT1_vect){

    rpm_count++;
    click_count++;

}

void init_hall_sensor(){

    //make PC4 input pin
    //DDRC &= ~(1<<PC4);
    //turn on pullup resistor
    PORTC |= (1<<PC4);

    //Enable PIN Change Interrupt 1 - This enables interrupts on pins
    //PCINT14...8 see p70 of datasheet
    PCICR |= (1<<PCIE1);

    //Set the mask on Pin change interrupt 1 so that only PCINT12 (PC4) triggers
    //the interrupt. see p71 of datasheet
    PCMSK1 |= (1<<PCINT12);
}

int main() {
  realtimeclock_setup();

    // init lcd
    lcd_init();
    FILE lcd_stream = FDEV_SETUP_STREAM(lcd_putchar, 0, _FDEV_SETUP_WRITE);
    lcd_home();

    // init serial port
    // uart_init();
    // FILE uart_stream = FDEV_SETUP_STREAM(uart_putchar, uart_getchar, _FDEV_SETUP_RW);
    // stdin = stdout = &uart_stream;

    init_hall_sensor();

    // turn on interrupt handler
    sei();

    while(1) {

        // write into to the LCD
        lcd_line_two();
        fprintf_P(&lcd_stream, PSTR("Click count: %d "), click_count);
        lcd_line_three();
        fprintf_P(&lcd_stream, PSTR("Seconds elapsed: %d"), time_count);
        lcd_line_four();
        fprintf_P(&lcd_stream, PSTR("RPM: %d-----"), rpm);

        // write message to serial port
        // printf_P(PSTR("RPM %d\r\n"), rpm);

  }

  return 0;
}

Rather than sampling over a set time, I would suggest you calculate the time that lapses between each pulse. This would give you a faster response and should increase accuracy.

I used that method in my engine tach, here: http://www.nerdkits.com/forum/thread/1258/

I used the 16 bit timer, but you can probably do the same thing with the 8 bit timer using pin change interrupts. I used the input capture function of the 16 bit timer to avoid processing time fudging my timer readings, but this shouldn't be an issue at lower frequencies where a few CPU clocks won't change your data significantly. In retrospect, it was probably insignificant to my application too.

You will need to figure out roughly what the range of time between pulses will be, and set up the pre-scaler of the 8 bit timer so it does not overload within that time frame, or set up an overload counter, let it overload, and add the timer value to the overload counter multiplied by 256, effectively making a 16 bit timer (higher resolution.)

The RPM = (count * 4) / (timer frequency(in hz) * 60)