timing problem returning a pointer to an array

The following sketch (extracted from a much larger project) illustrates a problem I've run into. Perhaps someone here more versed in C/C++ can explain what's happening. NOTE: compiled with Arduino 1.0.3 and run on a Nano 3.0 clone.

void setup (){
  Serial.begin (115200);
}

void loop (){
  SEND_JOYSTICK_MESSAGE ();
  Serial.println ("");
  delay (1000);
}


void SEND_JOYSTICK_MESSAGE (){
  byte Category = B01; // Roboteq SetCommand
  byte Name = 0; // _G, but will contain data for both channels
  byte Target = B001; // Roboteq target ID
  byte ID[4];
  byte * IDaddress;
  IDaddress = CREATE_ID (Category, Name, Target);
  ID[0] = *IDaddress;
  ID[1] = *(IDaddress+1);
  ID[2] = *(IDaddress+2);
  ID[3] = *(IDaddress+3);
  Serial.println ("returned ID bytes:");
  Serial.print (ID[0]); // should be 64
  Serial.print ("    ");
  Serial.print (ID[1]); // should be 32
  Serial.print ("    ");
  Serial.print (ID[2]); // should be 0
  Serial.print ("    ");
  Serial.println (ID[3]); // should be 0
}

byte * CREATE_ID (byte Category, byte Name, byte Target) {
  unsigned int ID;
  Serial.print ("Category = ");
  Serial.print (Category);
  Serial.print ("   Name = ");
  Serial.print (Name);
  Serial.print ("   Target = ");
  Serial.println (Target);
  ID = (uint16_t) ((Category << 6 | Name ) << 3 | Target) << 5;
  Serial.print ("ID as uint16_t = ");
  Serial.println (ID);
  byte IDbytes[4];
  byte * IDaddress;
  IDbytes[0] = (ID>>8); // 64
  IDbytes[1] = ID; // 32
  IDbytes[2] = 0x00;
  IDbytes[3] = 0x00;
  IDaddress = IDbytes;
  delay (1);
  return IDaddress;
}

This functions exactly as it should. All of the calculation takes place in the CREATE_ID function. Three bytes (that happen to have the values 1, 0 and 1) are combined to make a uint16_t (in this case 16416 decimal) the upper 11 bits of which are a standard CAN identifier. That 16 bit number is then divided into 2 bytes (having the values of 64 and 32) which become the first two bytes of a 4-byte array. The remaining two bytes are set to 0. CREATE_ID then returns a pointer to that array, and the calling procedure just Serial.prints the results. Here's a sample of the output.

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
64    32    0    0

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
64    32    0    0

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
64    32    0    0

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
64    32    0    0

That's all OK, so what's the problem? The problem arises if the next to the last linedelay (1); is commented out. The output is then like this:

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
0    107    0    106

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
0    107    0    106

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
0    107    0    106

Category = 1   Name = 0   Target = 1
ID as uint16_t = 16416
returned ID bytes:
0    107    0    106

Why is that delay needed before returning the pointer?

Indeed, in the longer program from which I took this snippet, no delay gave consistent garbage results, delay(1) gave the correct results most of the time, but gave nonsense perhaps 1 time in 20, while delay(2) makes things behave as they should.

Can someone enlighten me?

Ciao,
Lenny

The mistake you are making is to return the address of local storage from a function. The 4-byte array ID ceases to exist when you return from function CREATE_ID, so you must not return a pointer to it. Try declaring ID with the 'static' storage class, that way it will persist when you return from the function.

Thanks dc42. Though I confess that this both makes sense and doesn't make sense to me. It makes sense that a pointer to a local array may be pointing to something else by the time I de-reference the pointer. Declaring the array static does indeed fix things.

What doesn't make sense is that that putting a delay before return seems keep that local array in memory for some extended time after the return. Obviously not a reliable "solution", but I am curious to know why that happens.

Ciao,
Lenny

Adding the delay call may be affecting where on the stack the array is allocated, because without it, the compiler can probably avoid allocating memory for variable IDaddress.

Once the local variable has gone out of scope, the content of its memory location is undefined. Innocuous changes to the code can affect whether or not those particular bytes get overwritten and when. The fact that it ever worked is a fluke.

If you only want to return a four-byte value then you could return them as a long, or store them in a static variable (you'd only be able to use that to hold one value, though, which could be misleading if this is not obvious at the API) or store them in a global variable (same restrictions, but less likely to be misleading) or having the caller pass in a pointer or reference argument indicating where the result should be stored.

You could also define a struct type which contains an array of 4 bytes, then return a struct from the function.

Thanks guys. As I will be using the array downstream, I don't want to convert to a long. The "static" modifier suits my needs very well, and would be needed to pass a pointer to an array or a pointer to a struct that contains an array. Ciao, Lenny

I wasn't suggesting passing a pointer to the struct, but rather the struct itself (as it is only 4 bytes in size it isn't massively inefficient).

Looking at the way your Create ID function works, you could even go a step further and try something like this:

typedef union{
  struct{
    //First byte in array
    unsigned name:6;
    unsigned category:2;
    
    //Second byte in array
    unsigned empty:5;
    unsigned target:3;
    //(Note first and second bytes have been swapped due to endian-ness)
    
    //remaining 2 bytes in array.
    unsigned int zero; 
  };
  byte array[4]; //access bytes through array as before.
  unsigned long value; //so its full value can be seen
} ID_TYPE;

ID_TYPE CREATE_ID (byte Category, byte Name, byte Target); //needed prototype to avoid it being added automatically before the ID_TYPE definition.


void setup (){
  Serial.begin (115200);
}

void loop (){
  SEND_JOYSTICK_MESSAGE ();
  Serial.println ("");
  delay (1000);
}

void SEND_JOYSTICK_MESSAGE (){
  byte Category = B01; // Roboteq SetCommand
  byte Name = 0; // _G, but will contain data for both channels
  byte Target = B001; // Roboteq target ID
  ID_TYPE ID;
  ID = CREATE_ID (Category, Name, Target);
  Serial.println ("returned ID bytes:");
  Serial.print (ID.array[0]); // should be 64
  Serial.print ("    ");
  Serial.print (ID.array[1]); // should be 32
  Serial.print ("    ");
  Serial.print (ID.array[2]); // should be 0
  Serial.print ("    ");
  Serial.println (ID.array[3]); // should be 0
}

ID_TYPE CREATE_ID (byte Category, byte Name, byte Target) {
  ID_TYPE ID = {Name,Category,0,Target,0};
  //The line above initialises the struct in the union.
  //The same could be accomplished with:
  /*ID_TYPE ID;
  ID.name = Name;
  ID.category = Category;
  ID.target = Target;
  ID.empty = 0;
  ID.zero = 0;*/
  
  Serial.print ("Category = ");
  Serial.print (Category);
  Serial.print ("   Name = ");
  Serial.print (Name);
  Serial.print ("   Target = ");
  Serial.println (Target);
  Serial.print ("ID as uint32_t = ");
  Serial.println (ID.value); //this value is different to yours due to endian-ness, but the resulting array is the same.
  return ID;
}

It's be so much simpler just to make the array a global - no need to return it from a a function or wrap it in a struct.

Although using static worked, I think I've come up with a simpler way of doing what I want to do. Instead of returning a pointer from a function, if I do things as I would have done in Pascal or PerfectScript (WordPerfect's scripting interpreter), I can pass a pointer to the array I want to fill as a parameter. Like so:

void setup (){
  Serial.begin (115200);
}
void loop () {
  SEND_JOYSTICK_MESSAGE ();
  delay (1000);
}
void SEND_JOYSTICK_MESSAGE (){
  byte Category = B01; // Roboteq SetCommand
  byte Name = 0; // _G, but will contain data for both channels
  byte Target = B001; // Roboteq target ID
  byte ID[4];
  CREATE_ID (Category, Name, Target, ID);
  Serial.println ("ID bytes:");
  Serial.print (ID[0]);
  Serial.print ("    ");
  Serial.print (ID[1]);
  Serial.print ("    ");
  Serial.print (ID[2]);
  Serial.print ("    ");
  Serial.println (ID[3]);
  // other stuff to compose CAN message
}
void CREATE_ID (byte Category, byte Name, byte Target, byte * IDbytes) {
  unsigned int ID;
  ID = (uint16_t) ((Category << 6 | Name ) << 3 | Target) << 5;
  IDbytes[0] = (ID>>8);
  IDbytes[1] = ID;
  IDbytes[2] = 0x00;
  IDbytes[3] = 0x00;
}

This gives me what I want while reducing the number of RAM-occupying even-local variables to a minimum and doesn't require that anything persist beyond the moment that I need it. I think it's also cleaner and easier to read.

BTW, aside from a general prejudice about using globals, I have a particular reason for wanting to limit their use in this program. Most of the people who will use this program have essentially no programming knowledge and absolutely no interest in learning that skill. It will have a section at the top containing global variables for "user settings" and I'd like to leave things unencumbered with other globals unless absolutely needed. Most all of the code will be moved into a library and I'll provide a relatively simple "manual" so that the user can personalize hes/her settings.

Ciao,
Lenny

Yes, that' a better solution. And I agree about minimizing the use of global variables.