Posted on May 17, 2011 at 12:41It 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?)
