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

spi: clarify traits are for master mode. #396

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Aug 1, 2022

All dirvers out there assume the traitsare for master mode. However, there are some HALs
out there that impl the traits for SPI in slave mode, which will break when used with drivers.
(stm32f1xx-hal, stm32f4xx-hal, stm32l4xx-hal, probably more).

If we add SPI slave traits in the future, they should be a separate set of traits because the
API would look quite different:

  • you'd want the API to give extra extra info about the transaction lengths: if you start a transfer with 256-byte buffer but the master only sends 10 bytes (sets CS low, sends 10 bytes, sets CS high),
    you'd want transfer to return, saying "I got only 10 bytes", instead of hanging waiting for the other 246 bytes.
  • The bus/device split doesn't make sense in slave mode.

Also IMO the spi, spi_slave naming is fine even if inconsistent, since the vast majority
of uses are master mode. I wouldn't name the spi module spi_master.

All dirvers out there assume the traitsare for master mode. However, there are some HALs
out there that impl the traits for SPI in slave mode, which will break when used with drivers.
(stm32f1xx-hal, stm32f4xx-hal, stm32l4xx-hal, probably more).

If we add SPI slave traits in the future, they should be a separate set of traits because the
API would look quite different:

- you'd want the API to give extra extra info about the transaction lengths: if you start a transfer with 256-byte buffer but the master only sends 10 bytes (sets CS low, sends 10 bytes, sets CS high),
you'd want `transfer` to return, saying "I got only 10 bytes", instead of hanging waiting for the other 246 bytes.
- The bus/device split doesn't make sense in slave mode.

Also IMO the `spi, spi_slave` naming is fine even if inconsistent, since the vast majority
of uses are master mode. I wouldn't name the `spi` module `spi_master`.
@rust-highfive
Copy link

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

@vldm
Copy link

vldm commented Aug 1, 2022

Hi there again, sorry if i annoy you with this spi slave :)

But wanting to clarify some some usage examples:

you'd want the API to give extra extra info about the transaction lengths: if you start a transfer with 256-byte buffer but the master only sends 10 bytes (sets CS low, sends 10 bytes, sets CS high),
you'd want transfer to return, saying "I got only 10 bytes", instead of hanging waiting for the other 246 bytes.

Not always you need to give feedback to developer. (Real world example https://img.jdzj.com/UserDocument/2017y/wenzihui/dn/zl7968.pdf)
Slave spi drived vfd display.
But in it's specification it enforce:
a) min CLK timing
b) count of bits

And it's responsibility of master to make sure that it transfer needed amount of bits, so slave just wait for FINISH(LAT in case of VFD) signal in order to understand that transfer is finished.

My thoughts is that BUS configuration code are very common for slave and master, and BUS usage code (transfer/read implementation) are almost same for them.

But there is lack of cancellation with feedback.
Example of use cases, that need cancellation:

  • Handling "not enough bytes/bits" program error
  • Handling timeouts
  • Handling multi-master setup
  • Handling any other interruption that should force transfer to abort

All of them are important on master too, not only on slave.

But SpiDevice is a trait that implement SS and concrete usage scenario, and there can be used to define master/slave logic.

@Dirbaio Dirbaio added this to the v1.0.0 milestone Aug 2, 2022
@Dirbaio Dirbaio marked this pull request as ready for review August 2, 2022 18:40
@Dirbaio Dirbaio requested a review from a team as a code owner August 2, 2022 18:40
@Dirbaio
Copy link
Member Author

Dirbaio commented Aug 2, 2022

oops I had this accidentally marked as draft

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!
bors r+

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

Successfully merging this pull request may close these issues.

6 participants