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.
- 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.
- 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!
- 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:
- 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.
- 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.
- 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.
- 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.
- 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.
Nice one!
I wonder how many of these kind of practices is around. It all makes me think that hardware is overrun all the time. In my short experience in embedded systems (5 years) I have seen on-flight code that does all the job inside a one and only interrupt (no tasks…), source files of 7000 lines, not using version control systems and sharing files by email… and a lot of other horrible things.
I am not sure if this is a matter of inexperience, lack of knowledge or that they just don’t care… if it works, don’t touch it.
I don’t think your experience is unusual. Part of the problem is that there aren’t enough examples available to show how things should be done – which is a large part of my motivation for this series of posts.
Hi Nigel,
I’m liking these almost daily posts you’ve been doing recently! 🙂
I also am a firm believer of using the “require prototypes” checkbox, and use it in all of my projects. As for why IAR doesn’t default with this checked, its most likely because they are trying to minimize service calls. As a serious firmware developer, I’m sure you are an active forum reader for most of the processors you specialize in. So I’m sure you are familiar with the amount of people that post on the forums with a cursory understanding of the C language.
I’ve even seen posts of people complaining about sample code they received that had “require prototypes” checked, and they didn’t understand what it meant or how to fix it! So there you go! I guarantee that if they defaulted with it set, it would be a nightmare for them.
Minimizing service calls is also likely related to the reason that the IAR MSP430 compiler disables interrupts when doing multiplies using the hardware multiplier. If they didn’t disable interrupts, people would mess it up (task and ISR both doing multiplies, which would corrupt the results), and they would have customers complaining that their compiler didn’t work (or bothering the silicon vendor asserting the chip had bugs).
I would prefer a compiler option for this because I always try to minimize the amount of time that interrupts are disabled. Perhaps we’ll have one someday. In the meantime, I can schedule my multiplies so they don’t obstruct my ISRs (or write my own multiply routines, heavily borrowed from IAR).
I’m glad you are liking the frequent posts Jeff. I haven’t done a lot of posting in the last few months, and so I see this mini-surge as a regression to the mean type of exercise. Incidentally, I don’t spend a lot of time on the forums. It’s mainly because the investment in time just doesn’t produce enough benefits – and this occurs because there is so much dross posted. Anyway, I suspect you are correct about the IAR ‘require prototypes’ checkbox. I actually have a lot of sympathy for them in this regard. Should they do what customers want, or should they do what is right? It’s easy to take the moral high ground – but not so easy to make a payroll every month.
I always put static and non-static utility functions at the top. The high level functions are at the bottom. This way I don’t need to write and maintain prototype declarations at the top. I don’t see why this would be a problem, when applied consistently?
Hi Kasper – I laid out my position on this topic in this post. There was a very good discussion thread that followed that I recommend you read. When you are done I’m sure you will still disagree with me! Seriously, one of the reasons I write this blog is that as a consultant it’s very easy for me to get isolated. By airing my ideas on a blog, I’m fortunate to get some very thoughtful responses from the readers – which in turn makes me reconsider some of my own ideas. Incidentally, I know I’m biased, but has anyone else noticed the incredibly high quality of the comments that are posted here? I don’t know for sure, but I strongly suspect that many of the regular readers come here more for the comments than the articles!
To me it would be an issue of maintainability. I put static prototypes at the top of the module along with short comments on what they do, followed by initialization functions (usually very short, no more than one or two of them typically), followed by the global functions. The statics always come last.
This keeps me from having to search through a pile of utility functions in order to find the higher level functions. The statics are usually very small functions that are easy to read and understand, but they get in the way and make it difficult to find the high level functions. In a source file containing up to 3000 lines, this may translate into a significant amount of search time in the editor.
After having done it both ways, I feel that putting the statics at the end makes the code much more maintainable.
I couldn’t have put it better!
I was brought up writing Pascal, where the mantra was “you don’t use a forward declaration”!!
That lives with me still. So I have to disagree here – I really dislike C code with prototypes for the functions that follow, it leads to 2 instances of the same thing that have to be maintained in the long run.
I’ve settled on a method where every code unit has an INIT and SHUTDOWN entry point, and these always appear last (bottom) and 2nd last respectively – so easy to find. All local functions are static and have a huge comment box at the beginning of the block where they appear, then all exported functions (again separated by a huge comment block).
This approach, as well as a function naming convention (the topic, I’m sure, of a completely separate set of comments for another day) means that in general everything is easy to find. Using a modern GUI editor, or emacs, its usually only a click to find where anything actually is (and going to the code not the prototype is helpful too).
So, Nigel, we’ll agree to differ on this one.
Fair enough! On a more serious note, I always respect someone that does something based on a set of principles. What drives me nuts is when someone does something because that’s the way they learned to do it, without giving it an iota of thought as to whether what they are doing is good, bad or indifferent.
I also put my statics at the bottom for the same reasons. After writing them once do I ever even need to look at them again? And since my editor tends to open documents from the top I never have to when scrolling on down to the functions I care about. Alot of people may argue that today’s editors jump you around by context in a click so you never have to scroll find your functions but for some reason I still do. I think there is something to be said about how the order of functions lends to readability as well.
I also try to put the main functions as close to the top as possible, while everything said about modern IDEs may be true, I know quite some people who simply use a text-editor with syntax highlighting for taking a peek in other peoples sourcecode.
And there is most likely no fancy “Goto definition” thing implemented, so I try to make it easy for the reader to get a feeling of the flow of the program. And the flow is all handled in those main functions and not in the helper ones. As groovyd stated, most editors show the contents from top and not from bottom of the file. So they will start scrolling down and if they reach the main part faster, that’s a good thing.
One thing that keeps bothering me is the placement of global variables. I don’t know where to put them so that the readability and maintainability of the code is improved. Right now I put them in the main file, and in other modules needing them I declare them as extern. But somehow this just doesn’t feel good.
I’m glad that the compiler I first got to use gave me at least warnings about implicit declared functions, which led me on the search on what is going on, and I finally found out that it requires function prototypes. The move from oo-programming with Java to embedded software using C is not as easy as some people say.
I really like your blog as it helps me a lot with stuff I really didn’t have to fight with while programming oop.
You can solve your global variable problem by not using any :-). Seriously, there isn’t a great solution to the issue you have raised. For what it is worth, I too normally define all global variables in main.c and then extern them elsewhere. The best thing I can say about this practice is that people nearly always look at main.c – and thus they are told immediately about the globals in the project. Anyway, I’m glad you like the blog
I agree that globals are BAD and should be avoided at all costs. However (saw that coming, didn’t you), if you do need a global variable, it should be owned by some module other than main. There is some module that is more responsible for that variable than any other. Typically this would be the module that either changes the data regularly, or the module that does some heavy processing on the data. Once you identify the module that needs to own that variable, then that module should declare the variable in it’s header file and define it in it’s source file. Essentially the module is saying to the world, “I have this variable that you might want to know about” just like it does with the functions that are declared in it’s header file.
All that being said, I have to circle back around and say, “Don’t use globals”! The only time I’ve used globals in the last fifteen years was when I literally ran out of code space and had to remove some of my accessor functions to free up a few bytes.
For exported variables I declare them “extern” in the header of the code unit, and then declare then in the body of the that code unit as well.
I actually go one step further and have a set of universal defines from a “common” include files that goes into everything. This declares two names:
export – which is a verb “please export this to the world”
and
exported – which is a noun: “this is a thing that has been exported”
In the header file I declare a global as “exported” and in the body as “export”. I find this adds clarity.
The #defines are just:
#define export
#define exported extern
I’m not usually a fan of redefining key words. However I think this has merit. ‘extern’ is not exactly descriptive, whereas ‘exported’ is. All in all an interesting suggestion.
This was something that came after much confusion and mucking about.
I’m not much of a fan of redefinition either, but settled in the end on this as the least of several evils.
At one time I dumped the word “static” and used “private” everywhere, but then C++ compilers gave me hell, so that one ended going back to static again. I don’t really like static as a keyword, its not really descriptive under ALL circumstances either.
I know this is a little late, but…
In your comment below you stated that you also had ‘private’ replacing ‘static’ but that c++ compilers gave you trouble. Not sure if you/others are aware that export is a reserved keyword in c++?!
Is there a reason that you have these as lower case and not upper? ie
#define EXPORT
#define EXPORTED extern
and then perhaps
#define PRIVATE static
or eve
#define INTERNAL static (in case you still wanted lowercase…)
As upper case I would find it more obvious that these are user based #defines and not some compiler extension.
otherwise this is an interesting idea/concept, especially the other tip you have listed below with the Fast_Int. I’m always looking for ways to make code more portable and this looks like a promising idea – thanks for sharing!
Agreed about the unnecessary exposure of implementation details in the header file.
As another nitpick, I would prefer the function was named I2C_Enable() (removing the redundant “I2C”). But, that still doesn’t tell the user of this code what the function *does*. Does it set the GPIO pins to the I2C mode? Does it initialize a buffer of some sort? Does this enable a master or slave mode?
Great post. Keep them coming, even though they give me a headache… 🙂
You should have seen the headache I got looking through all of the code!
I think the mysterious use of Int32 in this and previous examples is an attempt by the author to optimize for the ARM architecture, which likes to work with 32-bit words. I sometimes find myself doing this sort of thing on very low-end processors where every cycle counts, or where I’m trying to get maximum possible performance from a ‘bottleneck’ without resorting to assembler. However, when you’re using a heavyweight processor like this for a very undemanding application, it’s more important – as you point out – to make sure the parameter width makes sense.
I notice, incidentally, that none of these functions do even basic bounds-checking on parameters. Personally, I always do a basic check on pointers at the very least, to make sure they are not NULL and are within the range of known RAM.
This thing about “optimal” sized ints for operations can be solved – in the place where Int32 and the various data types are defined, define a type called “Fast_Int” or “Optimal_Int” and use that everywhere. Then when porting to other machines, that type can be defined appropriately to the machine type. JUST MAKE SURE YOU COMMENT THE USE AND UPPER BOUND!
The use of Int32 and similar names should be confined to legacy code, along with the idea of defining your own types called Fast_Int or whatever.
This is 2010 – the C99 standard has been in common use for several years now (sarcasm intended). In particular, has existed for ages and even if you have to deal with a primitive compiler without it, there is no reason why you can’t write your own version.
When you want a 32-bit integer, the correct type is int32_t or uint32_t. When you want a fast unsigned integer supporting up to 8 bit values, use uint_fast8_t. The type name says what you want, it does what you want, it’s portable across compilers and targets, and it’s consistent for every modern developer.
Nigel,
In the past I had at least once a discussion about the location of defines, when the scope for them was only one source file. I agree with you that in that case they shall simply be in that one source file. But my experience is that some folks have been tought “defines go into header files”. And for whatever reason, its hardly impossible to talk them out of that mantra….. Not to defend, maybe to understand the writer, he probably had the same teacher 🙁
They can talk themselves out of putting defines in header files. All they need to do is have a project with 200+ source files where all sources call one header file with a define in it that affects only one source file…and then change the define! It’s more entertaining if they have a slow computer. 😀 It happened here at our company, and believe me the guy is now sold on putting the define in the source file!
That’s certainly one way of getting the message across!
Hi Nigel,
Thank you for writing about the code review you are doing on the library! From you I learned about the compiler setting for ‘Require prototypes’ for IAR and turned it on in my current project, and set to work making the warnings go away. I also added the GCC compiler warning, “-Wmissing-prototypes”, for an open source project that I work on. It is a library, and it is critically important that the source functions match the header API.
Best Regards,
Steve