Skip to content
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

HardwareTimer: Fix assert failed when using TIMER_OUTPUT_COMPARE #1247

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

ABOSTM
Copy link
Contributor

@ABOSTM ABOSTM commented Dec 1, 2020

Summary

HardwareTimer: Fix assert failed when using TIMER_OUTPUT_COMPARE

When assert is activated there may be assert failed, specially
when using Tone or Servo with TIM6 or TIM7.
"assert_param(IS_TIM_CC1_INSTANCE(htim->Instance));"
This is due to the fact that when using timer instances without
output (like TIM6 and TIM7 specially used for Tone and Servo)
in TIMER_OUTPUT_COMPARE mode, the API setMode() requires a channel,
even if it is not used.
This was made like this to simplify the HardwareTimer driver,
and there is no functional issue, but as there is an assert failed
reported when assert is activated, this should be fixed.

TIMER_OUTPUT_COMPARE becomes obsolete, but kept for compatibility
reason.
When only timing configuration is needed, no need to set mode,
just keep the default TIMER_DISABLED.

Validation
Test passed:

  • Servo
  • Tone
  • HardwareTimer non regression test (HardwareTimer_OutputInput_test)

Fixes #1244

ABOSTM added a commit to ABOSTM/STM32Examples that referenced this pull request Dec 1, 2020
After implementation of:
stm32duino/Arduino_Core_STM32#1247
it is better to use TIMER_DISABLED instead of  TIMER_OUTPUT_COMPARE
Even if TIMER_OUTPUT_COMPARE has been kept for compatibility
reason and is still working.

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
ABOSTM added a commit to ABOSTM/STM32Examples that referenced this pull request Dec 1, 2020
After implementation of:
stm32duino/Arduino_Core_STM32#1247
it is better to use TIMER_DISABLED instead of  TIMER_OUTPUT_COMPARE
Even if TIMER_OUTPUT_COMPARE has been kept for compatibility
reason and is still working.

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
@ABOSTM ABOSTM requested a review from fpistm December 1, 2020 17:02
When assert is activated there may be assert failed, specially
when using Tone or Servo with TIM6 or TIM7.
"assert_param(IS_TIM_CC1_INSTANCE(htim->Instance));"
This is due to the fact that when using timer instances without
output (like TIM6 and TIM7 specially used for Tone and Servo)
in TIMER_OUTPUT_COMPARE mode, the API setMode() requires a channel,
even if it is not used.
This was made like this to simplify the HardwareTimer driver,
and there is no functional issue, but as there is an assert failed
reported when assert is activated, this should be fixed.

TIMER_OUTPUT_COMPARE becomes obsolete, but kept for compatibility
reason.
When only timing configuration is needed, no need to set mode,
just keep the default TIMER_DISABLED.

Fixes stm32duino#1244

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
@ABOSTM ABOSTM force-pushed the HARDWARETIMER_ASSERT_FAILED branch from 09b074b to 3e313f9 Compare December 1, 2020 17:06
@ABOSTM
Copy link
Contributor Author

ABOSTM commented Dec 2, 2020

@ghent360 fix is there.

@fpistm fpistm added bug 🐛 Something isn't working fix 🩹 Bug fix labels Dec 2, 2020
@fpistm fpistm added this to the 2.0.0 milestone Dec 2, 2020
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fpistm
Copy link
Member

fpistm commented Dec 2, 2020

Due to GH Action issue (resolved now) some of the checks are failed anyway it is ok.
PR could be merged.

@fpistm fpistm merged commit 8888918 into stm32duino:master Dec 2, 2020
fpistm pushed a commit to stm32duino/STM32Examples that referenced this pull request Dec 2, 2020
After implementation of:
stm32duino/Arduino_Core_STM32#1247
it is better to use TIMER_DISABLED instead of  TIMER_OUTPUT_COMPARE
Even if TIMER_OUTPUT_COMPARE has been kept for compatibility
reason and is still working.

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
ABOSTM added a commit to ABOSTM/STM32Ethernet that referenced this pull request Dec 3, 2020
With introduction of PR
stm32duino/Arduino_Core_STM32#1247
Usage of TIMER_OUTPUT_COMPARE becomes obsolete.
Note: removing setMode(1, TIMER_OUTPUT_COMPARE)
also works before PR #1247.

Signed-off-by: Alexandre Bourdiol <alexandre.bourdiol@st.com>
@VitorBoss
Copy link

VitorBoss commented Jun 11, 2021

Sorry to send a message here, I've tried to do it on the forum but couldn't login, I won't open a issue without confirming it first.
This PR has been included in a while but I can't use the API like before, here is an example code:

    Timer1.setOverflow(0xFFFF, TICK_FORMAT);
    Timer1.setPrescaleFactor(((Timer1.getTimerClkFreq()/1000000) * TIMER_RESOLUTION)-1);   //4us resolution
    Timer1.setMode(2, TIMER_OUTPUT_COMPARE);
    Timer1.setMode(3, TIMER_OUTPUT_COMPARE);

    Timer1.attachInterrupt(2, boostInterrupt);
    Timer1.attachInterrupt(3, vvtInterrupt);
    Timer1.resume();

On v1.9.0 it works flawlessly, on 2.0.0 it don't work at all, I tried this:

    Timer1.setOverflow(0xFFFF, TICK_FORMAT);
    Timer1.setPrescaleFactor(((Timer1.getTimerClkFreq()/1000000) * TIMER_RESOLUTION)-1);   //4us resolution
    Timer1.setMode(2, TIMER_DISABLED);
    Timer1.setMode(3, TIMER_DISABLED);

    Timer1.attachInterrupt(2, boostInterrupt);
    Timer1.attachInterrupt(3, vvtInterrupt);
    Timer1.resume();

It refuses to work with channels, if I remove the channel number I got timer interrupts, what am I missing?
Edit: Tested on Black F407VE and F411CE BlackPill

@ABOSTM
Copy link
Contributor Author

ABOSTM commented Jun 11, 2021

Hi @VitorBoss, you are right, the behavior has been changed/improved thanks to this PR:
As you noticed TIMER_OUTPUT_COMPARE becomes obsolete and you changed it to TIMER_DISABLED, great.
But the behavior you described looks normal to me,
TIMER_DISABLED will disable the channel: it is intend to configure timer in timebase mode without PWM output and thus only period is valid, not the channel. So in this case you cannot get channel interrupt, but only period interrupt.

Alternatively you can configure the timer with channel configuration,
for example (depending whether you need PWM output)
Timer1.setMode(channel, TIMER_OUTPUT_COMPARE_PWM1, PA_8); PWM will be output to PA8
or
Timer1.setMode(channel, TIMER_OUTPUT_COMPARE_PWM1); implicitly pin is NC (Not connected)

@VitorBoss
Copy link

First, thank you for the reply.

If I understood you right is just a matter of replacing the TIMER_OUTPUT_COMPARE with TIMER_OUTPUT_COMPARE_TOGGLE with no pin as argument right?

Please add this to the documentation, I was trying to make it work for a week looking on the wiki

@ABOSTM
Copy link
Contributor Author

ABOSTM commented Jun 11, 2021

Using TIMER_OUTPUT_COMPARE_TOGGLE or TIMER_OUTPUT_COMPARE_PWM1 just impact output wave form (see reference manual TIMx_CCMR1 for details) so if you intend to use timer with no pin, both will be the same.
That said, there something not clear to me in your code:
both callback boostInterrupt() and vvtInterrupt() will be called at the same frequency (period is unique for a given timer whatever the cahnnel). And because you didn't specify the pulse duration, both callback will occurs at the same time.
If this is what you wanted to do, there is no need to use 2 channels, only update interrupt is necessary and in the update callback you call both boostInterrupt() and vvtInterrupt().
See https://github.com/stm32duino/STM32Examples/blob/master/examples/Peripherals/HardwareTimer/Timebase_callback/

Using 2 channel may be interesting only if you wanted to have the same frequency for both callbacks, AND you want a specific delay between both callback. If it is what you wanted to do, it is necessary to specify both pulse duration (the delta between both will determined the delay between both callbacks).
And even, there is another (better) way to do this: use update interrupt callback and only 1 channel to specify the delay between the 2 callbacks.

@VitorBoss
Copy link

VitorBoss commented Jun 11, 2021

The use case here is an engine management system, each channel do 2 independent PWM wave form with independent frequency settings, the period isn't the same.

Thank you for clarify my doubts.

@uzi18
Copy link

uzi18 commented Jun 11, 2021

@valeros do you have more info about this project?

@ABOSTM
Copy link
Contributor Author

ABOSTM commented Jun 14, 2021

@VitorBoss,

with independent frequency settings, the period isn't the same.

1 timer = 1 period, so you need 2 timers

each channel do 2 independent PWM wave form

So totally there are 4 PWM wave form?
2 with a the same frequency F1, and 2 with another frequency F2 ?
If yes, then you need 2 timers, each timer with 2 channels and pin associated to output the wave form.
Mode should probably be TIMER_OUTPUT_COMPARE_PWM1 (or TIMER_OUTPUT_COMPARE_PWM2).
And, if all you need is to output wave form, then you don't need to configure interruption at all.

You can have a look at example https://github.com/stm32duino/STM32Examples/tree/master/examples/Peripherals/HardwareTimer/PWM_FullConfiguration
Look at how "pin" is configured with no need to toggle it in interrupt, PWM wave form for pin is automatically generated by TIM hardware (interrupt is for pin2 only not pin).

Also as stated in wiki: https://github.com/stm32duino/wiki/wiki/HardwareTimer-library

The use of this library suppose you have some basic knowledge of STM32 hardware timer architecture. First of all remind that all timers are not equivalent and doesn't support the same features. Please refer to the Reference Manual of your MCU.

So I strongly suggest you to read Reference Manual, chapter relative to TIMx

@VitorBoss
Copy link

Sorry, I won't clear, each channel do 2 software PWM, even with same timer period the code uses interrupts to trigger the PWM period(pwm_max_count = 1000000L / (TIMER_RESOLUTION * configPage.idleFreq * 2);), the code is made to be flexible and compatible with any MCU, when I first start porting the code to be compatible with STM32 I didn't use the TIMx pins, everything is handle inside the interrupts.

The only problem I had with latest release was the compatibility with TIMER_OUTPUT_COMPARE, now everything is working again. Thank you for your support, it is very much appreciated.

I know you are busy, if you have a time check out the project https://github.com/noisymime/speeduino

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

Successfully merging this pull request may close these issues.

HardwareTimer: assert failed when assert is activated, specially when using Servo or Tone
4 participants