1
\$\begingroup\$

I am writing a C language code for PIC16f877a for an Alarm clock. In short, the alarm clock shows Time, Set alarm and a temperature & humidity reading on an LCD.

I have written interrupts for updating the time, for setting the alarm on and off and few others I plan to write.

The issue is, I am at half point of my code and the Stack overflow problem has risen in the proteus simulation. I have identified the part of code causing the problem. But I can't seem to understand how should I change it to prevent stack overflow.

Can someone help me debug this code? And please note down bad practices in C microcontroller programming which cause similar bugs? I can't seem to find such list of bad practices on the internet.

Here is the code I have written for setting the alarm/time on the clock. The overflow starts happening once I select the option of setting time. Then keeps happening until PIC crashes even when I come out of the setTime routine.

#define UP RD0   // Push buttons active high
#define DOWN RD1
#define SELECT RD2
#define BACK RD3
#define SNOOZE RD4

void UpdateSetTimeScreen() {
    if (ptr > 1)
        LCD_cursor(ptr + 1, 1);
    else
        LCD_cursor(ptr, 1);

    if (set[ptr] == 0)
        LCD_puts("_");
    else
        LCD_display_value(set[ptr]);
}

void SetTime() {
    LCD_clear();
    LCD_puts("Set Time");

    LCD_cursor(0, 1);
    LCD_puts("__:__");
    LCD_cursor(0, 1);

    set[0] = 0;
    set[1] = 0;
    set[2] = 0;
    set[3] = 0;

    ptr = 0;

    unsigned int i =1;

    while (i) {
        if (UP == 1) {
            while (UP);
            switch (ptr) {
                case 0:
                {
                    if (set[ptr] < 2) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
                case 1:
                {
                    if (set[ptr] < 3) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
                case 2:
                {
                    if (set[ptr] < 5) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
                case 3:
                {
                    if (set[ptr] < 9) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
            }
        }
        if (DOWN == 1) {
            while (DOWN);
            if (set[ptr] > 0) {
                set[ptr]--;
                UpdateSetTimeScreen();
            }
        } else if (BACK == 1) {
            while (BACK);
            ptr--;
        } else if (SELECT == 1) {
            while (SELECT);
            if (ptr == 3)
                i=0;
            else
                ptr++;
        }
    }

    hour = (set[0]*10) + set[1];
    minute = (set[2]*10) + set[3];
}

void SetAlarm() {
    LCD_clear();
    LCD_puts("Set Alarm Time");
    LCD_cursor(0, 1);
    LCD_puts("__:__");
    LCD_cursor(0, 1);

    set[0] = 0;
    set[1] = 0;
    set[2] = 0;
    set[3] = 0;

    ptr = 0;
    unsigned int j = 1;

    while (j) {
        if (UP == 1) {
            while (UP);
            switch (ptr) {
                case 0:
                {
                    if (set[ptr] < 2) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
                case 1:
                {
                    if (set[ptr] < 3) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
                case 2:
                {
                    if (set[ptr] < 5) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
                case 3:
                {
                    if (set[ptr] < 9) {
                        set[ptr]++;
                        UpdateSetTimeScreen();
                    }
                    break;
                }
            }
        }
        if (DOWN == 1) {
            while (DOWN);
            if (set[ptr] > 0) {
                set[ptr]--;
                UpdateSetTimeScreen(ptr);
            }
        } else if (BACK == 1) {
            while (BACK);
            if(ptr > 0)
                ptr--;
        } else if (SELECT == 1) {
            while (SELECT);
            if (ptr == 3)
                j=0;
            else
                ptr++;
        }
    }

    AlarmHr = (set[0]*10) + set[1];
    AlarmMin = (set[2]*10) + set[3];

}

void SetTemp() {
}

void print_configuration_screen() {
    LCD_clear();
    switch (scr) {
        case 1:
        {
            LCD_puts("* Set Time");
            LCD_cursor(0, 1);
            LCD_puts("  Set Alarm");
            break;
        }
        case 2:
        {
            LCD_puts("  Set Time");
            LCD_cursor(0, 1);
            LCD_puts("* Set Alarm");
            break;
        }
        case 3:
        {
            LCD_puts("  Set Alarm");
            LCD_cursor(0, 1);
            LCD_puts("* Temp Mode");
            break;
        }
        case 4:
        {
            LCD_puts("* Set Alarm");
            LCD_cursor(0, 1);
            LCD_puts("  Temp Mode");
            break;
        }
    }
}

void ConfigurationMode() {
    scr = 1;
    print_configuration_screen();
    while (mode == 2) {
        if (BACK == 1) {
            while (BACK);
            mode = 1;
        } else if (UP == 1) {
            while (UP);
            if (scr > 1) {
                if (scr == 3)
                    scr = 4;
                else if (scr == 4)
                    scr = 1;
                else
                    scr--;
            }
            print_configuration_screen();
        } else if (DOWN == 1) {
            while (DOWN);
            if (scr == 4)
                scr == 3;
            else if (scr < 3)
                scr++;
            print_configuration_screen();
        } else if (SELECT == 1) {
            while (SELECT);
            switch (scr) {
                case 1: SetTime(); // Set time
                    break;
                case 2:
                {
                    SetAlarm(); // Set Alarm
                    mode = 1;
                    break;
                }
                case 3: SetTemp();
                    break;
                case 4:
                {
                    SetAlarm();
                    mode = 1;
                    break;
                }
            }
            scr = 1;
            print_configuration_screen();
        }
    }
}

I am attaching screenshots to better help you guys understand the point when issue arises:

Everything fine

Everything is fine until I go inside the SetTime or SetAlarm routine. enter image description here

As I press UP/DOWN to set time, on each press, a few 7-8 stack overflows occur. enter image description here

After I come out of the ConfigurationMode() using BACK, the MCU goes haywire and stack overflows start happening in thousands until it crashes. enter image description here

\$\endgroup\$
  • 1
    \$\begingroup\$ One overflow is too many, at that point your program can't be trusted to do anything sensible any more. \$\endgroup\$ – Colin Mar 23 '17 at 6:44
  • \$\begingroup\$ @Colin__s This is the first time I am attempting to write a code with so many nested loops plus interrupts happening at the same time. Can you please identify how I should handle these nested loops? \$\endgroup\$ – Mohsin Anees Mar 23 '17 at 6:49
2
\$\begingroup\$

You can try, in the ConfigurationMode(), combining case 2 and case 4 and instead of calling SetAlarm cut the code and paste it into case 2/4.

Same for case 3 if SetTemp is going to have some code code it into case 3 rather than calling SetTemp(). For now you can remove SetTemp() from case 3.

The same for case 1 move the setTime()

If ConfigurationMode() is called from only one place, move the code where it is called.

The rule being, do not call procedures unless there is a justifiable reason.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your help. I will alter my code and see what happens. \$\endgroup\$ – Mohsin Anees Mar 23 '17 at 7:26
  • \$\begingroup\$ Can you just declare the function 'inline'? \$\endgroup\$ – Scott Seidman Mar 23 '17 at 13:07
  • \$\begingroup\$ @ScottSeidman I am not familiar with PIC. It's better not to declare a function unless absolutely necessary. Many programmers use functions and procedures to make their code more readable and easier to maintain. If a routine is called from only one location, then it is wasteful to performance and stack resources. Every time a routine is called the current state of the micro must be saved in the stack. Then the state is restored when the routine completes. Be careful where you pick up programming techniques, most of the sample code you find on the internet is crap. \$\endgroup\$ – Misunderstood Mar 23 '17 at 15:19
  • \$\begingroup\$ @Misunderstood -- the code for a function declared to be inline is simply copied into the caller before compile. See mouser.com/ds/2/268/…, page 158. I'm well aware of the overhead involved in calling functions, but this option can make code more understandable and maintainable without the overhead. \$\endgroup\$ – Scott Seidman Mar 23 '17 at 15:36
  • \$\begingroup\$ When you asked I did not realize you were trying to tell me something. Okay, I read it. It's a better way. Good to know. It does have dependencies, and I assume the compiler will warn you when it cannot execute as an inline function. I do not do much embedded or C coding, and when I did it was 20 years ago, so not familiar with inline functions. \$\endgroup\$ – Misunderstood Mar 23 '17 at 15:57
4
\$\begingroup\$

The other answer made suggestions as to how you could overcome the stack overflow problem. However I feel it is necessary to point out why there is this problem with this type of microcontroller.

From the PIC16F87xA data sheet the block diagram of the MCU shows this: enter image description here

These processors have a hardware stack of 8 levels. That means that the number of levels of calls and interrupts can never be nested more than 8 deep. Exceed that and your program takes a break and goes out to lunch - and never makes it back to work.

This architecture has its limitations over others that place the stack off in the data memory where the stack can be much larger. The huge advantage of using a dedicated small register file stack like this is speed. Separate memory cycles are not needed to push and pop the return address for subroutine calls and interrupt service routines. That advantage does come with the need to carefully manage the stack level utilization throughout your program.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.