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

demo futures API #13

Closed
wants to merge 1 commit into from
Closed

demo futures API #13

wants to merge 1 commit into from

Conversation

vitiral
Copy link

@vitiral vitiral commented Jun 11, 2017

demoing that rust-embedded/nb#2 should be the way we go here. There really isn't a reason to create a separate API!

@vitiral
Copy link
Author

vitiral commented Jun 11, 2017

Note: there are a few outstanding questions in the comments -- but I think the basics here are sound -- we should not create a new data type when minimal no_std futures will do the job. No need to force users to do type conversion on something that is already zero cost.

@vitiral vitiral mentioned this pull request Jun 11, 2017
@japaric
Copy link
Member

japaric commented Jun 12, 2017

I'm afraid that this is not how you write future based APIs. You have to return a type that implements the Future trait. Let me show you why this API is wrong:

timer.wait(); // returns Async<()>

This is wrong because is not a blocking operation but you wanted a blocking operation. However, this code doesn't return a warning because Async is not marked as #[must_use]. If the API had returned a future then you would have gotten a warning: "futures do nothing unless polled". If you had used the current nb based API you would have gotten a warning: "unused result which must be used".

This API is not appropriate for callback-style / event-driven code as explained in rust-embedded/nb#2 (comment). You want an error instead of Async::NotReady when this HAL is used in callbacks.

This API is not compatible with the await! operator planned for futures. That operator only works with functions that return a future.

Also... isn't await supposed to just "work" with futures?

An await operator that works with futures will be added to the futures crate but you don't need the Future abstraction to implement the await generator; generators suffice as they are a more general version of futures. In fact the await operator that will land in the futures crate turns the future abstraction into ... a generator.

we should not create a new data type when minimal no_std futures will do the job

"we should not create a new data type (futures) when generators will do the job (await)". Just that generators were no close to being implemented when a mechanism for async IO was sorely needed so the futures crate was created.

No need to force users to do type conversion

What type conversion? Users can directly use the futures based API in the hal-futures crate. HAL implementers only need to implement the nb-based traits.

something that is already zero cost.

(Except that futures are not really zero cost when you compose them which is how you have to use them to write cooperative code. I really hope that generators won't have this problem but we can't tell until we test them.)

@vitiral
Copy link
Author

vitiral commented Jun 12, 2017

@japaric you were right, there were several things I was not understanding. Hopefully rust-embedded/nb#3 helps clarify. I will update this PR once we have reached a conclusion there (basically the update will change futures::Async -> futures::Poll

@homunkulus
Copy link
Contributor

☔ The latest upstream changes (presumably dbafdc7) made this pull request unmergeable. Please resolve the merge conflicts.

@vitiral
Copy link
Author

vitiral commented Sep 22, 2017

I think this can be closed now

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