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

Timer additions: raw access to prescaler and auto-reload #322

Merged
merged 6 commits into from
May 2, 2021

Conversation

Windfisch
Copy link
Contributor

I frequently use timers for precise measurements, and unfortunately, start_countdown performs intransparent calculations which can introduce rounding errors.

This PR adds the capability to set and read the raw prescaler and auto-reload values, also we can now read the timer's count; this is useful when ARR is set to 0xFFFF, i.e. the timer is just "free-running".

@Windfisch
Copy link
Contributor Author

Hm, seems like it fails to build with cargo check --features=stm32f100,rt but works with cargo check --features=stm32f100,rt.

I don't understand why it gives me this error for stm32f100, but not for f103.

error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
   --> src/timer.rs:366:44
    |
366 |                       self.tim.arr.write(|w| w.arr().bits(arr) );
    |                                              ^^^^^^^^^^^^^^^^^ call to unsafe function

I could just wrap around an unsafe, but I'd prefer discussing this first. Is this a bug in the stm32 crate or am I missing something? Please help.

@TheZoq2
Copy link
Member

TheZoq2 commented Apr 6, 2021

I think there are some inconsistencies between the different f1 variants which causes that unsafe thingy, we can probably add an unsafe block and accept the warning it will throw on the other versions

Copy link
Member

@TheZoq2 TheZoq2 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for the review delay. I added a few notes which would be nice if they are addressed

src/timer.rs Outdated Show resolved Hide resolved
src/timer.rs Outdated Show resolved Hide resolved
@Windfisch
Copy link
Contributor Author

Just to give a update, I've seen your suggestions and will implement them later this week. Thanks for the help :)

@Windfisch Windfisch requested a review from TheZoq2 April 16, 2021 18:33
@Windfisch
Copy link
Contributor Author

I've implemented your requested changes and merged with the current master. Hopefully everything is fine now :)

@burrbull
Copy link
Member

merged with the current master

Don't merge master in PRs, you make commit history non-linear. Rebase only.

@Windfisch
Copy link
Contributor Author

Thanks for the hint, I've updated my PR.

It may be helpful to state "rebase over merge" somewhere in a contibutor's guidelines document, or just under the "Contributing" header of the README, since it is not an obvious thing to do; other projects prefer it the other way round. Or is there such a document which I failed to find?

@TheZoq2
Copy link
Member

TheZoq2 commented May 2, 2021

Great work! Adding a CONTRIBUTING document stating rebasing is prefered is a pretty good idea. Though I assume that tends to almost always be the case when working on PRs, and the debate is more often about wether or not to do the final "merge" as a rebase or true merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants