Fix spi initialization

[Sh3Rm4n]

I noticed, that the SPI is not working on my STM Discovery Board.
With some debugging, I first found out, that the SPI is for example not initialized as master.

I used PyCortexMDebug, to see if
the registers are correctly set in combination with the svd file provided by
stm32-rs.

I did provide a minimal example, to reproduce these steps. Ideally, this example
should be extended, to be helpful for newcomers and not just a configuration
example, without using the SPI. Something like connecting MOSI and MISO to echo
the output and do assert_eq! or similar.

Reproduction Steps:

openocd.cfg:

source [find interface/stlink.cfg]
source [find target/stm32f3x.cfg]

openocd -c openocd.cfg
cargo run --example spi --features stm32f303

openocd.gdb

target extended-remote :3333

svd STM32F303.svd

break examples/spi.rs:41
break examples/spi.rs:50

load

c

svd SPI1 CR1

c

svd SPI1 CR1

Output:

Breakpoint 1, main () at examples/spi.rs:41
41          let _spi = Spi::spi1(
Fields in SPI1 CR1:
        BIDIMODE:  0  Bidirectional data mode enable
        BIDIOE:    0  Output enable in bidirectional mode
        CRCEN:     0  Hardware CRC calculation enable
        CRCNEXT:   0  CRC transfer next
        CRCL:      0  CRC length
        RXONLY:    0  Receive only
        SSM:       0  Software slave management
        SSI:       0  Internal slave select
        LSBFIRST:  0  Frame format
        SPE:       0  SPI enable
        BR:        0  Baud rate control
        MSTR:      0  Master selection
        CPOL:      0  Clock polarity
        CPHA:      0  Clock phase
halted: PC: 0x08000ace

Breakpoint 2, main () at examples/spi.rs:50
50          loop {}
Fields in SPI1 CR1:
        BIDIMODE:  0  Bidirectional data mode enable
        BIDIOE:    0  Output enable in bidirectional mode
        CRCEN:     0  Hardware CRC calculation enable
        CRCNEXT:   0  CRC transfer next
        CRCL:      0  CRC length
        RXONLY:    0  Receive only
        SSM:       1  Software slave management
        SSI:       1  Internal slave select
        LSBFIRST:  0  Frame format
        SPE:       1  SPI enable
        BR:        0  Baud rate control
        MSTR:      0  Master selection
        CPOL:      1  Clock polarity
        CPHA:      1  Clock phase

The fixed version outputs:

Breakpoint 1, main () at examples/spi.rs:41
41          let _spi = Spi::spi1(
Fields in SPI1 CR1:
        BIDIMODE:  0  Bidirectional data mode enable
        BIDIOE:    0  Output enable in bidirectional mode
        CRCEN:     0  Hardware CRC calculation enable
        CRCNEXT:   0  CRC transfer next
        CRCL:      0  CRC length
        RXONLY:    0  Receive only
        SSM:       0  Software slave management
        SSI:       0  Internal slave select
        LSBFIRST:  0  Frame format
        SPE:       0  SPI enable
        BR:        0  Baud rate control
        MSTR:      0  Master selection
        CPOL:      0  Clock polarity
        CPHA:      0  Clock phase
halted: PC: 0x08000af8

Breakpoint 2, main () at examples/spi.rs:50
50          loop {}
Fields in SPI1 CR1:
        BIDIMODE:  0  Bidirectional data mode enable
        BIDIOE:    0  Output enable in bidirectional mode
        CRCEN:     0  Hardware CRC calculation enable
        CRCNEXT:   0  CRC transfer next
        CRCL:      0  CRC length
        RXONLY:    0  Receive only
        SSM:       1  Software slave management
        SSI:       1  Internal slave select
        LSBFIRST:  0  Frame format
        SPE:       1  SPI enable
        BR:        3  Baud rate control
        MSTR:      1  Master selection
        CPOL:      0  Clock polarity
        CPHA:      0  Clock phase
>>>

Explanation

As we can see, the mstr bit is not set.

This is because, the register method was split up in multiple writes in

stm32f3xx-hal/src/spi.rs

Line 180 in 4fd3ae6

unsafe {

This was introduced in 96a03cd.

@dfrankland, i can't test if this is introduces a regression for the
stm32f373, as i only have the discovery board available.

https://docs.rs/svd2rust/0.16.1/svd2rust/#write

The write method takes a closure with signature (&mut W) -> &mut W. If the
"identity closure", |w| w, is passed then the write method will set the CR2
register to its reset value. Otherwise, the closure specifies how the reset
value will be modified before it's written to CR2.

So, when w is split up, every untouched bit is reset. As MSTR is set at the
beginning, it will be reset with the following write invocations.

[dfrankland]

Glad you caught this, and it's good to see the conversation over on #36!

I don't have a stm32f373 so I am unable to test this. Previously, I made that change because the stm32f373 device feature wouldn't compile without it. I think after the updates to the device crate (stm32f3) were done, this made my change no longer needed, but I could be wrong. (I just noticed another update to the device crate is available too)

[Sh3Rm4n]

Ok it seems to compile for the stm32f373, so that is a great sign. 👍

Yeah, the 0.9 version of the stm32f3 crate was released a few weeks ago. Out of curiosity I compiled this crate with the version and it worked. Maybe we should just bump it, as it comes with some improvements for the stm32f3 device family.

I would like to expand the example to confirm, that data transmission is possible. Maybe I have time for this today.

[strom-und-spiele]

I'm not quite getting it. (so feel free to skip this post, if you're busy - I'd be happy for the clarification though)

The doc says:

The write method takes a closure with signature (&mut W) -> &mut W. […], the closure specifies how the reset value will be modified before it's written to CR2.
I understand:

spi.cr1.write(
    // start of closure - the reset value is being loaded (in this case, it's 0)
    |w| {
        // after setting the master bit, 
        // br() returns a _BRW struct, which is stored in w for further modification.
        w.mstr().set_bit().br();
        unsafe {  w.bits(br);  }
    // end of closure 
    // it modified the reset value (0) such that 
    // * the mstr bit is set and 
    // * the br part is set according to the br variable
    }
// end of write - the modified reset value is written.
) 

however (if I understand your comment correctly) this seems not to be the case, but the following is happening:

spi.cr1.write(
    // start of closure
    |w| {
        // first finished expression statement within the closure.  
        // it started with loading the reset value, modified it and wrote it at the end of the expression. 
        // the _BRW struct is stored in w for further modification 
        w.mstr().set_bit().br();     
        // however, the reset value is loded again, the br part is being modified 
        // and then everything is written to the register (again) 
        unsafe {  w.bits(br);  }
    }
)

This is odd.

Next @Sh3Rm4n wrote

Out of curiosity I compiled this crate with the [0.9] version [of stm32f3] and it worked.
Does this mean that the odd behavior above is fixed in that version?

[Sh3Rm4n]

Next @Sh3Rm4n wrote

Out of curiosity I compiled this crate with the [0.9] version [of stm32f3] and it worked.

Does this mean that the odd behavior above is fixed in that version?

Well, I have to correct myself: I thought, that I did test to compile the hal with version 0.9 of stm32f3, hover as I've tried again, this would'nt work as it requires some changes. So I can not confirm if this is working (the spi fix is not needed), when upgrading stm32f3.

however (if I understand your comment correctly) this seems not to be the case, but the following is > happening:

[...]

That's how I have observed it, while debugging the code. At first, I thought that it would work like you described in the first example, however it apparently does not. I could confirm the behavior, while deubgging the code build with the debug profile. release was hard to follow, however the spi was still not working.

I tried to look for STR instruction in the assembler output, but was quickly overwhelmed and did not find the relevant parts. My guess would be, that multple STR instructions can be found in the closure, which would actually reset the value to default every time for every w instance.

[Sh3Rm4n]

Ready for review, as I've added an example, and confirmed that it is working for the stm32f303 discovery board.

I find the spi transfer method is not really ergonomic, so this is the best I could do with my lacking rust experience, so I will happily implement every improvement suggestions. 🙂

[Sh3Rm4n]

Ok, I now found out what the real issue was.

                        unsafe {
                            w.bits(br);
                        }

The bits is directly applied to w, which would overwrite the register, with the given 3 bits, which would set CPHA, CPOL, and MSTR instead of writing to BR.

This is an easy fix:

diff --git a/src/spi.rs b/src/spi.rs
index 28e59ed..e422b4a 100644
--- a/src/spi.rs
+++ b/src/spi.rs
@@ -175,12 +175,9 @@ macro_rules! hal {
                             .cpol()
                             .bit(mode.polarity == Polarity::IdleHigh)
                             .mstr()
-                            .set_bit()
-                            .br();
+                            .set_bit();
 
-                        unsafe {
-                            w.bits(br);
-                        }
+                        w.br().bits(br);
 
                         w.spe()
                             .set_bit()
-- 
2.24.0

So every thing before is practically discarded, the bit's after are set, which can be seen in my initial comment.

I've looked so many times on the code and just realized it now 😅

So the docs are right, @strom-und-spiele 😀

[strom-und-spiele]

That's reassuring to hear.
Actually I read the solution for the issue yesterday as well. The docs state

An expression statement is one that evaluates an expression and ignores its result.

So obviously the first block is not reassigning a struct to w but rather ignoring that returned struct. Nice that you recognized the function-name overlap. 👍

[dfrankland]

So far as I can see, this doesn't cause any regression. I ran a couple examples over on proton-c.

I tried to setup and use PyCortexMDebug, but I couldn't get it to install on my system.

[Sh3Rm4n]

Nice!

If I remeber correctly, i just cloned the PyCortexMDebug

Called

python -m setup.py install --user

and sourced svd_gdb.py in my .gdbinit

source <path_to>/PyCortexMDebug/cmdebug/svd_gdb.py

However, any debugger, which can read SVD files would work fine. I can not
name any alternative off the top of my head.

This one looks really promising, but I did not test it yet.

[Sh3Rm4n]

I've updated the gdb example, as the breakpoint has to beset at examples/spi.rs
instead of spi.rs. The latter would cause a breakpoint in the spi module,
which I did not intend.

I reconfirmed that the spi initialization is still working.

[dfrankland]

Thanks to your setup instructions I was able to test and reproduce given the steps. Looks great! I rechecked for any regressions as well: everything works as expected.

[dfrankland]

@Sh3Rm4n feel free to merge this PR as well