Control the PWM duty cycle with a rotary encoder in C

Question

I am currently working on a project where I am designing an embedded AVR system using ATmega88 as the controller.

I am trying to output a PWM signal and I want to control its duty cycle with a rotary encoder (using interrupts).

The PWM signal is outputting as intended, however the encoder inputs don't seem to be doing anything despite the functions added. So, assuming that my AVR and encoder boards are working correctly, what is wrong with my C code?

I am using PC4 and PC5 (PCINT12 and PCINT13) as my encoder inputs and I am trying to control a PWM signal at PD7. The IDE I am using is Microchip studio.

#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>
#include <stdlib.h>
#include <stdbool.h>
#include <stdio.h>

// headers
//#include "LEDs.h"
//#include "PWM.h"
//#include "Encoder.h"


unsigned char duty;
bool up;

int main(void) {

    up = true;
    duty = 0;
    OCR0A = duty;
    
    pin_init();
    PWM_init();
    interrupt_init();
    PD7_on();
    
    while(1) {
        OCR0A = duty;
    }
}

int pin_init() {
    
    DDRD = 0xFF;
    
    DDRC = 0x00;
    
    return 1;
}

int PWM_init() {
    
    //setting correct bits for the pwm waveform
    TCCR0A |= (1<<COM0A1) | (1<<COM0A0) | (1<<COM0B1)| (1<<COM0B0) | (1<<WGM01)| (1<<WGM00);


    //setting prescaler to 8
    TCCR0B |= (1<< CS01);
    
    return 1;
}

int interrupt_init() {
    
    PCICR = (1 << PCIE1); //enabling pin change interrupts for group
    PCMSK1 |= (1 << PCINT12) | (1 << PCINT13);
    
    sei(); //setting global interrupts
    
    return 1;
}

int PD7_on() {
    
    PORTD = (1<<PD7);   
    return 1;
}

int PD7_swich() {
    
    if(PORTD == (1<<PD7)) {
        
        PORTD = (0 << PD7);
    }else{
        
        PORTD = (1 << PD7);
    }
    
    return 1;
}

int inc_duty() {
    
    if(duty == 255) {
        up = false;
    }else if(duty == 0) {
        up = true;
    }
    
    if(up) {
        duty++;
    }else {
        duty--;
    }
    
    return 1;
}

ISR(PCINT1_vect, ISR_BLOCK) {
    
    PD7_swich();
    inc_duty();
}

Answer 1

Initialization code for the pin-change interrupt looks legit. Considering, this is a some test code, and you're not expecting it to handle the encoder rotations properly.

But it is not clear how electrical connections are made? Do you use external pull-up resistors? Because they are not enabled in the code.

Variable duty is being used in the main() and also being changed from the interrupt routine. Therefore it has to be marked volatile. Otherwise the compiler probably will reuse the same value in the loop (in main()) without reading actual value from memory.

Probably, usage of pin-change interrupts is not the best way to handle encoders, because encoders are tend to bounce. You may have to implement some kind of debounce approach.

Answer 2

I am trying to control a PWM signal at PD7.

The ATmega88 data sheet specifies OC0A at PD6, and OC0B at PD5. I don't see any OutputCompare (PWM) functionality connected to PD7!

Apart from that, there are many minor problems with your code:

Missing volatile Qualification for duty

duty is used by more than one thread (main and ISR) and should hence be declared volatile.

However, the missing volatile does not cause problems in

    while (1)
    {
        OCR0A = duty;
    }

because this writes to uint8_t (which is unsigned char), and due to strict aliasing rules, the compiler must assume that this write may change duty, which is also unsigned char. And the missing volatile doesn't cause problems in the ISR either, so the issue must be somewhere else...

Toggling an I/O Port

On ATmega88, you can toggle a port bit by writing 1 to the respective PIN register:

void PD7_swich (void)
{
    PIND = 1 << PD7; // *NOT* PIND |= 1 << PD7
}

c.f. "13.2.2 Toggle the Pin" in the ATmega88 manual.

If that functonality is not available, one would if (PORTD & (1 << PD7)) rather than if (PORTD == 1 << PD7).

Use proper Prototypes

Compiling you code will produce a flood of warnings because you are calling functions before providing a prototype. Having code that produces warnings is always a bad thing to have, even if it's just that real problems may hide amongst benigne warnings. You can fix them by:

• Define the function before to using it, or
• Provide a prototype before using it.

For example, you could put main at the end of the module, or add protoypes prior to main like:

void PD7_swich (void); // In a header it it's supposed to be global.
static void PD7_swich (void); // In the module if it's supposed to be local.

avr-gcc ABI usually requires proper protoypes for correct parameter passing, so you are well advised by adding -Wmissing-prototypes and -Wstrict-prototypes to the command line options, or even better -Werror=missing-prototypes and -Werror=strict-prototypes.

It's also strange to return int everywhere, where the most natural return type would be void for all these functions. Moreover, an argument list of () is not the same like (void) in C, thus you want to use (void) and not ().

Debounce Input I/O

Rotary encoders, just like ordinary switches, usually requre debouncing. This is most easily performed in software because it is adjustable and needs no extra parts. One way to achieve this is to sample the relevant port signals every, say, 5ms in a timer ISR. This is much more reliable than IRQing on I/O signal(s).

Additional benefit is that you have more freedom in the schematic and how to wire controller and encoders.