Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am working on project in which I need to display different colors on RGB led. I am using pwm to drive different colors on LED. My Pic is PIC24FJ64GA004 with which I am working on now. Basic concept of this project is to use switch to controls colors.

Colors on RGB led will be according to days and month in year. For that I am using 7-segment led with switch to count days and month.

Problem is that I have only one switch on my board. I have designed hardware and bits for it has been tested as well. So hardware works fine.

At the moment my problem is pic code.

I am stuck in switch use now. I have one switch on board. I need to display how many times switch pressed on 7 segment. Problem is that I am new to Pic code and confuse as well. First switch status will be checked. If switch pressed for two seconds it will go days mode. otherwise it will stick to months mode.It will display month or days according to that.I have pasted my code here please give me some positive advice.

I would like to know is this coding is going in right way or not.

void switch_function(void)
{
    if (PORTAbits.RA4==1)               // is SW1 pressed?
    {                            
        IEC0bits.T1IE       = 1;        // Enable Output Compare interrupts
           T1CONbits.TON        = 1;        // Start Timer1 with assumed settings   
        if (modecounter==0)                 // Checking status of month  
        {
            if (PORTAbits.RA4==1) 
            {    
                counter++;
                if (counter== 12)
                {   
                    counter = 0;
                } 
            }
        } 
        else if(modecounter ==1)            // Checking status of days
        {
            if (PORTAbits.RA4==1)
            { 
                counter++;
                if (counter== 32)
                {  
                    counter = 0 ;
                }
            }
        }
        else
        {
        }     
    }
    else
    {
        counter =0;
    }
    return ;
}

  //***** Timer1 interrupt *****//

void __attribute__((interrupt, auto_psv)) _T1Interrupt(void)
{ 
    timer_counter++;

    if (timer_counter== 2)
    {
        if (PORTAbits.RA4==1) 
        {  // is SW1 still has pressed status after 2sec delay?
            modecounter = 1;         // switch to days 
        }
        else
        {
            modecounter = 0;  // switch to months
        }          
    }
    else
    {
    }
    IFS0bits.T1IF=0; /* clear timer1 interrupt flag */      
    return;
} 

//***** Timer2 interrupt *****//

void __attribute__ ((__interrupt__, no_auto_psv)) _T2Interrupt(void)
{
    IFS0bits.T2IF = 0; /* clear timer2 interrupt flag */    
    return;
}
share|improve this question

closed as off topic by Quentin Pradet, William Morris, Brian Reichle, Donald.McLean, palacsint Feb 12 '13 at 15:03

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

1  
Hello, and welcome to Code Review! Unfortunately, we only focus on already working code (see the faq), so your question is off-topic. Sorry for the inconvenience. –  Quentin Pradet Feb 12 '13 at 12:46
    
Post it on stackoverflow.com with tags c embedded pic. –  Lundin Feb 13 '13 at 15:02
add comment

1 Answer 1

up vote 0 down vote accepted

A few comments:

  1. Why check this twice PORTAbits.RA4==1 in the same if-block?
  2. Following can be rewritten:

    counter++;
    if (counter== 12)
    {   
        counter = 0;
    } 
    

    to:

    if(++counter == 12)
    {
        counter = 0;
    }
    

    or:

    counter = (++counter == 12) ? 0 : counter;
    
  3. Same here

    if (PORTAbits.RA4==1) 
    {  // is SW1 still has pressed status after 2sec delay?
        modecounter = 1;         // switch to days 
    }
    else
    {
        modecounter = 0;  // switch to months
    }
    

    to:

    modecounter = (PORTAbits.RA4==1) ? 1 : 0;
    
share|improve this answer
    
I am using switch twice because I want to switch counter from month to days. –  MZF786 Feb 12 '13 at 16:00
    
First It will add month and then will add days. In order to switch to days. Need to press switch for 2 seconds. –  MZF786 Feb 12 '13 at 16:01
add comment

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