Skip to content
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.

Sign up for GitHub

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

Refactor SPI MISO setup #2557

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor SPI MISO setup #2557

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2f8042
Refactor SPI MISO setup
Nov 17, 2024
323ec3d
update HIL
Nov 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions esp-hal/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- The `ledc::ChannelHW` trait is no longer generic. (#2387)
- The `I2c::new_with_timeout` constructors have been removed (#2361)
- `I2c::new()` no longer takes `frequency` and pins as parameters. (#2477)
- The `spi::master::HalfDuplexReadWrite` trait has been removed. (#2373)
- The `spi::master::HalfDuplexReadWrite` trait has been removed. (#2373, #2557)
- The `Spi::with_pins` methods have been removed. (#2373)
- The `Spi::new_half_duplex` constructor have been removed. (#2373)
- The `HalfDuplexMode` and `FullDuplexMode` parameters have been removed from `Spi`. (#2373)
- The `Spi::new_half_duplex` constructor have been removed. (#2373, #2557)
- The `HalfDuplexMode` and `FullDuplexMode` parameters have been removed from `Spi`. (#2373, #2557)
- Removed the output pin type parameter from `ledc::{Channel, ChannelIFace}` (#2388)
- Removed the output pin type parameter from `mcpwm::operator::{PwmPin, LinkedPins}` (#2388)
- Removed the output pin type parameter from `parl_io::{ClkOutPin, ClkInPin, RxClkInPin}` (#2388)
Expand Down
24 changes: 22 additions & 2 deletions esp-hal/src/spi/master.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,12 @@ use super::{DmaError, Error, SpiBitOrder, SpiDataMode, SpiMode};
use crate::{
clock::Clocks,
dma::{Channel, DmaChannelConvert, DmaEligible, DmaRxBuffer, DmaTxBuffer, Rx, Tx},
gpio::{interconnect::PeripheralOutput, InputSignal, NoPin, OutputSignal},
gpio::{
interconnect::{PeripheralInput, PeripheralOutput},
InputSignal,
NoPin,
OutputSignal,
},
interrupt::InterruptHandler,
peripheral::{Peripheral, PeripheralRef},
peripherals::spi2::RegisterBlock,
Expand Down Expand Up @@ -618,9 +623,24 @@ where

/// Assign the MISO (Master In Slave Out) pin for the SPI instance.
///
/// Enables input functionality for the pin, and connects it to the MISO
/// signal.
pub fn with_miso<MISO: PeripheralInput>(self, miso: impl Peripheral<P = MISO> + 'd) -> Self {
crate::into_mapped_ref!(miso);
miso.enable_input(true, private::Internal);

self.driver().miso.connect_to(&mut miso);

self
}

/// Assign the SIO1/MISO pin for the SPI instance.
///
/// Enables both input and output functionality for the pin, and connects it
/// to the MISO signal and SIO1 input signal.
pub fn with_miso<MISO: PeripheralOutput>(self, miso: impl Peripheral<P = MISO> + 'd) -> Self {
///
/// Note: You do not need to call [Self::with_miso] when this is used.
pub fn with_sio1<SIO1: PeripheralOutput>(self, miso: impl Peripheral<P = SIO1> + 'd) -> Self {
Copy link
Contributor

@bugadani bugadani Nov 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bugadani bugadani Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🥲

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crate::into_mapped_ref!(miso);
miso.enable_input(true, private::Internal);
miso.enable_output(true, private::Internal);
Expand Down
6 changes: 4 additions & 2 deletions hil-test/tests/spi_full_duplex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,10 @@ mod tests {
}
}

let (miso, mosi) = mosi.split();
#[cfg(pcnt)]
let (mosi_loopback_pcnt, mosi) = mosi.split();
let mosi_loopback_pcnt = miso.clone();

// Need to set miso first so that mosi can overwrite the
// output connection (because we are using the same pin to loop back)
let spi = Spi::new_with_config(
Expand All @@ -83,7 +85,7 @@ mod tests {
},
)
.with_sck(sclk)
.with_miso(unsafe { mosi.clone_unchecked() })
.with_miso(miso)
.with_mosi(mosi);

let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000);
Expand Down
Loading