This statement show you are early in your understanding of embedded C programming. The alignment of the bit fields is a hardware feature that should be implementation defined because hardware level bit manipulations are very hardware dependant.

Yes it is a hardware characteristic but not a compiler implementation characteristic, the language could be flexible enough to let me represent any hardware bit ordering and alignment I need, at the very least it could be a compiler option or something I can specify as a directive in the source.

 

Yes it is a hardware characteristic but not a compiler implementation characteristic, the language could be flexible enough to let me represent any hardware bit ordering and alignment I need, at the very least it could be a compiler option or something I can specify as a directive in the source.

The C language compiler for X version of hardware does allow for any possible representation of hardware bit ordering and alignment you would need on X set of devices. Making it implementation defined is an obvious better choice if we care more about the efficient and logical hardware operation of the device being easily implemented into a usable compiler (for sometimes very odd-ball hardware) than the wants of the programmer.

 

Anyway enough of this - now, why do I read the RF_SETUP register as being all zeros when the manual states clearly that when reset the values are far from all zeros:



I'm reading other registers and seeing the values I expect, but there's something odd about this one...

 

Let me try writing to it and re-reading it...

 

Yep it works, for some odd reason when reset, the register is all zeros not what the manual says. If I write something then when I read it again, I see the values come back - and I do zeroize the target variable to zero before reading just in case the read was doing nothing.

 

Well in the specific case here that struct is 32 bits long, but is actually a single byte padded by three bytes. The struct is aligned on a 4 byte boundary because I specified "int" I could specify "char" and the struct would shrink and end up aligning on a 1 byte boundary, I guess I might even do that but I'd need to consider implications.

Padding is a very different, and very specific, thing. Padding refers to padding bytes between the storage elements of a structure, not within them.

You specified your bit fields be stored in a storage element of type unsigned int. That type (on your compiler) is four bytes. Of the 32 bits in that storage element, the defined bit fields use eight of them. The values of the other 24 or undefined. They are NOT pad bytes. To see that this is the case, the compiler was completely free to assign the bit fields to the most significant byte of the storage element, which on a little endian machine would have placed the three unused bytes in that element as the first three bytes in the structure. However, the C language explicitly prohibits pad bytes at the beginning of a structure, hence those unused bytes cannot be pad bytes.

C could have done something similar, that's all I'm saying here.

So is your goal in this thread to understand something about C, or to complain more about C? Very different discussions.

 

Oh man, this is all my own fault ! I have a (copied) init routine that forces that reg to all zeros before my test code gets hit!

 

Oh man, this is all my own fault ! I have a (copied) init routine that forces that reg to all zeros before my test code gets hit!

Oops!

Everyone that has never done something similar, raise your hand. Anyone? Anyone?

Glad you tracked down the problem -- and hopefully learned something about the language that will help you avoid making at least some subtle, hard-to-find errors down the road.

 

The C language compiler for X version of hardware does allow for any possible representation of hardware bit ordering and alignment you would need on X set of devices. Making it implementation defined is an obvious better choice if we care more about the efficient and logical hardware operation of the device being easily implemented into a usable compiler (for sometimes very odd-ball hardware) than the wants of the programmer.

OK you might have a point here, even if the programmer could alter the alignments and padding and ordering that would still tie the code to whatever hardware, we'd still have to adjust it if we compiled the code for some incompatible platform - I accept that.

Sop it would make sense to me if this could be represented as a command line compiler argument, a bit like some compilers have already for stuff like target CPU and stuff...

 

Oops!

Everyone that has never done something similar, raise your hand. Anyone? Anyone?

Glad you tracked down the problem -- and hopefully learned something about the language that will help you avoid making at least some subtle, hard-to-find errors down the road.

Thanks. I'm enjoying coding for these devices, I haven't done much low level "systemy" kinds of development for a decade or so.

I've now got a good strategy for structuring the code with these unions, callbacks and so on. I don't like trying to develop non-trivial code in a sloppy haphazard manner because sooner or later that sloppiness itself becomes a hurdle, I do like to get a handle on how the code is organized and so on, early on.

I now have calls like this:

NrfReg_RF_SETUP rf = { 0 };

NrfLibrary.Read.RfSetupRegister(device_ptr, &rf, &status);

Where the function signature is strict - can only accept that specific argument type for the register values. Even though most of these register read/write calls are just single byte values (and ultimately the SPI HAL stuff is indifferent), the scope for human error with a lot of registers and stuff is much higher with a non-specific signature, when one copies/pastes etc and sooner or later something will misbehave because of human error - the less scope for developer misunderstanding the better.

 

...
So is your goal in this thread to understand something about C, or to complain more about C? Very different discussions.

Sometimes it takes a while to see why C is a great practical language at the shallow end of the computing pool. This is double true during the chip shortage when you need to really squeeze, by bending the programming rules, the last drop out of what chips you already have.

 

Well this union does work as desired, I can see memory change as expected as I set/unset various bits under debug:

union nrf_reg_EN_RXADDR_union
{
    uint8_t value;

    struct nrf_reg_EN_RXADDR
    {
        unsigned int ERX_P0 : 1;
        unsigned int ERX_P1 : 1;
        unsigned int ERX_P2 : 1;
        unsigned int ERX_P3 : 1;
        unsigned int ERX_P4 : 1;
        unsigned int ERX_P5 : 1;
        unsigned int RESERVED : 2;

    } fields;

};

i.e. setting P3 to 1 and P5 to 1 shows a raw byte value in memory of 0x28.

Here is another approach. Since we are only dealing with a single byte, endian matters are not going to be an issue. The union can be used as an intermediary rather than being part of the API:

#include <stdint.h>

/*
The `pack` pragma is supported by *most* compilers
*/
#pragma pack(push, 1)
typedef struct {
  unsigned int ERX_P0 : 1;
  unsigned int ERX_P1 : 1;
  unsigned int ERX_P2 : 1;
  unsigned int ERX_P3 : 1;
  unsigned int ERX_P4 : 1;
  unsigned int ERX_P5 : 1;
  unsigned int RESERVED : 2;
} nrf_reg_EN_RXADDR;
#pragma pack(pop)

/*
The the trailing underscore indicates that the union is for internal use only
*/
typedef union {
  nrf_reg_EN_RXADDR fields;
  uint8_t value;
} nrf_reg_EN_RXADDR_union_;

/*
Conversion API
*/
uint8_t nrf_reg_to_byte(nrf_reg_EN_RXADDR fields) {
  nrf_reg_EN_RXADDR_union_ ret;
  ret.fields = fields;
  return ret.value;
}

nrf_reg_EN_RXADDR nrf_reg_from_byte(uint8_t value) {
  nrf_reg_EN_RXADDR_union_ ret;
  ret.value = value;
  return ret.fields;
}

/*
Sample usage
*/
#include <stdio.h>

int main(void) {
  printf("nrf_reg_to_byte(nrf_reg_from_byte(0x28)): 0x%x\n",
         nrf_reg_to_byte(nrf_reg_from_byte(0x28)));
}

 

Sometimes it takes a while to see why C is a great practical language at the shallow end of the computing pool. This is double true during the chip shortage when you need to really squeeze, by bending the programming rules, the last drop out of what chips you already have.

View attachment 279734

I don't see how that image proves that C leads to less memory use than some other potential options, I mean what are you comparing to? what other languages have you written that code in, in order to make the comparison? Much of what is said here about C's generated code size is likely anecdotal.

Besides compilers can often optimize in different ways as you know, one can optimize for speed or size.

 

Here is another approach. Since we are only dealing with a single byte, endian matters are not going to be an issue. The union can be used as an intermediary rather than being part of the API:

#include <stdint.h>

/*
The `pack` pragma is supported by *most* compilers
*/
#pragma pack(push, 1)
typedef struct {
  unsigned int ERX_P0 : 1;
  unsigned int ERX_P1 : 1;
  unsigned int ERX_P2 : 1;
  unsigned int ERX_P3 : 1;
  unsigned int ERX_P4 : 1;
  unsigned int ERX_P5 : 1;
  unsigned int RESERVED : 2;
} nrf_reg_EN_RXADDR;
#pragma pack(pop)

/*
The the trailing underscore indicates that the union is for internal use only
*/
typedef union {
  nrf_reg_EN_RXADDR fields;
  uint8_t value;
} nrf_reg_EN_RXADDR_union_;

/*
Conversion API
*/
uint8_t nrf_reg_to_byte(nrf_reg_EN_RXADDR fields) {
  nrf_reg_EN_RXADDR_union_ ret;
  ret.fields = fields;
  return ret.value;
}

nrf_reg_EN_RXADDR nrf_reg_from_byte(uint8_t value) {
  nrf_reg_EN_RXADDR_union_ ret;
  ret.value = value;
  return ret.fields;
}

/*
Sample usage
*/
#include <stdio.h>

int main(void) {
  printf("nrf_reg_to_byte(nrf_reg_from_byte(0x28)): 0x%x\n",
         nrf_reg_to_byte(nrf_reg_from_byte(0x28)));
}

This is an interesting idea, I like it, I'll study this a bit more later! Hiding the unions from the API consumer is certainly a good idea.

 

Here is another approach. Since we are only dealing with a single byte, endian matters are not going to be an issue. The union can be used as an intermediary rather than being part of the API:

#include <stdint.h>

/*
The `pack` pragma is supported by *most* compilers
*/
#pragma pack(push, 1)
typedef struct {
  unsigned int ERX_P0 : 1;
  unsigned int ERX_P1 : 1;
  unsigned int ERX_P2 : 1;
  unsigned int ERX_P3 : 1;
  unsigned int ERX_P4 : 1;
  unsigned int ERX_P5 : 1;
  unsigned int RESERVED : 2;
} nrf_reg_EN_RXADDR;
#pragma pack(pop)

/*
The the trailing underscore indicates that the union is for internal use only
*/
typedef union {
  nrf_reg_EN_RXADDR fields;
  uint8_t value;
} nrf_reg_EN_RXADDR_union_;

/*
Conversion API
*/
uint8_t nrf_reg_to_byte(nrf_reg_EN_RXADDR fields) {
  nrf_reg_EN_RXADDR_union_ ret;
  ret.fields = fields;
  return ret.value;
}

nrf_reg_EN_RXADDR nrf_reg_from_byte(uint8_t value) {
  nrf_reg_EN_RXADDR_union_ ret;
  ret.value = value;
  return ret.fields;
}

/*
Sample usage
*/
#include <stdio.h>

int main(void) {
  printf("nrf_reg_to_byte(nrf_reg_from_byte(0x28)): 0x%x\n",
         nrf_reg_to_byte(nrf_reg_from_byte(0x28)));
}

This is a very good idea, and I leveraged it. Because pointer casting is relatively trouble free in C we can avoid conversion functions too, the args below are now using typedef names for the struct alone:

static void _ReadConfigRegister(NrfSpiDevice_ptr device_ptr, NrfReg_CONFIG_ptr Value, NrfReg_STATUS_ptr NrfStatus)
{
    _ReadSingleByteRegister(device_ptr, NrfRegister.CONFIG, &(((NrfReg_CONFIG_union_ptr)(Value))->value), NrfStatus);
}
static void _WriteConfigRegister(NrfSpiDevice_ptr device_ptr, NrfReg_CONFIG Value, NrfReg_STATUS_ptr NrfStatus)
{
    _WriteSingleByteRegister(device_ptr, NrfRegister.CONFIG, ((NrfReg_CONFIG_union_ptr)&(Value))->value, NrfStatus);
}

Where:

typedef union nrf_reg_CONFIG_union NrfReg_CONFIG_union, * NrfReg_CONFIG_union_ptr;
typedef struct nrf_reg_CONFIG_struct NrfReg_CONFIG, * NrfReg_CONFIG_ptr;

and

struct nrf_reg_CONFIG_struct
{
    uint8_t PRIM_RX : 1;
    uint8_t PWR_UP : 1;
    uint8_t CRCO : 1;
    uint8_t EN_CRC : 1;
    uint8_t MASK_MAX_RT : 1;
    uint8_t MASK_TX_DS : 1;
    uint8_t MASK_RX_DR : 1;
    uint8_t RESERVED : 1;

};

union nrf_reg_CONFIG_union
{
    uint8_t value;
    Reg_CONFIG fields;
};

These names for these structs and unions are becoming a bit unwieldy too so I'll likely refactor all those a little later.

Overall your idea is excellent, I've done that kind of thing a few times in the past "overlaying" some type over another type's storage. In PL/I incidentally we have the "defined" keyword, allows you to declare an instance of some struct and specify another different type variable as it's base address, ultimately leading to the same result more or less.

 

This is a very good idea, and I leveraged it. Because pointer casting is relatively trouble free in C we can avoid conversion functions too, the args below are now using typedef names for the struct alone:

static void _ReadConfigRegister(NrfSpiDevice_ptr device_ptr, NrfReg_CONFIG_ptr Value, NrfReg_STATUS_ptr NrfStatus)
{
    _ReadSingleByteRegister(device_ptr, NrfRegister.CONFIG, &(((NrfReg_CONFIG_union_ptr)(Value))->value), NrfStatus);
}
static void _WriteConfigRegister(NrfSpiDevice_ptr device_ptr, NrfReg_CONFIG Value, NrfReg_STATUS_ptr NrfStatus)
{
    _WriteSingleByteRegister(device_ptr, NrfRegister.CONFIG, ((NrfReg_CONFIG_union_ptr)&(Value))->value, NrfStatus);
}

Where:

typedef union nrf_reg_CONFIG_union NrfReg_CONFIG_union, * NrfReg_CONFIG_union_ptr;
typedef struct nrf_reg_CONFIG_struct NrfReg_CONFIG, * NrfReg_CONFIG_ptr;

and

struct nrf_reg_CONFIG_struct
{
    uint8_t PRIM_RX : 1;
    uint8_t PWR_UP : 1;
    uint8_t CRCO : 1;
    uint8_t EN_CRC : 1;
    uint8_t MASK_MAX_RT : 1;
    uint8_t MASK_TX_DS : 1;
    uint8_t MASK_RX_DR : 1;
    uint8_t RESERVED : 1;

};

union nrf_reg_CONFIG_union
{
    uint8_t value;
    Reg_CONFIG fields;
};

These names for these structs and unions are becoming a bit unwieldy too so I'll likely refactor all those a little later.

Overall your idea is excellent, I've done that kind of thing a few times in the past "overlaying" some type over another type's storage. In PL/I incidentally we have the "defined" keyword, allows you to declare an instance of some struct and specify another different type variable as it's base address, ultimately leading to the same result more or less.

As I remember, the FOTRAN COMMON keyword let you treat a set of memory locations as different variable types in different functions. We leveraged that while writing a database system in the language.

 

No, the struct has a single 32-bit unsigned int that is partitioned into a 7-bit bit field named RF_CH and a 1-bit bit field named RESERVED. Just where these 8 bits are located within those 32 bits is, with some constraints, up to the compiler implementer.

The C language standard does require that if an adjacent bit field member can be allocated within the storage unit of the prior one, that it must. So if unsigned int was just 16-bits on this compiler, the size of the struct would just be two bytes.

You missed my point. Memory is memory. get the address of the structure, it's size, and then output the bytes it's composed of and see what they data looks like. Not hard to do.

 

This is a very good idea, and I leveraged it. Because pointer casting is relatively trouble free in C we can avoid conversion functions too, the args below are now using typedef names for the struct alone:

static void _ReadConfigRegister(NrfSpiDevice_ptr device_ptr, NrfReg_CONFIG_ptr Value, NrfReg_STATUS_ptr NrfStatus)
{
    _ReadSingleByteRegister(device_ptr, NrfRegister.CONFIG, &(((NrfReg_CONFIG_union_ptr)(Value))->value), NrfStatus);
}
static void _WriteConfigRegister(NrfSpiDevice_ptr device_ptr, NrfReg_CONFIG Value, NrfReg_STATUS_ptr NrfStatus)
{
    _WriteSingleByteRegister(device_ptr, NrfRegister.CONFIG, ((NrfReg_CONFIG_union_ptr)&(Value))->value, NrfStatus);
}

Where:

typedef union nrf_reg_CONFIG_union NrfReg_CONFIG_union, * NrfReg_CONFIG_union_ptr;
typedef struct nrf_reg_CONFIG_struct NrfReg_CONFIG, * NrfReg_CONFIG_ptr;

and

struct nrf_reg_CONFIG_struct
{
    uint8_t PRIM_RX : 1;
    uint8_t PWR_UP : 1;
    uint8_t CRCO : 1;
    uint8_t EN_CRC : 1;
    uint8_t MASK_MAX_RT : 1;
    uint8_t MASK_TX_DS : 1;
    uint8_t MASK_RX_DR : 1;
    uint8_t RESERVED : 1;

};

union nrf_reg_CONFIG_union
{
    uint8_t value;
    Reg_CONFIG fields;
};

These names for these structs and unions are becoming a bit unwieldy too so I'll likely refactor all those a little later.

Overall your idea is excellent, I've done that kind of thing a few times in the past "overlaying" some type over another type's storage. In PL/I incidentally we have the "defined" keyword, allows you to declare an instance of some struct and specify another different type variable as it's base address, ultimately leading to the same result more or less.

STOP declaring your structures this way- it's WRONG. If you want to create a type, use TYPEDEF.

typedef struct
{
    uint8_t PRIM_RX : 1;
    uint8_t PWR_UP : 1;
    uint8_t CRCO : 1;
    uint8_t EN_CRC : 1;
    uint8_t MASK_MAX_RT : 1;
    uint8_t MASK_TX_DS : 1;
    uint8_t MASK_RX_DR : 1;
    uint8_t RESERVED : 1;
}nrf_reg_CONFIG_struct;

When you do it the way you 2 it, you create 2 problems- 1) You have to keep telling the compiler it's a struct and using that keyword everywhere. And 2, you're mis-using the foreward name declaration. 'nrf_reg_CONFIG_struct' is the type you're creating, so it goes at the end of the typedef. Putting after the keyword 'struct' is a foreward declaration name that the compiler uses ONLY in case the structure needs to reference ITSELF before it's been defined fully- like for a linked list.

Example:

typedef struct qelem
   {
   qelem *prevQptr;               // struct references itself before it's fulling defined
   int  myVal
   char  anotherVal
   }q;

I here you saying 'But everybody does it this way'- they are WRONG. I remember when this trend began. Just because the compiler can manage to make it work, doesn't mean it's right. Do it the right way for portability, maintainability, and understandability reasons.

IMHO :: stepping off my soapbox ::

 

STOP declaring your structures this way- it's WRONG. If you want to create a type, use TYPEDEF.

typedef struct
{
    uint8_t PRIM_RX : 1;
    uint8_t PWR_UP : 1;
    uint8_t CRCO : 1;
    uint8_t EN_CRC : 1;
    uint8_t MASK_MAX_RT : 1;
    uint8_t MASK_TX_DS : 1;
    uint8_t MASK_RX_DR : 1;
    uint8_t RESERVED : 1;
}nrf_reg_CONFIG_struct;

When you do it the way you 2 it, you create 2 problems- 1) You have to keep telling the compiler it's a struct and using that keyword everywhere. And 2, you're mis-using the foreward name declaration. 'nrf_reg_CONFIG_struct' is the type you're creating, so it goes at the end of the typedef. Putting after the keyword 'struct' is a foreward declaration name that the compiler uses ONLY in case the structure needs to reference ITSELF before it's been defined fully- like for a linked list.

Example:

typedef struct qelem
   {
   qelem *prevQptr;               // struct references itself before it's fulling defined
   int  myVal
   char  anotherVal
   }q;

I here you saying 'But everybody does it this way'- they are WRONG. I remember when this trend began. Just because the compiler can manage to make it work, doesn't mean it's right. Do it the right way for portability, maintainability, and understandability reasons.

IMHO :: stepping off my soapbox ::

I don't think you are correct in some of what you say, reread the post you replied to, it specifically shows the use of typedef, perhaps you missed it...

Defining all typedefs as I do (ahead of all structs or union definitions) allows one to refer to those type names (that is, declare self referential structures) inside the struct or union definitions before the appearance of that struct/union definition:

typedef struct node Node, * Node_ptr;

struct node
{
    int count;
    Node_ptr left_ptr;
    Node_ptr rite_ptr;
};

Declaring a struct that contains pointers to itself (for example) is made very easy this way. Also there is no need to "keep telling the compiler it's a struct and using that keyword everywhere". To declare instance of "struct node" all we need to do is:

Node top; // declare a root node

Node_ptr left = malloc(sizeof(Node));
Node_ptr rite = malloc(sizeof(Node));

top.left_ptr = left;
top.rite_ptr = rite;

You can see too that I have a practice of using typedef for both structures and pointers to those structures, this improves code readability IMHO.

Declaring the struct and typedef jointly prevents the declaration of certain self referential data structures, look:



Isolating typedefs to a dedicated header and including that before the header that defines those structs/unions is the most pragmatic way to represent these things, I use it as a convention for that reason, I'm aware of no down sides. I used C daily, routinely in my software work for years until about ten years ago. My job was in fact API design, I did this for a living on various platforms, I'm rusty of course (as I said in a prior post) but have used the language for some rather large and sophisticated designs.

There's nothing here that can be described as "wrong" this is fully supported by the language.

Now, you gave an example:

typedef struct qelem
   {
   qelem *prevQptr;               // struct references itself before it's fulling defined
   int  myVal
   char  anotherVal
   }q;

Tell me, how would you represent this:

typedef struct value_node ValueNode, * ValueNode_ptr;
typedef struct expr_node ExprNode, * ExprNode_ptr;

struct value_node
{
    ExprNode_ptr expression_ptr;
};

struct expr_node
{
    ValueNode_ptr value_ptr;
};

 

You missed my point. Memory is memory. get the address of the structure, it's size, and then output the bytes it's composed of and see what they data looks like. Not hard to do.

Not hard at all -- I showed code to do it back here.