3

I'm developing code for Atmel, which defines consistent register names to program port pins. For instance:

  • PORTC is traditionally used to set (high or low) any pin on port C
  • PINC is used to read the state of a particular pin on port C
  • DDRC is used to read/write the direction (0=input, 1=output) of any pin on port C

So I found some code — which I fully understand — that defines a PIN macro like this:

#define LED_PIN    C,7

Then the following macros (restricted to the use case I'm talking about here):

#define _BV(bit)                        (1 << bit)
...
#define _SET(type, name, bit)           type ## name |= _BV(bit)
#define _CLEAR(type, name, bit)         type ## name &= ~ _BV(bit)
#define _TEST(type, name, bit)          ( type ## name  & _BV(bit) )
...
#define OUTPUT(pin)         _SET(DDR, pin)
#define INPUT(pin)          _CLEAR(DDR, pin)
...
#define HIGH(pin)           _SET(PORT, pin)
#define LOW(pin)            _CLEAR(PORT, pin)

So I can write a function main() as follows:

main() {
  OUTPUT(LED_PIN);
  HIGH(LED_PIN);
}

While it is very convenient and prevents from defining three macros (e.g. PINC7, PORTC7 and DDRC7) for just one LED, my main concern about this is that is imposes to also redefine AVR macros to account for that kind of notation, bearing in mind AVR macros use SFR registers as arguments (from sfr_defs.h):

#define bit_is_set(sfr, bit) (_SFR_BYTE(sfr) & _BV(bit))

Also, AVR defines macros to convert from [what I'd call] bare registry names to actual memory-mapped special-function register names (sfr) for avr-gcc:

#define _SFR_BYTE(sfr) _MMIO_BYTE(_SFR_ADDR(sfr))

and _SFR_ADDR() adds an offset 0 or 20 according to the target Atmel CPU. So in order to enhance compatibility with those AVR library functions, which take an SFR register plus an optional argument, I decided to rewrite the initial code and tried the following:

#define _SFR_BIT(type, name, bit)   type ## name, bit
#define _PORT(pin)                  _SFR_BIT(PORT, pin)
#define _PIN(pin)                   _SFR_BIT(PIN, pin)
#define _DDR(pin)                   _SFR_BIT(DDR, pin)

#define set_bit(sfr, bit)           _SFR_BYTE(sfr) |= _BV(bit)
#define set_output(pin)             set_bit(_PIN(pin))

but then I face a compiler error message as soon as I write something like this in function main():

set_output(LED_PIN);

main.cpp:72:19: error: macro "set_bit" requires 2 arguments, but only 1 given

I get the same error if I try this, too:

#define set_output(pin)             set_bit(_SFR_BIT(DDR, pin))

Why is that macro OUTPUT(), which passes only two out of of three arguments to _SET() compiles fine and my macro doesn't?

As Jens Gustedt pointed to, the generic explanation lies in the order of which the compiler resolves macros.

To transform an arbitrary number of arguments to be passed to a function with a fixed number of arguments using a macro, the function name can be made an argument to the macro:

#define EXPAND(f, ...)              f(__VA_ARGS__)
#define _SFR_BIT(type, name, bit)   type ## name, bit
...
#define set_bit(sfr, bit)           _SFR_BYTE(sfr) |= _BV(bit)
...
#define set_output(pin)             EXPAND(set_bit, _SFR_BIT(PIN, pin))

How many and which arguments are passed to the function (first argument) is resolved without any "argument missing" compiler error.

A refined version:

#define _SFR_X(f, type, name, bit)  f(type ## name, bit)
...
#define set_bit(sfr, bit)           _SFR_BYTE(sfr) |= _BV(bit)
...
#define set_output(pin)             _SFR_X(set_bit, PIN, pin)

DISCLAIMER

The code is not yet in production. The code may be ugly, of course. It's mostly some base work under construction. Hence not yet finalized.

Community
  • 1
  • 1
  • Names starting with underscores are reserved for the implementation. Don't use them in application code. – too honest for this site Aug 08 '16 at 12:39
  • @Olaf Are you talking about _BV() ? –  Aug 08 '16 at 12:41
  • 1
    Is that the only name starting with an underscore?? What in myy comment is unclear? Oh, and your code can invoke undefined behaviour, depending on the shift count. In general shifting signed integers an arbitrary number of bits should be avoided. Use unsigned integers and fixed-width types! – too honest for this site Aug 08 '16 at 12:50
  • As a sidenote: your code is confusing, thus badly to maintain. Dont get too fancy with macros! – too honest for this site Aug 08 '16 at 12:53
  • @Olaf ... No, of course no [it's not the only name ...] I assumed you knew where those underscores came from since I didn't write *that* part of the code rather than copy it as is from somewhere else, *which is one of the reason I wanted to rewrite it*! So only _BV() made sense as I thought you meant there actually *is* something better than _BV(), which is an Atmel macro. –  Aug 08 '16 at 12:54
  • 1
    And by the way, what I want here is *understand* why the code doesn't work. Let's talk about refinement later on, shall we? Eliminating non-working code won't tell me why it doesn't work anyway, so... –  Aug 08 '16 at 12:55
  • Maybe, because `DDR` expands to a `type, name` tuple, and the next macro evaluation of `_SET` sees them as the first and second parameters? – Koshinae Aug 08 '16 at 12:56
  • "My code doesn't work" is not a **specific** error description. See [ask]. Nor is "explain me that code I found somewhere" welcome. Don't just use code you don't understand. (And definitively some code from the internet does not define what it allowed in C and what not). – too honest for this site Aug 08 '16 at 12:58
  • 1
    @Olaf: can you **please** give time instead of jumping on your high horse? In case you don't know: I'm busy, working, typing a question, answering comments and yet have other work to do in spite of addressing your... requirements. I *will* reword my question but don't give a damn if I'm not prompt enough for you — especially given the fact that someone *already* gave me the appropriate answer. And you're off: it's not *all* the code that I found on the internet. Only the part I wrote to rewrite it is in question! –  Aug 08 '16 at 13:22
  • 1
    Well, as I'm busy, too, I don't intend to do **your** job (which includes writing maintainable code). It is **your** project after all - unless you'd work for me, in that case I'd check your whole project first and - depneding on that analysis - we might have a talk (well, we had definitively for the `_` names already). – too honest for this site Aug 08 '16 at 13:37

1 Answers1

3

The decomposition of the arguments for set_bit into several takes place before the _SFR_BIT macro is expandend. So when it then sees the comma that resulted from the expansion of the latter, it is already too late.

A common trick is to expand the argument once more

#define set_bit0(...)   set_bit(__VA_ARGS__)
#define set_output(pin) set_bit0(_PIN(pin))

Here _PIN(pin) is expanded and passt to set_bit0. When passing the arguments from set_bit0 to set_bit it sees the already expandend sequence, including the comma.

Jens Gustedt
  • 76,821
  • 6
  • 102
  • 177
  • @Nasha: Simple solution: don't do it. sometimes it is better just to accept typing a bit more, but have the code readable. – too honest for this site Aug 08 '16 at 12:55
  • 1
    @Olaf: sorry. "*don't do it*" is no *solution* nor a valid explanation for "*why doesn't it work?*". See my previous comments. –  Aug 08 '16 at 13:01
  • @Nasha: See my comments to your question. "why does the code not work" is not a valid question here without further details. Nor is "explain that code". And a comment is not to provide a solution, but to provide/ask for more information. If you don't like well meant information, just write a complete question and add that. But don't wonder if your code messes up eventually. – too honest for this site Aug 08 '16 at 13:01
  • 1
    @Olaf: give me a break, will you? –  Aug 08 '16 at 13:02