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.
Could have been worse: you often see loops like this where the induction vars aren’t volatile.
Why should the loop variable be volatile? If we assume that the function is designed to take an integer parameter, the variable Dly is only being used inside that loop and not being modified anywhere else. Are there perhaps some optimisers that will conclude that the loops aren’t actually doing anything and remove them?
Actually in my experience almost all compilers will optimize this function away to nothing if the loop variable is not declared volatile. What I find interesting is that the author knew enough to declare the variable volatile – but still made the rest of the mistakes that I mention.
The author declared both Dly and Dly100 as volatile – only Dly100 in the inner loop needs to be volatile. Making Dly volatile is unnecessary for the correctness of the code, and makes it larger.
I suppose you refer to the risk that the compiler optimizes away the loop:
for(Dly100 = 500; Dly100; Dly100–)
;
to something like:
Dly100 = 0;
Do compilers actually do that?
Yes!
The compiler is being forced to generate the Dly100 = 500 assignment, and also the Dly100 condition, and also the Dly100- expression because Dly100 was declared volatile.
Most assuredly they do. Its actually a common problem with some compilers on some embedded systems when you MUST read a hardware register and discard the result. The compiler can optimise the assignment operation (in C) away to nothing and forcing the actual read becomes a bit of an art form. Declaring volatile might only be the first step in a world of ugliness.
Ashleigh: You may be interested to know that I recently wrote a post with John Regehr on this very topic. You can read it here: https://embeddedgurus.com/stack-overflow/2010/03/reading-a-register-for-its-side-effects-in-c-and-c/
Nice posts. Thank you!
Agree in everything except “Now while I’m aware that indentation and the use of white space is very much a question of personal preference…”. IMHO I don’t think this is a personal preference, when working with other people everything should look similar to facilitate the reading and maintenance. and the use of spaces, indentations, etc. should also be in the coding standards.
I quite agree Aleix. In this case I’m not sure if the author was working to a coding standard or not.
I was surprised you didn’t mention the variable declaration – putting Dly100 at the end of the line, after the assignment, had me puzzled for a second.
I know it’s legal – but, like most of the other problems here, that doesn’t make it good!
Years ago I got badly caught out by a VxWorks BSP – in the code initialising the DRAM controller they did some “nifty” coding that saved a few lines and hid a huge nightmare for the next person trying to edit it.
I thought about mentioning it. In an exercise such as this there is a fine line between pedantry and having a valid point. I’m trying to avoid pedantry – but now you mention it I really should have raised this point.
I can understand why you didn’t comment on this, but I think people underestimate how even something as ‘simple’ as declaring a few variables can go wrong if trying to do too much and be too smart.
When interviewing for embedded softies one of the code snippets I talk about is:
int* i, j = 0;
You would be amazed at how few people know the types of i and j!
There’s more. The comments in the comment block are utterly useless, only telling you what you can learn more quickly by reading the code — the name of the function, the name and type of the parameter it takes, and what it returns. The poor choice of name and type of the argument could have been mitigated by a useful comment, such as “number of 100 us to delay”.
Also, and this is perhaps pedantry
for(;Dly != 0;Dly–)
is more readable than
for(;Dly;Dly–)
If you’re testing if Dly is non-zero, the best approach is to write just that.
I agree it is best to write just that. And I would therefore write Dly, rather than Dly !=0…
Because in Dly != 0 , you are actually saying ‘compare Dly with 0, if equal, expression result is 0, if not equal, expression result is not zero’ Then, if the expression was part of a condition, the compiler will evaluate if the reult of the expression was 0. Which is exactly what Dly would do.
By example
if (Dly != 0) causes to compiler to produce more code than
if (Dly)
Most modern, smart, optimising compilers are all over this particular anomaly in how we write code. Furtunate, because I have seen plenty of coding standards that require the redundant explicit compare to 0 syntax.
Even more insidious is the ‘logical’ extension to define ‘TRUE’ as ‘!0’ and to sprinkle things like
if (Dly == TRUE)
as an even more ‘readable’ form of (Dly != 0) And then, to catch the ‘=’ and ‘==’ mistakes, this should be
if (TRUE == Dly)
I leave it as an exercise for the reader to infer the pitfall in this.
And here lies one of the biggest issues of C/C++ I’ve encountered so far.
It simply shouldn’t be possible to check if an integer is true. Because a number just can’t be true. Is 10 true? Maybe. Is -1 true? Maybe. Is 0 true? Why not.
Yes I know, C says 0 is false and everything not 0 is true. But I think of this only as a workaround for not implementing a boolean.
So I would always write the actual expression which can correctly be translated into a real boolean value like (Dly != 0) which results either in true or false but not in 125. It’s just another form of type-safety for me, or more like the real intent made clear, because I want to continue the loop as long as Dly is not zero.
Also I spontaneously can’t really see why the compiler would produce more code using if (Dly != 0) instead of if(Dly). It could be translated into a simple JNZ/JZ in both cases. But I haven’t actually checked with a compilation what happens in both cases.
It used to be that compilers would generate better code quality for !Dly versus (Dly != 0). However in my experience all decent compilers make this optimization today.
“Quite frankly there is so much to offend one about this function that I’m not too sure where to begin.”
That is exactly what I was thinking when I got to this line.
And your last line does a beautiful job of summarizing the code.
The problem is that a lot of vendors get away with excuses like “Well, it’s not meant to be used as-is, it’s just an example”. But we all know how that goes.
For the record, I’ve found that the code from a few companies (Luminary (now TI), NXP & Freescale come to mind) tends to be pretty decent overall. Of course these are semiconductor companies, not tool vendors.
Michael Barr: You might have a new front-runner for your next “This Code Stinks” presentation!
After reading this, I think that the IAR programmers would benefit greatly from introducing MISRA C in their company. Here is a supplier of MISRA tools I found on the web for them:
http://www.iar.com/website1/1.0.1.0/468/1/
🙂
(The tool of one of their friendly competitors found no less than 8 MISRA violations in that little code, and then I’m not counting the C99-only “//” comments.)
The irony is rather rich isn’t it? I appreciate that your comment is somewhat in jest Daniel. However you do make a valid point. IAR (and countless others) would do well to run their code through Lint and MISRA before releasing it.
We have just recently begin to use Lint in the organization. The first project we submitted to Lint, included an “example display driver.” That display module generated several thousand Lint warnings. Closer to home, it complained about our original code as well. Now, I have had a personal coding standard for years. It has been influenced by reading and by painful experience, and I take pride in it. Nevertheless, Lint calls to my attention to details that I pass over and compels me to think about them. I avoided it for years because I kept hearing that the effort (friction if you will) to use it was not worth the benefit. I wish I had tested that assertion for myself earlier.
I’m glad you are now a Lint fan. I have been for many years and use it religiously. Yes it is a pain to set up (especially for IAR compilers!). However the effort in setting it up pays for itself over and over again. In my opinion, if you aren’t using Lint you aren’t serious about code quality. I’m sure that this statement will upset quite a few people – but I can’t see how anyone can really disagree with it.
I sort of think us as concerned embedded coders should maybe come together and develop an open source library of hardware drivers that is as target-portable as possible without considerable performance cost. I feel like most of my job is actually re-writing things I know a thousand others have re-written. Perhaps we really need to pool together and publish some standard within ourselves and knock out a quality embedded driver library. And maybe we even abstract it at such a level where you only need to fill in some fundamental core API implementation which then allows the easy interface to the higher level of the typical peripheral function.
I got some dozens of drivers to contribute that interface with a low-low-level API that is fairly target portable. As well I have implemented the low-low levels for AVR32 and atleast a couple of Atmel’s ARM-based chips.
I agree. For many microcontroller projects, I spend a lot of time writing (or extending or fixing) peripheral driver code. When I do this, I always think about how many other firmware engineers are doing exactly the same thing. With the exception of Luminary Micro’s Stellaris (now part of Texas Instruments), I know of no other microcontroller vendor that offers a complete peripheral driver library. Most vendors just offer some example code, of varying quality, as we have seen in this series of articles. Usually, these example programs are self-contained, and thus, not in a form that can be dropped into my project and used easily. Having an open source library would certainly save us firmware engineers a lot of time.
This particularly lousy function Dly100us(void *arg) seems to be copied from one LCD driver to another. I remember struggling with it in the otherwise quite decent code for the Stellaris Cortex-M3 MCUs (part of the Stellarisware library from Luminary Micro, now TI.)
Now you mention it, I think I’ve seen it before too. Proof that code propagates – including the stuff that should not.
In the unlikely case I want to delay for more than 6.5 seconds (the maximum value if it were 16-bit), the last thing I want is something wrapping around and delaying for a confusingly low amount of time.
Shudder.
At the very least, prototype the function as:
void delay_us( unsigned int const microseconds );
So somebody reading the header file has at least a chance of guessing what this function does, that it really does expect a single integer parameter, and what units to measure the parameters in rather than the just-now-invented “100us” units.
It’s your business if you want to commit atrocities in your code, but at least hide it from the user.
Strangely enough, this is exactly the type of code I used to write all of the time when I used to test my own ‘C’ compilers; however, I would have probably written this instead:
void Dly100us(void **********************************************arg)
{
volatile int Dly = (int)arg, Dly100;
for(;!!!!!!!!Dly;(*(int*)&Dly)–)
for(Dly100=Dly100=Dly100=500; !!!!!!Dly100; –*(int*)&Dly100);
}
Now that would really be something to complain about!
I agree this is awful code. You wrote above about pedantry – but to many every point you make might be seen as pedantic. You might as well go for it wholeheartedly.
My experience of code from silicon vendors is universally “look but don’t touch”. I use it to steal ideas and nothing more. I’ve not ever found any code from a silicon vendor without serious faults.
Are you ing me with this code?
It almost looks like an entry in the
“How much can you do wrong in 10 lines or fewer?”
contest.
But I only wanted to add, when I do spin loops, I simply add
a comment to that effect:
while (!intflag) /* spin */ ;
Of course, spin loops are likes hen’s teeth….
Does volatile really work for local variables ?
By example, the section 7.1.5 of the ARM AAPCS states the following :
“however, a compiler may ignore a volatile qualification of an automatic variable whose address is never taken unless the function calls setjmp()”
No, the compiler may not ignore volatile ever. Maybe that text was written to document non-standard behavior of the particular compiler?
That text is taken from the “Procedure Call Standard for the ARM Architecture” that is meant to be implemented by all of the compilers for the ARM architecture.
Besides, the following statement, taken from the C standard (5.1.2.3 Program Execution) seems ambiguous to me :
“An actual implementation need not evaluate part of an expression if it can deduce that its
value is not used and that no needed side effects are produced (including any caused by
calling a function or accessing a volatile object)”
I interpret it as saying that side effects can be optimized out, even volatile accesses, if the compiler can ensure they are not needed (by example, volatile local variables).
There is nothing ambiguous. It says “…need not evaluate part of an expression if…value is not used AND that no needed side effects are produced”. In other words:
if(!value_used && !side_effects)
{
optimize();
}
The act of accessing a volatile, by reading or writing to it, is a side-effect. So the compiler is not allowed to optimize. Read the paragraph above the one you cited (5.1.2.3 §2):
“Accessing a volatile object, … are all side effects.”
The parenthesis in the text you cited is there to further emphasize that calling functions and accessing volatiles are side effects.
I understand your interpretation and know that volatile is a side effect.
My problem is precisely the word “needed”. Why talking about “needed side effects” if all the side effects are needed ? I think “needed side effects” can be interpreted as saying “side effects, which are all needed” or “amongst all the side effects, only those who are needed”. It is not clear to me which one is intended.
And I’m still embarassed by the ARM ABI which states “however, a compiler may ignore a volatile qualification of an automatic variable whose address is never taken unless the function calls setjmp()”
The document can be found here : http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042d/IHI0042D_aapcs.pdf
The offending sentence is page 28