This was the first leap year since Zune was released, and yesterday was the 366th day of the year, sooooo....

Also spotted in this code: several "goto"s.

Consider the function OALIoCtlHalInitRTC, lines 122 and 139-143. The last else block has no obvious terminator, but it looks like the else is empty because of the comment and the indentation. So the line after the label cleanUp is the body of the else. This means that if the if on line 131 is true, the cleanUp code never gets executed. Now this may be intentional (but I doubt it since the label is named cleanUp), but that should be documented in the code.

I consider this use of a goto to be legitimate, since it avoids duplication of the cleanup code (and the last thing you want is to have multiple similar blocks of code that need to be maintained/modified in tandem). This idiom often appears in the linux kernel IIRC. But this code is quite terrible since it doesn't use the idiom correctly, produces an unexpected execution path, is a problem to maintain, and is a problem to read. It seems that it ends up being a no-op since the line after the cleanUp label is a debugging line or something, but that doesn't make this a good practice.

But really, this isn't really a problem with the use of goto, it's a problem with the optional braces on single statement blocks in C. The goto and the label chosen just make it harder to spot. I've made it a habit to use braces around all blocks, even empty ones, to make the intent clearer and avoid this problem.

The rest reads "HAL (hardware abstraction layer) initialize RTC (real-time clock)".. and that's what it does.

This is from five minutes looking at the source. Very little college embedded programming. What more do you want?

// This function is called by WinCE OS to initialize the time after boot.

So how about calling it:

init_time

or maybe

init_time_after_boot

You're either going to have a naming conflict, or you're going to have a bunch of idiots who wanted to initialize a different clock, who start posting to forums saying "i don't understand???! i called init_time waht's wrong???"

Sure, they could call it

SendTheIOCTLCommandToInitializeTheRealTimeClockForTheOEMAbstractionLayer

but sometimes the names of functions are going to be ugly.

Isn't that the definition of "when to extract code into its own function"?

That is to say, I could have fixed the bug without knowing what it was. (I'm a hs student and don't do much bug-shooting in code that wasn't written by me or my close friends, so I don't know if this is common.)

Edit: Actually, I'm cutting it close :-). The last time we changed the calendar was in 1972, when we settled on integer leap seconds -- the same year that C was invented.

"Anyone who thinks programming dates and times is easy probably hasn't done much of it."

This should work if that was a >= sign. And people at work don't believe me when I say how insanely careful I have to be when I write any code, especially code that deals with people's payroll and vacation time. One missing equals symbol and every Zune in the world froze. One missing equals symbol and every employee could lose 1/12 of their accumulated vacation time.

The ability to pay attention to "mundane details" is just as important as the ability to envision the whole software application from the project manager point-of-view, especially when working in smaller companies without additional oversight.

I think the "while" condition should actually be

    while ((days > 365 && !IsLeapYear(year)) || (days > 366 && IsLeapYear(year)))

And then the inner "if" condition can be eliminated. Thus the final code could be:

    while ((days > 365 && !IsLeapYear(year)) || (days > 366 && IsLeapYear(year)))
    {
        if (IsLeapYear(year))
            days -= 366;
        else
            days -= 365;
        year += 1;
    }

Edit: if you want to get ternary-operator fancy:

    while (days > (IsLeapYear(year) ? 366 : 365))
    {
        days -= IsLeapYear(year) ? 366 : 365;
        year += 1;
    }

    days_in_year = IsLeapYear(year) ? 366 : 365;
    while (days > days_in_year) {
        days -= days_in_year;
        year += 1;
    }

#define diy(y) (IsLeapYear(y) ? 366 : 355)

while (days > diy(year)) { days -= diy(year); year += 1; }

But I wouldn't do it with a define but rather:

while (days > (days_in_year = IsLeapYear(year) ? 366 : 365)) { days -= days_in_year; year += 1; }

    while (days > (days_in_year = IsLeapYear(year) ? 366 : 365)) {
        days -= days_in_year;
        year += 1;
    }

  for (;;) {
    int days_in_year = IsLeapYear(year) ? 366 : 365;
    if (days <= days_in_year) break;
    days -= days_in_year;
    year += 1;
  }

Wrong. Completely ignoring all software engineering "best practices" was the problem. They could have reused a working library. They could have written unit tests tests. QA could have tested it. Other developers could have reviewed it, or written test cases without knowledge of how the code worked.

Basically, they ignored every rule for writing good code, and then ended up with a serious bug. Well, yeah. The whole "unit testing" "fad" exists for a reason -- it greatly improves the quality of software.

Many people take software development seriously enough to not let one brain fart cause a piece of software to brick thousands of devices.

I wouldn't put all the blame on the programmer (who probably is now scared to death).

Do you have the rights to this code?

Some context would be nice...

If it is indeed a Freescale-supplied driver issue, it makes me wonder how many other devices failed yesterday...

Leap = (Year%400) ? 0 : 1;