Support for different SPI configurations

[katyo]

This is awesome HAL in terms of usage safety than any other I seen before. But it isn't so yet flexible in several cases.

I would like to configure SPI for display driver, which uses unidirectional setup with only SCLK and MOSI lines.

As I can see, the Pins trait implementation for that case is missing.
Since MISO pin in target device already used as GPIO for other purpose, so I cannot use Pins<(sclk, miso, mosi)>.

Also there are some other SPI setups like a bi-directional half-duplex with single data line.

[therealprof]

Hey @katyo, thanks for this great suggestion. Indeed the implementation is rather barebones and lacking a lot of functionality. Unidirectional communication should be rather easy to implement by defining dummy pins and allowing their use like it has been done e.g. here: https://github.com/stm32-rs/stm32f4xx-hal/blob/master/src/spi.rs .

Maybe you'd like to give it a try?

[katyo]

Okay, it looks not so difficult.

[TheZoq2]

This is something I would find useful as well!

Let us know if you run into any issues if you decide to implement it!

[Windfisch]

Hi!
I made an attempt at implementing this, but it turned out more difficult than expected.
The issue is that we cannot just go the stm32f4xx-hal way by implementing traits PinMiso<SPI>, PinMosi<SPI> and PinSck<SPI>, all of which contain the NoMiso/etc. types in addition to the actual pins, and then requiring a tuple of these three traits.
This works for the STM32F4, but not with the remapping system of the F1...

My attempt ultimately fails with conflicting implementation for (_, _, _) in the second impl<SPI, PinMiso, PinMosi, PinSck> Pins<SPI> for (PinMiso, PinMosi, PinSck) clause :(

The only other way I found is to have eight impl pins<SPI1> for (..., ..., ...) { blah = true }, where the ... enumerate all possible combinations of pin and not-a-pin, and then have eight more for the blah=false case, and then eight more for SPI2 :/

Do you have a better idea on how to do this?

[Windfisch]

Okay, this attempt is a bit ugly, since it enumerates all seven possible combinations of "some pin" vs "NoPin", but it looks like it could work. (untested!)
What do you think?

[TheZoq2]

Hi, thanks for working on this!

It is indeed a tricky problem, as I also discovered when doing a similar thing to PWM in #120 which also ended up with some conflicting pin impls.

I think your latest attempt is perfectly fine, it doesn't add a lot of code and does exactly what we want. Additionally, it's not public API and unlikely to change, so even if it's not the neatest solution possible, it does what it should.

[Windfisch]

Thanks for your reply! I'd say I'm adding the same for UART and other peripherals and file it as a PR soon :).

[TheZoq2]

Sounds great, though I do foresee a potential problem if this is implemented.

Me and @therealprof discussed this in the matrix channel a while back, and I'm not sure he agrees, but I figure it might be worth pointing out anyway because I don't see why it wouldn't be a problem.

First, an assumption:

1. The configuration of the peripherals don't change depending on the remapping. I.e., even if NoMosi is passed, the perhipheral still acts as if it had a pin.
2. There is no mechanism to tell an AFIO pin which device to listen to

Look at the following code:

// Both SPI1 and 3 can use these pins
let pb3 = gpiob.into_alternate_push_pull();
let pb4 = gpiob.into_alternate_push_pull();
let pb5 = gpiob.into_alternate_push_pull();
// Set up SPI1 to only be an input. Since the configuration doesn't
// change, it will still be trying to output stuff to pb3 and pb5.
// Normally, this wouldn't be an issue, because the pins wouldn't be in alternate mode
let spi1 = Spi::spi1(..., (NoSck, NoMosi, pb4), ...);
// However, if another peripheral also uses the pins, they will
// need to be in AFIO mode, which means that we now have
// two peripherals that both try to control the 3 pins.
let spi3 = Spi::spi3(... (pb3, pb5, NoMiso), ...);

I'm not sure if there is a neat way to work around this. An easy option might be to just configure the SPI peripheral in RX-only mode if the Mosi pin is not used. However, I don't believe there is a way to not use the clock.

Another option might be to always take all the pins as input, and return new versions of the unused pins which can't be switched into AFIO mode.

[therealprof]

Ugh, I just read the RM0008 about the AFIO operation for the first time; I wasn't aware it's that bad until just now.

I guess the only sane approach here is to create special versions of the remappable pins and only allow those for peripheral use. To switch the pins into a certain mapping you have to turn in all generic pins changed by a mapping flag into a to-be-created-function which will consume all pins, activate the remapping and return the special pins which will prevent that a remapped pin can be used by a foreign peripheral. However to use them as GPIO we'd have to add another version of the pin which allows all GPIO functions but no AF functions.

[therealprof]

An easy option might be to just configure the SPI peripheral in RX-only mode if the Mosi pin is not used.

That's probably a good idea anyway but I don't see how that would prevent conflicts with other peripherals.

However, I don't believe there is a way to not use the clock.

I don't think promiscuous clock use is a big issue which needs solving at the moment.

[TheZoq2]

I guess the only sane approach here is to create special versions of the remappable pins and only allow those for peripheral use. To switch the pins into a certain mapping you have to turn in all generic pins changed by a mapping flag into a to-be-created-function which will consume all pins, activate the remapping and return the special pins which will prevent that a remapped pin can be used by a foreign peripheral. However to use them as GPIO we'd have to add another version of the pin which allows all GPIO functions but no AF functions.

Yep, this was the idea I had as well. It's ugly, and would take a bit of work to implement.

Also, this wouldn't allow the use of multiple peripherals which share a pin, but with that pin disabled by one of them.

That's probably a good idea anyway but I don't see how that would prevent conflicts with other peripherals.

Presumably, the peripheral wouldn't try to control the pins it doesn't use.

Though it does lead to another issue. Should .read be supported on a SPI device which doesn't have a MISO pin? If yes, we introduce UB, and if no, we need special cases for SPI and UART devices which don't have all pins assigned.

promiscuous clock use
I don't really understand what you mean by this. The clock I'm refering to is the SCK output of the SPI devices.

[therealprof]

Also, this wouldn't allow the use of multiple peripherals which share a pin, but with that pin disabled by one of them.

That's the whole idea, no?

STM32F1 (unlike other families) don't have a full multiplexer, they always seem to connect a pin to many AF peripherals or a simplified multiplexer (controlled by the remapping) at once.

Presumably, the peripheral wouldn't try to control the pins it doesn't use.

I don't see anything in the manual which would suggest your presumption to be true and guaranteed. I would not want to implement something like that unless the manual says, "BTW: you can use a pin with a different peripheral if you turn of some function in this peripheral". I would assume that such a configuration bit only disables some internal logic but electrically it's still connected.

I don't really understand what you mean by this. The clock I'm refering to is the SCK output of the SPI devices.

I thought you were talking about peripheral block clocks. But why would you ever want to disable SCK? You can't use use SPI without a clock.

[TheZoq2]

I don't see anything in the manual which would suggest your presumption to be true and guaranteed.

Good point.

I thought you were talking about peripheral block clocks. But why would you ever want to disable SCK? You can't use use SPI without a clock.

SPI is useful for bit-banging. For example, I control the ws2812s in my keyboard using just the MOSI pin

[therealprof]

SPI is useful for bit-banging. For example, I control the ws2812s in my keyboard using just the MOSI pin

Fair enough but how does that relate to the pin mapping? You still can't use that pin for another peripheral, only GPIO.

[Windfisch]

I suspect that if you would just let the user keep the pin, they would not only be able to use it as GPIO (fine), but also use it to configure a different, conflicting peripheral (bad); right?

edit/PS:
.read() should be supported on a SPI without MISO, because you need to wait for a write to succeed (which is when the read has finished). Or we offer a method that does not return anything but "ok" or "try again, not finished yet".

Maybe the right solution is to not have a .spi1() function alone, but additionally .spi1_nomiso, .spi1._nosck_nomosi (which consume all three pins, and return those unused pins as "tainted" pin that can work as gpio, but not as peripheral pin) etc. Eww...

Or... we give up and panic at runtime :/ (which I usually frown upon, but I can't think of any legit use case (apart from "let's see if I can break this HAL") that would lead into such problems)

[TheZoq2]

Fair enough but how does that relate to the pin mapping? You still can't use that pin for another peripheral, only GPIO.

Yea, I was still thinking that disabling parts of the functionality would free up the pins, never mind.

.read() should be supported on a SPI without MISO, because you need to wait for a write to succeed (which is when the read has finished). Or we offer a method that does not return anything but "ok" or "try again, not finished yet".

Yea, that's true. Allthough you could argue that we should have dedicated types for that to avoid confusion.

Maybe the right solution is to not have a .spi1() function alone, but additionally .spi1_nomiso, .spi1._nosck_nomosi (which consume all three pins, and return those unused pins as "tainted" pin that can work as gpio, but not as peripheral pin) etc. Eww...

Yea, that's a bit ugly, it would be nice to achieve the same thing without too many overloads. Perhaps something like

fn spi1<PINS>(...) -> (SPI, PINS::TaintedPins)

Or... we give up and panic at runtime :/ (which I usually frown upon, but I can't think of any legit use case (apart from "let's see if I can break this HAL") that would lead into such problems)

One problem with this is that you're not always getting panic messages, for example, panic-abort doesn't show anything. And even if you have panic_semihosting, you need to remember to check the openocd terminal whenever something just stops working.

Also, I could see some projects configuring pins at runtime (for example, drone firmware tends to configure that stuff via a GUI).

[therealprof]

I suspect that if you would just let the user keep the pin, they would not only be able to use it as GPIO (fine), but also use it to configure a different, conflicting peripheral (bad); right?

Yeah, so you need different types to prevent that.

[johnnyegel]

It seem like a long time since the last time anyone did anything in regards to the SPI configuration. But I just wanted to point out that SPI Clock and MOSI pins should also be possible to configure as Open Drain (not just Push-Pull). All STM32Fxxx I've worked with have 5V tolerant SPI pins. So in order to interface something which runs on 5V, you simply add a pull-up on these pins and configure them as Open Drain. As far as I can tell, this HAL will not let you do this.

[TheZoq2]

That's true. I think adding that functionality wouldn't be super difficult. If you'd like to give it a try, I could tell you where to start looking :)

[burrbull]

Super difficult

 use crate::gpio::gpioc::{PC10, PC11, PC12};
-use crate::gpio::{Alternate, Floating, Input, PushPull};
+use crate::gpio::{Alternate, Floating, Input, OpenDrain, PushPull};
 use crate::rcc::{Clocks, Enable, GetBusFreq, Reset, APB1, APB2};
 use crate::time::Hertz;
 
@@ -144,8 +144,10 @@ macro_rules! remap {
             const REMAP: bool = $state;
         }
         impl Sck<$name> for $SCK<Alternate<PushPull>> {}
+        impl Sck<$name> for $SCK<Alternate<OpenDrain>> {}
         impl Miso<$name> for $MISO<Input<Floating>> {}
         impl Mosi<$name> for $MOSI<Alternate<PushPull>> {}
+        impl Mosi<$name> for $MOSI<Alternate<OpenDrain>> {}
     };
 }

[johnnyegel]

Thanks for the quick response on this. Yes, I will patch the code and try it out during next week. Once more, thanks a lot :)