Glitchy 8051 Code

Help
2009-04-20
2013-03-12
  • Curtis Been

    Curtis Been - 2009-04-20

    Hello Everyone,

    I have some code below that seems to give me a glitchy output. This code is a attempt to strobe 2 of 8 Leds the sequence is as follows:
               0, 1, 2,
               3, 4, 5,
               6, 7,

    Led number 4 stays illuminated as 7, 6, 3, 0, 1, 2, 5 are  strobed and then it should strobe back to 7 in the opposite order. But what happens is the it goes as it should from 7 to 5 then all Leds light up then the light flashes back in the opposite order to 7 (as expected) then this is followed by a random sequence of flashes then it seems to reset and perform like it did at the beginning of this sentence. 
    This is a trivial program an attempt to learn the C language to program microprocessors so I'm not sure if this is an SDCC question or a C question. I appreciate all the help.
                           Thank you,
                             Curtis Been

    /**************************************

    Project:        Led Array

    Purpose:        Use an array to update the status of P1

    Processor:      Atmel at89c51ed2

    **************************************/
    #include <at89c51ed2.h>

            /*    Port Definition   */

    #define Leds P1

            /*      typedefs        */

    typedef unsigned char pbyte;
    typedef unsigned int pnumb;

            /*  static declarations */

    static pnumb Now = 5;
    static pnumb *pNow = &Now;

    bit Up_or_Down = 0;

            /*Maximum int_rate ~ 2500*/
    static pnumb count = 0, int_rate = 2100;

            /*      Prototype       */

    void delay(pnumb Pause);
    void exchange(pnumb *start_value, pnumb *end_value);
            /*      Start of main   */

    void main(void)  {

          pnumb x=0, y=0;
        pnumb Up_Clockwise[11] = {0xff, 0x10, 0x30, 0x14, 0x12,
                                         0x11, 0x18, 0x50, 0x90, 0xff};

            for(;;)
            {
                    Leds = Up_Clockwise[*pNow];
                    delay(1);
                    exchange(&y, &x);
                    if(*pNow == x)*pNow=y;
            }
    }

    void delay(pnumb Pause)  {

    pnumb i;

            for(i=0;i<Pause;i++)
            {
            ET1=1;
            TMOD=0x20;
            TH1=0x6;
            EA=1;
            TR1=1;
            }
    }

    void timer1(void) interrupt 3  {
            TF1=0;
            count++;

            if(count == int_rate)
            {
                    count = 0;

                    if(Up_or_Down == 0)Now--;
                    if(Up_or_Down == 1)Now++;
            }
    }

    void exchange(pnumb *start_value, pnumb *end_value)  {

    pnumb temp;

            if((*pNow == *start_value) && (Up_or_Down == 0))
            {       /*   Downward Count     */
            Up_or_Down =! Up_or_Down;
            *start_value = 9;
            *end_value = 1;
            }

              /*values are reversed here*/
            temp = *start_value;
            *start_value = *end_value;
            *end_value = temp;

            if((*pNow == *end_value) && (Up_or_Down == 1))
            {       /*   Upward Count       */
            Up_or_Down =! Up_or_Down;
            *start_value = 1;
            *end_value = 8;
            }      
    }

     
    • Maarten Brock

      Maarten Brock - 2009-04-23

      Your delay looks kind of strange to me.

      I also suggest to make count and int_rate static members of the ISR.

      But a more serious problem is that you change the value of Now which is a multibyte variable inside the ISR but do not protect access when outside the ISR. You should make all accesses to it atomic and a good way to do it here is to change it to a char in __data memory (default for small model). Otherwise use the critical keyword. I also advise to let only the ISR or only the main loop write to it instead of both.

      Maarten

       

Log in to post a comment.