More problems with sprintf?

[nigelmercier]

[Repost in new thread, as a forum bump doesn't seem to work :)]

Having discovered sprintf (see here), I realised that it was idea for my time conversion function, see this thread. So I took the plunge, and converted it, and it worked for a while, but now something has gone wrong!

This function is passed an integer number of seconds, and returns a text string " HH:MM" derived from it. But it doesn't work anymore; I guess I'm doing something dumb!

Note that txt4[4] and txt7[7] are global scratchpad variables.

char * formatHours(int seconds) 
{ // convert integer seconds timer to string: " HH:MM" 
  byte hrs, mins ; 
  int fullmins ; 
  char time[7] = " " ;  // leading space 
 
 
  fullmins = seconds / 60 ; // convert seconds to minutes 
  hrs =  fullmins / 60 ;    // convert minutes into hours 
  mins = fullmins % 60 ;    // remainder of same division is minutes 
 
  sprintf(txt4, "%02d", hrs) ;    // format 2 places, leading zero 
  strcat(time, txt4) ;   // add to time string 
  strcat(time, ":") ;    // add a colon 
 
  sprintf(txt4, "%02d", mins) ;  // format 2 places, leading zero 
  strcat(time, txt4) ; // 
 
  return (strcpy(txt7, time)) ; 
} 

The calls to sprintf() return the value 3, so that bit is working. But the function returns something like "255:256"

[Banfa]

For what input value do you get that behaviour?

Also you should be able to do the whole thing with a single call to sprintf.

[nigelmercier]

The initial call to the function starts with 0, then increments every second. I let it run for a couple of minutes while I checked my code; does it look OK?

Yes, I'm sure I could do it with a single call, but I'm eating my elephant one byte at a time :)

[Banfa]

The code looks ok apart from the fact the the time array and txt7 are not nearly long enough to deal with the maximum values you might get.

Hours is seconds / 60 / 60, seconds is an int so the maximum value of hours (assuming a 32bit int is1193046 and this can be positive or negative so the maximum number of characters for the hours field is 8. Minutes is seconds / 60 % 60 so its maximum value is 59 again either positive or negative so its maximum field width is 3. Plus a space character plus a colon plus a NULL terminator means your minimum buffer size for time[] and txt7[] should be

BufferSize = 8 + 3 + 1 + 1 + 1 = 14

Twice as long as the 7 bytes provided by your program. In fact the output you have "255:256" is 8 bytes long and overruns the buffer length causing undefined behaviour in your program.

255 is a completely plausible value for hours given either enough running time or incorrect calling code passing the wrong value into the function.

256 for minutes is much harder to explain. You haven't said, IIRC, if this program is single threaded or multi-threaded and you are unnecessarily using the global buffer txt4 and txt7 is also not allocated locally allowing any part of your program to alter it. At this time some sort of buffer corruption (or accidental reuse) seems like the most obvious explanation for the minutes value.

[nigelmercier]

Thanks for your reply, but I'm a bit confused. I thought the sprintf buffer had to be large enough to contain the string that is returned? This would never be more than two characters: 59 minuits, or 24 hours. It certainly never got past a couple of minutes in testing.

There is a single thread, and I have checked the value of the return value before it returns, still wrong. In fact the values of the buffer just after the sprintf calls are also wrong. However, I did try a new variable as buffer of size 15 (the documentation said this could be necessary), same result.

[Banfa]

You are right, the sprintf buffer does have to be large enough to contain the returned buffer. It is your responsibility as the programmer to make sure that this constraint is met, sprintf has no way of knowing the size of the buffer it has been passed a pointer to it will just write as many characters as the format string and data require and if the buffer is not big enough then it will write outside the limits of the buffer causing undefined behaviour. The buffer you were supplying was not long enough to hold the maximum output of sprintf given the range of possible inputs to your function.

That is not the range of inputs you are expecting but the absolute maximum which in the case of your program happens if formatHours is passed a value -2147483648. However unlikely this is it is within the accepted range of values of your function and this is the value that gives the largest buffer usage; " −596523:-74"

It is important to oversize the buffer like this because you want to avoid any possibility of undefined behaviour which when it starts causing problems can be really hard diagnose.

I think you are probably going to have to get the value of seconds that are producing the erroneous output, either using a debugger or by putting it into the buffer allong with the converted values.

[Banfa]

BTW, are you defining byte as

typedef short byte;

or

typedef unsigned short byte;

?

This is almost certainly contributing to your problems as you are doing all your calculations using int and then storing results in short or unsigned short values. Changing hrs and mins to int might make it a bit more obvious where the error is but I remain convinced that the values you are passing to formatHours are in an odd range that you are not expecting.

[nigelmercier]

OK, so the sprintf buffer needs to be long enough to hold the possible values of my type byte (unsigned char). The mins calculation limits this to 59, and the hours is unlikely to get above 12 or so. Even if it did, it can't go above 255, so I figured that a buffer of 4 was OK. I'll try a longer buffer and report back.

The value of seconds starts at 0, and increments every second. After a couple of minutes, (with a value of around 150) the function is still returning "255..."

[Banfa]

How do you know the value of seconds is 150? Do you print it out or view it with a debugger?

Again I find it hard to believe that byte is unsigned char since mins is a byte and mins gets a value of 256 (although I am hard pressed to work out how) and 256 is out of range for a char either signed or unsigned. I am assuming your platform has 8 bit chars, is this true?

[donbock]

• Change the type for hrs and mins from 'byte' to 'int'.
• Add a trap at the front of your program for the case when the input seconds is negative.
• Add a trap at the front of your program for the case when hrs is bigger than you think it should be (ie, when it is greater than or equal to 100).
• Change function prototype so the caller provides the buffer to build the string in (to do that the caller should pass a pointer to the output buffer and its length). Caller can go ahead and specify txt7 if you wish, but your function ought not to assume some particular global buffer for its output.
• txt4 should be an automatic variable instead of a global.
  'time' is a reserved name; think of something else to call that variable.
• Explicitly check for and prevent overflow of the output buffer.

[nigelmercier]

Curiouser and curiouser ...

I have created a new buffer (char zonk[15]), turned off the interrupts, removed all the other processing, and hard coded 23 and 59 into the calls to sprintf.

char * formatHours(int seconds) // argument ignored for test
{ // convert integer seconds timer to string: " HH:MM"
   char zonk[15] ; // buffer with odd name to make sure it is unique
 
  sprintf(zonk, "%02d", 23) ;    // hrs format 2 places, leading zero
  strLCD5XY(zonk, 0, 2) ; // print on screen -> displays "279" *****
 
  sprintf(zonk, "%02d", 59) ;  // mins format 2 places, leading zero
  strLCD5XY(zonk, 44, 2) ;  // print on screen -> displays "315" *****
 }

Still not working, see ***** above.

[donbock]

@nigelmercier
So it doesn't work when you pass an integer constant ...

• sprintf doesn't work; or
• strLCD5XY doesn't work

Did sprintf come with your compiler? If so, then I suggest you take a close look at strLCD5XY. Are you sure you're passing the right arguments to it? Are you sure some other part of your code isn't corrupting the display after this snippet does the right thing?

[jkmyoung]

It is odd that those values are exactly 256 greater than the value passed in.

[nigelmercier]

@jkmyoung
Yes, that is interesting. It certainly explains why I was getting results like 256:257, as the passed values would have been 0 (hours) and the number of minutes.

The sprintf() function came with my compiler, strLCD5XY() is my own function, but simply takes a string argument and displays it at co-ordinates x, y. It has worked fine until now, and is used throughout the project.