Split nb and can traits to separate crates. #394

Dirbaio · 2022-07-26T14:05:46Z

Following up from #177 (comment), this PR splits nb and can traits to separate crates.

I propose keeping these crates (and embedded-hal-async) separate forever. (instead of merging EHA into EH when stable as we originally planned when we created EHA).

Reasons for splitting nb:

its future is unclear. Some people who would like to see nb deprecated. By splitting, if we do deprecate it, we won't get stuck with it in the main embedded-hal crate forever.
It makes the module paths in the main crate cleaner.
Traits tied to a particular "execution model" in their own crates, which make the overall structure easier to understand IMO.

Reasons for splitting can:

It has some open concerns: Additional CAN trait for extended features #381 It's not clear when can::ErrorKind::Acknowledge should be returned #387
Even if we do solve these, it's nice to have separate crates for the "more specialized" traits so that they can be major-bumped in the future if more concerns arise, without having to major-bump the main embedded-hal crate.

TODO:

Name it embedded-can, not embedded-hal-can.
Move/remove docs/examples from the main crate that mention nb.
Mention the subcrates from the main crate's docs.
Add CI
move embedded-hal to a subdir as well
What should the nb, can crate versions be? 1.0? (no opinion on this from me).
- can: 0.4
- nb: 1.0

rust-highfive · 2022-07-26T14:05:49Z

r? @ryankurte

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

burrbull · 2022-07-26T18:35:34Z

Could you add links to -async and -nb crates in embedded-hal README.md ?

adamgreig · 2022-07-26T19:13:55Z

We discussed this a bunch at today's meeting (logs starting here), a few key points:

Lots of support for splitting nb traits into embedded-hal-nb (and therefore keeping embedded-hal-async separate for good)
For CAN, it seems like it might make most sense to use a new repository and publish to the old embedded-can crate rather than an embedded-hal-can; in general this approach seems favourable for more complicated protocols/peripherals/concepts like CAN, USB, DMA, etc (see also embedded-dma, embedded-i2s, etc). This way the CAN traits can change/have breaking releases/develop without impacting the embedded-hal crate versioning, without being much more work for HAL/driver/end users to actually use.

ryankurte · 2022-08-04T03:25:54Z

outcomes from #394 (comment) sound good to me!

(looking forward to daylight savings here in a couple of months so the meetings are a paletable 08:00 instead of 06:00 here 🤣)

adamgreig · 2022-08-23T18:40:39Z

Discussed in today's meeting: Let's go with putting the CAN traits into an embedded-can folder in this repository, and then we can publish a new version of embedded-can with the latest traits from embedded-hal 0.2.

Dirbaio · 2022-09-06T20:14:37Z

bors try

bors · 2022-09-06T20:16:46Z

try

Build succeeded:

Dirbaio · 2022-09-06T21:23:08Z

This should be now ready to go :)
ping @rust-embedded/hal

The changelog checker is angry because I moved CHANGELOG.md to the subdir. How do we do this? Ideally the checker would require updating changelog for crate X only if you've touched files from subdir X, but it doesn't seem that's supported...

eldruin

Thanks! I added a few comments.
If you do not find any alternative, we can remove the changelog check since it does not seem to work with workspace repos.
Note that this includes removing the blocking modules from embedded-hal::X and puts all the traits there.
This is in line with only supporting one execution model but should be noted as such.
Please add a changelog entry to the e-h crate about it.

embedded-can/Cargo.toml

embedded-can/README.md

embedded-hal-async/README.md

embedded-hal-bus/src/spi.rs

embedded-hal-nb/README.md

embedded-hal-nb/src/serial.rs

embedded-can/Cargo.toml

eldruin

After a rebase this should be good to go.
In the future let's please avoid such huge patchsets.
It just makes reviewing much more time-intensive and adds delay.
There are several easier-to-review PRs hiding inside this one.

embedded-hal-async/src/digital.rs

Dirbaio · 2022-09-26T11:27:33Z

Rebased, and split off the clippy stuff to a separate PR (#406)

- Move "project-wide" stuff (CoC, legal boilerplate) to the main README. - List crates in main README.

eldruin

Thanks!
bors r+

394: Split nb and can traits to separate crates. r=eldruin a=Dirbaio Following up from #177 (comment), this PR splits `nb` and `can` traits to separate crates. I propose keeping these crates (and `embedded-hal-async`) separate forever. (instead of merging EHA into EH when stable as we originally planned when we created EHA). Reasons for splitting `nb`: - its future is unclear. Some people who would like to see `nb` deprecated. By splitting, if we do deprecate it, we won't get stuck with it in the main `embedded-hal` crate forever. - It makes the module paths in the main crate cleaner. - Traits tied to a particular "execution model" in their own crates, which make the overall structure easier to understand IMO. Reasons for splitting `can`: - It has some open concerns: #381 #387 - Even if we do solve these, it's nice to have separate crates for the "more specialized" traits so that they can be major-bumped in the future if more concerns arise, without having to major-bump the main `embedded-hal` crate. TODO: - [x] Name it `embedded-can`, not `embedded-hal-can`. - [x] Move/remove docs/examples from the main crate that mention `nb`. - [x] Mention the subcrates from the main crate's docs. - [x] Add CI - [x] move `embedded-hal` to a subdir as well - [x] What should the nb, can crate versions be? 1.0? (no opinion on this from me). - [x] can: 0.4 - [x] nb: 1.0 Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

bors · 2022-09-26T14:30:07Z

Build failed:

clippy

.github/bors.toml

eldruin · 2022-09-26T14:31:24Z

bors retry

bors · 2022-09-26T14:31:28Z

👎 Rejected by too few approved reviews

eldruin

bors r+

bors · 2022-09-26T14:34:00Z

Build succeeded:

406: Enforce clippy in CI r=therealprof a=Dirbaio Depends on #394 CI wasn't actually enforcing clippy, because `actions-rs/clippy-check` needs to run as `pull_request_target` to get the token to add the "fancy" comments to the diff. It does nothing when ran as `pull_request`. `pull_request_target` is troublesome because it uses the yaml from `master` instead of from the PR branch, which means when bumping clippy you don't know whether CI passes until *after* it's merged. I've made it use `cargo clippy` instead, which works on `pull_request`. Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

rust-highfive assigned ryankurte Jul 26, 2022

rust-highfive added S-waiting-on-review Review is incomplete T-hal labels Jul 26, 2022

Dirbaio changed the title ~~Split nb and can traits to separate crates.~~ WIP: Split nb and can traits to separate crates. Jul 26, 2022

Dirbaio added this to the v1.0.0 milestone Aug 2, 2022

eldruin mentioned this pull request Aug 17, 2022

Ship a 1.0 release #177

Closed

Dirbaio force-pushed the split-nb-can branch 5 times, most recently from d4b3c7b to f1c4356 Compare September 6, 2022 20:10

bors bot added a commit that referenced this pull request Sep 6, 2022

Try #394:

b4d4aae

Dirbaio marked this pull request as ready for review September 6, 2022 21:21

Dirbaio requested a review from a team as a code owner September 6, 2022 21:21

Dirbaio changed the title ~~WIP: Split nb and can traits to separate crates.~~ Split nb and can traits to separate crates. Sep 6, 2022

eldruin reviewed Sep 7, 2022

View reviewed changes

Dirbaio force-pushed the split-nb-can branch 2 times, most recently from 13aa203 to 3aacca7 Compare September 13, 2022 17:39

tjallingt reviewed Sep 16, 2022

View reviewed changes

embedded-can/Cargo.toml Outdated Show resolved Hide resolved

eldruin reviewed Sep 21, 2022

View reviewed changes

embedded-hal-async/src/digital.rs Outdated Show resolved Hide resolved

Dirbaio force-pushed the split-nb-can branch 2 times, most recently from da6d127 to 66f5909 Compare September 26, 2022 11:18

Dirbaio mentioned this pull request Sep 26, 2022

Enforce clippy in CI #406

Merged

Dirbaio added 7 commits September 26, 2022 13:29

Split nb and can traits to separate crates.

3b6b07d

Move embedded-hal to a subdir, add workspace, simplify CI.

6d22f98

Update bors config.

04a2825

Reorganize READMEs

bc3fc45

- Move "project-wide" stuff (CoC, legal boilerplate) to the main README. - List crates in main README.

List creates in embedded-hal rustdocs.

1328ed7

Add crate split to changelog.

db070ba

Removed changelog check in CI.

d417c60

Dirbaio force-pushed the split-nb-can branch from 66f5909 to d417c60 Compare September 26, 2022 11:30

Dirbaio mentioned this pull request Sep 26, 2022

async: switch to async-fn-in-traits, release v0.2.0-alpha.0 #407

Merged

eldruin previously approved these changes Sep 26, 2022

View reviewed changes

eldruin reviewed Sep 26, 2022

View reviewed changes

.github/bors.toml Outdated Show resolved Hide resolved

eldruin dismissed their stale review via c48550c September 26, 2022 14:31

Do not enforce clippy

c48550c

eldruin approved these changes Sep 26, 2022

View reviewed changes

eldruin added the no changelog label Sep 26, 2022

bors bot merged commit 416aa2b into rust-embedded:master Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split nb and can traits to separate crates. #394

Split nb and can traits to separate crates. #394

Dirbaio commented Jul 26, 2022 •

edited

Loading

rust-highfive commented Jul 26, 2022

burrbull commented Jul 26, 2022

adamgreig commented Jul 26, 2022

ryankurte commented Aug 4, 2022

adamgreig commented Aug 23, 2022

Dirbaio commented Sep 6, 2022

bors bot commented Sep 6, 2022

Dirbaio commented Sep 6, 2022

eldruin left a comment

eldruin left a comment

Dirbaio commented Sep 26, 2022

eldruin left a comment

bors bot commented Sep 26, 2022

eldruin commented Sep 26, 2022

bors bot commented Sep 26, 2022

eldruin left a comment

bors bot commented Sep 26, 2022

Split nb and can traits to separate crates. #394

Split nb and can traits to separate crates. #394

Conversation

Dirbaio commented Jul 26, 2022 • edited Loading

rust-highfive commented Jul 26, 2022

burrbull commented Jul 26, 2022

adamgreig commented Jul 26, 2022

ryankurte commented Aug 4, 2022

adamgreig commented Aug 23, 2022

Dirbaio commented Sep 6, 2022

bors bot commented Sep 6, 2022

try

Dirbaio commented Sep 6, 2022

eldruin left a comment

Choose a reason for hiding this comment

eldruin left a comment

Choose a reason for hiding this comment

Dirbaio commented Sep 26, 2022

eldruin left a comment

Choose a reason for hiding this comment

bors bot commented Sep 26, 2022

eldruin commented Sep 26, 2022

bors bot commented Sep 26, 2022

eldruin left a comment

Choose a reason for hiding this comment

bors bot commented Sep 26, 2022

Dirbaio commented Jul 26, 2022 •

edited

Loading