embedded software boot camp

Setting a bad example – part 4

Monday, August 9th, 2010 by 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!

11 Responses to “Setting a bad example – part 4”

  1. groovyd says:

    Hey Nigel,

    I really value your opinions on coding and was wondering if I were to email you some of mine would you give me an honest critique? I understand if you are very busy but would very much appreciate it.

    -Daryl

    • Nigel Jones says:

      I normally charge people to look at their code 🙂 – but in your case Daryl I appreciate some of the many insightful comments you have posted on this blog. So please go ahead and send me some of your code. A quick turn around is not guaranteed!

  2. D Wellesley says:

    Wow, thank you so much for this set of articles. I purchased an IAR kickstart kit some years ago and pulled my hair out trying to understand some of their libraries. When I tried to get help from them on porting FreeRTOS I Gave up alltogether. I would like to hear further comments regarding blocking vs. non blocking code in IAR’s libraries.

    • Nigel Jones says:

      To be clear – this series is not about IAR’s library code – it’s about code supplied with an IAR development board. I haven’t delved into the IAR library code too much, but when I have it has seemed OK. In terms of library functions, IAR does a good job of stating which functions are re-entrant. Off hand I can’t remember if they ever mention if a library function blocks.

      • D Wellesley says:

        OOps, I meant to say example code not libraries(sorry). specifically drivers. My experience was with a LCD display driver. I2C if I remember correctly.

  3. Laz says:

    Having modified font pixel files before, even if it was well commented, it’s not so easy to mess with…

    With regard to filling out structure parameters, I often make a copy of the structure template, and then just put comments between the value and the name. That way I never miss one or get mis-aligned, and I always know the variable type (pointer or not, etc).

    FontType_t Terminal_18_24_12 =
    {
    12, // Int32U H_Size;
    24, // Int32U V_Size;
    0, // Int32U CharacterOffset;

    … etc

  4. Tony says:

    I think this one is nit-picking. Firstly, there’s a good chance the font data was generated using an automatic tool. You shouldn’t be fiddling with the raw data. Secondly, font rendering in general is a very application-specific thing and can get immensely complicated depending on the requirements. The author is obviously just trying to do the bare minimum – I’m guessing the font rendering per se is not the point of the example but an irritiating necessity. Sometimes, the excuse “it’s only an example” is valid.

    • Nigel Jones says:

      I think this is a fair point Tony. However on the assumption that the font was generated by some sort of tool then I think the comment block should say so, and also tell you what the tool is, where to find it etc so that the font can be maintained.

  5. Mac says:

    I suspect it was generated automagically by a tool – possibly even just an Excel file.

    However, that makes it worse. Code that is generated by a tool *MUST* have a comment to explain it – a simple:

    //
    // This excerpt was generated from the Excel file :
    //
    // (V0.01)
    (Huge File)
    // — End of Automatically generated section —

    That way anyone can see it was generated and hunt down the Excel file (or tool) – which should be archived in the version control system along with the code.

    The automatic generator can easily add a comment to each line it’s generating. eg: // Ascii[###] = ‘F’

    That way then when someone inevitably hand-tweaks a line they can do it easily.
    If someone hand edits the output (as we all do from time to time) they can add a simple note.

  6. To me it looks like unfinished. May be the code was copy/pasted from other project and the comments were left for later, then forgotten.

Leave a Reply to groovyd

You must be logged in to post a comment.