Check sum and serial port communication

Question

I am trying to send a message with a check sum from an USB to serial port converter and back again. But I can't make it work for some reason even tough I have tried for a hole day. I am starting to think that there is something wrong with the hardware but I thought that it would be a good idea to ask you guys before giving up... I am using cygwin for executing and compiling programs on a windows computer.

I would have liked to attach a picture of the terminal when the program is running but you have to have an reputation of >10 which I ain't :(. So I will have to describe it instead..... I get the following information from the terminal when I try to send 'a'.

Enter a message: a
The checksum for your message to send is: 97
Your message to send is: a
a@
The checksum of the received message is: 97
The message wasn't properly received!
You received the following: a
a@

I receive what I send, but the check sum for 'a' should be 'a' right? So the string that should have been sent is "aa" instead of "a
a@" or am I completely lost here.

I have attached the relevant parts of the code below (printWhite() etc. change the color of the text and nothing more)

void main(){
     regularSend(); 
     regularReceive();
}

void regularSend(){ 
    char buff2s[20];
    unsigned char calculatedCS;
    printf("Enter a message: ");
    fgets(buff2s,15,stdin); // string input from keyboard
    calculatedCS=checkSum(buff2s,strlen(buff2s));
    printf("The checksum for your message to send is: %i\n",calculatedCS); 
    buff2s[strlen(buff2s)]=calculatedCS; //adds a checksum
    buff2s[strlen(buff2s)]='\0'; //adds a new string terminator
    printf("Your message to send is: %s\n",buff2s);
    bytes_written=write(fd,buff2s,strlen(buff2s));
    sleep(1); 
}



 void regularReceive(){
    unsigned char buffR[20];
    unsigned char rCS;
    bytes_read=read(fd,buffR,sizeof(buffR)-1);
            if(strlen(buffR)<1){ 
                printRed();
                printf(" No reply from the serial device! \n");
                printWhite();
            }
            else{
                rCS=checkSum(buffR,strlen(buffR)-2); //1 byte extra now
                printf("The checksum of the received message is: %i\n", rCS);
                true_or_false=check_checkSum(buffR,strlen(buffR)-1);
                    if(true_or_false==1){
                        buffR[strlen(buffR)-1]='\0'; //removes checksum
                        printf("Your message was: %s\n",buffR);
                    }
                    else{
                    printRed();
                    printf("The message wasn't properly received!\n");
                    printWhite();
                    printf("You received the following: %s\n\n",buffR);
                }
            }
}

unsigned char checkSum(char buff[], unsigned char nbrOfBytes){
    unsigned char ic;
    unsigned int t_cSum=0;
        for (ic=0;ic<nbrOfBytes-1;ic++){
            t_cSum=t_cSum+buff[ic];
        }
    return (unsigned char) t_cSum; //returns a 8bit checksum
}
unsigned char check_checkSum(char buffR_t[], unsigned char nbrOfBytes){ //<- check this
    unsigned char r_cSum=checkSum(buffR_t,nbrOfBytes-1); //calculates the expected value of the checksum byte
        if(r_cSum==(unsigned char)buffR_t[nbrOfBytes-2]){
            printGreen();
            printf("A message has been received! \n");
            printWhite();
            return 1;
        }
        else{
            return 0;
        }
}

Is someone able to spot my mistakes(I am almost sure that there is at least 2)? I would appreciate any help in this matter! Best regards Henrik

Answer 1

As @Roddy pointed out, code is bad for it overwrites the string's \0 with the checksum, then tries to find the string length in the next line of a potentially unterminated string!

buff2s[strlen(buff2s)]=calculatedCS; //adds a checksum
buff2s[strlen(buff2s)]='\0'; //adds a new string terminator

Suggest:

size_t len = strlen(buff2s);
calculatedCS = checkSum(buff2s, len);
buff2s[len] = calculatedCS; //adds a checksum
buff2s[++len] = '\0'; //adds a new string terminator
bytes_written=write(fd, buff2s, len + 1);  

Since working with strings, change checksum generator and checker to insure it does not create a '\0'.

unsigned char checkSum(char buff[], size_t nbrOfBytes){
    size_t ic;
    unsigned int t_cSum = 0;
    for (ic=0; ic<nbrOfBytes-1; ic++){
        t_cSum += buff[ic];
    }
    return (unsigned char) (t_cSum % 255 + 1);
}

RegularReceive() will not work. Suggest reading via fgetc() until a '\0' occurs. (or timeout or buffer full). In particular the following is dangerous as it is not known that buffR ends with a '\0' and the next function is strlen(buffR).

bytes_read = read(fd, buffR, sizeof(buffR)-1);
if(strlen(buffR)<1){ 

Answer 2

 buff2s[strlen(buff2s)]=calculatedCS; //adds a checksum
 buff2s[strlen(buff2s)]='\0'; //adds a new string terminator

Bad :-(

Just because your string has a null terminator doesn't mean it's nulls all the way beyond. Your first 'strlen' tells you where the current end is. You overwrite that with the checksum, but where the 'new terminator' goes could be anywhere. Do this instead.

 int len = strlen(buff2s);
 buff2s[len]=calculatedCS; //adds a checksum
 buff2s[len+1]='\0'; //adds a new string terminator

(ideally, checking for buffer overflow as well...)