Refactor SPI MISO setup

[Dominaezzz]

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 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• I have added necessary changes to user code to the Migration Guide.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Fixes #2511 .

Not everyone wants to configure the "output part" of the MISO signal.

My thinking here is, most users see "MISO" as an input pin/signal. If they want the bidirectional variant, they should use with_sio1 to signify they want "Serial Input/Output" rather than "MISO".

This also allows setting up loopback without unsafe.

For aesthetics, it might be worth having a with_sio0 (alongside with_miso), just so it reads better when also using sio2, sio3, etc.

Testing

CI

[bugadani]

I have two small notes:

• None of our documentation (I mean, outside of esp-hal) actually refers to the signal as SIO1, I think. Just to be clear, I think simply calling this something like miso_inout would be better than trying to follow obscure signal namings.
• It's not possible to assign input and output to different pins. While not terribly useful, the hardware supports it.

Depending on whether we want to support splitting signals between pins, MISO being singled out may be somewhat inconsistent.

[Dominaezzz]

• None of our documentation (I mean, outside of esp-hal) actually refers to the signal as SIO1, I think.

From the S3 TRM, Table 30-3 "FSPIQ | SPI3_Q | MISO/SIO1 (serial data input and output, bit1)".
Though it's the only mention of it.

MISO being singled out may be somewhat inconsistent.

The inconsistency is coming from the hardware, not the hal.

Depending on whether we want to support splitting signals between pins

Sending a peripheral's output signal to multiple pins is much more useful and will probably look very similar. Might as well support both before cementing the API 🙂

[bugadani]

Sending a peripheral's output signal to multiple pins is much more useful

It is possible to connect different output signals, OutputSignal::connect_to does just that. As the Instance traits are public and they contain the peripheral signals, I don't think we need to do really anything to enable that case. We don't really bind the pin to the signal, so I guess from a type safety standpoint we might change that, but the functionality itself is there. As this is probably not a common use case, I'd not change the peripheral drivers more, except document the possibility.

I am not quite sure about naming. Ideally I'd like to avoid multiple functions that have different names but do the exact same thing. But I have to acknowledge that QSPI/OSPI naming differs from SPI, and that the same bus can be used to connect different devices, so we need to cover all this in a single driver.

I guess considering this, with_sio1 makes sense. I really, really resist the idea of with_sio0, though, but it just looks wrong not to have it if the user really wants to configure a QSPI/Octal SPI bus.

Well, there's my long rant that includes me coming around to this idea.

[Dominaezzz]

Regarding the multiple pins stuff. I had imagined something along the lines of with_miso(OutputGroup([peripherals.GPIO1, peripherals.GPIO2, ....])). So the drivers won't change, it'd be just a new implementation of PeripheralOutput.

It is possible to connect different output signals, OutputSignal::connect_to does just that.

Interesting, I didn't realize that the intention was for these to be public. It has some interesting consequences, ownership wise. I'll open a separate issue about this.

[bugadani]

I had imagined something along the lines of with_miso(OutputGroup([peripherals.GPIO1, peripherals.GPIO2, ....])). So the drivers won't change, it'd be just a new implementation of PeripheralOutput.

I've thought about this but since pins are different types, it's not this simple. Tuples would work.

[bugadani]

Interesting, I didn't realize that the intention was for these to be public.

It was, but they are hidden currently as there was no effort made to actually hammer out the kinks. That being said, the hardware ensures that the worst that can happen is old settings getting overwritten - or external hardware blowing up by old connections getting stuck 🥲

[bjoernQ]

To me

.with_mosi(p1)
.with_sio1(p2)

looks weird.

But I have to admit also this

.with_mosi(p1)
.with_miso(p2)
.with_sio2(p3)
.with_sio3(p4)

already looked weird - so it's not worse at least

[Dominaezzz]

I'll come back to this once I figure out how to craft up a good migration guide.

But I'll close this for now as there's already enough (almost 30) lingering PRs that aren't being updated.

[MabezDev]

Would you mind reopening this? We've got through most of the backlog now, sorry about that 😅

[Dominaezzz]

Sure! I still need to figure the migration guide so it might still linger for a while.