Slave spi for stm32

[vldm]

I commented to issue #623 about my efforts on implementing SPI slave.

I create two commits:

• In first demo of double spi was added and code that is needed to make spi slave work in sync code (no demo for sync version is provided, because spi.rs demo can be converted to slave just by changing two lines of code)
• In second commit i have removed a lot of set_spe because they brake the whole logic - by making first pseudo clk during (spe=false, spe=true) we also can fix this logic if implement some call back for setting slave select in it, just after spe=true

[vldm]

I am refering to this line
https://github.com/embassy-rs/embassy/blob/master/embassy-stm32/src/spi/mod.rs#L276

Is there any reason why ssi = false? for v3/v4 while for other it set to true?

[vldm]

What do you think if we will refactor api to make it builder like? It could be second way for initialization.

let builder = SpiBuilder::new(p.SPI3)
  .sck( p.PC10)
  .mosi(p.PC12)
  .build(Config { // runtime config can be kept in build, or moved to other build methods.
      frequency: Hz(1_000_000),
      ..Default::default()
});

vs:

let mut spi = Spi::new(
    p.SPI3,
    p.PC10,
    p.PC12,
    p.PC11,
    NoDma,
    NoDma,
    Hertz(1_000_000),
    Config::default(),
);

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cd53ccfdc02245cadebf0fa07a70b35d

[Dirbaio]

We're intentionally not using builders, because they become very complicated when they interact with generics. (for example the fact that calling .mosi() changes the Mosi generic param). Also, we had a NoPin exactly like yours in the past, it was removed, see #605.

Multiple constructors is the most straightforward way to do it, and there's little code duplication because they internally all call into new_inner.

[vldm]

Yes, i usually don't like builders too, but for pin configuration (because they usually are static typed) i found it verbose and clear.

The problem with constructors is that they params is annonymous, and it is much easier to make a mess with pin configs.

[Dirbaio]

I agree having so many params is unfortunate. However, modern IDEs can show inline hints with the param names (this pic is from rust-analyzer + vscode with the default config), so I don't think this justifies having a builder.

[Dirbaio]

Thanks for working on this!

I think it would be best if the SPI slave driver was a separate struct, instead of a "mode" in the same SPI struct. The reason is they're fundamentally very different, so they require different APIs:

• Slave mode must not implement the embedded-hal SPI traits, those are just for master mode.
• The concept of "frequency" doesn't make sense in slave mode, we don't choose it (the master does).
• It needs to handle the CS pin in hardware. Handling it in software is not possible, because you must do it as fast as the master drives the bus, if you do it too slow you can lose data.
• The API needs to give extra extra info about the transaction lengths. For example, if you start a transfer with 256-byte buffer but the master only sends 10 bytes (sets CS low, sends 10 bytes, sets CS high).

[vldm]

Thanks for fast response!

* The API needs to give extra extra info about the transaction lengths. For example, if you start a transfer with  256-byte buffer but the master only sends 10 bytes (sets CS low, sends 10 bytes, sets CS high).

• Are you sure that you want to support only one SS mode?
  For example in stm32 TI mode they rise NSS and then lower it before each "frame" (lowering edge - represent ss).

Good point about additional information during slave transfer. But doesn't it look like task for futures composition?
For example:

exti_ss.wait_for_low();
select(slave_spi.transfer(...), exti_ss.wait_for_high());
//Then if it was exti, check some status, and cancel transfer, or pause transfer by setting ssi=true, and continue transfer on next ss

Of course for doing this transfer should support cancellation, as i understand - currently it doesn't.

• Thats also the question about keeping NSS hardware, what kind of speed do you expect, how can i "feel" that exti is not fast, or maybe it's doxumented somewhere? (Sorry for noobish question) As i understand it's not always important to react on SS "fast". Setting SS just enforce child to prepare first bit and it can stay prepared for long time.
  But if make ss hardware on slave, why not do the same on master too?

The concept of "frequency" doesn't make sense in slave mode, we don't choose it (the master does).

Agree, but what do you think about supporting TI mode, in that mode frequency is recommended parameter.

Slave mode must not implement the embedded-hal SPI traits, those are just for master mode.

I found the comment on FullDuplex trait about "master only" purposes, but why other traits should be avoided.

Again, sorry for questions if they looks noobish, i am new in embedded rust, and looks from the view of regular rust developer.

[Dirbaio]

Thats also the question about keeping NSS hardware, what kind of speed do you expect, how can i "feel" that exti is not fast, or maybe it's doxumented somewhere? (Sorry for noobish question) As i understand it's not always important to react on SS "fast". Setting SS just enforce child to prepare first bit and it can stay prepared for long time.

Yes, in master mode, it doesn't matter if you leave a bit of time between setting SS low and starting the transfer. The slave chip "stays prepared" for longer than it needs to, but it all still works.

In slave mode, we are the slave, so we don't control the timing on anything. It's the master that decides the delay between setting SS low and starting sending data. It could do it instantly, with zero delay. If we do "wait for SS low, then start the transfer" in software, if we start the transfer too late we will lose the first bytes of incoming data. So it is broken. The result is not "it works but it's a bit slower", it's "it doesn't work".

But if make ss hardware on slave, why not do the same on master too?

in master mode you need more flexibility, for example multiple SS pins for talking to different slaves on a shared bus. Doing it in software makes this flexibility easier. In slave mode you'll never want multiple SS pins.

For example in stm32 TI mode they rise NSS and then lower it before each "frame" (lowering edge - represent ss).

TI mode is extremely rarely used. I've never seen it used. It's OK if we don't support it for now. And when we do we can make it a config param, having separate structs for master/slave doesn't prevent adding it in the future.

in that mode frequency is recommended parameter.

source?

why other traits should be avoided.

It's true it's not documented, just opened rust-embedded/embedded-hal#396

[vldm]

In slave mode, we are the slave, so we don't control the timing on anything.

But usually all slave devices can define minimum interface timings that it will accept.

Anyway, thanks, for response - i got your point.

in that mode frequency is recommended parameter.

source?
stm32f4 manual:

The result is not "it works but it's a bit slower", it's "it doesn't work".

I understood, i just cant figure out, how much exti output handling (+ ssi bit setting) is slower than hardware nss.

Just wanting to keep more freedom to user of this library, hardware NSS is limit port that user can use.
And also if this timings could be documented somewhere - it would be good for user.

I will try to implement slave module in separate pullrequest - just to check how it would look on code.
And let's discuss expediency of making separate trait (keeping current traits for master only) in embedded-hal pull-request.

[Dirbaio]

And let's discuss expediency of making separate trait (keeping current traits for master only) in embedded-hal pull-request.

I'd focus on getting it to work in embassy-stm32 first. A trait is not needed to use it, just to write hal-independent drivers.