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

Add embedded-hal v1.0.0-alpha.8 support #106

Closed
wants to merge 3 commits into from
Closed

Add embedded-hal v1.0.0-alpha.8 support #106

wants to merge 3 commits into from

Conversation

almindor
Copy link
Contributor

Fixed Delay trait for use with embedded-hal v1.0.0-alpha.8

@almindor almindor marked this pull request as ready for review July 31, 2022 18:37
@almindor almindor requested a review from a team as a code owner July 31, 2022 18:37
@almindor almindor mentioned this pull request Jul 31, 2022
Open
@almindor
Copy link
Contributor Author

almindor commented Jul 31, 2022 •
edited
Loading

@Disasm Not quite sure how we want to support pre-releases. I guess we could keep the PRs open and just push pre-release crates.io (I think that's how e-h did it?)

This is the first in a big list of deps to support all of:
rust-embedded/riscv-rt#101
riscv-rust/e310x#26
riscv-rust/e310x-hal#45

This was referenced Jul 31, 2022
Draft
Closed
@Disasm
Copy link
Member

Disasm commented Jul 31, 2022

I think only riscv and e310x-hal require update as we should be able to use multiple riscv versions simultaneously. I think it's ok to create a release from a PR branch if it works, but please don't merge these branches to master as it may cause problems in the future.

@almindor almindor added the do not merge this code should not be merged to master label Jul 31, 2022
rmsyn
rmsyn reviewed Oct 13, 2023
View reviewed changes
src/delay.rs
#[inline]
fn delay_us(&mut self, us: u64) {
fn delay_us(&mut self, us: u32) -> Result<(), Self::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the latest embedded-hal release candidate, embedded-hal-1.0.0-alpha.11 this trait no longer returns a Result, nor has an associated Error type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we aim at 1.0.0-rc.1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we aim at 1.0.0-rc.1?

Definitely, same story there: https://docs.rs/embedded-hal/1.0.0-rc.1/embedded_hal/delay/trait.DelayUs.html

Wasn't sure about the separation, so just went with the alpha line of releases.

@romancardenas romancardenas mentioned this pull request Oct 16, 2023
Merged
@romancardenas
Copy link
Contributor

#147 points to e-h-1.0.0-rc1 and solves all the conflicts. It also bumps MSRV to 1.60.

@romancardenas
Copy link
Contributor

Closing, as this has been addressed in #147

@romancardenas romancardenas deleted the e-h-1.0.0-alpha.8 branch November 27, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmsyn rmsyn rmsyn left review comments

@romancardenas romancardenas romancardenas left review comments

Assignees
No one assigned
Labels
do not merge this code should not be merged to master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@almindor @Disasm @romancardenas @rmsyn