0

I'm using the following macro function

#define setAsOutput(bit, i)   { *bit ## _DDR[i] |= (1 << bit[i]); }

to simplify defining and setting some register values

// registers
volatile uint8_t *FOO_DDR[] = {&DDRA, &DDRA};
uint8_t FOO[] = {PA1, PA2};

setAsOutput(FOO, 0);

// these are defined somewhere else
#define PA1     1
#define PA2     2
#define DDRA    _SFR_IO8(0X01)

This gives code equivalent to

DDRA |= (1 << PA1);

However the line volatile uint8_t *FOO_DDR[] = {&DDRA, &DDRA}; is effectively redundant as the A in DDRA is always repeated in the FOO values, i.e. PA1 and PA2.

Ideally it could be removed completely and the macro changed to something like

#define setAsOutput(bit, i)   { DDR ## <second char of bit[i]> |= (1 << bit[i]); }

but getting the second character of the name of bit[i] doesn't seem possible.

Is there a way to rewrite the macro function so that FOO_DDR doesn't need to be explicitly defined and can be instead implied from {PA1, PA2}?

101
  • 8,514
  • 6
  • 43
  • 69
  • Macros can't access array elements, macros are expanded before processing C code. – Barmar May 26 '18 at 21:39
  • @Barmar so at a minimum I would always need define the values `A` and `1` to generate both `DDRA` and `PA1`? Note that it's always `DDRx` and `Pxy`. – 101 May 26 '18 at 21:42

3 Answers3

1

It would generally help if you could provide an MCVE so others can easily compile your code, see how it works, and try tweaking it.

You should not need to define things like DDRA and PA1 in your code. Just pass the appropriate option to the compiler to specify what AVR you are using (e.g. -mmcu=atmega1284p) and then add #include <avr/io.h> at the top of your program to get those definitions. And there is usually not much point in copying those definitions from io.h into questions on StackOverflow since they are pretty standard. Those definitions come from avr-libc, so if you really want to provide those details, you could just say what version of avr-libc you are using.

One of the main premises of your question is that the code you posted with arrays and macros is equivalent to DDRA |= (1 << PA1);. Unfortunately, that premise is incorrect. When GCC sees DDRA |= (1 << PA1);, it can actually compile that down to a single, atomic AVR instruction that sets bit 1 of the DDRA register. When GCC sees your code, it does something much more complicated that ends up reading, writing, and modifying the register. So the array code wastes CPU cycles and is not safe to use if interrupts might modify the DDRA register.

If you don't believe me, you can take a look at this godbolt.org link which compares the assembly for the two methods:

https://godbolt.org/g/jddzpK

It looks like you can actually fix this problem by just adding const qualifiers to your arrays. Then the compiler will know what values your arrays are holding at compile time and can generate good code.

volatile uint8_t * const FOO_DDR[] = {&DDRA, &DDRA};
uint8_t const FOO[] = {PA1, PA2};

Now on to your main question, which is how to get rid of redundant arrays. I don't think there is an easy way to do this, and having two const arrays in your program is not a big deal and they will probably be optimized away at compile time anyway. What you could do is expand those arrays so that they contain entries for every pin on your chip. Then when you want to write to a pin, you would simply use a pin number which is an index into the arrays (instead of needing to define new arrays). The vast majority of your code would then deal with these pin numbers and not worry about the arrays. So here is how I would write it:

#include <avr/io.h>

// Here is a general GPIO library for your chip.
// TODO: expand these arrays to cover every pin on the chip

#define setAsOutput(i)   { *pin_dir[i] |= (1 << pin_bit[i]); }
#define setHigh(i)   { *pin_value[i] |= (1 << pin_bit[i]); }

static volatile uint8_t * const pin_dir[] = {
  &DDRA,  // Pin 0
  &DDRA,  // Pin 1
};

static volatile uint8_t * const pin_value[] = {
  &PORTA,  // Pin 0
  &PORTA,  // Pin 1
};

static const uint8_t pin_bit[] = {
  PA1,    // Pin 0
  PA2,    // Pin 1
};

// Pin definitions for your particular project.
// (e.g. pin 0 is connected to a green LED)

#define GREEN_LED_PIN 0

void nice()
{
  setAsOutput(GREEN_LED_PIN);
  setHigh(GREEN_LED_PIN);
}

Each GPIO function call above ultimately compiles down into one assembly instruction.

If you dig around in the Arduino Core code, you'll find arrays just like this. (But the Arduino people make the mistake of accessing those arrays in a wasteful way in their pinMode and digitalWrite functions.)

Note that with the code I provided above, there is a big risk that you will accidentally pass a pin number that is not a compile-time constant, and therefore the compiler will not be able to optimize it, and produce wasteful/unsafe code. That's one reason why it would be better to use inline assembly and C++ templates like the FastGPIO library does.

101
  • 8,514
  • 6
  • 43
  • 69
David Grayson
  • 84,103
  • 24
  • 152
  • 189
1

If you want the code to be equivalent to DDRA |= (1 << PA1); - i.e. the simplest instruction that made at compile time without reading/writing arrays and pointers to IO registers. You may do it like this.

1) Let's assume we have definded somewhere (e.g. thru <avr/io.h>)

#define PA1     1
#define PA2     2
...
#define DDRA    _SFR_IO8(0X01)
#define PB1     1
#define PB2     2
...
#define DDRB    _SFR_IO8(0X01)

2) You want to have some kind of declaration like this:

#define BIG_RED_LED PA1
#define SMALL_GREEN_LED PB2

for then just to use them like

setAsOutput(BIG_RED_LED);
setAsOutput(SMALL_GREEN_LED);
setLow(BIG_RED_LED);
setHigh(SMALL_GREEN_LED);

etc., where each line is a simple write to a BIT in corresponding IO register.

To achieve that you can define tons of

#define DDR_PA0 DDRA
#define PORT_PA0 PORTA
#define PIN_PA0 PINA
#define DDR_PA1 DDRA
#define PORT_PA1 PORTA
#define PIN_PA1 PINA
...
#define DDR_PB0 DDRB
#define PORT_PB0 PORTB
#define PIN_PB0 PINB
...

and then

#define setAsOutput(px)   { DDR_ ## px |= (1 << px); }
#define setHigh(px)   { PORT_ ## px |= (1 << px); }
#define setLow(px)   { PORT_ ## px &= ~(1 << px); }
etc.

then, each time in your code happened something like setAsOutput(PA1) it will be compiled exactly the same as DDRA |= (1 << PA1);

But if you want to store them in the array and access by array index, as it is in your example, then you have no other way except as defining two arrays, or array of structs, where both elements will contain the bit number, or bit mask, and the pointer to the IO/register. Since, although name PA1 PA2 etc. have A letter in it, at the runtime it will be compiled into it's value. I.e. 'PA1' will be 1, but also PB1 will be 1 too. So it is no way for the compiler to know which register is accessed, considering only the index inside that array.

But here I can give you several little life-hacks: 1) since registers PINx, DDRx, PORTx are almsot always going in succession in that order (refer to the register set summary in the datasheet), you do not need to store them all, it is enough to store only reference to PINx register, and calculate location of DDRx and PORTx just adding 1 or 2 to the address, since AVR have instructions to inderect memory access with displacement, the code will be effective enough. 2) those registers are located in the lower memory addresses, so instead of storing 2/4-byte pointers you can cast them to byte and cast them back to pointer when accessing. It will not only save space but also speed up. Also it is always a good practice to store that kind of tables in the flash memory, instead of wasting RAM. 3) AVR architecture has only one position bit shifting instructions, so (1 << x) where x is not known at the compile time - is compiled as a loop, which may be the part requiring the most of time in such kind of code. So, instead of storing uint8_t FOO[] = {PA1, PA2}; you may want to store uint8_t FOO[] = {(1 << PA1), (1 << PA2)}; - i.e. precalculated mask values.

AterLux
  • 4,566
  • 2
  • 10
  • 13
  • Thanks, I ended up exploiting the successive memory addresses but added array capability. – 101 Aug 14 '18 at 05:11
0

In the end I made use of the _MMIO_BYTE macro function in avr/sfr_defs.h to base new bit manipulation functions on:

#define SET_OUTPUT(pin)     (_MMIO_BYTE(OFFSET_ADDR((pin)[0] + 0x1)) |=  _BV((pin)[1]))
#define SET_INPUT(pin)      (_MMIO_BYTE(OFFSET_ADDR((pin)[0] + 0x1)) &= ~_BV((pin)[1]))
// etc

This gives easy pin definitions as arrays of pins or single pins:

#define NUM_LEDS 3

const uint16_t LEDS[NUM_LEDS][2] = {
    {PB, 4},
    {PB, 5},
    {PB, 6}
};
const uint16_t BUTTON[2] = {PB, 7};

Pins can then be manipulated like such:

SET_INPUT(BUTTON);
ENABLE_PULLUP(BUTTON);

for (int i = 0; i < NUM_LEDS; ++i) {
    SET_OUTPUT(LEDS[i]);
    SET_HIGH(LEDS[i]);
}

Source code is here: https://github.com/morefigs/avr-bit-funcs.

This was only written for a Mega 2560, but should be easily adaptable to other boards.

101
  • 8,514
  • 6
  • 43
  • 69