Division in C returns 0... sometimes

Question

Im trying to make a simple RPM meter using an ATMega328.

I have an encoder on the motor which has 306 interrupts per rotation (as the motor encoder has 3 spokes which interrupt on rising and falling edge, the motor is geared 51:1 and so 6 transitions * 51 = 306 interrupts per wheel rotation ) , and I am using a timer interrupting every 1ms, however in the interrupt it set to recalculate every 1 second.

There seems to be 2 problems. 1) RPM never goes below 60, instead its either 0 or RPM >= 60 2) Reducing the time interval causes it to always be 0 (as far as I can tell)

Here is the code

    int main(void){

    while(1){
        int temprpm = leftRPM;
        printf("Revs: %d \n",temprpm);
        _delay_ms(50);
    };

    return 0;
}

ISR (INT0_vect){
    ticksM1++;
}

ISR(TIMER0_COMPA_vect){

    counter++;

    if(counter == 1000){

        int tempticks = ticksM1;
        leftRPM = ((tempticks - lastM1)/306)*1*60;
        lastM1 = tempticks;
        counter = 0;    

    }

}

Anything that is not declared in that code is declared globally and as an int, ticksM1 is also volatile.

The macros are AVR macros for the interrupts.

The purpose of the multiplying by 1 for leftRPM represents time, ideally I want to use 1ms without the if statement so the 1 would then be 1000

Answer 1

For a speed between 60 and 120 RPM the result of ((tempticks - lastM1)/306) will be 1 and below 60 RPM it will be zero. Your output will always be a multiple of 60

The first improvement I would suggest is not to perform expensive arithmetic in the ISR. It is unnecessary - store the speed in raw counts-per-second, and convert to RPM only for display.

Second, perform the multiply before the divide to avoid unnecessarily discarding information. Then for example at 60RPM (306CPS) you have (306 * 60) / 306 == 60. Even as low as 1RPM you get (6 * 60) / 306 == 1. In fact it gives you a potential resolution of approximately 0.2RPM as opposed to 60RPM! To allow the parameters to be easily maintained; I recommend using symbolic constants rather than magic numbers.

#define ENCODER_COUNTS_PER_REV 306
#define MILLISEC_PER_SAMPLE 1000
#define SAMPLES_PER_MINUTE ((60 * 1000) / MILLISEC_PER_SAMPLE)

ISR(TIMER0_COMPA_vect){

    counter++;

    if(counter == MILLISEC_PER_SAMPLE)
    {

        int tempticks = ticksM1;

        leftCPS = tempticks - lastM1 ;

        lastM1 = tempticks;

        counter = 0;    

    }
}

Then in main():

int temprpm = (leftCPS * SAMPLES_PER_MINUTE) / ENCODER_COUNTS_PER_REV ;

If you want better that 1RPM resolution you might consider

int temprpm_x10 = (leftCPS * SAMPLES_PER_MINUTE) / (ENCODER_COUNTS_PER_REV / 10) ;

then displaying:

printf( "%d.%d", temprpm / 10, temprpm % 10 ) ;

Given the potential resolution of 0.2 rpm by this method, higher resolution display is unnecessary, though you could use a moving-average to improve resolution at the expense of some "display-lag".

Alternatively now that the calculation of RPM is no longer in the ISR you might afford a floating point operation:

float temprpm = ((float)leftCPS * (float)SAMPLES_PER_MINUTE ) / (float)ENCODER_COUNTS_PER_REV ;
printf( "%f", temprpm ) ;

Another potential issue is that ticksM1++ and tempticks = ticksM1, and the reading of leftRPM (or leftCPS in my solution) are not atomic operations, and can result in an incorrect value being read if interrupt nesting is supported (and even if it is not in the case of the access from outside the interrupt context). If the maximum rate will be less that 256 cps (42RPM) then you might get away with an atomic 8 bit counter; you cal alternatively reduce your sampling period to ensure the count is always less that 256. Failing that the simplest solution is to disable interrupts while reading or updating non-atomic variables shared across interrupt and thread contexts.

Answer 2

It's integer division. You would probably get better results with something like this:

leftRPM = ((tempticks - lastM1)/6);