SPI comms update.

) Calculate the CRC on the fly to reduce latency.
) Tell the SPI driver how many bytes to expect, so the main loop only has to lock the SPI object when that number of bytes has arrived.

Author: jonathanpallant

neotron-bmc-pico/src/main.rs

@@ -78,8 +78,8 @@ mod app {
 		Ps2Data0(u16),
 		/// Word from PS/2 port 1
 		Ps2Data1(u16),
-		/// Message from SPI bus
-		SpiRequest(neotron_bmc_protocol::Request),
+		/// SPI driver has a Request for us
+		SpiRx,
 		/// SPI CS went low (active)
 		SpiEnable,
 		/// SPI CS went high (inactive)
@@ -444,8 +444,9 @@ mod app {
 				}
 				Some(Message::SpiEnable) => {
 					if ctx.shared.state_dc_power_enabled.lock(|r| *r) != DcPowerState::Off {
-						// Turn on the SPI peripheral
-						ctx.shared.spi.lock(|s| s.start());
+						// Turn on the SPI peripheral and expect four bytes (the
+						// length of a Request).
+						ctx.shared.spi.lock(|s| s.start(4));
 					} else {
 						// Ignore message - it'll be the CS line being pulled low when the host is powered off
 						defmt::info!("Ignoring spurious CS low");
@@ -456,12 +457,39 @@ mod app {
 					ctx.shared.spi.lock(|s| s.stop());
 					defmt::trace!("SPI Disable");
 				}
-				Some(Message::SpiRequest(req)) => {
-					process_command(req, &mut register_state, |rsp| {
-						ctx.shared.spi.lock(|spi| {
-							spi.set_transmit_sendable(rsp).unwrap();
-						});
+				Some(Message::SpiRx) => {
+					defmt::trace!("SpiRx");
+					// Look for something in the SPI bytes received buffer:
+					let mut req = None;
+					ctx.shared.spi.lock(|spi| {
+						if let Some((data, crc)) = spi.get_received() {
+							use proto::Receivable;
+							match proto::Request::from_bytes_with_crc(data, crc) {
+								Ok(inner_req) => {
+									defmt::debug!("Got packet");
+									req = Some(inner_req);
+								}
+								Err(proto::Error::BadLength) => {
+									// This is a programming bug. We said
+									// start(4) earlier, so there should be four
+									// bytes here.
+									panic!("Wanted 4, got {}", data.len());
+								}
+								Err(e) => {
+									defmt::warn!("Bad Req {:?} ({=[u8]:x}", e, data);
+								}
+							}
+						}
 					});
+
+					// If we got a valid message, queue it so we can look at it next time around
+					if let Some(req) = req {
+						process_command(req, &mut register_state, |rsp| {
+							ctx.shared.spi.lock(|spi| {
+								spi.set_transmit_sendable(rsp).unwrap();
+							});
+						});
+					}
 				}
 				Some(Message::UartByte(rx_byte)) => {
 					defmt::info!("UART RX {:?}", rx_byte);
@@ -472,44 +500,6 @@ mod app {
 					// No messages
 				}
 			}
-
-			// Look for something in the SPI bytes received buffer:
-			let mut req = None;
-			ctx.shared.spi.lock(|spi| {
-				let mut mark_done = false;
-				if let Some(data) = spi.get_received() {
-					use proto::Receivable;
-					match proto::Request::from_bytes(data) {
-						Ok(inner_req) => {
-							mark_done = true;
-							req = Some(inner_req);
-						}
-						Err(proto::Error::BadLength) => {
-							// Need more data
-						}
-						Err(e) => {
-							defmt::warn!("Bad Req {:?} ({=[u8]:x}", e, data);
-							mark_done = true;
-						}
-					}
-				}
-				if mark_done {
-					// Couldn't do this whilst holding the `data` ref.
-					spi.mark_done();
-				}
-			});
-
-			// If we got a valid message, queue it so we can look at it next time around
-			if let Some(req) = req {
-				if ctx
-					.shared
-					.msg_q_in
-					.lock(|q| q.enqueue(Message::SpiRequest(req)))
-					.is_err()
-				{
-					panic!("Q full!");
-				}
-			}
 			// TODO: Read ADC for 3.3V and 5.0V rails and check good
 		}
 	}
@@ -580,11 +570,12 @@ mod app {
 	///
 	/// It fires whenever there is new data received on SPI1. We should flag to the host
 	/// that data is available.
-	#[task(binds = SPI1, shared = [spi])]
+	#[task(binds = SPI1, shared = [spi, msg_q_in])]
 	fn spi1_interrupt(mut ctx: spi1_interrupt::Context) {
-		ctx.shared.spi.lock(|spi| {
-			spi.handle_isr();
-		});
+		let has_message = ctx.shared.spi.lock(|spi| spi.handle_isr());
+		if has_message {
+			let _ = ctx.shared.msg_q_in.lock(|q| q.enqueue(Message::SpiRx));
+		}
 	}
 
 	/// This is the LED blink task.

neotron-bmc-pico/src/spi.rs

@@ -12,15 +12,16 @@ pub struct SpiPeripheral<const RXC: usize, const TXC: usize> {
 	rx_buffer: [u8; RXC],
 	/// How many bytes have been received?
 	rx_idx: usize,
+	/// How many bytes do we want?
+	rx_want: usize,
 	/// A space for data we're about to send
 	tx_buffer: [u8; TXC],
 	/// How many bytes have been played from the TX buffer
 	tx_idx: usize,
 	/// How many bytes are loaded into the TX buffer
 	tx_ready: usize,
-	/// Has the RX been processed? If so, lie and say we don't have any data
-	/// (otherwise we process incoming messages multiple times).
-	is_done: bool,
+	/// The in-progress RX CRC
+	rx_crc: neotron_bmc_protocol::CrcCalc,
 }
 
 impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
@@ -50,10 +51,11 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 			dev,
 			rx_buffer: [0u8; RXC],
 			rx_idx: 0,
+			rx_want: 0,
 			tx_buffer: [0u8; TXC],
 			tx_idx: 0,
 			tx_ready: 0,
-			is_done: false,
+			rx_crc: neotron_bmc_protocol::CrcCalc::new(),
 		};
 
 		spi.config(Self::MODE);
@@ -130,12 +132,11 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 			// 3f. Disable DMA mode
 			w.txdmaen().disabled();
 			w.rxdmaen().disabled();
-			// Extra: Turn on RX and Error interrupts, but not TX. The TX
-			// interrupt is turned off because we deliberately underflow the
-			// FIFO during the receive phase of the transaction.
-			w.rxneie().not_masked();
+			// Extra: Turn on RX and Error interrupts, but not TX. We swap
+			// interrupts once the read phase is complete.
+			w.rxneie().masked();
 			w.txeie().masked();
-			w.errie().not_masked();
+			w.errie().masked();
 			w
 		});
 
@@ -144,16 +145,30 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 		// 5. DMA registers - not required
 	}
 
-	/// Enable the SPI peripheral (i.e. when CS goes low)
-	pub fn start(&mut self) {
+	/// Enable the SPI peripheral (i.e. when CS goes low).
+	///
+	/// We tell it how many bytes we are expecting, so it knows when to update
+	/// the main thread.
+	pub fn start(&mut self, num_bytes: usize) {
+		if num_bytes > RXC {
+			panic!("Read too large");
+		}
 		self.rx_idx = 0;
+		self.rx_want = num_bytes as usize;
 		self.tx_idx = 0;
 		self.tx_ready = 0;
-		self.is_done = false;
+		self.rx_crc.reset();
 		// Empty the receive register
 		while self.has_rx_data() {
 			let _ = self.raw_read();
 		}
+		// Turn on RX interrupt; turn off TX interrupt
+		self.dev.cr2.write(|w| {
+			w.rxneie().not_masked();
+			w.txeie().masked();
+			w
+		});
+		// Tell the SPI engine it has a chip-select
 		self.dev.cr1.modify(|_r, w| {
 			w.ssi().slave_selected();
 			w
@@ -221,37 +236,46 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 	}
 
 	/// Get a slice of data received so far.
-	pub fn get_received(&self) -> Option<&[u8]> {
-		if !self.is_done {
-			Some(&self.rx_buffer[0..self.rx_idx])
-		} else {
-			None
-		}
-	}
-
-	/// Mark the RX as processed, so we don't do it again (until the SPI is
-	/// disabled and re-enabled by the next chip select).
-	pub fn mark_done(&mut self) {
-		self.is_done = true;
+	pub fn get_received(&self) -> Option<(&[u8], u8)> {
+		Some(((&self.rx_buffer[0..self.rx_idx]), self.rx_crc.get()))
 	}
 
-	pub fn handle_isr(&mut self) {
+	/// Call this when the SPI peripheral interrupt fires.
+	///
+	/// It will handle incoming bytes and/or outgoing bytes, depending on what
+	/// phase we are in.
+	pub fn handle_isr(&mut self) -> bool {
+		let mut have_packet = false;
 		let irq_status = self.dev.sr.read();
 		if irq_status.rxne().is_not_empty() {
-			self.rx_isr();
+			if self.rx_isr() {
+				// We've got enough, turn the RX interrupt off. Everything else
+				// we receive is going to be garbage.
+				self.dev.cr2.write(|w| {
+					w.rxneie().masked();
+					w
+				});
+				have_packet = true;
+			}
 		}
 		if irq_status.txe().is_empty() {
 			self.tx_isr();
 		}
+		have_packet
 	}
 
 	/// Try and read from the SPI FIFO
-	fn rx_isr(&mut self) {
-		let cmd = self.raw_read();
+	fn rx_isr(&mut self) -> bool {
+		let byte = self.raw_read();
+		if self.rx_want == 0 {
+			panic!("unwanted data 0x{:02x}", byte);
+		}
 		if self.rx_idx < self.rx_buffer.len() {
-			self.rx_buffer[self.rx_idx] = cmd;
+			self.rx_buffer[self.rx_idx] = byte;
+			self.rx_crc.add(byte);
 			self.rx_idx += 1;
 		}
+		self.rx_idx == self.rx_want
 	}
 
 	/// Call this in the TXEIE interrupt. It will load the SPI FIFO with some
@@ -264,7 +288,8 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 			self.raw_write(next_tx);
 			self.tx_idx += 1;
 		} else {
-			// No data - send nothing
+			// No data - send 0x00
+			self.raw_write(0x00);
 		}
 	}
 
@@ -285,6 +310,11 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 		// We must never set this to be longer than `TXC` as we do an unchecked
 		// read from `self.tx_buffer` in [`Self::tx_isr`].
 		self.tx_ready = data.len();
+		// Turn on the TX interrupt
+		self.dev.cr2.write(|w| {
+			w.txeie().not_masked();
+			w
+		});
 		Ok(())
 	}
 
@@ -303,6 +333,11 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 				// We must never set this to be longer than `TXC` as we do an
 				// unchecked read from `self.tx_buffer` in [`Self::tx_isr`].
 				self.tx_ready = n.min(TXC);
+				// Turn on the TX interrupt
+				self.dev.cr2.write(|w| {
+					w.txeie().not_masked();
+					w
+				});
 				Ok(())
 			}
 			Err(_) => Err(()),