Adding using getchr()

I was trying to make a program that would get a letter from the serial then add up all the numbers that follow it until the next letter appears in the serial port. I was using P2 for a visual aid only. Anyone know of a link I could go to so I could get on the right path.?

#include <reg51.h>
#include <stdio.h>

unsigned int getchr(void);
int tempval;
int result;
int X;


void main(void)
{

X = 0x00;
SCON = 0x50;
TMOD = 0x20;
TH1 = 0xFD; //9600 baud
TR1 = 1;
TI = 1;


while (1)
{

P2 = 0;

while (1)
{
tempval = getchr(); //GET CHAR FORM SERIAL PORT

if(tempval == 0x58 && result != 0x58) //CHECK FOR "X"
{result = 0x58;
tempval = 0x00;} // CLEAR tempval

if(result == 0x58 && tempval != 0x00)
{X = X +(tempval - 0x30);
// ADDS VALUE FROM X AND KEEPS ON ADDING EVEN IF A KEY
//WHEN SERIAL DATA ISN'T COMING IN.
tempval = 0x00;} // CLEAR tempval

if(result == 0x5A) //CHECK FOR "Z"
{X = 0x00; // CLEAR X
result =0x00;} // ZERO P2


P2 = X; //SEND X TO PORT P2

}
}
}


unsigned int getchr(void)
{
unsigned int chr, ch1;
ch1 = 0x00;
chr = 0x00;
ch1 = SBUF; //GET CHARACTER
chr = ch1 & 0x7f; //MASK OFF 8TH BIT
RI = 0; //CLEAR STATUS
return(chr);
}

Parents
  • Observations:

    - This program is a state machine. The use of result and tempval gives you an ad hoc sort of state, but it would be more clear to simply have an explicit state variable.

    - You can use character literals ('X') instead of numeric ones (0x58) for clarity. Most likely you should define constants for even better readability.

    - Take a look at the standard library function isdigit(), and some of its friends like isupper() if case sensitivity is an issue.

    - When you clear X upon detecting a Z, you destroy the sum you've accumulated by assigning 0. Presumably you wanted the sum for a reason.

    - There's a perfectly good getchar() already implemented. What's the mask to 7 bits for? Are you worried about parity? Or just being really finicky about your definition of ASCII?

    - Are you sure you want the sum of the single digits, as ASCII, between the X and Z, and not the value of the integer delimited by those characters? That is, if you get X123Z, you want the result to be 6, and not 123?

    - What happens if the input isn't well formed, and you get some other input (from line noise or whatever)?

    typedef enum
        {
        Idle,
        SummingDigits
        } InputState;
    
    InputState state;
    
    #define  SequenceStart   'X'
    #define  SequenceEnd     'Z'
    
    void main (void)
        {
        char c;
        int sum;
    
        state = Idle;
    
        for (;;)
            {
            c = getchr();
    
            switch (state)
                {
                case Idle :
                    if (c == SequenceStart)
                        {
                        sum = 0;
                        state = SummingDigits;
                        }
                    // else bad input; ignore
                    break;
    
                case SummingDigits :
                    if (c == SequenceEnd)
                       {
                       state = Idle;
                       // probably want to do something with sum here
                       }
                    else if (isdigit(c))
                       {
                       sum += c - '0';
                       }
                    // else bad input
                    break;
    
                } // switch state
    
            } // loop forever
    
        } // main
    
    

Reply
  • Observations:

    - This program is a state machine. The use of result and tempval gives you an ad hoc sort of state, but it would be more clear to simply have an explicit state variable.

    - You can use character literals ('X') instead of numeric ones (0x58) for clarity. Most likely you should define constants for even better readability.

    - Take a look at the standard library function isdigit(), and some of its friends like isupper() if case sensitivity is an issue.

    - When you clear X upon detecting a Z, you destroy the sum you've accumulated by assigning 0. Presumably you wanted the sum for a reason.

    - There's a perfectly good getchar() already implemented. What's the mask to 7 bits for? Are you worried about parity? Or just being really finicky about your definition of ASCII?

    - Are you sure you want the sum of the single digits, as ASCII, between the X and Z, and not the value of the integer delimited by those characters? That is, if you get X123Z, you want the result to be 6, and not 123?

    - What happens if the input isn't well formed, and you get some other input (from line noise or whatever)?

    typedef enum
        {
        Idle,
        SummingDigits
        } InputState;
    
    InputState state;
    
    #define  SequenceStart   'X'
    #define  SequenceEnd     'Z'
    
    void main (void)
        {
        char c;
        int sum;
    
        state = Idle;
    
        for (;;)
            {
            c = getchr();
    
            switch (state)
                {
                case Idle :
                    if (c == SequenceStart)
                        {
                        sum = 0;
                        state = SummingDigits;
                        }
                    // else bad input; ignore
                    break;
    
                case SummingDigits :
                    if (c == SequenceEnd)
                       {
                       state = Idle;
                       // probably want to do something with sum here
                       }
                    else if (isdigit(c))
                       {
                       sum += c - '0';
                       }
                    // else bad input
                    break;
    
                } // switch state
    
            } // loop forever
    
        } // main
    
    

Children
More questions in this forum