strcpy seems to be messing with previous serial out on Arduino

Question

I've always had a problem with the concept of char pointers, strings, Strings and most things pointer related. Maybe I'm too old for this ;-)

declared globally:

char * message;

the serialOut is a very short 8 character string, an identifier (X10D) and then the data (nnn), and null terminator. I am finding the data sent via serial to be trimmed at the front, missing the idetifier. On the first pass through, it would be full and correct, but on subsequent pass through, only the three digits were received.

message is a debug message which outputs to the screen for debugging.

device and onOff populate correctly.

function causing problems:

byte device = btag-X10_TAG_OFFSET;
byte onOff;
char serialOut[8];
memset (serialOut, 0, 8);
if(x10[device - 1]==1){
  onOff = 0;
}  else {
  onOff = 1;
}
x10[device-1]=-2;
sprintf(serialOut, "X10D%02i%i", device, onOff);
Serial.println(serialOut);
strcpy(message, serialOut); // this line appears to 'modify' the previous line

if I remove the last line and swap it with:

message = serialOut;

the preceeding Serial communications is complete!

If I have neither, then the data at the other end is garbage (not yet deciphered, but appear as unprintable characters—which is why I setup the debug).

I'm thinking this can't be related, but the equality seems to 'fix' the problem.

Answer 1

Because message is a pointer you have to make it point to some valid memory. If you don't do that, and it's a global variable, then it will be zero (i.e. pointing to NULL) and copying to it will lead to undefined behavior.

If you assign it to point to serialOut you instead have another case of undefined behavior, as it seems that serialOut is a local variable and it will be out of scope when the function it's defined in returns, making message point to unused memory.

The two obvious solutions are to either make message an array of big enough size to hold whatever you might want to copy into it, or dynamically allocate/reallocate enough space every time before you copy into it.

Answer 2

Dynamic allocations are a bad idea on AVR not because it is an 8bit MCU, but because of potential stack-heap collisions. The problem above is that you have a global char*, and being global, it gets initialized to NULL [this is part of the C standard, you have no control over this, it is done by the __do_clear_bss section of the final executable]. The moment you do strcpy() on this NULL pointer, you start writing at address 0. Now, if this was an x86 machine and you were working at an application level, the software would crash with a segmentation fault. There is no memory protection in AVR, so strcpy() will happily start copying the string to address 0 in RAM. This falls right into the register file if using absolute addresses, meaning that any variables that were cached on these registers are now trashed. Consider this test case:

#include <string.h>
int main(void) {
    char p[] = "hello";
    strcpy(0, p);
    while(1);
}

which compiles to:

-- snip --
00000096 <main>:
-- snip --
  a0:   cd b7           in  r28, 0x3d   ; 61
  a2:   de b7           in  r29, 0x3e   ; 62
  a4:   86 e0           ldi r24, 0x06   ; 6
  a6:   e0 e0           ldi r30, 0x00   ; 0
  a8:   f1 e0           ldi r31, 0x01   ; 1
  aa:   de 01           movw    r26, r28
  ac:   11 96           adiw    r26, 0x01   ; 1
  ae:   01 90           ld  r0, Z+
  b0:   0d 92           st  X+, r0
  b2:   8a 95           dec r24
  b4:   e1 f7           brne    .-8         ; 0xae <main+0x18>
  b6:   be 01           movw    r22, r28
  b8:   6f 5f           subi    r22, 0xFF   ; 255
  ba:   7f 4f           sbci    r23, 0xFF   ; 255
  bc:   80 e0           ldi r24, 0x00   ; 0
  be:   90 e0           ldi r25, 0x00   ; 0
  c0:   0e 94 63 00     call    0xc6    ; 0xc6 <strcpy>
-- snip --
000000c6 <strcpy>:
  c6:   fb 01           movw    r30, r22
  c8:   dc 01           movw    r26, r24
  ca:   01 90           ld  r0, Z+
  cc:   0d 92           st  X+, r0
  ce:   00 20           and r0, r0
  d0:   e1 f7           brne    .-8         ; 0xca <strcpy+0x4>
  d2:   08 95           ret

if done with avr-gcc version 4.8.2 and invoking the compiler with avr-gcc -O3 -mmcu=atmega168 -o avr.elf avr.c

From the AVR instruction set manual, the st instruction says: "Stores one byte indirect with or without displacement from a register to data space. For parts with SRAM, the data space consists of the Register File, I/O memory and internal SRAM (and external SRAM if applicable). For parts without SRAM, the data space consists of the Register File only."

which corroborates the above. Obviously, the compiler doesn't really know you trashed the registers, and will happily use them for other things, trashing them even further. Reading from them after the strcpy() will yield problems as well. You can fix the issue by making a buffer, if you want it global, as char message[8];. Since it is a global variable, all elements get automatically initialized to 0. As to why your data is getting trashed, I would have to see more of your code to determine why. Most likely though, due to memory corruption.

Cheers.