About a month ago I bought an ARM evaluation board from my favorite compiler vendor, IAR Systems. Like most people I bought the board in part because it comes with a lot of example code showing how to configure the peripherals on the NXP LPC1768. Upon receiving the board, I hooked it up and started browsing the example code that came with it. To my dismay I discovered a plethora of what I consider to be shoddy coding practices. Now I am well aware that one person’s shoddy practices is another person’s standard operating procedure, and so I intend to steer clear of areas that are just a matter of taste. Notwithstanding this, over the next few posts I will describe some of the things I found in the code – and more importantly why I think they are shoddy.
Before I describe the first example, I will comment on why I’m doing this. Three years ago, almost to the day, I wrote about the lousy code that hardware manufacturers supply as part of their application notes. When I wrote the post I had in the back of my mind the idea that hardware manufacturers aren’t really interested in code and so what should one expect? However when it comes to compiler vendors, what possible excuse do they have? To put it another way, where should engineers go to look for great examples of how to write embedded systems code if not the example code provided by compiler vendors? Do they not have a responsibility to publish code that uses best practices and sets a great example? Furthermore, even if they contracted the code out to some third party to write then I still think it is incumbent upon them to demand (and check) that the code is indeed the sort of code that they want to put their name on?
With all that being said, I will start with an example that is not particularly egregious!
Right associativity of the = operator
One of the lesser appreciated aspects of the C programming language is that the assignment (=) operator associates right to left. As a result the following is perfectly legal code.
uint16_t a,b,c; a = b = c = 6;
This code has the effect of setting all three variables to the value of 6. With that as a preamble, I found that the author of the evaluation board code loves this construct – even when it is inappropriate. For example consider this excerpt.
/*************************************************************************
* Function Name: GpioInit
* Parameters: void
* Return: void
*
* Description: Reset all GPIO pins to default: primary function
*
*************************************************************************/
void GpioInit(void)
{
// Set to inputs
FIO0DIR = \
FIO1DIR = \
FIO2DIR = \
FIO3DIR = \
FIO4DIR = 0;
// clear mask registers
FIO0MASK =\
FIO1MASK =\
FIO2MASK =\
FIO3MASK =\
FIO4MASK = 0;
// Reset all GPIO pins to default primary function
PINSEL0 =\
PINSEL1 =\
PINSEL2 =\
PINSEL3 =\
PINSEL4 =\
PINSEL7 =\
PINSEL8 =\
PINSEL9 =\
PINSEL10 = 0;
}
So why don’t I like this code? Well I have several complaints. The main problem is that in my opinion one should only make use of the right associative property of the assignment operator if it is essential that multiple variables have the same starting value. That is, if I have a piece of code that can only possibly work if two variables start with the same value, then I will make use of this construct. For example:
uint16_t foo, bar; foo = bar = 21; /* foo and bar must be equal to each other and start at 21 */
If instead it is merely coincidental that two variables start with the same value, then I will put their assignment on separate lines.
uint16_t foo, bar; foo = 14; bar = 14;
In the IAR code there is no requirement that the various registers be locked together with a starting value.
My second complaint is that because there is no requirement that the various registers be locked together this code is now a pain to maintain. For example, consider the case where it became necessary to change the value of FIO2DIR to 0x12345678; One would now have to do this:
// Set to inputs FIO0DIR = \ FIO1DIR = 0; FIO2DIR = 0x12345678; FIO3DIR = \ FIO4DIR = 0;
In other words I would have to edit an unrelated line (FIO1DIR) – which is a recipe ripe for disaster. To drive home this point, consider the case where rather than having to edit FIO1DIR I wanted to change just FIO4DIR. I would say that the likelihood of someone doing this is very high:
// Set to inputs FIO0DIR = \ FIO1DIR = \ FIO2DIR = \ FIO3DIR = \ FIO4DIR = 0x12345678;
In other words they might not notice the line continuation operator and simply change the value of the register they are interested in. The code would compile; the results would be disastrous.
While I am at it I will opine on the use of the line continuation operator. While this exists for good reason, it almost never leads to easy to read, easy to maintain code. In short I never use it unless I absolutely have no choice. To use it in a casual manner such as this is not good.
As a final point, I am sure that many people that purchase the evaluation board are quite inexperienced. The GpioInit function excerpted above makes unnecessary use of two advanced constructs (right associativity and the line continuation character). Furthermore the use of these constructs doesn’t make the code better – it makes it worse! I am quite sure that a novice C programmer would look at this code and be baffled as to what is going on – possibly triggering a call to IAR technical support.
