Problem passing an array struct element to a function

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

I'm having this problem in Arduino, but I posted on the arduino.cc forum and didn't get a reply, so I thought I'd try over here.  After all, there IS an AVR in there (smiley ) and I think this is more of a C conceptual problem than an Arduino problem.

 

I create a struct like this:

typedef struct{
  uint16_t height;
  uint16_t width;
  uint16_t bytes;
  const unsigned char dig[820] PROGMEM;
} digit;

I create an instance of it like this:

digit One = {31, 122, 16,
{0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0,
 0xff, 0xff, 0xff, 0xff, 0xfe, 0x00, 0x03, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0,
 0xff, 0xff, 0xff, 0xff, 0xfe, 0x00, 0x03, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0,
 [remainder of array omitted for brevity] ... }};

I pass elements of this instance to a function like this (X and Y are defined elsewhere):

Partial_Update_Display_Area(X, Y, One.width, One.height, One.dig);

If I create the array as a stand alone array, and use its name (One_Partial) in the function call like this:

Partial_Update_Display_Area(X, Y, One.width, One.height, One_Partial);

everything works fine.  If I pass One.dig to the function, it causes runtime (not compile time) errors (the function causes a bitmap to be sent to an epaper display; the error takes the form of garbage in the form of part of a wrong bitmap being sent to the display, as if the function is reading from the wrong part of flash).  I assume I'm not passing the array (well, technically, the address of the first element of the array) correctly when it's part of the struct, but maybe I'm doing something else wrong.  Anyway, any tips greatly appreciated.

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

I don't use Arduino, bu the first thing I would do is turn on compiler output/error messages. You would have seen an error telling you the progmem attribute was ignored. A struct or array store contiguous data, so you cannot split up portions of it to different memory types. The following example assumes all the data in the struct is constant, if not then you have to split up the part you want in ram, and the part you want in flash.

 

#include <xc.h>

//this is just a type, and takes no storage
typedef struct{
  uint16_t height;
  uint16_t width;
  uint16_t bytes;
  uint8_t dig[4];
} digit_t;

//now its used, and placed into flash (all of it)
__flash const digit_t One = { 31,122,16, {1,2,3,4} };

//a function that takes a pointer to a __flash const
//no need to pass every individual member, just need the address
//of the digit_t and you get access to everything in it
void func(__flash const digit_t* d){
    unsigned char h = d->dig[PORTB&3]; //just getting a dig[0-3] value
    PORTB = h; //so compiler doesn't optimize it all away and ruin our example
}

int main(void) {
    func(&One); //call with address of One
}
/*
0000008a <func>:
  8a:	25 b1       	in	r18, 0x05	; 5
  8c:	23 70       	andi	r18, 0x03	; 3
  8e:	82 0f       	add	r24, r18
  90:	91 1d       	adc	r25, r1
  92:	fc 01       	movw	r30, r24
  94:	36 96       	adiw	r30, 0x06	; 6
  //here is the progmem access
  96:	84 91       	lpm	r24, Z
  98:	85 b9       	out	0x05, r24	; 5
  9a:	08 95       	ret

Disassembly of section .text.startup.main:

0000009c <main>:
  //One flash address = 0x0080 (address can also be seen in map file)
  9c:	80 e8       	ldi	r24, 0x80	; 128
  9e:	90 e0       	ldi	r25, 0x00	; 0

  a0:	0e 94 45 00 	call	0x8a	; 0x8a <func>

  a4:	80 e0       	ldi	r24, 0x00	; 0
  a6:	90 e0       	ldi	r25, 0x00	; 0
  a8:	08 95       	ret
 */

 

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

Aah, putting the instance of the struct in flash solved the problem.  Thanks so much.

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

Indeed. Your problems started with:

typedef struct{
  uint16_t height;
  uint16_t width;
  uint16_t bytes;
  const unsigned char dig[820] PROGMEM;
} digit;

Think about it - a struct is a "group of items in memory". How you could you have a "group" with "some bits over here and some bits over there" ?

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

Odd; I would have expected the compiler to be able to cope with a constant pointer to unsigned char, even if it *is* in PROGMEM.

 

Though that said, it's been a while since I've played with PROGMEM and I do recall having all sorts of odd issues with it (like, you need to declare it in even byte lumps, or it stuck null bytes in).

 

Neil

 

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

Ah, but can one put PROGMEM in the function signature, assuming the function then access the array from flash?   I'm guessing that the compiler puts in code to copy from flash to SRAM at some point in the above working version.

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

barnacle wrote:
I would have expected the compiler to be able to cope with a constant pointer to unsigned char

But it's not just a pointer; it is an array - with initialisation.

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> Think about it

 

Ha.  If I had a nickel for every time I should have done that ...

 

Now, however, I find that if I define the struct properly and invoke ten different instances of it as follows:

typedef struct{
  uint16_t height;
  uint16_t width;
  uint16_t bytes;
  uint8_t dig[820];
} digit;

const digit Zero PROGMEM = {46, 122, 16,
{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0, 0x00, 0x00, 0x1f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0,
 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0, 0x00, 0x00, 0x00, 0x00, 0x1f, 0xff, 0xff, 0xff, 0xff, 0xc0,
... }};

const digit One PROGMEM = {31, 122, 16,
 {0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0,
 0xff, 0xff, 0xff, 0xff, 0xfe, 0x00, 0x03, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0,
... }};

...

the program compiles fine, with a light touch on flash and RAM:

Sketch uses 11678 bytes (36%) of program storage space. Maximum is 32256 bytes.
Global variables use 199 bytes (9%) of dynamic memory, leaving 1849 bytes for local variables. Maximum is 2048 bytes.

But, if I try to put the ten instances into an array, and put the array in flash, as follows:

const digit digits[] PROGMEM = {Zero, One, Two, Three, Four, Five, Six, Seven, Eight, Nine};

flash and RAM blow up:

Sketch uses 34100 bytes (105%) of program storage space. Maximum is 32256 bytes.
Global variables use 8459 bytes (413%) of dynamic memory, leaving -6411 bytes for local variables. Maximum is 2048 bytes.

Ooops.  I'm not sure why things blow up in this particular way, but clearly I should be using an array of pointers to the structs instead of the actual instances.  Trying any of these, however:

const digit *digits[] PROGMEM = {Zero, One, Two, Three, Four, Five, Six, Seven, Eight, Nine};
digit const *digits[] PROGMEM = {Zero, One, Two, Three, Four, Five, Six, Seven, Eight, Nine};

Yields:

variable 'digits' must be const in order to be put into read-only section by means of '__attribute__((progmem))'

This:

const *digit digits[] PROGMEM = {Zero, One, Two, Three, Four, Five, Six, Seven, Eight, Nine};

Yields:

expected initializer before 'digits'

I'm afraid my pointer fu is weak.  Thanks for any tips.  

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

lautman wrote:
if I try to put the ten instances into an array

 

Think about it:

typedef struct{
  uint16_t height;
  uint16_t width;
  uint16_t bytes;
  uint8_t dig[820];
} digit;

each struct takes 826 bytes - so ten of them takes 8260 bytes!

 

surprise

 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
const digit *digits[] PROGMEM = {Zero, One, Two, Three, Four, Five, Six, Seven, Eight, Nine};

See if you get better mileage with:

const digit *digits[] PROGMEM = {&Zero, &One, &Two, &Three, &Four, &Five, &Six, &Seven, &Eight, &Nine};

(obviously when reading you are going to want a double dereference)

 

Oh and did we determine why you are using PROGMEM rather than __flash? Is this because of the C++ nature of Arduino? Can't you just put the flash data and the access code into an extern "C" file? (I dunno if Arduino supports inter-mixing C and C++). Anyway it might make life easier, especially when it does come to things like double dereference etc.

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

Thanks, clawson, but no dice.

const digit *digits[] PROGMEM = {&Zero, &One, &Two, &Three, &Four, &Five, &Six, &Seven, &Eight, &Nine};

variable 'digits' must be const in order to be put into read-only section by means of '__attribute__((progmem))'

PROGMEM is the only way Arduino supports storing things in flash (with a special exception for strings that are printed to the console).

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

Why not just put them in an array-

 

modified previous example-

__flash const digit_t all[10] =
{
    { 31,122,16, {1,2,3,4} }, //Zero
    { 31,122,16, {5,6,7,8} }, //One
    //...
};

 

func(&all[1]);

 

now &all[0] is Zero, &all[1] is One, and so on, and you now have a single thing to reference by index.

Last Edited: Mon. Sep 23, 2019 - 04:41 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


lautman wrote:
but no dice.
You may want to have a play with:

 

https://cdecl.org/

 

It won't know what the type "digit" is but:

 

 

So that is declaring a (non const!) array of pointer to elements that ARE const int (or digit or whatever). Compare this too:

 

 

So now the array of pointers itself as well as the objects the elements point to are const. So moving back to:

 

const digit *digits[] PROGMEM = {&Zero, &One, &Two, &Three, &Four, &Five, &Six, &Seven, &Eight, &Nine};

variable 'digits' must be const in order to be put into read-only section by means of '__attribute__((progmem))'

 

I fairly confidently predict this will not be seen when you use:

 

const digit * const digits[] PROGMEM = {&Zero, &One, &Two, &Three, &Four, &Five, &Six, &Seven, &Eight, &Nine};

 

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

clawson wrote:

 

I fairly confidently predict this will not be seen when you use:

 

const digit * const digits[] PROGMEM = {&Zero, &One, &Two, &Three, &Four, &Five, &Six, &Seven, &Eight, &Nine};

Yup, that did it.  Thanks again.  Not having any luck trying to dereference, or double-dereference.

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

curtvm wrote:

Why not just put them in an array-

 

modified previous example-

__flash const digit_t all[10] =
{
    { 31,122,16, {1,2,3,4} }, //Zero
    { 31,122,16, {5,6,7,8} }, //One
    //...
};

 

func(&all[1]);

 

now &all[0] is Zero, &all[1] is One, and so on, and you now have a single thing to reference by index.

Thanks, curtvm.  I'll give this a try if I can't the the struct approach to work.  This seems to be the sort of problem structs were created for, plus I don't have a lot of experience with them, so this seemed like a good way to learn something.  Finally, structs would make the code more readable in this application, since each struct element is referenced by its name instead of an index, as would be the case with an array:  digits[x].width vs. digits[x][0].

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

lautman wrote:
each struct element is referenced by its name instead of an index

Is generally a Good Thing - but having names like "Zero", "One", "Two", etc seems to somewhat defeat the object?

 

frown

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 Finally, structs would make the code more readable in this application, since each struct element is referenced by its name instead of an index, as would be the case with an array:  digits[x].width vs. digits[x][0].

 

uint16_t w = digits[1].width;

vs

uint16_t w = One.width;

 

so its [1] vs One. I would rather have the index which gives more options when needed, plus the fact they are all contiguous and little thought needed to access.

 

If you really want the One, Two,...

 

#define One digits[1]

uint16_t w = One.width;
 

 

Last Edited: Mon. Sep 23, 2019 - 05:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Heh heh.  The array in each struct is actually a bitmap for a digit:  Zero -> 0, One -> 1, ....

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

even so, an index seems more appropriate there than naming them with the digit names

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

With an index, you can simplify things-

 

void Partial_Update_Display_Area(int X, int Y, uint8_t n){
    if(n >= sizeof(digits)/sizeof(digits[0]) ) return;
    func(&digits[n]);
}

 

Partial_Update_Display_Area(0,0,5);

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

curtvm wrote:

Why not just put them in an array-

 

modified previous example-

__flash const digit_t all[10] =
{
    { 31,122,16, {1,2,3,4} }, //Zero
    { 31,122,16, {5,6,7,8} }, //One
    //...
};

 

func(&all[1]);

 

now &all[0] is Zero, &all[1] is One, and so on, and you now have a single thing to reference by index.

Looking at this more closely, what is digit_t, and how would I access, say, the second element of One?  &all[1][1]?

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

typedef struct{

    uint16_t height;

    uint16_t width;

    uint16_t bytes;

    uint8_t dig[4];

} digit_t;

 

I added the _t to make it clear digit_t is a type, not a var.

 

__flash const digit_t all[10] =
{
    { 31,122,16, {1,2,3,4} }, //Zero
    { 31,122,16, {5,6,7,8} }, //One
    //...
};

 

change 'all' to whatever you want- digits, digit, whatever.

 

> the second element of One?

 

I'm not sure what you mean, there is no 'second element' of a struct. In your code One is a struct, in my code all[1] is a struct. Do you mean the second element of all[1].dig ?

 

all[1].dig[1] is the second element of the dig array in the second struct, equivalent to One.dig[1] if you had individual named structs like Zero, One, etc.

 

all[0] is a struct, all[1] is a struct, ..., all[9] is a struct- its an array of structs, just like any other array

all[0].width is a uint16_t member of the first struct

all[9].dig is an array member of the tenth struct

all[9].dig[3] is the fourth element of the array member dig in the tenth struct

 

__flash const digit_t* p = &all[5];

p points to the sixth struct in flash, and is at address all+(5*sizeof(digit_t)).

p->width is the same as all[5].width

p->dig[2] is the same as all[5].dig[2]

 

Since its an array of structs, which coincide with the numbers 0-9, now you only need to pass a number from 0-9 and you can get at all the struct data with knowing only that number. If you make separate structs such as One, Two, etc., now you have to figure out how to translate a number to a struct name with all the extra resulting code.

 

When you want to display a count from 0-9 at 3,3 for example-

for( uint8_t i = 0; i <= 9; i++, _delay_ms(500) ) display( 3,3, i );

 

I'm not sure what yours would look like with separate named structs for each number, but its not going to be pretty, or efficient.

 

There is no downside to using the array of structs, and has many benefits.

 

 

 

 

 

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

Got it, curtvm.  That's actually exactly the behavior I was trying to achieve.  I just started by creating the structs outside of the array.  Seems to be working in my application now, too.  Thanks!

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

barnacle wrote:

Odd; I would have expected the compiler to be able to cope with a constant pointer to unsigned char, even if it *is* in PROGMEM.

 

But array in C is NOT a pointer. This misconception is a source of many confusions.

Dessine-moi un mouton

Last Edited: Tue. Sep 24, 2019 - 10:03 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As opposed to confusion due to people not understanding the very close similarities between arrays & pointers:

 

https://www.avrfreaks.net/commen...

 

cheeky

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I thought there may be an issue with the compiler if it allows his declaration.   Here is what I get w/ avr-gcc 5.4.0:

 

$ cat zz1.c
#include <stdint.h>
#include <avr/pgmspace.h>

struct foo {
  uint8_t x;
  uint8_t y;
  const uint8_t z[10] PROGMEM;
} a[3];

uint8_t bar(uint8_t ix) {
  return a[ix].z[0];
}


$ avr-gcc -mmcu=atmega328p -S zz1.c
zz1.c:7:3: warning: '__progmem__' attribute ignored [-Wattributes]
   const uint8_t z[10] PROGMEM;
   ^

So I'm curious what is going on.