sizeof and structure

[Ravishankar S]

enum usb_offset {
usb_idx_function = 0,
usb_idx_size = usb_idx_function + sizeof(unsigned char),
usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned char),
usb_idx_i2c_subaddr = usb_idx_i2c_addr + sizeof(unsigned char),
usb_idx_i2c_datasize = usb_idx_i2c_subaddr + sizeof(unsigned char),
usb_idx_i2c_data = usb_idx_i2c_datasize + sizeof(unsigned char)
};
break;
...
}

At this point I expect to have 'usb_idx_i2c_addr' equal to 'data[3]', but

'usb_idx_i2c_addr' will be equal to 3 (the index) , not data[3]. enumeration
values _are_ the offsets into
the data[] array.

 

[Ravishankar S]

enum usb_offset {
usb_idx_function = 0,
usb_idx_size = usb_idx_function + sizeof(unsigned char),
usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned char),
usb_idx_i2c_subaddr = usb_idx_i2c_addr + sizeof(unsigned char),
usb_idx_i2c_datasize = usb_idx_i2c_subaddr + sizeof(unsigned char),
usb_idx_i2c_data = usb_idx_i2c_datasize + sizeof(unsigned char)
};
break;
...
}

At this point I expect to have 'usb_idx_i2c_addr' equal to 'data[3]',

but
values _are_ the offsets into
the data[] array.

Infact 'usb_idx_i2c_addr' is not 3 , but 2.
usb_idx_function = 0;
usb_idx_size = usb_idx_function + sizeof(unsigned char) = 0 + 1 = 1
usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned char) = 1 + 1 = 2

 

[fnegroni]

Can I ask a silly question?
Why are you using artificially manufactured structs to parse a byte
array?
Is it to make access to the byte array's sections more readable?
Why not just use macros?

I am a newbie so I am just curious to know.

 

[Ravishankar S]

Can I ask a silly question?
Why are you using artificially manufactured structs to parse a byte
array?
Is it to make access to the byte array's sections more readable?
Why not just use macros?

I am a newbie so I am just curious to know.

I suppose its for readability only. comprehension is better when structs are
used.

 

[Ben Bacarisse]

I suppose its for readability only. comprehension is better when structs are
used.

As someone who suggested using offsets + access functions I have to
say that it depends on how well the access functions are written. The
code can be made very readable but, more importantly, can be made more
portable (note that some byte fields are really parts of larger
types -- size_lo size_hi).

What is odd is to see both approaches "half" used.

 

[Ben Bacarisse]

I already asked about my problem and received several valuable advices which
I have followed to. Anyway I should explain briefly.
In the program I'm working on I receive a data stream, having structure as
follows:

| common_header | I2C_header | data ... |

Just to be clear... you call it a "common header". Do you sometimes
need to process messages like this:

| common_header | data ... |
or
| common_header | some_other_header | data ... |
?

My purpose is to parse the 'common_header', and read the data following it.

This is my code snippet:

#define MSG_SIZE 1000

enum ctrl_cmds {
CMD_ECHO = 0x0,
CMD_I2C_WRITE,
CMD_I2C_READ,
CMD_I2C_BURST_WRITE
};

struct __attribute__ ((packed)) USB_iobuf_common_header_s {
unsigned char function;
unsigned char size_lo;
unsigned char size_hi;
};

typedef struct USB_iobuf_common_header_s USB_iobuf_common_header;

struct __attribute__ ((packed)) USB_iobuf_i2c_header_s {
unsigned char addr;
unsigned char subaddr_lo;
unsigned char subaddr_hi;
unsigned char datasize_lo;
unsigned char datasize_hi;
};

typedef struct USB_iobuf_i2c_header_s USB_iobuf_i2c_header;

enum usb_offset {

When I posted to suggest using an offset enum like this, it was as an
alternative to non-portable packed structs. Not in addition to them!
This just gives you two things to maintain.

usb_idx_function = 0,
usb_idx_size = usb_idx_function + sizeof(unsigned char),
usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned char),

I thought I wrote 'usb_idx_size + sizeof(uint16_t),'. I certainly
intended you to document the offsets by using the sizes of the data in
the header and the function is followed by two bytes, not one. Of
course, you can have an index for usb_idx_size_lo and usb_idx_size_hi
if you want and then have the index of addr be that for size_hi + 1.

usb_idx_i2c_subaddr = usb_idx_i2c_addr + sizeof(unsigned char),
usb_idx_i2c_datasize = usb_idx_i2c_subaddr + sizeof(unsigned char),
usb_idx_i2c_data = usb_idx_i2c_datasize + sizeof(unsigned char)
};

char data[MSG_SIZE];

unsigned char is usually safer.

 

[suresh shenoy]

Hello,

I already asked about my problem and received several valuable advices which
I have followed to. Anyway I should explain briefly.
In the program I'm working on I receive a data stream, having structure as
follows:

      |  common_header | I2C_header | data ... |

My purpose is to parse the 'common_header', and read the data following it..

This is my code snippet:

#define MSG_SIZE 1000

enum ctrl_cmds {
    CMD_ECHO = 0x0,
    CMD_I2C_WRITE,
    CMD_I2C_READ,
    CMD_I2C_BURST_WRITE

};

struct __attribute__ ((packed)) USB_iobuf_common_header_s {
    unsigned char function;
    unsigned char size_lo;
    unsigned char size_hi;

};

typedef struct USB_iobuf_common_header_s USB_iobuf_common_header;

struct __attribute__ ((packed)) USB_iobuf_i2c_header_s {
    unsigned char addr;
    unsigned char subaddr_lo;
    unsigned char subaddr_hi;
    unsigned char datasize_lo;
    unsigned char datasize_hi;

};

typedef struct USB_iobuf_i2c_header_s USB_iobuf_i2c_header;

enum usb_offset {
     usb_idx_function = 0,
     usb_idx_size  = usb_idx_function  + sizeof(unsigned char),
     usb_idx_i2c_addr = usb_idx_size   + sizeof(unsigned char),
     usb_idx_i2c_subaddr = usb_idx_i2c_addr  + sizeof(unsigned char),
     usb_idx_i2c_datasize = usb_idx_i2c_subaddr + sizeof(unsigned char),
     usb_idx_i2c_data     = usb_idx_i2c_datasize + sizeof(unsigned char)

};

char data[MSG_SIZE];
char reg;
...

CDC_Read(..., data, MSG_SIZE);

USB_iobuf_common_header *hdr = (USB_iobuf_common_header *)data;
switch (hdr->function) {
    case CMD_I2C_READ:
        AT91F_TWI_ReadSingleIadr(AT91C_BASE_TWI, (usb_idx_i2c_addr << 16),
data[4], AT91C_TWI_IADRSZ_1_BYTE, &reg);
         ...
        break;
        ...

}

At this point I expect to have 'usb_idx_i2c_addr' equal to 'data[3]', but
it's always 0, though the other end is sending 0xC0. If I use 'data[3]' or
'data[4]' - everything works perfectly, but this is ugly hack.
I'm stuck. What's the problem with that?

The part of problem problem was with alignment of structure, I found out how
to disable padding and I also added the offsets (via 'enum') to be more
readable.

Thanks.

With best regards, Roman Mashak.  E-mail: (e-mail address removed)

Roman,

A better approach would be for you to create instances of the
structure and populate them as you read the stream.
Since u have declared the structs are 'packed' they are contigeous and
hence no padding of any sort. So in your case if you want to read the
common header

USB_iobuf_i2c_header header;

read(filehandle, &header, sizeof(USB_iobuf_i2c_header));

At this point the file pointer will be pointing to I2C_header and you
could use the same approach to populate the appropriate buffers.

This may help you alleviate some issues.

Suresh Shenoy

 

[Roman Mashak]

Hello,

I already asked about my problem and received several valuable advices which
I have followed to. Anyway I should explain briefly.
In the program I'm working on I receive a data stream, having structure as
follows:

| common_header | I2C_header | data ... |


My purpose is to parse the 'common_header', and read the data following it.

This is my code snippet:

#define MSG_SIZE 1000

enum ctrl_cmds {
CMD_ECHO = 0x0,
CMD_I2C_WRITE,
CMD_I2C_READ,
CMD_I2C_BURST_WRITE
};

struct __attribute__ ((packed)) USB_iobuf_common_header_s {
unsigned char function;
unsigned char size_lo;
unsigned char size_hi;
};

typedef struct USB_iobuf_common_header_s USB_iobuf_common_header;

struct __attribute__ ((packed)) USB_iobuf_i2c_header_s {
unsigned char addr;
unsigned char subaddr_lo;
unsigned char subaddr_hi;
unsigned char datasize_lo;
unsigned char datasize_hi;
};

typedef struct USB_iobuf_i2c_header_s USB_iobuf_i2c_header;

enum usb_offset {
usb_idx_function = 0,
usb_idx_size = usb_idx_function + sizeof(unsigned char),
usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned char),
usb_idx_i2c_subaddr = usb_idx_i2c_addr + sizeof(unsigned char),
usb_idx_i2c_datasize = usb_idx_i2c_subaddr + sizeof(unsigned char),
usb_idx_i2c_data = usb_idx_i2c_datasize + sizeof(unsigned char)
};

char data[MSG_SIZE];
char reg;
....

CDC_Read(..., data, MSG_SIZE);

USB_iobuf_common_header *hdr = (USB_iobuf_common_header *)data;
switch (hdr->function) {
case CMD_I2C_READ:
AT91F_TWI_ReadSingleIadr(AT91C_BASE_TWI, (usb_idx_i2c_addr << 16),
data[4], AT91C_TWI_IADRSZ_1_BYTE, &reg);
...
break;
...
}

At this point I expect to have 'usb_idx_i2c_addr' equal to 'data[3]', but
it's always 0, though the other end is sending 0xC0. If I use 'data[3]' or
'data[4]' - everything works perfectly, but this is ugly hack.
I'm stuck. What's the problem with that?

The part of problem problem was with alignment of structure, I found out how
to disable padding and I also added the offsets (via 'enum') to be more
readable.

Thanks.


With best regards, Roman Mashak. E-mail: (e-mail address removed)

 

[Ben Bacarisse]

Hello, Ben!
You wrote on Wed, 09 Jan 2008 11:35:25 +0000:

??|>> common_header | I2C_header | data ... |

BB> Just to be clear... you call it a "common header". Do you sometimes
BB> need to process messages like this:

??|> common_header | data ... |
BB> or
??|> common_header | some_other_header | data ... |
BB> ?

Probably never. The format of message is defined and fixed.

Then you could consider putting both headers in one struct and you
will (most likely) not have to do any system-specif packing magic --
the header will consist of 8 chars.

If, for some reason you all need indexes into a char array, you can
use the offsetof macro to get them.

Making indexes readable is suffice, then the offsets really should look like
this:

enum usb_offset {
usb_idx_function = 0,
usb_idx_size = usb_idx_function + sizeof(unsigned char),
usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned short),
usb_idx_i2c_subaddr = usb_idx_i2c_addr + sizeof(unsigned char),
usb_idx_i2c_datasize = usb_idx_i2c_subaddr + sizeof(unsigned short),
usb_idx_i2c_data = usb_idx_i2c_datasize + sizeof(unsigned short)
};

Am I correct?

Yes, but you might have to check short is the size you want. That is
why I suggested uint16_t.

Having indexes without access functions seems pointless to me. If you
want to have both the struct and the index enum, why not use offsetof?
It would be more readable than my method.

 

[Roman Mashak]

Hello, Ben!
You wrote on Wed, 09 Jan 2008 11:35:25 +0000:

??|>> common_header | I2C_header | data ... |

BB> Just to be clear... you call it a "common header". Do you sometimes
BB> need to process messages like this:

??|> common_header | data ... |
BB> or
??|> common_header | some_other_header | data ... |
BB> ?

Probably never. The format of message is defined and fixed.

??>>
BB> When I posted to suggest using an offset enum like this, it was as an
BB> alternative to non-portable packed structs. Not in addition to them!
BB> This just gives you two things to maintain.

??>> usb_idx_function = 0,
??>> usb_idx_size = usb_idx_function + sizeof(unsigned char),
??>> usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned char),

BB> I thought I wrote 'usb_idx_size + sizeof(uint16_t),'. I certainly
BB> intended you to document the offsets by using the sizes of the data in
BB> the header and the function is followed by two bytes, not one. Of
BB> course, you can have an index for usb_idx_size_lo and usb_idx_size_hi
BB> if you want and then have the index of addr be that for size_hi + 1.

Making indexes readable is suffice, then the offsets really should look like
this:

enum usb_offset {
usb_idx_function = 0,
usb_idx_size = usb_idx_function + sizeof(unsigned char),
usb_idx_i2c_addr = usb_idx_size + sizeof(unsigned short),
usb_idx_i2c_subaddr = usb_idx_i2c_addr + sizeof(unsigned char),
usb_idx_i2c_datasize = usb_idx_i2c_subaddr + sizeof(unsigned short),
usb_idx_i2c_data = usb_idx_i2c_datasize + sizeof(unsigned short)
};

Am I correct?

With best regards, Roman Mashak. E-mail: (e-mail address removed)

 

[Roman Mashak]

Hello, Ben!
You wrote on Thu, 10 Jan 2008 11:42:10 +0000:

[skip]
BB> Then you could consider putting both headers in one struct and you will
BB> (most likely) not have to do any system-specif packing magic -- the
BB> header will consist of 8 chars.

BB> Yes, but you might have to check short is the size you want. That is
BB> why I suggested uint16_t.

BB> Having indexes without access functions seems pointless to me. If you
BB> want to have both the struct and the index enum, why not use offsetof?
BB> It would be more readable than my method.

Oh, great idea. Why didn't I consider 'offsetof' macro.
Thanks!

With best regards, Roman Mashak. E-mail: (e-mail address removed)