initial ieee802.15.4 API by japaric · Pull Request #143 · nrf-rs/nrf-hal

japaric · 2020-05-05T17:09:20Z

what the title says

this PR adds an ieee802154::Radio API that wraps the RADIO peripheral and puts it in IEEE 802.15.4 mode.

I haven't checked the silicon erratas of (or tested) other chips (the code contains one workaround for (nRF52840 specific?) silicon errata) so for now I have put the API directly under nrf52840-hal. But I guess the API should also work on other nRF52 devices -- I think the nRF51 chips do not have an 802.15.4 mode? And I don't know about other chip families.

The implementation is not super IEEE compliant. It's missing a backoff delay mechanism when doing CCA (Clear Channel Assessment) before transmitting packets and it's also missing IFS (inter-frame spacing) between consecutive packets (the hardware has some built-in support for this but the PR doesn't expose it). But it is sufficient to send packets around and receive them on other nRF52s running the same implementation, though it would be good to check this against some third party implementation like the DWM1001 or the MRF24J40.

There are some TODOs here and there in terms of additional things the API could expose like changing the transmit power. I'm not sure if all those need to be added before a review though. They could be added as follow up PRs too.

TODO items

API to change TX power (the default value doesn't go very far, like tens of centimer or so in a noisy 2.4 GHz environment)
API to change the start of frame delimiter (the default value matches the IEEE spec)
API to extract the LQI (Link Quality Indicator)
verify that CRC are being checked. May need to send a malformed packet from a different device (MRF24J40?)
"The total usable payload (PSDU) is 127 octets, but when CRC is being used, this is reduced to 125 octets of usable payload." -- adjust MAX_LEN accordingly

thalesfragoso · 2020-05-05T17:21:48Z

Thanks for this work.

But I guess the API should also work on other nRF52 devices -- I think the nRF51 chips do not have an 802.15.4 mode? And I don't know about other chip families.

Both the nRF52810 and nRF52832 don't support 802.15.4 mode. 51 probably doesn't support it either.

Yatekii · 2020-05-05T17:48:15Z

Cool stuff! I need to test this on my boards =)

The 52840, 52811 52833 and 5340 support 802.15.4.

Are you aware of the https://crates.io/crates/ieee802154 crate? I think it is higher level (MAC) layer, but could be useful to test this and not reinvent the wheel.

Thanks for putting in the work!

japaric · 2020-05-06T08:24:47Z

Are you aware of the https://crates.io/crates/ieee802154 crate? I think it is higher level (MAC) layer, but could be useful to test this and not reinvent the wheel.

That's the MAC frame. Yes, that's what you should put in the payload of the Packet defined here (which is actually the PHY layer frame) if you want to be IEEE compliant.

Also, yes that would be useful to test this against the MRF24J40, which (iirc) does do HW-level MAC address filtering. Though that may end up being more of a test for the ieee802154 crate itself.

hannobraun · 2020-05-06T08:44:50Z

@japaric

though it would be good to check this against some third party implementation like the DWM1001 or the MRF24J40

The DW1000 uses UWB (ultra-wideband) in the PHY layer, so I don't think it can communicate with the nRF radio (I might be wrong wrong; despite my work with the DW1000/DWM1001, I know very little about radios).

Also, yes that would be useful to test this against the MRF24J40, which (iirc) does do HW-level MAC address filtering. Though that may end up being more of a test for the ieee802154 crate itself.

The DW1000 also does hardware-based address matching and this is used in the driver. To my knowledge, this has never caused any problems with ieee802154.

jonathanpallant · 2020-05-06T08:53:48Z

Yes, the DW1000 is a UWB PHY (so around 500 MHz bandwidth at 3 to 10 GHz depending on channel) and hence incompatible with a 2.4 GHz PHY (around 80 MHz bandwidth at 2.4 GHz) based system.

I wasn't aware the DW1000 had hardware address detection, so that's good to know!

Getting back on topic, Rust 802.15.4 on the nRF52 series is excellent. Great work :)

blueluna · 2020-05-08T10:59:39Z

Nice! Looks better than my old implementation.
https://github.com/blueluna/nrf52840-dk-experiments/tree/master/nrf52-radio-802154
I hope to try it out when I get time.

japaric · 2020-05-11T17:46:14Z

for those not getting notifications on new commits being pushed: I have added APIs to change the TX power and the start of frame delimiter; added a method the get a packet's Link Quality Indicator; and added CRC verification to the Radio.recv method (the method now returns a Result)

not all values in the -40..=8 range are valid configurations

this method lets you insert a delay between failed CCAs to be IEEE spec compliant

hubertmis

This driver looks nice. Based on experience with similar code in C, I've posted multiple suggestions that could be helpful to improve it.

hubertmis · 2020-06-07T05:58:37Z

+}
+
+/// Default Clear Channel Assessment method = Carrier sense
+pub const DEFAULT_CCA: Cca = Cca::CarrierSense;


The CarrierSense mode needs some calibration to work correctly. I would recommend EnergyDetection mode as a default one.

hubertmis · 2020-06-07T06:00:51Z

+/// IEEE 802.15.4 channels
+///
+/// NOTE these are NOT the same as WiFi 2.4 GHz channels
+// TODO it is possible to use non-standard frequencies below 2_400 MHz; should those be exposed too?


This list is correct. To be compliant with IEEE 802.15.4 other frequencies shall not be exposed. The hardware in selected nRF devices is capable of using other frequencies, but it should not be used in 802.15.4 protocol.

hubertmis · 2020-06-07T06:10:01Z

+
+            radio.radio.pcnf1.write(|w| {
+                w.maxlen()
+                    .bits(Packet::MAX_LEN + 2 /* CRC */) // payload length


This line is a little confusing. When we configure hardware, we usually work on PHY layer. Max PSDU length is 127 and it includes CRC. I would change this constant to MAX_PSDU_LEN = 127 and use it here without + 2. It would be unambiguous.

hubertmis · 2020-06-07T06:12:40Z

+    pub fn set_channel(&mut self, channel: Channel) {
+        self.disable();
+
+        // NOTE(unsafe) radio is currently disabled


It is OK to change frequency while radio is enabled. The FREQUENCY register is double buffered and it is read on RXEN or TXEN tasks.
There is no need to call self.disable(); before.

hubertmis · 2020-06-07T06:13:11Z

+
+    /// Changes the Clear Channel Assessment method
+    pub fn set_cca(&mut self, cca: Cca) {
+        self.disable();


Like with frequency it is OK to change CCA mode while radio is enabled.

hubertmis · 2020-06-07T06:44:12Z

+                match state {
+                    STATE_A::DISABLED => return,
+
+                    STATE_A::RXRU | STATE_A::RXIDLE | STATE_A::TXRU | STATE_A::TXIDLE => {


RXIDLE state may indicate ongoing CCA procedure that may result in TX with radio shorts. To be fully safe, following set of tasks should be triggered:
CCASTOP
STOP
DISABLE

hubertmis · 2020-06-07T06:44:54Z

+    }
+
+    /// Moves the radio from any state to the DISABLED state
+    fn disable(&mut self) {


There is a possibility that with RADIO SHORTS enabled this function would work in an unexpected way. I would add at least assertion here to verify that selected shorts are disabled.

hubertmis · 2020-06-07T06:51:12Z

+
+/// An IEEE 802.15.4 packet
+///
+/// This `Packet` is closest to the PPDU (PHY Protocol Data Unit) defined in the IEEE spec. The API


closest to the PPDU is ambiguous. I would state here clearly that this packet is PHR concatenated with PSDU

hubertmis · 2020-06-07T06:56:09Z

+    ///
+    /// NOTE `src` data will be truncated to `MAX_PACKET_SIZE` bytes
+    pub fn copy_from_slice(&mut self, src: &[u8]) {
+        let len = cmp::min(src.len(), Self::MAX_LEN as usize) as u8;


I don't think it is a good idea to silently limit the given slice size. When user of this function wants to transmit 200 bytes packet and calls this function, would get only 125 bytes transmitted and would not be notified that there was a problem. I would change function signature to return Result or add assertion.

hubertmis · 2020-06-07T06:57:24Z

+    ///
+    /// NOTE `len` will be truncated to `MAX_LEN` bytes
+    pub fn set_len(&mut self, len: u8) {
+        let len = cmp::min(len, Self::MAX_LEN);


Again silently truncated. Using such API could be a source of hard to find bugs in the higher layers.

the previous version result in `recv` resuming immediately after issuing a `*send`

japaric · 2020-06-09T09:34:57Z

@hubertmis thanks for the great feedback! (I have seen it just now). I'll incorporate it in the PR in the coming days.

- Packet::set_len: panic instead of silently clapping the value - Packet::copy_from_slice: panic instead of silently truncating - rename some internal constants related to the size of the parts of Packet - doc tweak: Packet is PHR + PSDU - keep Channel enum as it is (removed TODO) - add send_no_cca method - shortcut ccaidle to txen in try_send - simplify transitions to the TXIDLE / RXIDLE state

hubertmis

The driver looks solid. There is place to add more features and optimizations.

I'm not a fan of blocking recv method. But making it asynchronous would need serious refactoring of the driver. For simple applications blocking approach is sufficient.

hubertmis · 2020-06-17T11:29:43Z

@japaric , to verify CRC of generated frames, you can easily use https://github.com/NordicSemiconductor/nRF-Sniffer-for-802.15.4 . Firmware used in this project reports only frames with valid CRC.

Co-authored-by: Hubert Miś <hubert.mis@nordicsemi.no>

ieee802154.rs: add CCA-Energy Detection

hubertmis · 2020-07-25T16:14:12Z

I was thinking about timing out while receiving. Current implementation blocks execution of the calling thread until a frame is received or a timeout fires. It's not good for the system performance.

In drivers in C usually asynchronous approach is implemented: function like recv changes the driver state to 'receive' and ends it execution - it does not wait for actual frame. If the driver receives a frame, it calls a callback asynchronously. But it looks that this asynchronous callback approach is not perfect fit for Rust, because I think it does not go well with futures or programs using async/await.

Recently I've spotted https://github.com/rust-embedded/nb and it looks like a solution to this problem. The user of the driver could call recv() just to enter receive state. Later the user could call a new function like read() that would implement this nb scheme and return a frame if it was received before the function was called, or WouldBlock if the RX buffer is empty. With such API, the calling thread would not be blocked while waiting for frames and futures, async/await, or other models could be used.

What do you think about it?

jonas-schievink · 2021-03-02T18:18:25Z

+pub const DEFAULT_CCA: Cca = Cca::CarrierSense;
+
+/// Default radio channel = Channel 20 (`2_450` MHz)
+pub const DEFAULT_CHANNEL: Channel = Channel::_18; // _17 todo undo; set this to interfere with my wifi


It looks like this needs to be reset to channel 17.

Are the defaults specified somewhere or were they picked arbitrarily?

the FREQUENCY register reset value is 2, which doesn't match to any of the Channel values because they are all multiples of 5.
The channel values (the frequencies) are the standard IEEE frequencies.

changed the default channel to 11 (2405 MHz) which is closer to the reset value of the FREQUENCY register (2402 MHz)

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>

to enforce that the buffer is RAM allocated

jonas-schievink · 2021-03-03T13:37:30Z

Okay, I think this is ready to go. The remaining notes can be addressed later, and an async/non-blocking API can also be added then.

bors r+

bors · 2021-03-03T13:48:34Z

Build succeeded:

ci

jonas-schievink · 2021-03-04T19:04:34Z

I only just noticed that the module was added directly to the nrf52840-hal instead of nrf-hal-common. I've opened #299 to move it (and enable it for the nRF52833 in the process).

initial ieee802154 API

2cba8c7

japaric added 4 commits May 11, 2020 11:52

add set_tx_power

78ce3f8

add set_sfd

6239c27

packet: add LQI accessor

6ebf533

fix effective packet size & verify CRC

443662d

japaric added 3 commits June 5, 2020 12:54

tx: wait for the PHYEND event

403668b

set_txpower: take an enum

e734e63

not all values in the -40..=8 range are valid configurations

add try_send method

15eff9e

this method lets you insert a delay between failed CCAs to be IEEE spec compliant

hubertmis reviewed Jun 7, 2020

View reviewed changes

japaric added 4 commits June 8, 2020 20:02

clear the correct event registers

276ba3d

fix unintended panic in copy_from_slice

bdd9289

recv: include CRC value; more event handling fixes

4c46534

once more event handling

d624e80

the previous version result in `recv` resuming immediately after issuing a `*send`

hubertmis approved these changes Jun 17, 2020

View reviewed changes

Comment thread nrf52840-hal/src/ieee802154.rs Outdated

japaric and others added 5 commits June 17, 2020 14:39

Update nrf52840-hal/src/ieee802154.rs

3ea430f

Co-authored-by: Hubert Miś <hubert.mis@nordicsemi.no>

add recv_timeout

7076639

doc fix: timeout is in microseconds

b4fcf23

ieee802154.rs: implement CCA-ED

cbc008c

Merge pull request #1 from Lotterleben/radio_add_cca_ed

b57ba81

ieee802154.rs: add CCA-Energy Detection

jonas-schievink reviewed Mar 2, 2021

View reviewed changes

japaric and others added 4 commits March 3, 2021 13:45

Apply suggestions from code review

c88f864

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>

phantom field

50588d9

take &mut Packet from the send methods

35a0099

to enforce that the buffer is RAM allocated

set default channel to 11

572af75

bors Bot merged commit 6b299aa into nrf-rs:master Mar 3, 2021

Conversation

japaric commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thalesfragoso commented May 5, 2020

Uh oh!

Yatekii commented May 5, 2020

Uh oh!

japaric commented May 6, 2020

Uh oh!

hannobraun commented May 6, 2020

Uh oh!

jonathanpallant commented May 6, 2020

Uh oh!

blueluna commented May 8, 2020

Uh oh!

japaric commented May 11, 2020

Uh oh!

hubertmis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

japaric commented Jun 9, 2020

Uh oh!

hubertmis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hubertmis commented Jun 17, 2020

Uh oh!

hubertmis commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonas-schievink commented Mar 3, 2021

Uh oh!

bors Bot commented Mar 3, 2021

Uh oh!

jonas-schievink commented Mar 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

japaric commented May 5, 2020 •

edited

Loading

hubertmis commented Jul 25, 2020 •

edited

Loading