jamesmunns / nrf52-hal

A Rust HAL for the nrf52 (nrf52832) chip
Apache License 2.0
6 stars 1 forks source link

RFC: shared hal impl for nrf52840? #9

Closed wez closed 6 years ago

wez commented 6 years ago

I've got my eye on these nrf52840 based boards: https://www.particle.io/mesh, so I'm doing some prep work. https://github.com/wez/nrf52840 has an up to date set of bindings generated by svd2rust, and it appears from diffing nrf52 vs that repo that most of the core peripherals have equivalent definitions.

It feels in the spirit of DRY to have a HAL that can target both nrf52 and nrf52840.

What are your thoughts on how best to structure this?

In https://github.com/wez/atsamd21-rs/ I have a collection of svd2rust generated bindings, a single hal that targets them, and some board crates that consume the hal. It's a way to do something like this, but maybe not the best way.

jamesmunns commented 6 years ago

That would work for me, I've definitely kind of squatted the nrf52 name, and eventually someone is going to be interested in the nRF52810 as well.

I think what you have in atsamd would make sense - have the nrf52 repo become the cargo workspace, containing nrf52832, nrf52840, and nrf52-hal crates, and maybe later the board support crates for the official dev boards.

CC @hannobraun, any thoughts?

I would say the other option would be to make an nrf52-rs organization.

hannobraun commented 6 years ago

I'm in favor of working together. I'm not in favor of creating one HAL crate for both models. Long ago, in the dark past, when the current ecosystem barely existed (i.e. 2016 or so), I was doing my own thing (never got to release it because I discovered Jorge's stuff, and that was much better than mine). Back then I was working on a unified crate for LPC81x and LPC82x µCs, and even though those are very similar, it was a mess of #[cfg(...)] everywhere.

Since then, I've often thought about what a better approach might be. I've come up with this:

  1. Create one HAL crate per microcontroller. I.e. nrf52832-hal, nrf52840-hal, etc.
  2. Create a shared crate they all depend on, something like nrf52-hal-shared or nrf52-hal-common.
  3. Collect stuff that's useful for multiple HALs in the shared crate. Have HAL crates re-export stuff as appropriate.

I believe this approach has several advantages:

Now, I've never tried this approach, so I can't say for sure that it's better. What I do know is that I really want to try it before attempting the #[cfg(...)] approach ever again.

Regarding the organization:

wez commented 6 years ago

I went ahead and set up a nrf-rs organization and invited you both. @therealprof: I'm also happy to invite you to pull your nrf51 crates in too, but didn't want to just send an invite out of the blue. This thread has the context. I haven't evaluated how much overlap there is between nrf51 and nrf52 or whether it makes sense to try to make nrf51 fit into what I'm planning on doing in the short term below; your input is welcomed!

@jscarrott is the owner of the nrf52840 crate; you are likewise invited to join and share your thoughts.

Here's my immediate plan:

I'll set up a nrf-rs/nrf52 repo; I'll basically take nrf52-hal and restructure it to add a subdir for the current crate and "make room" to have the shared/common hal crate as well as the hal for the rfc52480.

For the shared crate: in https://github.com/wez/atsamd21-rs/blob/master/hal/src/lib.rs#L11 I came up with a simple way to centralize some of the cfg conditionals, so that the majority of the shared implementation is done in terms of the target_device alias and thus avoid littering the crate with conditionals. I have read https://github.com/jamesmunns/nrf52-hal/issues/8 which is another technique to consider. I'll defer having a strong opinion until I extract the first shared portion and we can discuss and figure out what we think then with a concrete example.

wez commented 6 years ago

Re: peripheral crates: I'd be on board with moving them into a common repo similar to the way that https://github.com/adamgreig/stm32-rs is done. In general: fewer repos is better. Having them be separate from the hal repo should work out easier based on my experience with a couple of version bumps in atsamd21-rs.

jscarrott commented 6 years ago

@wez I'd be happy to join. Unfortunately life has gotten in the way of me working on this project the last few months. And coincidentally I was looking forward to working on my hal implementation this weekend.


From: Wez Furlong notifications@github.com Sent: Friday, August 24, 2018 7:09:52 AM To: jamesmunns/nrf52-hal Cc: John Scarrott; Mention Subject: Re: [jamesmunns/nrf52-hal] RFC: shared hal impl for nrf52840? (#9)

Re: peripheral crates: I'd be on board with moving them into a common repo similar to the way that https://github.com/adamgreig/stm32-rshttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fadamgreig%2Fstm32-rs&data=02%7C01%7C%7Cee9d9eea1d274dd0834c08d60988357a%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636706877942653738&sdata=3bNpDNVyqRnbKANUXrVn5eWseVH2Om227u%2FkX9y0jzY%3D&reserved=0 is done. In general: fewer repos is better. Having them be separate from the hal repo should work out easier based on my experience with a couple of version bumps in atsamd21-rs.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fjamesmunns%2Fnrf52-hal%2Fissues%2F9%23issuecomment-415661353&data=02%7C01%7C%7Cee9d9eea1d274dd0834c08d60988357a%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636706877942673747&sdata=U5sBocI1pJo2Nhb3iV%2FdVfElhJEr1HXb1EmC1Zg2SJk%3D&reserved=0, or mute the threadhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABKN0eT5h1nwyQN_QbUXn0YHHFckYJImks5uT5iwgaJpZM4WI5hr&data=02%7C01%7C%7Cee9d9eea1d274dd0834c08d60988357a%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636706877942683763&sdata=%2BS36jgL2fs6Y1mtpFpRzcECa2apD%2FjfFeYdOkMFnwc0%3D&reserved=0.

wez commented 6 years ago

@jscarrott I think we all have various time pressures in other aspects of our lives, so I'd like to reassure you that we're not looking for a time commitment from you. Speaking for myself: I go through phases and bursts of activity looking at various different things, so one of my goals in this particular thing is to get the ball rolling such that it hopefully keeps rolling, even if that is very slowly, or at least is able to be kept moving, while any one of us is otherwise occupied.

With that in mind, I'd like to propose that we favor making progress over rigorous consensus. By that I mean: if there are PRs open that move things forward, and others in the org are too busy to review within a reasonable time, then I'd encourage the PR author to proceed with merging and landing that PR if their judgment is that there will be no problems. I don't want to get caught up in codifying a whole bunch of rules around this; let's keep communicating and I think things will fall out just fine.

Here's an example of the spirit of collaboration that I'd like to encourage and be part of; @hannobraun had opened up a series of PRs and after a couple of days, @jamesmunns communicated about when he'd have time to properly review, but encouraged just landing the changes in the meantime to unblock: https://github.com/jamesmunns/nrf52-hal/pull/7#issuecomment-414350392

wez commented 6 years ago

@jscarrott also, glad to hear that you're ready to work on the 52840 hal! I don't want any of this to get in the way of your weekend hacking plans--I don't want to turn a fun activity into a chore!

I'd suggest using https://github.com/nrf-rs/nrf52 as a base and perhaps just copying the nrf52-hal dir to nrf52840-hal as a starting point. Don't worry about sharing code between the devices at this stage; get something working and then we can figure out how to integrate and share the common parts.

jscarrott commented 6 years ago

@wez Believe me this isn't going to get in the way of any weekend plans. All I was planning on doing was carrying on modifying the nrf52-hal to work with the 52840 which seems to align quite nicely. Having someone else involved will be much appreciated. Especially as I am not very experienced with rust, pretty much all my embedded background being c/c++ with c#/f# for the desktop.

therealprof commented 6 years ago

@wez The Nordic chips have an interesting approach to doing stuff and I think there's plenty of overlap in peripherals between NRF51 and NRF52. Joining efforts makes a lot of sense and I'd be happy to collaborate.

Regarding on how to do it: I don't think previous experience from STM32 does apply to those chips since peripherals are not bound to specific pins and switched via alternate functions but can be assigned to any available pin so there're very few differences in the register description and implementation of HAL traits for different chips.

jamesmunns commented 6 years ago

Hey, I spoke with @hannobraun, and I think both of us are on board with moving the official home to the nrf-rs repo. In the next few days, I plan to add deprecation notices to my repos and a link to the new organization.

Once we need to make the next release to nrf52 or nrf52-hal, I can add whoever to the crates.io permission list (I will open up a tracking issue to remind myself others).

I will close, as I consider this "accepted" now :)

hannobraun commented 6 years ago

I can add whoever to the crates.io permission list (I will open up a tracking issue to remind myself others).

It's possible to give crates.io permissions to a GitHub team. I think that would be a neat solution, as it would concentrate permission management in one place, GitHub.

hannobraun commented 6 years ago

@jamesmunns I've moved all open issue and pull requests over to the new repo. Feel free to deprecate away!