New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assign proper gpio pins to nRF52-DK #1097

Merged
merged 4 commits into from Jul 9, 2018

Conversation

Projects
None yet
4 participants
@niklasad1
Copy link
Member

niklasad1 commented Jul 8, 2018

Pull Request Overview

In nrf52dk_base we had assumed some pins are the same but they were
not. Because for nRF52DK pin 19 and pin 20 were assigned to both the
LEDS and the SPI drivers!

However, now setup_board takes very many parameters which is not
efficient i.e, can't go in the registers but since this is during
the initialization I guess it is ok!

Closes #1079

Testing Strategy

This pull request was tested on nRF52-DK

TODO or Help Wanted

  • test on nRF52840DK

Documentation Updated

- [ ] Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.

@niklasad1 niklasad1 requested a review from bradjc Jul 8, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 8, 2018

which is not efficient i.e, can't go in the registers but since this is during the initialization I guess it is ok!

What do you mean "not efficient" and "can't go into the registers"? Do you mean the arguments passed to the common intializer don't fit in registers and therefore it's probably wasting stack space? My guess is actually that it's likely just inlined, so doesn't make a difference in practice.

It is getting a bit messy, and might be worth having a utlity struct or something to make it more readable, but i think it's fine for this pull request.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 8, 2018

I have an nrf52840 in my office I can test with on Monday, or maybe @bradjc can test earlier than that (or just confirm he thinks these changes are almost certainly good).

@niklasad1

This comment has been minimized.

Copy link
Member

niklasad1 commented Jul 8, 2018

What do you mean "not efficient" and "can't go into the registers"? Do you mean the arguments passed to the common intializer don't fit in registers and therefore it's probably wasting stack space? My guess is actually that it's likely just inlined, so doesn't make a difference in practice.

Yupp, I meant there too many arguments to go in the registers and goes via the stack instead! But, bad phrasing from my side I should have been more explicit. If I'm not mistaken I think ARM passes the first four arguments into r0-r3 (assuming 32-bit sized) and the rest on the stack. But, you are probably right w.r.t inlining (haven't checked the assembly)

It is getting a bit messy, and might be worth having a utlity struct or something to make it more readable, but i think it's fine for this pull request.

Agree!

niklasad1 added some commits Jul 8, 2018

Assign proper gpio pins to nRF52-DK
In `nrf52dk_base` we had assumed some pins are the same but they were
not. Because for `nRF52DK` pin 19 and pin 20 were assigned to both the
LEDS and the SPI drivers!

However, now `setup_board` takes very many parameters which is not
efficient i.e, can't go in the registers but since this is during
the initilization I guess it is ok!

@niklasad1 niklasad1 force-pushed the nrf52dk_base/remove-hardcoded-gpio-pins branch from db096f5 to f6f4120 Jul 8, 2018

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 8, 2018

@niklasad1

This comment has been minimized.

Copy link
Member

niklasad1 commented Jul 9, 2018

@phil-levis

In general, I have more trust in LLVM/Rust than my own assembly skills but you're right I should be more careful and read assembly in these cases! However, has anyone inspected the actual stack depth in Tock?

@alevy alevy dismissed their stale review via 69a9e6e Jul 9, 2018

@bradjc
Copy link
Contributor

bradjc left a comment

I tested on the nrf52840dk board and it works, but there is some naming that is a little confusing.

pub fn new(device: usize, client: usize, client_sector: usize) -> Self {
Self {
device,
client,

This comment has been minimized.

@bradjc

bradjc Jul 9, 2018

Contributor

This should be write_protect_pin.

Self {
device,
client,
client_sector,

This comment has been minimized.

@bradjc

bradjc Jul 9, 2018

Contributor

This should be hold_pin.

@alevy alevy dismissed their stale review via fc8236d Jul 9, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 9, 2018

@bradjc renamed.

&SpiPins::new(SPI_MOSI, SPI_MISO, SPI_CLK),
&Some(SpiMX25R6435FPins::new(
SPI_MX25R6435F_DEVICE,
SPI_MX25R6435F_CLIENT,

This comment has been minimized.

@bradjc

bradjc Jul 9, 2018

Contributor

I was hoping you would fix these as well :)

This comment has been minimized.

@alevy

alevy Jul 9, 2018

Member

HAPPY NOW???!!!!!

@alevy alevy force-pushed the nrf52dk_base/remove-hardcoded-gpio-pins branch from fc8236d to 802dffd Jul 9, 2018

@bradjc
Copy link
Contributor

bradjc left a comment

Haha actually no, DEVICE should be CHIP_SELECT, but I should have caught that on the first pass, so that one is one me.

@alevy alevy force-pushed the nrf52dk_base/remove-hardcoded-gpio-pins branch from 802dffd to f0f02e7 Jul 9, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 9, 2018

@bradjc OK, fixed that too. Don't say I never do nothing for you.

@alevy

alevy approved these changes Jul 9, 2018

@alevy alevy merged commit 2453754 into master Jul 9, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@alevy alevy deleted the nrf52dk_base/remove-hardcoded-gpio-pins branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment