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.

Sign up for GitHub

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

Jump to bottom

Remove dependence on embedded-hal for Delay #344

Merged
merged 6 commits into from Jun 25, 2021
Merged

Remove dependence on embedded-hal for Delay #344

merged 6 commits into from Jun 25, 2021

Conversation

David-OConnor
Copy link
Contributor

@David-OConnor David-OConnor commented Jun 12, 2021 •
edited

Fixes #343

No functional changes. Allows you to use Delay as previously, or without including or importing embedded-hal.

@David-OConnor David-OConnor requested a review from a team as a code owner June 12, 2021 20:04
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @adamgreig (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Jun 12, 2021
@David-OConnor
Copy link
Contributor Author

I don't understand the inline bit, so LMK what you're looking for there.

Disasm
Disasm reviewed Jun 12, 2021
View reviewed changes
src/delay.rs Outdated

/// Delay using the Cortex-M systick for a certain duration, ms.
pub fn delay_ms(&mut self, ms: u32) {
self.delay_us(ms * 1_000);
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is correct not for all argument values. Please see the delay_ms implementation below.

@David-OConnor
Copy link
Contributor Author

Fixed, and took a stab at #inline.

@Disasm
Copy link
Member

Disasm commented Jun 12, 2021

I don't think it's a good idea to inline the main delay_us function, but maybe someone knows rustc better than me :) I made it a separate private function on purpose, so that all the other functions can be marked #[inline].

@David-OConnor
Copy link
Contributor Author

Changed, and made the u32-conversion syntax consistent (went with as instead of from; can make all from if you want)

@Disasm
Copy link
Member

Disasm commented Jun 12, 2021

@David-OConnor I think as produced a warning somewhere that actually doesn't make sense in this specific case. from is better in general, because you can't unintentionally change types in a way that the program still compiles: if there is no From implementation it breaks, while as "works" even for 0x12345678u32 as u8.

@David-OConnor
Copy link
Contributor Author

Done

Disasm
Disasm reviewed Jun 12, 2021
View reviewed changes
src/delay.rs Show resolved Hide resolved
@David-OConnor @Disasm
Update src/delay.rs
4a393e0 
Co-authored-by: Vadim Kaushan <admin@disasm.info>
adamgreig
adamgreig approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 25, 2021

Build succeeded:

@bors bors bot merged commit 47246ec into rust-embedded:master Jun 25, 2021
@adamgreig adamgreig mentioned this pull request Jun 25, 2021
Merged
bors bot added a commit that referenced this pull request Jul 3, 2021
@bors @adamgreig
Merge #345
45e7ab7 
345: Prepare for v0.7.3 r=therealprof a=adamgreig

We've had #337 merged for a while and it seems like it would be helpful for users to get it released, and now #344 to go with it. No other changes since then (except a small doc change) so it should be an easy release.

Co-authored-by: Adam Greig <adam@adamgreig.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Disasm Disasm Disasm left review comments

@adamgreig adamgreig adamgreig approved these changes

Assignees

@adamgreig adamgreig

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Delay: Don't rely on EH traits
4 participants
@David-OConnor @rust-highfive @Disasm @adamgreig