SPI: CS must be configured before SPI peripheral

[jonahbron]

I'm trying to use the SPI library but it's not working as expected. The send method works fine and returns without incident, but the read method blocks forever.

    let byte = block!(spi.send(5u8).and_then(|_| spi.read()));
    if let Ok(byte) = byte {
        ufmt::uwriteln!(&mut serial, "wrote read {}\r", byte).unwrap();
    } else {
        ufmt::uwriteln!(&mut serial, "not wrote read\r").unwrap();
    }

It never makes it past the block macro, it seems that it's returning WouldBlock forever. All read does is try to flush, code here:

https://github.com/Rahix/avr-hal/blob/master/avr-hal-generic/src/spi.rs#L125

The is_write_in_progress is obviously true as expected, since a send was just executed. But it seems that the write complete status register is not being set.

https://github.com/Rahix/avr-hal/blob/master/avr-hal-generic/src/spi.rs#L127

Documentation for the SPI interrupt flag is in section 18.5.2 of this document:

http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-7810-Automotive-Microcontrollers-ATmega328P_Datasheet.pdf

I'm trying to debug this but any suggestions are welcome.

[jonahbron]

SPIF is cleared by hardware when executing the corresponding interrupt handling vector. Alternatively, the SPIF bit is cleared by first reading the SPI status register with SPIF set, then accessing the SPI data register (SPDR).

It seems like something is happening before the flush method is called that's clearing the interrupt flag. I tried disabling the interrupt (SPIE) but that didn't change the behavior.

[jonahbron]

Some more reading through section 17.1.2 made me realize I might be causing a conflict on the SS pin. I tried disconnecting everything, and now it's actually returning. However it doesn't seem to be reading any data back. Still debugging.

[lights0123]

Did the SPI feedback example work for you?

[jonahbron]

@lights0123 That's what I'm going to try next.

[jonahbron]

Hmmm, yup, the example worked perfectly. Must be something else.

[jonahbron]

Figured it out. The CS pin must be set to output mode before SPI is initialized. The example does that and it works fine, but my code was setting it to output mode after. All good!

[jonahbron]

Also for future reference, I had written code like this:

block!(spi.send(0).and_then(|_| { spi.read() }))?

This is incorrect because it will retry the entire expression if the read blocks at all, which causes it to re-send, creating an infinite loop of writing and retrying. The correct syntax is

block!(spi.send(0)).and_then(|_| { block!(spi.read()) })?

[Rahix]

Figured it out. The CS pin must be set to output mode before SPI is initialized. The example does that and it works fine, but my code was setting it to output mode after. All good!

This is an ugly detail that one can easily step over :/ Maybe we can handle this in avr-hal in a way that downstream code can't do it wrong?

[Rahix]

So the peripheral is probably immediately reset into slave mode in the case of configuring CS afterwards ... I think this is definitely something the HAL should handle.

[jonahbron]

We could accept a borrow of the CS pin as an argument and set its mode. Then the user would still have ownership.

[lights0123]

@jonahbron setting the mode is done by transferring ownership—into_output takes ownership of the pin and returns a new struct that only has output features.

[jonahbron]

Ah okay, so it would need to pass back out an OutputPin. The only way I know of to do that is for the constructor to return a tuple.

[Rahix]

I think we can't avoid moving the CS pin into the SPI device (and keeping it there). Any other way would leave open the possibility that a user later changes the pin's mode again. And this would immediately break the SPI device ...

Though of course the pin needs to be accessible to the user, for setting its state. We can either provide methods on the SPI device for that, or add a method that returns a reference to the pin:

// Either
pub fn cs_set_high(&mut self) { self.cs.set_high() }
pub fn cs_set_low(&mut self) { self.cs.set_low() }

// Or
pub fn cs(&mut self) -> &mut $CsType { &mut self.cs }

I think both approaches would be sound.

[Rahix]

Ah, but users probably want to move the CS pin into a driver ... So we'll need to somehow allow the user to own the pin but in a way where they can't modify its mode. I think the only feasible way to archieve this is with a wrapper type:

pub struct CsPin($CsType);

impl OutputPin for CsPin {
    // ...
}

impl Spi {
    pub fn new(peripheral: ..., cs: $CsType) -> (Spi, CsPin) {
        // ...
        (Spi { ...}, CsPin(cs))
    }
}

[jonahbron]

I think that's a great idea. If we take that approach, we still need a way to unwrap it if the user decides they don't want SPI active anymore. Maybe...

impl Spi {
            pub fn release(self, cs: CsPin) -> (Spi, SCLK, MOSI, MISO) {
                self.peripheral.spcr.write(|w| {
                    w.spe().clear_bit()
                });
                (self.peripheral, self.sclk, self.mosi, self.miso, cs.0)
            }
}

Although, it's possible that if a chip had two SPI peripherals, the user could use one to falsely unwrap the CS pin from another. So it's not 100% safe.

[Rahix]

Although, it's possible that if a chip had two SPI peripherals, the user could use one to falsely unwrap the CS pin from another.

We can prevent that by creating separate wrapper types for each SPI peripheral. So I think this approach looks like the way to go!

[jonahbron]

Excellent point. I'll get to work on it.

[Rahix]

@jonahbron, are you still interested in working on this or should someone else take up this issue?

[jonahbron]

I am but I forgot about it 😁 . Github doesn't do a great job of floating assigned issues up to your attention.

[Rahix]

No problem! Just need to know whether I should keep you assigned or not ;)