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

[David_AVD]

I want to create an array of structures to hold button info for my LCD project.

Code: [Select]

typedef struct
{
unsigned int top;
unsigned int left;
unsigned int height;
unsigned int width;
char *caption;
}Button;

struct Button Buttons[4];
But that code gives me an error:

Code: [Select]

array type has incomplete element type
After Googling for some time, I'm none the wiser as to the correct syntax.   

Can anyone help please?

[ve7xen]

You have typedef'd an annonymous struct. The 'struct' keyword in your array definition isn't necessary; the compiler is looking for a named struct called Button, not a type called Button.

IOW the compiler is expecting:

Code: [Select]

struct Button {
...
};


[c4757p]

Damn! Beat me to it...

What you have done is taken "struct {blah}" (an anonymous structure) and named it Button. Not 'struct Button', just Button. Lose the typedef and just say "struct Button {blah}".

The confusion may be this: typedef and struct are completely separate. struct creates a structure type, which can be named or anonymous. typedef creates an alias - you can even say "typedef unsigned short myfavoritenumbertype;". They are often used together but they are unrelated. You can "define" a type entirely without typedef.

[amyk]

Thus, you can now declare an array of 4 Button's as follows:

Code: [Select]

Button Buttons[4];

[David_AVD]

The only combination i can get to compile is:

Code: [Select]

struct Button
{
unsigned int top;
unsigned int left;
unsigned int height;
unsigned int width;
char *caption;
};

struct Button Buttons[4];

I needed to add the struct keyword to my draw procedure:

Code: [Select]

void Draw_Button(struct Button Btn);
But it does compile.  I'll be able to test it at work tomorrow.

[David_AVD]

Another question if i may; on an AVR, where would this data reside?

Would if be pulled into RAM at startup?  If so, that means those values could be modified during code execution right?

[Psi]

It will be in ram. If you initialize it to a value then that value will be stored in flash and get loaded into ram as needed.

Use the PROGMEM directive if you want access to readonly data without taking up space in ram. It means the data will be read directly out of flash whenever its needed (a little slower but gives you a crazy large amount of space compared to ram ).   

The internal EEPROM is only ever read/written when you actually use the eeprom read/write functions or use the EEMEM directive.
Also note...
- You can tell avrdude to use the last eeprom address as a counter for how many chip erase cycles have been performed. (So some programmers may do this by default, best to avoid this address)
- The first eeprom location 0x00 has been known to get corrupted in some situations, its best not to use it.
- The EEMEM directive lets the compiler pick where to store things in eeprom, i don't recommend using it for that reason.

[rsjsouza]

Although there are many sites around that talk about C, I usually use the one below to solve the typical syntax and stdlib questions.

http://www.cplusplus.com/

In this particular case, searching for struct yields a nice page:

http://www.cplusplus.com/doc/tutorial/structures/

[David_AVD]

Thanks for that.  I'll see how much of the 8K RAM is chewed up once I get more code written.

I can't think of a reason (at the moment) that I'd need to modify any of the button data for this project as the captions and positions will be fixed.

[Psi]

Are you sure you have 8K of ram.  Some AVR's do have 8K (ATMega640 for example) but its a pretty high end AVR.

Perhaps you meant 8K of flash?  in which case it probably has 512-1024 bytes ram.

[David_AVD]

Although there are many sites around that talk about C, I usually use the one below to solve the typical syntax and stdlib question.

http://www.cplusplus.com/

Thanks for that link.  I'll check it out.

I have programmed in a HLL (Delphi) for PC applications over many years, but the "C way" of doing things is doing my head in sometimes.  I'm not sure if knowing another (different) language is making it easier of harder some days!  lol

[Psi]

I have programmed in a HLL (Delphi) for PC applications over many years, but the "C way" of doing things is doing my head in sometimes.  I'm not sure if knowing another (different) language is making it easier of harder some days!  lol

It's not you, C is just a bad/confusing language.

I also do lots of Delphi programming for windows apps.

[David_AVD]

Are you sure you have 8K of ram.  Some AVR's do have 8K (ATMega640 for example) but its a pretty high end AVR.

Perhaps you meant 8K of flash?  in which case it probably has 512-1024 bytes ram.

It's an ATxmega128D3-AU, which has 128K of FLASH, 2K of EE and 8K of RAM according to the data sheet.   Cheap as chips (!) too. 

[Psi]

ah, its an Xmega.  That would explain it

[David_AVD]

Yeah, learning C and not using a PIC (like I have for more than 100 other projects) has been a bit of a learning curve.  All while doing the general day to day fun of running a business - sheesh!   

[rsjsouza]

(...) but the "C way" of doing things is doing my head in sometimes.  (...)

Yes, C is far from trivial and small things such as syntax or prototypes of functions always trip me. In my opinion one of the aspects I appreciate the most in C is the freedom to work with memory (a.k.a. pointers) but, as with everything, with freedom comes great responsibility...

One issue that usually trips starters and saves you from a lot of grief in the long run is the bit width of integer data types (int, char, long, etc.), which vary greatly among processors and compiler implementations. For that, always keep in mind that several compilers have an include file called <stdint.h> that contains well-specified types and their bit width. For example: if your code requires an 8-bit unsigned integer variable, instead of declaring it as "unsigned char" you would include the <stdint.h> file on top of our source file (#include <stdint.h>) and declare it as "uint8_t" instead. More details are here and here.

If it helps, you can always use a program called "Lint" to double-check your code. One variant called Splint is freeware and a well-known commercial one (PC-Lint) covers C/C++ and other guidelines.

Yeah, learning C and not using a PIC (like I have for more than 100 other projects) has been a bit of a learning curve.

If anything, I think you are doing it right: starting with a device with beefed up RAM and Flash is always a good idea, since it is one less thing to worry about. 

[grumpydoc]

You have typedef'd an annonymous struct. The 'struct' keyword in your array definition isn't necessary; the compiler is looking for a named struct called Button, not a type called Button.

IOW the compiler is expecting:

Code: [Select]

struct Button {
...
};


While the typedef does not create a "struct Button" it does create a "Button" so what is wrong with

Code: [Select]

typedef struct
{
unsigned int top;
unsigned int left;
unsigned int height;
unsigned int width;
char *caption;
} Button;

Button Buttons[4];


[IanB]

While the typedef does not create a "struct Button" it does create a "Button" so what is wrong with ...

In standard C, probably nothing. It works on my C compiler.

[ToBeFrank]

I needed to add the struct keyword to my draw procedure:

Code: [Select]

void Draw_Button(struct Button Btn);
But it does compile.  I'll be able to test it at work tomorrow.

When you get everything straightened out, you probably want:

Code: [Select]

void Draw_Button(const Button *Btn);
Otherwise, you're passing a copy of the entire struct.

[c4757p]

To elaborate on that:

Unlike in C++, which has sneaky behind-the-scenes "references", in C, you pass into the function exactly what you specify. If the function parameter is struct Button Btn, then you are passing the entire struct in, which means copying its data into the function's stack, a big waste of RAM on a MCU.

If you define the parameter as const struct Button *Btn, then you are actually passing in a memory address to the original struct. Instead of passing it foo, pass it &foo, which means "address of foo". And within the function, to access its contents, use Btn->bar instead of Btn.bar, which means "element bar at the address Btn" instead of "element Bar inside Btn".

If you do not declare it const, you will even be able to modify the original struct inside the function without having to return it back out, which may be desirable.

[Kremmen]

Why not just declare/define it a class button{} and be done with it - no pondering what to pass and what not? Or is there a specific reason to avoid the benefits of C++ (seeing how Atmel Studio supports C++ without any additional entry fee).
By the way, something like a button is the archetypical candidate for a C++ class so specifically avoiding making it one is... hmm.

[c4757p]

On one hand, I agree, buttons and other GUI stuff are the perfect application for C++. On the other hand, it's good C practice.

[IanB]

I don't think I've seen it directly mentioned here, but in C you have to say "struct Button", whereas in C++ you can simply say "Button".

You need to know whether you are using a C compiler or a C++ compiler. There are quite a few differences between C and C++ that can trip you up if you think you are using C++ but you actually have C.

[ToBeFrank]

I don't think I've seen it directly mentioned here, but in C you have to say "struct Button", whereas in C++ you can simply say "Button".

That's what the typedef is for.

[jpb]

I always use Button twice :

typedef struct Button
{

}Button;


Button buttons[]

It has always worked with the compilers I've used from Borland to Microsoft and has the advantage I don't have to remember which name is for the struct and which is for the typedef.

[c4757p]

You need to know whether you are using a C compiler or a C++ compiler. There are quite a few differences between C and C++ that can trip you up if you think you are using C++ but you actually have C.

Most of the big compilers like GCC will switch modes by file extension. Not sure about the dedicated MCU compilers, though.

[IanB]

Most of the big compilers like GCC will switch modes by file extension. Not sure about the dedicated MCU compilers, though.

Indeed. But in light of David's comment below, it seems highly likely his compiler is expecting C and not C++:

The only combination i can get to compile is:

Code: [Select]

struct Button
{
unsigned int top;
unsigned int left;
unsigned int height;
unsigned int width;
char *caption;
};

struct Button Buttons[4];

I needed to add the struct keyword to my draw procedure:

Code: [Select]

void Draw_Button(struct Button Btn);
But it does compile.  I'll be able to test it at work tomorrow.

[David_AVD]

Atmel Studio does seem to support both C and C++ files.  When you add a new file to the project (via wizard) it does give options for each.  I chose C each time.

As I mentioned earlier (perhaps in the other thread), I will stick with C for now as it will help me when I do more coding with PICs and XC8 or XC32.

[David_AVD]

When you get everything straightened out, you probably want:

Code: [Select]

void Draw_Button(const Button *Btn);
Otherwise, you're passing a copy of the entire struct.

Good call.  I don't have a handle on the syntax for calling the function and accessing the structure inside yet.

[IanB]

Atmel Studio does seem to support both C and C++ files.  When you add a new file to the project (via wizard) it does give options for each.  I chose C each time.

As I mentioned earlier (perhaps in the other thread), I will stick with C for now as it will help me when I do more coding with PICs and XC8 or XC32.

Right you are then. But in doing that, you may (will) find many "nice" features of C++ are absent, such as declaring variables in the middle of a block, using structs without the "struct" keyword, using the "const" keyword, passing variables by reference, and so on. Make sure your reference books and internet searches are specifically for "C" and not "C++".

[David_AVD]

OK, got that figured out now.   

Code: [Select]

Draw_Button(&Buttons[0]);
And inside the routine:

Code: [Select]

LCD_PRINT(Btn->caption,Btn->left+3,Btn->top+3,0);
Now I just need to work out why the assignments to caption are not working correctly.  The last assignment seems to be affecting all structures instances.

[IanB]

Now I just need to work out why the assignments to caption are not working correctly.  The last assignment seems to be affecting all structures instances.

Be careful with char *caption;

That's a pointer to a string of characters stored outside your structure.

However, if you write:

Code: [Select]

Buttons[0].caption = "First button";
That should only set the caption on Buttons[0] and should not affect any other buttons.

[David_AVD]

Now I just need to work out why the assignments to caption are not working correctly.  The last assignment seems to be affecting all structures instances.

Yeah, just saw what I did.  For whatever (brain fart) reason, I was using strcpy() instead of a simple = for the caption.  Thanks.

[westfw]

Our code style guide would have liked:

Code: [Select]

typedef struct button { ...
} button_t;


If you initialize it to a value then that value will be stored in flash and get loaded into ram as needed.

For avr-gcc it will get loaded into RAM during program startup if it's global (as is the case here), on the assumption that since it's in your program, it WILL be needed.

[ToBeFrank]

Atmel Studio does seem to support both C and C++ files.  When you add a new file to the project (via wizard) it does give options for each.  I chose C each time.

As I mentioned earlier (perhaps in the other thread), I will stick with C for now as it will help me when I do more coding with PICs and XC8 or XC32.

Right you are then. But in doing that, you may (will) find many "nice" features of C++ are absent, such as declaring variables in the middle of a block, using structs without the "struct" keyword, using the "const" keyword

None of these are absent in standard C. First one is in C99 (on most compilers it's --std=c99), the second is done with a typedef, and the third, C of course has the const keyword. Although on the last one I suspect you meant C doesn't do const functions.

[c4757p]

Our code style guide would have liked:

Code: [Select]

typedef struct button { ...
} button_t;


Yep, very common.

None of these are absent in standard C. First one is in C99 (on most compilers it's --std=c99), the second is done with a typedef, and the third, C of course has the const keyword. Although on the last one I suspect you meant C doesn't do const functions.

Yeah, C hasn't been that stagnant over the past few decades! It's been a really long time since I've had a C compiler yell at me for mid-block declarations. I'd argue that "using structs without the 'struct' keyword" is useless obfuscation, but still, just throw in a typedef! And what about the 'const' keyword? C++ has one additional use of it, but all that does is declare the class pointer that's passed in implicitly as const. Everything else originated in C.

[David_AVD]

While you guys have been talking about C vs C++ I've now got a screen full of (30) buttons, all nicely laid out.   

I created some constants and calculations for the H & V spacing between buttons, so I can change the whole grid with only a couple of keystrokes.   

[IanB]

None of these are absent in standard C. First one is in C99 (on most compilers it's --std=c99), the second is done with a typedef, and the third, C of course has the const keyword. Although on the last one I suspect you meant C doesn't do const functions.

Fair enough. I guess the last time I used C was before C99 become commonly supported by compilers, and I must have had a brain fart about the const keyword. It's been so long since I was last constrained to using C that my memory has got fuzzy.

[c4757p]

IMHO it's long enough not to call it "constrained" any more. With the right libraries and coding techniques it's quite a decent language.

[David_AVD]

One (seemingly simple) task I am having trouble with is testing a string (actually the caption in the struct mentioned earlier) to see if it contains a decimal point.

No amount of fluffing about with the strchr function seems to give me a suitable result.  What I basically want to do is:

Code: [Select]

If(CharacterInString(Btn->Caption),'.')
  DoThis;
else
  DoThat;

[ve7xen]

Code: [Select]

if (strchr(Btn->caption, '.'))
  do_something_when_the_string_contains_a_period()
else
  do_something_else()

Don't forget to include string.h. Should work fine assuming Btn is type 'Button *' and caption is type 'char *'. If it's not the problem is likely elsewhere and we'll need to see more of your code to help...

Remember that C is C, it's often helpful to run bits of your code that don't depend on hardware on a PC where you have access to easier debugging and less involved testing, especially when you run into problems. I usually try to separate my hardware interface functions from logic functions, then you can usually just write a simple test shim as main.c and copy the rest of the code directly from your embedded tree into a test tree that will compile on your PC.

[madires]

Code: [Select]

If(CharacterInString(Btn->Caption),'.')
  DoThis;
else
  DoThat;

The code needs a little fix:

Code: [Select]

If(CharacterInString(Btn->Caption, '.'))
  DoThis;
else
  DoThat;

And CharacterInString() could be something like:

Code: [Select]

uint8_t CharacterInString(char *String, char Char)
{
  uint8_t            Flag = 0;

  if (String == NULL) return Flag;       /* sanity check */

  while (String[0] != 0)
  {
    if (String[0] == Char)
    {
      Flag = 1;
      break;
    }

    String++;
  }

  return Flag;
}


[IanB]

And CharacterInString() could be something like:

For illustration, maybe; but in production code why not just use strchr?

[c4757p]

That must be the most complicated CharacterInString() I have ever seen. Are you by any chance a Java programmer?

Code: [Select]

char CharacterInString (const char *s, char c)
{
    for (s; *s; ++s)
        if (*s == c) return c;
    return 0;
}

Of course, one small change makes strchr():

Code: [Select]

char const *strchr (char const *s, char c)
{
    for (s; *s; ++s)
        if (*s == c) return s;
    return NULL;
}

So you may as well use that.

I might as well take the chance to opine a bit on the "sanity check", because sometimes they are a good thing. I don't think it's useful in this case, though. If the code calling this function passes it a NULL string by mistake, there's something seriously wrong with it. You want it to crash as close to the bug as possible. If the function goes ahead and returns a legal response, the parent function could go on its merry way for god knows how long before the error is discovered. Also, for MCU applications I'd rather be extra careful about things like that and save the space in my code memory rather than compile in branches that will never be followed.

For desktop applications, a better way would be to include assert.h and then use assert(String != NULL). You can easily use the preprocessor to just drop all the assert() calls from the code if you're not compiling in debug mode, and it'll give you a nice line-number error message. If your device has any sort of debug interface, I'd recommend setting this up to receive assert() failures.

[madires]

That must be the most complicated CharacterInString() I have ever seen. Are you by any chance a Java programmer?

It's overdone on purpose. If I remove the variable and the pointer check, and replace the "break" with a "return" both are the same.

I might as well take the chance to opine a bit on the "sanity check", because sometimes they are a good thing. I don't think it's useful in this case, though. If the code calling this function passes it a NULL string by mistake, there's something seriously wrong with it. You want it to crash as close to the bug as possible. If the function goes ahead and returns a legal response, the parent function could go on its merry way for god knows how long before the error is discovered. Also, for MCU applications I'd rather be extra careful about things like that and save the space in my code memory rather than compile in branches that will never be followed.

Basically I agree, but there are some situations where you want the program to keep running or enter a safe mode. There might be some errors nobody noticed yet and testing didn't show any problems. Some lucky customer triggers the error by coincidence and a motor/heater/whatever keeps running. If you need to know about such errors set a global error variable or return a "2" in case of the CharacterInString() function and add a proper error handling. We add MOV/TVS/clamping diodes as input protection, shouldn't we add some software protection too if feasible and if risks are being imminent? MCUs got errors too and bits might be inversed by cosmic particles or high EMI.

For desktop applications, a better way would be to include assert.h and then use assert(String != NULL). You can easily use the preprocessor to just drop all the assert() calls from the code if you're not compiling in debug mode, and it'll give you a nice line-number error message. If your device has any sort of debug interface, I'd recommend setting this up to receive assert() failures.

If you want the program being terminated, yes.

PS: I'm not a Java programmer.

[c4757p]

We add MOV/TVS/clamping diodes as input protection, shouldn't we add some software protection too if feasible and if risks are being imminent?

Well that is a good point.

[nctnico]

That must be the most complicated CharacterInString() I have ever seen. Are you by any chance a Java programmer?

I agree with Madires: his code is pretty solid altough I might also test for the maximum length of the string. Adding tests like these prevent an error from spreading like an oil stain and cause a reset/failure/exception in an unrelated piece of code. Error checks help to keep errors inside a 'compartment' so they are easier to track down.

[grumpydoc]

That must be the most complicated CharacterInString() I have ever seen.

In that case you have never examined the GNU C library strchr source code.

The naïve implementations ignore that fact that, on most modern processors, it is more efficient to read memory in 32-bit (or 64-bit, or 128-bit) chunks rather than 8 bits at a time.

The point being that one should normally use the library routines supplied unless detailed analysis shows them to be sub-optimal - they are usually written by better programmers than you (or me, for that matter).

[c4757p]

The glibc version is quite clever. I like the documentation too, it's well written for its complexity.

Error checks help to keep errors inside a 'compartment' so they are easier to track down.

Only if you actually throw some sort of error condition though. If you just return 0 as if the function was successful the error will spread just the same.

[westfw]

We add MOV/TVS/clamping diodes as input protection

Yes, but not at the inputs to every gate in an FPGA...
Figuring out where to put error checking in SW should be a more difficult decision than "just put them everywhere."

No amount of fluffing about with the strchr function seems to give me a suitable result.

We need a more detailed explanation of what seemed to be wrong with the original attempts...

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

[David_AVD]

Just changed the code to use arrays and get this error now!   

Code: [Select]

Error 1 unable to find a register to spill in class 'POINTER_REGS'

[mrflibble]

Waitaminute. You said you get no warnings. But if you used the static const char * , you should at the very least get a typecast warning since your struct says caption is a char *.

Maybe use -Wall compiler option somewhere?

And just noticed your update. I have no idea what that one means. Other than "ran out of registers on this here atmel so screw you hippie".

[David_AVD]

This is getting weirder and weirder.  The "spill" error can be triggered by assigning a value to my structure properties.  But... it also depends on the exact value!  What the hell is going on the AS6 ?

[ve7xen]

I'm getting lost as to what the actual problem is and what you're doing. Maybe post your entire code as it stands and we can see where there might be problems?

I would recommend, if you need to generate strings at all, anywhere in your code, that you never use string literals outside of strcpy and the like. It is extremely easy to accidentally mix that up and then you run around corrupting memory and causing weird failures.

It also looks like you might be running out of memory or having other resource allocation problems. Are you using a lot of stack space?

[IanB]

This is getting weirder and weirder.

When strange weirdness happens, it is often a sign that you have badly constructed code. C is designed to let experts write code to do exactly what they want with maximum efficiency. The corollary is that C is designed to let newcomers shoot themselves in the foot with the greatest of ease.

The direction of this thread suggests that maybe you are doing something horribly wrong? As others have said, maybe it is time to post your whole program and not just snippets of it.

[David_AVD]

It also looks like you might be running out of memory or having other resource allocation problems. Are you using a lot of stack space?

Where would I look to check this?

[David_AVD]

OK, I've rolled the code back to where it compiled this morning.  Maybe we should look at those warnings (to do with the font selection) before going any further.

The warning:

Code: [Select]

expected 'char *' but argument is of type 'const char *'
The function referred to:

Code: [Select]

static void LCD_SetFont(char *font)
{
cfont.font=font;
cfont.x_size=fontbyte(0);
cfont.y_size=fontbyte(1);
cfont.offset=fontbyte(2);
cfont.numchars=fontbyte(3);
}
The definition for the structure above:

Code: [Select]

typedef struct
{
char* font;
uint8_t x_size;
uint8_t y_size;
uint8_t offset;
uint8_t numchars;
}current_font;

current_font cfont;
Part of the font file:

Code: [Select]

const char SmallFont[1144] PROGMEM={         
0x08,0x0C,0x20,0x5F,
0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, // <Space>
0x00,0x00,0x20,0x20,0x20,0x20,0x20,0x20,0x00,0x20,0x00,0x00, // !
...
0x40,0xA4,0x18,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, // ~
}; 


The (4) calls that are causing the warnings:

Code: [Select]

void LCD_SelectFont(unsigned char f)
{
switch(f)
{
case 0:
LCD_SetFont(SmallFont); // causes a warning: expected 'char *' but argument is of type 'const char *'
break;
case 1:
LCD_SetFont(BigFont); // causes a warning: expected 'char *' but argument is of type 'const char *'
break;
case 2:
LCD_SetFont(SevenSegNumFont); // causes a warning: expected 'char *' but argument is of type 'const char *'
break;
default:
LCD_SetFont(BigFont); // causes a warning: expected 'char *' but argument is of type 'const char *'
}
}

[IanB]

Well that one is easy. Fix what the compiler is warning about:

Code: [Select]

static void LCD_SetFont(const char *font)

Code: [Select]

typedef struct
{
const char *font;

[HackedFridgeMagnet]

I appreciate this thread and the other C threads as I am just getting back up to speed on C.

Where would I look to check this?

Stack pointer register and know what is allocated to the stack, and whether it is the main stack of just a thread's stack.

Can you attach a debugger?
I would probably approach it by working out if it is the ' ' char or the '*' char or if it is the position of the assignments in the compiled code.
Look at the values in the debugger and what is getting assigned to what.
Check the dissassembly if it makes any sense to see what the compiler is doing.
Check the memory to see if the values are being assigned where you expect.

Warnings are good to watch, if you can reduce them to under 100. I can't I am using Yagarto.

Can you change LCD_SetFont to (const char* font)?  Beaten by IanB

[David_AVD]

I did that after my last post, but now I have just one (different) warning:

Code: [Select]

assignment discards 'const' qualifier from pointer target type [enabled by default]
The warning refers to

Code: [Select]

static void LCD_SetFont(const char *font)
{
cfont.font=font; // causes warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
cfont.x_size=fontbyte(0);
cfont.y_size=fontbyte(1);
cfont.offset=fontbyte(2);
cfont.numchars=fontbyte(3);
}


[David_AVD]

As for the other issue (wrong caption text), it happens to other buttons too.   Only when the caption is set to "" or "[space]" though.

[IanB]

I did that after my last post, but now I have just one (different) warning:

Code: [Select]

assignment discards 'const' qualifier from pointer target type [enabled by default]
The warning refers to

Code: [Select]

static void LCD_SetFont(const char *font)
{
cfont.font=font; // causes warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
cfont.x_size=fontbyte(0);
cfont.y_size=fontbyte(1);
cfont.offset=fontbyte(2);
cfont.numchars=fontbyte(3);
}


You did this, right?

Code: [Select]

typedef struct
{
const char *font;
uint8_t x_size;
uint8_t y_size;
uint8_t offset;
uint8_t numchars;
}current_font;

[David_AVD]

No, I missed adding the other const.  Thanks for the reality check.  That warning has now gone.

[David_AVD]

OK, back to these captions.  As part of the boot-up, I assign values to the structures like so:

Code: [Select]

Buttons[0].top = BtnRow1Top;
Buttons[0].left = BtnCol1Left;
Buttons[0].height = BtnHeight;
Buttons[0].width = BtnWidth;
Buttons[0].BgColour = BtnColourLabel;
Buttons[0].CaptionColour = VGA_BLACK;
Buttons[0].caption = "";
Buttons[0].GlyphIndex = 0;
This is done for about 30 buttons.  I have the definitions above in other files:

Code: [Select]

#define BtnCol1Left 32
#define BtnWidth 82
#define BtnHSpace 13
#define BtnVSpace 15
...
#define VGA_BLACK 0x000000
#define VGA_WHITE 0xFFFFFF
#define VGA_RED 0xFF0000
...

I haven't got around to doing this more elegantly yet (with a function that takes parameters).

Is this a bad way of doing things in embedded C ?

[c4757p]

You can do a full struct assignment just like they're regular data, which the compiler might optimize a bit better. (Though GCC with full optimizations might be smart enough not to need to do this. I think it looks a bit nicer, though.)

Sorry, I'm tired as all hell and can't be bothered to go look up all your type names, you'll have to adapt this:

Code: [Select]

Buttons[0] = (struct foo) {one, two, three, four};

Or you can initialize them all like this:

Code: [Select]

const struct foo foo_initializer = {one, two, three, four};
size_t i;
for (i = 0; i < ABOUT30; ++i)
    Buttons[i] = foo_initializer;

Or you could of course do:

Code: [Select]

#define FOO_INITIALIZER {one, two, three, four};
Buttons[ABOUT30] = {FOO_INITIALIZER, FOO_INITIALIZER, .........on and on....., FOO_INITIALIZER};

If you're using C99 features (GCC supports them; compile with c99 command instead of gcc, but your code will be much less portable to older compilers):

Code: [Select]

Buttons[ABOUT30] = {[0 ... ABOUT29] = {one, two, three, four}};

The last two will be the most efficient. The same feature can abbreviate struct literals:

Code: [Select]

Buttons[ABOUT30] = {[0 ... ABOUT29] = {.height = BtnHeight, .width=BtnWidth}};

and all other fields are left as zero.

As an aside..... C99... as in 1999... 14 years ago... why is this still "new" and "non-portable"?? Come on, compiler writers, get your shit together!

[IanB]

Code: [Select]

Buttons[0] = (struct foo) {one, two, three, four};


I'd be surprised if this worked? Assignment is not the same as initialization.

[c4757p]

You're correct. I tried to optimize what he had one step at a time, and that one just replaced assigning the fields one at a time with assigning them in bulk. Obviously it's more efficient to set them all in an initializer, if they are supposed to be default values like he said.

It will, of course, work, even if it won't work well.

(I don't trust my coding ability when I'm half falling asleep, so I did test all these code snippets before posting them )

[David_AVD]

Since most of the button properties don't change during program execution, is there a better way?

At present, the only ones that do need to change are the caption and possibly it's colour.

[c4757p]

Hmm.... it's a bit tricky to optimize all that out. You could make some settings global instead of per-button:

Code: [Select]

#define BUTTONWIDTH 20

One way you could perhaps do it is with macros:

Code: [Select]

#define buttonWidth(btn) (20)
#define buttonCaption(btn) ((btn).caption)
...

and then omit the fields you don't need. Then if you need to make a field mutable, you can add it to the struct and change the macro to access that. Bit ugly, though.

You can define individual struct fields const. I'm not sure if that extends to __flash and PROGMEM, and if the compiler would bother yanking them from the struct, but it might be worth investigating. Then they're still per-button, but don't reside in the more dear RAM. (I can say with certainly that PIC's xc8 compiler, at least the free version, doesn't bother, and I would suspect most others do not as well.)

If you have plenty of memory I'd keep it, and just keep it at the top of a list of things to cut back if you run out.

[David_AVD]

Looks like I have plenty of room yet:

Code: [Select]

Program Memory Usage : 27892 bytes   20.0 % Full
Data Memory Usage : 1053 bytes   12.9 % Full


[c4757p]

Pfffft... you could practically install Windows on that!

"Premature optimization is the root of all evil" - Knuth

That space exists for the entire purpose of being used by your program. There's no reason to muck about trying to conserve it when you've hardly touched it in the first place!

[IanB]

Since most of the button properties don't change during program execution, is there a better way?

You could do things very simply using one large initializer, like this:

Code: [Select]

struct Button Buttons[32] =
    { BtnRow1Top, BtnCol1Left, BtnHeight, BtnWidth, BtnColourLabel, VGA_BLACK, "Caption 1",
      BtnRow1Top, BtnCol2Left, BtnHeight, BtnWidth, BtnColourLabel, VGA_BLACK, "Caption 2",
      BtnRow1Top, BtnCol3Left, BtnHeight, BtnWidth, BtnColourLabel, VGA_BLACK, "Caption 3",
...
      BtnRow4Top, BtnCol8Left, BtnHeight, BtnWidth, BtnColourLabel, VGA_BLACK, "Caption 32" };

[David_AVD]

That space exists for the entire purpose of being used by your program. There's no reason to muck about trying to conserve it when you've hardly touched it in the first place!

Hence the confusion about that weird error I got when trying to change the caption to an array.  I can find a few references to it on the 'net, but nothing that really explains why I'm getting it.   

[David_AVD]

Could one issue be that the initialisation is convoluted in some cases?

Code: [Select]

Buttons[28].left = BtnCol8Left;
The above would expand out to:

Code: [Select]

Buttons[28].left = 32 + (7 * (82 + 13));
Can I (should I) change those #define to constants with a calculated (at compile time) value?

[IanB]

It will, of course, work, even if it won't work well.

(I don't trust my coding ability when I'm half falling asleep, so I did test all these code snippets before posting them )

I don't find it to work. For instance:

Code: [Select]

#include <stdio.h>

struct Thing {
    int n;
    double v;
    char *label;
};

int main()
{
    struct Thing foo;

    foo = (struct Thing){2, 3.14, "hello"};
   
    return 0;
}


Code: [Select]

1>------ Build started: Project: CStructAssignment, Configuration: Debug Win32 ------
1>  main.c
1>c:\users\ian\documents\visual studio 2010\projects\cstructassignment\main.c(13): error C2059: syntax error : '{'
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========
On the other hand, this does work:

Code: [Select]

#include <stdio.h>

struct Thing {
    int n;
    double v;
    char *label;
};

int main()
{
    struct Thing foo = {2, 3.14, "hello"};
   
    return 0;
}


[IanB]

Could one issue be that the initialisation is convoluted in some cases?

Code: [Select]

Buttons[28].left = BtnCol8Left;
The above would expand out to:

Code: [Select]

Buttons[28].left = 32 + (7 * (82 + 13));
Can I (should I) change those #define to constants with a calculated (at compile time) value?

That's easy to do. First, change your formula to one with variables in it. For example, it might be

Code: [Select]

(row-1)*80+(col-1)*12+16
That being the case, define a macro:

Code: [Select]

#define BTNCOLUMN(row, col)  (row-1)*80+(col-1)*12+16
Now use the macro in your initializer:

Code: [Select]

Buttons[28].left = BTNCOLUMN(3,4)

[ve7xen]

Could one issue be that the initialisation is convoluted in some cases?

Code: [Select]

Buttons[28].left = BtnCol8Left;
The above would expand out to:

Code: [Select]

Buttons[28].left = 32 + (7 * (82 + 13));
Can I (should I) change those #define to constants with a calculated (at compile time) value?

This is what macros are for in the first place. The preprocessor will expand it, the compiler will know it's all literals and turn it into a simple assignment. Keep things clear, there's usually no reason to 'simplify' expressions, the compiler can handle that just fine.

If you assign caption = "" at startup, how do the buttons gain a caption later?

[David_AVD]

That's easy to do. First, change your formula to one with variables in it. For example, it might be

Code: [Select]

(row-1)*80+(col-1)*12+16
That being the case, define a macro:

Code: [Select]

#define BTNCOLUMN(row, col)  (row-1)*80+(col-1)*12+16
Now use the macro in your initializer:

Code: [Select]

Buttons[28].left = BTNCOLUMN(3,4)

Wouldn't that assume that all rows and columns use the same H & V spacing?  In my case there is one row spaced further up the screen than the rest.

[David_AVD]

If you assign caption = "" at startup, how do the buttons gain a caption later?

The problem doesn't appear at startup (first draw), only later when I redraw a button that (then) has a caption of "" or " ".

EDIT: I did try to not draw the text if the caption was "", but didn't figure out the syntax.

[David_AVD]

Just for kicks, I set the button draw routine to print the caption pointer address instead of the caption itself.  Each address seems to be (decimal) 19 higher than the one before.

[ve7xen]

If you assign caption = "" at startup, how do the buttons gain a caption later?

The problem doesn't appear at startup (first draw), only later when I redraw a button that (then) has a caption of "" or " ".

EDIT: I did try to not draw the text if the caption was "", but didn't figure out the syntax.

I meant "show me the code you use to update the caption later" .

[Kremmen]

I was going to ask you to post the entire source unless it is somehow confidential.
The usual suspects are overwritten pointers somewhere; not easily located by just some code snippets.

[HackedFridgeMagnet]

Did you have a debugger? that would help.

[David_AVD]

I meant "show me the code you use to update the caption later" .

The code that updates the caption later is the same procedure that draws them the first time.

Code: [Select]

Buttons[0].caption = "";
DrawButtons(&Buttons[0]);


I was going to ask you to post the entire source unless it is somehow confidential.

It is a commercial product in development, so I don't want to publicly release the code for it.  I realise that may seem rude when you guys are helping me.   

Did you have a debugger? that would help.

Ummm... I don't know...  The programmer is an AVRISPII.   

[Kremmen]

I was going to ask you to post the entire source unless it is somehow confidential.

It is a commercial product in development, so I don't want to publicly release the code for it.  I realise that may seem rude when you guys are helping me.   

Did you have a debugger? that would help.

Ummm... I don't know...  The programmer is an AVRISPII.   

Even though buttons have been re-invented any number of times, i understand if you don't want to disclose a commercial effort.

Re AVRISP - it is just a programmer, i.e. you don't have a debugger. Since this is a commercial effort you should make it a priority to get one and learn how to use it.
Any number of bugs can easily stay hidden in released code unless you have a proper test plan, and execute the plan as well. An integral part of all low level testing is debugging. Just as an example, i always walk through every algorithm i write, in the Studio simulator. First with inputs that make sense, to verify that the functionality is there (testing for functionality) then with inputs that don't make sense, to test for robustness and recovery from errors elsewhere (testing against bugs).

In by book anyone who develops commercial software should understand the principle of the V model of quality assurance https://en.wikipedia.org/wiki/V-Modelwhether they actually use one of its many variants or not. While a relatively waterfall-ish model that seems ill suited to modern agile ways of working such as scrum, it is relatively easily integrated into cyclical development as well. In practice you have to go through the phases anyway in order to have a verified end result ready for deployment.
See also http://istqbexamcertification.com/what-is-v-model-advantages-disadvantages-and-when-to-use-it/

[andersm]

Just changed the code to use arrays and get this error now!

Code: [Select]

Error 1 unable to find a register to spill in class 'POINTER_REGS'

That is an internal compiler error. First check if your compiler is up-to-date, and if the error persists report the bug.

EDIT: I should add that the best place to ask AVR-specific questions is the AVR Freaks forum. That's all they do!

[madires]

OK, I've rolled the code back to where it compiled this morning.  Maybe we should look at those warnings (to do with the font selection)

Code: [Select]

static void LCD_SetFont(char *font)
{
cfont.font=font;
cfont.x_size=fontbyte(0);
cfont.y_size=fontbyte(1);
cfont.offset=fontbyte(2);
cfont.numchars=fontbyte(3);
}

Are you accessing the font (stored in PROGMEM) via pgm_read_byte()?

[c4757p]

I don't find it to work. For instance:
...

Works for me on GCC 4.7.2.

[David_AVD]

Are you accessing the font (stored in PROGMEM) via pgm_read_byte()?

Yes.  That part seems to be all good now.

[David_AVD]

That is an internal compiler error. First check if your compiler is up-to-date, and if the error persists report the bug.

EDIT: I should add that the best place to ask AVR-specific questions is the AVR Freaks forum. That's all they do!

I did join the AVR forum, but have found so far that you guys (here) have a good handle on the overall scheme of things.   

As for the compiler version, I did check today and Atmel Studio says I have the latest one.  There are reports of the bug, duplicate reports and reports that it's fixed, but here it still is.  I don't know enough to make a useful bug report I suspect.

[IanB]

I don't find it to work. For instance:
...

Works for me on GCC 4.7.2.

What if you try it with the -ansi flag?

[c4757p]

Still works. However:

Code: [Select]

$ gcc -ansi -pedantic test.c
test.c: In function 'main':
test.c:7:20: warning: ISO C90 forbids compound literals [-pedantic]

Looks like it's one of the C99 features they silently backported to the main compiler. Whoops...

(However, if it works for you it could end up being the most efficient way to accomplish struct assignment, so if you only plan on compiling your code with GCC or other C99-supporting compilers I'd say go for it)

[andersm]

There are reports of the bug, duplicate reports and reports that it's fixed, but here it still is.  I don't know enough to make a useful bug report I suspect.

Many errors can cause the same error message, it just indicates that something's wrong with the register allocator. For a bug report you basically need a way to reproduce it. If you can't cut down your program and still trigger the bug, you'd have to check with Atmel if they can accept a test case under non-disclosure.

[David_AVD]

Getting the display to print the pointer addresses was actually very helpful.  I can now better visualise how the various parts of the structure is stored and accessed.

I can see that changing the caption field to a static array would potentially use more space, but the strings would then be part of the structure itself.

I'll continue to play around and learn more to solve the root cause of the issue.

[David_AVD]

A bit more experimenting shows that the "spill error" is only triggered when the structure is more than 20 bytes in size.

That is, I can add and remove fields or change their size (char, int, etc) to make the structure over 20 bytes or not.

Changing the size of the array (of structs) didn't seem to have any effect.

So, in the meantime perhaps I can remove the caption (char array) from the struct and move the captions to their own array.

[c4757p]

Can you post the line where you define the struct array? And when the struct is too big, what does the compiler think these are:

Code: [Select]

// This is the array, just to say what I'm calling the type and array name
struct Button buttons[30];

sizeof (struct Button);
sizeof (buttons[0]);

The last line might give a warning, but it should compile.

[David_AVD]

Those lines each give the error:

Code: [Select]

expected identifier or '(' before 'sizeof'

[c4757p]

Syntax error before those lines? Sizeof is standard C, I can't see them throwing an error like that.

[David_AVD]

I expected the error as it needs to be assigned to something?

[IanB]

Those lines each give the error:

Code: [Select]

expected identifier or '(' before 'sizeof'

Try putting

Code: [Select]

#include <stddef.h>
at the top of your source file.

Also, you might want to do something like this in order to see what the size is:

Code: [Select]

size_t size = sizeof(struct Button);
printf("Size is: %u\n", size);
(And for printf you would need <stdio.h>)

[David_AVD]

Remember you're talking to a C n00b here.

I am putting those lines in the same global.h file as the structure declaration.  Right or wrong?

[David_AVD]

Putting them in main:

Code: [Select]

statement with no effect [-Wunused-value]
Ian's commands do compile, but where is the output supposed to show up?

[c4757p]

I expected the error as it needs to be assigned to something?

It's perfectly valid C to mention a value without using it.

Code: [Select]

2;

That's a perfectly valid statement. "Hey compiler, the number 2 exists!"

Useless, of course. Either dump it out to the LCD or over serial, or use it as the initializer for a global variable and then use a tool like objdump to read it. I'm not aware of a way to make the compiler output it.

[c4757p]

Ian's commands do compile, but where is the output supposed to show up?

sizeof just returns a number, it's up to you how you look at it. Dump it to the LCD?

Putting them in main:

Ahhhhh.... I see what you were doing! I should have been clearer about what they do...

[David_AVD]

Ah, there you go.  Assume nothing my friends!  LOL

OK, a quick bodge outputs 17 to the screen for the size of the structure without any caption field which is correct.

Adding a string field (array of 6 char) changes it to 23 as expected.

The error only seems to occur when I use strcpy and the structure is over 20 bytes?

[David_AVD]

I did try a separate array for the captions:

Code: [Select]

char Captions[32][6];
I don't get the "spill errror" until I try to use strcpy as well.

[c4757p]

How exactly are you calling strcpy, and how exactly are the copy destinations defined?

Remember that each character array must have one extra spot to hold the string terminator, and if you overstep the array length you are screwed (and strcpy couldn't care less).

Also note that char Captions[32][6] is thirty-two strings of length six, not six strings of length thirty-two. Not sure if that's intentional - I don't think it is. That may be your problem or not... Lengths of six doesn't seem too useful to me, but you did say "about 30 buttons"...

[IanB]

Also note that char Captions[32][6] is thirty-two strings of length six

Remembering the null terminator, it is actually 32 strings of length 5--trap for the unwary...

[David_AVD]

I have 32 buttons, each which fit strings of up to 5 characters.  The longest strings I'm using are 5 characters.

In the interim, I changed the Draw_Button routine to take the button number as a parameter, instead of the address of the structure.  This in conjunction with the separate caption array has fixed the original "wrong text" issue.  The "spill" error is also absent now.

[c4757p]

I'm out of suggestions for now!

One thing you can do when you have weird memory-related errors, though it's a bit much for now: if you can pull out the relevant sections into something that can compile for PC, run it under Valgrind's Memcheck tool. It's surprisingly good at finding memory oopses. Linux-only IIRC, as it uses some pretty advanced OS features.

[David_AVD]

Everything is currently behaving as it should, so I guess we'll call this issue sorted for now.   

Having the captions in another array is not a big deal, but it would be nice to find the real cause behind that compiler error message.

It *may* have been to do with the Draw_Button procedure (with the struct as the parameter) as that seem to solve it all.   

[IanB]

Well a microcontroller is a tiny computer, so you need to be sympathetic to the limitations of the architecture. You can't necessarily copy big objects around the same as you can on a big computer.

[David_AVD]

I want to thank you guys and say I appreciate your persistence with helping me with this.   

I did learn a lot out of it and hope some of the onlookers did as well.

[mrflibble]

Did you manage to find out what the root cause was? I notice you said that mucking about with Draw_Button helped... Since you mention that it seems to be strcpy related, either a buffer overflow as suggested or maybe a stack overflow (because the amount of sram on that atmel might not be all that much)? Sometimes looking at the linker output is helpful, since you can see which bits go where. Might give you that a-hah moment.

[David_AVD]

I know very little about the linker, etc.  I just write the code and hit F5 to program!   

I would like to try adding the caption field back into the structure soon to see if it keels over again.

RAM shouldn't be an issue.  This chip has 8K of it.   

[Kremmen]

In one message you noted that you are C n00b. Is C++ totally out of the question, do you think? Because reading this thread is like listening to someone drawing their nails across a blackboard. An object (sic) like a button just begs - no, screams - to be implemented in the OO paradigm. I may have stated that already. Implementing it would be an absolute breeze, at the same time cleaning the code which has the risk of becoming a bit messy soon.
Maybe something like this (just something so you get the idea):


class button {

private:
  uint8_t x,y,w,h;    //coordinates and dimensions
  bool visible;
  //other stuff you need
  char caption[11]; 
  show();
  hide();

public:
  button();
  button(uint8_t xpos, ypos, width, height, char *cap); // either this or the next one, depends how dynamic you want (dare) to be
  init(uint8_t xpos, ypos, width, height, char *cap);      // you want this for statically constructed buttons
  move(uint8_t dx, dy);
  moveto(uint8_t nx, ny);
  rename(char *newcap);
}

button::button() {  // not really needed because this happens anyway
  x=y=h=w=0;
  visible = false;
  caption[0] = 0;
}

button::button(uint8_t xpos, ypos, width, height, char *cap) {
  x=xpos;
  y=ypos;
  w=width;
  h=height;
  visible = false;
  strlcpy(&caption, cap, sizeof(caption));
  show();
}

button::init(uint8_t xpos, ypos, width, height, char *cap) {
  x=xpos;
  y=ypos;
  w=width;
  h=height;
  visible = false;
  strlcpy(&caption, cap, sizeof(caption));
  show();
}
button::show() {
  // do what needs to be done to draw the button
  visible = true;
}

button::hide{
  // overwrite the button with the background
  visible = false;

button::move(uint8_t dx, dy) {
  hide();
  x+=dx;
  y+=dy;
  show();
}

button::moveto(uint8_t dx, dy) {
  hide();
  x=nx;
  y=ny;
  show();
}

button::rename(char *newcap) {
  hide();
  strlcpy(&caption, newcap, sizeof(caption));
  show();
}

- - - - - - -
#define NUM_BUTTONS 32;
button my_buttons[NUM_BUTTONS];
...

for (i=0; i < NUM_BUTTONS; i++) {
  my_buttons.init(some_x, some_y, some_h, some_w, some_cap_p);
}

my_buttons[random_index].rename("newcaption");

//done

[David_AVD]

I been using Delphi for PC programming for many years so know the OO concept well.

The reason I stayed with C for Atmel was that I'm also coding other projects in XC8 for PIC processors.  All my previous PIC programming (100's of projects) has been in assembler.

Maybe my reasoning is flawed, but I'd like to learn a common HLL for both types for now.  Time will tell and there's no reason I can't use C++ in the future once I get better at C.

[Kremmen]

OK, you pays your money and you takes your choice, as they say. For me C and C++ are essentially the same language - you can easily write C using C++ if you really want to. There are a few things to look out for, but those are easily handled.

[David_AVD]

A quick update; I moved the caption arrays back into the (array of) structure and it now compiles without error.  So it looks like that internal error I was getting was caused by trying to pass the structure as a parameter.  Changing to only passing the index (to the array) was the real fix.  Anyway, I hope that helps someone else.   

[David_AVD]

Back on the code for this project.  Now I'm wanting to find the size of a structure.

I'm trying SizeOf(structure_name) but getting an error that it's an undefined function.  What unit does it live in?

[c4757p]

It's just sizeof, all lowercase.

[David_AVD]

Thanks for that.  Gawd I hate case sensitivity some days!   

[c4757p]

Rule of thumb: if it's C, it's lowercase.

[dannyf]

pass the structure as a parameter.

Pass it as a pointer to a structure. And use typedef.

[dannyf]

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.

The behavior is compiler specific.

This is where I think embedded C took a wrong turn. "const" should really mean what it is: it's read only. If you want to move it to flash, use some other modifiers. Taking "const" to mean "flash" is simply wrong in my view.

[magiccow]

The syntax looks fine, and works on the gcc compiler. I'm guessing it is a quirk of your compiler.

You could try breaking into two steps:

struct _button {
  unsigned int top;
  :
  :
  char *caption;
};

typedef struct _button Button;

[David_AVD]

Everything is working when I use a fixed size array for the caption inside the structure, but as I add more features to the program it's becoming apparent that fixed size is not the way to go.

Most of the buttons only use a few characters for a caption, but some need up to 30.  So, I thought about storing the pointer in the structure instead of the actual caption.

Code: [Select]

struct Button
{
uint16_t top;
uint16_t left;
uint16_t height;
uint16_t width;
uint8_t GlyphIndex;
unsigned long BgColour;
unsigned long FgColour;
_Bool IsLabel;
_Bool Enabled;
char *Caption;
};

struct Button Buttons[55];
This compiles fine, but the button drawing is wrong.  Each button now draws the text of the last one assigned.

Here's the function that sets up a button:

Code: [Select]

void ButtonSet(uint8_t BtnNo, uint16_t top, uint16_t left, uint16_t height, uint16_t width, uint8_t GlyphIndex, unsigned long BgColour, unsigned long FgColour, _Bool IsLabel, _Bool Enabled, void * caption)
{
Buttons[BtnNo].top = top;
Buttons[BtnNo].left = left;
Buttons[BtnNo].height = height;
Buttons[BtnNo].width = width;
Buttons[BtnNo].GlyphIndex = GlyphIndex;
Buttons[BtnNo].BgColour = BgColour;
Buttons[BtnNo].FgColour = FgColour;
Buttons[BtnNo].IsLabel = IsLabel;
Buttons[BtnNo].Enabled = Enabled;
strcpy(Buttons[BtnNo].Caption, caption);
}
And here's an example call to it:

Code: [Select]

ButtonSet(BtnSetup, BtnRow0Top, 380, BtnHeight, 250, 0, VGA_BLUE, VGA_YELLOW, 0, 1, "SETUP");

So it seems like the pointer to each caption is the same, hence the same caption appearing on all buttons.

There's obviously still a gap in my understanding of how pointers interact with the compiler and how it stores the strings in program memory.

Can anyone take a look please and tell me what bone-headed mistake (or assumption) I've made?

[IanB]

When you write char *Caption you are creating a pointer to the place where a caption may be stored, but you have not made available any storage. The pointer is in effect a pointer to nowhere. This means you cannot use strcpy because this will try to copy characters into a designated storage location that is supposed to already exist. If the destination does not exist it will copy characters into the void (which might, accidentally, happen to work on your machine, but it's not right).

I don't have time to go into details of how to fix the problem right now, but using char *Caption is certainly on the right lines. What you need to do is to store each caption somewhere, and then make the Caption pointer point to that location.

A classic way of doing this is to place all the captions end to end in a single block of memory with each caption terminated by a null. This would preferably be done statically during program initialization. Then you simply make each caption pointer point to the start of the right caption in this memory block. If the captions are fixed, you could allocate the memory block in read-only flash memory as has been hinted earlier in this thread.

[David_AVD]

OK, looks like I've figured this one out.  The issue was in the ButtonSet function.

Changing from:

Code: [Select]

strcpy(Buttons[BtnNo].Caption, caption);
To this:

Code: [Select]

Buttons[BtnNo].Caption = caption;
Solved it!

Thanks for the explanation Ian.   

EDIT: The captions change frequently during program execution.

[IanB]

EDIT: The captions change frequently during program execution.

I see, but are all the possible captions known in advance? Or might they have infinite possibilities?

If all the captions are known in advance you can pre-allocate them in a caption table, then select from them as desired.

For instance, when you write this...

Code: [Select]

ButtonSet(BtnSetup, BtnRow0Top, 380, BtnHeight, 250, 0, VGA_BLUE, VGA_YELLOW, 0, 1, "SETUP");
...the compiler sees the string "SETUP" and makes space for it somewhere in a table of program strings. In an ideal world (if the compiler is clever enough), it will notice two or three uses of "SETUP" and will make them share the same entry in the table to avoid duplication.

What the compiler does is usually fine, however if you take control of the situation you may have the opportunity to fine tune what happens (such as putting the strings in flash memory instead of RAM).

Since I don't have any familiarity with Atmel Studio I can't tell if this is important or necessary to do.

[David_AVD]

Some of the strings are static, while others are user input.

It turns out I'm not out of the woods yet.  The buttons which have no caption (use a glyph instead) are now drawing the last caption.

I'll work though it and see how far I get.   

[David_AVD]

The issue is only when the button has been assigned an empty string.

If I assign those to a single space instead and detect that in the drawing function, I can draw nothing instead.

It seems like an ugly work around however.   

[IanB]

Maybe the LCD library doesn't like empty strings?

[David_AVD]

Maybe the LCD library doesn't like empty strings?

Doesn't seem to be that.  I added a test for zero length string in there (to bail out if found) and no change.