Skip to content
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

alternative implementation for SPI pins #145

Merged
merged 3 commits into from
Dec 2, 2019
Merged

Conversation

burrbull
Copy link
Contributor

No description provided.

@therealprof
Copy link
Member

Looks good.

@burrbull
Copy link
Contributor Author

burrbull commented Nov 26, 2019

What is better?

let spi = dp.SPI2.spi(
        (spi::NoSck, spi::NoMiso, gpioa.pa10),
...

or

let spi = dp.SPI2.spi(
        (gpio::NoPin, gpio::NoPin, gpioa.pa10),
...

or implement both?

@therealprof
Copy link
Member

let spi = dp.SPI2.spi(
        (spi::NoSck, spi::NoMiso, gpioa.pa10),

Is better. It expresses intent and thus prevents misconfiguration.

@burrbull
Copy link
Contributor Author

Ok.

But misconfiguration is not possible in second variant too.

@therealprof
Copy link
Member

But misconfiguration is not possible in second variant too.

Well, misconfiguration in the sense of the user accidentally opting out of the wrong thing. If you read NoPin you know that you're not using a pin but which one. If you only allow NoMiso then everyone can see on a first glance that you're not using a physical MISO pin.

@burrbull burrbull mentioned this pull request Nov 26, 2019
@TheZoq2
Copy link
Member

TheZoq2 commented Nov 27, 2019

I agree that NoMiso/NoMosi is better than NoPin.

I would almost like to change it to SpiPins{miso, mosi, sck} for even more clarity. I find myself always having to look up the ordering of the pins when using SPI. That is more verbose though, so we should probably allow both.

Perhaps we could add `Impl

@TheZoq2
Copy link
Member

TheZoq2 commented Dec 1, 2019

Looks good in its current state, we can probably do the thing I mentioned in the previous comment later.

Edit: Oh yea, it's missing a changelog entry, could you add that? @burrbull

@burrbull
Copy link
Contributor Author

burrbull commented Dec 1, 2019

In last commit I've implemented Pins trait for different order.
Now you can pass (sck, miso, mosi) or (mosi, miso, sck) or (mosi, NoMiso, NoSck), etc.

@TheZoq2
Copy link
Member

TheZoq2 commented Dec 2, 2019

Looks good, thanks!

@TheZoq2 TheZoq2 merged commit 6aea05b into stm32-rs:master Dec 2, 2019
@burrbull burrbull deleted the spi branch December 3, 2019 15:50
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.

None yet

3 participants