Archive for the ‘Coding Standards’ Category

Configuring hardware – part 1.

Saturday, November 13th, 2010 Nigel Jones

One of the more challenging tasks in embedded systems programming is configuring the hardware peripherals in a microcontroller. This task is challenging because:

  1. Some peripherals are stunningly complex. If you have ever configured the ATM controller on a PowerQUICC processor then you know what I mean!
  2. The documentation is often poor. See for example just about any LCD controller’s data sheet.
  3. The person configuring the hardware (i.e. me in my case) has an incomplete understanding of how the peripheral works.
  4. One often has to write the code before the hardware is available for testing.
  5. Manufacturer supplied example code is stunningly bad

I think I could extend this list a little further – but you get the idea. Anyway, I have struggled with this problem for many years. Now while it is impossible to come up with a methodology that guarantees correct results, I have come up with a system that really seems to make this task easier. In the first part of this series I will address the most elemental task – and that is how to set the requisite bits in the register.

By way of example, consider this register definition.

This is a control register for an ADC found in the MSP430 series of microcontrollers. The task at hand is how to write the code to set the desired bits. Now in some ways this is trivial. However if you are serious about your work, then your concern isn’t just setting the correct bits – but doing so in such a manner that it is crystal clear to someone else (normally a future version of yourself) as to what you have done – and why. With this as a premise, let’s look at some of the ways you can tackle this problem.

Magic Number

Probably the most common methodology I see is the magic number approach. For example:

ADC12CTL0 = 0x362C;

This method is an abomination. It’s error prone, and very difficult to maintain. Having said that, there is one case in which this approach is useful – and that’s when one wants to shutdown a peripheral. In which case I may use the construct:

ADC12CTL0 = 0;   /* Return register to its reset condition */

Other than that, I really can’t see any justification for this approach.

Bit Fields

Even worse than the magic number approach is to attempt to impose a bit field structure on to the register. While on first glance this may be appealing – don’t do it! Now while I think bitfields have their place, I certainly don’t recommend them for mapping on to hardware registers. The reason is that in a nutshell the C standard essentially allows the compiler vendor carte blanche in how they implement them. For a passionate exposition on this topic, see this comment on the aforementioned post. Anyway, this approach is so bad I refuse to give an example of it!

Defined fields – method 1

This method is quite good. The idea is that one defines the various fields. The definitions below are taken from an IAR supplied header file:

#define ADC12SC             (0x001)   /* ADC12 Start Conversion */
#define ENC                 (0x002)   /* ADC12 Enable Conversion */
#define ADC12TOVIE          (0x004)   /* ADC12 Timer Overflow interrupt enable */
#define ADC12OVIE           (0x008)   /* ADC12 Overflow interrupt enable */
#define ADC12ON             (0x010)   /* ADC12 On/enable */
#define REFON               (0x020)   /* ADC12 Reference on */
#define REF2_5V             (0x040)   /* ADC12 Ref 0:1.5V / 1:2.5V */
#define MSC                 (0x080)   /* ADC12 Multiple Sample Conversion */
#define SHT00               (0x0100)  /* ADC12 Sample Hold 0 Select 0 */
#define SHT01               (0x0200)  /* ADC12 Sample Hold 0 Select 1 */
#define SHT02               (0x0400)  /* ADC12 Sample Hold 0 Select 2 */
#define SHT03               (0x0800)  /* ADC12 Sample Hold 0 Select 3 */
#define SHT10               (0x1000)  /* ADC12 Sample Hold 1 Select 0 */
#define SHT11               (0x2000)  /* ADC12 Sample Hold 1 Select 1 */
#define SHT12               (0x4000)  /* ADC12 Sample Hold 2 Select 2 */
#define SHT13               (0x8000)  /* ADC12 Sample Hold 3 Select 3 */

With these definitions, one can now write code that looks something like this:

ADCT12CTL0 = ADC12TOVIE + ADC12ON + REFON + MSC;

However, there is a fundamental problem with this approach. To see what I mean, examine the comment associated with the define REF2_5V. You will notice that in this case, setting the bit to zero selects a 1.5V reference. Thus in my example code, I have implicitly set the reference voltage to 1.5V. If one examines the code at a later date, then it’s unclear if I intended to select a 1.5V reference – or whether I just forgot to select any reference – and ended up with the 1.5V by default. One possible way around this is to add the following definition:

#define REF1_5V             (0x000)   /* ADC12 Ref = 1.5V */

One can then write:

ADCT12CTL0 = ADC12TOVIE + ADC12ON + REF1_5V + REFON + MSC;

Clearly this is an improvement. However there is nothing stopping you writing:

ADCT12CTL0 = ADC12TOVIE + ADC12ON + REF1_5V + REFON + MSC + REF2_5V;

Don’t laugh – I have seen this done. There is also another problem with the way the fields have been defined, and that concerns the fields which are more than 1 bit wide. For example the field SHT0x is used to define the number of clock cycles the sample and hold should be active. It’s a 4 bit field, and thus has 16 possible combinations. If I need 13 clocks of sample and hold, then I have to write code that looks like this:

ADCT12CTL0 = ADC12TOVIE + ADC12ON + REF1_5V + REFON + MSC + SHT00 + SHT02 + SHT03;

It’s not exactly clear from the above that I desire 13 clock samples on the sample and hold. Now clearly one can overcome this problem by having additional defines – and that’s precisely what IAR does. For example:

#define SHT0_0               (0*0x100u)
#define SHT0_1               (1*0x100u)
#define SHT0_2               (2*0x100u)
...
#define SHT0_15             (15*0x100u)

Now you can write:

ADCT12CTL0 = ADC12TOVIE + ADC12ON + REF1_5V + REFON + MSC + SHT0_13;

However, if you use this approach you will inevitably end up confusing SHT00 and SHT0_0 – with disastrous and very frustrating results.

Defining Fields – method 2

In this method, one defines the bit position of the fields. Thus our definitions now look like this:

#define ADC12SC             (0)   /* ADC12 Start Conversion */
#define ENC                 (1)   /* ADC12 Enable Conversion */
#define ADC12TOVIE          (2)   /* ADC12 Timer Overflow interrupt enable */
#define ADC12OVIE           (3)   /* ADC12 Overflow interrupt enable */
#define ADC12ON             (4)   /* ADC12 On/enable */
#define REFON               (5)   /* ADC12 Reference on */
#define REF2_5V             (6)   /* ADC12 Ref */
#define MSC                 (7)   /* ADC12 Multiple Sample Conversion */
#define SHT0                (8)   /* ADC12 Sample Hold 0 */
#define SHT1                (12)  /* ADC12 Sample Hold 1 */

Our example configuration now looks like this:

ADCT12CTL0 = (1 << ADC12TOVIE) + (1 << ADC12ON) + (1 << REFON) + (0 << REF2_5V) + (1 << MSC) + (13 << SHT0);

Note that zero is given to the REF2_5V argument and 13 to the SHT0 argument. This was my preferred approach for a long time. However it had certain practical weaknesses:

  1. It relies upon the manifest constants being correct / me using the correct manifest constant. You only need to spend a few hours tracking down a bug that ends up being an incorrect #define to know how frustrating this can be.
  2. It still doesn’t really address the issue of fields that aren’t set. That is, was it my intention to leave them at zero, or was it an oversight?
  3. There is often a mismatch between what the compiler vendor calls a field and what appears in the data sheet. For example, the data sheet shows that the SHT0 field is called SHT0x. However the compiler vendor may choose to simply call this SHT0, or SHT0X etc. Thus I end up fighting compilation errors because of trivial naming mismatches.
  4. When debugging, I often end up looking at a window that tells me that ADC12CTL0 bit 6 is set – and I’m stuck trying to determine what that means. (I recognize that some debuggers will symbolically label the bits – however it isn’t universal).

Eschewing definitions

We now come to my preferred methodology. What I wanted was a method that has the following properties:

  1. It requires me to explicitly set / clear every bit.
  2. It is not susceptible to errors in definition / use of #defines.
  3. It allows easy interaction with a debugger.

This is what I ended up with:

ADC12CTL0 =
 (0u << 0) |        /* Don't start conversion yet */
 (0u << 1) |        /* Don't enable conversion yet */
 (1u << 2) |        /* Enable conversion-time-overflow interrupt */
 (0u << 3) |        /* Disable ADC12MEMx overflow-interrupt */
 (1u << 4) |        /* Turn ADC on */
 (1u << 5) |        /* Turn reference on */
 (0u << 6) |        /* Reference = 1.5V */
 (1u << 7) |        /* Automatic sample and conversion */
 (13u <<  8) |      /* Sample and hold of 13 clocks for channels 0-7 */
 (0u << 12);        /* Sample and hold of don't care clocks for channels 8-15 */

There are multiple things to note here:

  1. I have done away with the various #defines. At the end of the day, the hardware requires that bit 5 be set to turn the reference on. The best way to ensure that bit 5 is set is to explicitly set it. Now this thinking tends to fly in the face of conventional wisdom. However, having adopted this approach I have found it to be less error prone – and a lot easier to debug / maintain.
  2. Every bit position is explicitly set or cleared. This forces me to consider every bit in turn and decide what it’s appropriate value should be.
  3. The layout is important. By looking down the columns, I can check that I haven’t missed any fields. Just as important, many debuggers present the bit fields of a register as a column just like this. Thus it’s trivial to map what you see in the debugger to what you have written.
  4. The value being shifted has a ‘u’ appended to it. This keeps the MISRA folks happy – and it’s a good habit to get into.
  5. The comments are an integral part of this approach

There are still a few problems with this approach. This is what I have discovered so far:

  1. It can be tedious with a 32 bit register.
  2. Lint will complain about shifting zero (as it considers it pointless). It will also complain about shifting anything zero places (as it also considers it pointless). In which case you have to suppress these complaints. The following macros do the trick:
#define LINT_SUPPRESS(n)  /*lint --e{n} */
LINT_SUPPRESS(835)        /**< Inform Lint not to worry about zero being given as an argument to << */
LINT_SUPPRESS(845)        /**< Inform Lint not to worry about the right side of the | operator being zero */

In the next part of this article I will describe how one can extend this technique to make configuring peripherals a lot less painful.

Setting a bad example – final thoughts

Sunday, August 15th, 2010 Nigel Jones

While I am sure that I could extend the setting a bad example  series of  articles I think it’s time to move on to other topics. Before I do so I’d like to give some final thoughts. The series has generated a lot of excellent comments. While the majority have been in response to a particular coding construct, a number of readers have expressed their frustration at how pervasive this problem is with vendor supplied code. My experience agrees with this assessment. In other words while this series has taken IAR to task, I don’t have the slightest doubt that if I had bought an ARM evaluation board from Keil, ImageCraft etc that I would have found similar things to complain about. In other words my experience is the norm and not the exception. Now I don’t think I’m going too far out on a limb by observing that  the code supplied with evaluation boards is very influential in that:

  1. It is likely to find itself incorporated into hundreds, if not thousands of products.
  2. Will be used both verbatim and as a template for future code by huge numbers of inexperienced engineers.

Thus the bottom line is that the code supplied with evaluation boards needs to be of the highest quality and to incorporate as many best practices as possible. While it would be great if this blog was influential enough to cause the vendors to change their ways, I suspect that little will really happen until people start complaining. Those of you that work for large organizations which buy a commensurate number of licenses are in the best position to make the change happen by loudly complaining to your sales representative every time you find some lousy code.

As always, thanks for reading.

Setting a bad example – part 5

Wednesday, August 11th, 2010 Nigel Jones

This is the fifth 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 code excerpt is taken from usb_t9.c in the AudioDevice project.

/*************************************************************************
 * Function Name: UsbCoreReq
 * Parameters:  UsbCoreReqType_t Type
 *
 * Return: Int32U
 *
 * Description: Return device states
 *
 *************************************************************************/
Int32U UsbCoreReq (UsbCoreReqType_t Type)
{
 switch(Type)
 {
 case UsbCoreReqDevState:
   return(UsbDevCtrl.State.DS);
 case UsbCoreReqDevSusState:
   return(UsbDevCtrl.State.Suspend);
 case UsbCoreReqConfiquration:
   return(UsbDevCtrl.Configuration);
 case UsbCoreReqInterface:
   return(UsbDevCtrl.Interfaces);
 case UsbCoreReqDevOwnAddress:
   return(UsbDevCtrl.DevAdd);
 case UsbCoreReqWakeUpEnableStatus:
   return(UsbDevCtrl.Feature.RemoteWakeupEnable);
 case UsbCoreReqSelfPoweredStatus:
   return(USB_SELF_POWERED);
#if USB_HIGH_SPEED > 0
 case UsbCoreReqSpeed:
   return(UsbDevCtrl.HighSpeed);
#endif // USB_HIGH_SPEED > 0
 }
 return((Int32U) -1);
}

I suspect that on first blush many readers will not be too offended by this function. However it contains several things that are pet peeves of mine. I’ve listed them in the order that they occur in the function

  1. As with most of the code, I found the function header to be almost useless. Having read it multiple times and also having looked at the code, I’m still not really clear what the purpose of this function is other than to provide access to some of the parameters of the structure UsbDevCtrl.
  2. This function does nothing more than switch on a parameter and return a value. As I outlined in this post, IMHO one should use multiple simple functions rather than one large function such as this. The resulting code is smaller, faster and the purpose of each function is much clearer. It also avoids the problem that is discussed in point 7 below.
  3. The author has used the variable name ‘Type’. While I know this is legal and it may even be somewhat meaningful in this case, I find the idea of using ‘Type’ as a variable name just plain confusing.
  4. The function contains no-less than nine return statements. Most coding standards require that a function have a single exit point. I have to admit that if you’d asked me ten years ago whether I thought this was a good idea I’d have probably equivocated. However in recent years I have really made an effort to ensure that all my functions have a single exit point – and I have found it has improved my code a lot. Of course there are the odd cases where it really is unavoidable. However in this case, maintaining a single exit point would have been trivial.
  5. The argument to the return statement is in parentheses. This seems to be a popular construct, which I simply do not understand. What benefit do the parentheses confer? To those that ask what harm are they doing, then I would suggest that they make the code harder to read.
  6. The switch statement has no default clause. Yes, one will fall through to the last return statement – but it’s a sloppy way of handling it.
  7. The pièce de résistance is of course the cast of -1 to an unsigned integer. The function is declared as returning an unsigned integer – but in one case it returns a signed integer that is cast to unsigned. I’m sure that you will not be surprised to hear that none of the functions that call UsbCoreReq() check to see if “-1″ has been returned.

I don’t think this example is quite as horrific as some of the others I have highlighted – but it’s still pretty bad. As always your comments make this interesting!

Setting a bad example – part 4

Monday, August 9th, 2010 Nigel Jones

This is the fourth 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 VirtualCom project but can be found in almost all of the projects.

This code excerpt is taken from a file ‘Terminal_18_24x12.c”.

/*************************************************************************
 *
 *    Used with ICCARM and AARM.
 *
 *    (c) Copyright IAR Systems 2006
 *
 *    File name   : Terminal_18_24x12.c
 *    Description : Font Terminal 18 (24x12)
 *
 *    History :
 *    1. Date        : December 2, 2006
 *       Author      : Stanimir Bonev
 *       Description : Create
 *
 *    $Revision: 28532 $
 **************************************************************************/
#include "drv_glcd.h"

static const unsigned char TextStream [] = {
 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

... One thousand five hundred and thirty+ more lines of data without a comment

0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
};

FontType_t Terminal_18_24_12 =
{
 12,
 24,
 0,
 255,
 (pInt8U)TextStream,
 "Terminal 18 (24x12)"
};

Other than the header shown above, there is not a single comment in a 1566 line file. To help complete the picture, here is the excerpt from the header file drv_glcd.h which defines the data type FontType_t.

typedef struct _FontType_t {
 Int32U H_Size;
 Int32U V_Size;
 Int32U CharacterOffset;
 Int32U CharactersNuber;
 pInt8U pFontStream;
 pInt8U pFontDesc;
} FontType_t, *pFontType_t;

I have not omitted the comments – there just aren’t any.

Now it’s reasonably clear that this file is used to define a font. To illustrate my complaint with this file, I will pose what I consider to be a quite reasonable situation.

The sales department comes to engineering and says “90% of our sales for this product are in Germany. We have received complaints from Germany that the digit seven is rendered incorrectly – can you please correct it?” (For those of you that don’t know, in Germany a ’7′ is usually rendered with a  horizontal bar through the body. I would demonstrate it – but I can’t within the limitations of my editing environment).

Now the question is, how big a job should this be – and how big a job will it actually be based upon how the font has been implemented? I will let you the reader decide the answer to the first question. To the second question however, I will suggest an answer of days. The reason is that before I can possibly make the change I have to get the following questions answered:

  1. How is TextStream[] indexed? It’s likely by ASCII number – but who knows for sure?
  2. Just how many bytes are there per character?
  3. How do the bytes map onto the character – by row or by column and do they start at the bottom, the top, the left or the right?
  4. What about italics and bold? Are they included in this font definition or not? If not, is there a separate file that defines them, or is it done algorithmically?
  5. Is there a German style ’7′ already in the font map that I could just use?

All of these questions could have and should have been answered in the font file. If it was me, I would have done it via a combination of coding changes and comments. For example I might have done something like this:

static const unsigned char TextStream [N_TYPES][N_CHARS][FONT_SIZE_IN_BYTES] =
{
 /* Start of standard fonts (c.f. bold and italic) */
 {
  /*  Character 0. This is the nul character and is non-printing.
  Appears so as to make indexing into TextStream[][] easy */
  {
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
    ...

Would this have been a lot of work up-front? Absolutely. However compared to what the poor sod that has to maintain the font has to go through, it would have been time well spent. Unfortunately the industry seems to measure productivity based on how quickly code is written – and not on how easy it is to maintain. I for one would like to see it changed!

Setting a bad example – part 3

Saturday, August 7th, 2010 Nigel Jones

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.