Bug in KBOOT code

cancel
Showing results for 
Show  only  | Search instead for 
Did you mean: 

Bug in KBOOT code

Jump to solution
760 Views
fredroeber
Contributor II

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.

Labels (1)
0 Kudos
1 Solution
589 Views
MichaelMinnick
NXP Employee
NXP Employee

Hi Fred,

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

Best Regards,

Mike

View solution in original post

0 Kudos
2 Replies
589 Views
fredroeber
Contributor II

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

0 Kudos
590 Views
MichaelMinnick
NXP Employee
NXP Employee

Hi Fred,

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

Best Regards,

Mike

0 Kudos