esp32-owb icon indicating copy to clipboard operation
esp32-owb copied to clipboard

Packed Union

Open iotexpert opened this issue 3 years ago • 4 comments

typedef union { /// Provides access via field names struct fields { uint8_t family[1]; ///< family identifier (1 byte, LSB - read/write first) uint8_t serial_number[6]; ///< serial number (6 bytes) uint8_t crc[1]; ///< CRC check byte (1 byte, MSB - read/write last) } fields; ///< Provides access via field names

uint8_t bytes[8];              ///< Provides raw byte access

} OneWireBus_ROMCode;

Should probably be

typedef union { /// Provides access via field names struct fields { uint8_t family[1]; ///< family identifier (1 byte, LSB - read/write first) uint8_t serial_number[6]; ///< serial number (6 bytes) uint8_t crc[1]; ///< CRC check byte (1 byte, MSB - read/write last) } __PACKED fields; ///< Provides access via field names

uint8_t bytes[8];              ///< Provides raw byte access

} OneWireBus_ROMCode;

I don't think that there is a guarantee that compiler will align the family, seri... to bytes.

iotexpert avatar Sep 06 '20 14:09 iotexpert

That's an interesting point, and you may be right, I'm not sure.

From what I can tell from reading articles such as http://www.catb.org/esr/structure-packing/#_structure_alignment_and_padding, the alignment is (usually? always?) taken from the smallest field, in this case a uint8_t (byte), or 1 byte, so no padding occurs, and it would be likely to be ok, but I'm not able to find out definitively across all C compilers. Unfortunately the __attribute((packed)) tag is non-standard, and I'm not sure where __PACKED is defined (is that in the IDF?).

In C at least, the first element of the struct (and the union) is guaranteed to be at offset zero, and the union array is guaranteed to be contiguous. ESR seems to think that if you follow his guidelines then you don't need to use implementation-specific declarations or tags.

It's also complicated by a potential difference in behaviour when compiled with a C++ compiler, using #pragma pack, and again this is implementation-defined. Also the struct doesn't have to start at offset zero either.

Can you offer an alternative representation of this data that is more portable?

DavidAntliff avatar Sep 07 '20 04:09 DavidAntliff

Hi David, what about using an union of 8 bytes and the three fields totalling 8 bytes?

csobrinho avatar Jul 26 '21 18:07 csobrinho

@csobrinho hmm, I don't quite follow - can you suggest some example code, please?

For future reference, here's the original code concerned:

https://github.com/DavidAntliff/esp32-owb/blob/6f90bcdc4d7c29f3888f18e1baf74ad3116c823d/include/owb.h#L82

I'm still on the fence about this whole thing because I haven't been able to find a truly portable solution, yet...

DavidAntliff avatar Jul 27 '21 02:07 DavidAntliff

Nevermind, I only saw the diff and didn't see it was already inside a union. Disregard my comment :)

csobrinho avatar Jul 28 '21 05:07 csobrinho