Add an unused pin

[david-sawatzke]

As proposed in #26 (comment). The 'example' is more of an integration test, but tests are (probably) dependent on something in std, so it doesn't work. I didn't add adc pins, since i can't imagine any reason for doing that

[HarkonenBade]

Yeah, I'd checked it out and compiling for cargo test automatically pulls in the test crate (https://github.com/rust-lang/rust/tree/master/src/libtest) which is not available on #[no_std] builds currently.

[HarkonenBade]

We could attempt to add tests based on https://github.com/japaric/utest which is a libtest replacement for micro controllers.

[therealprof]

I don't like the idea of generic Unused pins in gpio because they're not related to GPIO. Also the use of the Unused pins is a bit too simple for my taste and allows for easy user error. In my opinion it should be a spelled out explicitly if you don't want a to use a GPIO with a specific signal and if possible we should also prevent nonsense combination, like a serial without RX/TX or SPI without both MOSI/MISO (and actually also the lack of a SPI clock should not be valid).

[david-sawatzke]

I understand why you'd want to prevent usage of every pin as Unused, not quite sure if it's that important and it seems pretty involved. I mean, it's pretty obvious if you try to connect anything. There is a valid reason for needing no clk, see https://github.com/david-sawatzke/ws2812-spi-rs (using spi basically as a timed bit-banging peripheral).

allows for easy user error.

You mean something like accidentally making Tx used and Rx unused instead of the other way round?
I think you'd catch that just because there's no pin that can be both and so you'd get a compile time error

spelled out explicitly if you don't want a to use a GPIO with a specific signal

I'm not sure how you can make it much more obvious than giving an Unused struct to a pin. Or do you mean something like TxPinUnused?

[therealprof]

I understand why you'd want to prevent usage of every pin as Unused, not quite sure if it's that important and it seems pretty involved. I mean, it's pretty obvious if you try to connect anything.

Same reason why we want the compiler to check that we're using valid pins in valid configurations. I would not say it is obvious that someone nixed the wrong pin if you just read Unused in the code in some parameter location.

There is a valid reason for needing no clk, see https://github.com/david-sawatzke/ws2812-spi-rs (using spi basically as a timed bit-banging peripheral).

For me it is at least debatable whether it is a valid reason to throw out basic sanity checks because someone might want to abuse the SPI peripheral for bitbanging. SPI does not work without clocking and the main purpose of the implementation is to enable use of the SPI.

You mean something like accidentally making Tx used and Rx unused instead of the other way round?

Yes.

I'm not sure how you can make it much more obvious than giving an Unused struct to a pin. Or do you mean something like TxPinUnused?

Yes, cf. https://github.com/stm32-rs/stm32f4xx-hal/blob/master/src/serial.rs#L586

[david-sawatzke]

a valid reason to throw out basic sanity checks because someone might want to abuse the SPI peripheral for bitbanging

Unfortunately these leds are pretty popular (at least in arduino world) and that's one of the best ways to get a somewhat sane implementation across devices, so it's probably a configuration we should support (even though I'm somewhat biased).

I'm not convinced about the benefits of having separate ones, but i'll try to implement it, since it doesn't have a lot of disadvantages.

not say it is obvious that someone nixed the wrong pin if you just read Unused in the code in some parameter location.

Mmh, maybe we could declare the constructor as unsafe, so it stands out (and it is kind of unsafe to just not use intended pins)?

EDIT:

Same reason why we want the compiler to check that we're using valid pins in valid configurations.

But thats's only for stuff that's not really obvious when looking at hardware. We also don't prevent using pins that don't exist on a device, because it's obvious when looking at hardware

[therealprof]

Unfortunately these leds are pretty popular (at least in arduino world) and that's one of the best ways to get a somewhat sane implementation across devices, so it's probably a configuration we should support (even though I'm somewhat biased).

Yeah I know, I have a ton of them, too. But they're quite annoying so I switched to APA102C a while ago, so I'm also biased. ;)

I'm not sure how your driver can be platform independent. For one you have to depend on others implementing SPI in the same way and then there's still the precise timing required by the MCU and if people decide to implement their SPI in a way that's not compatible with your driver (like using buffering or DMA) then it won't work anyway.

Mmh, maybe we could declare the constructor as unsafe, so it stands out (and it is kind of unsafe to just not use intended pins)?

No, it is perfectly fine and safe to only use communication peripherals partially. But doing so should be:

• deliberate (by the user)
• obvious (to anyone reading the code)

Unused has neither property.

[david-sawatzke]

I depend on others not having too much time between writing each SPI byte, which is guranteed by having a fast enough cpu (and a somewhat sane SPI interface). That's all. They can't use buffering, because the nonblocking interface returns a byte for each byte sent and that also prevents all the dma implementations that would break this that i can think of. The timing isn't too precise, they only have to reach about 3MHz (these are very forgiving). It almost worked with the atsamd chips out of the box someone had nearby, the only issue was that idle low was incorrectly implemented.

I see very little difference between both implementations

let sck_pin = Unused::new(); // or NoSck
let mosi_pin = gpioa::PA4.into_X();
let miso_pin = Unused::new(); // or NoMiso

SPI.new(SPI1, (sck_pin, mosi_pin, miso_pin), clocks);

or

SPI.new(SPI1, (Unused::new(),gpioa::PA4.into_X(), Unused::new()), clocks);
SPI.new(SPI1, (NoSck{}, gpioa::PA4.into_X(), NoMiso{}), clocks);

And having all pins not used is really obvious in all cases.

No, it is perfectly fine and safe to only use communication peripherals partially

Safe in the memory sense, yes, probably. Safe in the 'this will/can work' sense, often not. This is also what svd2rust prevents with having it's bits methods as unsafe (I think).
You really need to think about doing this and maybe check the code of your dependencies before declaring that not using eg. Miso will work without breaking everything. (since we also don't uphold the guarantees that the embedded-hal traits make anymore)

[therealprof]

I depend on others not having too much time between writing each SPI byte, which is guranteed by having a fast enough cpu (and a somewhat sane SPI interface). That's all. They can't use buffering, because the nonblocking interface returns a byte for each byte sent and that also prevents all the dma implementations that would break this that i can think of. The timing isn't too precise, they only have to reach about 3MHz (these are very forgiving).

Reading that makes me shiver. ❄️

I really don't want to compromise regular SPI use for some hack that might or might not work.

[david-sawatzke]

I really don't want to compromise regular SPI use for some hack that might or might not work.

You also compromise regular spi usage by being able to disable Mosi and/or Miso, even though both configurations are often used, so I don't see the point. This hack is unfortunately the current standard that is also used by adafruit on the Pi and (i think) on the esp variants, so it seems like the best method.

Declaring NoSck as unsafe would also alleviate this issue a bit, since no one sane would use it then without thinking about the consequences, but not implementing it would always unnecessarily use at least one pin for these leds, while using it without any abstraction would not. (An incentive to not use an already existing implementation for the spi or forking this crate)

[therealprof]

You also compromise regular spi usage by being able to disable Mosi and/or Miso, even though both configurations are often used, so I don't see the point.

How is that a compromise? There's no point in having MOSI if a slave only streams in date and there's no point in MISO if your slave is an output device.

SPI is a synchronous bus though so it simply does not make sense to operate without clock. Similarly it makes to sense to operate without both MISO and MOSI.

Declaring NoSck as unsafe would also alleviate this issue a bit, since no one sane would use it then without thinking about the consequences, but not implementing it would always unnecessarily use at least one pin for these leds, while using it without any abstraction would not. (An incentive to not use an already existing implementation for the spi or forking this crate)

Okay.

[david-sawatzke]

We don't uphold our traits anymore, since we still have a FullDuplex trait, even though the input data is garbage. In cases like displays or maybe dacs, it isn't even obvious if the input is used, so just changing your device driver may render your code completely broken, even though the Traits are fully implemented. The same also happens with usart, but not as badly.

[therealprof]

We don't uphold our traits anymore, since we still have a FullDuplex trait, even though the input data is garbage.

Eh what? If you disconnect your device the data is similar garbage, if you have a different configuration between master and slave -> garbage, if you send wrong data -> garbage, if have a stream of data and mess up -> garbage. The only thing that a FullDuplex trait guarantees you is that you can simultaneously send as much data as you're receive. Nothing more, nothing less. Especially it doesn't say anything about the quality of the connection or the content of the data.

[david-sawatzke]

We're (the end user, technically) giving third party libraries this trait. If they update to use this trait fully (for doing lcd optimizations or whatever), functioning software may break without any warning. This seems pretty unsafe to me.

This could of course also happen by just not connecting the cables, but hardware is unsafe all the time, we can't remedy that.

[therealprof]

We're (the end user, technically) giving third party libraries this trait. If they update to use this trait fully (for doing lcd optimizations or whatever), functioning software may break without any warning. This seems pretty unsafe to me.

I've no idea what you're trying to say. Why is anything going to break? You're the one trying to abuse the SPI peripheral in undefined ways which may or may not work. The only thing the trait guarantees is that for every clock cycle generated at MSCK one bit is clocked out at MOSI and one bit clocked in at MISO, 4-16 bits at a time. Outside of the MSCK/MOSI and MSCK/MISO relationships (which are guaranteed by the MCU internally) there're absolutely guarantees whatsoever, especially no guarantees of continuous inter-word clocking. Anything can happen between frames and slaves using the SPI clock won't care because that's how you're supposed to do it...

[david-sawatzke]

I know that my thing isn't very stable, it can't be.

But for lcd stuff (for example) it's a different story. If you're using an ILI9341 display, the crate currently only writes to the display, so disabling the MISO pin works fine. If it's updated to read some status register for less waiting time (for example), the crate will (probably) break. This isn't expected behaviour for safe code, since the hardware is fine and it isn't even a semver breaking change (especially for using the non-blocking traits, which are always bidirectional).

By marking this as unsafe, we warn the user that may experience unforeseen breakage without warning and they should make sure to check what they are doing and their dependencies.

[therealprof]

@david-sawatzke There's literally no difference if the user configured the SPI incorrectly (clock speed to high, incorrect bitwidth, wrong clocking polarity/phase...) or there's a hardware problem (wires crossed, connection broken, interference, connected to a difference peripheral...).

RX-/TX-/MISO-/MOSI- only configurations are perfectly valid (case in point, RM0091 even explains all the different usecases and possibilities) and I don't want legal configurations penalised by unsafe. The only unsafe thing here is not using the SPI clock...

[david-sawatzke]

The hardware thing is the same with sck. You can also just not connect it by accident. Just because it's possible to screw up with hardware doesn't mean we should give up providing a safe environment in software. The clock speed stuff is also not great, but bitwidth is worked out via types and usually crates provide an spi configuration, so it's harder to screw up.

And these are valid configurations, just not with the traits we are handing out. If you change nothing and updates break everything because you implemented an FullDuplex without providing a Miso pin, that's not great, especially if you weren't warned beforehand (for example, by needing to use an unsafe block).

We could (and should) only implement Write for an spi peripheral which doesn't use a Miso pin. But there isn't an equivalent trait for the nonblocking & Read stuff, so we're basically hacking around limitations with the traits embedded_hal provides us, to save on pins, which is then not communicated to the driver crates.

The same is true for Tx/Rx, where this is basically only here until we do the proper implementations of the separate traits with the usarts.

[therealprof]

The hardware thing is the same with sck. You can also just not connect it by accident.

Yes, you can always mess up.

Just because it's possible to screw up with hardware doesn't mean we should give up providing a safe environment in software.

It is perfectly valid for SPI to not use MOSI or MISO. You cannot use a SPI bus without clock signal.

I'm trying to give an unsafe handle to abuse the SPI peripheral to do your bitbanging. You're free to take it or leave it.

If you change nothing and updates break everything because you implemented an FullDuplex without providing a Miso pin, that's not great, especially if you weren't warned beforehand (for example, by needing to use an unsafe block).

You always need domain knowledge, either because you have it yourself or someone told you. At the moment you would have to waste a suitable pin that is not hooked up to anything to use the peripheral (or worse you might be screwed because the hardware is using the suitable pin for something else already). If the hardware designer tells you: You need to use this configuration to talk to the component, then you want to be able to set exactly that.

We could (and should) only implement Write for an spi peripheral which doesn't use a Miso pin.

There is, it's called send. transfer is a combination of send and read because (surprise!) you actually have to send before you can read but can happily send without reading.

The same is true for Tx/Rx, where this is basically only here until we do the proper implementations of the separate traits with the usarts.

No, again it is perfectly valid to use either RX or TX solo or both together and there're plenty of usecases why to do that.

[david-sawatzke]

No, again it is perfectly valid to use either RX or TX solo or both together and there're plenty of usecases why to do that.

Yeah, but you shouldn't get an serial::Read trait out of a Peripheral which only has an Tx Pin safely.

There is, it's called send.

The trait is blocking::spi::Write, and there's no read equivalent. You shouldn't be able to get to get a peripheral that implements blocking::spi::Write (and transfer) without an MOSI pin safely.

We should implement these separately for peripherals, without needing the other things, maybe in a future PR.

Unfortunately there doesn't exist an blocking::spi::Read trait equivalent or a nonblocking, non-duplex spi traits.
It's perfectly valid to do so, but unfortunately embedded_hal currently doesn't have traits for that which you can use.

I'd maybe hold off on implementing this for pins that can already be represented with embedded_hal traits just by adding an implementation of the pin combination, like Miso with only the Write trait & both the serial pins.

[therealprof]

Yeah, but you shouldn't get an serial::Read trait out of a Peripheral which only has an Tx Pin safely.

I agree, also ideally you wouldn't be able to use NoTxPin and NoRxPin at the same time because that doesn't make sense.

The trait is blocking::spi::Write, and there's no read equivalent. You shouldn't be able to get to get a peripheral that implements blocking::spi::Write (and transfer) without an MOSI pin safely.

Actually we could do read without write (and also without MOSI), turning on the clock is enough to receive data via MISO.

It's perfectly valid to do so, but unfortunately embedded_hal currently doesn't have traits for that which you can use.

Feel free to propose it. Makes sense for me to have it.