468,738 Members | 2,591 Online
Bytes | Developer Community
New Post

Home Posts Topics Members FAQ

Post your question to a community of 468,738 developers. It's quick & easy.

More problems with sprintf?

[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.

Expand|Select|Wrap|Line Numbers
  1. char * formatHours(int seconds) 
  2. { // convert integer seconds timer to string: " HH:MM" 
  3.   byte hrs, mins ; 
  4.   int fullmins ; 
  5.   char time[7] = " " ;  // leading space 
  6.  
  7.  
  8.   fullmins = seconds / 60 ; // convert seconds to minutes 
  9.   hrs =  fullmins / 60 ;    // convert minutes into hours 
  10.   mins = fullmins % 60 ;    // remainder of same division is minutes 
  11.  
  12.   sprintf(txt4, "%02d", hrs) ;    // format 2 places, leading zero 
  13.   strcat(time, txt4) ;   // add to time string 
  14.   strcat(time, ":") ;    // add a colon 
  15.  
  16.   sprintf(txt4, "%02d", mins) ;  // format 2 places, leading zero 
  17.   strcat(time, txt4) ; // 
  18.  
  19.   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"
Mar 14 '10 #1
13 3896
Banfa
9,057 Expert Mod 8TB
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.
Mar 14 '10 #2
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 :)
Mar 14 '10 #3
Banfa
9,057 Expert Mod 8TB
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.
Mar 14 '10 #4
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.
Mar 14 '10 #5
Banfa
9,057 Expert Mod 8TB
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.
Mar 15 '10 #6
Banfa
9,057 Expert Mod 8TB
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.
Mar 15 '10 #7
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..."
Mar 15 '10 #8
Banfa
9,057 Expert Mod 8TB
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?
Mar 15 '10 #9
donbock
2,422 Expert 2GB
  • 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.
Mar 15 '10 #10
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.

Expand|Select|Wrap|Line Numbers
  1. char * formatHours(int seconds) // argument ignored for test
  2. { // convert integer seconds timer to string: " HH:MM"
  3.    char zonk[15] ; // buffer with odd name to make sure it is unique
  4.  
  5.   sprintf(zonk, "%02d", 23) ;    // hrs format 2 places, leading zero
  6.   strLCD5XY(zonk, 0, 2) ; // print on screen -> displays "279" *****
  7.  
  8.   sprintf(zonk, "%02d", 59) ;  // mins format 2 places, leading zero
  9.   strLCD5XY(zonk, 44, 2) ;  // print on screen -> displays "315" *****
  10.  }
Still not working, see ***** above.
Mar 15 '10 #11
donbock
2,422 Expert 2GB
@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?
Mar 15 '10 #12
jkmyoung
2,057 Expert 2GB
It is odd that those values are exactly 256 greater than the value passed in.
Mar 15 '10 #13
@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.
Mar 16 '10 #14

Post your reply

Sign in to post your reply or Sign up for a free account.

Similar topics

reply views Thread by Bo Peng | last post: by
3 posts views Thread by Christopher Benson-Manica | last post: by
5 posts views Thread by Steve | last post: by
2 posts views Thread by Simon | last post: by
5 posts views Thread by Lloyd Sheen | last post: by
5 posts views Thread by Chris | last post: by
reply views Thread by zhoujie | last post: by
xarzu
2 posts views Thread by xarzu | last post: by
By using this site, you agree to our Privacy Policy and Terms of Use.