4
\$\begingroup\$

After many years I started for fun to program in C again. Now I have an issue with the use of structures and unions.

The declaration:

typedef struct {
    unsigned char   SOAKTEMP;
    unsigned char   SOAKTIME;
    unsigned char   REFLOWTEMPERATURE;
    unsigned char   REFLOWTIME;
    unsigned char   BAKETEMP;
    unsigned int    BAKETIME;
    unsigned char   COOLDOWNTEMPERATURE;
    unsigned char   COOLDOWNTIME;
    unsigned char   KP;
    unsigned char   KI;
    unsigned char   KD;
    unsigned char   CYCLETIME;    
}EEPROM_DEFAULTS;

The initialisation:

EEPROM_DEFAULTS EEPD = {
                     170,    // SoakTemp
                      90,    // SoakTime   
                     220,    // ReflowTemperature
                      30,    // ReflowTime
                      80,    // BakeTemp
                    7200,   // BakeTime
                      60,    // CooldownTemperature
                       0,    // CooldownTime
                      11,    // Kp
                       5,    // Ki
                       1,    // Kd
                       5     // cycleTime

};

To write and read this data into the EEPROM of the PIC16F18877 I need to split BAKETIME into a high and a low byte.

I have done this with a Union.

union{ 
        unsigned int Baketime;
        struct
        {
            unsigned char BakeTimeLowbyte;
            unsigned char BaketimeHighbyte;
        };
        } BT;

It works but I do not like the way I have done this. I have tried in many ways to incorporate the union into the EEPROM_DEFAULTS typedef but without succes. So my knowledge runs short after so many years. Assistance would be nice.

\$\endgroup\$
  • \$\begingroup\$ Why do you need to make it into a low and high byte? \$\endgroup\$ – pipe Jan 11 '18 at 9:43
  • \$\begingroup\$ @pipe The EEPROM needs 8 bits. int is 16 bits. For communicating the data in an other part of the program I also need 8 bits. \$\endgroup\$ – Decapod Jan 11 '18 at 9:58
  • 2
    \$\begingroup\$ I'm prety sure the compiler can put the 16 bits into two bytes, in the correct order (think about the endian-ness of the microcontroller). For communication I would use a unsigned int pointer and simply iterate over the entire struct and not bother with splitting the 16 bit value. \$\endgroup\$ – JvO Jan 11 '18 at 10:01
  • \$\begingroup\$ Pure software questions like this are best asked at stackoverflow.com. Microcontroller programming is perfectly on-topic here on EE, but you'll generally get better answers over at SO. \$\endgroup\$ – Lundin Jan 12 '18 at 12:51
6
\$\begingroup\$

In order to integrate the union in the struct typedef, you need to use modern standard C. In the C11 version of the standard, anonymous unions/structs were added as a feature. With a standard C compiler you can do this:

typedef struct {
    unsigned char   SOAKTEMP;
    unsigned char   SOAKTIME;
    unsigned char   REFLOWTEMPERATURE;
    unsigned char   REFLOWTIME;
    unsigned char   BAKETEMP;

    union
    { 
      unsigned int  BAKETIME;
      struct
      {
        unsigned char BakeTimeLowbyte;
        unsigned char BaketimeHighbyte;
      };
    };

    unsigned char   COOLDOWNTEMPERATURE;
    unsigned char   COOLDOWNTIME;
    unsigned char   KP;
    unsigned char   KI;
    unsigned char   KD;
    unsigned char   CYCLETIME;    
}EEPROM_DEFAULTS;

You can now either use BAKETIME, BakeTimeLowbyte or BaketimeHighbyte as if they were 3 different struct members, even though they refer to the same 16 bit word.


However, this code is slightly problematic as you enforce the processor endianess upon your struct. If the code is only ever going to run on a Little Endian PIC, this is perhaps acceptable. But suppose you want to do the same thing with a struct that describes a data communication protocol. The sender, the network protocol and the receiver may all differ in endianess.

Therefore it would actually be better to keep the 16 bit integer from the initial code, then access individual bytes like this:

unsigned char hi = (unsigned char) (eeprom.BAKETIME >> 8);
unsigned char lo = (unsigned char) (eeprom.BAKETIME & 0xFF);

This code is endianess-independent and portable.


General good advice / code review:

  • This struct and the union may have padding bytes inserted anywhere inside. Padding is never an issue on 8 bit MCUs, but on most other systems. You might want to protect against unintended padding by checking that the struct size is correct, using a static_assert.
  • Avoid the default types of C, since they are non-portable and sometimes come with various nasty side effects such as implicit type promotion. Instead of unsigned char and unsigned int, use uint8_t and uint16_t from stdint.h
  • Remember that the variable instance of the EEPROM_DEFAULTS struct must be declared as volatile, since the EEPROM might be changed at any time.
\$\endgroup\$
  • \$\begingroup\$ To me, this is the stand out answer. Portability, good advice about using stdint.h and the code is much easier to read. +1 \$\endgroup\$ – DiBosco Jan 12 '18 at 13:02
  • \$\begingroup\$ A lot of usefull information in the comments and answers. All is clear appart from the use of the volatile declaration. I declared the instance volatile but the why is not really understood. In my program the contents of the EEPROM is to be changed only during the excecution of read and write functions. \$\endgroup\$ – Decapod Jan 12 '18 at 16:40
  • \$\begingroup\$ @Decapod volatile is there to avoid incorrect optimizations. If the compiler does not realize that the contents of the EEPROM may change at any time, it might decide to do all manner of weird optimizations, essentially treating what's stored there just as the read-only constants in flash. By declaring the instance volatile, the EEPROM rather gets treated by the compiler as if it was a collection of hardware registers, which is the correct behavior. \$\endgroup\$ – Lundin Jan 15 '18 at 7:37
5
\$\begingroup\$

You don't need to split anything. Just dump the structure to the EEPROM as it is.

Let EEPD be structure of EEPROM_DEFAULTS and assume eepromWrite8(uint8_t data,int index) is the writing function.

Then, it should be something like that:

eepromWrite8(*((uint8_t*)&EEPD)+index),index);

&EEPD is the address of EEPD, a pointer type of EEPD*.

It is casted into pointer type uint8_t*.

"+index" part is the pointer arithmetic which you calculate the offset to point (in the resolution of the type, that is why it is called pointer arithmetic)

Finally, you make an uint8_t from where the pointer points to. (The asterisk on the left)

\$\endgroup\$
  • \$\begingroup\$ I have changed the implementation according to your idea. And it functions perfectly well. Indeed no need to split anything at all. Just reading from or dumping into the EE prom with an iteration loop is enough. \$\endgroup\$ – Decapod Jan 11 '18 at 17:50
  • 2
    \$\begingroup\$ Alternatively, uint8_t* eeprom = (uint8_t*)&EEPD; eepromWrite8(&eeprom[index], something);. Or better yet, keep the struct. \$\endgroup\$ – Lundin Jan 12 '18 at 12:54
  • \$\begingroup\$ @Lundin I have tried to read from and write to the eeprom with the pointer arithmetic. At first impression I thought that it was working. However it is not. I understand the method and tried to extract an uint8_t value from the struct. No succes. Direct working with the struct and elements operates very well. I could leave it like that but I wonder what could go wrong. Could it be a limitation on the compiler? \$\endgroup\$ – Decapod Jan 16 '18 at 20:29
  • \$\begingroup\$ @Decapod, you should show the relevant parts of code. \$\endgroup\$ – Ayhan Jan 16 '18 at 21:39
  • \$\begingroup\$ @Decapod Struct padding can go wrong, although I don't see why a PIC compiler would introduce padding. It might be best to ask a new question regarding this (at stackoverflow.com) and include the code which isn't working. \$\endgroup\$ – Lundin Jan 17 '18 at 7:31
4
\$\begingroup\$

Unfortunately this is the only way of achieving what you want in C.

You can make the following simplification though.

typedef struct  {
    char lo, hi;
} BytePair;

typedef union {
    int intValue;
    BytePair bytes;
} Word;

// for example purposes / your large structure here
typedef struct {
    u8 a;
    u8 b;
    Word c;
} MyStruct;

I'm assuming an 'int' on your system is 16bits wide ?

\$\endgroup\$
  • \$\begingroup\$ I leave it the way it is then. The simplification is nice and clear but does not change the principle. \$\endgroup\$ – Decapod Jan 11 '18 at 9:56
  • 2
    \$\begingroup\$ I would add that the use of standard types in this case is ambiguous. I always try to explicitly convey the data size by use of uint8_t, uint16_t, uint32_t etc for this purpose. That way if you move to a different processor that has a different word size you code will still work as intended. \$\endgroup\$ – Luke Gary Jan 11 '18 at 11:08
  • \$\begingroup\$ This code isn't endian invariant. \$\endgroup\$ – Jeroen3 Jan 12 '18 at 12:51
  • \$\begingroup\$ "Unfortunately this is the only way of achieving what you want in C". This is wrong, see my answer. \$\endgroup\$ – Lundin Jan 12 '18 at 12:55
  • \$\begingroup\$ I should have qualified my answer with "This is the only way to build a structure with mixed structure/union members" as i wasn't attempting to answer the wider question WRT eepromWrite8(...). Good points on endianess. \$\endgroup\$ – PaulHK Jan 13 '18 at 4:44
1
\$\begingroup\$

I would argue that the method that is most clear is best, I see two ways you can do this. The first is the way you outlined, I would just use sized types such as uint8_t and uint16_t rather than unsigned int and char to explicitly show their sizes. The second way is just to mask off the individual bytes. This may require a little more typing but is also fairly clear in my mind. The idea is to bitwise and your data 8 bits at a time with 8 ones that are right justified (the low byte) then right shift the data 8 bits to move the 8 significant bits to the low position then repeat.

/* Helper Macros */
#define LOW_BYTE(data16)    ((uint8_t)(data & 0xFF))
#define HIGH_BYTE(data16)   ((uint8_t)((data>>8) & 0xFF) 
/* Actual data manipulation */
uint8_t bakeTimeHighByte = HIGH_BYTE(EEPD.BAKETIME)
uint8_t bakeTimeLowByte  = LOW_BYTE(EEPD.BAKETIME)

/* my test code */
void p(void){
    uint16_t data = 0x1234;
    printf("0x%04X\r\n", data);
    printf("Low Byte  : 0x%02X\r\n", LOW_BYTE(data));
    printf("High Byte : 0x%02X\r\n", HIGH_BYTE(data));
}

/* my output */
0x1234
Low Byte  : 0x34
High Byte : 0x12
\$\endgroup\$
  • 2
    \$\begingroup\$ Or just skip the icky macros. C programmers know C, they don't know "secret macro language x". printf("Low Byte : 0x%02X\r\n", (unsigned int)data & 0xFF); is much clearer code, and as a side effect we also got rid of the unwanted implicit promotion to int. \$\endgroup\$ – Lundin Jan 12 '18 at 12:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.