Skip to content

[PR 753 rework][SoftwareSerial] Add function to set interrupt priority #755

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

Merged
merged 3 commits into from
Nov 5, 2019

Conversation

fpistm
Copy link
Member

@fpistm fpistm commented Nov 4, 2019

This PR is a rework of #753.
Instead of allow to be able to change the interrupt priority of the Timer used by the SoftwareSerial library, it allows to change interrupt priority of all the timer used through the HardwareTimer class.

Fixes #751

This will allow to customize IRQ priority

Signed-off-by: Frederic Pillon <[email protected]>
@fpistm fpistm self-assigned this Nov 4, 2019
@fpistm fpistm added the enhancement New feature or request label Nov 4, 2019
@sjasonsmith
Copy link
Contributor

I like making this available for all hardware timers, instead of my change for only SoftwareSerial.

I think this will also solve another issue, which is the need to re-set the priority every time the timer is resumed.

I would like to test this on hardware, but will not have time to do so until tonight. You should have my results in the morning!

@fpistm
Copy link
Member Author

fpistm commented Nov 4, 2019

@sjasonsmith
Thanks

About:

I think this will also solve another issue, which is the need to re-set the priority every time the timer is resumed.

Well this is the case even with this PR because it is done by the HAL_TIM_Base_MspDeInit which is called by HAL_TIM_Base_Init each time resume is called.
Anyway, compared to the first implementation you've made, this is done only once instead of twice in setSpeed (one by the resume then call to NVIC_SetPriority)

@sjasonsmith
Copy link
Contributor

sjasonsmith commented Nov 5, 2019

@fpistm

I reworked this into my Marlin branch, and confirmed that it is working.

I am able to rearrange my three timers (2 HardwareTimer + 1 SoftwareSerial) in any order and verify proper preempting behavior.

I also verified that I can now set the priority once when initializing the HardwareTimer, rather than after resume as was previously required.

@fpistm fpistm requested a review from ABOSTM November 5, 2019 06:29
@fpistm
Copy link
Member Author

fpistm commented Nov 5, 2019

@sjasonsmith
Thanks for tests.
In that case I will merge this one instead of #753.
I will wait @ABOSTM feedback as he is the author of the HardwareTimer library.

@LinoBarreca
Copy link
Contributor

@fpistm I highly approve this PR (although it breaks interface compatibility with Arduino's HardwareTimer) it's kinda necessary on STM32 platform.

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@fpistm fpistm merged commit 2b1bdca into stm32duino:master Nov 5, 2019
@fpistm fpistm deleted the PR-753-Rework branch November 5, 2019 15:03
@fpistm fpistm added this to the 1.8.0 milestone Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow override for SoftwareSerial timer ISR priority
4 participants