Beware cut & paste

I've just spend nearly half a day tracking down a hard fault error on my board, and all because of cut and paste and bad programming practice.

I have four interrupt handlers, one for each of the timers on the LPC1227, these ISRs have to gain access to appropriate structures in memory that are dynamically allocated and so can't be hard coded. To deal with this I have a static array of pointers to the timer structures that is filled in when the user calls the hwimerCreate() function.

In my LARD framework the timers are known as timers 0-3 and enumerated as such.

typedef enum {
    HWTIMER_0,
    HWTIMER_1,
    HWTIMER_2,
    HWTIMER_3
} hwTimerTypes;

And there's an array of pointers to timer structures, one for each hardware timer.

hwTimer * hwTimers[N_HWTIMERS] = {0};

Now an ISR knows of course what hardware timer it was invoked by, but it needs to find the software structure that holds other information, such as a pointer to a user-supplied function to call. So it indexes into the hwTimers array with hwTimers[i]where i is the timer's logical number.

The old ISRs looked like this (much code removed for clarity)

void TIMER16_0_IRQHandler(void) {     // 16-bit Timer0
    hwTimer * t = hwTimers[HWTIMER_0];
}

void TIMER16_1_IRQHandler(void) {     // 16-bit Timer1
    hwTimer * t = hwTimers[HWTIMER_1];
}

void TIMER32_1_IRQHandler (void) {    // 32-bit Timer0
    hwTimer * t = hwTimers[HWTIMER_3];
}

void TIMER32_0_IRQHandler(void) {     // 32-bit Timer1
    hwTimer * t = hwTimers[HWTIMER_2];
}

Note that the two 16-bit timers are first and the 32-bitters follow, and that we have the order 16_0, 16_1, 32_1, 32_0 and index into the array of pointers using HWTIMER_0, 1, 3, 2 in that order. A little odd but it works and I never rearranged the code to be more logical. Note also that the comments are wrong and designed to confuse any future programmer.

Apart from the comments so far so good, but I changed the code in each ISR, and to save retyping I got one working then cut the text and pasted into the body of other three which meant they all used HWTIMER_0 as their index and // 16-bit Timer0 as the comment. So I then went down the page changing 0, 0, 0, 0, to the logical order of 0, 1, 2, 3 for the HWTIMER_x index and fixed the comments.

void TIMER16_0_IRQHandler(void) {      // 16-bit Timer0
    hwTimer * t = hwTimers[HWTIMER_0];
}

void TIMER16_1_IRQHandler(void) {      // 16-bit Timer1
    hwTimer * t = hwTimers[HWTIMER_1];
}

void TIMER32_1_IRQHandler (void) {     // 32-bit Timer0
    hwTimer * t = hwTimers[HWTIMER_2];
}

void TIMER32_0_IRQHandler(void) {      // 32-bit Timer1
    hwTimer * t = hwTimers[HWTIMER_3];
}

Then I guess I had dinner, watched some TV, whatever and got back to my programming to find that the two 16-bit timers work just fine but the 32-bit timers cause a hard fault.

Yikes.

Hours later, after looking at the index values and the comments a 1000 times and telling myself that they are in the logical order I finally look at the function names.

TIMER32_1_IRQHandler and TIMER32_0_IRQHandler are swapped, they were before as well but in that case so where the HWTIMER_x indexes so although it wasn't best practice because they were out of order it did work, this time I've been nice and logical in editing the indexes and comments to be sequential and forgotten that the functions are not sequential.

A quick swap of TIMER32_1_IRQHandler and TIMER32_0_IRQHandler and all things work.

So the moral of the story, be very careful with duplicating code with cut & paste, and organize like function that differ only in a number in a logical and numerical order.

Now I've forgotten what I was working on...that's right, I was trying to generate a 100uS break condition on the serial line.