C-Macros produces unexpected behavior

Question

I'm trying to make my header file easily changeable with macros. I'm debugging my code and it seems these MACROS are not doing what they are supposed to. Can someone tell me how I achieve the following effect? LED_ID_AMS etc.

#define LED_NUMBER                  (2)
#define LED_ID_X                    (0)
#define LED_ID_Y                    (1)
#define LED_PIN_X                   (0)
#define LED_PIN_Y                   (3)
#define LED_PORT_X                  (PORTE)
#define LED_PORT_Y                  (PORTG)
#define LED_DD_X                    (DDRE)
#define LED_DD_Y                    (DDRG)
#define LED_PORT(LED_ID_X)          (LED_PORT_X)
#define LED_PORT(LED_ID_Y)          (LED_PORT_Y)
#define LED_PIN(LED_ID_X)           (LED_PIN_X)
#define LED_PIN(LED_ID_Y)           (LED_PIN_Y)
#define LED_DD(LED_ID_X)            (LED_DD_X)
#define LED_DD(LED_ID_Y)            (LED_DD_Y)

What am I trying to achieve?

I'm trying to make it so I can loop through the port init like so:

for(i=0;i<LED_NUMBER;i++){
    /* set data direction to output*/
    LED_DD(i)|=((0x01)<<LED_PIN(i));
    /* turn on led */
    LED_PORT(i)|=((0x01)<<LED_PIN(i));
}

Answer 1

You will regret using too many macros later. Actually, you're regretting it already, as they don't work and, being macros, they are very difficult to debug.

Just a few points:

• your LED_PIN(i) expressions are always expanding to 0
• your LED_PORT(i) expressions are always expanding to PORTE whatever that may be

For instance LED_PIN(LED_ID_X) expands to LED_PIN_X. Note, macro parameter LED_ID_X is not used at all. Instead, LED_PIN_X simply expands to 0.

Answer 2

This should scream warnings at you, as e.g. LED_PORT(SOME_ARG) has several definitions. And in LED_PORT(LED_ID_X) the LED_ID_X is just a dummy argument, with absolutely no relation to your constant LED_ID_X.

You can make your code equally readable by using a constant array, perhaps used from macros like you try to do here.

Unless there are a massive number of LED_ID_<foo>, this is at best a minor simplification. Don't do that. If there is a lot of code futzing around with those is mostly the same way, it might make sense to define a macro that iterates some action over each of them, i.e.:

#define FROB_LEDS \\
     action(LED_ID_X); \\
     action(LED_ID_Y); \\
     action(LED_ID_Z);

and define action(X) locally as a macro to do the action on LED X, FROB them, and undefine action again. Quite ugly, true.

Answer 3

You'll have to add at least one of:

• arrays
• inline functions
• more complicated macros

And it also seems to me that dereferencing of hardware addresses will be required.

For example, using macros, you can define:

#define LED_PORT(i) *(uint16_t *)( \
    (i) == LED_ID_X ? LED_PORT_X : \
    (i) == LED_ID_Y ? LED_PORT_Y : \
    etc)

where:

#define LED_ID_X     (0)
#define LED_ID_Y     (1)
#define LED_PORT_X   (PORTE)
#define LED_PORT_Y   (PORTG)
#define PORTE        (0x11112222U) // example only
#define PORTG        (0x33334444U) // example only

Here uint16_t is only a guess: I'm assuming 16-bit ports in a 32-bit address space.

Or, using arrays and C99's designated initializers:

const uint32_t LED_PORT[] = {
    [LED_ID_X] = LED_PORT_X,
    [LED_ID_Y] = LED_PORT_Y
};

#define LED_PORT(i) (*(uint16_t *)LED_PORT[i])

And of course, without C99 you can use just:

const uint32_t LED_PORT[] = {LED_PORT_X, LED_PORT_Y};

which assumes that LED_ID_X is 0, etc.