Friday, November 4, 2011

Embedded System Code Review Checklist

This past summer I had the opportunity to work with my friend Gautam Khattak on several industry design reviews. By the time the dust had settled, we'd had a lot of time to think about patterns of common problems and the types of things that designers should be looking for in peer reviews for embedded system software.

As a result, we've written a two-page checklist for embedded system code reviews that you are free to use however you like.
The sections include: function, style, architecture, exception handling, timing, validation/test, and hardware. This is a pretty thorough starting point, but no doubt you will want to tailor these to your specific situation.

The checklists are broken into sections to make it easier to do perspective-based reviews. The idea is to divide up the sections among 3 or 4 reviewers so that each reviewer is thinking about somewhat different aspects of the code, resulting in better review coverage of potential issues.

Everyone has their own favorite thing to look for in a review. If you think we've missed one please let us know with a comment. One thing we have intentionally left out is system-level problems that are better done in a system-level review. These are primarily intended for module reviews that look at a couple hundred lines of code at a time.

Again, you can use these within your company or for any other purpose without having to ask our permission. But we'd be happy to hear from you if you've found them especially useful or if you have suggestions for things we've missed.

4 comments:

  1. F-7. Can this function be written with a single point of exit? (no returns in middle of function)

    Rule is a BS!
    The best explanation every about the history of that anachronism:
    http://programmers.stackexchange.com/a/118793

    ReplyDelete
    Replies
    1. Hmm, well, that is only one explanation. The very next reply in that same stackexchange posting explains that funneling everything to the same return-from point makes it easier to clean up loose ends, which is more along the lines of why I make this recommendation. Additionally, in code with returns all over the place it is often the case that the code is unstructured in general (if it were structured, you wouldn't need all the returns). If you are a relative beginner I highly recommend this rule despite what you say. If you are advanced then well you aren't going to follow this advice, but don't forget that beginners are often the folks how are going to have to maintain your code later.

      Next time instead of saying something is BS perhaps instead explain why you think it is misguided in more specific terms, and acknowledge others who might have contrary opinions (and, ideally, why you think they are misguided).

      Delete
  2. F-8. Are all variables initialized before use?

    Not applied everywhere, consider this:

    bool res = false;
    ...
    res = Foo();

    Keil will give a warning "Dead Assignment Eliminated"

    ReplyDelete
    Replies
    1. Most embedded compilers have terribly weak warnings. If you are 100% sure your compiler will warn you about all possible uninitialized variables, and that you will actually pay attention to the warning, then it's OK to tailor the list to remove this point. I have seen WAY too many embedded systems where the compiler didn't have the warning, didn't initialize, and sure enough there was a bug there.

      Delete

Please send me your comments. I read all of them, and I appreciate them. To control spam I manually approve comments before they show up. It might take a while to respond. I appreciate generic "I like this post" comments, but I don't publish non-substantive comments like that.

If you prefer, or want a personal response, you can send e-mail to comments@koopman.us.
If you want a personal response please make sure to include your e-mail reply address. Thanks!