C-code; array of struct ? (Atmel Studio 6)

[David_AVD]

With the array of struct code, I'm seeing some weirdness.  Here's how it's defined:

Code: [Select]

struct Button
{
unsigned int top;
unsigned int left;
unsigned int height;
unsigned int width;
unsigned long BgColour;
unsigned long CaptionColour;
char * caption;
unsigned char GlyphIndex;
};

struct Button Buttons[32];
Here's the assignment of values to a typical button:

Code: [Select]

Buttons[12].top = BtnRow1Top;
Buttons[12].left = BtnCol4Left;
Buttons[12].height = BtnHeight;
Buttons[12].width = BtnWidth;
Buttons[12].BgColour = BtnColourLabel;
Buttons[12].CaptionColour = VGA_BLACK;
Buttons[12].caption = " ";
I iterate through them like so to draw the whole screen:

Code: [Select]

unsigned char i;
for (i=0;i<=31;i++)
Draw_Button(&Buttons[i]);
Here's the draw routine:

Code: [Select]

void Draw_Button(struct Button *Btn)
{
LCD_FillRoundRect(Btn->left,Btn->top,Btn->left+Btn->width,Btn->top+Btn->height,Btn->BgColour);
FgColour=clButtonOutlineNormal;
LCD_DrawRoundRect(Btn->left,Btn->top,Btn->left+Btn->width,Btn->top+Btn->height);
BgColour=Btn->BgColour;
FgColour=Btn->CaptionColour;
LCD_PRINT(Btn->caption,Btn->left+((BtnWidth-(strlen(Btn->caption)*cfont.x_size))/2),Btn->top+((BtnHeight-cfont.y_size)/2),0);
}
Here's where I update one of the buttons:

Code: [Select]

void UpdateButton12(void)
{
if(**boolean check was here**)
Buttons[12].caption = "*";
else
Buttons[12].caption = " ";
Draw_Button(&Buttons[12]);
}
Now, the weird thing is that it prints the asterisk correctly, but when it should print the space, it actually prints whatever text was printed to the previous button I updated!

So it's like the caption is pointing to the wrong record in the array, but only when I print "" or " ".  This has got to be something really silly, but I can't see what.   

[madires]

Code: [Select]

char * caption;

Buttons[12].caption = " ";


caption is definied as a pointer to a string, therefor you would have to allocate memory for the string itself and place the memory's address in caption. If caption should just store a single character remove the asterisk:

Code: [Select]

char caption;

Buttons[12].caption = " ";


[David_AVD]

The captions can be from zero to 10 characters long and can change in length during program execution.

Are you saying I need to explicitly allocate memory for them?

[c4757p]

The usual way C handles strings might not play nicely with AVR's memory model. You may want to declare them PROGMEM and use them that way.

[Kremmen]

The usual way C handles strings might not play nicely with AVR's memory model. You may want to declare them PROGMEM and use them that way.

Err... no reason why strings wouldn't work exactly as expected in AVR's Harvard memory model. PROGMEM strings would be relevant only for constant strings where storing them in code space would make sense. But it is not a general answer. For variable strings you definitely want to do it the normal way.
In the above the strings are constant but coding a component like a button to make such an assumption would be gross misfeature.

[c4757p]

I was under the impression that the strings were constant. You can still move around constant strings, you just can't rewrite them...

[madires]

The captions can be from zero to 10 characters long and can change in length during program execution.

Are you saying I need to explicitly allocate memory for them?

Yes, but they may also be stored in the flash or eeprom (but would be fixed). Or you could:

Code: [Select]

  char caption[11];

  strcpy(Buttons[12].caption, "bla");

Or if you like the more secure way:

Code: [Select]

  strncpy(Buttons[12].caption, "bla", 10);

and set the last char always to zero.

[madires]

I was under the impression that the strings were constant. You can still move around constant strings, you just can't rewrite them...

Just if you define them as constant. The concept of strings in C might be a little bit confusing because there's no dedicated type. A string in C is actually an array of characters (char string[]) with a trailing zero (last char in array). You may define a fixed sized array or use a pointer and allocate the memory for storing the string. The latter allows you to support dynamic string sizes by re-allocating memory if required. The fixed sized array stores up to the specified number of characters (-1 for the trailing zero). Both may be altered any time.

[Kremmen]

I was under the impression that the strings were constant. You can still move around constant strings, you just can't rewrite them...

Yes that seems to be the case here. Technically there is nothing preventing you to specify PROGMEM strings and of course you can move the relevant pointers freely. The fly in the ointment in Harvard architecture is that you must handle PROGMEM constants differently from variables stored in RAM. That prevents one from writing general purpose code to handle any string. Therefore i would write the components so that they use only RAM based variables, even if the original source is in the flash memory space.
If memory is an issue then of course it is possible to code explicitly for flash based strings but the cost is loss of generality.

[IanB]

Just if you define them as constant. The concept of strings in C might be a little bit confusing because there's no dedicated type. A string in C is actually an array of characters (char string[]) with a trailing zero (last char in array). You may define a fixed sized array or use a pointer and allocate the memory for storing the string. The latter allows you to support dynamic string sizes by re-allocating memory if required. The fixed sized array stores up to the specified number of characters (-1 for the trailing zero). Both may be altered any time.

In general, if you mention a constant string in C like "hello" the compiler will treat it as constant data (a read-only string of characters) and will allocate space for it in a special area of program memory. You can then take a pointer to this string at any time as long as you only read it and don't try to write to it. So the program as shown by David should work.

However, while it will work on a normal computer, there may be something different about the Atmel environment where it doesn't. It is hard to say without more research.

[madires]

In general, if you mention a constant string in C like "hello" the compiler will treat it as constant data (a read-only string of characters) and will allocate space for it in a special area of program memory. You can then take a pointer to this string at any time as long as you only read it and don't try to write to it.

That's right!

So the program as shown by David should work.

Really? Please see again:

Code: [Select]

  char * caption;     /* inside the Button structure */

  Buttons[12].caption = " ";

Based on your suggestion it would be:

Code: [Select]

  const char my_string[] = "bla bla";

  Buttons[12].caption = (char *)my_string;

The "(char *)" is required for a "clean" assignment, otherwise gcc would give you following warning: assignment discards 'const' qualifier from pointer target type [enabled by default]. ;-)

However, while it will work on a normal computer, there may be something different about the Atmel environment where it doesn't. It is hard to say without more research.

AFAIK it's the same.

[Kremmen]

No research needed - the answer is known . The 'const' keyword is an instruction for the compiler to treat the declaration as constant in the sense that its value is not supposed to change, but it will _not_ cause the storage to be allocated from flash memory.
If you wish to allocate the space in flash then you need to direct the compiler to do so using __attribute__((__progmem__)), usually abbreviated by #define PROGMEM in the header 'pgmspace.h'.

[madires]

No research needed - the answer is known . The 'const' keyword is an instruction for the compiler to treat the declaration as constant in the sense that its value is not supposed to change, but it will _not_ cause the storage to be allocated from flash memory.
If you wish to allocate the space in flash then you need to direct the compiler to do so using __attribute__((__progmem__)), usually abbreviated by #define PROGMEM in the header 'pgmspace.h'.

To be sure I checked it and a const char mystring[] = "bla bla" is stored in the program code. Increasing or decresing the number of characters is reflected in a matching change of the code size. The string is loaded from the program code into RAM (code automatically created by compiler). Using PROGMEM for storage specification tells the compiler to store the variable/data into the program code without adding code to read it from there into RAM. So you have to read it manually. The same applies to the EEPROM.

[Kremmen]

Well of course the initialized string is part of the code space, how else would its contents be available during runtime? But unless you use the PROGMEM directive, it will be copied to RAM by the initialization code generated by the linker. So as far as your code is concerned, a 'const' is in RAM during runtime. Just as you write. No conflict there, but the "problem" is that a PROGMEM constant needs different processing which is less than practical in a generic component.

[andersm]

But unless you use the PROGMEM directive, it will be copied to RAM by the initialization code generated by the linker.

With GCC 4.7 or above the recommended way is to use the __flash directive and let the compiler generate the correct code, rather than using PROGMEM and the special progmem access functions.

[westfw]

Quote

   

So the program as shown by David should work.

Really?

Yes, really.

Please see again:

Code: [Select]

  char * caption;     /* inside the Button structure */

  Buttons[12].caption = " ";


That should be fine, especially on an AVR.  (on my desktop, it puts the string in a read-only segment and throws a kernel protection failure if you try to write it, but it compiles just fine, and this segment is NOT trying to write the string...)

All these PROGMEM and __flash suggestions are swell, but they don't address why the code isn't working as currently written.  I didn't see anything obvious. :-(

Do you have enough other RAM data structures that you could be running short on RAM?  You've got 500+ bytes of just your datastructure, plus whatever the graphics library uses...  What does avr-size say about your .elf file?

I would be debugging this on a desktop before trying to get it running  in a microcontroller (especially if the microcontroller lacks a good debugging environment.)

[mrflibble]

All these PROGMEM and __flash suggestions are swell, but they don't address why the code isn't working as currently written.  I didn't see anything obvious. :-(

Indeed. I also thought David's code to be correct. Should it be related to something silly being done by Atmel Studio, the following might be worth a try:

Code: [Select]

void UpdateButton12(void)
{
static const char *caption_space = " ";
static const char *caption_asterix = "*";

if(**boolean check was here**)
Buttons[12].caption = caption_asterix;
else
Buttons[12].caption = caption_space;
Draw_Button(&Buttons[12]);
}

If that suddenly does work, then it's something fishy where AVR Studio thinks scope is something that happens to other compilers.

[madires]

That should be fine, especially on an AVR.  (on my desktop, it puts the string in a read-only segment and throws a kernel protection failure if you try to write it, but it compiles just fine, and this segment is NOT trying to write the string...)

gcc actually does that!? It puts the string into the program code, copies it to RAM and assignes the address to the pointer. Didn't noticed that feature until now. IMHO that's a very bad C programmimg style.

[westfw]

IMHO that's a very bad C programmimg style.

Which bit?  Modifying a string constant?  Yes, you won't get many arguments about that; it was mostly used as an illustration of how C allows you to shoot yourself in the foot.  (not as bad as ForTran letting you overwrite constant integers, though.)

Code: [Select]

strptr = "string constant";Is perfectly legal, very common, and does just the right thing, though.  What you CAN'T do (at runtime) is

Code: [Select]

stringarray = "stringconstant";

[David_AVD]

I'm only following part of what you guys are saying.  I understand that the initialised values are stored in flash and copied to RAM at start up.  This makes perfect sense to me.

Is there a way to "reserve" a maximum space for each caption?  A static array would do that surely, but how does that affect what I want to do?

Just to clarify; some of the captions can and will change during program execution.  Some are static button labels that will never change, while others will change (and be re-drawn) every few seconds.

Since the rest of it all seems to work, could it be the way the text drawing is handling the pointer to the captions?  I set the caption to "" (or " ") and the actual caption referenced is another one; the previous one drawn.  This makes me think that there's a buffer somewhere that's being accessed each time, but doesn't get updated (overwritten) when the new input is "" or " ".

[mrflibble]

Since the rest of it all seems to work, could it be the way the text drawing is handling the pointer to the captions?  I set the caption to "" (or " ") and the actual caption referenced is another one; the previous one drawn.  This makes me think that there's a buffer somewhere that's being accessed each time, but doesn't get updated (overwritten) when the new input is "" or " ".

Hence my suggestion to try the bit with the static const char *.. if using that it suddenly works then Atmel fucked up. If it still doesn't work as expected chances are you fucked up.

[ve7xen]

It sounds like you're overwriting the (or one of) constant string which is copied into memory. If you do for example:

char *foo = " ";
foo[0]=',';

AIUI every reference to " " will end up being to the same memory address. If you don't make them const and later modify that memory, anything pointing to it will be changed as well. It sounds like you're mixing 'drawing' the strings into memory and using literals, so it would be easy to make this kind of mistake. Also it's quite likely that doing so will overwrite other strings (or other constants). This is how you can easily shoot yourself with C .

In your case I would probably just allocate space statically in your data structure e.g.

struct Button {
   char caption[10];
}

Any only ever mutate the string, don't use 'caption =' ever.

[mrflibble]

It sounds like you're overwriting the (or one of) constant string which is copied into memory. If you do for example:

Good point. Do you get any compiler warnings to that effect? Anyways, I suggest using the static const thing and seeing 1) if it works and 2) if you then DO get compilers warnings related to overwriting as ve7xen suggested.

[David_AVD]

Just tried the caption constants and it's slightly different.  Now it flips between "*" and "[space]2" (2 being the errant string).

I'll have a look at the array method in a moment.  I take it I should use strcpy with that?

EDIT: No compiler warnings related to that.  There is one for how the font set is selected, but that doesn't seem to be related.

[mrflibble]

WT... and if you exchange the two lines with the static const char* declarations? As in swap around their relative addresses... Any change in behavior? If yes ... interesting.