From bf8e0e2e0ecf11449642dab9426372b86f53227a Mon Sep 17 00:00:00 2001 From: Philip Levis Date: Tue, 27 Feb 2018 16:23:48 -0800 Subject: [PATCH 1/3] This patch is to deal with issue #617 Radio Race Condition. The issue is that if the RF233 has a frame in its memory and starts receiving another frame, the second one can overwrite the first. If software does not read the first one out fast enough, this can lead to corrupted packets. While the radio has some hardware features to try to protect against this (Sec 11.8 of manual, Dynamic Frame Buffer Protection), its use requires a very specific usage model of the SPI bus that Tock's SPI abstraction does not support: it requires that the software read a single byte from packet memory over the SPI bus, and without releasing the chip select line, read the rest of the packet (based on the value of this first byte). This patch explicitly disables reception after a packet is received, and re-enables it after the packet is successfully read out of the radio. It's been tested with the ieee802154 test applications, including at higher packet transmit rates. However, it is possible that there are edge cases on interleaved RX/TX events that might put the radio into a wedged state. I'll do a careful read-over of the code again in a few days once it's cleared from my head. --- capsules/src/rf233.rs | 75 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 11 deletions(-) diff --git a/capsules/src/rf233.rs b/capsules/src/rf233.rs index c491d3abd8..f3e9f5d6ac 100644 --- a/capsules/src/rf233.rs +++ b/capsules/src/rf233.rs @@ -124,12 +124,15 @@ enum InternalState { // received but not the rest of the packet yet. RX, // The packet has been successfully received + RX_TURNING_OFF, // Disabling packet reception + RX_READY_TO_READ, // Reception disabled, handle interrupt and start reading RX_START_READING, // Starting to read a packet out of the radio RX_READING_FRAME_LEN, // We've read the length of the frame RX_READING_FRAME_LEN_DONE, RX_READING_FRAME, // Reading the packet out of the radio RX_READING_FRAME_DONE, // Now read a register to verify FCS RX_READING_FRAME_FCS_DONE, + RX_ENABLING_RECEPTION, // Re-enabling reception } // There are two tricky parts to this capsule: buffer management @@ -180,6 +183,7 @@ pub struct RF233<'a, S: spi::SpiMasterDevice + 'a> { transmitting: Cell, receiving: Cell, spi_busy: Cell, + crc_valid: Cell, interrupt_handling: Cell, interrupt_pending: Cell, config_pending: Cell, @@ -265,6 +269,9 @@ fn interrupt_included(mask: u8, interrupt: u8) -> bool { } impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { + // This function is a bit confusing because the order of the logic in the + // function is different than the order of operations during transmission + // and reception. fn read_write_done( &self, mut _write: &'static mut [u8], @@ -310,12 +317,14 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { self.spi_tx.replace(_write); } + let state = self.state.get(); + // This case is when the SPI operation is reading the IRQ_STATUS // register from handling an interrupt. Note that we're done handling // the interrupt and continue with the state machine. if handling { self.interrupt_handling.set(false); - let state = self.state.get(); + let interrupt = result; // If we're going to sleep, ignore the interrupt and continue @@ -335,7 +344,8 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { self.state.set(InternalState::RX); } - // We've received an entire frame into the frame buffer. + // We've received an entire frame into the frame buffer. This should be + // in the InternalState::RX_READY_TO_READ state. // There are three cases: // 1. we have a receive buffer: copy it out // 2. no receive buffer, but transmission pending: send @@ -378,7 +388,10 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { // receiving a frame. if self.interrupt_pending.get() { match self.state.get() { - InternalState::RX_READING_FRAME_DONE | InternalState::RX_READING_FRAME_FCS_DONE => { + InternalState::RX_TURNING_OFF | + InternalState::RX_START_READING | + InternalState::RX_READING_FRAME_DONE | + InternalState::RX_READING_FRAME_FCS_DONE => { } _ => { self.interrupt_pending.set(false); @@ -387,7 +400,6 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { } } } - // Similarly, if a configuration is pending, we only start the // configuration process when we are in a state where it is legal to // start the configuration process. @@ -784,6 +796,20 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { // No operations in the RX state, an SFD interrupt should // take us out of it. InternalState::RX => {} + InternalState::RX_TURNING_OFF => { + // This is the case when the driver turns off reception in + // response to receiving a frame, to make sure it is not + // overwritten. Now we are reading to handle the interrupt and + // start reading out the frame. + self.state_transition_read( + RF233Register::IRQ_STATUS, + InternalState::RX_READY_TO_READ); + self.interrupt_handling.set(true); + } + // This state is when the driver handles the pending TRX_END interrupt + // on reception, so is handled above in the interrupt logic. + // the pending interrupt will be handled + InternalState::RX_READY_TO_READ => {} // Read the length out InternalState::RX_START_READING => { @@ -831,7 +857,15 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { ); } InternalState::RX_READING_FRAME_FCS_DONE => { - let crc_valid = result & PHY_RSSI_RX_CRC_VALID != 0; + // Store whether the CRC was valid, then turn the radio back on. + self.crc_valid.set((result & PHY_RSSI_RX_CRC_VALID) != 0); + self.state_transition_write( + RF233Register::TRX_STATE, + RF233TrxCmd::RX_AACK_ON as u8, + InternalState::RX_ENABLING_RECEPTION, + ); + } + InternalState::RX_ENABLING_RECEPTION => { self.receiving.set(false); // Stay awake if we receive a packet, another call to stop() @@ -854,7 +888,7 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { self.rx_client.get().map(|client| { let rbuf = self.rx_buf.take().unwrap(); let frame_len = rbuf[1] as usize - radio::MFR_SIZE; - client.receive(rbuf, frame_len, crc_valid, ReturnCode::SUCCESS); + client.receive(rbuf, frame_len, self.crc_valid.get(), ReturnCode::SUCCESS); }); } @@ -988,6 +1022,7 @@ impl<'a, S: spi::SpiMasterDevice + 'a> RF233<'a, S> { transmitting: Cell::new(false), receiving: Cell::new(false), spi_busy: Cell::new(false), + crc_valid: Cell::new(false), state: Cell::new(InternalState::START), interrupt_handling: Cell::new(false), interrupt_pending: Cell::new(false), @@ -1014,12 +1049,30 @@ impl<'a, S: spi::SpiMasterDevice + 'a> RF233<'a, S> { } fn handle_interrupt(&self) { - // Because the first thing we do on handling an interrupt is - // read the IRQ status, we defer handling the state transition - // to the SPI handler + // In most cases, the first thing the driver does on handling an interrupt is + // read the IRQ status; this pushes most logic to the SPI handler. + // The one exception is when the radio receives a packet; to prevent this + // packet from being overwritten before reading it from the radio, + // the driver needs to disable reception. This has to be done in the first + // SPI operation. if self.spi_busy.get() == false { - self.interrupt_handling.set(true); - self.register_read(RF233Register::IRQ_STATUS); + if self.state.get() == InternalState::RX { + // We've received a complete frame; need to disable + // reception until we've read it out from RAM, + // otherwise subsequent packets may corrupt it. + // Dynamic Frame Buffer protection (RF233 manual, Sec + // 11.8) is insufficient because we perform multiple + // SPI operations to read a frame, and the RF233 + // releases its protection after the first SPI + // operation. + self.state_transition_write( + RF233Register::TRX_STATE, + RF233TrxCmd::PLL_ON as u8, + InternalState::RX_TURNING_OFF); + } else { + self.interrupt_handling.set(true); + self.register_read(RF233Register::IRQ_STATUS); + } } else { self.interrupt_pending.set(true); } From 883e7b5da1958042f28c1119392dc2ba96ea270a Mon Sep 17 00:00:00 2001 From: Philip Levis Date: Tue, 27 Feb 2018 16:39:41 -0800 Subject: [PATCH 2/3] Formatting. --- capsules/src/rf233.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/capsules/src/rf233.rs b/capsules/src/rf233.rs index f3e9f5d6ac..99d14634d0 100644 --- a/capsules/src/rf233.rs +++ b/capsules/src/rf233.rs @@ -388,11 +388,10 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { // receiving a frame. if self.interrupt_pending.get() { match self.state.get() { - InternalState::RX_TURNING_OFF | - InternalState::RX_START_READING | - InternalState::RX_READING_FRAME_DONE | - InternalState::RX_READING_FRAME_FCS_DONE => { - } + InternalState::RX_TURNING_OFF + | InternalState::RX_START_READING + | InternalState::RX_READING_FRAME_DONE + | InternalState::RX_READING_FRAME_FCS_DONE => {} _ => { self.interrupt_pending.set(false); self.handle_interrupt(); @@ -803,7 +802,8 @@ impl<'a, S: spi::SpiMasterDevice + 'a> spi::SpiMasterClient for RF233<'a, S> { // start reading out the frame. self.state_transition_read( RF233Register::IRQ_STATUS, - InternalState::RX_READY_TO_READ); + InternalState::RX_READY_TO_READ, + ); self.interrupt_handling.set(true); } // This state is when the driver handles the pending TRX_END interrupt @@ -1068,7 +1068,8 @@ impl<'a, S: spi::SpiMasterDevice + 'a> RF233<'a, S> { self.state_transition_write( RF233Register::TRX_STATE, RF233TrxCmd::PLL_ON as u8, - InternalState::RX_TURNING_OFF); + InternalState::RX_TURNING_OFF, + ); } else { self.interrupt_handling.set(true); self.register_read(RF233Register::IRQ_STATUS); From ff133577d0326c8283fcc0d8f3159cbfc8fcf23f Mon Sep 17 00:00:00 2001 From: Philip Levis Date: Tue, 27 Feb 2018 16:51:06 -0800 Subject: [PATCH 3/3] Fix bug in which ENOACK was not correctly being returned. --- userland/libtock/ieee802154.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/userland/libtock/ieee802154.c b/userland/libtock/ieee802154.c index c1844526c0..042a083109 100644 --- a/userland/libtock/ieee802154.c +++ b/userland/libtock/ieee802154.c @@ -327,7 +327,14 @@ int ieee802154_send(unsigned short addr, err = command(RADIO_DRIVER, COMMAND_SEND, (unsigned int) addr, 0); if (err < 0) return err; yield_for(&tx_done); - return tx_result; + if (tx_result != TOCK_SUCCESS) { + return tx_result; + } else if (tx_acked == 0) { + return TOCK_ENOACK; + } else { + return TOCK_SUCCESS; + } + } // Internal callback for receive