0

I wrote this code in my exam, but it have some problems(and because of this i get failed).

While debugging through my board(i've got an atmel atmega328p Xplained mini), i don't know why, but while entering in the main switch and between switch (currentstatus) and case checkled: all my status variables are reset to zero. If i debug it via simulator it seems to work fine.

If i upload the code to the board it works bad, like if it doesn't recognise some interrupts.

Plus i want to ask you another thing. I use for my exam a daughter board that prevents to access to the right pin of PWM output and plus i have to drive specific outupt(the leds of my daughter board, for example in this code i've to drive PORTC5 and PORTC4), so i've to implement the PWM by hand. I've tried to implement the solution that you can see in the code(i'm using a timer of 1ms). Is there a more simple and smart solution?

Can you help me?

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

typedef enum{checkled, heatingup, exposure, stop} machine;
machine currentstate=checkled;

typedef enum{twenty, fourty, sixty, eighty} brightness;
brightness light=twenty;

typedef enum{twoseconds, fourseconds, eightseconds, sixteenseconds} exposuretime;
exposuretime time=twoseconds;

volatile uint16_t tick=0;
volatile uint16_t oldtick=0;

volatile uint8_t flagchanged=0;

ISR(PCINT2_vect)
{
    if((PIND&(1<<PIND2))==0)
    {
        if(currentstate==checkled)
            currentstate=heatingup;
    }

    if((PIND&(1<<PIND3))==0)
    {
        if(currentstate==exposure || currentstate==heatingup)
        {
            currentstate=stop;
            PORTC&=~(1<<PORTC4);
            tick=0;
        }
    }

    if((PIND&(1<<PIND4))==0)
    {
        if(currentstate==checkled)
        {
            flagchanged=1;

            if(time==sixteenseconds)//rise time
                ;
            else if(time==eightseconds)
                time=sixteenseconds;
            else if(time==fourseconds)
                time=eightseconds;
            else if(time==twoseconds)
                time=fourseconds;
        }
    }

    if((PIND&(1<<PIND5))==0)
    {
        if(currentstate==checkled)
        {
            flagchanged=1;

            if(time==sixteenseconds)//decrease time
                time=eightseconds;
            else if(time==eightseconds)
                time=fourseconds;
            else if(time==fourseconds)
                time=twoseconds;
            else if(time==twoseconds)
                ;
        }
    }

    if((PIND&(1<<PIND6))==0)
    {
        if(currentstate==checkled)
        {
            flagchanged=1;

            if(light==eighty)//rise brightness
                ;
            else if(light==sixty)
                light=eighty;
            else if(light==fourty)
                light=sixty;
            else if(light==twenty)
                light=fourty;
        }
    }

    if((PIND&(1<<PIND7))==0)
    {
        if(currentstate==checkled)
        {
            flagchanged=1;

            if(light==eighty)//decrease brightness
                light=sixty;
            else if(light==sixty)
                light=fourty;
            else if(light==fourty)
                light=twenty;
            else if(light==twenty)
                ;
        }
    }

}

ISR(TIMER1_COMPA_vect)
{
    tick++;
}

int main(void)
{
    //DDRs
    DDRD&=~((1<<PIND2)|(1<<PIND3)|(1<<PIND4)|(1<<PIND5)|(1<<PIND6)|(1<<PIND7));//input
    PORTD|=(1<<PORTD2)|(1<<PORTD3)|(1<<PORTD4)|(1<<PORTD5)|(1<<PORTD6)|(1<<PORTD7);//pull-up resistors

    DDRC|=(1<<PORTC0)|(1<<PORTC1)|(1<<PORTC2)|(1<<PORTC3)|(1<<PORTC4)|(1<<PORTC5);//output

    //PCINTERRUPT
    PCICR|=(1<<PCIE2);
    PCMSK2|=(1<<PCINT18)|(1<<PCINT19)|(1<<PCINT20)|(1<<PCINT21)|(1<<PCINT22)|(1<<PCINT23);

    //TIMER
    TCCR0A|=(1<<WGM01);
    TIMSK0|=(1<<OCIE0A);
    OCR0A=15;//((clockfrequency/prescaler)*time)-1 - 1millisecond
    TCCR0B|=(1<<CS02)|(1<<CS00);

    sei();

    volatile uint8_t counter=0;
    volatile uint16_t check=0;

    while(1)
    {
        switch (currentstate)//FORM HERE
        {
            case checkled://TO HERE every variable is reset to ZERO
                if(flagchanged!=0)
                {


                    if(light==twenty)//led intesita
                    {
                        PORTC&=~(1<<PORTC0);
                        PORTC&=~(1<<PORTC1);
                    }
                    else if(light==fourty)
                    {
                        PORTC|=(1<<PORTC0);
                        PORTC&=~(1<<PORTC1);
                    }
                    else if(light==sixty)
                    {
                        PORTC&=~(1<<PORTC0);
                        PORTC|=(1<<PORTC1);
                    }
                    else if(light==eighty)
                    {
                        PORTC|=(1<<PORTC0);
                        PORTC|=(1<<PORTC1);
                    }


                    if(time==twoseconds)//led time
                    {
                        PORTC&=~(1<<PORTC2);
                        PORTC&=~(1<<PORTC3);
                    }
                    else if(time==fourseconds)
                    {
                        PORTC|=(1<<PORTC2);
                        PORTC&=~(1<<PORTC3);
                    }
                    else if(time==eightseconds)
                    {
                        PORTC&=~(1<<PORTC2);
                        PORTC|=(1<<PORTC3);
                    }
                    else if(time==sixteenseconds)
                    {
                        PORTC|=(1<<PORTC2);
                        PORTC|=(1<<PORTC3);
                    }

                    flagchanged=0;
                }
            break;

            case heatingup:
                if(tick<=3000)
                {
                    if(tick!=oldtick)
                    {
                        oldtick=tick;

                        if((tick%500)==0)//duty cycle al 50%
                            PORTC^=(1<<PORTC5);

                        if((tick%5)==0)//5% duty cycle
                        {

                            if(counter<=94)//per il 95% del periodo sta spento
                            {
                                PORTC&=~(1<<PORTC4);
                            }
                            else if(counter>=95 && counter<=101)//per il 5% del periodo sta acceso
                            {

                                if(counter==101)
                                {
                                    PORTC&=~(1<<PORTC4);
                                    counter=0;
                                }
                                else
                                PORTC|=(1<<PORTC4);

                            }

                            counter++;
                        }
                    }
                }
                else
                {
                    currentstate=exposure;
                    counter=0;
                    tick=0;

                    PORTC|=(1<<PORTC5);

                    //checking time of how much the lamp needs to be light up
                    switch (time)
                    {
                        case twoseconds:
                            check=2000;
                        break;

                        case fourseconds:
                            check=4000;
                        break;

                        case eightseconds:
                            check=8000;
                        break;

                        case sixteenseconds:
                            check=16000;
                        break;
                    }
                }
                break;

            case exposure:
                if(tick<=check)
                {
                    switch (light)//Checking duty cycle
                    {
                        case twenty://20% duty cycle
                            if(tick!=oldtick)
                            {
                                if((tick%20)==0)
                                {

                                    if(counter<=4)//80% off
                                    {
                                        PORTC&=~(1<<PORTC4);
                                    }
                                    else if(counter>=5 && counter<=6)//20% on
                                    {
                                        if(counter==6)
                                        {
                                            PORTC&=~(1<<PORTC4);
                                            counter=0;
                                        }
                                        else
                                        PORTC|=(1<<PORTC4);

                                    }

                                    counter++;
                                }
                            }
                        break;

                        case fourty://40% duty cycle
                            if(tick!=oldtick)
                            {
                                if((tick%20)==0)
                                {

                                    if(counter<=3)//60% off
                                    {
                                        PORTC&=~(1<<PORTC4);
                                    }
                                    else if(counter>=4 && counter<=6)//40% on
                                    {
                                        if(counter==6)
                                        {
                                            PORTC&=~(1<<PORTC4);
                                            counter=0;
                                        }
                                        else
                                        PORTC|=(1<<PORTC4);

                                    }

                                    counter++;
                                }
                            }
                        break;

                        case sixty://60% duty cycle
                            if(tick!=oldtick)
                            {
                                if((tick%20)==0)
                                {

                                    if(counter<=2)//40% off
                                    {
                                        PORTC&=~(1<<PORTC4);
                                    }
                                    else if(counter>=3 && counter<=6)//60% on
                                    {
                                        if(counter==6)
                                        {
                                            PORTC&=~(1<<PORTC4);
                                            counter=0;
                                        }
                                        else
                                        PORTC|=(1<<PORTC4);

                                    }

                                    counter++;
                                }
                            }
                        break;

                        case eighty://80% duty cycle
                            if(tick!=oldtick)
                            {
                                if((tick%20)==0)
                                {

                                    if(counter<=1)//20% off
                                    {
                                        PORTC&=~(1<<PORTC4);
                                    }
                                    else if(counter>=2 && counter<=6)//80% on
                                    {
                                        if(counter==6)
                                        {
                                            PORTC&=~(1<<PORTC4);
                                            counter=0;
                                        }
                                        else
                                        PORTC|=(1<<PORTC4);

                                    }

                                    counter++;
                                }
                            }
                        break;

                    }
                }
                else
                {
                    PORTC&=~(1<<PORTC4);
                    PORTC&=~(1<<PORTC5);
                    currentstate=checkled;
                    flagchanged=1;
                }

            break;

            case stop://Emergency stop
                if(tick<=2000)
                {
                    if(tick!=oldtick)
                    {
                        oldtick=tick;
                        if((tick%250)==0)
                        PORTC|=(1<<PORTC5);
                    }
                }
                else
                {
                    PORTC&=~(1<<PORTC5);
                    flagchanged=1;
                    currentstate=checkled;
                }
            break;
        }
    }
}
RawCode
  • 3
  • 4
  • 2
    Welcome to stackoverflow.com. Please take some time to read [the help pages](http://stackoverflow.com/help), especially the sections named ["What topics can I ask about here?"](http://stackoverflow.com/help/on-topic) and ["What types of questions should I avoid asking?"](http://stackoverflow.com/help/dont-ask). Also please [take the tour](http://stackoverflow.com/tour) and [read about how to ask good questions](http://stackoverflow.com/help/how-to-ask). Lastly please learn how to create a [**Minimal**, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve). – Some programmer dude Jul 07 '18 at 07:40
  • Also please read [this question checklist](https://codeblog.jonskeet.uk/2012/11/24/stack-overflow-question-checklist/), and all of http://idownvotedbecau.se/ to learn some reasons your question might be down-voted. Finally, please [learn how to debug your programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). – Some programmer dude Jul 07 '18 at 07:41
  • You might make the shown code and thereby the question easier to comprehend by translating to English all the identifiers. – Yunnosch Jul 07 '18 at 08:03
  • Sure give me 2 minutes – RawCode Jul 07 '18 at 08:04
  • Can you demonstrate the problem without all the interrupt related code? If not it probably means the interrupts are messing with your logic. If yes you will probably get better help sooner if you show the interrupt-free code here. It would much better match the M in MCVE. – Yunnosch Jul 07 '18 at 08:04
  • Can you prove that your device does not have unexpected resets? They are a plausible source for unexpectedly returning variables to their initial state. – Yunnosch Jul 07 '18 at 08:06
  • Does you device have a watchdog? If you accidentally activate it (or it is on by default) you need to pet it to prevent it from barking, which is one reason for unexpected resets. – Yunnosch Jul 07 '18 at 08:08
  • Yes i can prove that this problem is indipendent from the interrupt: i changed values of my variables from the *Watch card* and the problem persist. My professor never talked about watchdog timers, in this class we don't use them, so i don't know how to use them. EDIT: i translated my code – RawCode Jul 07 '18 at 08:30
  • It is not quite possible to determine what the controller does just by reading the code. I can only tell you what I would do to figure out the problem, step by step: The first step would be to comment out the sei() instruction, not allowing interrupts to occur. If the problem disappears, it is related to the interrupt, otherwise not. A very typical problem is a stack overflow on these small controllers. – Tim Wensky Jul 07 '18 at 08:39
  • I want to point out another thing: It is always a bad idea to change variables (in this case currentstate) in an ISR and in a normal function, since the time when the variable is changed is totally asynchronous. Just after the while(1) statement, make a copy of currentstate and work with that copy so you can be sure the value does not change while your are working with it in your main loop. – Tim Wensky Jul 07 '18 at 08:43
  • I've just commented out sei() instruction and now it seems to work. I don't get it: what is generating this issue? What interrupt? Thanks @TimWensky for the advice, i really appreciate it. – RawCode Jul 07 '18 at 08:59
  • You're welcome! Let's get further: enable interrupts again and place a 'return' statement right after ISR(PCINT2_vect) { return; } Check if the error occurs again. – Tim Wensky Jul 07 '18 at 09:02
  • Yes, the problem occurs again. I've commented out all my conditions and put *return;* as you suggested. – RawCode Jul 07 '18 at 09:13
  • Ok, it is very likely you are facing a stack overflow. When entering the ISR the stack grows too far and is overwriting your variables. I do not know your IDE, but there must be an option somewhere where you can change the stack size. Try and find it and increae the stack size, let's say about 32 bytes. – Tim Wensky Jul 07 '18 at 09:26
  • I don't know why, but i just commented out the timer instrucions and the timer ISR and the problem doesn't occur anymore. Maybe did i set something bad? – RawCode Jul 07 '18 at 09:26
  • I do not think this is the real problem! Be careful! I think this is, because these 2 ISR's are running at the same time and together let the stack overflow. I suppose these to ISR's are nested. Can you change the ISR priority, so these 2 ISR's can not interrupt each other? – Tim Wensky Jul 07 '18 at 09:32
  • Of course it is possible you have a mistake in your timer initialization or are using the wrong interrupt vector. Definitly check. Sorry, but I do not know these controllers very well, so I cannot tell you. – Tim Wensky Jul 07 '18 at 09:35
  • Thank you so much for the help! I found the error: i settet up the TIMER0, but in in the ISR i was checking TIMER1. Just a dumb error. What go you think about the PWM output? – RawCode Jul 07 '18 at 09:48
  • You're welcome. The PWM output is ok, but you can drastically simplify the code. Just a thing to think about which could be very useful for you: counter = ( counter + 1 ) % MAX_COUNT_VALUE; How could that help you? What does this do to all of your 'if' clauses using counter? Also, is it necessary to always compare counter like '<=' or '>=', wouldn't a '==' be enough in most cases? And if so, wouldn't a 'switch' statement be a lot nicer? If you're interested in optimizing you're code and making it more elegant, just post another comment here. I'll check here and then. – Tim Wensky Jul 07 '18 at 11:07
  • I don't get how *counter = ( counter + 1 ) % MAX_COUNT_VALUE;* could help me, but yes i would like to improve my coding abilities. Thanks so much! – RawCode Jul 07 '18 at 12:55
  • I don't think the comment section here is a good way to communicate. I did not find another way to contact you, tough. Can you send me your Email? – Tim Wensky Jul 07 '18 at 15:13
  • @RawCode contact me under tim@wy-ease.com. Talk to you soon. – Tim Wensky Jul 07 '18 at 15:21

2 Answers2

1

In the main code, you have output compare interrupt of timer 0 enabled

TIMSK0|=(1<<OCIE0A);

But you have no interrupt handler for this vector (you have TIMER1_COMPA_vect - for timer 1 instead).

So, when the timer interrupt happens, it is handled by the default vector, which just redirects control to the reset vector, which causes the application to start from the very beginning, including variable initialization, which leads to all unintended behavior you are observing.

So, the solution is simple: either disable OCIE0A, or create a handler for this interrupt.

AterLux
  • 4,566
  • 2
  • 10
  • 13
0

You are enabling the Timer/Counter0 Compare Match A interrupt with below line

TIMSK0|=(1<<OCIE0A);

However, you are trying to use the Timer/Counter1's Compare A Interrupt ISR in below section.

ISR(TIMER1_COMPA_vect) { tick++; }

Suggested Modifications:

Either you enable the Timer/Counter1 Compare Match A interrupt by modifying the timer initialization as below and keep the ISR intact:

`TIMSK1|=(1<

OR

Keep the Timer/Counter0 Compare Match A interrupt initialization intact and modify the ISR as below to call the ISR of Timer/Counter 0 on interruption.

ISR(TIMER0_COMPA_vect) { tick++; }

Hope this will help..

Sanu
  • 3
  • 3