Structure with a size

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

I have two variables defined as unsigned chars that I would like to make members of a two-byte (short) structure. I can't seem to find a way to define the size of the structure, and the compiler complains about the "incompatible type" with a function that utilizes the structure. I'm sure there is a better way to do this. Code is as follows:

 

 

struct TapePos{
        unsigned char Minutes;
        unsigned char Seconds;
        } TapePos;

 

 

write_display(TapePos);

 

 

void write_display(short input)
{
    bintobcd(input);
    char upper = (thous << 4) | hunds;
    char lower = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp,GPIOB,upper);            // Write Thous Digit and Hunds Digit
    i2c_WriteReg(MCP_RTZdisp,GPIOA,lower);            // Write Tens Digit and Ones Digit
}

 

 

 

Thanks for the help!

 

Dan

This topic has a solution.
Last Edited: Mon. Feb 15, 2021 - 09:47 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This makes no sense!

 

Here you prototype (ie, declare) the function as taking your structure type:

write_display( TapePos );

 

But then you define it as taking a short:

void write_display( short input )
{
    bintobcd(input);
    char upper = (thous << 4) | hunds;
    char lower = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp,GPIOB,upper);            // Write Thous Digit and Hunds Digit
    i2c_WriteReg(MCP_RTZdisp,GPIOA,lower);            // Write Tens Digit and Ones Digit
}

Your struct has minutes & seconds (sexagesimal), but your  function definition is talking about Thousands, hundreds, tens, and units (decimal)

 

Your definition needs to match your declaration; eg,

void write_display( TapePos input )
{
    i2c_WriteReg( MCP_RTZdisp, GPIOB, input.Minutes );            // Write minutes
    i2c_WriteReg( MCP_RTZdisp, GPIOA, input.Seconds );            // Write seconds
}

 

What, exactly, are you trying to achieve here?

 

A good approach can be to start by writing the comment which describes what your function does - then write the code to do that.

 

/*
 * Function to ... [describe what it does]
 *
 * Parameters:
 *   [describe the parameters; what do they mean? how are they used?]
 *
 * Returns:
 *   [describe any return value]
 */

 

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 should have excluded some portions of the code, which were originally written to test certain concepts, but are no longer relevant. I realize this is confusing. My apologies. And no, I am not terribly adept at writing code, thus the question, which still stands...

 

Is there a way to force or restrict the size of the structure to two bytes?

 

~D

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

Follow Andy's advice i.e. write the comment lines first.
.
Then someone might implement the function to match your specification.
.
It is much easier to learn when you are shown examples from real life.
.
David.

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

What makes you think the struct is bigger than 2 bytes?

Surely the struct is always the sum size of its constituent members.

Last Edited: Sat. Feb 13, 2021 - 09:17 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

DHembree wrote:
I should have excluded some portions of the code

The best thing is to post a minimum but complete example which demonstrates your problem.

 

Please see Tip #1 in my signature, below, for how to properly post source code.

 

Or, as David says, give us a full description of what you're actually trying to achieve - then we can give suggestions on how to reach that goal.

 

DHembree wrote:
Is there a way to force or restrict the size of the structure to two bytes?

Why do you feel the need to do that?

 

http://www.catb.org/esr/faqs/sma...

 

I think you need to read-up on the basics of structures.

 

The size of a structure is defined by the sum of the sizes of its members (plus any padding)

 

Being an 8-bit processor, you won't get padding on an AVR - so a struct with two 1-byte members will be 2 byes.

 

schtevo wrote:
Surely the struct is always the sum size of its constituent members

In general, there may be padding - but, as noted,  unlikely in the case of AVR8.

 

DHembree wrote:
I am not terribly adept at writing code

As this is just standard 'C' stuff - not specific to AVR or Atmel/Microchip - the common advice is that you would be a lot better off learning the basics of the language on a PC.

A PC will give you a far more "comfortable" learning experience - without all the added complications and restrictions of small embedded microcontrollers.

 

Here are some 'C' learning & reference materials - including a free online textbook: 

 

https://blog.antronics.co.uk/2011/08/08/so-youre-thinking-of-starting-with-c/

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...
Last Edited: Sat. Feb 13, 2021 - 10:27 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

schtevo wrote:

What makes you think the struct is bigger than 2 bytes?

Surely the struct is always the sum size of its constituent members.

 

At first glance, yes. But the packing may not be such that the contents are aligned in the smallest possible way. Consider a structure with an int8_t and an int32_t on a 32 bit system; likely that's taken eight bytes.

 

With two int8_t, on an AVR, probably not a problem. It's going to fit into an int16_t.

 

Neil

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

DHembree wrote:
void write_display(short input)

Also avoid using short, int, long, better to use stdint.h types such as uint8_t etc....

as the sizes do not change from one compiler to another.

Now tell us about this tape drive you have connected to your AVR???

 

jim

 

 

Keys to wealth:

Invest for cash flow, not capital gains!

Wealth is attracted, not chased! 

Income is proportional to how many you serve!

 

Last Edited: Sat. Feb 13, 2021 - 08:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the input.

 

The compile error I get is:

 

incompatible type for argument 1 of 'write_display'

 

I assumed it was because of the padding. I figured if I could restrict or predefine what size structure was, then I would avoid the error.

 

I have hardware that drives an interrupt routine that in turn maintains the position of audio tape transport (cuddos, Jim, for figuring that out!) in minutes and seconds. The code needs to drive (existing) old-school 7-segment displays in BCD. I want to collectively refer to minutes and seconds (thus the structure) but need to also handle them individually.

 

I will look into using standard data types such as uint8_t instead of my current method.

 

~Dan

 

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

DHembree wrote:
I will look into using standard data types such as uint8_t instead of my current method.

You give up far too easily. We use a struct to keep related data items together. One benefit being that we can pass their members around as a single entity. Your {Play Time} data is a perfect example of use of a struct.

 

Here is an example of how it can be used: I thought your function write_display() was badly named; so I've renamed it to write_tapepos()

typedef struct TapePos {
    unsigned char Minutes;
    unsigned char Seconds;
    } TapePos;

void write_tapepos (TapePos tapepos)
{
    unsigned char digit;

    bintobcd(tapepos.Minutes);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOB, digit);            // Write Minutes

    bintobcd(tapepos.Seconds);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOA, digit);            // Write Seconds
}

void main (void)
{
    TapePos tapepos;

    tapepos.Minutes = 10;
    tapepos.Seconds = 59;
    write_tapepos(tapepos);
}

 

Enjoy:

 

{edit} OOPS!: I missed the typedef keyword from the struct definition. I've just added that; so my example should now compile correctly.

 

Last Edited: Mon. Feb 15, 2021 - 10:29 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

DHembree wrote:

The compile error I get is:

 

incompatible type for argument 1 of 'write_display'

DHembree wrote:

struct TapePos{
        unsigned char Minutes;
        unsigned char Seconds;
        } TapePos;

 

 

write_display(TapePos);

 

void write_display(short input)

your function should be type struct as in:

void write_display(struct TapePos);

 

Jim

I remember audio tape (300 baud KC standard), those were NOT the good old days! crying

 

Keys to wealth:

Invest for cash flow, not capital gains!

Wealth is attracted, not chased! 

Income is proportional to how many you serve!

 

Last Edited: Mon. Feb 15, 2021 - 09:08 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Oh, I didn't give up. I knew there was a solution at hand, and I knew the struct was the way to go, just didn't know how to implement it quite the right way. Your simplified code did the trick, but still needed Jim's suggestion of the 'struct' in the prototype. Thank you so much.

 

Jim, this is an display interface/controller board in a 70's vintage analog audio tape machine. As you can imagine, existing parts are well into obsolescence for these machines. We are re-designing to breathe new life into them. They still sound amazing!

 

Thanks, guys!

Dan

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

For curiosity: which tape machines?

 

Neil (old broadcast engineer, used to live and breathe Studer A80 and friends)

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

DHembree wrote:
Jim's suggestion of the 'struct' in the prototype.

That was what I said in #2,  except it wasn't clear whether you really wanted the parameter to be the struct or the short - so I went with the short.

 

The point is, the prototype (declaration) and the definition (implementation) need to match.

 

 

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: 1

N.Winterbottom wrote:

typedef struct TapePos {
    unsigned char Minutes;
    unsigned char Seconds;
    } TapePos;

void write_tapepos (TapePos tapepos)
{
    unsigned char digit;

    bintobcd(tapepos.Minutes);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOB, digit);            // Write Minutes

    bintobcd(tapepos.Seconds);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOA, digit);            // Write Seconds
}

Just to note that when you specify a struct as a function parameter this will involve a complete struct copy. While the struct is just two uchar's as in this case the size does not really matter but if the struct was something "real big" like an image with header fields and a large data array!) you might want to pass by reference not value in which case it would be:

void write_tapepos (TapePos * pTapepos)
{
    unsigned char digit;

    bintobcd(pTapepos->Minutes);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOB, digit);            // Write Minutes

    bintobcd(pTapepos->Seconds);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOA, digit);            // Write Seconds
}

or if you are using C++ then you can use a reference:

void write_tapepos (TapePos & tapepos)
{
    unsigned char digit;

    bintobcd(tapepos.Minutes);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOB, digit);            // Write Minutes

    bintobcd(tapepos.Seconds);
    digit = (tens << 4) | ones;
    i2c_WriteReg(MCP_RTZdisp, GPIOA, digit);            // Write Seconds
}

In this case you don't need to change the syntax of the member access as the "&" in the parameter (reference) hides the fact that it is access via pointer.

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

barnacle wrote:

For curiosity: which tape machines?

 

Neil (old broadcast engineer, used to live and breathe Studer A80 and friends)

 

Neil, the machines are MCI JH110 and JH24.

 

Thanks again for the help, all!

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

clawson wrote:
when you specify a struct as a function parameter this will involve a complete struct copy

also with a struct return value ?

 

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: 1

Yes.    Most C compilers will return the complete struct.

 

However,   (from memory) the C language does not mandate it.

 

C expects you to pass a pointer to the struct and return a struct pointer.

In practice it is perfectly reasonable to pass a small struct as an argument and return a small struct.    e.g. two uint32_t

 

It is not very efficient to pass a 10kB struct and return it.

But like many things in C.   It provides the gun and the bullets.     You can shoot your foot if that is what you want.

 

David.

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

awneil wrote:
also with a struct return value ?
Yes:

 

#include <stdlib.h>
#include <avr/io.h>

typedef struct {
    int width;
    int height;
    int bpp;
    uint8_t pixels[64 * 32];
} image_t;

__attribute__((noinline)) image_t genPic() {
    image_t pic;
    pic.width = 64;
    pic.height = 32;
    pic.bpp = 8;
    for (int n = 0; n < 64 * 32; n++) {
        pic.pixels[n] = rand();
    }
    return pic;
}

int main(void) {
    image_t newPic;
    newPic = genPic();
    PORTB = newPic.pixels[(newPic.width * 3) + 17];
    while(1);
}

The return code for genPic() is:

    return pic;
  c6:	80 e4       	ldi	r24, 0x40	; 64
  c8:	90 e0       	ldi	r25, 0x00	; 0
  ca:	9a 83       	std	Y+2, r25	; 0x02
  cc:	89 83       	std	Y+1, r24	; 0x01
  ce:	80 e2       	ldi	r24, 0x20	; 32
  d0:	90 e0       	ldi	r25, 0x00	; 0
  d2:	9c 83       	std	Y+4, r25	; 0x04
  d4:	8b 83       	std	Y+3, r24	; 0x03
  d6:	88 e0       	ldi	r24, 0x08	; 8
  d8:	90 e0       	ldi	r25, 0x00	; 0
  da:	9e 83       	std	Y+6, r25	; 0x06
  dc:	8d 83       	std	Y+5, r24	; 0x05
  de:	86 e0       	ldi	r24, 0x06	; 6
  e0:	98 e0       	ldi	r25, 0x08	; 8
  e2:	fe 01       	movw	r30, r28
  e4:	31 96       	adiw	r30, 0x01	; 1
  e6:	d6 01       	movw	r26, r12
  e8:	01 90       	ld	r0, Z+
  ea:	0d 92       	st	X+, r0
  ec:	01 97       	sbiw	r24, 0x01	; 1
  ee:	e1 f7       	brne	.-8      	; 0xe8 <genPic+0x66>
}
  f0:	c6 01       	movw	r24, r12
  f2:	ca 5f       	subi	r28, 0xFA	; 250
  f4:	d7 4f       	sbci	r29, 0xF7	; 247
  f6:	0f b6       	in	r0, 0x3f	; 63
  f8:	f8 94       	cli
  fa:	de bf       	out	0x3e, r29	; 62
  fc:	0f be       	out	0x3f, r0	; 63
  fe:	cd bf       	out	0x3d, r28	; 61
 100:	df 91       	pop	r29
 102:	cf 91       	pop	r28
 104:	1f 91       	pop	r17
 106:	0f 91       	pop	r16
 108:	ff 90       	pop	r15
 10a:	ef 90       	pop	r14
 10c:	df 90       	pop	r13
 10e:	cf 90       	pop	r12
 110:	08 95       	ret

OK some of that are the code for lines above (like pic.width = 64 etc) but the majority if a big old copy loop:

  de:	86 e0       	ldi	r24, 0x06	; 6
  e0:	98 e0       	ldi	r25, 0x08	; 8

...

  ec:	01 97       	sbiw	r24, 0x01	; 1
  ee:	e1 f7       	brne	.-8      	; 0xe8 <genPic+0x66>

So it's copying 0x806 bytes. That is 2054 bytes. That is 3 * int = 6 bytes for the metadata  plus 64x32 (2048) for the pixels.

 

Obviously there's only some AVRs you could even do this on and in those that maybe only have 4K you probably don't want multiple copies. So something more like:

__attribute__((noinline)) void genPic(image_t * pPic) {
    image_t pic;
    pPic->width = 64;
    pPic->height = 32;
    pPic->bpp = 8;
    for (int n = 0; n < 64 * 32; n++) {
        pPic->pixels[n] = rand();
    }
}

int main(void) {
    image_t newPic;
    genPic(&newPic);

is a better idea so only one lot of 2054 bytes ever needs to exist.

 

(the reason I know about this is that I once made the mistake and I think it was 12MB image planes involved at the time - the code seemed unusually "slow" even on a moderately fast PC!)