Code maintenance and magic numbers

Matthew Eshleman

4 years ago

In the field of firmware and embedded software we frequently encounter “magic numbers.” Examples may include:

The duration of a reset pulse.
The toggling of an undocumented bit.
The alignment of some RAM buffer, required by a hardware component.
And many more!

Having observed and directed the development of a large mixed C and C++ code base for over a decade, I’m particularly sensitive to ensuring that a code base is readable and maintainable. So let us look at an example.

Upon reviewing a module of C++ code controlling an external FPGA, we encounter a function such as the following:

void reset_fpga() 
{
   reset_pio.set_low();
   sleep(100);
   reset_pio.set_high();
}

The above code is pretty typical in our field. Ignoring the inline blocking sleep (assume this is acceptable in this project) my review might involve:

What are the units for sleep()? Is this 100 RTOS ticks? 100 CPU clock cycles? 100 milliseconds?
Upon searching for the sleep() function, we find that this function takes an integer representing milliseconds.
Why are we sleeping for 100ms? What was the source or requirement for this duration? Why not 50ms? Why not 200ms?
Pulling up the datasheet for the FPGA in question, we find that it recommends a minimum reset pulse duration of 50ms. Why did the code use an additional 50ms?

Immediately, we could improve this code with the following:

void reset_fpga() 
{
   constexpr int RESET_PULSE_DURATION_MS = 100;
   reset_pio.set_low();
   sleep(RESET_PULSE_DURATION_MS);
   reset_pio.set_high();
}

Ok, the above is a bit better, at least we can now read the units naturally. But I think further improvements are needed. I really want to understand the source of this requirement too. My next improvement might be:

void reset_fpga() 
{
   // References: 
   //   FPGA Datasheet rev 1.1, page 10, 50ms minimum.
   //   HW design doc ABC rev 1.0, page 22, for the additional margin.
   constexpr int RESET_PULSE_DURATION_MS = 100;
   reset_pio.set_low();
   sleep(RESET_PULSE_DURATION_MS);
   reset_pio.set_high();
}

Ok, again we are now a bit better. We now have some traceability to various requirements via a comment. However, the code is perhaps violating DRY with the documenting of the 50ms value in the comment. And again, I think we can improve it further, see the next example:

void reset_fpga() 
{
   constexpr int PULSE_MINIMUM_MS = 50; //FPGA Datasheet rev 1.1, page 10
   constexpr int PULSE_MARGIN_MS  = 50; //HW design doc ABC rev 1.0, page 22
   constexpr int RESET_PULSE_DURATION_MS = PULSE_MINIMUM_MS + PULSE_MARGIN_MS; 

   reset_pio.set_low();
   sleep(RESET_PULSE_DURATION_MS);
   reset_pio.set_high();
}

The code is again better and I might even be tempted to stop here. We have separated out the two sources of requirements: the FPGA datasheet requirement and the additional margin required by the hardware design team. By separating these values, we clarify the source of the requirements AND we make future maintenance easier. For example, perhaps on the next revision of the hardware, the design team realizes they were too conservative on the margin and reduces the margin to 10ms. Now the code change is easy and isolated.

Although I would be tempted to stop at the above, the units are not type safe. Can I request a new sleep() function that makes use of std::chrono? Assuming we can modify sleep (or provide an override), then we can make another change:

void reset_fpga() 
{
   using namespace std::chrono_literals;
   constexpr auto PULSE_MINIMUM = 50ms; //FPGA Datasheet rev 1.1, page 10
   constexpr auto PULSE_MARGIN  = 50ms; //HW design doc ABC rev 1.0, page 22
   constexpr auto RESET_PULSE_DURATION_MS = PULSE_MINIMUM + PULSE_MARGIN; 

   reset_pio.set_low();
   sleep(RESET_PULSE_DURATION_MS);
   reset_pio.set_high();
}

And that is about where I would stop. We have now accomplished the following:

The code is “readable” – I know the units.
The units are type safe, no longer using a bare integer. The human reader AND the compiler know the units.
Appropriate comments, which do not violate DRY, provide references for the source of the requirements.
The breakup of constants reflect the two sources for the total duration of the reset pulse.
Future code maintenance will be easier and more obvious, for example when the margin is changed.

So, given all of the above, what further changes or improvements would you recommend?

Photo by Joshua Sortino on Unsplash

Share: