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

Improve Spi device/peripheral docs #699

Merged
merged 4 commits into from
Oct 22, 2023
Merged

Conversation

fu5ha
Copy link
Contributor

@fu5ha fu5ha commented Oct 18, 2023

Documents the types of the Spi struct and brushes up a few other docs.

@jannic
Copy link
Member

jannic commented Oct 19, 2023

Very nice docs, thanks a lot!

//! let spi_device = peripherals.SPI0;
//! let spi_pin_layout = (mosi, sclk);
//!
//! let spi = Spi::new(spi_device, spi_pin_layout)
Copy link
Member

Choose a reason for hiding this comment

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

This still fails to compile:

help: consider giving `spi` an explicit type, where the value of const parameter `DS` is specified

That's a little bit unfortunate, as the type annotation clutters the code. Any suggestion on how to express this in a more readable way would be welcome!

Suggested change
//! let spi = Spi::new(spi_device, spi_pin_layout)
//! let spi = Spi::<_, _, _, 8>::new(spi_device, spi_pin_layout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made fu5ha#1 on top of this which is unfortunately a breaking change but would improve the type inference ergonomics for this. It's a bit unfortunate Rust can't infer the type default as-is, but yeah.

@jannic
Copy link
Member

jannic commented Oct 19, 2023

Don't worry about the CI error in "Verify build on MSRV". That's known, not caused by this change, and already fixed in #698.
But the failing doc comment in "Build and Test check" should be repaired before merging this.

@thejpster
Copy link
Member

#698 is now merged, so this just needs a rebase to fix that test, right?

@jannic jannic merged commit 5706ea2 into rp-rs:main Oct 22, 2023
8 checks passed
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