This is the second part in a series of  articles in which I’m ‘highlighting’ what I consider to be lousy code provided by IAR in their NXP LPC1764 development kit.

The following function is taken from the LCD_Demo project.

/*************************************************************************
 * Function Name: Dly100us
 * Parameters: void *arg
 * Return: void
 *
 * Description: Delay [100us]
 *        
 *************************************************************************/
void Dly100us(void *arg)
{
volatile Int32U Dly = (Int32U)arg, Dly100;
 for(;Dly;Dly--)
  for(Dly100 = 500; Dly100; Dly100--);
}

An example of the invocation of this function is:

#define GLCD_RESET_DLY                       50     // 5ms
...
Dly100us((void *)GLCD_RESET_DLY);

Quite frankly there is so much to offend one about this function that I’m not too sure where to begin.

Software Delay Loops

Anyone that works on embedded systems works out very quickly that software delay loops are a disaster. Now for low end processors, it is possible to craft a working delay function provided one knows the CPU clock frequency. However in this case, we are dealing with a Cortex 3 processor complete with the usual programmable PLL clock multiplier. Throw in a programmable number of wait states for Flash access, a 3-stage pipeline and of course different compiler optimization settings and it quickly becomes almost impossible to craft a software delay loop that can withstand changes in its environment. The author of this code has made no attempt to accommodate these sorts of changes and as a result this code will fail as soon as almost anything gets changed.

What is particularly bizarre about this example is that the processor has a huge number of hardware timers available – and yet they are not used.

void * arguments

Examination of the code shows that the function is declared as taking a (void *) argument. In crude terms the function expects to receive a pointer to anything. However as the example invocation shows, it is actually being passed an integer constant that has been cast to (void *). Furthermore, within the function, the code then casts the (void *) argument to Int32U. One wonders why on earth Dly100us() isn’t simply defined to accept an Int32u argument? Furthermore, since I believe it’s legal to cast just about anything to (void *) the author has insured that almost any parameter coding construct will be accepted by the compiler – including egregious errors.

Formatting

Within the limits of HTML formatting I have attempted to recreate the indentation used by the author. That is the ‘volatile’ line starts in the left most column and subsequent lines are indented two spaces. There are no blank lines. Now while I’m aware that indentation and the use of white space is very much a question of personal preference IMHO the author’s style leads very much to be desired. In particular I found the code hard to read because of the lack of obvious indentation and also the lack of blank lines between e.g. the variable declaration and the actual code.

Loops without braces

Neither of the for loops is enclosed with braces. While I’m aware this is perfectly legal syntax, just about every coding standard I have ever seen, together with MISRA requires that all loops have braces. This isn’t a case of the language police enforcing their prejudices upon you. Countless studies have shown that requiring braces helps stop lots of problems. As such it’s the smart thing to do

Putting a semi-colon at the end of a for loop

In a similar vein to the previous remark, putting a semi-colon at the end of a for loop statement is just asking for trouble.  If you really can’t find the energy to add a couple of braces then at least do this:

for(Dly100 = 500; Dly100; Dly100--)
    ;

It’s ugly as anything – but at least its existence is obvious.

Poor function name

You will note that the function is called Dly100us(). This is a complete misnomer. The function is actually intended to delay N * 100us where N is the value of the argument passed to the function. Thus the function is incorrectly and misleadingly named.  Most coding standards require that functions have meaningful names.

Poor variable names

The author has used two variables that are very similarly named – Dly and Dly100, Maybe it’s just me, but I find code much harder to read when variable names are similar. Code that is hard to read is hard to maintain.

Poor variable data types

If we ignore the (void *) mess described above, it’s apparent that the argument to the function is intended to be an Int32U. However does this make sense? If the inner loop does indeed delay for 100 microseconds, then with an Int32U argument, the total delay possible is 2^32 * 100e-6 = 429496.7296 seconds or 119.3 hours. Personally in the rare case that I do use software delay loops, the delays are at most milliseconds – and not hours. As a result it seems to me that it would make a lot more sense if the argument to Dly100() was an 8 bit or possibly a 16 bit variable. By making this change you could help prevent stupid coding mistakes that result in ridiculous length delays.

What about the watchdog?

Most well designed embedded systems encompass a watchdog. This code as written completely ignores the implication of delaying beyond the watchdog interval. Thus a small change to the delay time could easily result in the system taking a watchdog reset.

Summary

All in all a dreadfully written function. I shudder to think that there are people out there using this code as a template for their systems.

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 0×12345678; 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.