embedded software boot camp

Setting a bad example – part 1

Wednesday, August 4th, 2010 by Nigel Jones

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 0x12345678; 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.

38 Responses to “Setting a bad example – part 1”

  1. I am amazed at what you discovered, and equally appalled by it. What puzzles me is how anyone could even invent code like this. If a person knows enough C to construct such a monstrosity, (s)he should also realise that the job can and should be done more simply.

    The multiple assignments might be put down to obsessive optimisation and incomplete knowledge of the compiler’s capabilities in this area, but the use of the continuation character is just baffling. Until I read this, I had assumed that the continuation character was for use only within a macro definition and would be invalid anywhere else. Now that I’ve been enlightened, will I be using it outside of macros? Er.. NO!

    May I humbly suggest that your discovery itself is sufficient to trigger a call to IAR, if you haven’t already made one?

    • Nigel Jones says:

      I haven’t called IAR Peter. However I know that some of their staff read this blog so I’m sure they will hear about it. I think your point that if someone is skilled enough to write this, then they should realize that the job can and should be done more simply is a valid one. Personally as I have become more skilled in this area, I have found that I’m writing ever simpler code. In short, ‘sophisticated’ code is only acceptable if the problem demands it.

  2. Jörg Seebohn says:

    What’s even worse is if the GPIO registers are defined volatile and reading a value does not result in the same value previously written.

    Bad design.

    • Nigel Jones says:

      That’s a really good point Jörg – and one that had escaped me. The registers are indeed volatile – and thus the possibility of the author not getting what he wanted exists. A great example of why being ‘clever’ can bite you.

      • FullBandwidth says:

        But would the compiler necessarily have to generate a read? Wouldn’t the zero value already be sitting in a register and just get MOV’d out to each address?

        • Nigel Jones says:

          Yes I believe it would. I took a look at the code generated by the compiler and it does indeed perform a store followed by a read.

          • Marvin says:

            Since they’re registers, they should’ve been defined volatile. And thus yes, the compiler must generate a read from the register on the right side of the assignment.

            The continuation character, BTW, is completely useless here. The code:
            A =
            B = 0;

            is exactly the same as
            A = \
            B = 0;

  3. Things that bother me: The example here does not show what FIO?DIR, FIO?MASK, and PINSEL? are. They are allcaps which convention implies that they would be #define macros. I’m used to having direction control for all pins in one register, mask for all pins in one register, and pin selections for all pins in one register. In other words, I would have assumed that we are talking about three registers here; I would have assumed that FIO0DIR controls bit 0 of a register and that FIO1DIR controls bit 1 of the same register and so on. So the above constructs would invoke a lot of unnecessary bit manipulation code just to initialize three volatile registers. Plus it might not have resulted in complete atomic operation between each equal sign before going on to the next. But maybe the above does exist in 19 separately-addressable registers.

    Also, what happened to PINSEL5 & 6? Are there no pins 5 and 6 in the hw? Why did the example writer leave it out?

    Why do you say the line continuation exists for a good reason? The above code will compile without the backslash. Newlines are treated as ordinary and optional whitespace between statement tokens. I know it is necessary in C PreProcessor #define macros but under what conditions in C code would you have absolutely no choice but to use it?

    • Nigel Jones says:

      I didn’t want to muddle the example with too much detail. In the NXP LPC1764 there are five general purpose IO ports. There is a direction register per port and a mask register per port. However there are two pin selection registers per port. Thus there really are 19 registers. PINSEL5 and PINSEL6 are not physically implemented in the device because the pins they would configure don’t exist. Notwithstanding this it’s still always a good idea to explicitly state this via comment.

      I say that the line continuation character exists for a good reason simply because it’s needed for complex macros by the pre-processor. However to your point, off hand I can’t think of a good reason to use it in regular (non pre-processor) code. Can anyone come up with a justification for it?

  4. John Regehr says:

    I’d be interested to hear some followup from folks at IAR. Most students are unaware of the multiple-assignment form of the = operator and I certainly would never teach it to them, even for the case you mention where it’s essential that multiple registers start out with the same value. A named constant would be better. But anyway, code fragments like this are great discussion points in class. It’s not like these kids are going to get out into the real world and see only pretty code :).

  5. John Regehr says:

    By the way I hope “part 1” means there will be many more posts like this.

    • Nigel Jones says:

      There will certainly be a few more. I’m going to try and keep it to just the more egregious examples. Of course, there is a lot of the code I have not looked at yet!

  6. Bob Weiman says:

    I agree with your criticisms. I find that the quality of the software varies greatly even for the same vendor. I really hate example code that does not compile or work. You should not have to debug the example code!

    • I ended up having to debug the ST Micro hardware library for the STR750 ARM processor. A number of bugs needed to be fixed before I could use the library in my application. And to make matters worse, the USB library supplied by ST Micro needed debugging as well!

  7. groovyd says:

    Seriously, what is so hard about the hardware maker offering a good clean driver library for their hardware? Doesn’t it seem like it would vastly improve their odds of people buying their chips? And doesn’t having a good experience on one project make you want to use the same hardware on another? I mean it is something they write once and then it saves some 100,000 hours of people’s time in re-writing 10,000 different ways for 1,000 different products by 100 different companies later. In fact, taking outlining before creative writing as an example doesn’t it almost even seem like they should write the software drivers for their hardware before they design the hardware itself? Sort of a proof of concept kind of thing…

    • Nigel Jones says:

      It does seem rather obvious doesn’t it. In terms of the hardware vendors the only explanation I can come up is that firmware is their poor relation. In other words, hardware companies are about shipping silicon and so that’s where management’s attention is focused. Firmware is considered a necessary evil – and thus just enough is done to meet the market expectations. I would be interested in hearing from anyone that has come across what they consider to be very good hardware vendor supplied code – and also whether that figured in their decision to use the same hardware platform on future projects.

      • Bernhard Weller says:

        As being quite a beginner, my opinion is not based on much knowledge or understanding of good coding guidelines. But I guess, that given the fact I’m a beginner, code which I’m easily able to understand is some form of good code.

        Thus said, I really like the code examples provided by Texas Instruments for the MSP430 line. They deliver quite a bunch of mini-examples which cover like every module available in the derivate you chose. There is a nice description at the top of the file, and most examples also try to optimize the current consumption of the system.

        But then again, there is no such thing as using functions in these mini-examples, which most likely makes creating understandable and readable code much easier.

        • Nigel Jones says:

          Interesting. I do a lot of MSP430 work and I don’t like what TI provides -mainly because the code is often written without explanation.

        • Bernhard Weller says:

          Okay I recently had a lot of trouble with the defines of the MSP430 header files coming with the Code Composer Studio (CCS), I also studied some more examples for another model of the line and well, I must change my above mentioned opinion.
          Some things are truly horrible.

          First about the problems with the defines I had. The MSP430 line has an information memory segment in flash where it stores some calibration values, like for the digitally controlled oscillator. This is especially useful in serial communication via UART as you’ll have a reasonably precise timebase for most common baudrates.
          Now what you usually do is setting two control registers to the calibrated values like this:
          // Setup MCLK = 1 MHz
          BCSCTL1 = CALBC1_1MHZ;
          DCOCTL = CALDCO_1MHZ;
          Now as CCS is based on Eclipse, it supports such things as auto-completion, as I wrote my initialization routine it proposed the defines and I used them. What I got was:
          // Setup MCLK = 1 MHz
          BCSCTL1 = CAL_BC1_1MHZ;
          DCOCTL = CAL_DCO_1MHZ;
          This one has an added underscore in it. Well I haven’t put much thought into this at first, in fact I didn’t notice at all.
          As I got to testing my board, I received a lot of garbage but the bytecount was always correct, now this was really something awful to debug. I changed the MCU just in case I wasn’t careful enough during soldering. And magically it worked. But 4 other boards didn’t, and it’s not like I would kill 5 MCUs in a row.

          So what actually is happening, the define with the underscores indicates an offset in the flash of the calibration-data. But just from the defines name, you can’t tell at all. Unluckily in the case of 1MHz the DCO gets set to a value of about 1MHz even if you put the offset into the register instead of the actual value.

          I think this misleading define names in a header file which gets integrated into every project, are leading to a lot of problems, and it might be that there are products out there using the calibration offset instead of the actual value for the DCO.

          Another thing in the examples is the polling done on status bits, there is nothing wrong with polling (except for the common known issues), but the way it’s implemented:
          while(UCB0CTL1 & UCTXSTT); // Start condition sent?
          (taken from MSP430x54x_uscib0_i2c_04.c in the example codes supplied with MSP430F5418)

          You see, it’s just the semicolon at the end of the line indicating that this thing is actually waiting until the bit is no longer set. This can get lost pretty easy and it should be something more like this:
          while( (UCB0CTL1 & UCTXSTT) > 0)
          {
          // Wait until start condition was sent
          ;
          }

          So well I guess reading through a lot of the posts here made me more perceptive on problems and raised my standards on what good code should look like.

  8. Mick says:

    Currently, I interface a Renesas uC to many 3rd-party resources (cell modems, GPS, RFID, WiFi, etc.) and have been very dissatisfied with the availabilty of drivers for all of them (incl the micro), other than the c-code source I received for a SAVI RFID reader, years ago, which was very complete.

    It seems to me that the manufacturers could save time for not only developers (such as me), but *themselves* if they had a set of already written (well-documented!) drivers. Developers (me) may not realize the subtleties necessary for the low-level drivers (exact timing, polarity of signals, dependencies, etc.) that would be hard to fully describe in a written spec, but could be handled well by well-written code (a “picture” being worth a thousand words). Knowing how their hardware works, the mfrs are probably the best ones, barring outright incompetence, to write the low-level stuff.

    I imagine that these manufacturers’ staffs get inundated with questions regarding the right way to write an interface to their hardware. Seems to me the vast majority of hardware manufacturers are loathe to provide this kind of documentation. Maybe they like answering the same questions over and over, or maybe the lawyers told them they’d be more culpable in the event of a mistake – my guess is they think they might be giving away too many trade secrets if the code is too complete…so, “why bother?”

  9. dwhite says:

    I found a fundamental error in example code from Keil for the NXP1758 (MCB1700 CAN.uvproj specifically). The example sends and receives CAN messages at 500k baud but the actual baud rate is just far enough off to cause occasional errors when tested with a true 500k baud device or oscilloscope. Since the example sets both ports to the same incorrect baud rate and sends from one while receiving from the other, the example works error free.

    This is a cautionary tale for younger programmers. It seems that many new graduates (and managers) tend to want to download examples and get something running quickly without taking the time to make sure it is right.
    Assume NOTHING until you verify it yourself.
    Yes, it takes a lot of time to check every register setting but imagine the pain in trying to find a problem like this in the field with hardware and software engineers pointing fingers, etc.

  10. Weatherbee says:

    This is an obvious attempt by someone at optimization. The line continuation characters are completely unnecessary. If they really want to “optimize” a statement like this (at least for code size), then it would make more sense to put it in a for loop like:

    int *regs[] = { &FIO0DIR , &FIO1DIR , &FIO2DIR , &FIO3DIR , &FIO4DIR , &FIO0MASK, &FIO1MASK … };
    for (n=0; n<sizeoof(regs)/sizeof(regs[0]); n++) *regs[n]=0;

    For even more implementation dependent compression initialize a pointer to the lowest register in the region and then create an array of the differences between the addresses of the registers you want to clear, that way you might be able to have each table entry be an unsigned char or short or something which would use less memory.

  11. Ashleigh says:

    I’d like to pick up on the volatile thing mentioned above.

    The code as written can perfectly legally be turned by a compiler into a sequence of operations:

    Store 0 to register
    Read register
    Store to next register
    Read register
    Store to next register

    And so on. It can also be legally turned into:

    Store 0 to temp location
    Copy temp location to first register
    Copy temp location to second register
    etc

    And this latter is probably what the author hoped to achieve. The former may well (for example on an I/O port) end up doing a read of the physical port as well as a write – and you won’t know unless you look at the generated assembler code. WORSE – the behaviour of the code may change from one silicon rev to another!

    The point here though, is that EITHER construct is valid; when writing code the author should always be considering what the compiler MIGHT generate and considering the possibility of unforseen consequences. Checking the generated code is probably also a good idea. Letting the compiler optimise instead of the person is frequently the better practice.

    In general you are far better off using simple constructs even though the source code might not look as “efficient”. Firstly, it makes you intent very clear, and secondly it allows the compiler to sensibly optimise where that can be done. Even with all those registers declared volatile, a good optimising compiler will generate the code in my second example above when given a source of “X = 0; Y = 0; Z = 0;” – or possibly something optimised even better depending on the instruction set.

    This is a really good example of somebody trying to be too clever and hoping to force the compiler into a way of doing business instead of going for simplicity and clarity and letting the compiler do what it does best. (Even more surprising that this comes from IAR given that their compilers have some of the best optmisers I have ever come across).

    • Nigel Jones says:

      Beautifully put Ashleigh. I have confirmed that the compiler does do the first code sequence. Thus the author hit the trifecta of complicated code that might not work and is inefficient to boot.

    • David Brown says:

      The registers are defined as volatile, and the C standards are totally clear on the matter – a statement like “v1 = v2 = 0” means the same as “v2 = 0; v1 = v2” and thus only your first interpretation is correct.

      The code is therefore wrong.

  12. DyadyaSportivnihShtanah says:

    This is good

  13. arwcw says:

    When I saw the title of your blog post, I thought it would be something really seriously bad you’d found in their code. This is really quite trivial and not worth complaining about, in my opinion.

  14. alb says:

    I agree with arwcw. If you think this is worth complaining about, you need to read *a lot* more code. Everything in this code is legal; it doesn’t depend on side effects; and is clear to anyone who knows the language.

    Your preference for separate initialization of unrelated variables is just that, a preference. I expect you’ll not hear from IAR, because your entire column is about coding style, even though at the beginning you specifically say it won’t be.

    Read more code. Lots more. The quality of your complaints will improve dramatically. Truly, this is small potatoes.

  15. Suresh Shukla says:

    I agree with arwcw and alb.
    Absolutely no problems with this code. Its just a matter of style preference.

  16. Ashleigh says:

    Given that Nigel looked at the generated code and confirmed that it does in fact do as my first example (a few comments up) suggested it might, this actually shows that this particular example is a DISASTER of unintended consequences, not the trivial things suggested by commenters “arwew” and “alb”.

    Without descending into a slanging match, I’d actually like to suggest that they think through the article and the implications more.

  17. FrankSansC says:

    I must be really dumb but I don’t understand the difference between this :

    uint16_t foo, bar;
    foo = bar = 21;

    and this :

    uint16_t foo, bar;
    foo = 14;
    bar = 14;

    I’ve looked at the assembly code and here’s what I’ve got :

    // 351 uint16_t foo, bar;
    // 352 foo = bar = 21; /* foo and bar must be equal to each other and start at 21 */
    MOVS R0,#+21
    MOVS R6,R0
    MOVS R7,R0

    // 351 uint16_t foo, bar;
    // 352 foo = 14;
    MOVS R0,#+14
    MOVS R7,R0
    // 353 bar = 14;
    MOVS R0,#+14
    MOVS R6,R0

    Except that there’s one more instruction in the 2nd case, “foo” and “bar” have the same value at the end. I’m sorry, I’m sure that it must be “blindly obvious” but I don’t get it…

    • Bernhard Weller says:

      The difference is not in the result you get (which is both variables are 14 or 21) but the intent of the programmer made clear.
      In the first case, changing the value of 21 to anything else will lead to both variables containing this value. This might be necessary for some kind of algorithm or something. With the first code, you will assure that both have the same value, if someone feels the need to change foo to 35, but didn’t really get the point with the algorithm, he just changes it, and it still works because you programmed it that way.
      In the other case, you just happen to initialize both variables with the same value. Maybe at a later point you decide that variable foo should be 21 instead of 14, but bar should stay at 14. You can then easily change it, without having to change the code.

  18. Andrew Neil says:

    Yes, I have been similarly apalled at the state of much so-called “example” code – see:
    http://www.8052.com/forumchat/read/181034

    Example code needs to be, well – exemplary!

    A.

  19. Andreas says:

    A bit late to the party, but I personally use right associativity of the “=” operator only if it’s paramount that multiple variables be initialized to the same value (as in the example given by Neil) or if multiple variables have the “same” value, that comes from the same source and means the same thing. For example, we discern between GUI variables that are only being used for display and the “internal” variables that are used for computation, so if the user edits a value on the GUI and confirms his edit, there’s some code akin to

    internalVariableX = GUIVariableX = valueOfEditBuffer;

    because they’re both supposed to hold the SAME value, as opposed to simply being of EQUAL value.

Leave a Reply