Fix for Nanostack threading on EFR32 devices #5270

ryankurte · 2017-10-08T02:12:01Z

Description

This PR introduces a threaded wrapper for the SiliconLabs RAIL radio driver in NanostackRfPhyEfr32.cpp resolving the issue raised in #5155.

Status

READY

Tested on the EFR32FG target which will be the subject of another PR.

0xc0170 · 2017-10-09T08:25:38Z

@SeppoTakalo Can you please review

SeppoTakalo

Looks quite OK except I don't like that much of debugging prints from high priority threads.

I could imagine lots of timing related errors when all prints are enabled.

SeppoTakalo · 2017-10-09T08:54:59Z

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp

+                void* handle = (void*) rx_queue[rx_queue_tail];
+
+                RAIL_RxPacketInfo_t* info = (RAIL_RxPacketInfo_t*) memoryPtrFromHandle(handle);
+                tr_debug("rf_thread_loop: rx %d bytes with rssi: %d\n", info->dataLength - 1, info->appendedInfo.rssiLatch);


Quite extensive debugging. Two prints, total of 89 bytes, for every RX event.
In baud rate of 115200 takes about 773 us or more.

Also has potential of dead blocking if normal priority thread have already started to print.

SeppoTakalo · 2017-10-09T08:56:07Z

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp

@@ -22,6 +23,39 @@

 #include "mbed-trace/mbed_trace.h"
 #define  TRACE_GROUP  "SLRF"
+//#define tr_debug(...) printf(__VA_ARGS__)


Don't leave commented code blocks in commits.

SeppoTakalo · 2017-10-09T09:01:05Z

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp

+
+        platform_exit_critical();
+    }
+}


Overall, I would recommend to remove all debugging prints from this function unless it actually means error, or unhandled signal.

ryankurte · 2017-10-09T11:51:07Z

Thanks for the feedback @SeppoTakalo! Can squash it down to one commit now if that's useful to you?

SeppoTakalo · 2017-10-09T11:56:04Z

Yes, please squash because it is one logical change.

0xc0170 · 2017-10-09T11:49:41Z

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp

        waiting_for_ack = false;
+#ifdef MBED_CONF_RTOS_PRESENT
+        //NVIC_DisableIRQ(MacHw_IRQn);


please remove any dead code, it is on multiple lines in this code file

ryankurte · 2017-10-09T12:08:32Z

WOAH squashing that broke something and I seem to have two halves of the implementation there now :-/

Gotta sleep now but will fix / update tomorrow.

mbed-ci · 2017-10-10T01:51:07Z

Build : SUCCESS

Build number : 32
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5270/

Triggering tests

/test mbed-os

ryankurte · 2017-10-10T02:34:52Z

Updated an tested on the EFR32FG (#5271) platform, should be ready to go now unless you have any other suggestions?

mbed-ci · 2017-10-10T02:57:13Z

Test : SUCCESS

Build number : 12
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5270/12

stevew817 · 2017-10-10T15:37:43Z

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp

+            continue;
+        }
+
+        platform_enter_critical();


Isn't this going to create the same issue with nanostack? You're calling nanostack (phy_rx_cb) from within a critical section... Not sure you can do mutex operations in there?

Since what you're protecting is _head and _tail, I'd suggest caching their values into a local variable, to not run into a concurrent-update situation, and keep that critical section as short as possible.

In Mbed OS the Nanostack's platform_enter_critical() is just a mutex.
See here.

So it is a way of protecting Nanostack to call you when you are in a state that you cannot receive TX-start of any of this sort events.

SeppoTakalo · 2017-10-11T07:57:49Z

Actually, on a second look, I feel like the rf_start_cca() should be protected by somehow.
Either using own lock like the Atmel one does https://github.com/ARMmbed/atmel-rf-driver/blob/master/source/NanostackRfPhyAtmel.cpp#L1681 or using platform_enter_critical()

mbed-ci · 2017-10-11T10:40:21Z

Build : SUCCESS

Build number : 109
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5270/

Triggering tests

/test mbed-os

mbed-ci · 2017-10-11T21:38:21Z

Build : SUCCESS

Build number : 135
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5270/

Triggering tests

/test mbed-os

0xc0170 · 2017-10-13T12:30:09Z

@SeppoTakalo Please review the latest update

stevew817 · 2017-10-19T11:54:55Z

@0xc0170 this looks good to go to me.

0xc0170 · 2017-10-19T15:14:38Z

/morph test

mbed-ci · 2017-10-19T17:18:27Z

Test : SUCCESS

Build number : 123
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5270/123

adbridge · 2017-10-20T09:18:50Z

"@SeppoTakalo Please review the latest update"
BUMP

SeppoTakalo

These changes cannot be accepted here.

These are API changes for Trace library. Nothing to do with the topic of the PR anymore.

Trace library is originating from a external repository: https://github.com/ARMmbed/mbed-trace
so no changes should be applied here.

ryankurte · 2017-10-31T10:32:59Z

Ahh, I messed up with my working copy (which has all the changes) and merged that into the wrong branch, then failed at fixing it. Trying to work out how to sort it out now.

update fixed / back up to date, still have to replace those trace calls with macros.

Updated NanostackRfPhyEfr32 with a receive queue. Cleaned up debug messages, re-added to non-threaded calls. Removed debug print override Removed tr_debug override Removed normal-operation prints that could have timing implications if enabled Removed dead NVIC code (and a couple of dead log outputs)

adbridge · 2017-10-31T17:51:52Z

@SeppoTakalo Are you happy with the updates ?

SeppoTakalo · 2017-11-01T08:03:28Z

Looks OK.

0xc0170 · 2017-11-02T08:55:07Z

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp

@@ -254,6 +377,12 @@ static int8_t rf_device_register(void)
        radio_state = RADIO_INITING;
    }

+#ifdef MBED_CONF_RTOS_PRESENT


Does this make sense if this file already defines a thread above (line 69), thus assuming that without this conf macro present, this file would not compile anyway?

0xc0170 · 2017-11-02T08:55:43Z

/morph build

mbed-ci · 2017-11-02T09:19:41Z

Build : SUCCESS

Build number : 411
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5270/

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-11-02T11:19:57Z

Test : SUCCESS

Build number : 202
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5270/202

theotherjimmy · 2017-11-02T15:33:42Z

/morph uvisor-test

0xc0170 · 2017-11-08T13:13:00Z

/morph uvisor-test

mbed-ci · 2017-11-08T13:56:47Z

Build : SUCCESS

Build number : 459
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5270/

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-11-08T18:03:01Z

Test : SUCCESS

Build number : 279
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5270/279

0xc0170 · 2017-11-09T11:06:12Z

@alzix Please trigger uvisor for this patch (seems it is not active for us at the moment for some reason)

ryankurte · 2017-11-14T21:35:07Z

Thanks all for the help with this!

ryankurte mentioned this pull request Oct 8, 2017

EFR32FG support #5271

Closed

0xc0170 added the needs: review label Oct 9, 2017

SeppoTakalo approved these changes Oct 9, 2017

View reviewed changes

ryankurte changed the title ~~Fix for Nanostack theading on EFR32 devices~~ Fix for Nanostack threading on EFR32 devices Oct 9, 2017

SeppoTakalo approved these changes Oct 9, 2017

View reviewed changes

0xc0170 reviewed Oct 9, 2017

View reviewed changes

ryankurte force-pushed the fix/nanostack-efr32-threading branch 2 times, most recently from 70f55a3 to 6db6d26 Compare October 9, 2017 12:06

theotherjimmy added needs: work and removed needs: review labels Oct 9, 2017

ryankurte force-pushed the fix/nanostack-efr32-threading branch from 6db6d26 to 8e59dd3 Compare October 10, 2017 01:32

stevew817 reviewed Oct 10, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Oct 19, 2017

adbridge added the needs: review label Oct 20, 2017

SeppoTakalo suggested changes Oct 31, 2017

View reviewed changes

ryankurte closed this Oct 31, 2017

ryankurte force-pushed the fix/nanostack-efr32-threading branch from fd8ef87 to 7b2e9b1 Compare October 31, 2017 10:31

sg- removed the needs: review label Oct 31, 2017

ryankurte added 2 commits October 31, 2017 23:48

Added critical section around rf_start_cca

c64c7b8

ryankurte reopened this Oct 31, 2017

adbridge added the needs: review label Oct 31, 2017

Swapped to macro definition for debug prints

37a38d0

SeppoTakalo approved these changes Nov 1, 2017

View reviewed changes

0xc0170 reviewed Nov 2, 2017

View reviewed changes

theotherjimmy added needs: CI and removed needs: review labels Nov 2, 2017

0xc0170 added ready for merge release-version: 5.6.5 and removed needs: CI labels Nov 9, 2017

0xc0170 merged commit c3a14c9 into ARMmbed:master Nov 9, 2017

0xc0170 removed the ready for merge label Nov 9, 2017

Fix for Nanostack threading on EFR32 devices #5270

Fix for Nanostack threading on EFR32 devices #5270

Uh oh!

Conversation

ryankurte commented Oct 8, 2017

Description

Status

Uh oh!

0xc0170 commented Oct 9, 2017

Uh oh!

SeppoTakalo 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!

ryankurte commented Oct 9, 2017

Uh oh!

SeppoTakalo commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryankurte commented Oct 9, 2017

Uh oh!

mbed-ci commented Oct 10, 2017

Triggering tests

Uh oh!

ryankurte commented Oct 10, 2017

Uh oh!

mbed-ci commented Oct 10, 2017

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!

SeppoTakalo commented Oct 11, 2017

Uh oh!

mbed-ci commented Oct 11, 2017

Triggering tests

Uh oh!

mbed-ci commented Oct 11, 2017

Triggering tests

Uh oh!

0xc0170 commented Oct 13, 2017

Uh oh!

stevew817 commented Oct 19, 2017

Uh oh!

0xc0170 commented Oct 19, 2017

Uh oh!

mbed-ci commented Oct 19, 2017

Uh oh!

adbridge commented Oct 20, 2017

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

ryankurte commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adbridge commented Oct 31, 2017

Uh oh!

SeppoTakalo commented Nov 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Nov 2, 2017

Uh oh!

mbed-ci commented Nov 2, 2017

Triggering tests

Uh oh!

mbed-ci commented Nov 2, 2017

Uh oh!

theotherjimmy commented Nov 2, 2017

Uh oh!

0xc0170 commented Nov 8, 2017

ryankurte commented Oct 31, 2017 •

edited

Loading

0xc0170 commented Nov 9, 2017 •

edited

Loading