From 2d42cb7141a115c0e1de42257fc338a8848ed75d Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 11:00:24 -0400 Subject: [PATCH 01/11] nrf: 154: use correct define for CRC length --- chips/nrf52840/src/ieee802154_radio.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index fe96d8c36d..ddc9a943bf 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -1263,9 +1263,9 @@ impl<'a> kernel::hil::radio::RadioConfig<'a> for Radio<'a> { fn set_tx_power(&self, tx_power: i8) -> Result<(), ErrorCode> { // Convert u8 to TxPower match nrf52::constants::TxPower::try_from(tx_power as u8) { - // Invalid transmitting power, propogate error + // Invalid transmitting power, propagate error Err(()) => Err(ErrorCode::NOSUPPORT), - // Valid transmitting power, propogate success + // Valid transmitting power, propagate success Ok(res) => { self.tx_power.set(res); Ok(()) @@ -1294,9 +1294,9 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { ) -> Result<(), (ErrorCode, &'static mut [u8])> { if self.tx_buf.is_some() { // tx_buf TakeCell is only occupied when a transmission is underway. This - // check insures we do not interrupt an ungoing transmission + // check insures we do not interrupt an ongoing transmission. return Err((ErrorCode::BUSY, buf)); - } else if radio::PSDU_OFFSET + frame_len >= buf.len() { + } else if radio::MFR_SIZE + frame_len >= buf.len() { // Not enough room for CRC return Err((ErrorCode::SIZE, buf)); } From 003c4e6337197e9818a9b167a889f79eae7bbeb9 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 11:02:45 -0400 Subject: [PATCH 02/11] nrf: 154: Format doc header comment --- chips/nrf52840/src/ieee802154_radio.rs | 109 +++++++++++++------------ 1 file changed, 59 insertions(+), 50 deletions(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index ddc9a943bf..abe849a998 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -3,6 +3,65 @@ // Copyright Tock Contributors 2022. //! IEEE 802.15.4 radio driver for nRF52 +//! +//! This driver implements a subset of 802.15.4 sending and receiving for the +//! nRF52840 chip per the nRF52840_PS_v1.0 spec. Upon calling the initialization +//! function, the chip is powered on and configured to the fields of the Radio +//! struct. This driver maintains a state machine between receiving, +//! transmitting, and sending acknowledgements. Because the nRF52840 15.4 radio +//! chip does not possess hardware support for ACK, this driver implements +//! software support for sending ACK when a received packet requests to be +//! acknowledged. The driver currently lacks support to listen for requested ACK +//! on packets the radio has sent. As of 8/14/23, the driver is able to send and +//! receive 15.4 packets as used in the basic 15.4 libtock-c apps. +//! +//! ## Driver State Machine +//! +//! To aid in future implementations, this describes a simplified and concise +//! version of the nrf52840 radio state machine specification and the state +//! machine this driver separately maintains. +//! +//! To interact with the radio, tasks are issued to the radio which in turn +//! trigger interrupt events. To receive, the radio must first "ramp up". The +//! RXRU state is entered by issuing a RXEN task. Once the radio has ramped up +//! successfully, it is now in the RXIDLE state and triggers a READY interrupt +//! event. To optimize the radio's operation, this driver enables hardware +//! shortcuts such that upon receiving the READY event, the radio chip +//! immediately triggers a START task. The START task notifies the radio to begin +//! officially "listening for packets" (RX state). Upon completing receiving the +//! packet, the radio issues an END event. The driver then determines if the +//! received packet has requested to be acknowledged (bit flag) and sends an ACK +//! accordingly. Finally, the received packet buffer and accompanying fields are +//! passed to the registered radio client. This marks the end of a receive cycle +//! and a new READY event is issued to once again begin listening for packets. +//! +//! When a registered radio client wishes to send a packet. The transmit(...) +//! method is called. To transmit a packet, the radio must first ramp up for +//! receiving and then perform a clear channel assessment by listening for a +//! specified period of time to determine if there is "traffic". If traffic is +//! detected, the radio sets an alarm and waits to perform another CCA after this +//! backoff. If the channel is determined to be clear, the radio then begins a TX +//! ramp up, enters a TX state and then sends the packet. To progress through +//! these states, hardware shortcuts are once again enabled in this driver. The +//! driver first issues a DISABLE task. A hardware shortcut is enabled so that +//! upon receipt of the disable task, the radio automatically issues a RXEN task +//! to enter the RXRU state. Additionally, a shortcut is enabled such that when +//! the RXREADY event is received, the radio automatically issues a CCA_START +//! task. Finally, a shortcut is also enabled such that upon receiving a CCAIDLE +//! event the radio automatically issues a TXEN event to ramp up the radio. The +//! driver then handles receiving the READY interrupt event and triggers the +//! START task to begin sending the packet. Upon completing the sending of the +//! packet, the radio issues an END event, to which the driver then returns the +//! radio to a receiving mode as described above. (For a more complete +//! explanation of the radio's operation, refer to nRF52840_PS_v1.0) +//! +//! This radio state machine provides nine possible states the radio can exist +//! in. For ease of implementation and clarity, this driver also maintains a +//! simplified state machine. These states consist of the radio being off (OFF), +//! receiving (RX), transmitting (TX), or acknowledging (ACK). + +// Author: Tyler Potyondy +// 8/21/23 use crate::timer::TimerAlarm; use core::cell::Cell; @@ -16,56 +75,6 @@ use kernel::ErrorCode; use nrf52::constants::TxPower; -// This driver implements a subset of 802.15.4 sending and receiving for the -// nRF52840 chip per the nRF52840_PS_v1.0 spec. Upon calling the initialization -// function, the chip is powered on and configured to the fields of the Radio -// struct. This driver maintains a state machine between receiving, transmitting, -// and sending acknowledgements. Because the nRF52840 15.4 radio chip does not -// possess hardware support for ACK, this driver implements software support -// for sending ACK when a received packet requests to be acknowledged. The driver -// currently lacks support to listen for requested ACK on packets the radio has sent. -// As of 8/14/23, the driver is able to send and receive 15.4 packets as used in the -// basic 15.4 libtock-c apps. -// -// To aid in future implementations, a simplified and concise version of the nrf52840 radio -// state machine specification is described. Additionally, the state machine this driver separately -// maintains is also described. -// -// To interact with the radio, tasks are issued to the radio which in turn trigger interrupt events. -// To receive, the radio must first "ramp up". The RXRU state is entered by issuing a RXEN task. -// Once the radio has ramped up successfully, it is now in the RXIDLE state and triggers a READY -// interrupt event. To optimize the radio's operation, this driver enables hardware shortcuts such -// that upon receiving the READY event, the radio chip immediately triggers a START task. The -// START task notifies the radio to begin officially "listening for packets" (RX state). Upon -// completing receiving the packet, the radio issues an END event. The driver then determines -// if the received packet has requested to be acknowledged (bit flag) and sends an ACK accordingly. -// Finally, the received packet buffer and accompanying fields are passed to the registered radio -// client. This marks the end of a receive cycle and a new READY event is issued to once again -// begin listening for packets. -// -// When a registered radio client wishes to send a packet. The transmit(...) method is called. -// To transmit a packet, the radio must first ramp up for receiving and then perform a clear channel assessment -// by listening for a specified period of time to determine if there is "traffic". If traffic is -// detected, the radio sets an alarm and waits to perform another CCA after this backoff. If the -// channel is determined to be clear, the radio then begins a TX ramp up, enters a TX state and then sends -// the packet. To progress through these states, hardware shortcuts are once again enabled in this driver. -// The driver first issues a DISABLE task. A hardware shortcut is enabled so that upon receipt of the disable -// task, the radio automatically issues a RXEN task to enter the RXRU state. Additionally, a shortcut is enabled such that -// when the RXREADY event is received, the radio automatically issues a CCA_START task. Finally, a shortcut is also -// enabled such that upon receiving a CCAIDLE event the radio automatically issues a TXEN event to ramp up the -// radio. The driver then handles receiving the READY interrupt event and triggers the START task to begin -// sending the packet. Upon completing the sending of the packet, the radio issues an END event, to which -// the driver then returns the radio to a receiving mode as described above. -// (For a more complete explanation of the radio's operation, refer to nRF52840_PS_v1.0) -// -// This radio state machine provides nine possible states the radio can exist in. For ease of -// implementation and clarity, this driver also maintains a simplified state machine. These -// states consist of the radio being off (OFF), receiving (RX), transmitting (TX), -// or acknowledging (ACK). - -// Author: Tyler Potyondy -// 8/21/23 - const RADIO_BASE: StaticRef = unsafe { StaticRef::new(0x40001000 as *const RadioRegisters) }; From 660531e499b7bfdac72ed95c6852f3d6db9897f9 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 13:19:02 -0400 Subject: [PATCH 03/11] nrf52840: 154: implement config and power No address filtering for nrf52840 in 15.4 mode. --- chips/nrf52840/src/ieee802154_radio.rs | 148 ++++++++++++++++-------- chips/nrf52840/src/interrupt_service.rs | 1 + 2 files changed, 103 insertions(+), 46 deletions(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index abe849a998..c109d4da4b 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -65,6 +65,7 @@ use crate::timer::TimerAlarm; use core::cell::Cell; +use kernel::deferred_call::{DeferredCall, DeferredCallClient}; use kernel::hil::radio::{self, PowerClient, RadioChannel, RadioData, MAX_FRAME_SIZE, PSDU_OFFSET}; use kernel::hil::time::{Alarm, AlarmClient, Time}; use kernel::utilities::cells::{OptionalCell, TakeCell}; @@ -656,11 +657,30 @@ register_bitfields! [u32, ] ]; +#[derive(Debug, Clone, Copy)] +enum RadioState { + OFF, + TX, + RX, + ACK, +} + +/// We use a single deferred call for two operations: triggering config clients +/// and power change clients. This allows us to track which operation we need to +/// perform when we get the deferred call callback. +#[derive(Debug, Clone, Copy)] +enum DeferredOperation { + ConfigClientCallback, + PowerClientCallback, +} + pub struct Radio<'a> { registers: StaticRef, - tx_power: Cell, rx_client: OptionalCell<&'a dyn radio::RxClient>, tx_client: OptionalCell<&'a dyn radio::TxClient>, + config_client: OptionalCell<&'a dyn radio::ConfigClient>, + power_client: OptionalCell<&'a dyn radio::PowerClient>, + tx_power: Cell, tx_buf: TakeCell<'static, [u8]>, rx_buf: TakeCell<'static, [u8]>, ack_buf: TakeCell<'static, [u8]>, @@ -673,6 +693,8 @@ pub struct Radio<'a> { channel: Cell, timer0: OptionalCell<&'a TimerAlarm<'a>>, state: Cell, + deferred_call: DeferredCall, + deferred_call_operation: OptionalCell, } impl<'a> AlarmClient for Radio<'a> { @@ -682,21 +704,16 @@ impl<'a> AlarmClient for Radio<'a> { self.registers.task_ccastart.write(Task::ENABLE::SET); } } -#[derive(Debug, Clone, Copy)] -enum RadioState { - OFF, - TX, - RX, - ACK, -} impl<'a> Radio<'a> { pub fn new(ack_buf: &'static mut [u8; ACK_BUF_SIZE]) -> Self { Self { registers: RADIO_BASE, - tx_power: Cell::new(TxPower::ZerodBm), rx_client: OptionalCell::empty(), tx_client: OptionalCell::empty(), + config_client: OptionalCell::empty(), + power_client: OptionalCell::empty(), + tx_power: Cell::new(TxPower::ZerodBm), tx_buf: TakeCell::empty(), rx_buf: TakeCell::empty(), ack_buf: TakeCell::new(ack_buf), @@ -709,6 +726,8 @@ impl<'a> Radio<'a> { channel: Cell::new(RadioChannel::Channel26), timer0: OptionalCell::empty(), state: Cell::new(RadioState::OFF), + deferred_call: DeferredCall::new(), + deferred_call_operation: OptionalCell::empty(), } } @@ -737,18 +756,6 @@ impl<'a> Radio<'a> { self.registers.task_rxen.write(Task::ENABLE::SET); } - fn set_rx_address(&self) { - self.registers - .rxaddresses - .write(ReceiveAddresses::ADDRESS.val(1)); - } - - fn set_tx_address(&self) { - self.registers - .txaddress - .write(TransmitAddress::ADDRESS.val(0)); - } - fn radio_on(&self) { // reset and enable power self.registers.power.write(Task::ENABLE::CLEAR); @@ -759,8 +766,8 @@ impl<'a> Radio<'a> { self.registers.power.write(Task::ENABLE::CLEAR); } - fn set_tx_power(&self) { - self.registers.txpower.set(self.tx_power.get() as u32); + fn radio_is_on(&self) -> bool { + self.registers.power.is_set(Task::ENABLE) } fn set_dma_ptr(&self, buffer: &'static mut [u8]) -> &'static mut [u8] { @@ -1088,10 +1095,7 @@ impl<'a> Radio<'a> { self.ieee802154_set_tx_power(); - self.ieee802154_set_channel_freq(self.channel.get()); - - self.set_tx_address(); - self.set_rx_address(); + self.ieee802154_set_channel_freq(); // Begin receiving procedure self.enable_interrupts(); @@ -1144,14 +1148,15 @@ impl<'a> Radio<'a> { self.registers.mode.write(Mode::MODE::IEEE802154_250KBIT); } - fn ieee802154_set_channel_freq(&self, channel: RadioChannel) { + fn ieee802154_set_channel_freq(&self) { + let channel = self.channel.get(); self.registers .frequency .write(Frequency::FREQUENCY.val(channel as u32)); } fn ieee802154_set_tx_power(&self) { - self.set_tx_power(); + self.registers.txpower.set(self.tx_power.get() as u32); } pub fn startup(&self) -> Result<(), ErrorCode> { @@ -1180,25 +1185,40 @@ impl<'a> kernel::hil::radio::RadioConfig<'a> for Radio<'a> { Ok(()) } - fn set_power_client(&self, _client: &'a dyn PowerClient) { - // + fn set_power_client(&self, client: &'a dyn PowerClient) { + self.power_client.set(client); } fn reset(&self) -> Result<(), ErrorCode> { self.radio_initialize(); Ok(()) } + fn start(&self) -> Result<(), ErrorCode> { self.radio_initialize(); + + // Configure deferred call to trigger callback. + self.deferred_call_operation + .set(DeferredOperation::PowerClientCallback); + self.deferred_call.set(); + Ok(()) } + fn stop(&self) -> Result<(), ErrorCode> { self.radio_off(); self.state.set(RadioState::OFF); + + // Configure deferred call to trigger callback. + self.deferred_call_operation + .set(DeferredOperation::PowerClientCallback); + self.deferred_call.set(); + Ok(()) } + fn is_on(&self) -> bool { - self.registers.power.is_set(Task::ENABLE) + self.radio_is_on() } // Previous driver implementation // @@ -1206,23 +1226,34 @@ impl<'a> kernel::hil::radio::RadioConfig<'a> for Radio<'a> { false } - //################################################# - ///These methods are holdovers from when the radio HIL was mostly to an external - ///module over an interface - //################################################# - - //fn set_power_client(&self, client: &'static radio::PowerClient){ + fn set_config_client(&self, client: &'a dyn radio::ConfigClient) { + self.config_client.set(client); + } - //} - /// Commit the config calls to hardware, changing the address, - /// PAN ID, TX power, and channel to the specified values, issues - /// a callback to the config client when done. + /// Commit the config calls to hardware, changing (in theory): + /// + /// - the RX address + /// - PAN ID + /// - TX power + /// - channel + /// + /// to the specified values. **However**, the nRF52840 IEEE 802.15.4 radio + /// does not support hardware-level address filtering (see + /// [here](https://devzone.nordicsemi.com/f/nordic-q-a/19320/using-nrf52840-for-802-15-4)). + /// So setting the addresses and PAN ID have no meaning for this chip and + /// any filtering must be done in higher layers in software. + /// + /// Issues a callback to the config client when done. fn config_commit(&self) { - self.radio_off(); - self.radio_initialize(); - } + // All we can configure is TX power and channel frequency. + self.ieee802154_set_tx_power(); + self.ieee802154_set_channel_freq(); - fn set_config_client(&self, _client: &'a dyn radio::ConfigClient) {} + // Enable deferred call so we can generate a `ConfigClient` callback. + self.deferred_call_operation + .set(DeferredOperation::ConfigClientCallback); + self.deferred_call.set(); + } //################################################# /// Accessors @@ -1240,10 +1271,12 @@ impl<'a> kernel::hil::radio::RadioConfig<'a> for Radio<'a> { fn get_pan(&self) -> u16 { self.pan.get() } + /// The transmit power, in dBm fn get_tx_power(&self) -> i8 { self.tx_power.get() as i8 } + /// The 802.15.4 channel fn get_channel(&self) -> u8 { self.channel.get().get_channel_number() @@ -1345,3 +1378,26 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { Ok(()) } } + +impl DeferredCallClient for Radio<'_> { + fn handle_deferred_call(&self) { + // On deferred call we trigger the config or power callbacks. The + // `.take()` ensures we clear what is pending. + self.deferred_call_operation.take().map(|op| match op { + DeferredOperation::ConfigClientCallback => { + self.config_client.map(|client| { + client.config_done(Ok(())); + }); + } + DeferredOperation::PowerClientCallback => { + self.power_client.map(|client| { + client.changed(self.radio_is_on()); + }); + } + }); + } + + fn register(&'static self) { + self.deferred_call.register(self); + } +} diff --git a/chips/nrf52840/src/interrupt_service.rs b/chips/nrf52840/src/interrupt_service.rs index 0f3fdf5ce0..21c717adb3 100644 --- a/chips/nrf52840/src/interrupt_service.rs +++ b/chips/nrf52840/src/interrupt_service.rs @@ -34,6 +34,7 @@ impl<'a> Nrf52840DefaultPeripherals<'a> { self.nrf52.timer0.set_alarm_client(&self.ieee802154_radio); self.nrf52.pwr_clk.set_usb_client(&self.usbd); self.usbd.set_power_ref(&self.nrf52.pwr_clk); + kernel::deferred_call::DeferredCallClient::register(&self.ieee802154_radio); self.nrf52.init(); } } From 82fe9dae3d02210ecffa011b90d1cba5cef340a0 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 13:20:19 -0400 Subject: [PATCH 04/11] nrf52840: 154: do not start in init fn Calling `radio_initialize()` in the HIL::init function puts the radio in to RX mode. That means the radio is on unconditionally in any board that includes the 15.4. The HIL::start() function should be used to start the radio and put it in RX mode. --- chips/nrf52840/src/ieee802154_radio.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index c109d4da4b..063d6e3e34 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -1181,7 +1181,6 @@ impl<'a> Radio<'a> { impl<'a> kernel::hil::radio::RadioConfig<'a> for Radio<'a> { fn initialize(&self) -> Result<(), ErrorCode> { - self.radio_initialize(); Ok(()) } From ce2022b4ee94c4555e76cb4496382c22ac7a8800 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 13:23:24 -0400 Subject: [PATCH 05/11] nrf52840: 154: set state when turning off Avoid an issue where the radio is off but the state is not. --- chips/nrf52840/src/ieee802154_radio.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index 063d6e3e34..386d0b560c 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -763,6 +763,8 @@ impl<'a> Radio<'a> { } fn radio_off(&self) { + self.state.set(RadioState::OFF); + self.registers.power.write(Task::ENABLE::CLEAR); } @@ -1206,7 +1208,6 @@ impl<'a> kernel::hil::radio::RadioConfig<'a> for Radio<'a> { fn stop(&self) -> Result<(), ErrorCode> { self.radio_off(); - self.state.set(RadioState::OFF); // Configure deferred call to trigger callback. self.deferred_call_operation From 654fe8f834c0b98ef6a71c6d2354e78fec2bc8b0 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 13:32:24 -0400 Subject: [PATCH 06/11] nrf52840: 154: format comments --- chips/nrf52840/src/ieee802154_radio.rs | 182 +++++++++++++++---------- 1 file changed, 107 insertions(+), 75 deletions(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index 386d0b560c..0918b81095 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -787,9 +787,10 @@ impl<'a> Radio<'a> { } } - // TODO: RECEIVING ACK FOR A SENT TX IS NOT IMPLEMENTED // - // As a general note for the interrupt handler, event registers must still be cleared even when - // hardware shortcuts are enabled. + // TODO: RECEIVING ACK FOR A SENT TX IS NOT IMPLEMENTED + // + // As a general note for the interrupt handler, event registers must still + // be cleared even when hardware shortcuts are enabled. #[inline(never)] pub fn handle_interrupt(&self) { self.disable_all_interrupts(); @@ -798,14 +799,16 @@ impl<'a> Radio<'a> { let mut rx_init = false; match self.state.get() { - // It should not be possible to receive an interrupt while the tracked radio state is OFF + // It should not be possible to receive an interrupt while the + // tracked radio state is OFF. RadioState::OFF => kernel::debug!("[ERROR]--15.4 state machine diverged from expected behavior. Received interrupt while off"), RadioState::RX => { - //////////////////////////////////////////////////////////////////////////// - // NOTE: This state machine assumes that the READY_START shortcut is enabled - // at this point in time. If the READY_START shortcut is not enabled, the - // state machine/driver will likely exhibit undefined behavior. - //////////////////////////////////////////////////////////////////////////// + //////////////////////////////////////////////////////////////// + // NOTE: This state machine assumes that the READY_START + // shortcut is enabled at this point in time. If the READY_START + // shortcut is not enabled, the state machine/driver will likely + // exhibit undefined behavior. + //////////////////////////////////////////////////////////////// // Since READY_START shortcut enabled, always clear READY event self.registers.event_ready.write(Event::READY::CLEAR); @@ -815,23 +818,26 @@ impl<'a> Radio<'a> { self.registers.event_end.write(Event::READY::CLEAR); let result = self.crc_check(); - // Unwrap fail = Radio RX Buffer is missing (may be due to receive client not replacing in receive(...) method, - // or some instance in driver taking buffer without properly replacing). + // Unwrap fail = Radio RX Buffer is missing (may be due to + // receive client not replacing in receive(...) method, or + // some instance in driver taking buffer without properly + // replacing). let rbuf = self.rx_buf.take().unwrap(); // data buffer format: | PSDU OFFSET | FRAME | LQI | // length of received data transferred to buffer (including PSDU) let data_len = rbuf[MIMIC_PSDU_OFFSET as usize] as usize; - // Check if the received packet has a valid CRC and as a last resort - // confirm that the received packet length field is in compliance - // with the maximum packet length. If not, drop the packet. + // Check if the received packet has a valid CRC and as a + // last resort confirm that the received packet length field + // is in compliance with the maximum packet length. If not, + // drop the packet. if !result.is_ok() || data_len >= MAX_FRAME_SIZE + PSDU_OFFSET { self.rx_buf.replace(rbuf); return } - // lqi is found just after the data received + // LQI is found just after the data received. let lqi = rbuf[data_len]; let frame_len = data_len - radio::MFR_SIZE; @@ -839,7 +845,8 @@ impl<'a> Radio<'a> { // And because the length field is directly read from the packet // We need to add 2 to length to get the total length - // 6th bit in 2nd byte of received 15.4 packet determines if sender requested ACK + // 6th bit in 2nd byte of received 15.4 packet determines if + // sender requested ACK if rbuf[2] & ACK_FLAG != 0 && result.is_ok() { self.ack_buf .take() @@ -850,16 +857,20 @@ impl<'a> Radio<'a> { // 4th byte of received packet is 15.4 sequence counter let sequence_counter = rbuf[4]; - // The frame control field is hardcoded for now; this is the - // only possible type of ACK currently supported so it is reasonable to hardcode this + // The frame control field is hardcoded for now; + // this is the only possible type of ACK + // currently supported so it is reasonable to + // hardcode this. let ack_buf_send = [2, 0, sequence_counter]; - // Copy constructed packet into ACK buffer; first two bytes - // left empty for MIMIC_PSDU_OFFSET (see other comments) + // Copy constructed packet into ACK buffer; + // first two bytes left empty for + // MIMIC_PSDU_OFFSET (see other comments). ack_buf[2..5].copy_from_slice(&ack_buf_send); self.rx_buf.replace(rbuf); - // If the transmit function fails, return the buffer and return an error + // If the transmit function fails, return the + // buffer and return an error. if let Err((_, ret_buf)) = self.transmit(ack_buf, 3) { self.ack_buf.replace(ret_buf); Err(ErrorCode::FAIL) @@ -868,9 +879,10 @@ impl<'a> Radio<'a> { } }) .unwrap_or_else(|_| { - // The ACK was not sent; we do not need to drop the packet, but print msg - // for debugging purposes, notfiy receive client of packet, and reset radio - // to receiving + // The ACK was not sent; we do not need to drop + // the packet, but print msg for debugging + // purposes, notifiy receive client of packet, + // and reset radio to receiving. self.rx_client.map(|client| { start_task = true; client.receive( @@ -887,9 +899,9 @@ impl<'a> Radio<'a> { ); }); } else { - // Packet received that does not require an ACK - // Pass received packet to client and return radio to general - // receiving state to listen for new packets + // Packet received that does not require an ACK. Pass + // received packet to client and return radio to general + // receiving state to listen for new packets. self.rx_client.map(|client| { start_task = true; client.receive( @@ -904,23 +916,26 @@ impl<'a> Radio<'a> { } } RadioState::TX => { - //////////////////////////////////////////////////////////////////////// - // NOTE: This state machine assumes that the DISABLED_RXEN, CCAIDLE_TXEN, - // RXREADY_CCASTART shortcuts are enabled at this point in time. If - // the READY_START shortcut is not enabled, the state machine/driver - // will likely exhibit undefined behavior. - //////////////////////////////////////////////////////////////////////// - - // Handle Event_ready interrupt. The TX path performs both a TX ramp up and - // an RX ramp up. This means that there are two potential cases we must handle. - // The ready event due to the Rx Ramp up shortcuts to the CCASTART while the ready - // event due to the Tx ramp up requires we issue a start task in response to progress - // the state machine. + //////////////////////////////////////////////////////////////// + // NOTE: This state machine assumes that the DISABLED_RXEN, + // CCAIDLE_TXEN, RXREADY_CCASTART shortcuts are enabled at this + // point in time. If the READY_START shortcut is not enabled, + // the state machine/driver will likely exhibit undefined + // behavior. + //////////////////////////////////////////////////////////////// + + // Handle Event_ready interrupt. The TX path performs both a TX + // ramp up and an RX ramp up. This means that there are two + // potential cases we must handle. The ready event due to the Rx + // Ramp up shortcuts to the CCASTART while the ready event due + // to the Tx ramp up requires we issue a start task in response + // to progress the state machine. if self.registers.event_ready.is_set(Event::READY) { // In both cases, we must clear event self.registers.event_ready.write(Event::READY::CLEAR); - // Ready event from Tx ramp up will be in radio internal TXIDLE state + // Ready event from Tx ramp up will be in radio internal + // TXIDLE state if self.registers.state.get() == nrf52::constants::RADIO_STATE_TXIDLE { start_task = true; } @@ -948,14 +963,17 @@ impl<'a> Radio<'a> { ), ); } else { - // We have exceeded the IEEE802154_MAX_POLLING_ATTEMPTS and should - // fail the transmission/return buffer to sending client + // We have exceeded the IEEE802154_MAX_POLLING_ATTEMPTS + // and should fail the transmission/return buffer to + // sending client. let result = Err(ErrorCode::BUSY); - //TODO: Acked is hardcoded to always return false; add support to receive tx ACK + //TODO: Acked is hardcoded to always return false; add + //support to receive tx ACK. self.tx_client.map(|client| { - // Unwrap fail = TX Buffer is missing and was mistakenly not replaced after - // completion of set_dma_ptr(...) + // Unwrap fail = TX Buffer is missing and was + // mistakenly not replaced after completion of + // set_dma_ptr(...) let tbuf = self.tx_buf.take().unwrap(); client.send_done(tbuf, false, result); }); @@ -963,15 +981,17 @@ impl<'a> Radio<'a> { } } - // End event received; The TX is now finished and we need to notify the sending client + // End event received; The TX is now finished and we need to + // notify the sending client. if self.registers.event_end.is_set(Event::READY) { self.registers.event_end.write(Event::READY::CLEAR); let result = Ok(()); - //TODO: Acked is hardcoded to always return false; add support to receive tx ACK + // TODO: Acked is hardcoded to always return false; add + // support to receive tx ACK. self.tx_client.map(|client| { - // Unwrap fail = TX Buffer is missing and was mistakenly not replaced after - // completion of set_dma_ptr(...) + // Unwrap fail = TX Buffer is missing and was mistakenly + // not replaced after completion of set_dma_ptr(...) let tbuf = self.tx_buf.take().unwrap(); client.send_done(tbuf, false, result); }); @@ -979,11 +999,12 @@ impl<'a> Radio<'a> { } } RadioState::ACK => { - //////////////////////////////////////////////////////////////////////////// - // NOTE: This state machine assumes that the READY_START shortcut is enabled - // at this point in time. If the READY_START shortcut is not enabled, the - // state machine/driver will likely exhibit undefined behavior. - //////////////////////////////////////////////////////////////////////////// + //////////////////////////////////////////////////////////////// + // NOTE: This state machine assumes that the READY_START + // shortcut is enabled at this point in time. If the READY_START + // shortcut is not enabled, the state machine/driver will likely + // exhibit undefined behavior. + //////////////////////////////////////////////////////////////// // Since READY_START shortcut enabled, always clear READY event self.registers.event_ready.write(Event::READY::CLEAR); @@ -992,8 +1013,8 @@ impl<'a> Radio<'a> { if self.registers.event_end.is_set(Event::READY) { self.registers.event_end.write(Event::READY::CLEAR); - // Unwrap fail = TX Buffer is missing and was mistakenly not replaced after - // completion of set_dma_ptr(...) + // Unwrap fail = TX Buffer is missing and was mistakenly not + // replaced after completion of set_dma_ptr(...) let tbuf = self.tx_buf.take().unwrap(); // We must replace the ACK buffer that was passed to tx_buf @@ -1002,13 +1023,18 @@ impl<'a> Radio<'a> { // Reset radio to proper receiving state rx_init = true; - // Notify receive client of packet. The client.receive(...) function has been moved - // here to optimize the time taken to send an ACK. If the receive function was called in the - // RX state handler, there is a non deterministic time required to complete the function as it may - // be passed up the entirety of the networking stack (leading to ACK timeout being exceeded). + // Notify receive client of packet. The client.receive(...) + // function has been moved here to optimize the time taken + // to send an ACK. If the receive function was called in the + // RX state handler, there is a non deterministic time + // required to complete the function as it may be passed up + // the entirety of the networking stack (leading to ACK + // timeout being exceeded). self.rx_client.map(|client| { - // Unwrap fail = Radio RX Buffer is missing (may be due to receive client not replacing in receive(...) method, - // or some instance in driver taking buffer without properly replacing). + // Unwrap fail = Radio RX Buffer is missing (may be due + // to receive client not replacing in receive(...) + // method, or some instance in driver taking buffer + // without properly replacing). let rbuf = self.rx_buf.take().unwrap(); let result = self.crc_check(); @@ -1017,9 +1043,10 @@ impl<'a> Radio<'a> { // length of received data transferred to buffer (including PSDU) let data_len = rbuf[MIMIC_PSDU_OFFSET as usize] as usize; - // Check if the received packet has a valid CRC and as a last resort - // confirm that the received packet length field is in compliance - // with the maximum packet length. If not, drop the packet. + // Check if the received packet has a valid CRC and as a + // last resort confirm that the received packet length + // field is in compliance with the maximum packet + // length. If not, drop the packet. if !result.is_ok() || data_len >= MAX_FRAME_SIZE + PSDU_OFFSET { self.rx_buf.replace(rbuf); return @@ -1045,10 +1072,12 @@ impl<'a> Radio<'a> { } } - // Enabling hardware shortcuts allows for a much faster operation. However, this can also lead - // to race conditions and strange edge cases. Namely, if a task_start or rx_en is set while interrupts are disabled, - // the event_end interrupt can be "missed" and the interrupt handler will not be called. If the event is missed, the state - // machine is unable to progress and the driver enters a deadlock + // Enabling hardware shortcuts allows for a much faster operation. + // However, this can also lead to race conditions and strange edge + // cases. Namely, if a task_start or rx_en is set while interrupts are + // disabled, the event_end interrupt can be "missed" and the interrupt + // handler will not be called. If the event is missed, the state machine + // is unable to progress and the driver enters a deadlock. self.enable_interrupts(); if rx_init { self.rx(); @@ -1349,9 +1378,10 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { buf[MIMIC_PSDU_OFFSET as usize] = (frame_len + radio::MFR_SIZE) as u8; - // The tx_buf does not possess static memory. This buffer only temporarily holds - // a reference to another buffer passed as a function argument. The tx_buf holds - // ownership of this buffer until it is returned through the send_done(...) function + // The tx_buf does not possess static memory. This buffer only + // temporarily holds a reference to another buffer passed as a function + // argument. The tx_buf holds ownership of this buffer until it is + // returned through the send_done(...) function. self.tx_buf.replace(self.set_dma_ptr(buf)); // The transmit function handles sending both ACK and standard packets @@ -1362,9 +1392,11 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { self.state.set(RadioState::TX); // Instruct radio hardware to automatically progress from: - // --RXDISABLE to RXRU state upon receipt of internal disabled event - // --RXIDLE to RX state upon receipt of ready event and radio ramp up completed, begin CCA backoff - // --RX to TXRU state upon internal receipt CCA completion event (clear to begin transmitting) + // - RXDISABLE to RXRU state upon receipt of internal disabled event + // - RXIDLE to RX state upon receipt of ready event and radio ramp + // up completed, begin CCA backoff + // - RX to TXRU state upon internal receipt CCA completion event + // (clear to begin transmitting) self.registers.shorts.write( Shortcut::DISABLED_RXEN::SET + Shortcut::RXREADY_CCASTART::SET From 2266fbd5eea80b8b60faccd886534b27dd4f40f1 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 15:04:44 -0400 Subject: [PATCH 07/11] kernel: hil: 154: add useful constants and doc --- kernel/src/hil/radio.rs | 66 +++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/kernel/src/hil/radio.rs b/kernel/src/hil/radio.rs index 25e94b9c46..0ec44ca518 100644 --- a/kernel/src/hil/radio.rs +++ b/kernel/src/hil/radio.rs @@ -36,37 +36,53 @@ pub trait PowerClient { fn changed(&self, on: bool); } -/// These constants are used for interacting with the SPI buffer, which contains -/// a 1-byte SPI command, a 1-byte PHY header, and then the 802.15.4 frame. In -/// theory, the number of extra bytes in front of the frame can depend on the -/// particular method used to communicate with the radio, but we leave this as a -/// constant in this generic trait for now. -/// -/// Furthermore, the minimum MHR size assumes that -/// -/// - The source PAN ID is omitted -/// - There is no auxiliary security header -/// - There are no IEs -/// -/// ```text -/// +---------+-----+-----+-------------+-----+ -/// | SPI com | PHR | MHR | MAC payload | MFR | -/// +---------+-----+-----+-------------+-----+ -/// \______ Static buffer rx/txed to SPI _____/ -/// \__ PSDU / frame length __/ -/// \___ 2 bytes ___/ -/// ``` - -pub const MIN_MHR_SIZE: usize = 9; +// These constants are used for interacting with the SPI buffer, which contains +// a 1-byte SPI command, a 1-byte PHY header, and then the 802.15.4 frame. In +// theory, the number of extra bytes in front of the frame can depend on the +// particular method used to communicate with the radio, but we leave this as a +// constant in this generic trait for now. +// +// Furthermore, the minimum MHR size assumes that +// +// - The source PAN ID is omitted +// - There is no auxiliary security header +// - There are no IEs +// +// ```text +// +---------+-----+--------+-------------+-----+-----+ +// | SPI com | PHR | MHR | MAC payload | MFR | LQI | +// +---------+-----+--------+-------------+-----+-----+ +// \______ Static buffer for implementation __________/ +// \_________ PSDU _____________/ +// \___ 2 bytes ___/ \1byte/ +// ``` + +/// Length of the physical layer header. This is the Frame length field. +pub const PHR_SIZE: usize = 1; +/// Length of the Frame Control field in the MAC header. +pub const MHR_FC_SIZE: usize = 2; +/// Length of the MAC footer. Contains the CRC. pub const MFR_SIZE: usize = 2; +/// Maximum length of a MAC frame. pub const MAX_MTU: usize = 127; -pub const MIN_FRAME_SIZE: usize = MIN_MHR_SIZE + MFR_SIZE; +/// Minimum length of the MAC frame (except for acknowledgements). This is +/// explained in Table 21 of the specification. +pub const MIN_FRAME_SIZE: usize = 9; +/// Maximum length of a MAC frame. pub const MAX_FRAME_SIZE: usize = MAX_MTU; +/// Location in the buffer of the physical layer header. This is the location of +/// the Frame length byte. +pub const PHR_OFFSET: usize = 1; +/// Location in the buffer of the PSDU. This is equivalent to the start of the +/// MAC payload. pub const PSDU_OFFSET: usize = 2; +/// Length of the reserved space in the buffer for a SPI command. +pub const SPI_HEADER_SIZE: usize = 1; +/// Length of the reserved space in the buffer for LQI information. pub const LQI_SIZE: usize = 1; -pub const MAX_BUF_SIZE: usize = PSDU_OFFSET + MAX_MTU + LQI_SIZE; -pub const MIN_PAYLOAD_OFFSET: usize = PSDU_OFFSET + MIN_MHR_SIZE; +/// Required buffer size for implementations of this HIL. +pub const MAX_BUF_SIZE: usize = SPI_HEADER_SIZE + PHR_SIZE + MAX_MTU + LQI_SIZE; pub trait Radio<'a>: RadioConfig<'a> + RadioData<'a> {} // Provide blanket implementations for trait group From 4d64cafa9a4647b184779ff23802ad2cb863ecc7 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 15:05:23 -0400 Subject: [PATCH 08/11] nrf52840: 154: update tx/rx logic --- chips/nrf52840/src/ieee802154_radio.rs | 185 +++++++++++-------------- 1 file changed, 83 insertions(+), 102 deletions(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index 0918b81095..ce8f5f90cc 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -66,7 +66,7 @@ use crate::timer::TimerAlarm; use core::cell::Cell; use kernel::deferred_call::{DeferredCall, DeferredCallClient}; -use kernel::hil::radio::{self, PowerClient, RadioChannel, RadioData, MAX_FRAME_SIZE, PSDU_OFFSET}; +use kernel::hil::radio::{self, PowerClient, RadioChannel, RadioData}; use kernel::hil::time::{Alarm, AlarmClient, Time}; use kernel::utilities::cells::{OptionalCell, TakeCell}; use kernel::utilities::registers::interfaces::{Readable, Writeable}; @@ -87,20 +87,15 @@ pub const IEEE802154_ACK_TIME: usize = 512; //microseconds = 32 symbols pub const IEEE802154_MAX_POLLING_ATTEMPTS: u8 = 4; pub const IEEE802154_MIN_BE: u8 = 3; pub const IEEE802154_MAX_BE: u8 = 5; -pub const RAM_LEN_BITS: usize = 8; -pub const RAM_S1_BITS: usize = 0; -pub const PREBUF_LEN_BYTES: usize = 2; pub const ACK_BUF_SIZE: usize = 6; -// artifact of entanglement with rf233 implementation, mac layer -// places packet data starting PSDU_OFFSET=2 bytes after start of -// buffer to make room for 1 byte spi header required when communicating -// with rf233 over SPI. nrf radio does not need this header, so we -// have to pretend the frame buffer starts 1 byte later than the -// frame passed by the mac layer. We can't just drop the byte from -// the buffer because then it would be lost forever when we tried -// to return the frame buffer. -const MIMIC_PSDU_OFFSET: u32 = 1; +/// Where the 15.4 packet from the radio is stored in the buffer. The HIL +/// reserves one byte at the beginning of the buffer for use by the +/// capsule/hardware. We have no use for this, but the upper layers expect it so +/// we skip over it. +// We can't just drop the byte from the buffer because then it would be lost +// forever when we tried to return the frame buffer. +const BUF_PREFIX_SIZE: u32 = 1; #[repr(C)] struct RadioRegisters { @@ -657,11 +652,17 @@ register_bitfields! [u32, ] ]; -#[derive(Debug, Clone, Copy)] +/// Operating mode of the radio. +#[derive(Debug, Clone, Copy, PartialEq)] enum RadioState { + /// Radio peripheral is off. OFF, + /// Currently transmitting a packet. TX, + /// Default state when radio is on. Radio is configured to be in RX mode + /// when the radio is turned on but not transmitting. RX, + /// Transmitting an acknowledgement packet. ACK, } @@ -670,7 +671,10 @@ enum RadioState { /// perform when we get the deferred call callback. #[derive(Debug, Clone, Copy)] enum DeferredOperation { + /// Waiting to notify that the configuration operation is complete. ConfigClientCallback, + /// Waiting to notify that the power state of the radio changed (ie it + /// turned on or off). PowerClientCallback, } @@ -775,7 +779,7 @@ impl<'a> Radio<'a> { fn set_dma_ptr(&self, buffer: &'static mut [u8]) -> &'static mut [u8] { self.registers .packetptr - .set(buffer.as_ptr() as u32 + MIMIC_PSDU_OFFSET); + .set(buffer.as_ptr() as u32 + BUF_PREFIX_SIZE); buffer } @@ -801,7 +805,10 @@ impl<'a> Radio<'a> { match self.state.get() { // It should not be possible to receive an interrupt while the // tracked radio state is OFF. - RadioState::OFF => kernel::debug!("[ERROR]--15.4 state machine diverged from expected behavior. Received interrupt while off"), + RadioState::OFF => { + kernel::debug!("[ERROR]--15.4 state machine"); + kernel::debug!("Received interrupt while off"); + } RadioState::RX => { //////////////////////////////////////////////////////////////// // NOTE: This state machine assumes that the READY_START @@ -816,60 +823,68 @@ impl<'a> Radio<'a> { // Completed receiving a packet, now determine if we need to send ACK if self.registers.event_end.is_set(Event::READY) { self.registers.event_end.write(Event::READY::CLEAR); - let result = self.crc_check(); + let crc = self.crc_check(); // Unwrap fail = Radio RX Buffer is missing (may be due to // receive client not replacing in receive(...) method, or - // some instance in driver taking buffer without properly + // some instance in driver taking buffer without properly // replacing). let rbuf = self.rx_buf.take().unwrap(); - // data buffer format: | PSDU OFFSET | FRAME | LQI | - // length of received data transferred to buffer (including PSDU) - let data_len = rbuf[MIMIC_PSDU_OFFSET as usize] as usize; - - // Check if the received packet has a valid CRC and as a - // last resort confirm that the received packet length field - // is in compliance with the maximum packet length. If not, - // drop the packet. - if !result.is_ok() || data_len >= MAX_FRAME_SIZE + PSDU_OFFSET { - self.rx_buf.replace(rbuf); - return - } + // Data buffer format: | PREFIX | PHR | PSDU | LQI | + // + // Retrieve the length of the PSDU (actual frame). The frame + // length is only 7 bits, but of course the field is a byte. + // The nRF52840 product specification says this about the + // PHR byte (Version 1.8, section 6.20.12.1): + // + // > The most significant bit is reserved and is set to zero + // > for frames that are standard compliant. The radio + // > module will report all eight bits and it can + // > potentially be used to carry some information. + // + // We are not using that for any information so we just + // force it to zero. This ensures that `data_len` will not + // be longer than our buffer. + let data_len = (rbuf[radio::PHR_OFFSET] & 0x7F) as usize; // LQI is found just after the data received. let lqi = rbuf[data_len]; + // We drop the CRC bytes (the MFR) from our frame. let frame_len = data_len - radio::MFR_SIZE; - // Length is: S0 (0 Byte) + Length (1 Byte) + S1 (0 Bytes) + Payload - // And because the length field is directly read from the packet - // We need to add 2 to length to get the total length - // 6th bit in 2nd byte of received 15.4 packet determines if - // sender requested ACK - if rbuf[2] & ACK_FLAG != 0 && result.is_ok() { + // 6th bit in the first byte of the MAC header determines if + // sender requested ACK. If so send ACK first before handing + // packet reception. This optimizes the time taken to send + // an ACK. If we call the receive function here, there is a + // non deterministic time required to complete the function + // as it may be passed up the entirety of the networking + // stack (leading to ACK timeout being exceeded). + if rbuf[radio::PSDU_OFFSET] & ACK_FLAG != 0 && crc.is_ok() { self.ack_buf .take() .map_or(Err(ErrorCode::NOMEM), |ack_buf| { // Entered ACK state // self.state.set(RadioState::ACK); - // 4th byte of received packet is 15.4 sequence counter - let sequence_counter = rbuf[4]; + // 4th byte of received packet is the 15.4 + // sequence number. + let sequence_number = rbuf[radio::PSDU_OFFSET + radio::MHR_FC_SIZE]; // The frame control field is hardcoded for now; // this is the only possible type of ACK // currently supported so it is reasonable to // hardcode this. - let ack_buf_send = [2, 0, sequence_counter]; + ack_buf[radio::PSDU_OFFSET] = 2; + ack_buf[radio::PSDU_OFFSET + 1] = 0; + ack_buf[radio::PSDU_OFFSET + radio::MHR_FC_SIZE] = sequence_number; - // Copy constructed packet into ACK buffer; - // first two bytes left empty for - // MIMIC_PSDU_OFFSET (see other comments). - ack_buf[2..5].copy_from_slice(&ack_buf_send); + // Ensure we replace our RX buffer for the time + // being. self.rx_buf.replace(rbuf); - // If the transmit function fails, return the + // If the transmit function fails, replace the // buffer and return an error. if let Err((_, ret_buf)) = self.transmit(ack_buf, 3) { self.ack_buf.replace(ret_buf); @@ -878,10 +893,10 @@ impl<'a> Radio<'a> { Ok(()) } }) - .unwrap_or_else(|_| { + .unwrap_or_else(|err| { // The ACK was not sent; we do not need to drop // the packet, but print msg for debugging - // purposes, notifiy receive client of packet, + // purposes, notify receive client of packet, // and reset radio to receiving. self.rx_client.map(|client| { start_task = true; @@ -889,8 +904,8 @@ impl<'a> Radio<'a> { self.rx_buf.take().unwrap(), frame_len, lqi, - self.registers.crcstatus.get() == 1, - result, + crc.is_ok(), + Err(err), ); }); @@ -904,13 +919,7 @@ impl<'a> Radio<'a> { // receiving state to listen for new packets. self.rx_client.map(|client| { start_task = true; - client.receive( - rbuf, - frame_len, - lqi, - self.registers.crcstatus.get() == 1, - result, - ); + client.receive(rbuf, frame_len, lqi, crc.is_ok(), Ok(())); }); } } @@ -945,10 +954,9 @@ impl<'a> Radio<'a> { if self.registers.event_ccabusy.is_set(Event::READY) { self.registers.event_ccabusy.write(Event::READY::CLEAR); - //need to back off for a period of time outlined - //in the IEEE 802.15.4 standard (see Figure 69 in - //section 7.5.1.4 The CSMA-CA algorithm of the - //standard). + // Need to back off for a period of time outlined in the + // IEEE 802.15.4 standard (see Figure 69 in section 7.5.1.4 + // The CSMA-CA algorithm of the standard). if self.cca_count.get() < IEEE802154_MAX_POLLING_ATTEMPTS { self.cca_count.set(self.cca_count.get() + 1); self.cca_be.set(self.cca_be.get() + 1); @@ -968,8 +976,6 @@ impl<'a> Radio<'a> { // sending client. let result = Err(ErrorCode::BUSY); - //TODO: Acked is hardcoded to always return false; add - //support to receive tx ACK. self.tx_client.map(|client| { // Unwrap fail = TX Buffer is missing and was // mistakenly not replaced after completion of @@ -1023,13 +1029,7 @@ impl<'a> Radio<'a> { // Reset radio to proper receiving state rx_init = true; - // Notify receive client of packet. The client.receive(...) - // function has been moved here to optimize the time taken - // to send an ACK. If the receive function was called in the - // RX state handler, there is a non deterministic time - // required to complete the function as it may be passed up - // the entirety of the networking stack (leading to ACK - // timeout being exceeded). + // Notify receive client of packet that triggered the ACK. self.rx_client.map(|client| { // Unwrap fail = Radio RX Buffer is missing (may be due // to receive client not replacing in receive(...) @@ -1037,36 +1037,16 @@ impl<'a> Radio<'a> { // without properly replacing). let rbuf = self.rx_buf.take().unwrap(); - let result = self.crc_check(); - - // data buffer format: | PSDU OFFSET | FRAME | LQI | - // length of received data transferred to buffer (including PSDU) - let data_len = rbuf[MIMIC_PSDU_OFFSET as usize] as usize; - - // Check if the received packet has a valid CRC and as a - // last resort confirm that the received packet length - // field is in compliance with the maximum packet - // length. If not, drop the packet. - if !result.is_ok() || data_len >= MAX_FRAME_SIZE + PSDU_OFFSET { - self.rx_buf.replace(rbuf); - return - } - - // lqi is found just after the data received + // Data buffer format: | PREFIX | PHR | PSDU | LQI | + // + // See the RX case above for how these values are set. + let data_len = (rbuf[radio::PHR_OFFSET] & 0x7F) as usize; let lqi = rbuf[data_len]; - let frame_len = data_len - radio::MFR_SIZE; - // Length is: S0 (0 Byte) + Length (1 Byte) + S1 (0 Bytes) + Payload - // And because the length field is directly read from the packet - // We need to add 2 to length to get the total length - - client.receive( - rbuf, - frame_len, - lqi, - self.registers.crcstatus.get() == 1, - result, - ); + + // We know the CRC passed because otherwise we would not + // have transmitted an ACK. + client.receive(rbuf, frame_len, lqi, true, Ok(())); }); } } @@ -1250,9 +1230,9 @@ impl<'a> kernel::hil::radio::RadioConfig<'a> for Radio<'a> { self.radio_is_on() } - // Previous driver implementation // fn busy(&self) -> bool { - false + // `tx_buf` is only occupied when a transmission is underway. + self.tx_buf.is_some() } fn set_config_client(&self, client: &'a dyn radio::ConfigClient) { @@ -1364,19 +1344,20 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { frame_len: usize, ) -> Result<(), (ErrorCode, &'static mut [u8])> { if self.tx_buf.is_some() { - // tx_buf TakeCell is only occupied when a transmission is underway. This + // tx_buf is only occupied when a transmission is underway. This // check insures we do not interrupt an ongoing transmission. return Err((ErrorCode::BUSY, buf)); - } else if radio::MFR_SIZE + frame_len >= buf.len() { + } else if buf.len() < radio::PSDU_OFFSET + frame_len + radio::MFR_SIZE { // Not enough room for CRC return Err((ErrorCode::SIZE, buf)); } - if let RadioState::OFF = self.state.get() { + if self.state.get() == RadioState::OFF { self.radio_initialize(); } - buf[MIMIC_PSDU_OFFSET as usize] = (frame_len + radio::MFR_SIZE) as u8; + // Insert the PHR which is the PDSU length. + buf[radio::PHR_OFFSET] = (frame_len + radio::MFR_SIZE) as u8; // The tx_buf does not possess static memory. This buffer only // temporarily holds a reference to another buffer passed as a function From 37f20910ee5b7135d6dacfc72f2e442c8f64aedb Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Fri, 17 May 2024 18:09:03 -0400 Subject: [PATCH 09/11] nrf52840: 154: return error on tx if radio off This matches the HIL and the rf233 --- chips/nrf52840/src/ieee802154_radio.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index ce8f5f90cc..62ffc8ccb8 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -1343,7 +1343,9 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { buf: &'static mut [u8], frame_len: usize, ) -> Result<(), (ErrorCode, &'static mut [u8])> { - if self.tx_buf.is_some() { + if self.state.get() == RadioState::OFF { + return Err((ErrorCode::OFF, buf)); + } else if self.tx_buf.is_some() { // tx_buf is only occupied when a transmission is underway. This // check insures we do not interrupt an ongoing transmission. return Err((ErrorCode::BUSY, buf)); @@ -1352,10 +1354,6 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { return Err((ErrorCode::SIZE, buf)); } - if self.state.get() == RadioState::OFF { - self.radio_initialize(); - } - // Insert the PHR which is the PDSU length. buf[radio::PHR_OFFSET] = (frame_len + radio::MFR_SIZE) as u8; From 958a6a18f24c7b61e2087399d4a9c6439bb87889 Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Thu, 23 May 2024 11:31:31 -0400 Subject: [PATCH 10/11] nrf52840: ieee802154: better error check on busy Co-authored-by: Tyler Potyondy <77175673+tyler-potyondy@users.noreply.github.com> --- chips/nrf52840/src/ieee802154_radio.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index 62ffc8ccb8..7dbf3c61bd 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -66,7 +66,7 @@ use crate::timer::TimerAlarm; use core::cell::Cell; use kernel::deferred_call::{DeferredCall, DeferredCallClient}; -use kernel::hil::radio::{self, PowerClient, RadioChannel, RadioData}; +use kernel::hil::radio::{self, PowerClient, RadioChannel, RadioConfig, RadioData}; use kernel::hil::time::{Alarm, AlarmClient, Time}; use kernel::utilities::cells::{OptionalCell, TakeCell}; use kernel::utilities::registers::interfaces::{Readable, Writeable}; @@ -1345,9 +1345,7 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { ) -> Result<(), (ErrorCode, &'static mut [u8])> { if self.state.get() == RadioState::OFF { return Err((ErrorCode::OFF, buf)); - } else if self.tx_buf.is_some() { - // tx_buf is only occupied when a transmission is underway. This - // check insures we do not interrupt an ongoing transmission. + } else if self.busy() { return Err((ErrorCode::BUSY, buf)); } else if buf.len() < radio::PSDU_OFFSET + frame_len + radio::MFR_SIZE { // Not enough room for CRC From 9479df27f5d290e26c49ab63d3d09fadc10684bf Mon Sep 17 00:00:00 2001 From: Brad Campbell Date: Thu, 23 May 2024 16:54:41 -0400 Subject: [PATCH 11/11] nrf52840: ieee802154: clarify buf len comment --- chips/nrf52840/src/ieee802154_radio.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chips/nrf52840/src/ieee802154_radio.rs b/chips/nrf52840/src/ieee802154_radio.rs index 7dbf3c61bd..3e88465b31 100644 --- a/chips/nrf52840/src/ieee802154_radio.rs +++ b/chips/nrf52840/src/ieee802154_radio.rs @@ -1348,7 +1348,7 @@ impl<'a> kernel::hil::radio::RadioData<'a> for Radio<'a> { } else if self.busy() { return Err((ErrorCode::BUSY, buf)); } else if buf.len() < radio::PSDU_OFFSET + frame_len + radio::MFR_SIZE { - // Not enough room for CRC + // Not enough room for CRC or PHR or reserved byte return Err((ErrorCode::SIZE, buf)); }