Running Lights - Embedded "Hello World"

Question

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 <avr/io.h>

#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 <stdint.h>
#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);
        }
    }
}

Answer 1

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 <<= 1

<<= is a compound assignment operator. Everyone knows and is familiar with +=, correct? It's the most common of the ten compound assignment operators.

Meanwhile, << is a bitwise left shift operator (you used it in your toggleLights function).

So here's what's happening in our for loop.

We initialize lights to 0x00000001. Run the loop body. Update statement shifts lights to the left by 1 (lights <<= 1), and we end up with: 0x00000010.

We repeat this several times until we get to 0x10000000, and the next update statement shifts all of the bits out. We end up with 0x00000000 (just 0) and the for loop exits, because lights != 0 returns false now.

---

About preprocessor macros...

Preprocessor macros are sometimes necessary, but generally, lead to code that is harder to debug.

Rather than your version of the macro, I think the code is more clear if we just inline it in the loop:

toggleLights(lights);
#ifdef DEBUG
    delay(1000);
#endif

Of course, if we're using the DELAYITERATIONS in multiple spots, then your version is fine--we don't want to be writing extra code necessarily. But if we're only using it in one spot, write it more like this.

---

Use bools...

There are a number of ways to use true/false in C (which helps make your code more human readable). Pick one and use it.

---

Hexadecimal vs Binary

Using hexadecimal notation is arguably more readable than counting the number of 1s in binary notation. I think I'd prefer seeing 0xFF rather than 0b1111111.

_{Did you even notice there are only seven ones there?}

Answer 2

Your current delay code is busy-waiting, this means that in order to delay for the amount of time required you are just wasting however many CPU cycles you need in order for that amount of time to elapse. If I remember correctly _delay_ms is the library function that does something similar to what you already have. It works by calculating the number of no-op's required to wait based on the required ms for the delay given the clock speed. _delay_ms is however still a busy wait function and you should treat it's usage with suspicion if the delay time is anything bigger than minimal.

Because CPU power is limited and delaying for more than momentary times is a particularly common problem that people must solve with their embedded work the chip designers have included a bunch of timers with interrupts for solving this exact problem. So a better way to do achieve this delay is to use the internal timers that are made available to you. You will need to make some code that sets up the required counter value for the timer given the prescaling and delay amount needed. Then create an interrupt that fires on the timer compare, when you want to delay you have to enable that interrupt. Then in the ISR for the timer interrupt you can toggle the pin that drives the LED from there. This will save you a lot of CPU power and will introduce you to an important concept in doing more than one thing concurrently when you need a timer in your code.

Answer 3

Magic numbers

You have some magic numbers in your code. Create constants for them to make the code more readable and maintainable. Using an unnamed enum is a trick for creating constant int values. Using const qualified variables work in this case, but don't for things like in switch statements and for the size of arrays. See this answer for more info on declaring constants in C.

enum {
    LIGHTS_COUNT = 8,
    LEDPORT_INIT_VALUE = 0b11111111
};

Delay

You're running a busy loop to delay the process. This prevents the OS from scheduling other tasks instead of using resources for no-ops. See What are trade offs for “busy wait” vs “sleep”?. I recommend using sleep instead - this is the ordinary way to wait.

Use _delay_ms as @RubberDuck recommends in their answer.

Infinite loop

The conventional infinite loop in C uses for instead of while.

for (;;) {

}

Source files

For a project with only 4 functions, is it really necessary to split it into 3 different files? Using more source files only become useful if the modules are larger in my opinion.

Answer 4

This really is picky, picky, picky, and I recognize that.

However, source code is written for human beings, not computers, so your source code should be crystal clear, and as readable as possible. Keep this in mind.

1. There's nothing harmful in your header, led.h, but you didn't wrap it so that it's included only once. You can see this practice in all sorts of header files (stdio.h for instance) and it's done so that you don't compile errors when both .h and .c files include the same header file.

   #include <avr/io.h>
   
   #ifndef _LED_H_
   #define _LED_H_
   
   #define LEDPORT PORTE_OUT
   #define LEDPORT_DIR PORTE_DIR
   
   void init(void);
   void toggleLights(int ledPosition);
   
   #endif
   

2. Here's the picky part:

   Boolean tests in C language are oddly handled. We tend to think of one as true, and zero as false, but that's not exactly right. Zero is false, and not zero is true.

   So, your while loop:

   while(1)
   

   will run fine, and you may think it's amazingly clear. I do understand that. But so will

   while( -666 )
   

   or

   while(8675309)
   

   and that is significantly less clear.

   Really, what you're looking to express is TRUE or FALSE.

   You can find this sort of definition all over the place in C language headers:

   #define FALSE 0
   #define TRUE !FALSE
   

   or sometimes:

   #define TRUE 1
   

   which I like even more, because it removes the ambiguity.

   And, generally, I agree with much of what has been said above.

Answer 5

It's a small point in comparison to the other answers, but worth noting for a beginner. This is the LED header file.

#include <avr/io.h>

#define LEDPORT PORTE_OUT
#define LEDPORT_DIR PORTE_DIR

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

Anything in the header is available to outside code. There's no reason for anything to have access to LEDPORT_DIR. We want this port set up for output and that's it. Don't give yourself an opportunity to muck with that setting after it's been initialized. Minimally, move this to the led.c file, but practically, it's not doing much and could be removed entirely.

Sure, you could still access it via PORTE_DIR, but the point of the abstraction is that this module should be the only one to access PORTE by its real name. Anything else should work with the LEDPORT and nothing should be setting that port up for input.

---

Another thing to note is that your compiler offers some built in delay methods. Add #include <util/delay.h> and replace your hack with _delay_ms.