search

nrf-rs / nrf-hal

A Rust HAL for the nRF family of devices
Apache License 2.0
484 stars 134 forks source link

Continued Maintenance of `nrf-rs` crates #432

Open jamesmunns opened 6 months ago

jamesmunns commented 6 months ago

This has been discussed on the #nrf-rs room since late 2023, but many of the original users + maintainers of nrf-rs crates are no longer actively using them, myself included. The HAL has not been actively maintained since Jonas left Ferrous. At the moment, @therealprof is the most active maintainer on the microbit crate, and @diondokter is still active in discussing some issues.

These days I personally am often using embassy and embassy-nrf. It is actively maintained, and a reasonable path forward for all Nordic devices outside of the nRF51, which is not supported (yet). The embassy-nrf HAL can be used in async or non-async modes.

We are looking for maintainers of the nrf-rs crates by the end of January 2024, or we will likely archive the project to prevent further PRs being opened, and prevent users from getting the wrong impression about how active this crate is.

On chat, @BartMassey, @hdoordt, and @thejpster have discussed potentially being interested in continuing limited maintenance.

Opening an issue here to surface this discussion outside of just chat.

jamesmunns commented 6 months ago

CC @nrf-rs/all, @nrf-rs/nrf51, @nrf-rs/nrf52, @nrf-rs/nrf91.

thejpster commented 5 months ago

I'm afraid I will have to withdraw my interesting in taking over the org.

therealprof commented 5 months ago

I guess we will need to figure out where to go with microbit then, since it might be a bit confusing for discovery book readers to use a little maintained crate relying on an archived HAL...

BartMassey commented 5 months ago

@therealprof I am currently figuring out whether @hdoordt and I can/should take over maintenance of both crates for exactly that reason: I am in the middle of teaching an Embedded Rust class right now. Any input is greatly appreciated. The alternatives I can find:

  1. Continue iterating the Disco Book on top of its current infrastructure. Bring the microbit and nrf-hal crates up to current and fix at least any obvious bugs and significant deficiencies.

  2. Adapt the Embassy microbit-bsp crate to provide sync interfaces for Display and whatever. Move the Disco Book on top of microbit-bsp and embassy-nrf while keeping it sync.

  3. Move the Disco Book to be full async/await Embassy on top of the microbit-bsp and embassy-nrf crates as they exist today.

Any of the three would be significant work, but I don't have any better ideas: maybe someone else does?

I'm not entirely enamored with 3, as learning embedded Rust is hard without also having to figure out async/await stuff. But it is definitely doable.

2 and 3 would probably mean abandoning the Microbit v1 in the Disco Book, as Embassy doesn't support it. I personally think this is a good idea anyhow: the Microbit v2 is readily and cheaply available, and having a tutorial based on two slightly different pieces of hardware confuses the presentation in my opinion.

hdoordt commented 5 months ago

Hi there!

As @BartMassey said, the reason we're interested in taking on maintenance of nrf-hal is that we want for there to be good support for beginning embedded programmers to work with Rust. I think that it's best to reduce the amount of working parts as much as possible, and as such refrain from introducing a runtime if that's not strictly necessary. That's why I'm leaning towards to option 1 in Bart's comment. We'd put a stop on adding features, and remove support for the microbit V1 from the discovery book.

The only way I see this work is if we get write access to the discovery repo as well. @adamgreig How does the rust-embedded org feel about this?

@therealprof would you be willing to accept pull requests from us, or alternatively give us write access to the microbit repository? (I'm assuming you're the owner here)

Dirbaio commented 5 months ago

I was actually thinking porting the book to embassy-nrf some months ago (I ordered a microbit v2, then the plans fell apart because the store decided to take my money and not ship anything :grimacing:). It doesn't look very good on the embedded ecosystem if the discovery book uses an unmaintained HAL, but IMO using a "maintenance mode, no new features" HAL doesn't look that much better.

I still want to get the book ported eventually, if you guys want to help it'd be welcome, but if not I'll do it anyway. I guess it shouldn't be a problem to have both editions of the book given we already have both one for nrf and another for stm32f3.

2 and 3 would probably mean abandoning the Microbit v1 in the Disco Book, as Embassy doesn't support it.

@lulf has opened a PR for nrf51 support. https://github.com/embassy-rs/embassy/pull/2469 . I still think we should focus on microbit v2 only for the book, thumbv7em is much nicer to work with.

we want for there to be good support for beginning embedded programmers to work with Rust. I think that it's best to reduce the amount of working parts as much as possible, and as such refrain from introducing a runtime if that's not strictly necessary

You can use embassy-nrf with no async executor at all. you can only call blocking methods of course, but that yields exactly the same UX as nrf-hal. I agree the book should not use async, or at least not from the start, it could have a later chapter that introduces it.

BartMassey commented 5 months ago

@Dirbaio Definitely would rather do one thing rather than two.

While embassy-nrf works without an executor, microbit-bsp appears to require one for quite a bit of its functionality — especially the "display". Would you be willing to help provide blocking / sync methods for microbit-bsp so that it can also be used without an executor? If so, and especially if you want to help rewrite the Disco Book (again), this makes plan 2 look like the most promising option.

I can ship you a MB2 if you need me to — I have many of them 😀.

therealprof commented 5 months ago

Hi @hdoordt,

@therealprof would you be willing to accept pull requests from us, or alternatively give us write access to the microbit repository? (I'm assuming you're the owner here)

I would be thrilled to make you and @BartMassey maintainers for the microbit crate. I'm severly lacking time to work on OSS projects at the moment so everything is put on the backburner.

That is also the reason why I'm a bit hesistant to go embassy since I've never used it and don't have the time to get acquainted with it. However, I'd have no problem if someone else is willing to take over the work and wants to take microbit to embassy.

Also I'm not quite sure what the goal of microbit-bsp is and how much those goals would be the same as those of microbit.

Literally the only condition I would put on such a project is: the crate and all examples must always be compilable with a stable Rust version. (Again, no time to follow embassy to know whether that is still required or not using nightly imposes restrictions, but that was always the massive putoff for me to look into it)

hdoordt commented 5 months ago

@therealprof Great to hear it!

Literally the only condition I would put on such a project is: the crate and all examples must always be compilable with a stable Rust version. (Again, no time to follow embassy to know whether that is still required or not using nightly imposes restrictions, but that was always the massive putoff for me to look into it)

Recently Embassy was released on crates.io, and is at least in part usable with a stable compiler. We'd have to investigate to what extent microbits features are supported by the released version embassy. I have no current experience with embassy on nRF. @Dirbaio could you give us an idea of to what extent embassy can support the microbit crate when excluding nightly-only features?

eldruin commented 5 months ago

The only way I see this work is if we get write access to the discovery repo as well. @adamgreig How does the rust-embedded org feel about this?

Generally in the REWG we apply the 4-eye principle. I would be happy to review PRs to the discovery book. Please submit them on a chapter-by-chapter basis.

lulf commented 5 months ago

Hi @hdoordt,

@therealprof would you be willing to accept pull requests from us, or alternatively give us write access to the microbit repository? (I'm assuming you're the owner here)

I would be thrilled to make you and @BartMassey maintainers for the microbit crate. I'm severly lacking time to work on OSS projects at the moment so everything is put on the backburner.

That is also the reason why I'm a bit hesistant to go embassy since I've never used it and don't have the time to get acquainted with it. However, I'd have no problem if someone else is willing to take over the work and wants to take microbit to embassy.

Also I'm not quite sure what the goal of microbit-bsp is and how much those goals would be the same as those of microbit.

The goal of microbit-bsp was primarily to experiment with building a microbit BSP on embassy, and my intention in the long run was to submit changes to the microbit crate based on the learnings from that. I'm happy to help porting microbit over to embassy and maintaining the microbit crate if you're looking for maintainers @therealprof

Literally the only condition I would put on such a project is: the crate and all examples must always be compilable with a stable Rust version. (Again, no time to follow embassy to know whether that is still required or not using nightly imposes restrictions, but that was always the massive putoff for me to look into it)

With rust 1.75 you can use embassy on stable, and moreover all crates are released on crates.io. Adding nrf51 support to embassy-nrf is the only missing piece to fully support microbit v1 which is in progress.

therealprof commented 5 months ago

@lulf Thanks for your assessment. I'd be happy to also add you as maintainer for microbit.

With rust 1.75 you can use embassy on stable, and moreover all crates are released on crates.io.

Are there still limitations on stable and if so, how severe are they?

lulf commented 5 months ago

The only limitations AFAIK is that on stable the embassy 'async task stack' uses a memory arena that is configured using a feature flag, whereas on nightly it is the ideal size which requires the TAIT (type_alias_impl_trait) nightly feature. But in practice this is not a problem, more an optimization.

BartMassey commented 5 months ago

Regardless of the ultimate fate of this crate, @hdoordt and I think it would be good to triage it and leave it in reasonable shape beforehand. I think folks have submitted good-looking PRs that deserve to be merged. If nothing else, I'm still teaching from it this academic quarter.

@jamesmunns would like maintainer privs on the repo if you are willing. Our plan is to first tag all the Issues and PRs with priority and disposition, then merge as many of the PRs and close as many of the Issues as seems to make sense and isn't too hard. I'm not sure how long this will take, but we will start in the next few days.

There's obviously still quite a bit of interest in this crate, as reflected by recent contributions. If we can get it back to current, maybe someone else will want to help maintain it with us ongoing.

mattheww commented 5 months ago

Regardless of the ultimate fate of this crate, @hdoordt and I think it would be good to triage it and leave it in reasonable shape beforehand. I think folks have submitted good-looking PRs that deserve to be merged.

This proposal seems good to me, and if it would help I would be happy to look through #431 in detail.

Another thought that occurs to me, if review is the limiting factor, is that it ought to be possible to rework #431 so that it's easy to see that it makes no change at all for users of the embedded_hal_02 traits.

(At the expense of some source-level code duplication, but perhaps that's not a big problem if the goal is "one last release".)

BartMassey commented 5 months ago

Oh wow, there have been a lot of new traits added to @qwandor's excellent embedded-hal 1.0 PR since I last looked. Thanks for pointing that out, especially since I really need the embedded-hal 1.0 SPI impls right now. Since it's mostly just new trait impls, it should be straightforwardish to still use the crate with embedded-hal 0.2 I think? I've done it some with the earlier commits, and it seems fine.

Henk and I still waiting for commit privs so we can work in this repo directly. In the meantime, I am using my fork https://github.com/BartMassey-upstream/nrf-hal with my students (and will update it again now that I know).

Once Henk and I can commit, we'll review the PRs and merge those as much as possible, and also triage and tag Issues.

qwandor commented 5 months ago

I can also help out as a maintainer if needed.

BartMassey commented 5 months ago

@qwandor That would be cool. Thanks huge for your embedded-hal 1.0 patches. We should get this all moving again for sure.

qwandor commented 4 months ago

It looks like I've been added as a member by @therealprof, thanks! I've started to go through and review some of the PRs, but it looks like I don't have permission to merge them. Can someone give me the necessary permissions? I also see that the repository has been using bors, which seems to be deprecated. Can we disable it, or use GitHub merge queues instead?

jamesmunns commented 4 months ago

@qwandor looking now...

jamesmunns commented 4 months ago

@qwandor you are on the nrf52 team, which has "maintain" privs. @hdoordt and @BartMassey were both invited to the same team.

Can you link me to a PR you can't merge? It's possible there is some other rule (missing review or CI?) that is causing problems.

qwandor commented 4 months ago

Any PR, e.g. https://github.com/nrf-rs/nrf-hal/pull/425. The "Merge pull request" button shows greyed out, I guess because it's configured to use bors and I don't have permission to override that. But bors doesn't seem to work, which is consistent with https://bors.tech/ saying that it is deprecated.

jamesmunns commented 4 months ago

@qwandor I've disabled "require bors is green" in the "branch protection rules" for the nrf-hal repo.

Is it possible to merge now?

qwandor commented 4 months ago

Yep! Thanks.

qwandor commented 4 months ago

Does anybody else want to look through #431 before I merge it?

BartMassey commented 4 months ago

Apologies for being away from this for so long. It's been a busy couple of weeks.

I've had a student apply #431 on a fork and build some of our stuff. Everything seems to work, reportedly.

Here's what I'd like to do if folks have no objections:

It's a lot, and a big project, although I've done parts of it in my own repos already. I wouldn't attempt doing it globally without the help of my students and the folks here. I also wouldn't do it unless there is general consensus that it is ok with both the nrf-hal and microbit folks. But I do think that it would leave the ecosystem in a better place while the probable transition to embassy-nrf and microbit-bsp is in progress.

Thoughts? Hell no is an acceptable response, especially with reasons. I realize that I'm new here: greater wisdom is appreciated.

qwandor commented 4 months ago

Thanks for looking at #431!

I'm pretty sure there are no breaking changes since 0.16.0; looking at the diff the only API changes are adding new methods and types, so we should be able to release the current master as 0.16.1.

Once #431 is merged I'd like to make a few more changes, in particular making the embedded-hal 0.2 dependency optional (behind a feature flag) before we make a 1.0 release.

qwandor commented 4 months ago

@BartMassey I've merged a couple more small PRs and sent #434 to prepare for a 0.16.1 release. Once that's in and published I'll merge #431.

eldruin commented 4 months ago

If you are looking into making a 1.0 release of these crates, I would advise you to have a look at the Rust API Guidelines. Complying with that is probably a bunch of additional work. One of the necessities that might be tough to achieve is all public dependencies being stable C-STABLE.

qwandor commented 4 months ago

434 is merged but I don't seem to have permission to publish nrf-hal-common to crates.io.

BartMassey commented 4 months ago

@qwandor I expected that. Hopefully we can find out who can publish for us.

BartMassey commented 4 months ago

@eldruin Good point. I'll investigate the list of pre-1.0 dependencies and report back. I'll also go through the other checklist stuff: I have my own checklist https://github.com/BartMassey/crate-release that includes checking the API guidelines. @qwandor Someone should probably do that for 0.16.1 before we publish as well.

In the worst case we publish as 0.17.0, which is not the end of the world.

BartMassey commented 4 months ago

@qwandor The consensus in the embedded meeting is that you should be able to publish as a member of the nrf-rs/nrf52 team. Please let me know any details and we'll try to debug this.