HardwareTimer: rework internal mapping of arduino API to HAL/LL API #806

ABOSTM · 2019-11-29T14:38:57Z

Summary

Rework HardwareTimer internal working. And add new API to pause/resume individual channel.
Main rework:

HAL_TIM_Base_Init() is now called only once at object creation
HAL_TIM_xxx_ConfigChannel is now done in setMode()
HAL_TIM_xxx_Start is done in resumeChannel()
use LL when possible
Configuration are directly made through hardware register access (when possible),
then remove useless attribute
Add new API to pause only one channel (fixes HardwareTimer incomplete? #763):

     pauseChannel(uint32_t channel);
     resumeChannel(uint32_t channel);

integration of PR Flexible interrupts handling #767 Flexible interrupts handling

Update wiki when merged

cores/arduino/HardwareTimer.cpp

cores/arduino/HardwareTimer.h

cores/arduino/HardwareTimer.cpp

fpistm

There is a regression on tone.

Main rework: * HAL_TIM_Base_Init() is now called only once at object creation * HAL_TIM_xxx_ConfigChannel is now done in setMode() * HAL_TIM_xxx_Start is done in resumeChannel() * use LL when possible * Configuration are directly made through hardware register access (xhen possible), then remove useless attribut * Add new API to pause only one channel: pauseChannel(uint32_t channel) resumeChannel(uint32_t channel) fixes stm32duino#763 * integration of PR stm32duino#767 Flexible interrupts handling

When timerTonePinDeinit() is called, it will deinitialize timer. It is then necessary to recreate HardwareTimer object to initialize timer. So, when NoTone() is called, without destruct, only pause the timer. Also when frequency is null just pause the timer.

matthijskooijman

I'm a bit late to the party, but had a look at the code anyway. Overall, the changes look great, stuff is cleaner and more predictable. I left some small comments inline, mostly regarding the interrupt handling.

Regarding the interrupt enabling and disabling, which currently happens in both resume()/pause() and in attachInterrupt()/detachtInterrupt() (similarly for the channel interrupts).
I wonder:

Since interrupts are enabled in attachInterrupt, which might occur before start(), they may be enabled before the timer is running. I assume this is not problematic, since without a running timer, the interrupt would simply not trigger. If so, is it still needed to disable the interrupts in pause(), would it not be sufficient to disable the timer?
If pause() does not disable interrupts, resume() does not need to enable them (they will be exclusively enabled/disabled by attach/detachInterrupt() then.
For channels, the above does not hold AFAICS (since pauseChannel() does not halt the timer and disabling the channel only prevents input capture and output but still allows interrupts, so it must disable the interrupt to prevent it from triggering, I believe). This would probably mean that resumeChannel() / pauseChannel() should still enable/disable the interrupt, but that also means that attachInterrupt() should not enable the interrupt if the channel is not currently running.
The same could again be applied to the timer interrupt - only enable in attachInterrupt if the timer is actually running.

Overall, it might be interesting to think about or document how the various functions are expected to behave. E.g. things like what happens when you pause a channel when the timer is not running? What happens when you resume the timer then? What happens when you attach an interrupt while the timer is running? Etc. Also, are all relevant usecases covered?

matthijskooijman · 2019-12-10T11:45:39Z

cores/arduino/HardwareTimer.cpp

+    __HAL_TIM_CLEAR_FLAG(&(_timerObj.handle), TIM_FLAG_UPDATE);
+    // Enable update interrupt only if callback is valid
+    __HAL_TIM_ENABLE_IT(&(_timerObj.handle), TIM_IT_UPDATE);
+  }
  callbacks[0] = callback;


I think there is a race condition here, if the interrupt immediately triggers before callbacks[0] is set. Setting it before enabling interrupts would help, I think.

Right, I will take it into account.

matthijskooijman · 2019-12-10T11:46:05Z

cores/arduino/HardwareTimer.cpp

+    __HAL_TIM_CLEAR_FLAG(&(_timerObj.handle), interrupt);
+    // Enable interrupt corresponding to channel, only if callback is valid
+    __HAL_TIM_ENABLE_IT(&(_timerObj.handle), interrupt);
+  }
  callbacks[channel] = callback;


Same race condition.

Right, I will take it into account.

matthijskooijman · 2019-12-10T11:57:29Z

cores/arduino/HardwareTimer.cpp

  if ((channel == 0) || (channel > (TIMER_CHANNELS + 1))) {
    Error_Handler();  // only channel 1..4 have an interrupt
  }

+  if (callback != NULL) {
+    // Clear flag before enabling IT
+    __HAL_TIM_CLEAR_FLAG(&(_timerObj.handle), interrupt);


I believe that when the interrupt is already enabled (e.g. when you are changing the interrupt handler to a different one), this flag clear is not needed (and even harmful, since it might cause one interrupt to be skipped, I think).

I understand what you said, but Idisagree:
In the case you change the callback while timer is running, you will not loose interrupt, because if IT is pending, before the clear, it will run callback before execution the clear.
If IT arrive after the clear, it will run callback regularly. So from my point of view it is not harmful to make the clear.
And Clear is necessary for example in the following use case:
The timer is already running with interrupts disabled. After a while, Flags Update or Compare will be set. Then if we enable interrupt we must clear the flag to discard past interrupts and take into account only new ones.

I am not entirely sure about the timing details on STM32, but I can imagine that the interrupt arrives at the same time as the clear, and thus is cleared before it can run.

Another case is when code is called when interrupts are temporarily disabled (e.g. using the global interrupt mask, or when running from an ISR with higher priority). In that case, the flag might be set before the clear, but the interrupt cannot run yet, so the clear loses one interrupt.

And Clear is necessary for example in the following use case:
The timer is already running with interrupts disabled. After a while, Flags Update or Compare will be set. Then if we enable interrupt we must clear the flag to discard past interrupts and take into account only new ones.

Yeah, I agree on that. Hence my suggestion to only clear if the interrupt is not enabled yet, which I think should cover the case you describe? If the interrupt is already enabled, then the clear is not needed, assuming that any interrupts that are still pending should be handled by the new callback rather than be lost.

Right, I agree with your proposal. I implement it in #852

ABOSTM · 2019-12-16T13:26:12Z

Hi @matthijskooijman,
Thank for your comments.
I disagree with your proposal to enable interrupt only in resume()/resumeChannel() and only when timer or channel is running:
the following basic sketch would not work anymore, interrupts will never be enabled:

  MyTim->setMode(channel, TIMER_OUTPUT_COMPARE_PWM1, pin);
  // MyTim->setPrescaleFactor(8); 
  MyTim->setOverflow(100000, MICROSEC_FORMAT); // 100000 microseconds = 100 milliseconds
  MyTim->setCaptureCompare(channel, 50, PERCENT_COMPARE_FORMAT); // 50%
  MyTim->attachInterrupt(Update_IT_callback);
  MyTim->attachInterrupt(channel, Compare_IT_callback);
  MyTim->resume();

Also, I think about taking only part of your proposal: in attachInterrupt(), enable IT only if channel is running.
But there is currently no mean to know a channel is enabled (mode is not enough as 0 is a valid mode) and we can have tha case where we attach a callback to compare event without output on IO.
I think globally it is easier to understand:

attachInterrupt() will attach the call back and enable IT.
resume()/resumeChannel will resume/enable IT
dettach()/pause()/pauseChannel() will do the opposite.
And thus no need to have complex crosscase documentation like "what happends if ..."

That is right that if you call attachInterrupt() while channel is no running, it will enable the IT, and it can fire the callback. Up to application to configure channel 1rst.

ABOSTM

I will push update for race condition in separate PR since this one is closed

ABOSTM · 2019-12-16T10:01:37Z

cores/arduino/HardwareTimer.cpp

  if ((channel == 0) || (channel > (TIMER_CHANNELS + 1))) {
    Error_Handler();  // only channel 1..4 have an interrupt
  }

+  if (callback != NULL) {
+    // Clear flag before enabling IT
+    __HAL_TIM_CLEAR_FLAG(&(_timerObj.handle), interrupt);


I understand what you said, but Idisagree:
In the case you change the callback while timer is running, you will not loose interrupt, because if IT is pending, before the clear, it will run callback before execution the clear.
If IT arrive after the clear, it will run callback regularly. So from my point of view it is not harmful to make the clear.
And Clear is necessary for example in the following use case:
The timer is already running with interrupts disabled. After a while, Flags Update or Compare will be set. Then if we enable interrupt we must clear the flag to discard past interrupts and take into account only new ones.

ABOSTM · 2019-12-16T10:04:44Z

cores/arduino/HardwareTimer.cpp

+    __HAL_TIM_CLEAR_FLAG(&(_timerObj.handle), TIM_FLAG_UPDATE);
+    // Enable update interrupt only if callback is valid
+    __HAL_TIM_ENABLE_IT(&(_timerObj.handle), TIM_IT_UPDATE);
+  }
  callbacks[0] = callback;


Right, I will take it into account.

ABOSTM · 2019-12-16T10:05:18Z

cores/arduino/HardwareTimer.cpp

+    __HAL_TIM_CLEAR_FLAG(&(_timerObj.handle), interrupt);
+    // Enable interrupt corresponding to channel, only if callback is valid
+    __HAL_TIM_ENABLE_IT(&(_timerObj.handle), interrupt);
+  }
  callbacks[channel] = callback;


Right, I will take it into account.

matthijskooijman

I disagree with your proposal to enable interrupt only in resume()/resumeChannel() and only when timer or channel is running:

My proposal was probably not well-enough thought-through, partly meant as food for more thought than a specific proposal. I had another look just now, but it's been a while since I looked at the code, and I have no time right now to dive in again deep enough to come up with some proper examples and proposals, so maybe later.

I did respond to an inline comment again.

Also, the changes you made in #835 and #849 look good to me!

@matthijskooijman

rework stm32duino#806 after discussion with @matthijskooijman

@matthijskooijman

rework #806 after discussion with @matthijskooijman

fpistm mentioned this pull request Nov 29, 2019

Flexible interrupts handling #767

Closed

3 tasks

fpistm added Enhancement Fix labels Nov 29, 2019

fpistm added this to In progress in STM32 core based on ST HAL via automation Nov 29, 2019

fpistm added this to the 1.8.0🎄 🎅 milestone Nov 29, 2019

fpistm requested changes Nov 29, 2019

View reviewed changes

STM32 core based on ST HAL automation moved this from In progress to Needs review Nov 29, 2019

ABOSTM force-pushed the HT_REWORK_HAL_LL_MAPPING branch 2 times, most recently from a170fe8 to e8b3300 Compare December 2, 2019 16:46

fpistm approved these changes Dec 2, 2019

View reviewed changes

STM32 core based on ST HAL automation moved this from Needs review to Reviewer approved Dec 2, 2019

fpistm self-requested a review December 3, 2019 06:50

fpistm requested changes Dec 3, 2019

View reviewed changes

STM32 core based on ST HAL automation moved this from Reviewer approved to Needs review Dec 3, 2019

ABOSTM force-pushed the HT_REWORK_HAL_LL_MAPPING branch 2 times, most recently from b2c1548 to d8f79dd Compare December 3, 2019 14:03

ABOSTM force-pushed the HT_REWORK_HAL_LL_MAPPING branch from d8f79dd to 80dfa09 Compare December 3, 2019 17:29

fpistm self-requested a review December 6, 2019 17:16

STM32 core based on ST HAL automation moved this from Needs review to Reviewer approved Dec 6, 2019

fpistm approved these changes Dec 6, 2019

View reviewed changes

fpistm merged commit a7b8969 into stm32duino:master Dec 9, 2019

STM32 core based on ST HAL automation moved this from Reviewer approved to Done Dec 9, 2019

matthijskooijman reviewed Dec 10, 2019

View reviewed changes

ABOSTM commented Dec 16, 2019

View reviewed changes

ABOSTM mentioned this pull request Dec 16, 2019

HardwareTimer: set callback before enabling interrupt #835

Merged

ABOSTM mentioned this pull request Jan 2, 2020

New version 1.8.0 and Hardware Timer on MK4duo FW printer 3D #841

Closed

matthijskooijman reviewed Jan 2, 2020

View reviewed changes

ABOSTM added a commit to ABOSTM/Arduino_Core_STM32 that referenced this pull request Jan 3, 2020

HardwareTimer: Clear IT flag only if callback not previously configured

ccfed74

rework stm32duino#806 after discussion with @matthijskooijman

ABOSTM mentioned this pull request Jan 3, 2020

HardwareTimer: Clear IT flag only if callback not previously configured #852

Merged

fpistm pushed a commit that referenced this pull request Jan 13, 2020

HardwareTimer: Clear IT flag only if callback not previously configured

a6f7a98

rework #806 after discussion with @matthijskooijman

fpistm mentioned this pull request Jun 6, 2023

gh pr list do not show some PR cli/cli#7538

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HardwareTimer: rework internal mapping of arduino API to HAL/LL API #806

HardwareTimer: rework internal mapping of arduino API to HAL/LL API #806

ABOSTM commented Nov 29, 2019 •

edited

fpistm left a comment

matthijskooijman left a comment

matthijskooijman Dec 10, 2019

ABOSTM Dec 16, 2019

matthijskooijman Dec 10, 2019

ABOSTM Dec 16, 2019

matthijskooijman Dec 10, 2019

ABOSTM Dec 16, 2019

matthijskooijman Jan 2, 2020

ABOSTM Jan 3, 2020

ABOSTM commented Dec 16, 2019

ABOSTM left a comment •

edited

ABOSTM Dec 16, 2019

ABOSTM Dec 16, 2019

ABOSTM Dec 16, 2019

matthijskooijman left a comment

HardwareTimer: rework internal mapping of arduino API to HAL/LL API #806

HardwareTimer: rework internal mapping of arduino API to HAL/LL API #806

Conversation

ABOSTM commented Nov 29, 2019 • edited

fpistm left a comment

Choose a reason for hiding this comment

matthijskooijman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ABOSTM commented Dec 16, 2019

ABOSTM left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthijskooijman left a comment

Choose a reason for hiding this comment

ABOSTM commented Nov 29, 2019 •

edited

ABOSTM left a comment •

edited