Should SPI read() send dummy data?

[jgallagher]

I've been working with a very finicky SPI device attached to an stm32f4, and copy-paste-modified my way from drv/stm32h7-spi-server to an SPI server driver for the stm32f4. I couldn't get read() to work though, and based on the comments in the changes in #351, I'm wondering if there's a bug in stm32h7-spi-server's read implementation. It passes None as the tx buffer:

hubris/drv/stm32h7-spi-server/src/main.rs

Line 152 in 8e0b13b

self.ready_writey(SpiOperation::read, device_index, None, Some(dest))

which means ready_writey will never enter the block that transmits data (dummy or otherwise):

hubris/drv/stm32h7-spi-server/src/main.rs

Lines 366 to 395 in 8e0b13b

	if let Some((tx_data, tx_pos)) = &mut tx {
	while self.spi.can_tx_frame() {
	// If our position is less than our tx len,
	// transfer a byte from caller to TX FIFO --
	// otherwise put a dummy byte on the wire
	let byte: u8 = if *tx_pos < src_len {
	tx_data
	.read_at(usize::from(*tx_pos))
	.ok_or(RequestError::went_away())?
	} else {
	0u8
	};
	ringbuf_entry!(Trace::Tx(*tx_pos, byte));
	self.spi.send8(byte);
	*tx_pos += 1;
	made_progress = true;
	// If we have _just_ finished...
	if *tx_pos == src_len {
	// We will finish transmitting well before
	// we're done receiving, so stop getting
	// interrupt notifications for transmit
	// space available during that time.
	self.spi.disable_can_tx_interrupt();
	tx = None;
	break;
	}
	}
	}

I tested fixing this with a quick hack by adding an "else send dummy data" branch to that block, and it makes read() work for me. I doubt that's the best fix, but wanted to open this issue for awareness (or correction if I'm misreading/misunderstanding).

[cbiffle]

Interesting -- the intent with read is that it shifts out dummy data. I'm not sure it's currently being exercised on our side, though. The STM32H7 SPI driver state machine has gotten a little...hairy over the past year or so and I could totally believe that there's a bug here.

[cbiffle]

@jgallagher you're spot on here, btw. I have a fix prepared (which also greatly improves SPI performance) but it's currently traffic jammed behind a few other of my pending PRs.

[cbiffle]

This should be fixed on the stm32h7.