Add dual and quad SPI support

[elipsitz]

Fixes #49

This set of commits introduces a few changes, in order to support dual and quad SPI:

• SpiDriver::new_dual and SpiDriver::new_quad constructors, to allow creating a SpiDriver while specifying additional pins and modes.
• LineWidth enum
• And a TransactionExt struct (and functions that use it), which is an analog of the spi_transaction_ext_t in esp-idf.

By adding TransactionExt, this also enables hardware support for command, address, and dummy phases.

---

Overall, this is a little bit of a weird addition. I don't think it fits in super well with the existing API, but I couldn't think of a better way to do it.

In particular, some things that wouldn't work:

• Adding LineWidth to Operation, to allow using the same methods: this won't work because Operation comes from embedded_hal, and can't be modified
• Adding line width to SpiConfig: this seemed like a nice solution, but the problem is that multi-line spi devices generally don't do all their communications with quad spi -- often they have a 1-bit wide command phase, and then a 1 or 4-bit wide address, and then a 4-bit wide data phase. If this is just set on SpiConfig, then it wouldn't be possible to have narrower and wider phases.

Internally, everything had to be changed from spi_transaction_t to spi_transaction_ext_t -- this allows specifying the width of the command/address/dummy phases, which is needed because the older methods just use the data phase, and the newer methods (may) use those other phases.

The wider widths can only be used in half-duplex mode. With the current SPI api, there isn't a good way to encode this constraint with types (like in esp_hal: https://docs.esp-rs.org/esp-hal/esp-hal/0.20.0/esp32s3/esp_hal/spi/master/trait.HalfDuplexReadWrite.html). However, esp-idf checks this, and logs an error and returns an error code.

[ivmarkov]

@elipsitz Thank you for your contribution!

This is currently not passing CI. Would you look into that?

@Vollbrecht Given that you are the more knowledgeable than me in SPI, would you spend some time looking into this PR? I'll follow up with an API review, but I feel I need your feedback on the inner workings of quad / dual SPI support.

[Vollbrecht]

Thanks for digging into the SPI driver and try to improve it !

Since the spi driver is somewhat complicated and a bit intertwined i think we should try to focus on the two aspects with clear boundaries.

• How to provide a ergonomic story for command / addr definition.
• Support for (dual / quad / octo?) line support.

Yes its unfortunate that the embedded-hal Operation doesn't directly provide a mechanism for something like a command or address phase. Internally we map the Operation to an OperationIter enum variant. Maybe there is space here for us to squeeze in a way to define addr/comand as an operation. Though this is just a wild thought.

I am ok with bolting something onto the driver to make it work in general, but i want to be careful to not make the driver worse for people that just use it as a regular single line spi. I assume the switch to spi_transaction_ext_t makes hopefully only a minimal impact. We might want to test it before assuming it has no drastic impact. Though for basic dual / quad and/or (cmd/addr) support its not strictly needed to use spi_transaction_ext_t only when you additional want the a variable length in cmd/addr length right?

I will have a closer look later though that just my first thought that came to mind.

[Vollbrecht]

Why mark this as Half-Duplex? Does the dual and quad transaction behave differently than there single counter part. E.g Half and Full duplex have a special meaning -> You can have a "read only" in FullDuplex as in "HalfDuplex".

If i understand correctly you can have both "Half" and "Full-Duplex" in dual and quad mode. Though if you also want a command and address phase than you can only use "Half" duplex. But if you are running in "Half-Duplex" mode on dual/quad mode than you cannot use DMA memory afaik.

We need to be clear about the gotchas overall here.

[Vollbrecht]

As i mention in the other post i think we need to clarify, in what sense this is true here. E.g Dual can be half and full duplex, depending if cmd/addr are used or not.

[elipsitz]

Thanks for taking a look! Let me try to address each point:

This is currently not passing CI. Would you look into that?

Looks like that's because changing from spi_transaction_t to spi_transaction_ext_t increased the stack usage / future size past the Clippy threshold. I'll hold off for now as we discuss the approach we want to take.

Why mark this as Half-Duplex? Does the dual and quad transaction behave differently than there single counter part.

Marking it there might have been the wrong place, but from my understanding, you can only do dual/quad transactions in a half-duplex transaction.

"Full-duplex" would mean that the controller and peripheral are exchanging data at the same time -- with single SPI, that would be the controller clocking out data on MOSI, and the peripheral sending it back, simultaneously, in MISO.

"Half-duplex" still allows both to send data, but not at the same time. So in single-spi, you'd have the controller clocking out over MOSI, and then (or instead) clocking in data over MISO. OR, in 3-wire single spi, you'd have the same physical line used for both.

And dual/quad are similar, where you have 2 or 4 lines, but only one side is using them at a time. The ESP32 chips, from what I can tell, don't support any sort of full-duplex dual/quad mode (which I guess would be 4 lines, half being used by the controller, half being used by the peripheral)?

Anyway, the underlying esp-idf driver does verify all of this (e.g. you would need to set HalfDuplex in the Device Config in order to use dual/quad mode). But it's hard or impossible, given the current API, to enforce this at compile-time.

I assume the switch to spi_transaction_ext_t makes hopefully only a minimal impact.

There's no functional impact, but the main impact is that the struct is bigger (12 bytes larger each, afaik), so more stack space is used. Probably not a huge issue on the ESP32 chips (there's so much RAM!) but it does have an impact (e.g. see the Clippy lint causing the CI failures).

i think we should try to focus on the two aspects with clear boundaries.

Yeah, fair. I thought adding support for spi_transaction_ext_t was the simplest way to add dual/quad/octal support, but yeah there's definitely other ways to do that.

It also has other benefits (command/address/dummy -- it's more efficient to use hardware support than having separate Operations to mimick that with a delay between each phase).

Anyway, here's another idea: what about adding a SpiDeviceDriver::transaction_with_width? Similar to https://docs.esp-rs.org/esp-idf-hal/esp_idf_hal/spi/struct.SpiDeviceDriver.html#method.transaction, but it would look like:

pub fn transaction_with_width(
    &mut self,
    operations: &mut [(Operation<'_, u8>, LineWidth)],
) -> Result<(), EspError>

So a LineWidth is added to each Operation. Note that this is the esp_idf_hal::spi::Operation type, so we could modify the existing variants, but I don't think there's a backwards-compatible way to do that.

Another option would be to add additional variants to the Operation enum:

ReadWithWidth(&'a mut [Word], LineWidth),
WriteWithWidth(&'a [Word], LineWidth),

That fits in a little bit better but maybe starts overloading the Operation too much.

[elipsitz]

@Vollbrecht -- what do you think about the alternative transaction_with_width or ReadWithWidth / WriteWithWidth approaches? Do you think that's a better way to add dual/quad functionality?

[Vollbrecht]

Regarding the full_duplex stuff. Yeah i misremembered the datasheet a bit, i could have swarm that it had this wired mode to also allow to split input output lines in a 2/2 way, but that is total bogus :D

We try to be stable with our public api, though if it make sense i have no problem with a breaking change. But we should be smart about it and not willy nelly changing stuff for people that they actually don't wan't.

Luckily for us the SpiOperation is not public and thouse we have a bit of leeway here to play around anyway. I think that could be an escape hatch to add a 3rd variant, eg a ExtOperation(spi_transaction_ext_t), and leave the standard operation as is.

We would than internally need to adapt the run method a bit and when traversing the iter chain check if we either have a chained ExtOperation or a normal one. Well and then would duplicate maybe the other function that are used for the SpiBus stuff, for the case when you want to use ExtOperation. (Maybe bringing a arbitrary constraint in that the user can only use either Operation & delay or ExtOperant & delay if that makes our life simpler) With that our existing public api would not change much or anything at all, and we only would see additions. So that way we would put anything with cmd/addr and linewith into the ExtOperation, while not strictly needed.

At least that is were my thinking currently goes into a bit.

[elipsitz]

What would adding another SpiOperation variant with spi_transaction_ext_t add? The only change that switching from spi_transaction_t to spi_transaction_ext_t brings is the additional size of the struct, but if we add another enum variant, SpiOperation will grow to the largest enum variant size anyway.

Do you think it makes sense to explore adding line width support without command/address/dummy support? If so, which API makes the most sense to you? Adding pub fn transaction_with_width to SpiDeviceDriver, adding ReadWithWidth / WriteWithWidth to the public Operation enum, or something else?

[Vollbrecht]

Ok to take a step back. With the current approach as is you got

• a implementation that has no breaking api change ( big plus)
• slight overhead introduced since everything is now a spi_transaction_ext_t (minus)
  • the current impl always sets dynamic addr / cmd length (slight minus)
• the ExtTransaction api always create only a single transaction and doesn't take a slice of transactions, so it breaks a bit with the rest of the api here in its usage pattern.
  • For the SpiBus we always take just &[u8] slices and split them accordingly internally, though splitting with add/ cmd and data is a bit more complicated. So i understand here the usage of maybe a single transaction type of api.
  • For the SpiDeviceDriver we have both single transactions but also the &[Operation] pattern.

You are using spi_transaction_ext_t so you also get dynamic length of addr/ cmd and the dummy bits. Is that something you want specifically? I am asking since its not needed for addr/cmd if one wants to live with both beeing a fixed default size. Its also not needed for the linewith since this is set in spi_transaction_t.

I think if we would just go with the PR as it's now but only go back to spi_transaction_t then i would not see any big blockers. Though maybe still adding a option on the SpiDevice api that would allow to pass something like a &[ExtOperation] since sometimes you want a chip select cycle for multiple operations, and this can only work if we have some sort iterative list.

I will sleep over it and hopefully give tomorrow a bit more feedback :D

[elipsitz]

Though maybe still adding a option on the SpiDevice api that would allow to pass something like a &[ExtOperation] since sometimes you want a chip select cycle for multiple operations, and this can only work if we have some sort iterative list.

Yeah, to be useful, if we don't support command/address phases, we would need a way of putting multiple operations with different line widths in the same transaction (same lowering of the CS pin).

The extending the transaction function seems like the best way to do that, which we can do by expanding the Operation enum to add width (in a backwards-compatible way, with new variants rather than changing existing ones). Only Read and Write make sense to have a width, hence my suggestion of a new ReadWithWidth and WriteWithWidth.

[ivmarkov]

The extending the transaction function seems like the best way to do that, which we can do by expanding the Operation enum to add width (in a backwards-compatible way, with new variants rather than changing existing ones). Only Read and Write make sense to have a width, hence my suggestion of a new ReadWithWidth and WriteWithWidth.

I don't follow the conversation too closely, but I also don't want to keep us stuck on this topic and you waiting on our feedback.
So how about you just go with ReadWithWidth and WriteWithWidth?