Using Structures and Unions to Access SFR bit fields

rfengr said:

unsigned char                : 2;

rfengr said:

1) expected a field name

rfengr said:

2) Identifier MY_IE1 is undefined

Also, since you defined every field as a char, it will occupy a complete byte each, with only one (and one times 2) bits used.

The MSPGCC header file use the following construct for what you want:

typedef struct {
  volatile unsigned
    adc12burst:1,
    adc12refout:1,
    adc12sr:1,
    adc12df:1,
    adc12res:2,
    res1:1,
    adc12tcoff:1,
    adc12pdiv:1,
    res2:7;
} __attribute__ ((packed)) adc12ctl2_t;

This way, all bits are put into the same unsigned int. Notice that al 16 bits are defined, even if reserved. I'm not sure how to apply the packed attribute on IAR/CCS. If the struct isn't packed, it will still put each field into a separate char/int.

Thank for the reply,

I looked through "The ANSI C Programming Language, 2nd Ed." and I found out that bit-fields don't need names, as the case with your example.

"Almost everything about fields is implementation-dependent. Whether a field may overlap a word boundary is implementation-defined. Fields need not be names; unnamed fields (a colon and width only) are used for padding. The special width 0 may be used to force alignment at the next word boundary."

Also, since the main datastructure is a union, and since we define the width of each bit-filed using the colon character ":", there is no need for (packed). Note: IE1 is unsigned char (8 bits) and the bit-fields added up are less than 8 bits, so they are "packed", per say, in the same byte as IE1. 

In other words, the compiler still throws the same errors...

I think my answer lies somewhere in the linker.

Any linker experts out there ???

rfengr said:

Almost everything about fields is implementation-dependent

rfengr said:

Fields need not be names; unnamed fields (a colon and width only) are used for padding

About the packed thing, well, when listing the bits in the field as a comma separated list, I'd expect them packed already (yet the attribute is still added in the mspgcc headers), but you defined the bits as separate char fields of limited length, which may or may not lead to packed bits, depending on the implementation.

In any case, the 'field name expected' error for the use of @0x1000 is really annoying. I'd expect something like "illegal character before ';'" or something like that.
In mspgcc, you use " asm{"0x100"}" to assign a fixed address. (it fits nicely in the inline assembly support, otherwise the use of @ is nice too). I knew that in non-mspgcc projects this notation is used but I didn't know that this is IAR only.

IMHO it is a very BAD approach to have the user meddling with the linker scripts for this under CCS.

Please could  you help me with this code below my C coe is rusty... it appears related topic.. so I'll post it here.  I using the MSP430 Launch Pad MSP430G2231 along with CCS v4 to directly access the PORT1 bits directly instead of bitwise AND's, OR's.  I tried using a similar construct as above, but ran into some errors

#include  <MSP430G2231.h>

extern union 

{ unsigned char P1IN_val;

struct 

{

unsigned const char P1IN_0  : 1;

unsigned const char P1IN_1  : 1; 

     unsigned const char P1IN_2  : 1; 

     unsigned const char P1IN_3  : 1; 

     unsigned const char P1IN_4  : 1; 

     unsigned const char P1IN_5  : 1; 

     unsigned const char P1IN_6  : 1; 

     unsigned const char P1IN_7  : 1; 

   }P1IN_bit;

 }P1IN_byte;

extern union

 { unsigned char P1OUT_val;

  struct 

   {

     unsigned char P1OUT_0        : 1; 

     unsigned char P1OUT_1        : 1; 

     unsigned char P1OUT_2        : 1; 

     unsigned char P1OUT_3        : 1; 

     unsigned char P1OUT_4        : 1; 

     unsigned char P1OUT_5        : 1; 

   unsigned char P1OUT_6        : 1; 

     unsigned char P1OUT_7        : 1; 

   }P1OUT_bit;

 }P1OUT_byte;

enum

{

  P1OUT_0             = 0x01,

  P1OUT_1             = 0x02,

  P1OUT_2             = 0x04,

  P1OUT_3             = 0x08,

  P1OUT_4             = 0x10,

  P1OUT_5             = 0x20,

  P1OUT_6             = 0x40,

  P1OUT_7             = 0x80

};

extern union

{ unsigned char P1DIR_val;

struct 

   {

     unsigned char P1DIR_0        : 1; 

     unsigned char P1DIR_1        : 1; 

     unsigned char P1DIR_2        : 1; 

     unsigned char P1DIR_3        : 1; 

     unsigned char P1DIR_4        : 1; 

     unsigned char P1DIR_5        : 1; 

     unsigned char P1DIR_6        : 1; 

     unsigned char P1DIR_7        : 1; 

   }P1DIR_bit;

}P1DIR_byte;

enum

{

  P1DIR_0             = 0x01,

  P1DIR_1             = 0x02,

  P1DIR_2             = 0x04,

  P1DIR_3             = 0x08,

  P1DIR_4             = 0x10,

  P1DIR_5             = 0x20,

  P1DIR_6             = 0x40,

  P1DIR_7             = 0x80

};  

 //#define RLED BIT0 // Port 1, Pin 0 (P1.0) Hex 0x0001 [MSB...LSB]

#define LED1    P1OUT_byte.P1OUT_bit.P1OUT_0

//#define GLED BIT6

#define LED2    P1OUT_byte.P1OUT_bit.P1OUT_6 // Port 1, Pin 0 (P1.6) Hex 0x0040 [MSB...LSB]

//#define PB_S2   0x08 // Port 1, Pin 3 (P1.3) Hex 0x0008 [MSB...LSB]

#define PB_S2   P1IN_byte.P1IN_bit.P1IN_3  

void main(void)

{

  WDTCTL = WDTPW + WDTHOLD;                 // Stop watchdog timer

  //P1OUT &= ~0x41; // Set Port 1, Pin 0 (P1.0 output to active low, OFF!)

  LED1 = 0;

  LED2 = 0; 

  //P1DIR |= 0x41;                          // Set Port 1, Pin 0 and PIN 6 (P1.0 and .6) to output direction

  P1DIR_byte.P1DIR_bit.P1DIR_0 = 1;

  P1DIR_byte.P1DIR_bit.P1DIR_6 = 1;

  for (;;) { // Run Loop Forever

 //    while ((P1IN & PB_S2) != 0) { // Loop while button is UP

      while (PB_S2 != 0) {            // Loop while button is UP

    } // Active Low, do nothing.

 // When the button is pressed...

    //P1OUT |= GLED;                          // Turn on RED LED P1.0 using bitwise AND

    //P1OUT |= RLED;

    LED1 = 1;

    LED2 = 1;

 // while ((P1IN & PB_S2) == 0) {        // Loop while the button is down

    while (PB_S2 == 0) {

    }

// When the button is NOT pressed...

// P1OUT &= ~GLED; // Toggle P1.6 using bitwise OR                              // Delay

LED1 = 0;

// P1OUT &= ~RLED;

LED2 = 0; 

  }

}

Ed OReilly said:

I tried using a similar construct as above, but ran into some errors

Did you get compiler errors, linker errors or does the code just not run as expected?

And what are these enums for? It makes no sense to just define the eight bits over and over again or each port register. Just use BIT0..BIT7 for that. The only situation where separate enums are useful is when using them as type for type safety. Which makes not much sense for port bits. And of the enums are not used as types.

Also, well, I guess that just defining the unions does nto assign them to any specific memory location (especially not to the memory location of the port registers). They are just ram variables. And since they are extern, tehy are not allocated at all. You'll need to somehow generate a reference to the registers with these names.
This cannot be done directly in C, as C does not know about specific locations except if used through a pointer.
Whyt you basically need is something like &P1DIR_Byte = 0xYYYY, which is of course invalid ince references are no LVALUES and cannot be assigned a value.

It is also possible to make P1DIR_byte a type. Then generate

const P1DIR_byte * _P1DIR = 0xYYYY;

Then you can do *_P1DIR.P1DIR_bit.P1DIR_0 = 1;

In this case I suggest just generating a generic type PORT and use generic names like val or bit.BITx. You don't need the register name 10 times in the resulting member reference.

The most notable CCS errors were...

incompatible declarations at the end of the struct statement.  after some modifications, it would compile, however, the program did not work as expected.  Looking at the debugger view, it seems your were right the values did not amount anything.  as for the enum's, I copied this from the AIR io430.h  header file...bad idea.

 Nevertheless, my initial idea was to create a means to read or write  Port1 BITS directly without using bitwise mask.  For example, if i want to read the status of PORT1 BIT0, Why not just declare a set of variables for all the port bits ,

i.e. "#define Switch1 Port1.Bit0" , then use it in the code as ...while ( SWITCH1 == 1) ...then do something..

Do you have a code example I could use?

Thanks for the quick feedback.  Best Regards.

Hi rfengr,

why that complicated? The bit-definitions are all there (in the CCS header files), so if you want to set/clear a bit why not do it, i.e.:

P2OUT |= BIT3       // set BIT3 of P2OUT to 1
P2OUT &= ~BIT3      // clear BIT3 pr P2OUT to 2
P2OUT ^= BIT3       // toggles P2.3 

That also works for all other registers; you can use the Bit-names as per data sheet/users manual.

Be shure to enable code optimazitaion to get bit-set and bit clear-instructions out of this. In addition, you maybe would like to update CCS to the latest release.

By the way, I like using unions and structures too since the can ease one's life, BUT they also can be real pain too!

Rgds
aBUGSworstnightmare

Hi,
for testing bits you can:

1.) define a symbol i.e. like:
#define ENC_A             (P1IN & BIT6)   

2.) use the symbol i.e. like:
    if (ENC_A)
    {
        GrayCode |= 0x04;    // set bit 2 if ENC_A is high
    }

Rgds
aBUGSworstnightmare

Ed OReilly said:

my initial idea was to create a means to read or write  Port1 BITS directly without using bitwise mask

For your project convenience, just generate proper defines as already proposed:
#define CHIPSELECT (P1IN & BIT1)
if(CHIPSELECT){};

Especially for the port pins, doing a proper define can be more convenient than using a bitfield:
#define CS_TRIGGER {P1OUT&=~BIT1;P1OUT|=BIT1;}
CS_TRIGGER; // clear and set CS (add NOPs in the define if you're too fast)

You can even scale this by defining CS_PORT and CS_PIN and in your project setup you just have to change CS_PORT and CS_PIN when the hardware binding changes, rather than having to scan the whole code for the changed port and port bit.
And when you work with dynamically associated resources (e.g. a HAL), then the bitfields are completely useless.

Another one: to change a multibit-setting, you can of course generate e.g. a 3-bit field register.option and just set it. Here the bitfield seems to be useful. Yet for all such options exist defines such as UCS_SMCLK/UCS_ACLK. Since these already containtain the final bit values, thy cannot be uised for bitfields. Setting a value to such a multibit value, however, requires the compiler to load the bitfield, clear all bits of the value, then set the resulting bits and write it back. Which requires a register and 4 instruction (read/and/or/write). And if you're unlucky, the proper bit positions must be calculated first too (if the compiler cannot determine them at compile time). A
#define SET_CLOCK(x) CONTROL=(CONTROL&CLOCK_BITS)|x;
should generate the same instructions, looks better in the code and allows for cross-platform compatibility of the source.

Ed OReilly said:

as for the enum's, I copied this from the AIR io430.h  header