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

Add support for SiLabs EFR32. #1592

Merged
merged 6 commits into from Apr 24, 2017

Conversation

Projects
None yet
6 participants
@xiaom-GitHub
Copy link
Contributor

xiaom-GitHub commented Apr 14, 2017

This PR includes the necessary implementation within OpenThread scope to support EFR32MG12 SoC platform, the underly SDK/RAIL library package needs to be downloaded from Simplicity Studio. See README.md for more information.

@lanyuwen

This comment has been minimized.

Copy link
Member

lanyuwen commented Apr 14, 2017

This is awesome, thanks Xiao!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 14, 2017

Codecov Report

Merging #1592 into master will increase coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
+ Coverage   66.93%   67.36%   +0.43%     
==========================================
  Files         166      167       +1     
  Lines       21102    21109       +7     
  Branches     2588     2588              
==========================================
+ Hits        14124    14221      +97     
+ Misses       6062     5980      -82     
+ Partials      916      908       -8
Impacted Files Coverage Δ
examples/platforms/posix/flash.c 90.9% <ø> (ø) ⬆️
src/core/coap/coap_client.hpp 73.33% <0%> (-20%) ⬇️
src/core/coap/coap_client.cpp 52.38% <0%> (-19.81%) ⬇️
src/core/coap/coap_server.cpp 81.48% <0%> (-7.41%) ⬇️
src/core/coap/coap_server.hpp 88.88% <0%> (-3.97%) ⬇️
src/core/net/udp6.cpp 82.85% <0%> (-1.91%) ⬇️
src/core/thread/topology.hpp 89.02% <0%> (-1.22%) ⬇️
examples/apps/ncp/main.c 100% <0%> (ø)
examples/platforms/posix/radio.c 78.42% <0%> (+0.41%) ⬆️
src/ncp/ncp_base.cpp 30.04% <0%> (+0.47%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 030efba...90c9680. Read the comment docs.

@jwhui jwhui requested a review from aeliot Apr 14, 2017

static uint32_t sTimerLo = 0;
static uint32_t sAlarmT0 = 0;
static uint32_t sAlarmDt = 0;
static bool sIsRunning = false;

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

nit: could you align these = signs?

uint32_t expires;
bool fire = false;

if (sIsRunning)

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

Instead of wrapping the entire function in an if, could we use a VerifyOrExit call here instead?

{
if (expires >= sAlarmT0 && expires <= now)
{
fire = true;

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

Could you not just assign the conditional above to fire?

fire = (expires >= sAlarmT0 && expires <= now);
static bool sPromiscuous = false;
static uint8_t sChannel = 0;
static bool sIsReceiverEnabled = false;
static uint16_t sPanId = 0;

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

nit: Could you align the variable names and assignments here, so its easier to read?

bool otPlatRadioIsEnabled(otInstance *aInstance)
{
(void)aInstance;
return (sState != kStateDisabled) ? true : false;;

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

Why does this need to be an inline if? Couldn't you just return the conditional?

return (sState != kStateDisabled);
CORE_DECLARE_IRQ_STATE;
CORE_ENTER_CRITICAL();

otLogInfoPlat(sInstance, "EnableSrcMatch=%d", aEnable ? 1 : 0);

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

I don't think you need the inline if here.


if (aEnable)
{
sIsSrcMatchEnabled = true;

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

Couldn't we just set sIsSrcMatchEnabled = aEnable?

{
if (sIsSrcMatchEnabled)
{
if (aAddress->length == RAIL_IEEE802154_LongAddress)

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

Would it be worth collapsing these if statements into one?

if ( (aAddress->length == RAIL_IEEE802154_LongAddress &&
      findSrcMatchExtEntry(aAddress->longAddress) >= 0 ) ||
     (aAddress->length != RAIL_IEEE802154_LongAddress &&
      findSrcMatchShortEntry(aAddress->shortAddress) >= 0) )
{
    RAIL_IEEE802154_SetFramePending();
}
rxPacketInfo->appendedInfo.crcStatus == false ||
rxPacketInfo->appendedInfo.frameCodingStatus == false)
{
return;

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

As per the style guide, functions should only have one return statement. Could you use the VerifyOrExit macro here instead?

@@ -1,5 +1,5 @@
/*
* Copyright 2016 Nest Labs Inc. All Rights Reserved.
* Copyright 2016 The OpenThread Authors. All Rights Reserved.

This comment has been minimized.

@aeliot

aeliot Apr 17, 2017

Member

@jwhui @beriberikix Should the copyright on this file be changed? This is in the third_party directory, so is it by the OpenThread authors or is it by Nest?

This comment has been minimized.

@beriberikix

beriberikix Apr 17, 2017

Contributor

We'll follow-up. It is up to the contributor or copyright owner on how code in third_party is licensed, so I think it may be under The OpenThread Authors if we choose.

@xiaom-GitHub

This comment has been minimized.

Copy link
Contributor

xiaom-GitHub commented Apr 18, 2017

Hi @aeliot thanks for your detailed review, I had a update and tested with new commit, pls take a look.


if (length != rxPacketInfo->dataPtr[0])
{
ExitNow();

This comment has been minimized.

@aeliot

aeliot Apr 18, 2017

Member

Would it be better to us VerifyOrExit here?

VerifyOrExit(length == rxPacketInfo->dataPtr[0]);

This comment has been minimized.

@xiaom-GitHub

xiaom-GitHub Apr 19, 2017

Contributor

updated, thanks.

@xiaom-GitHub xiaom-GitHub force-pushed the xiaom-GitHub:pr/support_efr32 branch 2 times, most recently from f177bb3 to 6128453 Apr 19, 2017

@xiaom-GitHub

This comment has been minimized.

Copy link
Contributor

xiaom-GitHub commented Apr 20, 2017

Hi @jwhui @aeliot I rebased on the latest master branch and updated the "VerifyOrExit" check with new APIs, please take a look again. Thanks.


while (aSize--)
{
*byte++ = (uint8_t)(*(uint8_t *)(pAddress++));

This comment has been minimized.

@aeliot

aeliot Apr 20, 2017

Member

Do we need the (uint8_t) typecast here? Shouldn't a dereference of a (uint8_t *) return a uint8_t?

This comment has been minimized.

@xiaom-GitHub

xiaom-GitHub Apr 21, 2017

Contributor

pAddress is flash address, it's uint32_t type. Flash Read operation aligns by words, here we copy the flash data byte by byte to dest buffer, hence, convert the address pointer to uint8_t to read one byte each time until up to the aSize. Variable byte is also uint8_t type, it's better to keep this typecast here.

This comment has been minimized.

@aeliot

aeliot Apr 21, 2017

Member

Yes, but the (uint8_t) typecast is redundant. (*(uint8_t *)(pAddress++)) already returns as a uint8_t, since your dereferencing a uint8_t pointer.

static uint8_t sReceiveBuffer[IEEE802154_MAX_LENGTH + 1 + sizeof(RAIL_RxPacketInfo_t)];
static RadioPacket sReceiveFrame;
static uint8_t sReceivePsdu[IEEE802154_MAX_LENGTH];
static ThreadError sReceiveError;

This comment has been minimized.

@aeliot

aeliot Apr 20, 2017

Member

Can you align all the variable names here?

This comment has been minimized.

@xiaom-GitHub

xiaom-GitHub Apr 21, 2017

Contributor

Done. Thanks.

int8_t otPlatRadioGetRssi(otInstance *aInstance)
{
(void)aInstance;
return (uint8_t)(RAIL_RxGetRSSI() / 4);

This comment has been minimized.

@aeliot

aeliot Apr 20, 2017

Member

nit: Could this division be done with a bit shift, for efficiency? Maybe with a comment above just saying its a division by 4?

return (uint8_t)(RAIL_RxGetRSSI() >> 2 );

This comment has been minimized.

@xiaom-GitHub

xiaom-GitHub Apr 21, 2017

Contributor

Done. Thanks.

sTransmitError = kThreadError_Abort;
sTransmitBusy = false;
break;
}

This comment has been minimized.

@aeliot

aeliot Apr 20, 2017

Member

Do we need a default here? Is there any other potential values for aStatus?

This comment has been minimized.

@xiaom-GitHub

xiaom-GitHub Apr 21, 2017

Contributor

According to RAIL documentation, there are some other return values here, but they are events which are not needed to notify the upper layer, for example, callback for CCA success, callback for when a CCA begins, callback for when CCA retries. When failure happens such as CCA failure (channel busy) or User abort Tx transmission, we should notify upper MAC layer to start a backoff method or retransmission. I can add a default status here for integrity.

@xiaom-GitHub xiaom-GitHub force-pushed the xiaom-GitHub:pr/support_efr32 branch from 6128453 to 8128c62 Apr 21, 2017

@xiaom-GitHub xiaom-GitHub force-pushed the xiaom-GitHub:pr/support_efr32 branch from 8128c62 to 34dc4c7 Apr 21, 2017


void setChannel(uint8_t channel)
{
if (sChannel != channel)

This comment has been minimized.

@aeliot

aeliot Apr 21, 2017

Member

Can we use one of the error handling macros here?

CORE_DECLARE_IRQ_STATE;
CORE_ENTER_CRITICAL();

if (!otPlatRadioIsEnabled(aInstance))

This comment has been minimized.

@aeliot

aeliot Apr 21, 2017

Member

Can we use one of the error handling macros here instead of an if?

This comment has been minimized.

@aeliot

aeliot Apr 21, 2017

Member

Same goes for most of the otPlat functions in this file.

This comment has been minimized.

@xiaom-GitHub

xiaom-GitHub Apr 22, 2017

Contributor

Yes, definitely we can replace, and, may I have a question about what the benefit is for that? Does it help improve the code coverage or something else? That means all if should be replaced by otEXPECT_ACTION or otEXPECT? Please correct me if I miss something. Thanks.

This comment has been minimized.

@aeliot

aeliot Apr 22, 2017

Member

It generally helps with readability. Using the error handling macros reduces the number of braces, so that while reading you don't have to keep track of as many contexts, and also makes code more compact and concise.

This comment has been minimized.

@xiaom-GitHub

xiaom-GitHub Apr 24, 2017

Contributor

Updated and tested. Thanks.

@xiaom-GitHub xiaom-GitHub force-pushed the xiaom-GitHub:pr/support_efr32 branch from 34dc4c7 to 90c9680 Apr 24, 2017

@jwhui jwhui requested a review from aeliot Apr 24, 2017

@aeliot

aeliot approved these changes Apr 24, 2017

Copy link
Member

aeliot left a comment

LGTM

@jwhui

jwhui approved these changes Apr 24, 2017

Copy link
Member

jwhui left a comment

Looks great! Thanks for bringing a new platform to OpenThread!

@jwhui jwhui merged commit b83f644 into openthread:master Apr 24, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

vaas-krish pushed a commit to vaas-krish/openthread that referenced this pull request May 23, 2017

Merge pull request openthread#259 in TPS/openthread from feature/comm…
…s/github_2017_04_25 to master

* commit 'f25c6727badcddbe7d1d8263478ecb4ec6c40fec': (35 commits)
  Add `ExitNow()` in MARBLE-387 workaround path.
  Reduce MTD code size by removing some unused MeshCoP features (openthread#1361)
  Add/Update logs in `MeshForwarder` class (openthread#1649)
  Remove use of gnu-extension anonymous struct/union definitions. (openthread#1651)
  Require platform TRNG to provide requested output length or error. (openthread#1650)
  Ensure fairness in handling of data polls from sleepy children (openthread#1646)
  Update formatting in `NcpSpi` and `spi-slave.h` files (openthread#1648)
  Add EFR32 to readme. (openthread#1647)
  Add support for SiLabs EFR32. (openthread#1592)
  No payload no marker (openthread#1645)
  return the right error information for some cli commands (openthread#1644)
  Allow to clear Operation Dataset when a new network parameter is set. (openthread#1639)
  `NcpBuffer`: Fixing bug with handling of longer frame segments (openthread#1643)
  Fix inherits implicit virtual warning. (openthread#1641)
  Fix README.md command list of Diag module. (openthread#1640)
  Fix the error check for src/dst address get in `Frame::ToInfoString()` (openthread#1633)
  Set Commissioner Data when becoming Leader (openthread#1636)
  enable all features on posix (openthread#1634)
  Add `src/core` header files to PRETTY_FILES. (openthread#1632)
  Remove unnecessary configure options from pretty check. (openthread#1631)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment