0
\$\begingroup\$

The code below flashes some LEDs in order: pin 1, pin 2, pin 3, pin 4.

After this sequence has completed I would like to reverse direction and show pin 4, pin 3, pin 2, pin 1.

I wrote the code below but it's not working in reverse order.

What might be wrong?

#include <htc.h>

__CONFIG(1,OSCSDIS & HSPLL);
__CONFIG(2,BORDIS & PWRTDIS &WDTDIS);
__CONFIG(3,CCP2RC1);
__CONFIG(4,LVPDIS & STVREN);
__CONFIG(5,UNPROTECT);
__CONFIG(6,WRTEN);
__CONFIG(7,TRU);

#define _XTAL_FREQ   40000000


void delay_sec(unsigned char seconds)    // This function provides delay in terms of seconds
{
    unsigned char i,j;

    for(i=0;i<seconds;i++)
        for(j=0;j<100;j++)
            __delay_ms(10);
}
void led_display(char a)
{
    switch(a)
    {
        case 0: PORTB=0x01;PORTD=0x08;  break;
        case 1: PORTB=0x02;PORTD=0x04;  break;
        case 2: PORTB=0x04;PORTD=0x02;  break;
        case 3: PORTB=0x08;PORTD=0x01;  break;
    }
}

void main()
{
    TRISB=0x00;
    TRISD=0x00;
    char a,b;

    while(1)
    {
        led_display(a);

        a++;
        delay_sec(1);
        if(a==4)
        {
            a--;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

You increment a every time and then you decrement it if a == 4. So a equals three in the beginning of you while-loop, then you increment it to four and decrement it, because it equals four. Now it is three again when you jump back to the start of loop.

Try something like:

bool directionForward == true;

while(1) {
    led_display(a);
    delay_sec(1);
    if(directionForward) {
        a++;
        if(a==4) directionForward = false;
    } else {
        a--;
        if(a==0) directionForward = true;
    }
}

And one other point: Try to avoid using this delay-function. Use a timer interrupt instead.

\$\endgroup\$
  • \$\begingroup\$ Your first line uses a comparison operator instead of an assignment operator, which is what I'm sure you meant. Also, I don't think htc has a bool type. I do think it has a bit type, though. Not sure if true and false are built-in. \$\endgroup\$ – Dan Laks Oct 31 '14 at 17:07
2
\$\begingroup\$

As your code currently stands, it will increment a until it reaches 4, then it will decrement it to 3 and then increment it back to 4.

That is, led_display will be called with the following arguments

0
1
2
3
3
3
....

If I understand you correctly, you would like it to successively increment from 0 until it reaches 3, and then decrement its way down to 0

The way I would implement that is by using a "direction variable", one way of doing so would look like this

TRISB=0x00;
TRISD=0x00;
char a=0, b, dir=1;


while(1)
{
    led_display(a);

    a+=dir;
    delay_sec(1);

    if(a==3)
    {
        dir=-1;
    }

    else

    if(a==0)
    {
        dir=1;
    }

}

NOTE: this code is effectively the same as GER_Moki's, just the sequence and the operations are purposely verbose and closer to your original code for clarity

\$\endgroup\$
  • 2
    \$\begingroup\$ Or, even shorter: if (a==3 || a==0) dir = -dir; \$\endgroup\$ – RJR Oct 31 '14 at 8:03
  • 3
    \$\begingroup\$ I don't like writing magic numbers... Add #define UP 1 and #define DOWN -1 to the code and use it (dir=UP; dir=DOWN). \$\endgroup\$ – GER_Moki Oct 31 '14 at 10:52
  • \$\begingroup\$ Both completely correct comments. I consciously made the decision to keep it simple and aim it directly toward the questioner, who I feel may be not be a confident programmer; if my answer can address the question as simply as possible, then these two additions can only help along any beginning programmer toward mastery. \$\endgroup\$ – Al Longley Oct 31 '14 at 15:55
0
\$\begingroup\$

In addition to what is presented in these other answers, I don't see where the variable a is ever initialized. Since it is a local (on the stack) it is not be guaranteed to be 0 like it would be if it was a global variable.

The code currently reads:

char a,b;

while(1)
{
    led_display(a);

It should be something like:

char a,b;

a = 0;

while(1)
{
    led_display(a);

no matter what other changes are made.

\$\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.