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

delay: make infallible. #432

Merged
merged 1 commit into from
Mar 7, 2023
Merged

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Dec 20, 2022

1.0 alphas have been out for a while, some HALs out there implement it. I haven't seen one with an actual fallible delay.

I know the decision was made to make Delay fallible for consistency, but I think we should make one exception for just this one trait.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

I actually also have a hard time thinking about a failure mode for delay.
Maybe it could be useful in some HAL to end the delay early because something happened in an interrupt? (Same thing for the async version, without the need for an actual interrupt)

@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 16, 2023

I don't think we should allow that. The HAL should restart the delaying for whatever time's left, then return when the delay actually requested has elapsed.

It'd be very annoying for drivers otherwise, they would all need to write their own "retry loop".

@dbrgn
Copy link
Contributor

dbrgn commented Jan 17, 2023

It'd be very annoying for drivers otherwise, they would all need to write their own "retry loop".

I agree with this. When doing retry loops, you don't even know how much remaining time you have to wait without a timer. And if you're already using a timer, you probably wouldn't need the delay...

@Dirbaio Dirbaio marked this pull request as ready for review February 28, 2023 19:07
@Dirbaio Dirbaio requested a review from a team as a code owner February 28, 2023 19:07
@Dirbaio
Copy link
Member Author

Dirbaio commented Feb 28, 2023

This was brought up in today's WG meeting. No much discussion was had, some people expressed support and no one raised objections.

I think it should be good to go. Shall we merge? @rust-embedded/hal

@Dirbaio Dirbaio mentioned this pull request Feb 28, 2023
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

I'm fine with this.
Since breaks the current fallibility "policy", let's ask everyone.
@ryankurte, @therealprof: could you please review this as well?

@ithinuel
Copy link
Member

ithinuel commented Mar 1, 2023

Out of curiosity, is there a usecase for external clock source that could be fallible ? (eg delay with a timer over SPI/I2C)

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

Fine with me.

bors r+

@bors bors bot merged commit b036e16 into rust-embedded:master Mar 7, 2023
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.

5 participants