spi.rs needs work

[phansel]

https://github.com/stm32-rs/stm32g4xx-hal/blob/main/src/spi.rs

• embedded_hal_old has not been fully removed / replaced
• hal_02 is still used in the spi example
• little to no explanatory comments (compare: stm32h7xx-hal)
  • unclear what functions like set_bidi() actually do; do they convert it to full-duplex, or to half-duplex on a 3-wire interface?
• same issue: many instances of names abbreviated beyond being comprehensible

So far, I haven't been able to correctly read+write to an SPI device from stm32g474 with this HAL yet.

• Responses (on MISO) to writes (on MOSI) are either not captured by the device or are captured incorrectly.
• Reads, when they don't crash, make little temporal sense
  • 0x22 on the bus -> reports as [u8; 3] as [0x0c, 0x0c, 0x0c].
• With certain orders of operation, the microcontroller spontaneously panics.
• transfer() does not work at all. SCK sits at idle the entire time.

The intuitive API (spi.read(), spi.write()) type system is also broken in a way I have never seen before. rustc gets confused by spi.read() and isn't sure if it's a trait of the ADC, I2C, or different types of SPI.
spi.read(&mut out_buf) has to be fully specified as stm32g4xx_hal::prelude::SpiBus::read to even compile, and even then, the SPI read behavior is unreliable.

error[E0034]: multiple applicable items in scope
   --> src/main.rs:147:9
    |
147 |     spi.read(&mut read_buf);
    |         ^^^^ multiple `read` found
    |
note: candidate #1 is defined in the trait `_embedded_hal_adc_OneShot`
   --> /home/user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/embedded-hal-0.2.7/src/adc.rs:97:5
    |
97  |     fn read(&mut self, pin: &mut Pin) -> nb::Result<Word, Self::Error>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `stm32g4xx_hal::prelude::I2c`
   --> /home/user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/embedded-hal-1.0.0/src/i2c.rs:333:5
    |
333 |     fn read(&mut self, address: A, read: &mut [u8]) -> Result<(), Self::Error> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #3 is defined in the trait `stm32g4xx_hal::prelude::SpiBus`
   --> /home/user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/embedded-hal-1.0.0/src/spi.rs:446:5
    |
446 |     fn read(&mut self, words: &mut [Word]) -> Result<(), Self::Error>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: candidate #4 is defined in an impl of the trait `_embedded_hal_spi_FullDuplex` for the type `Spi<SPI, PINS>`
help: disambiguate the method for candidate #1
    |
147 -     spi.read(&mut read_buf);
147 +     _embedded_hal_adc_OneShot::read(&mut spi, &mut read_buf);
    |
help: disambiguate the method for candidate #2
    |
147 -     spi.read(&mut read_buf);
147 +     stm32g4xx_hal::prelude::I2c::read(&mut spi, &mut read_buf);
    |
help: disambiguate the method for candidate #3
    |
147 -     spi.read(&mut read_buf);
147 +     stm32g4xx_hal::prelude::SpiBus::read(&mut spi, &mut read_buf);
    |
help: disambiguate the method for candidate #4
    |
147 -     spi.read(&mut read_buf);
147 +     _embedded_hal_spi_FullDuplex::read(&mut spi, &mut read_buf);

[usbalbin]

cc @techmccat

[phansel]

I'll take a look at patching a few of these in time. Mega perplexed by the last one.

#91 seems related but is quite stale.

[techmccat]

The set_bidi and set_tx_only methods control the read FIFO behavior when writing (to prevent rx overflow events during write operations) and were not supposed to be public, oversight on my part (unless we want to expose a raw read/write api for bytes? i don't really think we should though)

I'm not sure what's happening with the conflicting methods though, I just tried to compile an old example I had laying around (https://github.com/techmccat/stm32g4xx-hal/blob/instance-traits/examples/spi-hal-one.rs) that didn't get merged with the hal-1 PR and even after adding calls to write() and read() it still compiles, the only thing you should need to do is import the prelude, which exposes embedded_hal::spi::SpiBus

[techmccat]

Regarding the overrun behavior, I've confirmed that it happens every time I run the test, and that it was working at least up until commit 1b7a8b4, then I guess I messed it up while updating my fork

Update: the issue seems to be here, probably because the new return value since the PAC stopped being a valid pointer to the data register?
Should've known all that pointer fuckery was bound to come back and bite me

@phansel could you check if the main branch of my fork still has problems on your side?

[phansel]

Your fork as of f5a2865 seems to perform more reliably -- it reads more than one byte mostly correctly (though my BMI323 apparently has a CHIP_ID of 0x44 instead of 0x43).

The clock timing on reads is still odd, though. At 1 MHz, there's a 20us gap between the first two read bytes, then a 75us gap between the second and third. Bytes from the third onward are followed by a uniform 40us gap, up to the maximum of 20 bytes read. Any extra bytes after index 19 in the read buffer are zeroed out. It's not clear where this limit of 20 bytes comes from.

The last issue I mentioned (conflicting methods) is still present. For reference, these are the imports I'm using:

use cortex_m_rt::entry;
use stm32g4xx_hal as hal;
use defmt::{Format, debug, error, info, trace, warn};
use defmt_rtt as _;
use hal::{
    delay::DelayFromCountDownTimer,
    gpio::{AF5, PA5, PA6, PA7},
    prelude::*,
    pwr::PwrExt,
    rcc::Config,
    spi,
    stm32::Peripherals,
    time::{ExtU32, RateExtU32},
    timer::Timer,
};

[techmccat]

I'm not too surprised by the timing since it's affected by the FIFO prefill behavior (prefill write fifo, then start read/write loop), though it's a bit odd that it performs that bad at 1MHz, in my benchmarks it could run at full speed 4MHz with the core clocked at 16MHz from the HSI. Have you built your code in debug mode?

The 20 byte limit is concerning but i'm unable to reproduce it when running the spi-hal-one example on a Nucleo G474RE with MOSI and MISO shorted together, doesn't help that I can't get the rp2040 I'm using as a logic analyzer to sample this spi traffic correctly (uLA reads are very noisy)
I've done further tests with my ST25R39 driver still seems to reliably detect and activate NFC-A cards too.

Regarding the conflicting methods issue, could you upload a full program with that issue? I've tried making a separate crate and running the example there as bin but the read/write/transfer methods are found just fine by the compiler

[phansel]

It was built in debug mode to comply with defmt -- I could see that affecting the timings. Will re-examine in release mode.

For the logic analyzer, it might help to drop the STM32 bus rate. I can see on my end that SCLK only clocks out 20 bytes when read() is given a 40 byte slice to fill.

Here's a minimum repro for rustc 1.90.0-nightly and 1.88.0-stable: stm32g4xx-hal-spi-demo.

[techmccat]

I think I've figured out the issue with your imports, you're importing the old FullDuplex trait on line 63, which means you now have two traits implementing a read() on spi in scope.

I've removed the import and called hal::hal_02::spi::FullDuplex::send directly and it seems to work fine, I can't see the limit to 20 read bytes though.
I think alternating between the old and new trait functions might break something since I didn't think about them being used together when I modified the driver

This is the output I'm getting from your program, after editing the loop to log all the read data on a single line for convenience

INFO  Booted up.
INFO  pre-read rbuf hex: [ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff, ff]:
INFO  Set CS low
INFO  LSbytes of rbuf: 0x00 0x00 0x00
INFO  Full rbuf: [00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
INFO  set cs high

I've also tested it with MISO connected to 3v3 and it fills with 0xff as it should

[phansel]

Thanks for clarifying. It seems like we're on the right track!

• Calling the embedded-hal 1.0 traits (read/write/transfer) implemented under hal::prelude::SpiBus (and also not importing both versions of the trait) works well. The scoping issue does not occur.
• Number of bytes read: I was misreading my oscilloscope. It was 20 groups of 2 byte clocks (for a debug build), not 20 groups of one byte. The device also happened to have the right register values (0s after byte 20) to make it look like a bug.
• Release builds are quicker and don't have the peculiar byte timing.
• The incorrect (0x44) register read seems to recur predictably when MISO is loaded with a scope probe. I get the right ID value (0x43) when I remove the probe, regardless of clock speed. Disappointing for me, but not a library or chip issue.

The one behavioral bug remaining is that read()+write() and transfer(), with the same input and output buffers have different behavior. This is present in release and debug builds.

Here's how the last bit manifests. This issue isn't obvious with MISO shorted to 3V3 or ground, since the whole buffer has the same value -- perhaps the internal circular buffer has an off-by-one? I'll take a look now.

write+read:
read buffer before txn: [[255, 255, 255, 255, 255, 255, 255, 255, 255, 255]]
read buffer after txn: [[0, 68, 72, 0, 0, 0, 0, 8, 0, 16]]

transfer:
read buffer before txn: [[255, 255, 255, 255, 255, 255, 255, 255, 255, 255]]
read buffer after txn: [[0, 0, 68, 72, 0, 0, 0, 0, 8, 0]]

I pushed this change to the bug demo branch.

[phansel]

The off-by-one-word is present for transfer_in_place as well. The extra byte in the read buffer index 0 doesn't seem to be the n-1th one put in the wrong spot as it's always 0x00 or 0xFF.

[techmccat]

If I understand the SpiBus trait correctly, that should be the intended behavior: transfer does reading and writing at the same time, while write + read does it sequentially, so the value read by transfer is offset by 1 because you wrote one byte on the bus and the SPI slave was holding the line low.
As far as I understand it's meant for SPI devices that can do read while write.

[phansel]

@techmccat good catch! Seems to be right. The RX buffer would read in a byte during the first word on the TX SCLK. In the case of the BMI323, it looks like SDO could be hi-z at that point, so the stray byte in the read buffer could be anything between 0-255.

I might have caught this if I checked with a larger write buffer.

Do you want to leave the hal_02 trait instances in the examples? I may put in some time later today to update the documentation and add an example SpiDevice implementation.

[techmccat]

HALs don't need to implement SpiDevice unless they want to expose mechanisms for hardware chip select control, but updating the examples to the new Hal would be helpful