This is the third 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. This example is taken from the Accelerometer project. The accelerometer is accessed via an I2C bus, and so the main purpose of this example code is to demonstrate how to use the I2C interface. If you have ever had to write an I2C driver you will know that they are deceptively hard. In my case the project I’m working on does indeed need an I2C driver – and so I was expecting to be able to borrow heavily from the IAR code. Alas I found the code to be almost completely useless. To see why, read on …

Upon examination of the I2C driver the first thing I found were a number of ‘helper functions’. For example:

/*************************************************************************
 * Function Name: I2C_EnableI2C
 * Parameters: void
 * Return: void
 * Description: Enable I2C device.
 *************************************************************************/
#pragma inline
void I2C_EnableI2C(void)
{
 I2C2CONSET |= I2CON_I2EN;
}

While in general I’m all in favor of these types of helper functions, I had three problems with the way they were implemented.

1. Clearly if anyone other than the I2C driver was to invoke these functions then really bad things would happen. Despite this, the author has not seen fit to declare the function static, thus opening them up for the world to use.
2. These helper functions are located at the top of the file.  There has been an interesting thread of discussion on this blog regarding whether this is good, bad or indifferent practice. Personally I strongly dislike it as it puts an implementation minutiae in my face – when want I want to know is ‘what’s the big picture’. Thus IMHO these types of functions should be at the end of the file – but I recognize that there are those who disagree!
3. The next thing that puzzled me was that I couldn’t find the definition of ‘I2CON_I2EN’ anywhere in the file. Clearly it had to exist somewhere and so a project wide search showed that it was defined in the the file i2c_drv.h. Now the only module that could possibly have any interest in the value of ‘I2CON_I2EN’ is the driver code i2c_drv.c. Despite this, the author has seen fit to put the define in the header file rather than the C file. I have to admit that I have seen this done by a lot of other people – and for the life of me I cannot understand the logic. Why would you go out of your way to make public something that inherently should remain private? When combined with the fact that the helper functions are also public the only possible explanation is that the author expected code outside the driver to interact with the I2C hardware!

I also noticed that these functions were being declared without a prototype – and yet the code was compiling without a warning. I thus examined the compilation options – and sure enough the author had the ‘Require prototypes’ box unchecked.

Now to be fair to the author, and for reasons only known to IAR, this is the default setting. Notwithstanding this, building code without requiring prototypes is just inexcusable.

Anyway, next up in the driver was an initialization function and then this function:

/*************************************************************************
 * Function Name: I2C_MasterWrite
 * Parameters:  unsigned char addr -- the slave address which you send message to
 *        unsigned char *pMsg -- the point to the message
 *        unsigned long numMsg -- the byte number of the message
 * Return: int
 *                 0: success
 *       non-zero: error number
 *
 * Description: Transmit messages
 *
 *************************************************************************/
int I2C_MasterWrite (unsigned char addr, unsigned char *pMsg , unsigned long numMsg)
{
 return I2C_Transfer (addr, pMsg , numMsg, WRITE, 0);
}

I looked at the header and was immediately disappointed as I noticed the following:

1. The author had suddenly and mysteriously switched to using the built in data types (char, int, long), despite the rest of the code in the project using defined data types Int32u etc. Whenever I see lack of data type discipline I know I’m in for a bad experience.
2. The first parameter was described as “the slave address which you send message to”. Well anyone that has done any work with the I2C bus knows that the concept of an address is poorly defined. In the I2C standard, addresses are 7 bits. However, what gets sent to an I2C slave is the address left shifted one place with a R/W bit placed in the LSB position. As a result, many people consider the slave address to be the byte that is transmitted with the LSB masked out. While I’m not particularly interested in arguing about which camp is correct, the point is that tremendous ambiguity exists in this area. As a result a well written I2C driver should be unambiguous about what it expects for a slave address. In this case I’m clueless as to what to pass to the driver.
3. The second parameter is a pointer to the message to be transmitted. However, the pointer is not defined as pointing to const – which it should be. Again this is indicative of someone who is sloppy.
4. The third parameter is described as “the byte number of the message”. I’m sure the author meant “the number of bytes in the message”. I’m not going to complain about this – on the assumption that the author’s native language is not English. Notwithstanding this, I do still have a problem with the third parameter – and that’s the choice of the data type. I have written a lot of I2C code in my life – and I have never been in a situation where I needed to send so many bytes to a peripheral that the length byte needed to be an unsigned long. I would have thought a 16 bit variable would have been quite enough.
5. The return type was defined as ‘int’ with zero being success and non-zero being an error number. However, it was completely unclear what were the possible list of error numbers. IMHO if you are going to return an error code from a function then the return type should be an enumeration – which presumably via the suitable choice of enumeration names would describe the particular error. Notwithstanding this, I hunted around and in the header file for the driver I found the following:

/* Status Errors */
#define I2C_OK                         0       // transfer ended No Errors
#define I2C_IDLE                    1       // bus idle
#define I2C_BUSY                     2       // transfer busy
#define I2C_ERROR                   3       // err: general error
#define I2C_NO_DATA                 4       // err: No data in block
#define I2C_NACK_ON_DATA           5       // err: No ack on data
#define I2C_NACK_ON_ADDRESS       6       // err: No ack on address
#define I2C_DEVICE_NOT_PRESENT     7       // err: Device not present
#define I2C_ARBITRATION_LOST       8       // err: Arbitration lost
#define I2C_TIME_OUT                 9       // err: Time out occurred
#define I2C_SLAVE_ERROR           10      // err: Slave mode error
#define I2C_INIT_ERROR               11      // err: Initialization (not done)
#define I2C_RETRIES                 12      // err: Initialization (not done)

These appeared to fit the bill – or so I thought. However, looking at the body of the above code, it was apparent that all the real work got done in the I2C_Transfer() function and so I turned my attention to it. The first part of the code is below:

/*************************************************************************
 * Function Name: I2C_Transfer
 * Parameters:  unsigned char addr -- the slave address which you send message to
 *        unsigned char *pMsg -- the point to the message
 *        unsigned long numMsg -- the byte number of the message
 *        LPC_I2C_TransMode_t transMode -- Read, Write, Write then read
 *        unsigned long numWrite -- this is only for "Write then read" mode
 *            
 * Return: int
 *                 0: success
 *       non-zero: error number
 *
 * Description: Transfer messages
 *
 *************************************************************************/
int I2C_Transfer (unsigned char addr, unsigned char *pMsg , unsigned long numMsg,
 LPC_I2C_TransMode_t transMode, unsigned long numWrite)
{
unsigned int timeout = DLY_I2C_TIME_OUT;
 if (transMode == WRITETHENREAD)
 {
   if (numWrite >= numMsg)
     return 1;
   else
     I2CMsg.nrWriteBytes = numWrite;
 }
 else
   I2CMsg.nrWriteBytes = 0;
...
}

In looking at this code I noticed that it would immediately return the value of 1 if some criterion was met. However looking at my list of error codes, a 1 is ‘I2C_IDLE’. That clearly didn’t make sense. Despite this, on examining the rest of the function I found that it did indeed return the error codes listed above. In other words the author was just too lazy to define another error code and so he simply reused an existing error code – even though it had nothing to do with the issue at hand. By this point I was utterly convinced that the code was a complete train wreck.

While I also found a lot more to dislike about the implementation, my final complaint is actually a lot more subtle – but in many ways the most important of all. Having reviewed the entire driver, I realized that it didn’t contain an interrupt handler. The implication (which was borne out by closer code inspection) was that the I2C driver worked using polling. Now if you have done any work with I2C you will know that it is quite legal for an addressed I2C slave to stretch the clock and to in general take its sweet time responding to read requests. As a result it is quite usual for an I2C transaction to take a millisecond to complete. Throw in the cases where bus contention occurs and the worst case I2C transaction time can rapidly hit multiple milliseconds. Maybe it’s just me, but I don’t invest in a 100 MHz Cortex processor to have it spinning its wheels polling an I2C bus. Now while I can accept that in this case the author’s intention may have been to just demonstrate some basic I2C functionality, at no point in the source code comments or in a readme file did the author state “WARNING: This driver uses polling and may block for up to N ms”. IMHO any code that is likely to block must come with the appropriate warnings. The fact that this one doesn’t tells me a lot about the author’s attitude to real time response.

I think that will do it for the I2C driver. Next up – how to make code maintenance hard.

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.