Skip to content
This repository has been archived by the owner on May 18, 2022. It is now read-only.

Updated nRF51 support #97

Merged
merged 3 commits into from Nov 29, 2019
Merged

Conversation

JJJollyjim
Copy link
Contributor

@JJJollyjim JJJollyjim commented Nov 28, 2019

This is an updated version of #59 against the lastest master, and with the code refactored into a single crate, since it turns out there are very few differences between the implementations!

To end up with clear naming without breaking backwards compatibility, I renamed rubble-nrf52 to rubble-nrf5x, and made rubble-nrf52 a shim crate that re-exports all of rubble-nrf5x - I'm happy to rethink this approach if you would prefer something else.

The nrf51 build is tested in CI, though I have not added demos yet (the nrf51 crate doesn't provide a memory.x (I'm planning to change this)).

I've got the beacon example is working, though I am yet to try the other one (I don't have a pin for UART exposed on the device I'm testing on...).

Screenshot_20191129-025638

@JJJollyjim JJJollyjim changed the title Nrf51 Updated nRF51 support Nov 28, 2019
@JJJollyjim
Copy link
Contributor Author

Whoops, looks like I'm running into a rustfmt bug in CI: rust-lang/rustfmt#3819

@JJJollyjim
Copy link
Contributor Author

Have refactored to avoid parameter attributes, should pass CI this time :)

@jonas-schievink
Copy link
Owner

Awesome! Putting all the nRF5x support in one crate is definitely the way to go, especially now that I've looked at the nRF5340 (it's again pretty much the same radio).

I've got the beacon example is working, though I am yet to try the other one (I don't have a pin for UART exposed on the device I'm testing on...).

Technically you don't need to see the output just to try it, and all the debug printing might be too much for a 16 MHz Cortex-M0 anyways. Unfortunately there's currently a bug that means that the link layer doesn't work with logging compiled out (#94), this might also apply to the nRF51 (or might not).

Will take a closer look at this later. Thanks a lot for the PR!

Copy link
Owner

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To end up with clear naming without breaking backwards compatibility, I renamed rubble-nrf52 to rubble-nrf5x, and made rubble-nrf52 a shim crate that re-exports all of rubble-nrf5x - I'm happy to rethink this approach if you would prefer something else.

Backwards-compat isn't really a concern currently (Rubble isn't in a state where people could build significant things on top of it), but it would be useful to publish a dummy release of rubble-nrf52 pointing to the new name, like I did for rubble-nrf52810. I think for now you can just delete the rubble-nrf52 crate completely. I'll publish a dummy crate once the next version is ready.

pub fn new(
radio: RADIO,
ficr: pac::FICR,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since FICR is read-only, it should suffice to pass a reference to it instead of taking ownership (which would make it inaccessible to other code).

It might even be a good idea to take that reference even for the nRF52 chips even though it isn't needed, in order to make the API more uniform (and passing an additional reference isn't really much of a burden to the user). It would allow getting rid of the #[cfg]s.

Copy link
Owner

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks a lot!

@jonas-schievink jonas-schievink merged commit 391075a into jonas-schievink:master Nov 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants