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

Flexible interrupts handling #767

Closed
wants to merge 14 commits into from

Conversation

LinoBarreca
Copy link
Contributor

@LinoBarreca LinoBarreca commented Nov 8, 2019

Summary

This PR modifies (internally) how timers are started so that it will be possible to attach an interrupt to an already running timer.

This PR fixes/implements the following bugs/features

  • Bug: If the timer is initially started without a user-interrupt (it's rather a callback from an interrupt) it's impossible to assign one without stopping and restarting the timer. If the interrupt is assigned before starting the timer, instead the user can attach and detach them freely and have the callback still invoked. This was undocumented before today (I altered the wiki adding this warning but then I realized that a patch which handled it was better than a warning)
  • Bug: The user calls detachInterrupt and he expects to not have something besides systick that can interrupt his code. His code is still interrupted by HardwareTimer IRQ handler (and this is bad for time critical code, for example when the user, using the callback, wants to give a clock to something)
  • Feature: you can now attachInterrupt and detachInterrupt anytime

Motivation
Improvement in usage and predictable behavior if the user didn't read the "warning" in the documentation.

Validation
Sorry it's untested at the moment. But, in theory, it should work fine.

Code formatting
I changed everything that travis complained about.

Closing issues
Closes the issues I mentioned in "bugs".
I didn't open them because I preferred to see if I could use the same time to find a solution instead of creating a problem :)

This lets the user start the timer without a callback and add it after.
Moreover, now it is faster after detachInterrupt because
the ISR is actually stopped and no context switching is made.
This is good in performance critical code.
@LinoBarreca LinoBarreca marked this pull request as ready for review November 8, 2019 20:48
@fpistm fpistm requested a review from ABOSTM November 9, 2019 07:16
@fpistm
Copy link
Member

fpistm commented Nov 14, 2019

Hi @LinoBarreca
Is there any reason to close it?

@LinoBarreca
Copy link
Contributor Author

LinoBarreca commented Nov 14, 2019

@fpistm yes. Let me explain it.
Actually, despite the "logic" for me was good, it seems I made a mistake.
I made the assumption that the timer registers could be set even when the timer were not "active" (before resume).

@ABOSTM told me that, if the timer doesn't have a clock, you can't access its registers. I assume he is right because he is way more expert than me.

what was the logic behind my PR:

  • user creates an instance of HardwareTimer()
  • user calls attachInterrupt() to say "I want to receive the interrupt when the timer will be active"
  • attachInterrupt remembers the user callback and sets a register on the timer (this fails according to Alexandre)
  • user calls resume()
  • resume gives the clock to the timer and starts it. Since the above call failed the register in the timer will not have the information "I must trigger the IRS" and the user callback never gets called.

I thought my logic was working because with the setOverFlow() (actually called before resume()) we still access a register (ARR) of an "unstarted timer" so I thought that should work the same with the DIER register (called by attachInterrupt() ). I didn't really understand why, before resume, we can write to ARR but not DIER but if he says so I believe him (and I closed the PR because it worked in my mind but it will not work in real life)

If I misunderstood something (or you could clarify my doubt), however, let me know :)

@ABOSTM
Copy link
Contributor

ABOSTM commented Nov 14, 2019

Hi @LinoBarreca , Your understanding of the issue is correct,
nevertheless, as I said in #763, I am thinking about enabling timer peripheral clock at HardwareTimer object creation.
Thus if we go for this implementation, your pull request makes sense. So we can keep this PR opened

@LinoBarreca
Copy link
Contributor Author

LinoBarreca commented Nov 14, 2019

ok. Thanks Alexandre but...can you please explain me why, with the timer clock disabled, we can write to ARR (setOverflow) and PSC (setPrescaler) but not to DIER?
(again, I trust you, but it seems a bit not logical to me...I don't mean to offend anyone... there must be a reason why....but I'd like to know it)
....if I had a board I could test it..but unfortunately I don't own one...

@LinoBarreca LinoBarreca reopened this Nov 14, 2019
@ABOSTM
Copy link
Contributor

ABOSTM commented Nov 14, 2019

From hardware point of view it is not possible to write ARR or PSC registres (or any register) with timer clock disabled.
Concerning SetOverflow and setPrescaler, if you look carefully, there is a trick:
that is right thas we try to update registers (in case clock is already present, it allows to update ARR and PSC on the fly while timer is running), but in case clock is not present, we also save the value in structure so that it will be configured later (during the resume):

  __HAL_TIM_SET_AUTORELOAD(&_HardwareTimerObj.handle, ARR_RegisterValue);
  _HardwareTimerObj.handle.Init.Period = ARR_RegisterValue;

@LinoBarreca
Copy link
Contributor Author

LinoBarreca commented Nov 14, 2019

  __HAL_TIM_SET_AUTORELOAD(&_HardwareTimerObj.handle, ARR_RegisterValue); <-this fails if the timer doesn't have a clock (=not yet started) but succeeds if the timer is running.
  _HardwareTimerObj.handle.Init.Period = ARR_RegisterValue; <- this saves the value and sets it again at timer start

now I understood. You were very kind.

@fpistm fpistm added the On hold label Nov 18, 2019
@fpistm
Copy link
Member

fpistm commented Nov 18, 2019

I add label OnHold as it depends on #763

This lets the user start the timer without a callback and add it after.
Moreover, now it is faster after detachInterrupt because
the ISR is actually stopped and no context switching is made.
This is good in performance critical code.
ABOSTM added a commit to ABOSTM/Arduino_Core_STM32 that referenced this pull request Nov 28, 2019
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)
  fixes stm32duino#763
* integration of PR stm32duino#767 Flexible interrupts handling
ABOSTM added a commit to ABOSTM/Arduino_Core_STM32 that referenced this pull request Nov 29, 2019
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
@fpistm
Copy link
Member

fpistm commented Nov 29, 2019

Hi @LinoBarreca
@ABOSTM implemented #763 and included your PR during this.
I you can provide your feedback, this will be fine, then we could close this one as it is now duplicated in #806.
Thanks in advance

@fpistm fpistm added Duplicated and removed On hold labels Nov 29, 2019
@fpistm fpistm added this to In progress in STM32 core based on ST HAL via automation Nov 29, 2019
ABOSTM added a commit to ABOSTM/Arduino_Core_STM32 that referenced this pull request Dec 2, 2019
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
ABOSTM added a commit to ABOSTM/Arduino_Core_STM32 that referenced this pull request Dec 2, 2019
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
ABOSTM added a commit to ABOSTM/Arduino_Core_STM32 that referenced this pull request Dec 3, 2019
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
@fpistm
Copy link
Member

fpistm commented Dec 5, 2019

I close this one has integrated in #806.

@fpistm fpistm closed this Dec 5, 2019
STM32 core based on ST HAL automation moved this from In progress to Done Dec 5, 2019
@LinoBarreca LinoBarreca deleted the FlexibleInterrupts branch December 20, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants