Arduino to arduino serial communication without a library

Question

So I'm trying to send an array from one arduino to another through serial communication, without using the software serial library. The arduinos are wired rx->tx, tx->rx and ground to ground. I have a struct which holds the array of data and 2 test characters. The entire struct is then sent 1 char at a time. The receiver then receives 1 char at a time, re-calculates the test chars and if the xor of the sent and calculated tests chars is 1 then we assume an error exists and turn on a red LED. If no error exists then we turn on a green LED. I'm having some issues with my code however. It never sends any data to the 2nd arduino. Can anyone see any reason why?

Sender:

#include <avr/io.h>
#include <util/delay.h>
#include <string.h>

#define F_CPU 16000000L
#define BAUD_RATE 9600
#define MYUBRR (F_CPU / 16 / BAUD_RATE) - 1
#define END_OF_MESSAGE 47
#define END_OF_SECTION 35
#define START_OF_MESSAGE 2
#define _BV(bit) (1<<(bit))

struct packet{
    /*Packet to send a part of the message. */
    char *data;
    char vert_check;
    char horz_check;    
};

void serialInit()
{
    UBRR0H = (char) (MYUBRR>>8);
    UBRR0L = (char) MYUBRR;
    UCSR0C = (3 << UCSZ00);
    UCSR0B = (1 << RXEN0) | (1 << TXEN0);
}

unsigned char serialCheckTx()
{
    return(UCSR0A & _BV(UDRE0));
}

void serial_send(unsigned char DataOut)
{
    while(serialCheckTx() == 0){;}
    UDR0 = DataOut;
}

int isOddParity(unsigned char myChar) {
    int parity = 0;
    for (myChar &= ~0x80;  myChar != 0;  myChar >>= 1) {
        parity ^= (myChar & 1);   // Toggle parity on each '1' bit.
    }
    return parity;
}

int main(void)
{
    serialInit();
    char hozcheck, vertcheck;
    /* Add message to array below to send 
     * Initialised to toets.
     */
    char *s = {'T', 'o', 'e', 't', 's'};
    struct packet sendpacket;
    sendpacket.data = s;
    hozcheck = 0;
    for(int i=0; i<strlen(s); i++){
        hozcheck += s[i];
    }
    vertcheck = (isOddParity(s[0]) + '0');

    sendpacket.horz_check = hozcheck;
    sendpacket.vert_check = vertcheck;

    serial_send(START_OF_MESSAGE);
    serial_send(sendpacket.horz_check);
    serial_send(END_OF_SECTION);
    serial_send(sendpacket.vert_check);
    serial_send(END_OF_SECTION);
    for(int j=0; j<strlen(s); j++){
        serial_send(s[j]);
    }
    serial_send(END_OF_MESSAGE);
}

Receiver:

#include <avr/io.h>
#include <util/delay.h>
#include <string.h>

#define END_OF_MESSAGE 47
#define START_OF_MESSAGE 2
#define F_CPU 20000000L
#define BAUD_RATE 9600
#define MYUBRR (F_CPU / 16 / BAUD_RATE) - 1
#define _BV(bit) (1<<(bit))
#define START_OF_MESSAGE 2
#define END_OF_MESSAGE 47
#define END_OF_SECTION 35

struct packet{
    char *data;
    char vert_check;
    char horz_check;    
};

void serialInit()
{
    UBRR0H = (char) (MYUBRR>>8);
    UBRR0L = (unsigned) MYUBRR;
    UCSR0C = (3 << UCSZ00);
    UCSR0B = (1 << RXEN0) | (1 << TXEN0);
}

unsigned char serialCheckRx()
{
    return(UCSR0A & _BV(RXC0));
}

unsigned char serialReceive()
{
    while (serialCheckRx() == 0){;}
    return UDR0;
}

void *array_concat(char *a, int an, char b){
    int s = sizeof(char);
    char *p = malloc(s * (an + 1));
    memcpy(p, a, an*s);
    memcpy(p + an*s, b, s);
    return p;
}

int isOddParity(unsigned char myChar) {
    int parity = 0;

    for (myChar &= ~0x80;  myChar != 0;  myChar >>= 1) {
        parity ^= (myChar & 1);   // Toggle parity on each '1' bit.
    }
    return parity;
}

int main(void)
{
    DDRB = 0b111111;
    serialInit();
    char tmp = 0, recvert = 0, rechoz = 0, *s, testhoz, testvert;
    int testbit = 0;
    /*to rebuild our packages*/
    struct packet packet1;
wait:
    while((tmp = serialReceive()) != 2){
        goto wait;
    }

    while((tmp = serialReceive()) != 35){
        /*Should be a single char for horizontal*/
        rechoz = tmp;
    }
    while((tmp = serialReceive()) != 35){
        /*Should be a single char for "vert" parity*/
        recvert = tmp;
    }
    while((tmp = serialReceive()) != 47){
        /*While input is not end of message grow s*/
        s = array_concat(s, strlen(s), tmp);
    }

    packet1.data = s;
    packet1.vert_check = recvert;
    packet1.horz_check = rechoz;
    testhoz = 0;
    for(int i=0; i<strlen(s); i++){
        testhoz += s[i];
    }
    testvert = ((isOddParity(s[0])) + '0');
    if((packet1.vert_check ^ testvert) == 1)
        testbit = 1;
    if((packet1.horz_check ^ testhoz) == 1)
        testbit = 1;
    if(testbit == 1){
        /*RED LED*/
        PORTB = 0b100000;
    } else {
        PORTB = 0b010000;
    }
    free(s);
}

Answer 1

Not sure it has a bearing on your problem or not, but I can't help highlight this:

wait:
    while((tmp = serialReceive()) != 2){
        goto wait;
    }

That has to be the the most evil piece of code ever written in the history of mankind. I'm sorry, but it is.

while((tmp = serialReceive()) != 2);

is all you need. Or if you must be "pedantic":

while((tmp = serialReceive()) != 2) {
    continue;
}

There is never any call to use goto (I don't know why it was ever even included in C), and certainly not to re-run a while loop that is going to re-run itself anyway.

---

In your receiver you are using dynamic memory, and doing it extremely badly.

First you start out with a pointer

char *s;

Then when you receive a data character you pass that pointer to your array_concat() routine as the source for where to copy data from:

s = array_concat(s, strlen(s), tmp);

You are at that point asking it for the length of the string in an undefined pointer, which is itself undefined. It could be 0, it could be 65535, or it could be anywhere in between.

Then in array_concat you allocate memory to a pointer depending on that string length (the Uno doesn't have 65535 bytes of RAM, so anything more than the available memory will fail to allocate). And then you copy that number of bytes from the undefined source to the (possibly undefined) destination. That will then make all sorts of horrible things happen.

Then, assuming you even get that far, you return the new pointer and immediately give it to s.

Next iteration (assuming the first iteration worked) you do it all again, but now you have lost what s used to point to. That memory is gone for good. You have created a memory leak.

So you need to:

1. Make sure s is initialized to NULL
2. Do not use strlen() since you aren't working with a string but an array of data - instead maintain a count of the number of bytes received
3. Only do any copying of data if s actually points to some valid data (not NULL)
4. free() the old data pointer once you have finished copying the data (if it was pointing to real allocated memory)
5. Don't use memcpy to copy one byte - or if you must, then pass it the address of the byte to copy, not the byte itself - otherwise it will try and copy data from the location specified by the value of the byte, which really isn't what you want.

TBH I wouldn't use any dynamic memory at all. Instead I would stick to fixed size arrays that are big enough to hold the longest message you will send / receive. It's less fraught with danger, though it can be more wasteful of RAM.

Alternatively, if you do want to use dynamic memory, then send the length of the data with the header of your packet and allocate all the memory you need in one malloc() call before receiving all the data.

Answer 2

One obvious problem with the code is that s is not 0-terminated. Taking strlen of such string techically is an Undefined Behaviour, and all bets are off.

I can only speculate that it does return something larger than the max value an integer may hold (keep in mind that it returns size_t). It means that the loop

for(int i=0; i<strlen(s); i++){
    hozcheck += s[i];
}

is in fact infinite, and the program neve got a chance to send anything.