Arduino to arduino serial communication without a library

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.


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.