Skip to content

feat: SPIM configuration #64

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

Merged
merged 3 commits into from
Feb 28, 2019
Merged

feat: SPIM configuration #64

merged 3 commits into from
Feb 28, 2019

Conversation

fmckeogh
Copy link
Contributor

This PR allows the configuration of the frequency, mode and over-read character of the SPIM interfaces.

@fmckeogh fmckeogh mentioned this pull request Feb 27, 2019
Copy link
Member

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, let me know what you think of the nitpicks, then I'm happy to approve/merge

Thanks!

} else if mode == MODE_2 {
w.order().msb_first().cpol().active_low().cpha().leading()
} else {
w.order().msb_first().cpol().active_low().cpha().trailing()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe panic or return Err here if not mode 0..3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are only 4 modes possible (parity and phase are booleans), so if it is not mode 0 through 2, it must be 3, and there are no other possibilities. I agree this is ugly and unclear, adding the Eq derive would mean being able to use a match which would be much nicer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a match here ;) It's the rusty way to do this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't match on the value since it doesn't implement Eq. The next embedded-hal release will fix that, but I'd prefer not to wait for that.

error: to use a constant of type `X` in a pattern, `X` must be annotated with `#[derive(PartialEq, Eq)]`

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I am very sorry, it's even written down below ... I didn't read on first ... I somehow thought it's an enum.

@fmckeogh
Copy link
Contributor Author

Oops, I forgot to fix the examples

);
// Configure mode
spim.config.write(|w| {
// Can't match on `mode` as embedded_hal::spi::Mode only derives PartialEq, not Eq
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Can't match on `mode` as embedded_hal::spi::Mode only derives PartialEq, not Eq
// Can't match on `mode` due to embedded-hal, see https://github.com/rust-embedded/embedded-hal/pull/126

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@Yatekii
Copy link
Contributor

Yatekii commented Feb 28, 2019

Looks good to me now.

The changes with the embedded-hal can be added once its new version is live.

Copy link
Member

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Thanks @Chocol4te!

@jamesmunns jamesmunns merged commit cdd573d into nrf-rs:master Feb 28, 2019
@fmckeogh fmckeogh deleted the feat_spim_config branch February 28, 2019 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants