Is this struct acceptable?

sherzaad · December 30, 2024, 4:05pm

Hello,

I created the following struct to hold some data. All is working well (my actual code is running on a UNO) but I wanted to ask here if the struct itself is 'proper'!

constructive feedback wlecomed!

Example code using my struct:

typedef struct
{
	union { 
		uint32_t ex : 31;
		struct{
			uint16_t std : 16;
			uint16_t stdh : 15;
            uint16_t rtr : 1; 
		}__attribute__((packed)); 
	} id; 
	
	struct {
		uint8_t ide : 1; 	
		uint8_t fdf : 1;
		uint8_t brs : 1; 
		uint8_t esi : 1;
		uint8_t dlc : 4; 
	}__attribute__((packed))ctrl; 
	uint8_t *data; 
} tCAN;

tCAN msg;
void setup() {
  Serial.begin(115200);

  msg.id.ex = 0xFADCBEADL;

  Serial.println(msg.id.ex, HEX);
  Serial.println(msg.id.std, HEX);
  Serial.println(msg.id.rtr);
  msg.id.rtr = 1;
  Serial.println(msg.id.rtr);
  msg.id.rtr = 0;
  Serial.println(msg.id.rtr);

}

void loop() {
  // put your main code here, to run repeatedly:

}

output:

7ADCBEAD
BEAD
0
1
0

J-M-L · December 30, 2024, 5:47pm

The struct is fine (you don’t need to say typedef in C++) but the way you use it is not (In C++, accessing a union member that was not the last written to is well-defined only if the type being accessed is trivially compatible with the last written type; otherwise, it results in undefined behavior.)

kenb4 · December 30, 2024, 6:06pm

If you want to learn more, the term is type punning

sherzaad · December 31, 2024, 8:16am

Thank you for your reply.

so if I understood correctly, the struct and union usage here is acceptable is C but not in C++.

so my next question/request would be - How to refactor this struct for C++ so that I can use it without risk on undefined behaviour?

Thank you for the help!

J-M-L · December 31, 2024, 8:21am

Yes C allows this type of access, but not C++.

The link provided by @kenb4 provides further information. The typical approach would consist in adding a memcpy(). The compiler recognizes in most cases what’s the intention and won’t perform any memcpy() and that removes the UB.

Alternatively you could simply use accessor functions in your structure that would perform the necessary masking and bit shifting on a raw representation of the data.

sherzaad · December 31, 2024, 3:21pm

yeah... did not quite get how to use the memcpy to do that hence me asking!

like this?

typedef struct
{
	struct { 
		uint32_t _ex : 31;
		uint32_t rtr : 1; 
    
		void ex(uint32_t val){
		  _ex = val;
		}
		uint32_t ex(){
		  return _ex;
		}

		void std(uint16_t val){
		  _ex = (uint32_t) val;
		}

		uint16_t std(){
		  return (uint16_t) _ex;
		}
	
	} __attribute__((packed))id; 
	
	struct {
		uint8_t ide : 1; 	
		uint8_t fdf : 1;
		uint8_t brs : 1; 
		uint8_t esi : 1;
		uint8_t dlc : 4; 
	}__attribute__((packed))ctrl; 
	uint8_t *data; 
} tCAN;

tCAN msg;
void setup() {
  Serial.begin(115200);

  msg.id.ex(0xFADCBEADL);

  Serial.println(msg.id.ex(), HEX);
  Serial.println(msg.id.std(), HEX);
  msg.id.std(0xACED);
  Serial.println(msg.id.std(), HEX);
  Serial.println(msg.id.ex(), HEX);
  Serial.println(msg.id.rtr);
  msg.id.rtr = 1;
  Serial.println(msg.id.rtr);
  msg.id.rtr = 0;
  Serial.println(msg.id.rtr);
}

void loop() {
  // put your main code here, to run repeatedly:

}

7ADCBEAD
BEAD
ACED
ACED
0
1
0

if you could refactor the struct from post#1 to demostraste how to use memcpy to reply the union it would be much appreciated!

gfvalvo · December 31, 2024, 4:06pm

That's a 32-bit constant, but you've only specified ex as a 31-bit field:

BTW, you'll find unions used this way extensively in certain Arduino board support packages (eg SAMD21). It's legal if you appropriately compile the code as C and not C++. Files with a '.c' extension are compiled as C. Within .cpp files (and .h files #include(d) in .cpp files) the following construct is used:

// Code here is compiled as C++

#ifdef __cplusplus
 extern "C" {
#endif

// Code here is compiled as C

#ifdef __cplusplus
}
#endif

// Code here is compiled as C++

J-M-L · December 31, 2024, 4:12pm

yes, something like that — using bit masks and shifting

I'm not sure how you would want this to exactly work

your union holds a 32 bit structure (std, stdh, rtr) and a 31 bit (ex) attribute

it's like you had

struct __attribute__((packed)) A {
  uint32_t std : 16;
  uint32_t stdh : 15;
  uint32_t rtr : 1;
};

struct __attribute__((packed)) B {
  uint32_t ex : 31;
};

union C {
  B w;
  A  elems;
};

the number of meaningful bits in A and B are not the same, so if you start the lifetime of a B type element in a C instance

  C msg;
  msg.w.ex = 0xFEDCBA98;

then you can't know what happens to the 32th bit.

you coud initialise a A type instance with the bits of w

  A tmp;
  memcpy(&tmp, &msg.w, sizeof tmp);

or the msg

  A tmp;
  memcpy(&tmp, &msg, sizeof tmp);

and this way you have a valid A instance where it's legit to access the various fields, but you can't really say what made it there.

sherzaad · December 31, 2024, 5:08pm

yes that is correct.

msg.id.ex = 0xFADCBEADL; was to check/prove that only bits 0-30 would be stored into 'ex'

so, I guess in summary, if I want to continue using my struct as in the original post and not worry about 'refactoring' to be 'C++ legal' using the ifdef preprocessor would resolve the conflict, right?

#ifdef __cplusplus
 extern "C" {
#endif
typedef struct
{
	union {  //In C, C99 and later standards explicitly allow type punning using unions. C++ does not allow this
		uint32_t ex : 31;
		struct{
			uint16_t std : 16;
			uint16_t stdh : 15;
            uint16_t rtr : 1; 
		}__attribute__((packed)); 
	} id; 
	
	struct {
		uint8_t ide : 1; 	
		uint8_t fdf : 1;
		uint8_t brs : 1; 
		uint8_t esi : 1;
		uint8_t dlc : 4; 
	}__attribute__((packed))ctrl; 
	uint8_t *data; 
} tCAN;
#ifdef __cplusplus
}
#endif

tCAN msg;
void setup() {
  Serial.begin(115200);

  msg.id.ex = 0xFADCBEADL;

  Serial.println(msg.id.ex, HEX);
  Serial.println(msg.id.std, HEX);
  Serial.println(msg.id.rtr);
  msg.id.rtr = 1;
  Serial.println(msg.id.rtr);
  msg.id.rtr = 0;
  Serial.println(msg.id.rtr);

}

void loop() {
  // put your main code here, to run repeatedly:

}

as for the memcpy use, I suspected it would be a case of using 'outside' of the struct which for me is not as elegant as the struct format I am aiming for (unless there is any actually way of course! )

gfvalvo · December 31, 2024, 5:40pm

You need more. All punned accesses to the union must also be compiled as C, in a .c file. This may be more trouble than it's worth.

Header.h:

#ifndef HEADER_H
#define HEADER_H

#include <Arduino.h>

#ifdef __cplusplus
extern "C" {
#endif
typedef struct
{
  union {  //In C, C99 and later standards explicitly allow type punning using unions. C++ does not allow this
    uint32_t ex : 31;
    struct {
      uint16_t std : 16;
      uint16_t stdh : 15;
      uint16_t rtr : 1;
    } __attribute__((packed));
  } id;

  struct {
    uint8_t ide : 1;
    uint8_t fdf : 1;
    uint8_t brs : 1;
    uint8_t esi : 1;
    uint8_t dlc : 4;
  } __attribute__((packed))ctrl;
  uint8_t *data;
} tCAN;

uint32_t getEx(tCAN *theStruct);
uint16_t getStd(tCAN *theStruct);
uint16_t getStdh(tCAN *theStruct);
uint16_t getRtr(tCAN *theStruct);

#ifdef __cplusplus
}
#endif

#endif

Main.ino (or a .cpp file):

#include "Header.h"

tCAN msg;
void setup() {
  Serial.begin(115200);

  msg.id.ex = 0xFADCBEADL;

  Serial.println(msg.id.ex, HEX);

  // Punned access:
  Serial.println(getStd(&msg), HEX);

  // Punned access:
  Serial.println(getRtr(&msg));

  // OK to access same union type as last written in C++
  msg.id.rtr = 1;
  Serial.println(msg.id.rtr);

  // OK to access same union type as last written in C++
  msg.id.rtr = 0;
  Serial.println(msg.id.rtr);

}

void loop() {
  // put your main code here, to run repeatedly:

}

Access.c:

#include "Header.h"

uint32_t getEx(tCAN *theStruct) {
  return theStruct->id.ex;
}

uint16_t getStd(tCAN *theStruct) {
  return theStruct->id.std;
}

uint16_t getRtr(tCAN *theStruct) {
  return theStruct->id.rtr;
}

uint16_t getSth(tCAN *theStruct) {
  return theStruct->id.stdh;
}

gfvalvo · December 31, 2024, 6:53pm

Correction:
You can do it all in a single file without a separate .C:

#ifdef __cplusplus
extern "C" {
#endif

typedef struct
{
  union {  //In C, C99 and later standards explicitly allow type punning using unions. C++ does not allow this
    uint32_t ex : 31;
    struct {
      uint16_t std : 16;
      uint16_t stdh : 15;
      uint16_t rtr : 1;
    } __attribute__((packed));
  } id;

  struct {
    uint8_t ide : 1;
    uint8_t fdf : 1;
    uint8_t brs : 1;
    uint8_t esi : 1;
    uint8_t dlc : 4;
  } __attribute__((packed))ctrl;
  uint8_t *data;
} tCAN;

uint32_t getEx(tCAN *theStruct) {
  return theStruct->id.ex;
}

uint16_t getStd(tCAN *theStruct) {
  return theStruct->id.std;
}

uint16_t getRtr(tCAN *theStruct) {
  return theStruct->id.rtr;
}

uint16_t getSth(tCAN *theStruct) {
  return theStruct->id.stdh;
}

#ifdef __cplusplus
}
#endif

tCAN msg;
void setup() {
  Serial.begin(115200);

  msg.id.ex = 0xFADCBEADL;

  Serial.println(msg.id.ex, HEX);

  // Punned Accesses:
  Serial.println(getStd(&msg), HEX);
  Serial.println(getRtr(&msg));

  // Not punned. OK to access same union type as last written in C++
  msg.id.rtr = 1;
  Serial.println(msg.id.rtr);

  // Not punned. OK to access same union type as last written in C++
  msg.id.rtr = 0;
  Serial.println(msg.id.rtr);

}

void loop() {
}

PieterP · January 1, 2025, 3:28pm

extern "C" only affects linkage, the code is still compiled as C++, so the union trick would be UB.

Personally, I would just add setter/getter functions using bitwise logic (shifts and masks) to update/query a uint32_t id member variable. This also avoids the issue of endianness and bit field order (which are both implementation defined).

gfvalvo · January 1, 2025, 3:30pm

Ahh... So the code in My Post #10 with a separate .c file (compiled as C) would be required.

Topic		Replies	Views
need some coding suggestions Programming	6	551	May 5, 2021
Accessing a member of a struct within a newly defined Union type Programming	2	277	May 6, 2021
Structure Arduino Programming	8	1420	May 5, 2021
what am i doing wrong (union/struct) Programming	3	1099	May 6, 2021
union and struct to make byte array Programming	7	43804	May 5, 2021

Is this struct acceptable?

Related topics