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

fix #2071 add support for max overflow value for 32 bits timer #2072

Closed
wants to merge 3 commits into from

Conversation

GianfrancoIU1JSU
Copy link

Summary
the following fixes #2071 in the way proposed in the issue itself.

this will now allow to set the 32bit HW timers overflow to the full value.

the getOverflow() function was also modified to accomodate the new value.
this will not cause problems with existing code since they are using values castable in uint32_t

This PR fixes the following bug
#2071

Validation
the modification was tested and work as expected
testing code present in #2071

@fpistm fpistm added bug 🐛 Something isn't working fix 🩹 Bug fix labels Jul 17, 2023
@fpistm fpistm added this to the 2.7.0 milestone Jul 17, 2023
@fpistm
Copy link
Member

fpistm commented Jul 17, 2023

Hi @GianfrancoIU1JSU
Thanks for the PR, we will review it, as a first comment, it seems not complete and probably generate warnings.
Check should be done if the value is higher than an UINT32_MAX.
GetOverflow should also be updated to return a uint64_t.
Examples and codes using them should be also updated.

@GianfrancoIU1JSU
Copy link
Author

Hi @fpistm

it seems not complete and probably generate warnings.

in teory the only place where a it can trigger a warning is on the return value of the
getOverflow() since in already made code is used with uint32_t
this is a narrowing conversion and could generate a warning but i could not get it,
even with -Wall build flag.
warning a side for existing code doesn't pose a issue since they are using values that fit in uint32_t

Check should be done if the value is higher than an UINT32_MAX.

Check should be done if value - 1 > UINT32_MAX,
the point of the modification is to allow the value to be 0x100000000
which will became UINT32_MAX after the -1

GetOverflow should also be updated to return a uint64_t.

I've already done that in this PR be7e127

Examples and codes using them should be also updated.

this modification does not break existing code.
some examples could be added demonstrating usage of 32 bit timers like:

  • long period measurements
  • long period wave form generation

if you agree I can make the corrections and remake the PR
and in a separate PR I can build a coulple of new examples

@GianfrancoIU1JSU GianfrancoIU1JSU changed the title fix #2070 add support for max overflow value for 32 bits timer fix #2071 add support for max overflow value for 32 bits timer Jul 17, 2023
@fpistm fpistm added the abandoned No more work on this label Jul 21, 2023
@fpistm
Copy link
Member

fpistm commented Jul 21, 2023

Hi @GianfrancoIU1JSU
Unfortunately, we remembered that the HardwareTimer library uses all timer like 16 bits timer. See

https://github.com/stm32duino/Arduino_Core_STM32/wiki/HardwareTimer-library#1-introduction

For genericity purpose, HardwareTimer library uses all timers like a 16bits timer (even if some may be wider).

So even if this PR allow you to use max 32 bits values, it is not enough to support properly 32 bits timers as some side effects can occurs.

@fpistm fpistm closed this Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned No more work on this bug 🐛 Something isn't working fix 🩹 Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HardwareTimer: support 32 bit timers
3 participants