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

implement embedded-hal aplha SPI traits #488

Merged
merged 1 commit into from
Oct 30, 2022

Conversation

ptpaterson
Copy link
Contributor

This PR implements the latest embedded-hal v1.0.0-alpha.9 SPI traits (just the blocking traits).

Summary

Took advice straight from the repo to do it.

https://github.com/rust-embedded/embedded-hal/blob/9eb6dabe4694d7bf6400de7c10665d6439b0f370/embedded-hal/src/spi.rs#L132-L142

# For HAL authors

HALs **must** implement [`SpiBus`], [`SpiBusRead`] and [`SpiBusWrite`]. Users can combine the bus together with the CS pin (which should
implement [`OutputPin`](crate::digital::blocking::OutputPin)) using HAL-independent [`SpiDevice`] implementations such as the ones in [`embedded-hal-bus`](https://crates.io/crates/embedded-hal-bus).

HALs may additionally implement [`SpiDevice`] to **take advantage of hardware CS management**, which may provide some performance
benefits. (There's no point in a HAL implementing [`SpiDevice`] if the CS management is software-only, this task is better left to
the HAL-independent implementations).

HALs **must not** add infrastructure for sharing at the [`SpiBus`] level. User code owning a [`SpiBus`] must have the guarantee
of exclusive access.

Logic for read and write was also inspired by the embassy project: it took me a really long time to realize that you still have to send empty words to do read-only and drop reads to do write-only, and the embassy implementation demonstrated that.

https://github.com/embassy-rs/embassy/blob/master/embassy-rp/src/spi.rs

Testing

I tested this by rewriting mcp2515 crate implementation to use SpiDevice.

Other notes:

This will affect the existing PR #469. cc: @pietgeursen

@ptpaterson ptpaterson mentioned this pull request Oct 28, 2022
Copy link
Member

@jannic jannic left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

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.

2 participants