Bug in KBOOT code

fredroeber — Sun, 14 Sep 2014 12:19:39 GMT

Hi, I've been looking into the recently released KBOOT code. It installs as FSL_Kinetis_Bootloader_1_0_2. Just wanted to mention that the microseconds_delay function in src/microseconds/src/microseconds_sysclk.c is wrong. The function in question is:

//! @brief Delay specified time
//!
//! @param us Delay time in microseconds unit
void microseconds_delay(uint32_t us)
{
    uint32_t currentTicks = microseconds_get_ticks();


    //! The clock value in Mhz = ticks/microsecond
    uint64_t ticksNeeded = (us * s_tickPerMicrosecondMul8 / 8) + currentTicks;
    while(microseconds_get_ticks() > ticksNeeded)
    {
        ;
    }
}

The check "while(microseconds_get_ticks() > ticksNeeded)" is backwards I believe and unsafe too. It's unsafe because when the get_ticks value is in the "roll over" region and the check is done right (ie microseconds_get_ticks() < ticksNeeded) you could have cases where the test "misses" and the delay goes for a very long time. Correct code does a signed subtraction and checks that result.

It seems like this delay function isn't actually used in any of the loader code so the error isn't actually hurting anything. But having a broken function just begs problems in the future if someone goes to use the function. So I figured I would report it. I'm just starting to look at the loader code to adapt it to my project and figured I would just see what the code looked like. Doesn't give me a warm fuzzy feeling that one of the first things I happened to look at had a bug. But, hey, it's probably the only one ;-).

Just thought I'd alert the maintainers so they might fix it.

Re: Bug in KBOOT code

fredroeber — Sun, 14 Sep 2014 12:56:33 GMT

Hey folks, just an update. So, I'm not used to using 64 bit values on small MCUs. The delay function uses such values in the time comparison code so I guess we wouldn't have to worry about time value roll over issues and problems with them if it used the values right. If the current_ticks value was declared uint64_t to match microseconds_get_ticks then I think the only other fix needed is to reverse the conditional test.

Fred

Re: Bug in KBOOT code

MichaelMinnick — Wed, 17 Sep 2014 14:31:27 GMT

Hi Fred,

The reversed conditional test has been fixed for the next KBOOT release. Thanks for pointing it out!

Best Regards,

Mike

Kinetis MicrocontrollersのトピックRe: Bug in KBOOT code

Bug in KBOOT code

Re: Bug in KBOOT code

Re: Bug in KBOOT code