1

I have a structure which contains memory for an EEPROM:

#pragma pack(push,1)
struct EEPROM_Memory
{
  char Device_ID[8];
  uint8_t Version_No;
  otherRandomVariables...
};
#pragma pack(pop)

I then create an instance, populate the struct and calculate it's CRC using:

CRC32::calculate(reinterpret_cast<uint8_t*>(&(memory)), sizeof(EEPROM_Memory));

I have been going crazy trying to figure out why the CRC check returns different values.

My best guess is pragma pack is not working therefore there is still some padding. This padding is volatile thus resulting in different CRC values.

What would be a nice solution (i.e not manually checking each value in the struct for their CRC)?

Any suggestions much appreciated!

P.S. I have looked at this question but sadly it does not give many suggestions on what to do. Surely this is a common problem!

P.P.S I am using an ESP32 microcontroller, unsure which compiler. The CRC library I am using is CRC32 by Christopher Baker.

Will Powell
  • 71
  • 8
  • 1
    Once solution is to manually `memcpy`, field by field, into a single `char` buffer, thus logically proving that there's no padding. But before doing that, it's laughably simple to check if there's no padding: add everything up and compare what `sizeof` tells you. Did you try that? – Sam Varshavchik Jun 24 '22 at 18:04
  • 1
    Adding to Sam's comment: If you do rely on there being no padding, better add a `static_assert` checking this. – fabian Jun 24 '22 at 18:09
  • Thank you for your replies. `size of` shows to have the correct number of bytes (I.e. no padding). Even after I do a `memcmp` for two identical structures, they return different checksums. – Will Powell Jun 24 '22 at 18:14
  • What data type is `memory`? – Ben Voigt Jun 24 '22 at 18:32
  • Dump the bytes for the complete structure in Hex and workout what's different. Then work backwards to why the bytes are different. – Richard Critten Jun 24 '22 at 18:32
  • I have dumped the data and flippin' identical. I am convinced it's a library problem at this point but I have tried another with the same problem. @BenVoigt `memory` is a reference of the struct (type `EEPROM_Memory&`) – Will Powell Jun 24 '22 at 18:45
  • If the bytes are identical and the CRC changes then all that's left is the CRC code itself. Is the code by any chance including the CRC output in the bytes being CRC'd ie putting the CRC result into `struct EEPROM_Memory` somewhere ? – Richard Critten Jun 24 '22 at 18:48
  • @RichardCritten No, the CRC output is not part of the EEPROM::Memory instance. Appreciate the sanity checking though. – Will Powell Jun 24 '22 at 18:52
  • 1
    "...the CRC check returns different values." Different from what? Are you using the same CRC algorithm/function each time? Or are you comparing against a CRC that was calculated elsewhere with a different algorithm? Do you get different results for the same data when you call the CRC function repeatedly? Is the seed value set correctly? – kkrambo Jun 24 '22 at 19:27
  • 1
    Christopher Baker's API is unclear to me but I suspect you may have to call `reset()` before each new CRC calculation. That appears to reset the seed to zero. If you don't call `reset()` then it might continue with the seed value equal to the previous output. – kkrambo Jun 24 '22 at 19:31
  • 1
    Also, the `update` method is not static so you shouldn't be able to call it like `crc::update()`. I think you have to create an instance of the crc class and call it like `crc.update()` – kkrambo Jun 24 '22 at 19:38
  • Subsequent comments have invalidated all the assertions/assumptions in the question. SO is not a discussion forum, you should place all the new information in the question, not the comments. `#pragma pack` is supported for compatibility with MSVC you should normally use `struct __attribute__ ((__packed__)) ...` - the behaviour of a compiler for unrecognised pragmas is to ignore them, they are compiler specific in name, syntax and semantics - if you use `#pragma` you _really_ have to know what compiler you are using to be sure it will do _anything_. – Clifford Jun 25 '22 at 10:07
  • > _" [...] This padding is volatile [...]"_ : It is undefined/non-deterministic, not "volatile" - that would suggest that it can change spontaneously - it can't. And in any case, there is no padding. – Clifford Jun 25 '22 at 10:08
  • The error is clearly in code not shown in your question; making it very difficult to answer - hence the endless comments and "guesses. How have you initialised `memory`? Is it `static` or automatic? You have only said that you have "polulated" it - if it is no statically initialised and you do not explicitly assign _all_ members, it will contain non-deterministic values. I strongly suggest explicit zero initialisation: `struct EEPROM_memory memory = {0} ;`. We cannot take your word for it that the unseen code is correct; you have to provide all necessary information for diagnosis. – Clifford Jun 25 '22 at 10:24
  • @kkrambo : Looking at the source, CRC32::calculate() is static and internally instantiates a temporary CRC32, the constructor initialises the `state` member. – Clifford Jun 25 '22 at 10:29
  • `CRC32::calculate()` is a template function. You can (and probably should) invoke it as the designer no doubt intended: `CRC32::calculate( &memory, 1 ) ;` - I cannot see that will fix your problem, but it does leave the casting to the implementation so avoid potential errors. – Clifford Jun 25 '22 at 10:36

3 Answers3

4

Pretty sure the pack is working as it should and you are using it wrong.

Note that pack is not recursive. So if your any of your otherRandomVariables is a struct then that struct may contain padding.

What you should do is static_assert the size of the struct. Then you know if it has the expected size or some padding. If it contains any structs assert the size of those structs first.

Goswin von Brederlow
  • 11,875
  • 2
  • 24
  • 42
  • Thank you for your answer. I do not have any nested structures. I have used `assert`, unfortunately `static assert` does not compile, and `sizeof` is always the correct size. Any other ideas? – Will Powell Jun 24 '22 at 18:30
  • 1
    That makes no sense: `static_assert(sizeof(Foo) == 123);` should compile just fine. If the size if correct then it can't be a padding issue. So do print out the whole memory for the struct and go through the CRC code step by step. – Goswin von Brederlow Jun 24 '22 at 18:56
2

I have a suspicion that memory is a pointer, and you're taking its address, thereby calculating the checksum of the pointer and following bytes rather than the data it points to.

The reinterpret_cast is wholly unnecessary, the calculate function you are using is templated. And like all the cast keywords, reinterpret_cast cripples compiler type-checking and disables a whole lot of useful warnings and errors, so you really should consider it a last resort.

Try this:

auto result = CRC32::calculate(memory, 1);

If and only if you have tried that and it fails to compile because memory is not actually a pointer, then

auto result = CRC32::calculate(&memory, 1);
Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • I appreciate you being suspicious! Sadly it does not compile when I use `calculate(memory, 1)` as `memory` is not a pointer. You are right I did not need the `reintrpret_cast`, however still the same problem. I will try to use a different library. Any recommendations? – Will Powell Jun 24 '22 at 19:01
  • @WillPowell Ben could have avoided "guessing" had you included the instantiation or at least specified the _type_ of `memory` e.g. `struct EEPROM_memory memory = {0} ;` – Clifford Jun 25 '22 at 10:19
  • @WillPowell : Then `CRC32::calculate( &memory, 1 ) ;` - it is a template function, you probably should not attempt to cast it - it defeats the purpose of being a template. – Clifford Jun 25 '22 at 10:39
1

Solved - I was doing something silly.

Previously I used nested structures for the EEPROM Memory, and as Goswin said, using pack does not work recursively. So this resulted in unwanted padding.

I had previously removed this as it was suspicious but after not resolving my problem, due to a silly mistake, I put it back in!

Thank you for all your responses - It is good to know that going through Structs byte by byte is not frowned upon, you just need to be hyper aware of padding.

Will Powell
  • 71
  • 8
  • It sounds like you should accept @Goswin von Brederlow's answer then. – romkey Jun 25 '22 at 22:59
  • As I suggested then, the error was in code not provided in the question. Unless you are prepared to update the question to expose the information necessary for it to be answerable, the question has no merit in remaining in SO and should probably be deleted. Had you posted your original code Godwin's answer would be validated, had you posted sufficient of the modified code, your "mistake" may have been spotted. Ultimately even this "answer" is not really an answer; all we know is that you fixed it. In view of the lack of clarity and answerability, I am voting to close unfortunately. – Clifford Jun 26 '22 at 10:06
  • Apologies for the lack of clarity. I have accpeted Goswin's answer. I still think this question holds value as there are few posts on here discussing how to pack and manipulate structs – Will Powell Jun 26 '22 at 17:12