Skip to main content
7 of 12
added 65 characters in body
nhgrif
  • 25.4k
  • 3
  • 64
  • 129

###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 mot 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.


###Hexidecimal 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 0x1111111.

Did you even notice there are only seven ones there?

nhgrif
  • 25.4k
  • 3
  • 64
  • 129