More bug-killing standards for firmware coding

5/6/2009 12:52 PM EDT

Brace style and in fact most of the "Look and Feel" of code is highly personal. You can spend long hours arguing about it and never convince anyone. If you are consulting, you will do it the way the client wants it or you will find they don't stay your client for long.

If your employer already has a standard, use it. If there is no standard, define one for as wide a group as possible. This may be just yourself or it could be for the whole company. Enforce it across the entire group with "pretty printer" such as indent. Everyone can then write code how they want to, until it is ready for configuration management or review (which ever is earlier) Before submitting the code, run it through the pretty printer. At that point, everyone's code should look the same.

If a developer REALLY disagrees with the standard, they could write there own rules for the pretty printer and run the code through it when they check it out, then run the standard again when it gets checked in.

Eventually, everyone will start to write code that looks like the pretty printer output.

Also, I submit rule 16. No code should ever be allowed which causes a compiler warning.

This should be obvious, but most large systems have some code which causes compiler warnings. These can range form the benign such as a variable not used in a function to the critical such as a pointer not initialized before use.

I worked on one large system (>1,000,000 lines not including comments) which had about 800 unique warnings in the build (A warning in a header file occurs every time the file is included. Yikes!). I categorized the warnings from critical to benign and began to look at the critical issues. I had just fixed a warning for the use of a pointer before it was initialized, when I received a field report of a system reset, caused by the very problem I had just fixed.

5/7/2009 4:04 AM EDT

I'd like to add rule 16:

The use of conditionals without operators is forbidden:

if (x) is not permitted.

I've seen code where "x" above was enumerated and used without an operator. It worked fine for years when the enumeration only had 2 values that by happy coincidence were 0 and 1. when a 3rd enumerant with value 2 was added, guess what broke?

And the coding standard we use did forbid this but it was done anyhow.

Always use

if (x operator variable-or-value)
{
stuff ;
}

5/7/2009 4:09 AM EDT

Oh, and rule 17:

When you export variables from a code unit, they go in a header file. (we all get that one).

17A: executable code never, ever, ever goes in a header file. (seen that. disaster in the making).

17B: when you put exported stuff in a header file, always, always always, include the header into the code unit to which that header applies.

fred.h declares:

extern char fred;

fred.c does:

#include "fred.h"

and then later:

char fred;

The compiler here is your friend. It helps to detect things like type mismatches between the header and the C file (of the item "fred" in this case). In the case of functions, the compiler can detect and warn you of mismatched parameters and return types.

Doing this saves hours, and hours, in tracking down strange defects. And it costs nothing.

(I might also add:

17C: Always include your header files into a C file using ALPHABETICALY order. It makes finding whats there very easy. And if you have header files that must be included in a specific order you have other far more fundamental problems to sort out... so go fix those, and then use alpha order.)

5/7/2009 4:16 AM EDT

I'll add another one thats maybe contentious. But especially for small embedded systems:

No dynamic or heap memory under any circumstances, ever.

Exception 1: If you need dynamic allocation, then make sure you have the source code for your run-time, then read the source for malloc and free and get a good understanding of how they work. Make sure free space is consolidated. If you can't get the source, can't understand it, or it does not consolidate free space, then don't use it. See exception 2.

Exception 2: If you must have a dynamic allocator and you have less than 190% trust in the one provided by your vendor, then write your own. It's not all that hard and you remain in total control. Dish things out from an integer or char array that you declare of a fixed size.

COROLLARY: Don't use recursion. (Exception: if you can't avoid recursion then include EXPLICIT code to monitor the depth of the recursive calls, and abort if it gets too deep.)

5/7/2009 5:39 PM EDT

IMHO there is an exception to rule 13 when code space is severely limited. Unlike INIT variables, BSS variables do not require a copy in ROM. If you know for a fact (don't assume, check!) that the startup code initializes the BSS segment to zero, then it's ok to rely on it; just do everybody a favor by documenting your assumptions with a good comment.

5/7/2009 5:48 PM EDT

About rule 14: Global variables can be mostly eliminated by clever use of C++-like techniques such as accessor functions. C99 supports inline functions which makes accessor functions essentially "free" as far as CPU code and time is concerned; if you need to protect a global across context changes (or interrupt handlers), then the accessor can use critical sections or any mechanism provided by the kernel (semaphores...)

5/7/2009 6:01 PM EDT

Sorry, but I have a small problem with rule 15 as well...

I like code that reads like a normal sentence. When "if variable x is equal to constant zero" conveys exactly my intent, I should be able to write it as such: "if (x == 0)". I understand your concern, but I don't think there is anything valuable to gain by allowing embedded assignments such as "if(x=c)" so these should be flagged by the automated tool and eliminated. A lot of compilers already warn you about those. And I agree with Bailey2468: don't ignore warnings!

6/4/2009 1:03 PM EDT

marvin99,

The best case I've seen for using assignment in conditionals is when code follows this structure:

if ((status = Step1()) == FAILURE)
{
warn("Step 1: %s\n", msg(status));
}
else if ((status = Step2()) == FAILURE)
{
warn("Step 2: %s\n", msg(status));
}
else if ((status = Step3()) == FAILURE)
...

6/26/2009 3:07 PM EDT

Rule 15 makes even less sense since it applies to inequalities as well.

if (6 > x)

looks bizarre. Agreed with marvin99, it's unneeded if you don't allow assignment within and if statement.

10/20/2009 5:19 PM EDT

Concerning Bailey2468's comment that you should not include code in headers. While normal code should not be included any inline code must be included in a header. I understand that there are some compilers that handle this extremely poorly (putting a static instance in every object module), however, that is the only way inlines can be inlined.

Concerning MrQ "17C" - Many coding standards require that no header file include another. I happen to agree with that as I have seen this cause many unexpected issues. I believe it is not practical to assume you can always include things in any named order for this and other reasons. I see no advantage to including things in alphabetical order.

Concerning marvin99's assertion that having access functions for everything that is global and that they are "free". If you do that you are just fooling yourself into believing you have solved the problem. If global variables are "memory mapped IO" then wrapping them in a function doesn't help at all unless that function prevents access by all the other asynchronous processes.

6/29/2010 11:09 AM EDT

q