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: Allow delaying initialization to setup method #1534

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

matthijskooijman
Copy link
Contributor

Previously, the only way to initialize a HardwareTimer instance was to
pass an instance to the constructor. When using a global (more
specifically, with static storage duration) HardwareTimer instance, the
initialization would happen early in startup, before main() and setup()
had a chance to run. This would mean that on any errors (e.g. no timer
found for a specific pin when using e.g. pinmap_peripheral), the board
would just lock up in early startup, unable to show a meaningful error
at all.

To prevent this, you would have to allocate the HardwareTimer object
dynamically on the heap, but that is not always a good idea from a
memory management perspective.

This commit adds an argumentless constructor that does not initialize
the timer yet, and a setup() method that accepts the timer instance and
does the actual initialization. This allows delaying the actual
initialization until in or after the sketch setup() function, and also
makes it easier to change the timer to use dynamically, based on e.g.
user input or EEPROM contents or similar.

Note that de-initializing a timer and switching to using a different one
is not currently supported, trying to call setup() twice results in an
error.

@matthijskooijman
Copy link
Contributor Author

Note that I have not tested this particular commit in the main branch, but it is forward-ported verbatim from a slightly customized STM32 core we are using (where this commit works as expected). Looking at the diff between both versions for HardwareTimer, I would not expect any issues.

Previously, the only way to initialize a HardwareTimer instance was to
pass an instance to the constructor. When using a global (more
specifically, with static storage duration) HardwareTimer instance, the
initialization would happen early in startup, before main() and setup()
had a chance to run. This would mean that on any errors (e.g. no timer
found for a specific pin when using e.g. pinmap_peripheral), the board
would just lock up in early startup, unable to show a meaningful error
at all.

To prevent this, you would have to allocate the HardwareTimer object
dynamically on the heap, but that is not always a good idea from a
memory management perspective.

This commit adds an argumentless constructor that does not initialize
the timer yet, and a setup() method that accepts the timer instance and
does the actual initialization. This allows delaying the actual
initialization until in or after the sketch setup() function, and also
makes it easier to change the timer to use dynamically, based on e.g.
user input or EEPROM contents or similar.

Note that de-initializing a timer and switching to using a different one
is not currently supported, trying to call setup() twice results in an
error.
@matthijskooijman
Copy link
Contributor Author

I've pushed some astyle fixes.

@fpistm
Copy link
Member

fpistm commented Nov 8, 2021

Hi @matthijskooijman
Thank you for this PR, @ABOSTM will review it soon 😉

@fpistm fpistm added this to In progress in STM32 core based on ST HAL via automation Nov 8, 2021
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.

@matthijskooijman Thanks for this PR. It makes sense.
LGTM.

STM32 core based on ST HAL automation moved this from In progress to Reviewer approved Nov 10, 2021
@fpistm fpistm added this to the 2.2.0 milestone Nov 10, 2021
@fpistm fpistm merged commit e0c5be1 into stm32duino:main Nov 10, 2021
STM32 core based on ST HAL automation moved this from Reviewer approved to Done Nov 10, 2021
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