Skip to content
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

[platforms] new platform Qorvo GP712. #1942

Merged
merged 15 commits into from Jul 11, 2017
Merged

[platforms] new platform Qorvo GP712. #1942

merged 15 commits into from Jul 11, 2017

Conversation

@erja-gp
Copy link
Contributor

@erja-gp erja-gp commented Jun 28, 2017

No description provided.

@googlebot
Copy link

@googlebot googlebot commented Jun 28, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

Loading

@erja-gp
Copy link
Contributor Author

@erja-gp erja-gp commented Jun 28, 2017

CLA -> I signed it!

Loading

@jwhui jwhui changed the title new platform Qorvo GP712. [platforms] new platform Qorvo GP712. Jun 28, 2017
@@ -0,0 +1,274 @@
#
# Copyright (c) 2016-2017, The OpenThread Authors.
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should be 2017

Loading

-I$(top_srcdir)/examples/platforms \
-I$(top_srcdir)/examples/platforms/utils\
-I$(top_srcdir)/src/core \
-lrt \
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: align \

Loading

#include "utils/flash.h"

static int sFlashFd;
uint32_t sEraseAddress;
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: align variable names

Loading

{
FLASH_SIZE = 0x40000,
FLASH_PAGE_SIZE = 0x800,
FLASH_PAGE_NUM = 128,
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look like they should maybe be #defines rather then enums. If this is going to be an enum, then the names should start with a k and be CamelCase rather then all caps snake_case

Loading

otError utilsFlashInit(void)
{
otError error = OT_ERROR_NONE;
char fileName[20];
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: align variable names

Loading

uint8_t readChar;
//Remove trigger byte from pipe
res = read(PlatSocketPipeFd [0], &readChar, 1);
(void)(res);
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why assign to res if its not going to be used?

Loading

PlatSocketRx(readLen, buffer, clientSocket->socketId);

res = write(PlatSocketPipeFd [1], &someByte, 1); //[1] = write fd
(void)(res);
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have res if your just going to assign to it and then ignore the value?

Loading


PlatSocketClose(clientSocket->socketId);

return NULL;
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return something other then NULL?

Loading

void PlatSocketRxNewConn(uint8_t id)
{
//Find first non-valid client in list - add here
if (PlatSocketConnection.isValid == 0)
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation is off

Loading

//All sockets
if(PlatSocketConnection.isValid)
{
if((int)PlatSocketConnection.socketId == (int)socketId)
Copy link
Contributor

@aeliot aeliot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both sides of this conditional are originally uint32_t, so why are they both being cast to int before comparison?

Loading

@erja-gp
Copy link
Contributor Author

@erja-gp erja-gp commented Jun 29, 2017

@aeliot : these commits should cover all of the comments you made. Some of the code was very close to the posix code, and by e.g. removing the checks for WIN the difference becomes a bit bigger, but I guess that's not really an issue.

Loading


void cbQorvoRadioTransmitDone(otRadioFrame *aPacket, bool aFramePending, otError aError)
{
otPlatRadioTransmitDone(pQorvoInstance, aPacket, aFramePending, aError);
Copy link
Contributor

@chshu chshu Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not recommended to use the legacy TransmitDone() callback anymore, could you use the new TxDone() instead?

Loading

Copy link
Contributor Author

@erja-gp erja-gp Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the remark, but this is not a trivial change and we do not plan to do that immediately.

Loading

Copy link
Contributor

@chshu chshu Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is not mandatory for now, the short-term benefit is that ACK messages could also be used for link quality evaluation. But eventually, we need to use the new TxDone() callback for some new features.

Loading

Copy link
Contributor

@aeliot aeliot Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually have the same problem with another platform I'm working on. It seams like we may need to come up with a way for these two to coexist going forwards.

Loading

@googlebot
Copy link

@googlebot googlebot commented Jun 30, 2017

CLAs look good, thanks!

Loading

#
define build-arch
$(ECHO) " BUILD $(1)-$(TargetTuple)"
$(MAKE) $(JOBSFLAG) -C $(BuildPath)/$(1)-$(TargetTuple) -w \
Copy link
Member

@jwhui jwhui Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other examples Makefiles have $(BuildPath)/$(TargetTuple). Was having $(BuildPath)/$(1)-$(TargetTuple) a conscious decision?

Loading

#
define stage-arch
$(ECHO) " STAGE $(1)-$(TargetTuple)"
$(MAKE) $(JOBSFLAG) -C $(BuildPath)/$(1)-$(TargetTuple) -w \
Copy link
Member

@jwhui jwhui Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

Loading

CONFIGURE_TARGETS += configure-$(1)
BUILD_TARGETS += do-build-$(1)
STAGE_TARGETS += stage-$(1)
BUILD_DIRS += $(BuildPath)/$(1)-$(TargetTuple)
Copy link
Member

@jwhui jwhui Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

Loading

return error;
}

void platformUartUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, fd_set *aErrorFdSet, int *aMaxFd)
Copy link
Member

@jwhui jwhui Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove this function?

Loading

@jwhui
Copy link
Member

@jwhui jwhui commented Jul 7, 2017

Also, please add a third_party/Qorvo/README.md file that provides version and license information for the contents. You can find an example with third_party/mbedtls/README.md.

Loading


// Write the page
r = pwrite(sFlashFd, &(dummyPage[0]), FLASH_PAGE_SIZE, (off_t)address);
otEXPECT_ACTION(((int)r) == ((int)(FLASH_PAGE_SIZE)), error = OT_ERROR_FAILED);
Copy link
Contributor

@aeliot aeliot Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are both sides of this conditional cast to int? I think you can do the comparison without the cast.

Loading

{
if (localInstance)
{
return !otTaskletsArePending(localInstance);
Copy link
Contributor

@aeliot aeliot Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should only be one return statement, as per the style guide.

Loading

{
uint16_t panid;
} otCachedSettings_t;
static otCachedSettings_t otCachedSettings;
Copy link
Contributor

@aeliot aeliot Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline between type definition and usage.

Loading

static uint8_t sScanstate = 0;
static int8_t sLastReceivedPower = 127;


Copy link
Contributor

@aeliot aeliot Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra newline

Loading

otEXPECT_ACTION(tcsetattr(s_out_fd, TCSANOW, &termios) == 0, perror("tcsetattr"); error = OT_ERROR_GENERIC);
}

return error;
Copy link
Contributor

@aeliot aeliot Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should only be one return statement.

Loading


// hack
res = pipe(PlatSocketPipeFd);
(void)(res);
Copy link
Contributor

@aeliot aeliot Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why assign to res if its going to be ignored?

Loading

@jwhui jwhui requested a review from aeliot Jul 10, 2017

enum
{
IEEE802154_MIN_LENGTH = 5,
Copy link
Contributor

@aeliot aeliot Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Align =

Loading

{
buf = malloc(length + 2);
memcpy(buf, buffer, length);
buf[length] = '\n';
Copy link
Contributor

@aeliot aeliot Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: align =

Loading

@jwhui jwhui requested a review from aeliot Jul 10, 2017
aeliot
aeliot approved these changes Jul 11, 2017
Copy link
Contributor

@aeliot aeliot left a comment

LGTM, except my comments in uart-socket.c about unused variables.

Loading

@erja-gp
Copy link
Contributor Author

@erja-gp erja-gp commented Jul 11, 2017

@aeliot Indeed I missed out two more unused variables -> fixed it.

Loading

jwhui
jwhui approved these changes Jul 11, 2017
@jwhui jwhui merged commit 3078359 into openthread:master Jul 11, 2017
3 checks passed
Loading
@beriberikix
Copy link
Contributor

@beriberikix beriberikix commented Jul 11, 2017

🎉 congrats! 🎉 Feel free to add Qorvo the AUTHORS file.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants