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.
Friday, November 4, 2011
10 comments:
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!
Subscribe to:
Post Comments (Atom)
Static Analysis Ranked Defect List
Crazy idea of the day: Static Analysis Ranked Defect List. Here is a software analysis tool feature request/product idea: So many times we...
-
It is common to see small helper functions implemented as macros, especially in older C code. Everyone seems to do it. But you should ... -
(If you want to know more, see my Webinar on CRCs and checksums based on work sponsored by the FAA.) If you are looking for a lightwei... -
Oct 3, 2014: updated with video of the lecture Here is my case study talk on the Toyota unintended acceleration cases that have been in ...
F-7. Can this function be written with a single point of exit? (no returns in middle of function)
ReplyDeleteRule is a BS!
The best explanation every about the history of that anachronism:
http://programmers.stackexchange.com/a/118793
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.
DeleteNext 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).
F-8. Are all variables initialized before use?
ReplyDeleteNot applied everywhere, consider this:
bool res = false;
...
res = Foo();
Keil will give a warning "Dead Assignment Eliminated"
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 A-7. Are there any if/else structures nested more than two deep? (consecutive “else if” is OK)
ReplyDelete A-8. Are there nested switch or case statements? (they should never be nested)
What is the solution for that? My project have many conditions which must tested before take an action
Foe example my project is elevator control card " when the elevator car requested many safety inputs must tested and direction of car moving and many others before i give the motor the run decision "
Thanks
You should most likely be using statecharts instead. There is a lecture on statecharts here: https://users.ece.cmu.edu/~koopman/lectures/index.html
ReplyDeletealso, this course builds an entire elevator system using statecharts instead of flowcharts (look at the course project): https://users.ece.cmu.edu/~koopman/ece649/
If during code reviews, I frequently detect some developers of my team violating the official coding rules. But they mostly do not care to fix them and the technical leader doesn't make any comment at all; are there ways you recommend to make the team apply the coding rules? Or should I just move to another company where quality is actually respected?
ReplyDeleteMartin, I sympathize with your situation. There are many reasons this can happen, and it is sometimes more of a cultural issue than ill intent. Did you talk to the technical leader about this in private? (Or if the technical leader is not approachable that is a different kind of issue.) The ways I've seen work in the past all amount to the team members having an experience where they have come to their own conclusion that it is worth doing. Sometimes in a classroom, but more often after having someone find a bug and make the point (the "teachable moment") that this is a bug that was missed in peer review due to poor coding style.
ReplyDelete(I can't offer personal advice in this blog, but my general advice is always to see if there is a non-threatening way to improve the situation before moving. But without crossing the line that the people there will say bad things about you later.)
ReplyDeleteThanks for the reply. I talked with the Technical Lead about it, and she said she would help encourage the rules during the reviews, but it has not happened. I suspect that because of business reasons, the focus is just to have a "working software" and not much importance is placed on structural quality. I'll consider the general advice, although I think it is best to find a place where the culture of quality is respected. By the way, I recently found your blog and resources and I think they're exceptional. Thank you for sharing your knowledge and wisdom.
Delete