config struct to eeprom, best approach to deal with padding and CRC?

[rvalente]

Hello Mates,

Compiler is the GNU C for arm
IDE is STM32CubeIDE

I have all my non volatile data in a struct, this struct is loaded from the eeprom during power on cycle, to keep data integrity I'm using a CRC in the struct end

When turning on the device I read the EEPROM from my choosen address + the config size to buffer
Load the buffer to the struct using memcpy

Calculate the buffer CRC, which is all the data except the last two bytes (short int) which is the CRC and compare with the struct CRC.

But GNU C compilers uses struct padding and automatic alignment (which is the change in position of datatypes to better fit the data in memory, optimizing it access), so the last variable in your struct not necessarily is the last byte in the struct memory.

So, how should I approach this? Should I do my CRC calculation out of the struct? Like the next byte or so?

[dmendesf]

You can disable that data packing and have full control of the data alignment. You can also use sizeof() to return the actual size of the struct.

[rvalente]

You can disable that data packing and have full control of the data alignment. You can also use sizeof() to return the actual size of the struct.

I have other structs in code, so I do not believe disabling this good optimization resource is the best option..

Is it possible to tell the compiler that only for this struct I always wanna have these variable in this exact order?

[mikeselectricstuff]

Or maybe just crc the whole of the eeprom, including any unused areas due to padding.

[thinkfat]

You can disable that data packing and have full control of the data alignment. You can also use sizeof() to return the actual size of the struct.

I have other structs in code, so I do not believe disabling this good optimization resource is the best option..

Is it possible to tell the compiler that only for this struct I always wanna have these variable in this exact order?

__attribute__((packed)) disables struct member alignment, declaring the struct as "volatile" should make sure the compiler doesn't change order of members.

[Scrts]

Just write full struct and read it full with all the padding. Ignore it once the data is in the RAM. Unless you have limited RAM?

[ejeffrey]

The complier should never change the order of struct members, just add padding as specified by the ABI. Volatile only controls reordering of memory access not placement.  The packed attribute can and should be set on a per struct basis.  You can even have two versions of the struct: one packed for storage in the eeprom and one with standard alignment for use in memory. You just need to copy between then with a field be field copy instead of memcpy.

[donotdespisethesnake]

So, how should I approach this? Should I do my CRC calculation out of the struct? Like the next byte or so?

Make a union with an array of bytes. Do your CRC with the raw data.

If alignment is done for performance, and the CPU can access data on any byte boundary, then you can use "__attribute__ ((__packed__))" on the eeprom struct.

[rvalente]

Tks mates, Ill try those suggestions and report

[SiliconWizard]

So, how should I approach this? Should I do my CRC calculation out of the struct? Like the next byte or so?

Your call. You can either disable struct padding as said earlier, or do that separately as you yourself suggested.

Beware of disabling struct padding, as it could lead to unaligned accesses, which may or may not be supported on your target. Even if they are, they'll be less efficient (but that may not matter). Just something to keep in mind.

Either way, I usually choose the second solution.

[mikerj]

Just write full struct and read it full with all the padding. Ignore it once the data is in the RAM. Unless you have limited RAM?

Agreed, I'm struggling to see what the problem here is unless the goal is to minimize EEPROM space. You simply need the data in the EEPROM to be an exact byte copy of the struct, and you need to be consistent with the way the CRC is calculated on read and write. offsetof() and sizeof() are your friends.

[SiliconWizard]

Just write full struct and read it full with all the padding. Ignore it once the data is in the RAM. Unless you have limited RAM?

Agreed, I'm struggling to see what the problem here is unless the goal is to minimize EEPROM space. You simply need the data in the EEPROM to be an exact byte copy of the struct, and you need to be consistent with the way the CRC is calculated on read and write. offsetof() and sizeof() are your friends.

Indeed. If you put the CRC value as the last member of the struct, you can simply compute the CRC on the first 'offsetof(StructType, CRC)' bytes of the structure.

[ejeffrey]

I wouldn't use a union for this, it is not really what they are for.  It is perfectly OK in C to cast a struct foo * to a char * and use that for accessing the byte level representation of the struct to calculate the CRC or do a byte copy.  If you use a union you have to make sure that the length of the char[] field matches the length of the data fields.

Just use offsetof() and sizeof() as suggested by SiliconWizard and mikrj.  Packed can still be useful if you want to minimize space or if you need to conform to a binary representation that is architecture independent.  Of course, to be ABI independent you need to have the same type sizes, so make sure to use int32_t not int etc. 

AFAIK, gcc should correctly emit valid instruction sequences to access unaligned fields in a packed structure as long as you access via the structure type using the . or -> operators.  However, if you try to get tricky and pass around pointers to member elements without making sure those pointers have the unaligned attribute it will fail.

That, plus efficiency is why if you want packed storage for space efficiency I recommend having a packed and unpacked version of the struct type and doing a member-by-member copy between them.  Only the packed version would have the CRC.

[mikerj]

If not wasting space is a goal you can usually avoid packing and unaligned access by arranging the struct members in decreasing size order i.e. start with 32 bit, then 16 bit, then 8 bit.  This isn't always convenient though.

[donotdespisethesnake]

I wouldn't use a union for this, it is not really what they are for.  It is perfectly OK in C to cast a struct foo * to a char * and use that for accessing the byte level representation of the struct to calculate the CRC or do a byte copy.  If you use a union you have to make sure that the length of the char[] field matches the length of the data fields.

What nonsense. 

So many C programmers are just clueless.

[nctnico]

Hello Mates,

Compiler is the GNU C for arm
IDE is STM32CubeIDE

I have all my non volatile data in a struct, this struct is loaded from the eeprom during power on cycle, to keep data integrity I'm using a CRC in the struct end

When turning on the device I read the EEPROM from my choosen address + the config size to buffer
Load the buffer to the struct using memcpy

Calculate the buffer CRC, which is all the data except the last two bytes (short int) which is the CRC and compare with the struct CRC.

But GNU C compilers uses struct padding and automatic alignment (which is the change in position of datatypes to better fit the data in memory, optimizing it access), so the last variable in your struct not necessarily is the last byte in the struct memory.

So, how should I approach this? Should I do my CRC calculation out of the struct? Like the next byte or so?

Put the CRC calculation at the beginning of the struct. In my software I have created a header struct which has a CRC, size and version number. This is the first element of a data struct which has all the other data. The size field is filled with the size of the entire data struct and the version number is initialised depending on the software version. Now I can feed the data struct into a simple routines which casts it to the header struct and uses the size to calculated the CRC or check it (CRC and version). The rest of the data struct is just data to the CRC and version checking routine.
Don't mess around with padding of the data. It is not necessary and can only result in slower code. If size is a concern then sort the data struct based on the size of the variables to get to the least amount of alignment padding.

Code: [Select]

typedef struct TEepromHeader
{
uint32_t crc32; //CRC32 over the data
uint16_t version; //version number
uint16_t size; //size of the data
} TEepromHeader;

typedef struct TSetupData
{
TEepromHeader eeprom_header; // eeprom header
char serial[45]; //serial number
        ... more fields ....
} TSetupData;



Having the version number in there allows for an upgrade path of the firmware; in theory you can support various TSetupData versions and do silent conversions between them.