Support 3-wire-SPI #2919
Support 3-wire-SPI #2919
Conversation
esp-hal/src/spi/master.rs
Outdated
| || (address != Address::None && address.mode() != DataMode::SingleThreeWire) | ||
| || data_mode != DataMode::SingleThreeWire) | ||
| { | ||
| return Err(Error::ArgumentsInvalid); |
There was a problem hiding this comment.
Should we use Unsupported instead?
There was a problem hiding this comment.
I was considering that (and actually I used it first) but the description suggests something else (well I could change the description ofc) - but on the other hand looking at how it's currently used it might be fine - I don't have a preference - happy to change it
There was a problem hiding this comment.
I don't think we want to have two different error variants that mean the same thing. Why something isn't supported is, in my opinion, not that important.
There was a problem hiding this comment.
really no preference on my side - maybe there are more opinions on that - if not I will remove ArgumentsInvalid
(maybe rename Unsupported to OperationUnsupported?)
bjoernQ
force-pushed
the
three-wire-spi
branch
from
9e5dde9 to
dbc6922
Compare
esp-hal/src/spi/mod.rs
Outdated
| pub enum DataMode { | ||
| /// `Single` Data Mode - 1 bit, 2 wires. | ||
| /// Clock, CS and one data line (SIO0) | ||
| SingleThreeWire, | ||
| /// `Single` Data Mode - 1 bit, two data lines. (SIO0, SIO1) | ||
| Single, | ||
| /// `Dual` Data Mode - 2 bit, 2 wires | ||
| /// `Dual` Data Mode - 2 bits, two data lines. (SIO0, SIO1) | ||
| Dual, | ||
| /// `Quad` Data Mode - 4 bit, 4 wires | ||
| /// `Quad` Data Mode - 4 bit, 4 data lines. (SIO0 .. SIO3) | ||
| Quad, | ||
| #[cfg(spi_octal)] | ||
| /// `Octal` Data Mode - 8 bit, 8 wires | ||
| /// `Octal` Data Mode - 8 bit, 8 data lines. (SIO0 .. SIO7) | ||
| Octal, | ||
| } |
There was a problem hiding this comment.
Wild thought here with regards to naming consistency.
I think it actually makes more sense for Single to mean "3 wire half duplex".
This way, Single, Dual, Quad and Octal all mean half duplex and the name strictly refers to the number of data pins used.
Then there can be a special mode for when there are two data lines that aren't used at the same time.
There was a problem hiding this comment.
Makes sense - and did that. Maybe the now Single variant should be named ThreeWire to make things more obvious? I think three-wire and four-wire are more common names
There was a problem hiding this comment.
My issue with the "FourWire" name is that both Dual and Quad use four wires and it'll be somewhat unclear that this means "Single" but over the full duplex wiring.
There was a problem hiding this comment.
yeah - I'm open to naming suggestions
There was a problem hiding this comment.
Some inspiration
HalfAndHalfDualHalvesCoupleHalfTwoHalvesSingleSplitSplitPairSerialSingleWithTwoWiresSingle(SingleMode::ThreeWire)
bjoernQ
force-pushed
the
three-wire-spi
branch
2 times, most recently
from
c177bd6 to
7091b11
Compare
bjoernQ
force-pushed
the
three-wire-spi
branch
from
ae64707 to
5f16261
Compare
bjoernQ
force-pushed
the
three-wire-spi
branch
from
5f16261 to
098ab64
Compare
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Implement 3-wire half-duplex SPI.
Closes #2687
Closes #1826
Testing
While there is an adapted hil-test I used a C3 to simulate a slave-device and checked the implementation works for me