Bug in TIM firmware

[andreas2]

Posted on August 03, 2008 at 20:51

Bug in TIM firmware

[andreas2]

Posted on May 17, 2011 at 12:41

It seems there is a nasty bug in the TIM_SelectOCxM() function in FWLib v2.0.2. Apart from being horribly ugly and unreadable, the following code destroys the value of the modified CCMR register.

Code:

if((TIM_Channel == TIM_Channel_1) ||(TIM_Channel == TIM_Channel_3))

{

/* Reset the OCxM bits in the CCMRx register */

*((vu32 *)((*(u32*)&TIMx) + CCMR_Offset + (TIM_Channel>>1))) &= CCMR_OC13M_Mask;

/* Configure the OCxM bits in the CCMRx register */

*((vu32 *)((*(u32*)&TIMx) + CCMR_Offset + (TIM_Channel>>1))) = TIM_OCMode;

}

else

{

/* Reset the OCxM bits in the CCMRx register */

*((vu32 *)((*(u32*)&TIMx) + CCMR_Offset + ((u16)(TIM_Channel - 4)>> 1))) &= CCMR_OC24M_Mask;

/* Configure the OCxM bits in the CCMRx register */

*((vu32 *)((*(u32*)&TIMx) + CCMR_Offset + ((u16)(TIM_Channel - 4)>> 1))) = (u16)(TIM_OCMode << 8);

}

Simply substituting '|=' for '=' should ''fix'' it, but i'd rather see it rewritten completely to something like the following.

Code:

void TIM_SelectOCxM(TIM_TypeDef* TIMx, u16 TIM_Channel, u16 TIM_OCMode)

{

vu16* pCCMRx;

u16 mask;

/* Check the parameters */

assert_param(IS_TIM_123458_PERIPH(TIMx));

assert_param(IS_TIM_CHANNEL(TIM_Channel));

assert_param(IS_TIM_OCM(TIM_OCMode));

/* Disable the Channel: Reset the CCxE Bit */

TIMx->CCER &= ~(CCER_CCE_Set << TIM_Channel);

if ((TIM_Channel == TIM_Channel_1) ||(TIM_Channel == TIM_Channel_3))

{

mask = CCMR_OC13M_Mask;

}

else

{

mask = CCMR_OC24M_Mask;

TIM_OCMode <<= 8;

}

if ((TIM_Channel == TIM_Channel_1) || (TIM_Channel == TIM_Channel_2))

{

pCCMRx = &TIMx->CCMR1;

}

else

{

pCCMRx = &TIMx->CCMR2;

}

/* Configure the OCxM bits in the CCMRx register */

*pCCMRx = (*pCCMRx & mask) | TIM_OCMode;

}

This avoids the stupid type-punning and is probably more efficient as well.

(Why is the library littered with things like taking the address of a peripheral pointer, casting it to a pointer-to-u32 and dereferencing it, often in order to compare it to a constant value? Is there any reason at all to do this, seems unnecessary to me?)

[16-32micros]

Posted on May 17, 2011 at 12:41

Hi andreas1,

Thanks for pointing out this bug, I will escalate to our software team and let you know accordingly later on about your interesting feed-backs. Thx.

Cheers,

STOne-32.