Skip to content

Bug in SetCCRRegister() #531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cdrose opened this issue May 28, 2019 · 5 comments · Fixed by #532
Closed

Bug in SetCCRRegister() #531

cdrose opened this issue May 28, 2019 · 5 comments · Fixed by #532
Assignees
Labels
bug 🐛 Something isn't working
Milestone

Comments

@cdrose
Copy link

cdrose commented May 28, 2019

I have been doing some work with CCP timer outputs and there are a few bugs to fix.

In timer.c attachIntHandleOC() case1 for compare channel 1 is correct, but case 2, 3 and 4 have != HAL_OK when the logic should be == HAL_OK, which then causes the call to HAL_TIM_OC_Start_IT() to be skipped and the channel is not enabled.

eg.

case 2:
      obj->irqHandleOC_CH2 = irqHandle;
      if (HAL_TIM_OC_ConfigChannel(handle, &sConfig, TIM_CHANNEL_2) == HAL_OK) {
        HAL_TIM_OC_Start_IT(handle, TIM_CHANNEL_2);
      }

There is also a bug in the SetCCRRegister() function in timer.c, the __HAL_TIM_SET_COMPARE is meant to be passed the TIM_CHANNEL_X constants (0x00, 0x04, 0x08, 0x0C) but the conversion from integer 1-4 to these values is incorrect. Just multiplying by 4 results in an off by 1 error (either that or we should pass in integer 0-3 but thats not consistent with other usage in timer.c

I suggest a switch instead to make the intent more clear and avoid the odd conversion function:

void setCCRRegister(stimer_t *obj, uint32_t channel, uint32_t value)
{
  switch (channel)
  {
	  case 1: channel = TIM_CHANNEL_1; break;
	  case 2: channel = TIM_CHANNEL_2; break;
	  case 3: channel = TIM_CHANNEL_3; break;
	  case 4: channel = TIM_CHANNEL_4; break;
  }

  __HAL_TIM_SET_COMPARE(&(obj->handle), channel, value);
}

I would also assume this fix needs to be applied to getCCRRegister() too though I havent tested that.

@fpistm fpistm self-assigned this May 28, 2019
@fpistm
Copy link
Member

fpistm commented May 28, 2019

Hi @cdrose
Could you share your sketch and also detail which board you use.
Thanks in advance

@fpistm fpistm added the bug 🐛 Something isn't working label May 28, 2019
@fpistm fpistm added this to the 1.6.0 milestone May 28, 2019
@fpistm
Copy link
Member

fpistm commented May 28, 2019

I will submit a patch soon. --> patch submitted

fpistm added a commit to fpistm/Arduino_Core_STM32 that referenced this issue May 29, 2019
TIM channel values used was confusing and required to map them
correctly while there is no added value to do this.

Use the HAL definition: TIM_CHANNEL_x will avoid any mismatch.

Fix stm32duino#531

Signed-off-by: Frederic.Pillon <[email protected]>
@fpistm
Copy link
Member

fpistm commented May 29, 2019

@cdrose
could you test with the PR #532?
Thanks in advance

@cdrose
Copy link
Author

cdrose commented May 30, 2019

@fpistm
Looks good to me, thanks for the speedy patch!

@fpistm
Copy link
Member

fpistm commented May 30, 2019

@cdrose
Welcome
Thanks for pointing this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants