Skip to content

nrf802154: Fix IFS timings#11146

Merged
3 commits merged intoRIOT-OS:masterfrom
bergzand:pr/nrf802154/fix_ifs
Mar 15, 2019
Merged

nrf802154: Fix IFS timings#11146
3 commits merged intoRIOT-OS:masterfrom
bergzand:pr/nrf802154/fix_ifs

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Mar 8, 2019

Contribution description

This PR fixes two issues with the IFS timing in the nrf802154 radio driver.

I've used the following patch to measure timings of the radio IFS timer:

diff --git a/cpu/nrf52/radio/nrf802154/nrf802154.c b/cpu/nrf52/radio/nrf802154/nrf802154.c
index 1a57a8799..9ce87eaa1 100644
--- a/cpu/nrf52/radio/nrf802154/nrf802154.c
+++ b/cpu/nrf52/radio/nrf802154/nrf802154.c
@@ -24,6 +24,7 @@
 #include <errno.h>

 #include "cpu.h"
+#include "periph/gpio.h"
 #include "luid.h"
 #include "mutex.h"

@@ -184,6 +185,7 @@ static void _timer_cb(void *arg, int chan)
 {
     (void)arg;
     (void)chan;
+    gpio_clear(GPIO_PIN(1,9));
     mutex_unlock(&_txlock);
     timer_stop(NRF802154_TIMER);
     timer_clear(NRF802154_TIMER, 0);
@@ -193,6 +195,7 @@ static int _init(netdev_t *dev)
 {
     (void)dev;

+    gpio_init(GPIO_PIN(1, 9), GPIO_OUT);
     int result = timer_init(NRF802154_TIMER, TIMER_FREQ, _timer_cb, NULL);
     assert(result >= 0);
     (void)result;
@@ -415,6 +418,7 @@ void isr_radio(void)
             case RADIO_STATE_STATE_Tx:
             case RADIO_STATE_STATE_TxIdle:
             case RADIO_STATE_STATE_TxDisable:
+                gpio_set(GPIO_PIN(1, 9));
                 timer_start(NRF802154_TIMER);
                 DEBUG("[nrf802154] TX state: %x\n", (uint8_t)NRF_RADIO->STATE);
                 _state |= TX_COMPLETE;

The GPIO is set before the timer is started and cleared in the interrupt.

Using this at the current master results in the following timing (LIFS value):
nrf_orig

measured on the nrf52840 using examples/gnrc_networking with

ping6 fe80::7b67:1860:420c:f43a -c 100  -i 250 -s 200

The payload size of 200B is the most important as it causes the packet to be split into multiple frames.

The correct timing for the LIFS should be:

  • LIFS: 40 symbols (IEEE 802.15.4-2011: macLIFSPeriod – 40 symbols)
  • Symbol rate: 62.5Ksymbol/s ((IEEE 802.15.4-2011 table 66, 2450 DSSS)
    giving us 40 symbols / 62.5Ksymbol/s = 640μs.

This doesn't match the current measured timeout value.

Fix

The fix here is twofold. The first commit is modifies the TIMER_FREQ to match the symbol rate as specified in the ieee 802.15.4 specification. With this a timer tick and an ieee 802.15.4 symbol are equal in duration. The SIFS and LIFS defines then also match their durations in symbols.

The second issue is caused by timer_clear function. It doesn't clear the hardware timer counter, but
is designed to clear the allocation of the channel. The consequence is that the IFS timer here is not set to zero in the callback, but only stopped at the current value. When the timer is started again, it has to
count the full timer range until it matches the timeout value again. With the timer never reset to zero, it doesn't count the LIFS/SIFS duration but the timer range * TIMER_FREQ duration (1024 μs in the intial measurement).

Measuring again with the same setup as above gives:
nrf_fixed

Matching the calculated LIFS value from the spec.

Testing procedure

I won't blame anyone for not reproducing my measurement with their own logic analyzer. Testing that ping with large payloads (multiple L2 frames) still work should IMHO be enough. Ping response timings should decrease slightly (121ms vs 117ms average for 1000B payload).

Issues/PRs references

None

@bergzand bergzand added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers labels Mar 8, 2019
@bergzand bergzand requested review from a user and haukepetersen March 8, 2019 20:11
@bergzand bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 8, 2019
@ghost
Copy link
Copy Markdown

ghost commented Mar 9, 2019

Oh darn, i should have measured this values more carefully... probably calculated wrong LIFS though.

Thank you for the PR, i will test this one as well.
Code looks good, so this one can probably go in right away. 👍

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Mar 9, 2019

Thank you for the PR, i will test this one as well.
Code looks good, so this one can probably go in right away. +1

One thing this PR doesn't accomodate for is compensating the timing for the delay caused by starting the radio again. I'll submit a follow-up asap so that you don't have to build a test setup twice :)

@bergzand
Copy link
Copy Markdown
Member Author

@SemjonKerner I've added a commit that disables the hardware IFS handling in the radio. This reduces the overhead of switching the radio to TX mode to around 40 μs (from 130μs) .

@ghost
Copy link
Copy Markdown

ghost commented Mar 12, 2019

The PR is reasonable, the code looks good, ping still works as expected and I can reproduce all timings with and without this PR.
ACK

@miri64 proxy again?
Nvm, @haukepetersen is requested as reviewer anyway.

@bergzand
Copy link
Copy Markdown
Member Author

With the tests from @SemjonKerner as done in #11138 I was still able to trigger incorrect IFS issues with this code. The issue that caused this was that the timer is initialized running. The new fixup commit inserts a timer_stop() after timer initialization.

@SemjonKerner could you validate the code here with both ping (gnrc_networking) and with txtsnd? It should behave correctly timingwise except for the issue fixed by #11138

@ghost
Copy link
Copy Markdown

ghost commented Mar 15, 2019

Ping and txtsend with small and large frames still work. Timings look good now.
You can squash.

@bergzand bergzand force-pushed the pr/nrf802154/fix_ifs branch from d89db8d to 413d3b0 Compare March 15, 2019 11:45
@bergzand
Copy link
Copy Markdown
Member Author

squashed, thanks for the review!

@ghost
Copy link
Copy Markdown

ghost commented Mar 15, 2019

#11138 came between, please rebase. Sorry for the inconvenience.

ieee802.15.4 specifies 40 symbols as LIFS value and 12 symbols as SIFS
value. Furthermore, the 2.4Ghz DSSS mode has a symbol rate of
62.5Ksymbols/s. To have the LIFS and SIFS in the code match the timings
from the specification, the TIMER_FREQ must match the symbol rate of
62.5Ksymbol/s such that one tick of the timer equals one symbol in time.
The timer_clear function doesn't clear the hardware timer counter, but
is designed to clear the allocation of the channel. The consequence is
that the IFS timer here is not set to zero in the callback, but only
stopped at the current value. When the timer is started again, it has to
count the full timer range until it matches the timeout value again.

This commit fixes this issue by using timer_set instead of
timer_set_absolute. This way the current timer value (when the timer is
stopped) is read and the IFS timeout value is added to the current timer
value.
@bergzand bergzand force-pushed the pr/nrf802154/fix_ifs branch from 413d3b0 to 658fb06 Compare March 15, 2019 12:39
@bergzand
Copy link
Copy Markdown
Member Author

#11138 came between, please rebase. Sorry for the inconvenience.

Rebased!

@ghost
Copy link
Copy Markdown

ghost commented Mar 15, 2019

Great, thanks for your contribution :)

@ghost ghost merged commit 7820a78 into RIOT-OS:master Mar 15, 2019
@bergzand bergzand deleted the pr/nrf802154/fix_ifs branch March 15, 2019 13:02
@danpetry danpetry added this to the Release 2019.04 milestone Mar 26, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants