Change SPI driver to use internal NSS signal instead of disabling the whole block.

Author: thejpster

neotron-bmc-pico/src/main.rs

@@ -80,6 +80,10 @@ mod app {
 		Ps2Data1(u16),
 		/// Message from SPI bus
 		SpiRequest(neotron_bmc_protocol::Request),
+		/// SPI CS went low (active)
+		SpiEnable,
+		/// SPI CS went high (inactive)
+		SpiDisable,
 		/// The power button was given a press
 		PowerButtonShortPress,
 		/// The power button was held down
@@ -281,7 +285,6 @@ mod app {
 		let spi = neotron_bmc_pico::spi::SpiPeripheral::new(
 			dp.SPI1,
 			(pin_sck, pin_cipo, pin_copi),
-			2_000_000,
 			&mut rcc,
 		);
 
@@ -347,7 +350,7 @@ mod app {
 	/// Our idle task.
 	///
 	/// This task is called when there is nothing else to do.
-	#[idle(shared = [msg_q_out, msg_q_in, spi, state_dc_power_enabled, pin_dc_on, pin_sys_reset, pin_cs])]
+	#[idle(shared = [msg_q_out, msg_q_in, spi, state_dc_power_enabled, pin_dc_on, pin_sys_reset])]
 	fn idle(mut ctx: idle::Context) -> ! {
 		// TODO: Get this from the VERSION static variable or from PKG_VERSION
 		let mut register_state = RegisterState {
@@ -377,36 +380,46 @@ mod app {
 				}
 				Some(Message::PowerButtonLongPress) => {
 					if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::On {
-						defmt::info!("Power button held whilst on.");
+						defmt::info!("Power off requested!");
 						ctx.shared
 							.state_dc_power_enabled
 							.lock(|r| *r = DcPowerState::Off);
-						defmt::info!("Power off!");
+						// Stop any SPI stuff that's currently going on (the host is about to be powered off)
+						ctx.shared.spi.lock(|s| s.stop());
+						// Put the host into reset
 						ctx.shared.pin_sys_reset.lock(|pin| pin.set_low().unwrap());
+						// Shut off the 5V power
 						ctx.shared.pin_dc_on.set_low().unwrap();
-						ctx.shared.spi.lock(|s| s.disable());
 						// Start LED blinking again
 						led_power_blink::spawn().unwrap();
 					}
 				}
 				Some(Message::PowerButtonShortPress) => {
 					if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::Off {
-						defmt::info!("Power button pressed whilst off. Powering up.");
-						// Button pressed - power on system
+						defmt::info!("Power up requested!");
+						// Button pressed - power on system.
+						// Step 1 - Note our new power state
 						ctx.shared
 							.state_dc_power_enabled
 							.lock(|r| *r = DcPowerState::Starting);
+						// Step 2 - Hold reset line (active) low
+						ctx.shared.pin_sys_reset.lock(|pin| pin.set_low().unwrap());
+						// Step 3 - Turn on PSU
 						ctx.shared.pin_dc_on.set_high().unwrap();
+						// Step 4 - Leave it in reset for a while.
 						// TODO: Start monitoring 3.3V and 5.0V rails here
 						// TODO: Take system out of reset when 3.3V and 5.0V are good
-						ctx.shared.pin_sys_reset.lock(|pin| pin.set_high().unwrap());
-						ctx.shared.spi.lock(|s| s.enable());
+						// Returns an error if it's already scheduled (but we don't care)
+						let _ = exit_reset::spawn_after(RESET_DURATION_MS.millis());
 					}
 				}
 				Some(Message::PowerButtonRelease) => {
 					if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::Starting {
 						defmt::info!("Power button released.");
-						// Button released after power on
+						// Button released after power on. Change the power
+						// state machine t "On". We were in 'Starting' to ignore
+						// any further button events until the button had been
+						// released.
 						ctx.shared
 							.state_dc_power_enabled
 							.lock(|r| *r = DcPowerState::On);
@@ -417,10 +430,23 @@ mod app {
 					if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::On {
 						defmt::info!("Reset!");
 						ctx.shared.pin_sys_reset.lock(|pin| pin.set_low().unwrap());
-						// Returns an error if it's already scheduled
+						// Returns an error if it's already scheduled (but we don't care)
 						let _ = exit_reset::spawn_after(RESET_DURATION_MS.millis());
 					}
 				}
+				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());
+					} else {
+						// Ignore message - it'll be the CS line being pulled low when the host is powered off
+						defmt::info!("Ignoring spurious CS low");
+					}
+				}
+				Some(Message::SpiDisable) => {
+					// Turn off the SPI peripheral. Don't need to check power state for this.
+					ctx.shared.spi.lock(|s| s.stop());
+				}
 				Some(Message::SpiRequest(req)) => {
 					process_command(req, &mut register_state, |rsp| {
 						ctx.shared.spi.lock(|spi| {
@@ -487,7 +513,7 @@ mod app {
 	#[task(
 		binds = EXTI4_15,
 		priority = 4,
-		shared = [ps2_clk0, msg_q_in, ps2_dat0, exti, spi, pin_cs, kb_decoder],
+		shared = [ps2_clk0, msg_q_in, ps2_dat0, exti, pin_cs, kb_decoder],
 	)]
 	fn exti4_15_interrupt(mut ctx: exti4_15_interrupt::Context) {
 		let pr = ctx.shared.exti.pr.read();
@@ -511,12 +537,15 @@ mod app {
 		}
 
 		if pr.pr4().bit_is_set() {
-			if ctx.shared.pin_cs.lock(|pin| pin.is_low().unwrap()) {
-				// If incoming Chip Select is low, turn on the SPI engine
-				ctx.shared.spi.lock(|s| s.start());
+			let msg = if ctx.shared.pin_cs.lock(|pin| pin.is_low().unwrap()) {
+				// If incoming Chip Select is low, tell the main thread to turn on the SPI engine
+				Message::SpiEnable
 			} else {
-				// If incoming Chip Select is high, turn off the SPI engine
-				ctx.shared.spi.lock(|s| s.stop());
+				// If incoming Chip Select is high, tell the main thread to turn off the SPI engine
+				Message::SpiDisable
+			};
+			if ctx.shared.msg_q_in.lock(|q| q.enqueue(msg)).is_err() {
+				panic!("queue full");
 			}
 			// Clear the pending flag for this pin
 			ctx.shared.exti.pr.write(|w| w.pr4().set_bit());
@@ -663,7 +692,8 @@ mod app {
 	#[task(shared = [pin_sys_reset, state_dc_power_enabled])]
 	fn exit_reset(mut ctx: exit_reset::Context) {
 		defmt::debug!("End reset");
-		if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::On {
+		if ctx.shared.state_dc_power_enabled.lock(|r| *r) != DcPowerState::Off {
+			// Raising the reset line takes the rest of the system out of reset
 			ctx.shared.pin_sys_reset.lock(|pin| pin.set_high().unwrap());
 		}
 	}

neotron-bmc-pico/src/spi.rs

@@ -17,29 +17,23 @@ pub struct SpiPeripheral<const RXC: usize, const TXC: usize> {
 	tx_idx: usize,
 	/// How many bytes are loaded into the TX buffer
 	tx_ready: usize,
-	/// Has the RX been processed?
+	/// 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,
-	/// Are we enabled? If not, start and stop won't work.
-	enabled: bool,
 }
 
 impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 	pub fn new<SCKPIN, MISOPIN, MOSIPIN>(
 		dev: pac::SPI1,
 		pins: (SCKPIN, MISOPIN, MOSIPIN),
-		speed_hz: u32,
 		rcc: &mut Rcc,
 	) -> SpiPeripheral<RXC, TXC>
 	where
 		SCKPIN: stm32f0xx_hal::spi::SckPin<pac::SPI1>,
 		MISOPIN: stm32f0xx_hal::spi::MisoPin<pac::SPI1>,
 		MOSIPIN: stm32f0xx_hal::spi::MosiPin<pac::SPI1>,
 	{
-		defmt::info!(
-			"pclk = {}, incoming spi_clock = {}",
-			rcc.clocks.pclk().0,
-			speed_hz
-		);
+		defmt::info!("pclk = {}", rcc.clocks.pclk().0,);
 
 		let mode = embedded_hal::spi::MODE_0;
 
@@ -80,29 +74,33 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 			w.lsbfirst().clear_bit();
 			// 2e. Configure the CRCL and CRCEN bits if CRC is needed (it is not)
 			w.crcen().disabled();
-			// 2f. Turn off soft-slave-management (SSM) and slave-select-internal (SSI)
-			w.ssm().disabled();
-			w.ssi().slave_selected();
+			// 2f. Turn on soft-slave-management (SSM) (we control the NSS signal with the SSI bit).
+			w.ssm().enabled();
+			w.ssi().slave_not_selected();
 			// 2g. Set the Master bit low for slave mode
 			w.mstr().slave();
 			w
 		});
 
 		// 3. Write to SPI_CR2 register
 		dev.cr2.write(|w| {
-			// 3a. Configure the DS[3:0] bits to select the data length for the transfer.
+			// 3a. Configure the DS[3:0] bits to select the data length for the transfer (0b111 = 8-bit words).
 			unsafe { w.ds().bits(0b111) };
-			// 3b. Disable hard-output on the CS pin
+			// 3b. Disable hard-output on the CS pin (ignored in Master mode)
 			w.ssoe().disabled();
 			// 3c. Frame Format
 			w.frf().motorola();
 			// 3d. Set NSSP bit if required (we don't want NSS Pulse mode)
 			w.nssp().no_pulse();
-			// 3e. Configure the FIFO RX Threshold to 1/4 FIFO
+			// 3e. Configure the FIFO RX Threshold to 1/4 FIFO (8 bits)
 			w.frxth().quarter();
-			// 3f. LDMA_TX and LDMA_RX for DMA mode - not used
-			// Extra: Turn on RX Not Empty Interrupt Enable
-			w.rxneie().set_bit();
+			// 3f. Disable DMA mode
+			w.txdmaen().disabled();
+			w.rxdmaen().disabled();
+			// Extra: Turn on RX, TX and Error interrupts
+			w.rxneie().not_masked();
+			w.txeie().not_masked();
+			w.errie().not_masked();
 			w
 		});
 
@@ -118,7 +116,6 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 			tx_idx: 0,
 			tx_ready: 0,
 			is_done: false,
-			enabled: false,
 		};
 
 		// Empty the receive register
@@ -129,23 +126,8 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 		spi
 	}
 
-	/// Allow the [`Self::start`] and [`Self::stop`] functions to actually work.
-	pub fn enable(&mut self) {
-		self.enabled = true;
-	}
-
-	/// Prevent the [`Self::start`] and [`Self::stop`] functions from working.
-	pub fn disable(&mut self) {
-		self.stop();
-		self.enabled = false;
-	}
-
-	/// Enable the SPI peripheral (i.e. when CS is low)
+	/// Enable the SPI peripheral (i.e. when CS goes low)
 	pub fn start(&mut self) {
-		if !self.enabled {
-			return;
-		}
-
 		self.rx_idx = 0;
 		self.tx_idx = 0;
 		self.tx_ready = 0;
@@ -155,37 +137,18 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 			let _ = self.raw_read();
 		}
 		self.dev.cr1.modify(|_r, w| {
-			w.spe().enabled();
+			w.ssi().slave_selected();
 			w
 		});
 		// Load our dummy byte (our TX FIFO will send this then repeat it whilst
 		// it underflows during the receive phase).
 		self.raw_write(0xFF);
-		// Get an IRQ when there's RX data available
-		self.enable_rxne_irq();
 	}
 
-	/// Disable the SPI peripheral (i.e. when CS is high)
+	/// Disable the SPI peripheral (i.e. when CS goes high)
 	pub fn stop(&mut self) {
-		self.disable_rxne_irq();
 		self.dev.cr1.modify(|_r, w| {
-			w.spe().disabled();
-			w
-		});
-	}
-
-	/// Enable RX Not Empty interrupt
-	fn enable_rxne_irq(&mut self) {
-		self.dev.cr2.modify(|_r, w| {
-			w.rxneie().set_bit();
-			w
-		});
-	}
-
-	/// Disable RX Not Empty interrupt
-	fn disable_rxne_irq(&mut self) {
-		self.dev.cr2.modify(|_r, w| {
-			w.rxneie().clear_bit();
+			w.ssi().slave_not_selected();
 			w
 		});
 	}
@@ -197,13 +160,13 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 
 	fn raw_read(&mut self) -> u8 {
 		// PAC only supports 16-bit read, but that pops two bytes off the FIFO.
-		// So force a 16-bit read.
+		// So force an 8-bit read.
 		unsafe { core::ptr::read_volatile(&self.dev.dr as *const _ as *const u8) }
 	}
 
 	fn raw_write(&mut self, data: u8) {
-		// PAC only supports 16-bit read, but that pops two bytes off the FIFO.
-		// So force a 16-bit read.
+		// PAC only supports 16-bit read, but that pushes two bytes onto the FIFO.
+		// So force an 8-bit write.
 		unsafe { core::ptr::write_volatile(&self.dev.dr as *const _ as *mut u8, data) }
 	}
 
@@ -224,6 +187,10 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 
 	pub fn handle_isr(&mut self) {
 		let irq_status = self.dev.sr.read();
+		if irq_status.fre().is_error() || irq_status.ovr().is_overrun() {
+			// Handle errors!?
+			defmt::info!("SPI sr=0b{:08b}", irq_status.bits());
+		}
 		if irq_status.rxne().is_not_empty() {
 			self.rx_isr();
 		}
@@ -244,9 +211,10 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 	/// Call this in the TXEIE interrupt. It will load the SPI FIFO with some
 	/// data, either from `tx_buffer` or a padding byte.
 	fn tx_isr(&mut self) {
-		if (self.tx_idx < self.tx_ready) && (self.tx_idx < self.tx_buffer.len()) {
-			// We have some data yet to send
-			let next_tx = self.tx_buffer[self.tx_idx];
+		if self.tx_idx < self.tx_ready {
+			// We have some data yet to send. This is safe as long as we do the
+			// bounds check when we set `self.tx_ready`.
+			let next_tx = unsafe { *self.tx_buffer.get_unchecked(self.tx_idx) };
 			self.raw_write(next_tx);
 			self.tx_idx += 1;
 		} else {
@@ -262,12 +230,15 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 		self.tx_ready = 0;
 		self.tx_idx = 0;
 		if data.len() > TXC {
-			// Too much data
+			// Too much data. This check is important for safety in
+			// [`Self::tx_isr`].
 			return Err(TXC);
 		}
 		for (inc, space) in data.iter().zip(self.tx_buffer.iter_mut()) {
 			*space = *inc;
 		}
+		// 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();
 		Ok(())
 	}
@@ -284,7 +255,9 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
 
 		match message.render_to_buffer(&mut self.tx_buffer) {
 			Ok(n) => {
-				self.tx_ready = n;
+				// 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);
 				Ok(())
 			}
 			Err(_) => Err(()),