Confused by passing character arrays?

[nigelmercier]

I'm still not comfortable with passing character arrays, and I think this is the problem with this code:

char * formatHours(int decimin)
{ // convert integer deci-mins (6 seconds) to string: "XXhYYm"
  byte hrs, mins ;
  char time[7] ;  // global scratch variable txt4 is also char [7]
  hrs =  decimin / (60 * 10) ; // convert 1/10s of a minute into hours
  mins = decimin % (60 * 10) ; // remainder of same division is minutes

  ByteToStr(hrs, txt4) ; 
  time[0] = txt4[2] ;
  time[1] = txt4[3] ;
  time[2] = "h" ;

  ByteToStr(mins, txt4) ;
  time[3] = txt4[2] ;
  time[4] = txt4[3] ;
  time[5] = "m" ;

  return time ;
}

It seems to return a blank string. If I substitute the last line with return "123456" ; then that displays OK, so I guess the fault is in the above.

This formatHours() function is called within a nested call, like this:

strLCD5XY(formatHours(resDeciMin), 48 , 5) ; // display on LCD in small font

I have successfully used strLCD5XY() with string arguments, viz:

char txt4[4] ;
// do something to txt4 here ...
strLCD5XY(txt4, 66, 2) ;      // display on LCD in small font

[Banfa]

You are returning a pointer to a local array, as soon as the function returns the array is destroyed so the pointer you return becomes invalid.

You need to return a pointer to data that has a lifetime that extends beyond the lifetime of the function call. That is either static data or data allocted on the heap. However each of these have there own problems.

For static data there is only 1 data buffer each successive call to the function over-writes the data so you can store the returned pointer for future use. Additionally in a multi-threaded environment 2 threads could try to run the function at the same time both accessing the buffer which would certainly produce the wrong results if not undefined behaviour. Any function using static data is not re-entrant.

For data allocated on the heap then the calling function has to store the returned pointer and free it when it has finished with it otherwise you get a memory leak.

The best solution (in my opinion) is for the calling function to pass a pointer to a buffer for this function to use (and may be a buffer size as well to guard against buffer overrun). You can still return the a pointer this buffer to allow you to use the function directly in a printf call. This way the calling function has complete control of the buffer used.

[nigelmercier]

ISWYM about the pointer, but strangely I got it to work:

  ByteToStr(hrs, txt4) ;
  strcat(time, txt4 +1) ;
  strcat(time, "h") ;
debug1 = mins ;
  ByteToStr(mins, txt4) ;
  strcat(time, txt4 +1) ;
  strcat(time, "m") ;

  return time ;

[Banfa]

No you did not. It only appears to work it is actually undefined behaviour.

Undefined behaviour is bad because once invoked anything can happen the behaviour of the program is completely undefined. One of the things that can happen is that the program appears to work all through testing right up until the time that it is critical that it does work and then fail. That is although it appears to work now it could stop working at any time in the future for no apparent reason.

[nigelmercier]

What I meant was that I got the function to do what I expected, now I need to consider the undefined behavior.

I have changed my return statement to:

return (strcpy(txt7, time)) ;

where txt7 is a global scratch variable. Is this OK?

[Banfa]

Well that will work as long as you understand the limitations of using a static data buffer as I outlined in post #2.

Also generally speaking global data is bad practice so you should give your global scratch variable the smallest scope possible. What that actually means is declaring it static inside the function like this

char * formatHours(int decimin)
{ // convert integer deci-mins (6 seconds) to string: "XXhYYm"
  static char txt7[7];

<Code Snipped> 
  return (strcpy(txt7, time)) ;
}

This limits the scope of txt7 to the formatHours function ensuring no-one else can mess with it directly.

[donbock]

What's the advantage of building the string in the time array and then copying it to the txt7 array compared with building the string in the txt7 array in the first place?