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));
}

This is a bit of a harsh response I'm getting here... not how I remember stackoverflow. I admit it was not all too specific. I've added some details. — Flying Swissman, Mar 16 '14 at 17:11
Well now your question's understandable at least. Do you really need macros though? Plain old arrays couldn't work? — Mat, Mar 16 '14 at 17:16
I want to do it with precompiler directives so its easily modify and as much happens at precompile time as possible — Flying Swissman, Mar 16 '14 at 17:19
but seeing the question was downvoted to -2 already I probably have no chance at getting a reasonable answer now... — Flying Swissman, Mar 16 '14 at 17:21
Thing is the pre-compiler doesn't know about your runtime `for` loop. It's only parsing the body once, so you'll get only one substitution. Either that involves an array index/pointer arithmetic or some other calculation based on `i`, or your idea won't work. Those things cannot be fixed at compile time with the pre-processor combined with a runtime `for` loop. There's tricks to do loops with the pre-processor, but that's arcane magic as far as I'm concerned. Try something simple first, optimize if it happens to be a bottleneck. — Mat, Mar 16 '14 at 17:33
I've added some more, so you see what I mean. I will come away from using macros like you suggest I think. — Flying Swissman, Mar 16 '14 at 17:35
I'm missing a definition of the LED_DD(x) macro your using in the first statement within the loop body? — mfro, Mar 16 '14 at 18:11

SzG · Accepted Answer · 2014-03-16T18:29:56.617

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.

edited Mar 16 '14 at 18:29

answered Mar 16 '14 at 17:23

SzG

12,333
4
28
41

tell me if point 1 & 2 still hold true for the changes I've added and I'll accept you're answer and be happy man – Flying Swissman Mar 16 '14 at 17:45
Yes, my 2 points are still valid after the changes. I'll explain in the answer why. – SzG Mar 16 '14 at 18:27
I had fergotten to add a few lines, now its complete, I would appreciate if you looked at it one more time. – Flying Swissman Mar 16 '14 at 19:45
It's getting worse. Now you have multiple macros with the same name. – SzG Mar 16 '14 at 20:19

Joseph Quinsey · Answer 2 · 2014-03-16T21:55:39.677

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.

score 0 · Answer 3 · answered Mar 16 '14 at 21:55

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.

C-Macros produces unexpected behavior

3 Answers3