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.
