C Code Problem (2)

[Digibin]

Hey. I'm working on a project involving a LCD screen and an ATTiny48, and decided to write a sort of splash screen to display on the LCD before moving into the main program code. Just for fun/practice. This follows from a previous post I made here:

https://www.eevblog.com/forum/microcontrollers/c-code-problem/

Before I started this afternoon the code worked fine and as expected, but after writing a few lines for my splash screen, it breaks. The splash displays as expected but once it moves onto the main program code it doesn't quite function properly. The .c file is attached, and the snippet that I've added and that appears to break it is this:

Code: [Select]

/* Boot splash screen */
lcd_init(LCD_DISP_ON); // Initialize LCD to 4-bit mode
lcd_clrscr(); // Clear display

const char bat[8] = "   Bat  "; // Splash screen text, top line
// const char box[8] = "   Box  "; // Splash screen text, bottom line
char buffer[8]; // Buffer string to manipulate bat and box
int8_t i = 3;
int8_t j = 4;

for (int8_t h=4; h>=0; h--)
{
memset(buffer, '\0', sizeof(buffer)); // Reset/erase buffer string
// strncpy(buffer, bat, 8); // Copy bat into buffer for manipulation

buffer[i] = '<'; // Set '<' in buffer string
for (int8_t k=i-1; k>=0; k--)
{
buffer[k] = ' '; // And set all preceding elements to ' '
}

buffer[j] = '>'; // Set '>' in buffer string
for (int8_t k=j+1; k<=7; k++)
{
buffer[k] = ' '; // And set all proceeding elements to ' '
}

lcd_gotoxy(0,0); // Go to first line
lcd_puts(buffer); // Put string to display
_delay_ms(1000);
lcd_clrscr(); // Clear display

i--; // Decrement to move to next stage of splash screen
j++; // Increment to move to next stage of splash screen
}

Basically this just displays '<' and '>' moving from the middle of the LCD to the sides with the text 'Bat' appearing in this middle as they move outwards. This works fine but once it moves onto the main program code somehow my variable 'enc_count' is being incremented over and over until it reaches the upper limit that I set for it. This should be increment/decrement when I turn a rotary encoder, but somehow the program is doing it by itself.

I've narrowed it down to the strncpy line. If I remove that line the program code works as normal (but of course the splash screen doesn't really work). Based on what I learned from the previous thread linked to above, is my use of strncpy perhaps causing stack overflow or something?

As with sprintf is it better not to use strcpy in my application (i.e. with limited memory in my ATTiny48)?

Any help or ideas would be appreciated.

Cheers.

[helius]

Code: [Select]

const char bat[8] = "   Bat  ";
This is incorrect, your string constant is length 9 and needs an array like bat[9] to hold it.
Strings in C are null-terminated, that means that the last element of the string is a character of value 0.

[Digibin]

Good point, corrected that, but the same thing happens!

[Maxlor]

Yup, lack of null termination is the problem here, and this line is probably where it goes wrong: lcd_puts(buffer);

lcd_puts doesn't know when the string will end, and will keep reading and reading until it finds a '\0' somewhere in memory.

You don't have to use null-terminated strings btw, it's perfectly fine to keep track of your string length some other way, such as compile time constants. You just can't use function expecting null-terminated strings if you go that route.

[grumpydoc]

Good point, corrected that, but the same thing happens!

Did you increase buffer to char[9]?

As Maxlor says passing a improperly terminated string to a routine which expects a nul terminated one is not going to go well.

[grumpydoc]

Another problem with the code is in the looping.

The loop controlled by "h" iterates 5 times with values 4,3,2,1 and 0

i starts out as 3 and you decrement each time round the loop, so on the last iteration it will be -1 - there is no buffer[-1] so the line

Code: [Select]

buffer[i] = '<';
Will scribble on random memory.

Similarly you write past the other end of the buffer array as well when j is 8 on the last interation

[Digibin]

Unfortunately that doesn't fix it, I hoped it would! Yes I've changed the buffer length to [9] and reduced the for loop iterations to h=3 (four iterations):

Code: [Select]

const char bat[9] = "   Bat  "; // Splash screen text, top line
// const char box[9] = "   Box  "; // Splash screen text, bottom line
char buffer[9]; // Buffer string to manipulate bat and box
int8_t i = 3;
int8_t j = 4;

for (int8_t h=3; h>=0; h--)
{
memset(buffer, '\0', sizeof(buffer)); // Reset/erase buffer string
strncpy(buffer, bat, 8); // Copy bat into buffer for manipulation

buffer[i] = '<'; // Set '<' in buffer string
for (int8_t k=i-1; k>=0; k--)
{
buffer[k] = ' '; // And set all preceding elements to ' '
}

buffer[j] = '>'; // Set '>' in buffer string
for (int8_t k=j+1; k<=7; k++)
{
buffer[k] = ' '; // And set all proceeding elements to ' '
}

lcd_gotoxy(0,0); // Go to first line
lcd_puts(buffer); // Put string to display
_delay_ms(1000);
lcd_clrscr(); // Clear display

i--; // Decrement to move to next stage of splash screen
j++; // Increment to move to next stage of splash screen
}

I still think that strcpy is doing something here. If I comment out that line it all works just fine. How much memory does strcpy occupy when called? I had a problem before when trying to use sprintf in that it consumed too much memory and caused problems.

[nctnico]

I'd replace
const char bat[9] = "   Bat  ";   

with:
const char bat[] = "   Bat  ";

In that case you never have to worry about setting the length. The same goes for buffer. Just make it longer than necessary to avoid buffer overruns.

[Jeroen3]

How much memory does strcpy occupy when called?

Code: [Select]

char *strcpy(char *s1, const char *s2)
{
    char *s = s1;
    while ((*s++ = *s2++) != 0)
;
    return (s1);
}Probably only the stackframe, since there are only so few variables they stay in the registers.

It will however eat all your memory if it never finds 0.

[helius]

Code: [Select]

for (int8_t k=i-1; k>=0; k--)


Do you know that int8_t is a signed type? A signed 8-bit integer has a range [-128..127]. An unsigned 8-bit integer has a range [0..255]. If it's unsigned, then k>=0 is always true.

Also, in terms of style, what do you need h for? It seems to me you could simplify your code to

for (int8_t i=3, j=4; i>=0; i--, j++)

[grumpydoc]

Do you know that int8_t is a signed type? A signed 8-bit integer has a range [-128..127]. An unsigned 8-bit integer has a range [0..255]. If it's unsigned, then k>=0 is always true.

There's no real advantage to using int8_t anyway.

Also, in terms of style, what do you need h for? It seems to me you could simplify your code to

for (int8_t i=3, j=4; i>=0; i--, j++)

The whole thing seems quite fragile with lots of small loops to copy various bits into the string - change the length of the message and it will all fall apart

Why not do it with one loop to build the string and one to "move" the '<>' and try to make everything work automatically should someone come along and change the message.

E.g.

Code: [Select]

static const char bat[] = "   Bat  "; // Splash screen text, top line
char buffer[sizeof(bat)]; // Buffer string to manipulate bat and box
int l = strlen(bat);
int i, h;

for (h=1; h <= l/2; h++) {
  for (i=0; i < l; i++)
  {
    if (i < l/2-h || i > l/2+h-1) {
      buffer[i] = ' ';
    } else if (i == l/2-h) {
      buffer[i] = '<';
    } else if (i == l/2+h-1) {
      buffer[i] = '>';
    } else {
      buffer[i] = bat[i];
    }
  }
  buffer[i] = '\0';
   
  lcd_gotoxy(0,0);       // Go to first line
  lcd_puts(buffer);       // Put string to display
  _delay_ms(1000);
  lcd_clrscr();       // Clear display
}

EDIT: Oh, BTW I padded out your modified copy into something that would run on a linux box and ran valgrind over it - seemed OK, so you might have a problem in some code you're not showing us.

[eneuro]

Probably, the quickest way will be... clear those few lines of code and start from the begining 

const char bat[] = "Bat";                              // Splash screen text, top line
char buffer[9];   
...
...
#define bat_len  sizeof(bat)        /// Should be NOT 3, but 4 bytes long, I guess
#define buffer_len sizeof(buffer)
...
bat[bat_len-1]= '\0';  //Not needed- bat should be already null(0) terminated when declared like above...
...
memset(buffer, '\0', buffer_len );                     // Reset/erase buffer string
///// strncpy(buffer, bat, ;     
memset(buffer, ' ', buffer_len-1 );  // fill with space
strncpy(&buffer[((buffer_len-1)-bat_len)/2 ], bat, buffer_len );        // Center Bat in the middle of the buffer, etc...
...
buffer[buffer_len-1]= '\0';   /// Ensure buffer is null (0) terminated before sending to LCD or printf, etc \...
...

Than add a few loops and let C preprocesor evaluate  buffer_len and bat_len for you (less bytes of output code probably )  and always ensure you have null (0) terminated strings

[grumpydoc]

Not only is this not needed

Code: [Select]

bat[bat_len-1]= '\0';  //Not needed- bat should be already null(0) terminated when declared like above...
but as you said

Code: [Select]

const char bat[] = "Bat";   
the first line is actually invalid (write to read-only string)

[nctnico]

Code: [Select]

const char bat[] = "Bat";   that line is actually invalid (write to read-only string)

Nope. You can initialise constants! Any decent toolchain (compiler et al) will put the initilisation value into program space (=flash on a microcontroller).

[IanB]

but as you said

Code: [Select]

const char bat[] = "Bat";   
that line is actually invalid (write to read-only string)

Doubling up on nctnico, that = is not an assignment it is an initialization. Assignment and initialization are different.

[helius]

You sometimes have to read the whole post 
it is assigned in a subsequent line

[grumpydoc]

Nope. You can initialise constants!

Yes, of course you can initialise constants - they'd be sod all use if not.

But you can't write to them - which is what you did later, it wasn't the initialisation I was worried about - as helios says read the wole post; the code writes to a "const" string.

[eneuro]

Any decent toolchain (compiler et al) will put the initilisation value into program space (=flash on a microcontroller).

Yep.
It was defined as global constant in C program and  compiled using avr-gcc

const char bat[]= "Bat";

It is  at the end of AVR ATTiny flash program in output binary:

$ cat test.bin |bin2hex | grep Bat

0001910: f51f 220f 331f 441f 551f 9695 8795 7795  ..".3.D.U.....w.
0001920: 6795 89f7 0097 7607 71f7 cf01 bd01 0895  g.....v.q.......
0001930: f894 ffcf e0ff aa42 6174 0000            .......Bat..

[paf]

Grumpy mode on:

Are you compiling with all the warnings turned on?   
That is, using the -Wall command line flag of (avr-)gcc? 
If you are not using the warnings or you are not looking at the warnings, the problem is not a programming problem.
You really like pain and suffering.

Grumpy mode off.

C is a very powerful language, and like all powerful tools, should be used with extreme care.
The compiler warnings are one of the easiest/fastest ways of getting feedback about our source code.

   

[eneuro]

C is a very powerful language, and like all powerful tools, should be used with extreme care.

Common, quite usefull AVR program it is only afew lines of code and in many cases main concern is output code size and its speed, so anyway if you want to ensure that MPu will do what you want you have to look into assembler listing and see if everything is fine 
Academic discussions about coding style doesn't matter-the only thig what matters is output MPU assembler code.

[donotdespisethesnake]

The AVR GCC behaviour is rather non-standard, because the AVR has a Harvard architecture. Constants are copied from Flash to RAM to allow data pointers to work correctly. However, writing to string literals is illegal in C, and a bad programing habit, although you can get away with it on some implementations.

If you try the same thing on most other CPUs, you will get incorrect results and possibly a bus fault. This exact same bug occurred recently on another forum where a user was porting from AVR to ARM and wondering why it didn't work.

[dannyf]

If you try the same thing on most other CPUs, you will get incorrect results and possibly a bus fault.

What would be a few examples of such CPUs?

[eneuro]

the code writes to a "const" string.

Any problem to define it like this if we want introduce better programming habbits?

static char bat[]= "Bat";

It is not a point and problem was probably in not null '\0' terminated strings there, so it doesn't matter how such string is declared-if we want change it, yep it is better show that it might change somewhere in the code, so forget about const and make it static ...

BTW: More important to use '\0' rather that 0x00 as null string terminator if we want code more portable

[grumpydoc]

What would be a few examples of such CPUs?

Anything with an MMU should be able to mark pages read-only and fault if written to if the MMU is being used.

RISC architectures tend not to like mis-aligned reads/writes, eg trying to read a 32bit word from an odd address and will generate a bus fault if you try - SPARC, MIPS, ARM < v6 all do this

I'm fairly sure the various x86 multimedia instruction sets want data to be aligned even though normal loads and stores on x86 don't.

ARM >= v6 will do unaligned reads in hardware, but as with all archictures which can it is less efficient to make use of this feature.

[Jeroen3]

Static has no effect on the generated code for global scope variables.
For global names it makes use of the object outside of the current module (c file) illegal.

However, if you use static in a block or function (that is between '{' and '}'), static gives the object a persistent memory allocation, while keeping it only accessible from the scope it was declared in. It is persistent, that means it is still the last value the next time you enter the block. You do not initialize static types inside a block.

Writing char bat[] = "bat"; is legal in any scope. If you have global strings that you'll be using outside main scope (from interrupt for example) you must make them volatile. That way the compiler always forces read-modify-write order for that object instead of assuming it's value for the sake of optimization.

declaration: char bat[16];
assignment: bat[1] = 'a';
initialization: char bat[] = "text"; char bat2[] = { 1, 2, 3, 4 };