Keep all SPI sub-operations within a single transaction #50
Conversation
|
Not sure why |
|
I just realised the |
|
You can still implement it with conditional compilation of esp idf >= esp idf v4.4 |
|
I have but how do I get around the unused variable it might leave behind. Also, are there any plans to make esp-idf 4.4 a min dep? |
|
4.3 still has 21 months of support (see https://docs.espressif.com/projects/esp-idf/en/latest/esp32/versions.html#support-periods) so it's unlikely we'll stop supporting it completely for a while. With that said, we'll probably make the 4.4 branch the default soon. |
src/spi.rs
Outdated
| ) -> Result<(), SpiError> { | ||
| let flags = 0; | ||
|
|
||
| #[cfg(any(esp_idf_version = "4.4", esp_idf_version_major = "5"))] |
There was a problem hiding this comment.
Can you turn this check into !V4.3?
I.e. something like #[cfg(not(esp_idf_version = "4.3"))]
This way, if this code is still around when ESP-IDF V6 is released, it won't break (and we don't support ESP-IDF releases < 4.3).
src/spi.rs
Outdated
| break; | ||
| } | ||
|
|
||
| if offset == buf.len() && lock.is_none() { | ||
| lock = Some(self.lock_bus()?); | ||
| let is_last_chunk; |
There was a problem hiding this comment.
Nit: you can just do let is_last_chunk = if .... Will shorten the code a bit.
| @@ -336,6 +336,7 @@ impl<SPI: Spi, SCLK: OutputPin, SDO: OutputPin, SDI: InputPin + OutputPin, CS: O | |||
| for offset in (0..len).step_by(TRANS_LEN) { | |||
| let read_chunk_end = min(offset + TRANS_LEN, read.len()); | |||
| let write_chunk_end = min(offset + TRANS_LEN, write.len()); | |||
| let is_last_chunk = offset + TRANS_LEN < len; | |||
There was a problem hiding this comment.
Most important question: is the API of embedded-hal mandating that all transfers not using the Transaction traits should still be in a single transaction? I.e., if I do an innocent long write, it should be in a single transaction?
There was a problem hiding this comment.
I don't see anything mandating this in the embedded-hal API, so technically this isn't strictly breaking contract.
However, I've got some justifications.
- I'm using this to talk to an epaper driver IT8951 and it requires every transaction to have some preamble bits. I spent a few hours wondering why my code wasn't working until I came to look at the source code here. I've now ended up keeping my
writes andreads < 32 to make sure I'm in a single transaction. It's a bit awkward to be breaking up my transfers when the library I'm using is also doing the same thing. - The next version of
embedded-halwill be enforcing this. https://github.com/rust-embedded/embedded-hal/blob/b5c29b39880e467ef65ceb5534422d0e58539119/src/spi/blocking.rs . Everything has now been written in terms of transactions.
There was a problem hiding this comment.
I think the crux of the problem is that the notion of SPI transaction that ESP-IDF has does not work with buffers of arbitrary length (only up to 64 bytes), and does not work when the read buffer length is different from the write buffer length. Otherwise, we would've not be breaking the user-supplied buffers into multiple ESP-IDF "transactions".
Regarding your comment about the big upcoming changes to embedded-hal SPI traits: given that currently we support e-hal 1.0-alpha7 (in addition to e-hal 0.2) and that we plan to stay up-to-date with e-hal 1.0 pre-releases, perhaps the correct angle to approach the problem is to actually think how the new upcoming e-hal SPI traits can be implemented on top of the ESP-IDF SPI interface, and then in addition provide backwards compat with e-hal 0.2?
There was a problem hiding this comment.
Well, with DMA it can do more than 64 bytes but it's been disabled in here for some reason. Of course, I do appreciate that some people might want to disable DMA, which brings us back.
I've already given that some thought. I've also thought about how DMA might fit into it since it's something I really want.
Would it be alright if I pinged you on Matrix?
There was a problem hiding this comment.
The SpiBus trait can't be easily implemented in terms of esp-idf's spi interface, since you need a device to perform transactions but the SpiDevice can be.
There was a problem hiding this comment.
I mean, at the moment, what we have with the e-hal 0.2 is essentially only the notion of an spi bus and we emulate it that way.
There was a problem hiding this comment.
Yes it can indeed be emulated with a lock lasting the duration of the SpiBus trait lifetime. It just felt a bit kludgy at first.
There was a problem hiding this comment.
Pulling out the Bus from Master will mean breaking changes. Is that acceptable?
There was a problem hiding this comment.
Yes. Not nice but we are at 0.X, so that's okay. And, most of the user code should be coded against the 0.2 e-hal traits anyway. It is the initialization of the SPI driver that might break.
If you have cycles, let's try to explore the path and once we have concrete code, then actually we can say how much and what would break.
|
Obsoleted by #61 |
Starts to address #48 .
This makes sure that calls to
transfer,write,readandtransfer_inplaceregardless of how big the given buffer is happens within a single transaction. Users who wish to break up their transactions can easily do so themselves by calling these operations multiple times.This doesn't fix
execyet as I want to start small.