Skip to content

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Sep 26, 2022

Latest Rust nightlies have somewhat usable async-fn-in-trait support already! 🎉

embassy-nrf updated here embassy-rs/embassy#974

Paprecuts encountered:

  • there's this annoying error playground, workaround is to use the concrete type instead of Self::Error. This is a limitation of all async fns, not just in traits, but it hits especially hard within traits, so I dunno if there's plans to improve it.

async fn return type cannot contain a projection or Self that references lifetimes from a parent scope

Due to the last 2 I've left SpiDevice alone for now.

@Dirbaio Dirbaio requested a review from a team as a code owner September 26, 2022 14:10
@rust-highfive
Copy link

r? @eldruin

(rust-highfive has picked a reviewer for you, use r? to override)

@Dirbaio Dirbaio marked this pull request as draft September 26, 2022 14:10
@Dirbaio Dirbaio force-pushed the afit branch 2 times, most recently from 4d1a0a7 to c00fc47 Compare September 26, 2022 15:20
@korken89
Copy link

It seems the 2 issues with SpiDevice has been fixed in nighly now.

@Dirbaio
Copy link
Member Author

Dirbaio commented Oct 24, 2022

converted all traits, embedded-hal-async no longer explodes when compiling now! I encountered another rustc bug rust-lang/rust#103457 but thankfully we don't need the + 'a anymore, so just removing it worked...

I haven't tried updating embassy yet, it's still possible it explodes with actual use.

@korken89
Copy link

How are we on this, is this ready to merge?
I'm about to start writing some example drivers and would love this :D

If not I'll use a git-dep to this branch.

@Dirbaio
Copy link
Member Author

Dirbaio commented Oct 31, 2022

Since AFIT support is very recent in rustc we should check that it works end-to-end first. embedded-hal-async itself builds without ICEs now, but I haven't tried implementing it in a HAL and using it from a driver yet, it's still possible these things ICE. I wanted to update embassy but haven't found time for it yet. If you want to help test it out by using it as a git dep please go ahead! :)

When we know it works well we can merge it and release it as embedded-hal-async 0.2 :)

@korken89
Copy link

korken89 commented Nov 1, 2022

Super, then I know where it stands!
I'll git-dep for now and start hacking :)

@Dirbaio
Copy link
Member Author

Dirbaio commented Nov 1, 2022

No longer builds in nightly-2022-10-29 and newer. Filed rust-lang/rust#103850

@Dirbaio
Copy link
Member Author

Dirbaio commented Nov 1, 2022

Also, side note: A few places use T::Error instead of Self::Error because Self is not allowed in async fn return if it has lifetimes. Not a blocker since it can always be workarounded by writing the full Self type, but it can be annoying if it has lots of generics. Will likely be fixed in rust-lang/rust#103491

@Dirbaio
Copy link
Member Author

Dirbaio commented Nov 23, 2022

Status update!

So, I'm fairly certain this is ready to go now!

@Dirbaio Dirbaio changed the title wip: async: switch to async-fn-in-traits async: switch to async-fn-in-traits Nov 23, 2022
@Dirbaio Dirbaio marked this pull request as ready for review November 23, 2022 16:14
@Dirbaio Dirbaio changed the title async: switch to async-fn-in-traits async: switch to async-fn-in-traits, release v0.2.0-alpha.0 ad0b9f2 Nov 23, 2022
@Dirbaio Dirbaio changed the title async: switch to async-fn-in-traits, release v0.2.0-alpha.0 ad0b9f2 async: switch to async-fn-in-traits, release v0.2.0-alpha.0 Nov 23, 2022
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.

Looks good to me, thank you!
bors r+

@bors bors bot merged commit 812471b into master Nov 23, 2022
@bors bors bot deleted the afit branch November 23, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants