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

Rtc v2 #136

Merged
merged 32 commits into from
Sep 8, 2020
Merged

Rtc v2 #136

merged 32 commits into from
Sep 8, 2020

Conversation

David-OConnor
Copy link
Contributor

Based on #93, by @smedellin90

Has most of the review changes implemented, but is missing the unsafe justifications; I'm not sure how to do them.

Copy link
Collaborator

@teskje teskje 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 your effort! Most of my comments should be pretty easy for you to fix, even though I realize many apply to code not originally written by you.

Cargo.toml Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Show resolved Hide resolved
@David-OConnor
Copy link
Contributor Author

All reviews addressed.

Copy link
Collaborator

@teskje teskje 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! Just a small suggestion, I think some of these modifys should actually be writes now. I marked them with comments.

src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
src/rtc.rs Outdated Show resolved Hide resolved
@smedellin90
Copy link

@David-OConnor , thank you for getting around to cleaning up and improving the code!
@ra-kete , thank you for the reviewing and your patience!

@teskje
Copy link
Collaborator

teskje commented Sep 8, 2020

LGTM. Can you add a changelog entry too? Sorry, forgot about that before.

@David-OConnor
Copy link
Contributor Author

David-OConnor commented Sep 8, 2020

Done. @smedellin90 : Thanks for making this. I've been relying on it for months!

@teskje teskje merged commit 788aedb into stm32-rs:master Sep 8, 2020
@teskje teskje mentioned this pull request Sep 8, 2020
sprhawk pushed a commit to sprhawk/stm32f3xx-hal that referenced this pull request Sep 10, 2020
Add support for the onboard real-time clock (RTC)
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