[platforms] new platform Qorvo GP712. by erja-gp · Pull Request #1942 · openthread/openthread

erja-gp · 2017-06-28T10:00:47Z

No description provided.

googlebot · 2017-06-28T10:00:50Z

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.

erja-gp · 2017-06-28T11:18:06Z

CLA -> I signed it!

aeliot · 2017-06-28T16:38:11Z

examples/Makefile-gp712

@@ -0,0 +1,274 @@
+#
+#  Copyright (c) 2016-2017, The OpenThread Authors.


nit: should be 2017

aeliot · 2017-06-28T16:50:08Z

examples/platforms/gp712/Makefile.am

+    -I$(top_srcdir)/examples/platforms      \
+    -I$(top_srcdir)/examples/platforms/utils\
+    -I$(top_srcdir)/src/core                \
+    -lrt        \


nit: align \

aeliot · 2017-06-28T16:55:33Z

examples/platforms/gp712/flash.c

+#include "utils/flash.h"
+
+static int sFlashFd;
+uint32_t sEraseAddress;


nit: align variable names

aeliot · 2017-06-28T16:56:30Z

examples/platforms/gp712/flash.c

+{
+    FLASH_SIZE = 0x40000,
+    FLASH_PAGE_SIZE = 0x800,
+    FLASH_PAGE_NUM = 128,


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

aeliot · 2017-06-28T17:04:23Z

examples/platforms/gp712/flash.c

+otError utilsFlashInit(void)
+{
+    otError error = OT_ERROR_NONE;
+    char fileName[20];


nit: align variable names

aeliot · 2017-06-28T18:10:26Z

examples/platforms/gp712/uart-socket.c

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


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

aeliot · 2017-06-28T18:11:31Z

examples/platforms/gp712/uart-socket.c

+                PlatSocketRx(readLen, buffer, clientSocket->socketId);
+
+                res = write(PlatSocketPipeFd [1], &someByte, 1); //[1] = write fd
+                (void)(res);


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

aeliot · 2017-06-28T18:12:12Z

examples/platforms/gp712/uart-socket.c

+
+    PlatSocketClose(clientSocket->socketId);
+
+    return NULL;


Should this return something other then NULL?

aeliot · 2017-06-28T18:12:31Z

examples/platforms/gp712/uart-socket.c

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


nit: indentation is off

aeliot · 2017-06-28T18:14:42Z

examples/platforms/gp712/uart-socket.c

+    //All sockets
+    if(PlatSocketConnection.isValid)
+    {
+        if((int)PlatSocketConnection.socketId == (int)socketId)


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

erja-gp · 2017-06-29T12:58:05Z

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

chshu · 2017-06-29T13:29:28Z

examples/platforms/gp712/radio.c

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


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

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

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.

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.

googlebot · 2017-06-30T22:18:38Z

CLAs look good, thanks!

jwhui · 2017-07-07T17:03:24Z

examples/Makefile-gp712

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


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

jwhui · 2017-07-07T17:03:32Z

examples/Makefile-gp712

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


See comment above.

jwhui · 2017-07-07T17:03:36Z

examples/Makefile-gp712

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


See comment above.

jwhui · 2017-07-07T17:05:22Z

examples/platforms/gp712/uart-posix.c

+    return error;
+}
+
+void platformUartUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, fd_set *aErrorFdSet, int *aMaxFd)


Just remove this function?

jwhui · 2017-07-07T17:07:26Z

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.

aeliot · 2017-07-07T17:09:35Z

examples/platforms/gp712/flash.c

+
+    // 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);


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

aeliot · 2017-07-07T17:11:17Z

examples/platforms/gp712/platform.c

+{
+    if (localInstance)
+    {
+        return !otTaskletsArePending(localInstance);


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

aeliot · 2017-07-07T17:12:15Z

examples/platforms/gp712/radio.c

+{
+    uint16_t panid;
+} otCachedSettings_t;
+static otCachedSettings_t otCachedSettings;


nit: newline between type definition and usage.

aeliot · 2017-07-07T17:12:26Z

examples/platforms/gp712/radio.c

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


nit: extra newline

aeliot · 2017-07-07T17:14:27Z

examples/platforms/gp712/uart-posix.c

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


Should only be one return statement.

aeliot · 2017-07-07T17:16:29Z

examples/platforms/gp712/uart-socket.c

+
+    // hack
+    res = pipe(PlatSocketPipeFd);
+    (void)(res);


Why assign to res if its going to be ignored?

aeliot · 2017-07-10T16:47:44Z

examples/platforms/gp712/radio.c

+
+enum
+{
+    IEEE802154_MIN_LENGTH = 5,


nit: Align =

aeliot · 2017-07-10T16:48:59Z

examples/platforms/gp712/uart-socket.c

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


nit: align =

aeliot

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

erja-gp · 2017-07-11T06:21:27Z

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

beriberikix · 2017-07-11T16:33:07Z

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

erja-gp added 6 commits June 28, 2017 11:57

Added Qorvo RPi in examples

4af0b0f

Added Readme.md for RPi

cacc75b

renaming platform example directory to gp712

453e072

moved most library code to examples/platforms/gp712

1870b2b

fixed non-functional comments from jwhui

5aaf423

aligned gp712 and posix reset procedure

8839651

jwhui changed the title ~~new platform Qorvo GP712.~~ [platforms] new platform Qorvo GP712. Jun 28, 2017

jwhui added enhancement P3 labels Jun 28, 2017

aeliot suggested changes Jun 28, 2017

View reviewed changes

erja-gp added 3 commits June 29, 2017 13:35

fixing the comments from aeliot

7eb0662

fixed radio, logging and Makefile comments

ab4ad4e

fixed uart-socket indentation and casting

555dec6

chshu reviewed Jun 29, 2017

View reviewed changes

erja-gp added 2 commits July 5, 2017 21:26

code style updates

29257cd

code style updates uart-posix.c

28c4c3f

jwhui reviewed Jul 7, 2017

View reviewed changes

aeliot reviewed Jul 7, 2017

View reviewed changes

erja-gp added 2 commits July 10, 2017 10:33

updates after code review

8d3eea8

added readme with link to license file

3026049

jwhui requested a review from aeliot July 10, 2017 15:43

aeliot reviewed Jul 10, 2017

View reviewed changes

fixed alignments

facc50d

jwhui requested a review from aeliot July 10, 2017 19:00

aeliot approved these changes Jul 11, 2017

View reviewed changes

removed remaining unused vars

3923382

jwhui approved these changes Jul 11, 2017

View reviewed changes

jwhui merged commit 3078359 into openthread:master Jul 11, 2017

beriberikix mentioned this pull request Jul 11, 2017

[docs] add Qorvo logo #1980

Merged

		@@ -0,0 +1,274 @@
		#
		# Copyright (c) 2016-2017, The OpenThread Authors.

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

Conversation

erja-gp commented Jun 28, 2017

Uh oh!

googlebot commented Jun 28, 2017

Uh oh!

erja-gp commented Jun 28, 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!

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!

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!

erja-gp commented Jun 29, 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!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jun 30, 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!

Choose a reason for hiding this comment

Uh oh!

jwhui commented Jul 7, 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!

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!

Choose a reason for hiding this comment

Uh oh!

aeliot left a comment

Choose a reason for hiding this comment

Uh oh!

erja-gp commented Jul 11, 2017

Uh oh!

beriberikix commented Jul 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants