Archive for the ‘Compilers / Tools’ Category

A volatile tempest

Monday, September 27th, 2010 Nigel Jones

Regular readers will know that I often comment on the use of volatile in embedded systems. As a result I am occasionally contacted about my opinion on whether a compiler is generating correct code – particularly when hardware is being accessed. Well I was contacted last week by Ratish Punoose who had a classic problem in that his code compiled okay on GCC but not on IAR. He had contacted IAR, who in turn basically said the compiler is correct – and here is the explanation. Ratish turned to me and John Regehr for our opinions. Well John and I came to similar opinions – namely:

  1. Ratish’s code was a bit weird, but not dramatically so.
  2. The explanation from IAR made no sense.
  3. It did indeed appear to be a compiler bug.

Ratish then posted his issue to the Msp430 forum on Yahoo. You can read his post and the responses here.

I’m sure many of you are at this point thinking that IAR is in for another round of bashing from me. Well you’d be wrong. One of the first responders to Ratish’s post was Paul Curtis of Rowley compilers. Paul gives an admirable explanation as to why Ratish’s code is wrong (and by extension so am I). Now I’m sure that IAR and Rowley are fierce competitors, and so Paul is also to be commended for leaping to the defense of IAR.

Furthermore, later in the thread Anders Lindgren of IAR chimes in and adds his detailed and compelling explanation.

Having read the posts from Paul and Anders I think they are right and I’m wrong. So thanks Gentlemen for:

  1. Setting me straight
  2. Proving that in the wonderful world of volatile accesses, there is always something more to learn.

I think there are several other lessons to be learned from this episode. However I think I’ll save them for another post.

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.