Running Lights - Embedded "Hello World"

I'm getting my feet wet in embedded development, and like all those before me, my first task was to make an LED blink. I went a bit further than that and made a "runner" that lights up each led from 0 to 7 in sequence. The sequence begins as soon as the board is powered on. You can see it in action here.

I'm using an ATxMegaA1 chip with the avr-gcc compiler and tool chain. The document I consulted most was the Using IO pins and external interrupts doc.

I decided that creating an "Led" concept was a nice abstraction to wrap around the port they're attached to on the board. I also introduced a macro to change the delay when I'm debugging, because the simulator is orders of magnitudes slower than the real chip.

It's my first ever c, so please be brutal.

led.h

#include 

#define LEDPORT PORTE_OUT
#define LEDPORT_DIR PORTE_DIR

void init(void);
void toggleLights(int ledPosition);

led.c

#include "led.h"

void init(void)
{
    LEDPORT_DIR = 0b11111111; //Configure LED port for output
    LEDPORT = 0b11111111; //LEDs are active low, this makes sure they're off on start up
}

//Turns light at provided port pin on and all others off.
void toggleLights(int ledPosition)
{
    LEDPORT = ~(1 << ledPosition);
}

Blink.c

#include 
#include "led.h"

#ifdef DEBUG
    #define DELAYITERATIONS 0
#else
    #define DELAYITERATIONS 10000
#endif 

void delay(volatile uint32_t d)
{
    while (d-- != 0)     // loops while non-0 and decrements
    ;
}

int main(void)
{
    init();

    while(1)
    {
        for (int i = 0; i < 8; i++)
        {
            toggleLights(i);
            delay(DELAYITERATIONS);
        }
    }
}

Toggling? Or setting?

Our toggleLights function is oddly named and doesn't do what I'd expect. (Specifically, a toggle turns off things on and on things off.)

In C, and especially in embedded C, we shouldn't be afraid to use unsigned integers as bit arrays.

So, perhaps we want a setLights function that looks something like this:

void setLights(uint8_t lights) {
    LEDPORT = lights;
}

And to turn each of the eight lights on, one at a time, we can bitshift over them in our for loop:

for (uint8_t lights = 1; lights != 0; lights <<= 1) {
    setLights(lights);
    // delay
}

So now we're effectively doing what your code attempts to do, but the setLights function is even more flexible. It just sets the lights to exactly whatever we tell it to set them to (arguably, the function may not need to exist at all... but it does restrict our input to a correct type and give us some sense of safety).

Using typedef

Firstly, we should be using a uint8_t here rather than an int because we're never going to use any values that we can't represent with 8 bits and that'll save us 24 bits.

But how about we use our own name for the type?

typedef uint8_t light_positions;

So now,our setLights function becomes:

void setLights(light_positions lights);

And our for loop becomes:

for (light_positions lights = 1; lights != 0; lights <<= 1) { // ...

This type helps add to the clarity of what this variable is supposed to represent and be used for. This can be especially useful in embedded C where you might be prone to having lots of variables of the uint8_t type (or other sizes of unsigned integers).

`lights Did you even notice there are only seven ones there?

StackExchange Code Review Q#98157, answer score: 27