Atmel microprocessor and rotary encoder controlling speed of 7 segment display

Question

I am trying to get a rotary encoder to control the speed of a 7 segment display counting from 0-9 with the Atmel (ATmega328P Xplained mini) microprocessor. My problem is that whenever I run the program the display just counts faster and faster until you can just see an "8", sometimes it seems that I can keep the speed down by turning the rotary encoder CCW and sometimes no effect at all. As I am not that experienced in programming and especially not this stuff I hope someone is capable and willing to help.

Here is my code:

#include <avr/io.h>

void Display (uint8_t x)
{
    static uint8_t tabel[] =
    {0b11000000,0b11111001,0b10100100,0b10110000,0b10011001,0b10010010,0b10000010,0b11111000,0b10000000,0b10010000};
    PORTD = tabel[x];
}

int GetInput (void)
{
    uint8_t x = PINC&1;
    uint8_t y = (PINC>>1)&1;
    if (x == 0 && y == 0) {return 0; }
    else if (x == 1 && y == 0) {return 1;}
    else if (x == 0 && y == 1) {return 2;}
    else {return 3;}
}

int main(void)
{
    DDRD = 0xFF;  // set PortD as an output
    DDRC = 0x00;  // set PortC as an input
    PORTB = 0x03; // Activate Pull-up resistors

    float d = 9000;
    int tick = 0;
    int i = 0;
    int input, state = 0; // initial state
    int oldInput = 0;

    while (1)
    {   
        input = GetInput();
        if (oldInput == 0 && input == 1)
        {
            d = (d * 1.1);
            //slower
        }else if (oldInput == 0 && input == 2)
        {
            d = (d * 0.9);
            //faster
        }else if (oldInput == 1 && input == 0)
        {
            d = (d * 0.9);
            //faster
        }else if (oldInput == 1 && input == 3)
        {
            d = (d * 1.1);
            //slower
        }else if (oldInput == 2 && input == 0)
        {
            d = (d * 1.1);
            //slower
        }else if (oldInput == 2 && input == 3)
        {
            d = (d * 0.9);
            //faster
        }else if (oldInput == 3 && input == 1)
        {
            d = (d * 0.9);
            //faster
        }else if (oldInput == 3 && input == 2)
        {
            d = (d * 1.1);
            //slower
        }
        oldInput = input;

        switch (state)
        {
            case 0: //ini
                Display(0);
                state = 1;
                break;

            case 1: //count
                if (i == 9)
                {
                    i = 0;
                    Display(i);
                }
                else
                {
                    i++;
                    Display(i);
                }
                state = 2;
                break;

            case 2: // delay
                if (tick < d)
                {
                    state = 2;
                    tick++;
                }
                else
                {
                    state = 1;
                    tick = 0;
                }
                break;

            case 3: //reset / destroy
                break;
        }
    }
}

Answer 1

First try changing the GetInput function to return a more useful value. Note that bit 0 and bit 1 of PINC already combine to form the integer that you're reconstructing.

int GetInput (void)
{
    // array to convert grey scale bit patterns to direction indicators.
    // Rows indexed by lastValue, columns indexed by thisValue, and the
    // content is -1 for CCW, +1 for CW, 0 for no motion.  Note that 0 is
    // also used for an invalid transition (2 bits changed at once), but a
    // different value could be used for fault detection.
    static const int tableGreyToDirection[4][4] =
    {
        0 , -1, 1 , 0 ,                        // lastValue==0
        1 , 0 , 0 , -1,                        // lastValue==1
        -1, 0 , 0 , 1 ,                        // lastValue==2
        0 , 1 , -1, 0                          // lastValue==3
    };

    static uint8_t lastValue = 0;             // A valid default starting value
    uint8_t thisValue = (PINC & 0b00000011);  // Use the bottom two bits as a value from 0..3
    int result = tableGreyToDirection[lastValue][thisValue];
    lastValue = thisValue;
    return result;
}

You can then simplify the test in the loop greatly.

while (1)
{
    // Check the direction of the encoder: -1 = CCW, +1 = CW, anything else = no motion.
    input = GetInput();
    if(0 < input)
    {
        // Motion is CW, so increment the delay (within reasonable bounds).
        if(8900 > d) d += 100;
    }
    else if(0 > input)
    {
        // Motion is CCW, so decrement the delay (within reasonable bounds).
        if(100 < d) d -= 100;
    }

    // Keep the rest as it is...
}

It would be advisable to change d to be a uint16_t and tidy it up a little. Further tips include using #define to provide readable names for constants. E.g. in my table of directions you could use:

#define ENCODER_CW 1
#define ENCODER_CCW -1
#define ENCODER_NEITHER 0

...

static const int tableGreyToDirection[4][4] =
{
    ENCODER_NEITHER, ENCODER_CCW, ENCODER_CW, ENCODER_NEITHER,  // lastValue==0
    ...

I'm sure you can fill it out yourself.

Answer 2

I checked your SW, but I can't find big issue instantly. You'd better check below part.

1. If you didn't touch the encoder but speed is faster and faster : do you have a scope to check the encoder input port whether noise is input from the port or not.
2. If two input port is stable, please check your value also stable : old input and new input value should be same : check by log or output toggle unused port when the value is changed. you can debug your own code.
3. You'd better add amount tick value than multiply directly to prevent d value becomes 0.
4. your CPU has to run as fast as detect port status change in main loop. - I think it is possible if this code is all of your system.