String corruption, but only when using itoa()?

Go To Last Post
15 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi all,

 

Been several years since I've posted on here after a break from any AVR/programming work.

 

So, after much searching - I can't seem to find quite the same problem in the forums, despite many threads surrounding string corruption. I've been working on a large project for several weeks, and in the last few days have run into one problem in particular I can't seem to get around.

 

I'm using strings in my application, written out onto a SPI TFT screen - where I'm displaying fixed text, variable text, changing colours etc all of which is working just fine. These strings are actually strings as pointers.

 

My string is declared in a .c file as:

char *str;

and defined in a header file as:

extern char *str;

The string is then used as below, the functions display_goto_xy() and display_write_string() functions are my functions which deal with all the low level SPI routines to write data out to the display. Wherever I use the below, and whatever I place in "xxxxxxxxxxx" gets written out onto my display with no problems at all. Note I haven't written in any PROGMEM functionality yet, before I get bashed for RAM usage of fixed strings, it's on the todo list.

display_goto_xy(3, cursor_y);
str = "xxxxxxxxxxx";
display_write_string(str, BLACK);

However, problems occur when I need to write a variable out, in particular when I need to convert an integer to string and then put this onto the display. So below, I'm clearing the previously displayed text by writing a string of spaces out onto the display (which works just fine in the above code without using itoa). Then I'm using itoa to convert a variable into a string, and push that out onto the display.

display_goto_setpoint(x,y);
str = "       ";
display_write_string(str, BLACK);
display_goto_setpoint(x,y);
itoa(variable, str, 10);
display_write_string(str, BLACK);

However, the problem I'm getting, is the displayed string seems to still contain old data. For example, if I write a value of 12 to the display (which displays fine), then 1023 (which displays fine), then 0, what I see on the display is 0023, despite having already cleared the previously displayed data.

 

The display is clearing fine, if I pause before writing the converted integer data, the screen is indeed displayed - so the routines writing to the display are working just fine.

 

What am I possibly doing wrong? When I write directly into the string, I don't have to 'clear' the contents of the array first, when using itoa(), do I have to empty out the array first?

 

Note I'm using using dtstrf() in place of itoa() where I need to convert float data to strings, and I get exactly the same issue as above...

dtostrf(float_data, 5, 1, str); // convert degrees C float into integer

Many thanks for your help in advance, I've gotten nothing but good replies in the past here.

 

EDIT - I should add, this is on a ATMega1284p @ 16MHz (large RAM requirement application for ADC data buffers before writing to SD), custom board. Entire application written in C, using avr-gcc Toolchain inside AS7.

 

Thanks

This topic has a solution.
Last Edited: Sun. Feb 2, 2020 - 12:39 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Life is less confusing if you declare your buffer as an array instead of an initialised pointer.

char str[10];   //enough space for 9 letters + NUL

You can predict the maximum width needed for an int16_t or int32_t but f-p is not so easy.

If you exceed the array bounds of your buffer,  sh*t happens.

 

David.

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The problem is you're not providing any storage for where itoa wites to.

jtw_11 wrote:

My string is declared in a .c file as:

char *str;

This doesn't declare a string.

It declares (and defines) a pointer to a char.

It doesn't define anywhere for the storage of a string.

 

jtw_11 wrote:

str = "xxxxxxxxxxx";

the"xxxxxxxxxxxx" string literal creates a null terminated string, stored somewhere in memory, and returns a pointer to the start of this string.

So you now have the start of this string in str.

So far so good.

Although you could just as well write

display_write_string("xxxxxxxxxxx", BLACK);

 

jtw_11 wrote:

itoa(variable, str, 10);

This writes the result of itoa starting at wherever str is pointing.

But at this point, str is pointing to the last string literal that you assigned to str, and so will be trying to write to that location.

It is undefined to try and overwrite a string literal.

Imagine if your string literal was located somewhere read only (like in flash), what is supposed to happen then?

It can't work.

If the string literal is in RAM then it might actually work, assuming it doesn't overwrite something else important, but it is still wrong.

 

 

You need to provide some other storage for itoa to write to.

 

char buf[10]; //make sure it is big enough for whatever wil be put in

itoa(variable, buf, 10);

display_write_string(buf, BLACK);

 

You don't need to initialise or clear buf before itoa.

 

EDIT

bear in mind that if you have the same string literal dotted throughout the code

str = "    ";

/* stuff */

str = "    ";

/* stuff */

etc.

 

then the compiler can be smart and only create one copy of this string literal, so each use of it will refer to the same address.

So if your code modifes the contents, the next time that string literal is used it may no longer be containing what you expect.

I suspect that is what is happening in your case..

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are using-

str = "       ";

as if that is using the initialized data that was in that string. Not necessarily so, at least not after the first use.

 

That char string is in a data location (ram). Has an address. You are simply assigning that address to your pointer. Fine. You clear the lcd with this string. Fine. You now use itoa on that string. Fine. Send to lcd. Fine. Now you do the same thing again, but your assignment of str to the string of spaces 'reuses' that initialized data string address, and you think you are clearing the lcd with spaces, but all you are doing is writing your last itoa value that also used that string.

 

The compiler takes that "       " and puts in data as any other var, but also will reuse its address if you do another assignment to the same thing. Add another space to the second assignment and you will probably just get a reuse of the previous string in addition to another char (prepended to original string, so address is -1 of previous string).

 

 

#include <stdlib.h>

char* str;

int main(){

    str = "       "; //data address happens to be 0x100 in this example, and can see the initial data- 0x20 0x20 ... 0x00

    //breakpoints on the nop's
    asm("nop");
    itoa(1023, str, 10);
    asm("nop"); //str = 0x31 0x30 0x32 0x33 0x00 0x20 0x20 0x00
    itoa(0, str, 10);
    asm("nop"); //str = 0x30 0x00 0x32 0x33 0x00 0x20 0x20 0x00

    str = "       "; //now we think we are getting a string of spaces

    //now notice *str is NOT a string of spaces, but is simply pointing to the previous string (address)

    //in data/ram- which has already been modified by itoa, you are no longer getting your string of spaces

    asm("nop"); //str = 0x30 0x00 0x32 0x33 0x00 0x20 0x20 0x00

}

Last Edited: Sun. Dec 8, 2019 - 07:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'd point out, "you now use itoa on that string" is not actually fine. That's writing to a literal and is undefined behavior. On a lot of systems, that will fail. It might kill the program. It might create VERY strange behaviors later. Who knows?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 

 

the_real_seebs wrote:
On a lot of systems, that will fail. It might kill the program

 

Well I just had to test that on Windows; with this result:

 

 

I guess the_real_seebs is right; that would occur on a lot of systems.

 

 

 

Last Edited: Sun. Dec 8, 2019 - 09:38 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

>I'd point out, "you now use itoa on that string" is not actually fine.

 

It has a defined behavior in this case even though its undefined, and the results are fine in the limited sense that itoa is happy to use that chunk of ram in this case. Without a compiler moving a string to flash in an mcu (not likely unless you specify), it may work but there are too many variables in play to count on anything. The compiler may also combine strings when it can, so you can also end up with overlapping strings that you think are separate entities. In a pc, it will most likely get moved to the read-only section, and is the cause of problems when trying to write to its address.

 

The bottom line, since there are much better ways to do these things there is no need to mess around with trying to write to string literal addresses.

 

The first item to add on the lcd function list would be a function to just clear a line, clear display, clear whatever. No need to keep a string/array of spaces in any form to do this. The next step is to just create/use a local buffer (stack) for itoa/sprintf/whatever use. No need for a global char pointer, and no need to try writing to string literals.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
char *str;
str = "       ";
itoa(variable, str, 10);

I must be getting old. Why wouldn't you do this the way we've been doing it for the last 3 or 4 decades?...

char str[] = "       ";
itoa(variable, str, 10);

or maybe just:

char str[8];
itoa(variable, str, 10);

The only reason I can think of for creating a separate pointer to the storage area is in the scenario:

char * str;

str = (char *)malloc(some_length_not_known_at_compile time);

itoa(variable, str, 10);

free(str);

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes,  it might work with some compilers on some targets

char str[] = "       ";  //array is going into SRAM
char str[8];             //array is going into SRAM
char *str = "might go into read-only memory";

It seems foolish to rely on one particular AVR compiler putting anonymous strings into SRAM.

Especially when it is so straightforward to just declare str[8] as a local  (or a global)

 

It makes no difference to how you actually use itoa() or dtostrf()

But whatever you do,  you must be sure that the buffer array is big enough to contain all the letters with a terminating NUL.

 

David.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi gents,

 

Thanks very much for your help and apologies for the late reply, the day job and Christmas has kept me most busy.

 

I've since purchased K&R, refreshed on strings, read and re-read your replies and re-written my above code. Removing strings as pointers, and just using regular char arrays has a) simplified the code significantly and b) entirely resolved the issue I was having.

 

I had no idea that in effect I was potentially attempting to write to read-only memory.

 

Ref the comments about writing out strings of spaces to clear previously lines, this is only because I haven't written the function to clear out a set number of chars on a given line yet - I won't be clearing with spaces forever, but thanks for the feedback.

 

Thanks all

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

 

I must be getting old. Why wouldn't you do this the way we've been doing it for the last 3 or 4 decades?...

char str[] = "       ";
itoa(variable, str, 10);

or maybe just:

char str[8];
itoa(variable, str, 10);

 

Ah, Clawson - I know what I meant to ask on this one... Your example of string literal assignment won't compile for me, due to the empty index brackets. Am I missing something, or was that just a typo?

 

char str[] = "       ";

Above results in 'expected expression before ']' token' compiler error.

 

EDIT - Entirely just answered my own question reading back through K&R. The above line defines the array of chars, char str[], brackets being empty as str will just be sized appropriately for the string between quotes, only that you can't then write to str with another string literal. Any further write to str must be either zeroed, or by index.

Last Edited: Fri. Dec 27, 2019 - 01:35 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jtw_11 wrote:
EDIT - Entirely just answered my own question reading back through K&R. The above line defines the array of chars, char str[], brackets being empty as str will just be sized appropriately for the string between quotes

Yes, this is a specific use of a string literal where it is used to provide the initialiser list for the array

char str[] = "abcd";

is same as

char str[] = {'a', 'b', 'c', 'd', '\0'};

jtw_11 wrote:
only that you can't then write to str with another string literal.

Not directly, no, str is just an array of char, and you can't assign another value to the array as a whole.

You could use a function like strcpy

strcpy(str, "1234");

 

 

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jtw_11 wrote:
Ah, Clawson - I know what I meant to ask on this one... Your example of string literal assignment won't compile for me, due to the empty index brackets. Am I missing something, or was that just a typo?
I have no idea why that would not compile. It is quite valid in compliant C compilers to leave [] empty when an initial value is given. In effect you are saying "I can't be bothered to work out how many characters are in this, you count for me then set aside enough storage to hold it (and a 0x00 terminator in the case of a character string)". Show the error and identify your C compiler.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
jtw_11 wrote:
Ah, Clawson - I know what I meant to ask on this one... Your example of string literal assignment won't compile for me, due to the empty index brackets. Am I missing something, or was that just a typo?
I have no idea why that would not compile. It is quite valid in compliant C compilers to leave [] empty when an initial value is given. In effect you are saying "I can't be bothered to work out how many characters are in this, you count for me then set aside enough storage to hold it (and a 0x00 terminator in the case of a character string)". Show the error and identify your C compiler.
I infer that the initialization was not the problem. jtw_11 also attempted an assignment with similar syntax.

The latter is what failed to compile.

 

Initialization is not the same as assignment.

Iluvatar is the better part of Valar.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

skeeve wrote:

clawson wrote:
jtw_11 wrote:
Ah, Clawson - I know what I meant to ask on this one... Your example of string literal assignment won't compile for me, due to the empty index brackets. Am I missing something, or was that just a typo?
I have no idea why that would not compile. It is quite valid in compliant C compilers to leave [] empty when an initial value is given. In effect you are saying "I can't be bothered to work out how many characters are in this, you count for me then set aside enough storage to hold it (and a 0x00 terminator in the case of a character string)". Show the error and identify your C compiler.
I infer that the initialization was not the problem. jtw_11 also attempted an assignment with similar syntax.

The latter is what failed to compile.

 

Initialization is not the same as assignment.

 

Indeed, initialization compiles just fine - only my poor and illegal attempt at assignment that wouldn't compile.

 

Having reworked everything to work as I expected, I seem to however have re-introduced a similar problem somehow. I've written my own library to display large scaled integers (to avoid floating point math) as if they were really float values, which works very nicely with an enormous reduction in clock cycles to achieve the same result.

 

However, all my other displayed values mirror the first sampled and converted value, but only 1 second later (which is the refresh period of the display), so I'm sure I've simply made a far more fundamental mistake somewhere... If I'm truly stuck I'll post something here.

 

Thanks again all