Skip to content

Commit 0efe4a8

Browse files
thejpsterthejpster
authored
Merge pull request #41 from Neotron-Compute/fix-spi-disable
2 parents c728714 + 5c9446c commit 0efe4a8

File tree

2 files changed

+94
-88
lines changed

2 files changed

+94
-88
lines changed

‎neotron-bmc-pico/src/main.rs‎

Lines changed: 49 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ mod app {
8080
Ps2Data1(u16),
8181
/// Message from SPI bus
8282
SpiRequest(neotron_bmc_protocol::Request),
83+
/// SPI CS went low (active)
84+
SpiEnable,
85+
/// SPI CS went high (inactive)
86+
SpiDisable,
8387
/// The power button was given a press
8488
PowerButtonShortPress,
8589
/// The power button was held down
@@ -281,7 +285,6 @@ mod app {
281285
let spi = neotron_bmc_pico::spi::SpiPeripheral::new(
282286
dp.SPI1,
283287
(pin_sck, pin_cipo, pin_copi),
284-
2_000_000,
285288
&mut rcc,
286289
);
287290

@@ -347,7 +350,7 @@ mod app {
347350
/// Our idle task.
348351
///
349352
/// This task is called when there is nothing else to do.
350-
#[idle(shared = [msg_q_out, msg_q_in, spi, state_dc_power_enabled, pin_dc_on, pin_sys_reset, pin_cs])]
353+
#[idle(shared = [msg_q_out, msg_q_in, spi, state_dc_power_enabled, pin_dc_on, pin_sys_reset])]
351354
fn idle(mut ctx: idle::Context) -> ! {
352355
// TODO: Get this from the VERSION static variable or from PKG_VERSION
353356
let mut register_state = RegisterState {
@@ -377,36 +380,46 @@ mod app {
377380
}
378381
Some(Message::PowerButtonLongPress) => {
379382
if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::On {
380-
defmt::info!("Power button held whilst on.");
383+
defmt::info!("Power off requested!");
381384
ctx.shared
382385
.state_dc_power_enabled
383386
.lock(|r| *r = DcPowerState::Off);
384-
defmt::info!("Power off!");
387+
// Stop any SPI stuff that's currently going on (the host is about to be powered off)
388+
ctx.shared.spi.lock(|s| s.stop());
389+
// Put the host into reset
385390
ctx.shared.pin_sys_reset.lock(|pin| pin.set_low().unwrap());
391+
// Shut off the 5V power
386392
ctx.shared.pin_dc_on.set_low().unwrap();
387-
ctx.shared.spi.lock(|s| s.disable());
388393
// Start LED blinking again
389394
led_power_blink::spawn().unwrap();
390395
}
391396
}
392397
Some(Message::PowerButtonShortPress) => {
393398
if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::Off {
394-
defmt::info!("Power button pressed whilst off. Powering up.");
395-
// Button pressed - power on system
399+
defmt::info!("Power up requested!");
400+
// Button pressed - power on system.
401+
// Step 1 - Note our new power state
396402
ctx.shared
397403
.state_dc_power_enabled
398404
.lock(|r| *r = DcPowerState::Starting);
405+
// Step 2 - Hold reset line (active) low
406+
ctx.shared.pin_sys_reset.lock(|pin| pin.set_low().unwrap());
407+
// Step 3 - Turn on PSU
399408
ctx.shared.pin_dc_on.set_high().unwrap();
409+
// Step 4 - Leave it in reset for a while.
400410
// TODO: Start monitoring 3.3V and 5.0V rails here
401411
// TODO: Take system out of reset when 3.3V and 5.0V are good
402-
ctx.shared.pin_sys_reset.lock(|pin| pin.set_high().unwrap());
403-
ctx.shared.spi.lock(|s| s.enable());
412+
// Returns an error if it's already scheduled (but we don't care)
413+
let _ = exit_reset::spawn_after(RESET_DURATION_MS.millis());
404414
}
405415
}
406416
Some(Message::PowerButtonRelease) => {
407417
if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::Starting {
408418
defmt::info!("Power button released.");
409-
// Button released after power on
419+
// Button released after power on. Change the power
420+
// state machine t "On". We were in 'Starting' to ignore
421+
// any further button events until the button had been
422+
// released.
410423
ctx.shared
411424
.state_dc_power_enabled
412425
.lock(|r| *r = DcPowerState::On);
@@ -417,10 +430,24 @@ mod app {
417430
if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::On {
418431
defmt::info!("Reset!");
419432
ctx.shared.pin_sys_reset.lock(|pin| pin.set_low().unwrap());
420-
// Returns an error if it's already scheduled
433+
// Returns an error if it's already scheduled (but we don't care)
421434
let _ = exit_reset::spawn_after(RESET_DURATION_MS.millis());
422435
}
423436
}
437+
Some(Message::SpiEnable) => {
438+
if ctx.shared.state_dc_power_enabled.lock(|r| *r) != DcPowerState::Off {
439+
// Turn on the SPI peripheral
440+
ctx.shared.spi.lock(|s| s.start());
441+
} else {
442+
// Ignore message - it'll be the CS line being pulled low when the host is powered off
443+
defmt::info!("Ignoring spurious CS low");
444+
}
445+
}
446+
Some(Message::SpiDisable) => {
447+
// Turn off the SPI peripheral. Don't need to check power state for this.
448+
ctx.shared.spi.lock(|s| s.stop());
449+
defmt::trace!("SPI Disable");
450+
}
424451
Some(Message::SpiRequest(req)) => {
425452
process_command(req, &mut register_state, |rsp| {
426453
ctx.shared.spi.lock(|spi| {
@@ -487,7 +514,7 @@ mod app {
487514
#[task(
488515
binds = EXTI4_15,
489516
priority = 4,
490-
shared = [ps2_clk0, msg_q_in, ps2_dat0, exti, spi, pin_cs, kb_decoder],
517+
shared = [ps2_clk0, msg_q_in, ps2_dat0, exti, pin_cs, kb_decoder],
491518
)]
492519
fn exti4_15_interrupt(mut ctx: exti4_15_interrupt::Context) {
493520
let pr = ctx.shared.exti.pr.read();
@@ -511,12 +538,15 @@ mod app {
511538
}
512539

513540
if pr.pr4().bit_is_set() {
514-
if ctx.shared.pin_cs.lock(|pin| pin.is_low().unwrap()) {
515-
// If incoming Chip Select is low, turn on the SPI engine
516-
ctx.shared.spi.lock(|s| s.start());
541+
let msg = if ctx.shared.pin_cs.lock(|pin| pin.is_low().unwrap()) {
542+
// If incoming Chip Select is low, tell the main thread to turn on the SPI engine
543+
Message::SpiEnable
517544
} else {
518-
// If incoming Chip Select is high, turn off the SPI engine
519-
ctx.shared.spi.lock(|s| s.stop());
545+
// If incoming Chip Select is high, tell the main thread to turn off the SPI engine
546+
Message::SpiDisable
547+
};
548+
if ctx.shared.msg_q_in.lock(|q| q.enqueue(msg)).is_err() {
549+
panic!("queue full");
520550
}
521551
// Clear the pending flag for this pin
522552
ctx.shared.exti.pr.write(|w| w.pr4().set_bit());
@@ -663,7 +693,8 @@ mod app {
663693
#[task(shared = [pin_sys_reset, state_dc_power_enabled])]
664694
fn exit_reset(mut ctx: exit_reset::Context) {
665695
defmt::debug!("End reset");
666-
if ctx.shared.state_dc_power_enabled.lock(|r| *r) == DcPowerState::On {
696+
if ctx.shared.state_dc_power_enabled.lock(|r| *r) != DcPowerState::Off {
697+
// Raising the reset line takes the rest of the system out of reset
667698
ctx.shared.pin_sys_reset.lock(|pin| pin.set_high().unwrap());
668699
}
669700
}

‎neotron-bmc-pico/src/spi.rs‎

Lines changed: 45 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -17,29 +17,23 @@ pub struct SpiPeripheral<const RXC: usize, const TXC: usize> {
1717
tx_idx: usize,
1818
/// How many bytes are loaded into the TX buffer
1919
tx_ready: usize,
20-
/// Has the RX been processed?
20+
/// Has the RX been processed? If so, lie and say we don't have any data
21+
/// (otherwise we process incoming messages multiple times).
2122
is_done: bool,
22-
/// Are we enabled? If not, start and stop won't work.
23-
enabled: bool,
2423
}
2524

2625
impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
2726
pub fn new<SCKPIN, MISOPIN, MOSIPIN>(
2827
dev: pac::SPI1,
2928
pins: (SCKPIN, MISOPIN, MOSIPIN),
30-
speed_hz: u32,
3129
rcc: &mut Rcc,
3230
) -> SpiPeripheral<RXC, TXC>
3331
where
3432
SCKPIN: stm32f0xx_hal::spi::SckPin<pac::SPI1>,
3533
MISOPIN: stm32f0xx_hal::spi::MisoPin<pac::SPI1>,
3634
MOSIPIN: stm32f0xx_hal::spi::MosiPin<pac::SPI1>,
3735
{
38-
defmt::info!(
39-
"pclk = {}, incoming spi_clock = {}",
40-
rcc.clocks.pclk().0,
41-
speed_hz
42-
);
36+
defmt::info!("pclk = {}", rcc.clocks.pclk().0,);
4337

4438
let mode = embedded_hal::spi::MODE_0;
4539

@@ -80,29 +74,35 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
8074
w.lsbfirst().clear_bit();
8175
// 2e. Configure the CRCL and CRCEN bits if CRC is needed (it is not)
8276
w.crcen().disabled();
83-
// 2f. Turn off soft-slave-management (SSM) and slave-select-internal (SSI)
84-
w.ssm().disabled();
85-
w.ssi().slave_selected();
77+
// 2f. Turn on soft-slave-management (SSM) (we control the NSS signal with the SSI bit).
78+
w.ssm().enabled();
79+
w.ssi().slave_not_selected();
8680
// 2g. Set the Master bit low for slave mode
8781
w.mstr().slave();
8882
w
8983
});
9084

9185
// 3. Write to SPI_CR2 register
9286
dev.cr2.write(|w| {
93-
// 3a. Configure the DS[3:0] bits to select the data length for the transfer.
87+
// 3a. Configure the DS[3:0] bits to select the data length for the transfer (0b111 = 8-bit words).
9488
unsafe { w.ds().bits(0b111) };
95-
// 3b. Disable hard-output on the CS pin
89+
// 3b. Disable hard-output on the CS pin (ignored in Master mode)
9690
w.ssoe().disabled();
9791
// 3c. Frame Format
9892
w.frf().motorola();
9993
// 3d. Set NSSP bit if required (we don't want NSS Pulse mode)
10094
w.nssp().no_pulse();
101-
// 3e. Configure the FIFO RX Threshold to 1/4 FIFO
95+
// 3e. Configure the FIFO RX Threshold to 1/4 FIFO (8 bits)
10296
w.frxth().quarter();
103-
// 3f. LDMA_TX and LDMA_RX for DMA mode - not used
104-
// Extra: Turn on RX Not Empty Interrupt Enable
105-
w.rxneie().set_bit();
97+
// 3f. Disable DMA mode
98+
w.txdmaen().disabled();
99+
w.rxdmaen().disabled();
100+
// Extra: Turn on RX and Error interrupts, but not TX. The TX
101+
// interrupt is turned off because we deliberately underflow the
102+
// FIFO during the receive phase of the transaction.
103+
w.rxneie().not_masked();
104+
w.txeie().masked();
105+
w.errie().not_masked();
106106
w
107107
});
108108

@@ -118,34 +118,26 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
118118
tx_idx: 0,
119119
tx_ready: 0,
120120
is_done: false,
121-
enabled: false,
122121
};
123122

124123
// Empty the receive register
125124
while spi.has_rx_data() {
126125
let _ = spi.raw_read();
127126
}
128127

129-
spi
130-
}
131-
132-
/// Allow the [`Self::start`] and [`Self::stop`] functions to actually work.
133-
pub fn enable(&mut self) {
134-
self.enabled = true;
135-
}
128+
// Enable the SPI device
129+
spi.stop();
130+
spi.dev.cr1.write(|w| {
131+
// Enable the peripheral
132+
w.spe().enabled();
133+
w
134+
});
136135

137-
/// Prevent the [`Self::start`] and [`Self::stop`] functions from working.
138-
pub fn disable(&mut self) {
139-
self.stop();
140-
self.enabled = false;
136+
spi
141137
}
142138

143-
/// Enable the SPI peripheral (i.e. when CS is low)
139+
/// Enable the SPI peripheral (i.e. when CS goes low)
144140
pub fn start(&mut self) {
145-
if !self.enabled {
146-
return;
147-
}
148-
149141
self.rx_idx = 0;
150142
self.tx_idx = 0;
151143
self.tx_ready = 0;
@@ -155,37 +147,15 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
155147
let _ = self.raw_read();
156148
}
157149
self.dev.cr1.modify(|_r, w| {
158-
w.spe().enabled();
150+
w.ssi().slave_selected();
159151
w
160152
});
161-
// Load our dummy byte (our TX FIFO will send this then repeat it whilst
162-
// it underflows during the receive phase).
163-
self.raw_write(0xFF);
164-
// Get an IRQ when there's RX data available
165-
self.enable_rxne_irq();
166153
}
167154

168-
/// Disable the SPI peripheral (i.e. when CS is high)
155+
/// Disable the SPI peripheral (i.e. when CS goes high)
169156
pub fn stop(&mut self) {
170-
self.disable_rxne_irq();
171157
self.dev.cr1.modify(|_r, w| {
172-
w.spe().disabled();
173-
w
174-
});
175-
}
176-
177-
/// Enable RX Not Empty interrupt
178-
fn enable_rxne_irq(&mut self) {
179-
self.dev.cr2.modify(|_r, w| {
180-
w.rxneie().set_bit();
181-
w
182-
});
183-
}
184-
185-
/// Disable RX Not Empty interrupt
186-
fn disable_rxne_irq(&mut self) {
187-
self.dev.cr2.modify(|_r, w| {
188-
w.rxneie().clear_bit();
158+
w.ssi().slave_not_selected();
189159
w
190160
});
191161
}
@@ -197,13 +167,13 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
197167

198168
fn raw_read(&mut self) -> u8 {
199169
// PAC only supports 16-bit read, but that pops two bytes off the FIFO.
200-
// So force a 16-bit read.
170+
// So force an 8-bit read.
201171
unsafe { core::ptr::read_volatile(&self.dev.dr as *const _ as *const u8) }
202172
}
203173

204174
fn raw_write(&mut self, data: u8) {
205-
// PAC only supports 16-bit read, but that pops two bytes off the FIFO.
206-
// So force a 16-bit read.
175+
// PAC only supports 16-bit read, but that pushes two bytes onto the FIFO.
176+
// So force an 8-bit write.
207177
unsafe { core::ptr::write_volatile(&self.dev.dr as *const _ as *mut u8, data) }
208178
}
209179

@@ -244,14 +214,14 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
244214
/// Call this in the TXEIE interrupt. It will load the SPI FIFO with some
245215
/// data, either from `tx_buffer` or a padding byte.
246216
fn tx_isr(&mut self) {
247-
if (self.tx_idx < self.tx_ready) && (self.tx_idx < self.tx_buffer.len()) {
248-
// We have some data yet to send
249-
let next_tx = self.tx_buffer[self.tx_idx];
217+
if self.tx_idx < self.tx_ready {
218+
// We have some data yet to send. This is safe as long as we do the
219+
// bounds check when we set `self.tx_ready`.
220+
let next_tx = unsafe { *self.tx_buffer.get_unchecked(self.tx_idx) };
250221
self.raw_write(next_tx);
251222
self.tx_idx += 1;
252223
} else {
253-
// No data - send padding
254-
self.raw_write(0xFF);
224+
// No data - send nothing
255225
}
256226
}
257227

@@ -262,12 +232,15 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
262232
self.tx_ready = 0;
263233
self.tx_idx = 0;
264234
if data.len() > TXC {
265-
// Too much data
235+
// Too much data. This check is important for safety in
236+
// [`Self::tx_isr`].
266237
return Err(TXC);
267238
}
268239
for (inc, space) in data.iter().zip(self.tx_buffer.iter_mut()) {
269240
*space = *inc;
270241
}
242+
// We must never set this to be longer than `TXC` as we do an unchecked
243+
// read from `self.tx_buffer` in [`Self::tx_isr`].
271244
self.tx_ready = data.len();
272245
Ok(())
273246
}
@@ -284,7 +257,9 @@ impl<const RXC: usize, const TXC: usize> SpiPeripheral<RXC, TXC> {
284257

285258
match message.render_to_buffer(&mut self.tx_buffer) {
286259
Ok(n) => {
287-
self.tx_ready = n;
260+
// We must never set this to be longer than `TXC` as we do an
261+
// unchecked read from `self.tx_buffer` in [`Self::tx_isr`].
262+
self.tx_ready = n.min(TXC);
288263
Ok(())
289264
}
290265
Err(_) => Err(()),

0 commit comments

Comments
 (0)