1
\$\begingroup\$

I'm programming the Atmega328P in embedded C. I'm going to use 16 pins for turning on LEDs, and I have to use PORTB, PORTD and PORTC for this. I would like to just iterate a pointer so I can turn them on/off instead having to deal with what port each pin relates to. So an example would be how I need three if statements to iterate through the 16 pins like shown below:

// Enable outputs
DDRB = 0b00111111; // 6 outputs
DDRC = 0b00110000; // 2 outputs
DDRD = 0xFF;       // 8 outputs

// LED pins
uint8_t ledpin[] = {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
uint8_t idx = 0;

// Set LEDs
if (idx < 8)
    for(int i = 0; i < 8; i++)
        PORTD |= (1 << ledpin[i]);

if( idx > 8 && idx < 14)
    for(int i = 8; i < 14 ; i++)
        PORTB |= (1 << ledpin[i]);

if (idx >= 14)
    for(int i = 14; i < 16; i++)
        PORTC |= (1 << ledpin[i]);

I would rather use a memory map where I could just say "pin0 address + offset" like this:

// Base address of ports
uint8_t base 0x00
uint8_t offset = 0x04;

// LED pins
uint8_t ledpin[] = {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};

// Set LEDs
for(int i = 0; i < 16; i++){
    (base + offset) |= (1 << ledpin[i]); // Pseudo code
     offset += 0x04;
}

The code above is obviously not optimized, nor totally functioning code but I think it gets my point accross as to what I want to achieve. Ultimately I will use this in a code to control a LED cube which will work quite differently, but it's essential that I don't need to decode what port each pin belong to all the time. Is there a good way to solve this with virtually no overhead?

EDIT

After investigating and trying out a lot of stuff with good help from the answers I have found that it's not possible to address individual bits in the AVR ports. Each PORTB/C/D have their own address, but it's not possible to manipulate the bits with memory mapping, like incrementing a pointer address. Atmega328P datasheet page 280 shows the memory map of the IO ports (image below).

enter image description here

\$\endgroup\$
  • 1
    \$\begingroup\$ The last line of your pseudocode doesn't even have an assignment...what are you trying to do? What does 0x00 represent here? What kind of data do you expect offset() to return? Your original code is changing a single bit at a time using the OR operator but your pseudocode doesn't have the OR operator...I don't get it. \$\endgroup\$ – Elliot Alderson Dec 13 '19 at 17:31
  • \$\begingroup\$ Also you have a semi colon after the for statement in the last but one line, probably unintentionally. \$\endgroup\$ – Michel Keijzers Dec 13 '19 at 17:35
  • \$\begingroup\$ Consult the table of addresses in the data sheet, there is a densely contiguous sequence of PINx/DDRx/PORTx without even any gaps between ports. \$\endgroup\$ – Chris Stratton Dec 13 '19 at 17:39
  • 2
    \$\begingroup\$ Also use bitshifts to access the individual bits. \$\endgroup\$ – Chris Stratton Dec 13 '19 at 18:04
  • \$\begingroup\$ @ElliotAlderson I updated the part of the code for clarity. It was also kind of intentionally not correct, because I'm not sure how memory mapping works. I hope this way represents better what I'm trying to achieve. \$\endgroup\$ – C. K. Dec 13 '19 at 19:57
3
\$\begingroup\$

Is there a good way to solve this with virtually no overhead?

There is no practical way to get 'virtually no overhead' in C on AVR. Code that looks like it should have low overhead often doesn't, and you have to look at the disassembly to see the real code.

AVR has a RISC architecture that needs many instructions to do 'simple' operations. IN, OUT, and SBI/CBI (bit set/clear) instructions take literal arguments only, which must be 'hard-coded' at compile time. But there is no barrel shifter, so bit masks computed at run time can take many instructions to produce. Pointers are made from 8 bit registers that are loaded one byte at a time, so pointer operations may use more cycles than port I/O with conditional code.

Here's the best I have managed to produce so far (It's basically the same as your code except the pin masks are precomputed, and it's wrapped in a function instead of inlined)...

// LED pin masks
#define LP0  1<<0
#define LP1  1<<1
#define LP2  1<<2
#define LP3  1<<3
#define LP4  1<<4
#define LP5  1<<5
#define LP6  1<<6
#define LP7  1<<7
#define LP8  1<<0
#define LP9  1<<1
#define LP10 1<<2
#define LP11 1<<3
#define LP12 1<<4
#define LP13 1<<5
#define LP14 1<<0
#define LP15 1<<1

// pin mask array
uint8_t ledpin_mask[] = {LP0,LP1,LP2,LP3,LP4,LP5,LP6,LP7,LP8,LP9,LP10,LP11,LP12,LP13,LP14,LP15};

// turn on LED
void setled(uint8_t pin)
{
  if (pin < 8)
  {
       PORTD |= (ledpin_mask[pin]);
  }
  else if ( pin < 14)
  {
       PORTB |= (ledpin_mask[pin]);
  }
  else
  {
       PORTC |= (ledpin_mask[pin]);
  }
}

...which generates the following machine code:-

void setled(uint8_t pin)
{
  96:   28 2f           mov r18, r24
  98:   30 e0           ldi r19, 0x00   ; 0
if (pin < 8)
  9a:   88 30           cpi r24, 0x08   ; 8
  9c:   40 f4           brcc    .+16        ; 0xae <setled+0x18>
{
     PORTD |= (ledpin[pin]);
  9e:   9b b1           in  r25, 0x0b   ; 11
  a0:   f9 01           movw    r30, r18
  a2:   e0 50           subi    r30, 0x00   ; 0
  a4:   ff 4f           sbci    r31, 0xFF   ; 255
  a6:   80 81           ld  r24, Z
  a8:   89 2b           or  r24, r25
  aa:   8b b9           out 0x0b, r24   ; 11
  ac:   08 95           ret
}
else if ( pin < 14)
  ae:   8e 30           cpi r24, 0x0E   ; 14
  b0:   40 f4           brcc    .+16        ; 0xc2 <setled+0x2c>
{
     PORTB |= (ledpin[pin]);
  b2:   95 b1           in  r25, 0x05   ; 5
  b4:   f9 01           movw    r30, r18
  b6:   e0 50           subi    r30, 0x00   ; 0
  b8:   ff 4f           sbci    r31, 0xFF   ; 255
  ba:   80 81           ld  r24, Z
  bc:   89 2b           or  r24, r25
  be:   85 b9           out 0x05, r24   ; 5
  c0:   08 95           ret
}
else
{
     PORTC |= (ledpin[pin]);
  c2:   98 b1           in  r25, 0x08   ; 8
  c4:   f9 01           movw    r30, r18
  c6:   e0 50           subi    r30, 0x00   ; 0
  c8:   ff 4f           sbci    r31, 0xFF   ; 255
  ca:   80 81           ld  r24, Z
  cc:   89 2b           or  r24, r25
  ce:   88 b9           out 0x08, r24   ; 8
  d0:   08 95           ret

That's 12 or 14 machine code instructions executed depending on the port, which is probably about as close as you can get to 'virtually no overhead' without coding in optimized assembler.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I guess this is the best I can do then! I'm wondering how it compares to the digitalWrite function in Arduino, which is terrible. I will run some tests! \$\endgroup\$ – C. K. Dec 14 '19 at 0:05
  • \$\begingroup\$ Even without a barrel shifter the asker's current sequential access can be accomplished with a simple single bit shift per iteration and a small amount of logic where the port address changes. \$\endgroup\$ – Chris Stratton Dec 14 '19 at 1:43
  • \$\begingroup\$ @ChrisStratton Excellent answer! \$\endgroup\$ – TomServo Dec 14 '19 at 2:00
  • \$\begingroup\$ @ChrisStratton - yes, but then the code becomes less general purpose. The code will ultimately be used to "control a LED cube which will work quite differently", implying that it might not just set bits sequentially. \$\endgroup\$ – Bruce Abbott Dec 14 '19 at 2:04
1
\$\begingroup\$

How about a structure of the addresses?

//PORTD = 0x10000
struct {
 void * 0x10000;
 void * 0x10001;
 void * 0x10002;
 ...
 // PORTB = 0x20000
 void * 0x20000;
 void * 0x20001;
etc.
}

Then you can iterate over the structure.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That's rather wasteful, they already have a dense and contiguous order in the hardware. \$\endgroup\$ – Chris Stratton Dec 13 '19 at 17:36
  • 1
    \$\begingroup\$ If the OP had theLED hardware already in chip order, the question wouldn't exist. Also and optimized solution wasn't requested. \$\endgroup\$ – Aaron Dec 13 '19 at 17:42
  • 1
    \$\begingroup\$ Your suggestion doesn't seem to be the same as the original question. The OP was manipulating individual bits at a single address for each port but you seem to have a distinct address for each bit. I'm not very familiar with the AVR...can you explain how this would work? \$\endgroup\$ – Elliot Alderson Dec 13 '19 at 19:27
  • \$\begingroup\$ @Aaron I think this is close to what I wanna do. Will investigate it further. \$\endgroup\$ – C. K. Dec 13 '19 at 20:16
  • \$\begingroup\$ @ElliotAlderson I have investigated this and it's not possible to refer to each bit in a port with an address, only the address of the actual port. So that leaves me with using PORTB/C/D as just the same as using memory mapping techniques. \$\endgroup\$ – C. K. Dec 14 '19 at 1:00
1
\$\begingroup\$

Unfortunately you can't have a pointer to certain bit of port directly.

If a struct that for each led contains both the port address and bit index is too wasteful, use one more level of indirection. I introduce a concept of LED Handle which is simply a byte to conveniently point indirectly to an IO port and bit in it.

First, generate a table that contains the addresses of used IO ports, for example:

volatile uint8_t *portlist[3]={&PORTB, &PORTC, &PORTD};

Now you can just fetch the address to a port via table indexes (0-2).

Next, generate the list of LEDs, by generating an array of "LED Handles", where the "LED Handle" is just a byte where for example high nybble is index to the port table, and low nybble is the bit in the port.

uint8_t ledlist1[] = {0x07, 0x14, 0x26};

In this case, first entry, 0x07, uses port index 0, which is PORTB, and led bit 7.

0x14 would be port index 1 (PORTC) and led bit 4. 0x26 would be port index 2 (PORTD) and bit 6.

Example function to set a bit to one via this led handle:

void led_on(uint8_t ledhandle)
{
    volatile uint8_t *ledport = portlist[ledhandle>>4];
    *ledport |= (1 << (ledhandle&0x07));
}

So, it uses the high 4 bits of the parameter as index to port table, from where it fetches the address of the port to ledport pointer. Then, it uses the low 3 bits of the parameter to calculate a bit mask for the bit to turn on, and accesses the port address pointed by ledport to set it on.

Then you can just do a for loop that for each entry in LED table calls the led_on like this:

for (i=0; i<3; i++)
    led_on(ledlist1[i]);

If this sounds awkward, you can change the led_on to take directly the LED table index, so you don't have to work with "LED Handles" that are contained in the LED table.

You can have further macros to make it more abstract, for example:

#define PB 0
#define PC 1
#define PD 2
#define LED(portindex,portbit) ((portindex<<4)|(portbit))

uint8_t ledlist2[]={
    LED(PB,7),
    LED(PC,4),
    LED(PD,6)
};
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I guess this works, but it doesn't really help me too much with the task I'm wanting to do. It seems simple if-else statements will actually be the easiest and most efficient! \$\endgroup\$ – C. K. Dec 14 '19 at 0:59

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.