-
Notifications
You must be signed in to change notification settings - Fork 223
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
Conversation
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`.
r? @ryankurte (rust-highfive has picked a reviewer for you, use r? to override) |
Hi there again, sorry if i annoy you with this spi slave :) But wanting to clarify some some usage examples:
Not always you need to give feedback to developer. (Real world example https://img.jdzj.com/UserDocument/2017y/wenzihui/dn/zl7968.pdf) 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.
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. |
oops I had this accidentally marked as draft |
There was a problem hiding this 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+
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
transfer
to return, saying "I got only 10 bytes", instead of hanging waiting for the other 246 bytes.Also IMO the
spi, spi_slave
naming is fine even if inconsistent, since the vast majorityof uses are master mode. I wouldn't name the
spi
modulespi_master
.