1
\$\begingroup\$

I am trying to set multiple values into a 32 bit register, and each value has 2 bits. Trying to come up with an elegant way to do this.

Write now I have something like this:

struct MuxRegister {
Uint16 PIN1:2;
Uint16 PIN2:2;
...
}

#define LED PIN1      (in a different file of course)
#define PWM      PIN2
#define Mux_GPIO 1         (value from 0-3 selects different options)
#define Mux_PWM  3
MuxRegister.LED = Mux_GPIO;
MuxRegister.PWM = Mux_PWM;
...

And I want to do something where I don't have to go through each MuxRegister.X and MuxRegister.Y for each of the 16 pins in the structure.

My idea was to do something like this:

#define BIT1 0x00000001
#define BIT2 0x00000002
#define BIT1 0x00000004
#define BIT2 0x00000008
#define LED (BIT1&&BIT2)
#define PWM (BIT3&&BIT4)
MuxRegister |= ((Mux_GPIO && LED) || (Mux_PWM && PWM) || ... ; 

But there is no mechanism to preserve the spacing for the Mux code (values between 0 and 3) register beyond the very first position. As in, the value for Mux_PWM in this example is 0x00000003 but the value for PWM is 0x0000000C, and they don't interact at all since they aren't "overlapping".

Is there a mechanism with which to space out the values of the Mux codes so that they interact with the correct pin values? Sorry if I'm not explaining the question well. I don't have a lot of the correct terminology.

\$\endgroup\$
  • \$\begingroup\$ do you mean that you're wanting to use 0b00000000000000000000000000000011 instead of 0x00000003? If you wan to do that an ezsier way, you could always use LSL and LSR (<< & >>) to bit-shift your values you're putting in/reading \$\endgroup\$ – Robherc KV5ROB Feb 14 '16 at 1:51
  • \$\begingroup\$ The && operator is not the & operator. I think you need to study the basics of both the bitwise and the logical operators before you do anything else. \$\endgroup\$ – Lundin Feb 17 '16 at 10:53
1
\$\begingroup\$

I would try just using a uint32_t instead of bit fields. Here's what I would probably do:

typedef enum
{
    LED,
    PWM,
    ...
} MUX_E;

uint32_t MuxRegister;

void SetMuxOption (uint8_t const option, MUX_E const bits)
{
    MuxRegister &= ~(uint32_t) (3 << 2*bits);
    MuxRegister |= (uint32_t) (option << 2*bits);
}
\$\endgroup\$
  • \$\begingroup\$ Your SetMuxBits() won't work if the MSB in the two-bit field is already set. \$\endgroup\$ – Adam Haun Feb 14 '16 at 3:30
  • \$\begingroup\$ How? Each value has 2 bits with 01 being on and 00 being off. There is no way to set the MSB of each two-bit field this way. If the MSB is needed, then this won't work. That's how I interpreted the question, anyway. \$\endgroup\$ – Swarles Barkely Feb 14 '16 at 3:42
  • \$\begingroup\$ His value for Mux_PWM is 3. It's definitely possible for the MSB to be set. \$\endgroup\$ – Adam Haun Feb 14 '16 at 3:44
  • \$\begingroup\$ Ah missed that. Answer updated. \$\endgroup\$ – Swarles Barkely Feb 14 '16 at 3:50
0
\$\begingroup\$

There isn't a super-concise, elegant way of doing this that I know of. If you want to set multiple bits, you need, at minimum, the following information for each field:

  1. The bit position (shift)
  2. The number of bits in the field (mask)
  3. The data that goes in the field

It looks like you also want to give functional names to the pins and the mux values. That's a lot of information.

Honestly, I think your first code snippet is a good start. The bit field structure can encode the shifts, masks, and pin names in the structure type, which makes the code reasonably concise. You could save a step by naming the bit fields LED, PWM, etc. instead of PIN1, PIN2, etc.

Alternately, you could shrink the code size by making a constant array of mux values, then loading them via a for loop:

//                          P1: LED   P2: PWM   P3: ADC   P4: SW
const Uint32 muxValues[] = {MUX_GPIO, MUX_PWM,  MUX_ADC,  MUX_GPIO, ... //12 more

MuxRegister = 0x00000000;
for (pin = 0; pin < sizeof(muxValues); pin++)
{
    MuxRegister |= muxValues[pin] << 2*pin;
}

Or you could do something like:

MuxRegister = (MUX_LED << 0) | (MUX_PWM << 2) | (MUX_ADC << 4) | (MUX_GPIO << 6) | ...

You can get fancy with macros to improve readability:

#define PIN(p)  (2 * (p - 1))
MuxRegister = (MUX_LED << PIN(1)) | (MUX_PWM << PIN(2)) | (MUX_ADC << PIN(3)) | (MUX_GPIO << PIN(4)) | ...

But I don't think you're going to find something massively shorter. You have sixteen unique mux selections, and one way or another, they all have to go in your code. If you want unique pin names, that's another sixteen values.

\$\endgroup\$
0
\$\begingroup\$

Unions can be your friend:

typedef union {
    struct {
        Uint32 PIN1:2;
        Uint32 PIN2:2;
        ...
    } bits ;
    Uint32 reg;
} MuxRegister;

Now you can access it like:

MuxRegister muxReg;
...
muxReg.reg = 1234; //As an integer
muxReg.bits.PIN1 = 1; //As bits.

You will note that I made the bit-field entries Uint32 because you are using 32bit registers (not sure why you made them Uint16?). You should also consider using standard types from <stdint.h> such as uint32_t for better portability.

Depending on your compiler and whether it allows anonymous structures, you may be able to do:

typedef union {
    struct {
        Uint32 PIN1:2;
        Uint32 PIN2:2;
        ...
    };
    Uint32 reg;
} MuxRegister;

Which would be accessed as:

MuxRegister muxReg;
...
muxReg.reg = 1234; //As an integer
muxReg.PIN1 = 1; //As bits.

Sometimes unions can be a problem, but most of the time they work fine. Most of the time the issue will be due to alignment of the words, but this is quite easy to check. This really useful bit of code allows for compile time checking that a union is the expected size - usually if alignment goes out of whack the size will change.

#define ASSERT_CONCAT_(a, b) a##b
#define ASSERT_CONCAT(a, b) ASSERT_CONCAT_(a, b)
#define ct_assert(a,e) enum { ASSERT_CONCAT(assert_sizeof_, a) = 1/(!!(sizeof(a) == e)) }

Then you can do:

ct_assert(MuxRegister,sizeof(Uint32));

Which will flag up an error if the union is not a 32bit word.


Also, as a side note, be careful with your operators.

MuxRegister |= ((Mux_GPIO && LED) || (Mux_PWM && PWM) || ... ; 

That would do the bitwise or of MuxRegister with boolean true or false. Why? because you are using logical operators. (Mux_GPIO && LED) will result in either true if both values are non-zero or false otherwise.

I think what you meant by that line is:

MuxRegister |= ((Mux_GPIO & LED) | (Mux_PWM & PWM) | ... ; 

This uses bitwise operators.

\$\endgroup\$
  • 1
    \$\begingroup\$ Illegal use of a union, and bitfields are by definition unpredictable, so very bad to use for something like this. Generally this approach just works, and then when it doesnt, it breaks dramatically. There is no reason whatsoever for the compiler to support this. \$\endgroup\$ – old_timer Feb 14 '16 at 6:46
  • \$\begingroup\$ @dwelch I'm used to the syntax with typedefs so it is entirely possible that the syntax was wrong. Either way I've updated it to a form which I know is correct. \$\endgroup\$ – Tom Carpenter Feb 14 '16 at 15:09
  • \$\begingroup\$ +1 as situations like this rarely need to be portable and the technique is useful; also shows the power of unions. With no portability requirement, compiler limitations are not as important. Although the update is widely portable, embedded code does not often require it. \$\endgroup\$ – Peter Smith Feb 14 '16 at 18:50
  • \$\begingroup\$ if you have multiple things in a union you cannot legally cross from one to the other, or set one to see it reflected in the other. usually works, but the compiler doesnt have to make it work. bitfields, have little to no value. lastly never ever use a struct across compile domains (hardware is a separate compile domain). any one of these things taken by itself is a bad habit that WILL bite you some day, combine all of them and when any one does, the pain will be severe, if you do any of these as a habit then you will have a lot of code to repair. \$\endgroup\$ – old_timer Feb 15 '16 at 5:24
  • \$\begingroup\$ the union, bitfields, pointing structures at things, is only generating read-modify writes with shifts and masks and or bit set/clear instrucitons if the instruction set supports them. just write that stuff directly in your code, it is portable, wont bite you in the end and the compiler can find the bit set/clear if you have them and optimize that. is not a bad habit and wont bite you badly some day and cause a major re-write of your code. for many it takes 10-20 years to get bitten as you will tend to stay in one field or set of tools for a while. \$\endgroup\$ – old_timer Feb 15 '16 at 5:27

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.