Async #334

Dirbaio · 2021-12-20T00:23:12Z

Depends on #331 #323

Separate crate

Following the discussions on #285, this adds the async traits in a separate crate. TLDR of the reasons:

We want to be able to major-bump EHA independently, to leave the main EH crate at 1.0.
At the very least we'll have to major-bump when switching from GATs to async-fn-in-traits.
GAT-based traits require nightly, so they'd have to be behind an "unstable" Cargo feature if they were in EH.

Once we're confident EHA is "1.0 ready", we can move the traits to the main EH crate. This can be done backwards-compatibly: move the traits, then do one last update to EHA making it just reexport them from EH.

Single repo

This moves embedded-hal to a subdir, then adds another for embedded-hal-async.

The advantage of having both crates in this repository is that a single PR can update both the blocking and async traits. Since they are close mirrors of each other, it's very likely a change to one (e.g. docs) will affect the other.

Trait design

The design closely mirrors the blocking traits.

The only differrence is I have removed the iter methods such as write_iter because they don't work well with async. Fundamentally operate byte by byte, so the core has to wake up for each one. It can't kick off a single bit DMA transfer and then sleep or do something else.

HAL use

GAT-based async traits have been in use for a while at embassy-traits and have worked great. IMO the approach is proven.

Working HAL implementation for nRF chips here embassy-rs/embassy#552. STM32 coming soon.

TODO:

Crate-level docs, README
Fix CI scripts
Add changelog for the -async crate. Should it go at the root, or should each crate have its own?

… traits.

rust-highfive · 2021-12-20T00:23:15Z

r? @eldruin

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

burrbull · 2021-12-20T03:54:22Z

This is excellent. But moving embedded-hal to subdir breaks all existent PRs. Same as all links.
When we merge crates, we will have the same problem or useless subdir instead.

eldruin · 2021-12-20T08:51:20Z

Thank you for this proposal and the write-up.
However, @ryankurte and I discussed yesterday about creating a separate embedded-hal-async repository.
Advantages:

This repository stays "single-purpose".
No file shuffling now and whenever embedded-hal-async is absorbed, since that is the plan.

Drawbacks:

Not possible to change blocking/nb traits as well as async traits in a single commit.

I have created an RFC in the WG about taking over embedded-hal-async here.

Dirbaio · 2021-12-20T20:39:14Z

Git is able to track moved files, I think it should be able to merge across renames? 🤔

If moves are a concern I'd leave the subdir in when merging the crates. It's not useless, it'll avoid more file moves if/when we want to maintain more crates here. (Perhaps embedded-hal-compat? Perhaps stuff for sharing spi/i2c buses?)

and re single-purpose: I'd argue async and blocking traits are two sides of the same coin, same purpose :)

There are more advantages to have stuff in a single repo:

embedded-hal-async depends on embedded-hal (for the Error types/traits, and shared stuff like the SPI Mode/Polarity/etc enums). When are embedded-hal changes propagated to embedded-hal-async? In a single repo it's trivial, you just update both atomically. With separate repos, however:
- Option 1: wait for release. Change is done to embedded-hal repo, wait for when the change is released in embedded-hal crates.io, embedded-hal-async repo is updated to the new embedded-hal release, corresponding changes are then finally done. Problem: very slow.
- Option 2: git dep. Change is done to embedded-hal repo, embedded-hal-async repo is changed to a git dep pointing to the new embedded-hal repo, embedded-hal-async is updated. Problem: needs manual Cargo.toml changes when releasing since you can't have git deps in crates.io.
For users using git for both crates, Cargo automatically does the right thing with [patch.crates-io] with a single repo. With multiple repos, users have to micromanage the git revisions for both since it's no longer guaranteed that embedded-hal-async latest master works with embedded-hal latest master.
Updating blocking+async in the same PR allows noticing earlier if some change to the blocking traits doesn't work well in the async version (for example GAT generics are bit more fiddly), vs merging them in embedded-hal blocking and later noticing in the embedded-hal-async repo they don't work well and having to revisit.

From my experience in embassy, managing a set of interdependent crates in separate repos gets really annoying really fast. The probe-rs project is also moving to single-repo for the same reasons. I strongly suggest doing single-repo :)

Dirbaio · 2021-12-20T21:00:06Z

Another option is leaving embedded-hal at the root and having embedded-hal-async in a subdir. This means no file moving, but the file structure ends up a bit anti-intuitive IMO.

huntc · 2021-12-20T21:05:36Z

I would also like to voice my support for a single repo as the start point. CI, release cadence, team structure and perhaps other factors can influence the decision on multiple vs single repositories. However, I’ve also come to learn that starting with a single repository generally works out well.

Also, async and non-async embedded development solves the same problem, sometimes together, so it feels right to combine them in this repository. We can avoid promoting a division between async and non-async development. :-)

eldruin · 2021-12-20T21:31:44Z

If embedded-hal-async would be long-lived, I agree with all the pain points. Especially the inter-dependency problems.
Somehow I imagined embedded-hal-async to not live that long in the grand scheme of things, though.
Just have a few major bumps and then integrate into embedded-hal, at which point the separate repository would be archived and async traits would just be a regular part of embedded-hal.
That is what we are doing with embedded-can, for example.

Opinions @rust-embedded/hal?

Dirbaio · 2021-12-20T21:35:23Z

embedded-hal-async will live for at least max(e-h1.0 gets released, async-fn-in-traits is stable in Rust). I guess the limiting factor will be the second, which can be easily 1 year or more.

Co-authored-by: Lachlan Sneff <lachlan.sneff@gmail.com>

Dirbaio · 2021-12-20T21:46:37Z

so perhaps the sweet spot is to have only async-embedded-hal in a subdir? I've change the PR to do that.

No pain of moving files, no pain of multiple repos. The only pain is the "weird" file structure, but that's much less than separate repos IMO, and it's temporary until they get merged.

burrbull · 2021-12-21T03:12:52Z

so perhaps the sweet spot is to have only async-embedded-hal in a subdir?

Yes, should be the best way.

eldruin · 2021-12-21T08:03:14Z

That would be an acceptable compromise, thanks!

ryankurte

lgtm, thanks for bearing with us!

ryankurte · 2021-12-21T20:26:48Z

embedded-hal-async/src/serial.rs

+        Self: 'a;
+
+    /// Reads words from the serial interface into the supplied slice.
+    fn read<'a>(&'a mut self, read: &'a mut [Word]) -> Self::ReadFuture<'a>;


i don't really want to derail this PR further / we can totally discuss elsewhere, but, it seems to me this needs to be able to return a partially filled result.

say you're reading serial payloads of size up to N, to use this trait you're going to need to always poll with &[Word; 1] to guarantee you'll return, undermining the benefits of buffering / DMA.

i think in this case the trait would be more useful with Result<usize, Self::Error> that tells you how full the buffer is, then the HAL can choose to return on EOF / timeout / buffer full as is relevant for the hardware.

The problem with that is it's not specified when it returns, so different impls will do different things, making it useless for drivers.

In Embassy we specified it like this:

read returns on buffer full.

read_until_idle returns on buffer full, or when at least 1 byte has been received and then the line goes idle.

read_until_idle is a separate trait because some chips (nrf) can't do it natively so they need a hardware timer to do it. Forcing everyone to waste a timer when not needed would be rude, so there's a special impl supporting it that you can opt-in to use.

in my experience, read is still useful if you know your packets have a fixed size, and it maps to a single DMA read operation which is the lowest overhead possible.

(this also applies to the uart blocking traits, IMO we should have a way to do reads without nb. The current nb read trait works really badly in the nRF because it can only do DMA reads.)

The problem with that is it's not specified when it returns, so different impls will do different things, making it useless for drivers.

i don't think adding this _may_ return a partial buffer on timeout or idle is useless? this is how reads under linux typically work, it just implies the need for a check and buffering at the driver level if you're not doing byte-wise processing.

... some chips (nrf) can't do it natively so they need a hardware timer to do it

rude of them (nrf) imo

in my experience, read is still useful if you know your packets have a fixed size, and it maps to a single DMA read operation which is the lowest overhead possible.

the problem with this approach appears when you know your packet sizes but you somehow end up out-of-sync or with corrupted data, then you have to have application timeouts to recover anyway.

342: Add embedded-hal-async crate in subdirectory r=ryankurte a=eldruin This is a simpler version of #334 where an empty `embedded-hal-async` crate is added inside a subfolder. I propose merging this and add all the traits and docs piece by piece so that they can be discussed comfortably. The CI adaption went fine except for the changelog check, which cannot check that the changelog in the `embedded-hal-async` subfolder was touched. All credit for this idea goes to `@Dirbaio` ! Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>

Dirbaio · 2022-01-11T22:36:13Z

Converting to draft, will open PRs for each individual trait, first is #344 . Will close this when they're all opened.

Dirbaio · 2022-01-12T12:07:24Z

Closing in favor of #344 #346 #347 #348 #349

Dirbaio added 7 commits December 19, 2021 09:19

spi: enforce all traits (nb+blocking) have the same Error type.

f8110e6

Add changelog.

2c40bbf

spi/blocking: group methods into read-only, write-only and read-write…

44e0da1

… traits.

spi/blocking: add read_batch, write_batch.

3307308

spi/blocking: rename exec to batch

63dd044

Add changelog.

74e9373

spi: rename inplace -> in_place

02f1371

Dirbaio requested a review from a team as a code owner December 20, 2021 00:23

rust-highfive assigned eldruin Dec 20, 2021

rust-highfive added S-waiting-on-review Review is incomplete T-hal labels Dec 20, 2021

Dirbaio mentioned this pull request Dec 20, 2021

Add asynchronous versions of most embedded-hal traits using GATs #285

Closed

Add embedded-hal-async crate.

519e99c

Co-authored-by: Lachlan Sneff <lachlan.sneff@gmail.com>

Dirbaio force-pushed the async branch from 505a609 to 519e99c Compare December 20, 2021 21:43

ryankurte approved these changes Dec 21, 2021

View reviewed changes

ryankurte reviewed Dec 21, 2021

View reviewed changes

eldruin mentioned this pull request Dec 21, 2021

Add embedded-hal-async crate in subdirectory #342

Merged

Dirbaio mentioned this pull request Jan 2, 2022

spi: group into read-only, write-only and read-write traits. #323

Closed

Dirbaio marked this pull request as draft January 11, 2022 22:35

Dirbaio mentioned this pull request Jan 12, 2022

serial: add ReadExact, ReadUntilIdle #349

Closed

Dirbaio closed this Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async #334

Async #334

Dirbaio commented Dec 20, 2021

rust-highfive commented Dec 20, 2021

burrbull commented Dec 20, 2021

eldruin commented Dec 20, 2021 •

edited

Loading

Dirbaio commented Dec 20, 2021 •

edited

Loading

Dirbaio commented Dec 20, 2021

huntc commented Dec 20, 2021

eldruin commented Dec 20, 2021 •

edited

Loading

Dirbaio commented Dec 20, 2021 •

edited

Loading

Dirbaio commented Dec 20, 2021

burrbull commented Dec 21, 2021

eldruin commented Dec 21, 2021

ryankurte left a comment

ryankurte Dec 21, 2021

Dirbaio Jan 2, 2022

ryankurte Jan 11, 2022

Dirbaio commented Jan 11, 2022

Dirbaio commented Jan 12, 2022

Async #334

Async #334

Conversation

Dirbaio commented Dec 20, 2021

Separate crate

Single repo

Trait design

HAL use

TODO:

rust-highfive commented Dec 20, 2021

burrbull commented Dec 20, 2021

eldruin commented Dec 20, 2021 • edited Loading

Dirbaio commented Dec 20, 2021 • edited Loading

Dirbaio commented Dec 20, 2021

huntc commented Dec 20, 2021

eldruin commented Dec 20, 2021 • edited Loading

Dirbaio commented Dec 20, 2021 • edited Loading

Dirbaio commented Dec 20, 2021

burrbull commented Dec 21, 2021

eldruin commented Dec 21, 2021

ryankurte left a comment

Choose a reason for hiding this comment

ryankurte Dec 21, 2021

Choose a reason for hiding this comment

Dirbaio Jan 2, 2022

Choose a reason for hiding this comment

ryankurte Jan 11, 2022

Choose a reason for hiding this comment

Dirbaio commented Jan 11, 2022

Dirbaio commented Jan 12, 2022

eldruin commented Dec 20, 2021 •

edited

Loading

Dirbaio commented Dec 20, 2021 •

edited

Loading

eldruin commented Dec 20, 2021 •

edited

Loading

Dirbaio commented Dec 20, 2021 •

edited

Loading