New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Nanostack threading on EFR32 devices #5270

Merged
merged 3 commits into from Nov 9, 2017

Conversation

Projects
None yet
8 participants
@ryankurte
Contributor

ryankurte commented Oct 8, 2017

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.

@ryankurte ryankurte referenced this pull request Oct 8, 2017

Closed

EFR32FG support #5271

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 9, 2017

@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.

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp Outdated
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);

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 9, 2017

Contributor

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.

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp Outdated
@@ -22,6 +23,39 @@
#include "mbed-trace/mbed_trace.h"
#define TRACE_GROUP "SLRF"
//#define tr_debug(...) printf(__VA_ARGS__)

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 9, 2017

Contributor

Don't leave commented code blocks in commits.

platform_exit_critical();
}
}

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 9, 2017

Contributor

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

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

@ryankurte

This comment has been minimized.

Contributor

ryankurte commented Oct 9, 2017

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

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Oct 9, 2017

Yes, please squash because it is one logical change.

features/nanostack/FEATURE_NANOSTACK/targets/TARGET_SL_RAIL/NanostackRfPhyEfr32.cpp Outdated
waiting_for_ack = false;
#ifdef MBED_CONF_RTOS_PRESENT
//NVIC_DisableIRQ(MacHw_IRQn);

This comment has been minimized.

@0xc0170

0xc0170 Oct 9, 2017

Member

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

@ryankurte ryankurte force-pushed the ryankurte:fix/nanostack-efr32-threading branch 2 times, most recently Oct 9, 2017

@ryankurte

This comment has been minimized.

Contributor

ryankurte commented Oct 9, 2017

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.

@ryankurte ryankurte force-pushed the ryankurte:fix/nanostack-efr32-threading branch Oct 10, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

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

This comment has been minimized.

Contributor

ryankurte commented Oct 10, 2017

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

@mbed-ci

This comment has been minimized.

continue;
}
platform_enter_critical();

This comment has been minimized.

@stevew817

stevew817 Oct 10, 2017

Contributor

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?

This comment has been minimized.

@stevew817

stevew817 Oct 10, 2017

Contributor

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.

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 11, 2017

Contributor

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

This comment has been minimized.

Contributor

SeppoTakalo commented Oct 11, 2017

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

This comment has been minimized.

mbed-ci commented Oct 11, 2017

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

This comment has been minimized.

mbed-ci commented Oct 11, 2017

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

This comment has been minimized.

Member

0xc0170 commented Oct 13, 2017

@SeppoTakalo Please review the latest update

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Oct 19, 2017

@0xc0170 this looks good to go to me.

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

/morph test

@mbed-ci

This comment has been minimized.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 20, 2017

"@SeppoTakalo Please review the latest update"
BUMP

@ryankurte ryankurte closed this Oct 31, 2017

@ryankurte ryankurte force-pushed the ryankurte:fix/nanostack-efr32-threading branch to 7b2e9b1 Oct 31, 2017

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

@ryankurte

This comment has been minimized.

Contributor

ryankurte commented Oct 31, 2017

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.

ryankurte added some commits Oct 6, 2017

Threaded wrapper for rail driver on EFR32 platforms
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)

@ryankurte ryankurte reopened this Oct 31, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 31, 2017

@SeppoTakalo Are you happy with the updates ?

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Nov 1, 2017

Looks OK.

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

This comment has been minimized.

@0xc0170

0xc0170 Nov 2, 2017

Member

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

This comment has been minimized.

Member

0xc0170 commented Nov 2, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 2, 2017

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

This comment has been minimized.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 2, 2017

/morph uvisor-test

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 8, 2017

/morph uvisor-test

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 8, 2017

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

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 9, 2017

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

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

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@ryankurte

This comment has been minimized.

Contributor

ryankurte commented Nov 14, 2017

Thanks all for the help with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment