pkg/openthread: core and tests by jia200x · Pull Request #7082 · RIOT-OS/RIOT

jia200x · 2017-05-18T23:44:48Z

This is the OpenThread core from #5552 . I'm spliting #5552 into multiple PR for an easier review.
After merging this one, I can push the sock adoption.

Cheers!

aabadie

Some style issue and comments. But this is much better than in #5552. Thanks for keeping the effort on that @jia200x and @biboc !

aabadie · 2017-05-19T07:32:37Z

pkg/openthread/contrib/platform_radio.c

+{
+    _dev->driver->get(_dev, NETOPT_IPV6_IID, aIeee64Eui64, sizeof(eui64_t));
+}
+


empty lines that can be removed

this was fixed in the last commit

aabadie · 2017-05-19T07:33:09Z

pkg/openthread/contrib/platform_radio.c

+    _set_channel(aChannel);
+    return kThreadError_None;
+}
+


empty line. One empty line between function impl is enough

this was fixed in the last commit

aabadie · 2017-05-19T07:34:38Z

pkg/openthread/contrib/platform_misc.c

+otPlatResetReason otPlatGetResetReason(otInstance *aInstance)
+{
+    (void)aInstance;
+    // TODO: Write me!


C++ style comment. Maybe try to fix this TODO

This is the platform abstraction function of the posix example in OpenThread, as seen here

ok, then let's leave it like this!

aabadie · 2017-05-19T07:35:40Z

pkg/openthread/contrib/platform_random.c

+    char cpu_id[CPUID_LEN];
+    cpuid_get(cpu_id);
+    uint32_t seed = 0;
+    for (int i = 0; i < (int) CPUID_LEN; i++) {


should be unsigned instead of int and then no need to cast I think

aabadie · 2017-05-19T07:36:28Z

pkg/openthread/include/ot.h

+ *
+ * @file
+ *
+ * @author  José Ignacio Alamos <[email protected]>


encoding issue

aabadie · 2017-05-19T07:39:26Z

tests/openthread/Makefile

+# Comment this out to disable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:
+


This line can be removed

aabadie · 2017-05-19T07:40:55Z

tests/openthread/main.c

+
+    otMessageInfo mPeer;
+
+    //Set dest address


cpp style comment

aabadie · 2017-05-19T07:41:02Z

tests/openthread/main.c

+    //Set dest address
+    memcpy(&mPeer.mPeerAddr.mFields, &(pkt->ip_addr), sizeof(ipv6_addr_t));
+
+    //Set dest port


cpp style comment

aabadie · 2017-05-19T07:41:34Z

tests/openthread/main.c

+int _udp(int argc, char **argv)
+{
+    if (argc < 3) {
+        return 1;


maybe print usage here

aabadie · 2017-05-19T07:47:43Z

tests/openthread/Makefile

+endif
+
+ifneq (,$(filter at86rf2%,$(DRIVER)))
+  FEATURES_REQUIRED = periph_spi periph_gpio


Then this line could be split in 2 until openthread is added to auto_init.

jia200x · 2017-05-19T15:22:09Z

@aabadie @biboc addressed all issues

aabadie · 2017-05-19T15:23:02Z

pkg/openthread/contrib/openthread.c

+void openthread_uart_run(void)
+{
+    char buf[256];
+    // uint8_t index=0;


Can be removed

aabadie · 2017-05-19T15:24:13Z

pkg/openthread/include/ot.h

+#define OPENTHREAD_XTIMER_MSG_TYPE_EVENT (0x2235)   /**< xtimer message receiver event*/
+#define OPENTHREAD_NETDEV_MSG_TYPE_EVENT (0x2236)   /**< message received from driver */
+#define OPENTHREAD_SERIAL_MSG_TYPE_EVENT (0x2237)   /**< event indicating a serial (UART) message was sent to OpenThread */
+#define OPENTHREAD_MSG_TYPE_RECV (0x2238)           /**< event for frame reception */


nit: the defined values could be aligned

this is very weird... I have them aligned.
It seems to be a different file. I'm checking

can you check if you see them aligned in your editor? with raw vi I can see them aligned...

aabadie · 2017-05-19T15:34:50Z

tests/openthread/Makefile

@@ -0,0 +1,49 @@
+APPLICATION = openthread_test


just call application "openthread"

aabadie · 2017-05-19T15:37:08Z

pkg/openthread/include/ot.h

+#define OPENTHREAD_XTIMER_MSG_TYPE_EVENT (0x2235)        /**< xtimer message receiver event*/
+#define OPENTHREAD_NETDEV_MSG_TYPE_EVENT (0x2236)        /**< message received from driver */
+#define OPENTHREAD_SERIAL_MSG_TYPE_EVENT (0x2237)        /**< event indicating a serial (UART) message was sent to OpenThread */
+#define OPENTHREAD_MSG_TYPE_RECV (0x2238)                /**< event for frame reception */


still not aligned, but maybe we are not looking at the same thing. I meant aligning (0x2238) with other values. The doxygen are right !

agh, I also see the comments misaligned. I'm into that.

aabadie · 2017-05-19T15:41:37Z

tests/openthread/main.c

+
+int main(void)
+{
+#if defined(MODULE_OPENTHREAD_CLI) || defined(MODULE_OPENTHREAD_NCP)


I don't see where MODULE_OPENTHREAD_CLI and MODULE_OPENTHREAD_NCP are defined.

ah, I actually I can safely remove that because they will be handled in @biboc examples (FTD, MTD, etc). I would leave it in the core files because they will be needed.

ok, so if I understand correctly, this PR only provides the ifconfig and udp commands provided in this main.c file ?

How can I play with openthread_cli ?

aabadie · 2017-05-19T15:49:27Z

pkg/openthread/Makefile

+		--prefix=/ \
+		--enable-default-logging \
+		${OPENTHREAD_ARGS}
+	cd $(PKG_BUILDDIR) && make -j99 --no-print-directory DESTDIR=$(PKG_BUILDDIR)/output install PREFIX=/


j99 is too much (my computer doesn't like that!). Please use something more realistic (-j4 is good)

ok, I'm on it

aabadie · 2017-05-19T16:05:27Z

I could make it work on IoT-Lab ! Great!
The commands output in the test application could be improved.

aabadie · 2017-05-19T16:05:48Z

pkg/openthread/contrib/netdev/openthread_netdev.c

+#include "openthread/ncp.h"
+#endif
+
+#define ENABLE_DEBUG (1)


Should be 0

you are right. I'm on it

aabadie · 2017-05-19T16:06:22Z

pkg/openthread/contrib/platform_misc.c

+
+#include "openthread/types.h"
+#include "openthread/platform/misc.h"
+#include "periph/pm.h"


Are you sure this include is required ?

it's for the pm_reset

ah yes indeed, sorry

aabadie · 2017-05-19T16:06:35Z

pkg/openthread/contrib/platform_radio.c

+#include "openthread/platform/radio.h"
+#include "ot.h"
+
+#define ENABLE_DEBUG (1)


Should be 0

aabadie · 2017-05-19T16:07:04Z

pkg/openthread/contrib/platform_settings.c

+#include "openthread/types.h"
+
+#include "debug.h"
+#define ENABLE_DEBUG (1)


Should be 0

jia200x · 2017-05-19T16:09:00Z

@aabadie what would you add specifically to the commands output?
Cheers!

aabadie · 2017-05-19T16:10:49Z

tests/openthread/main.c

+    for (const otNetifAddress *addr = otIp6GetUnicastAddresses(ot_instance); addr; addr = addr->mNext) {
+        char addrstr[IPV6_ADDR_MAX_STR_LEN];
+        ipv6_addr_to_str(addrstr, (ipv6_addr_t *) &addr->mAddress.mFields, sizeof(addrstr));
+        printf("inet6 %s\n", addrstr);


Is there a way to distinguish the different kind of addresses ? (link-local, RLOC, EID)

Not sure on OT side. Probably yes. Anyway, it's always possible to check the beginning of the ipv6 addr and print it next to the IP.

Ok, that's fine like this.

aabadie · 2017-05-19T16:17:45Z

pkg/openthread/Makefile

+		${OPENTHREAD_ARGS}
+	cd $(PKG_BUILDDIR) && make -j4 --no-print-directory DESTDIR=$(PKG_BUILDDIR)/output install PREFIX=/
+
+		cp $(PKG_BUILDDIR)/output/lib/libmbedcrypto.a ${BINDIR}/libmbedcrypto.a


The cp indentation looks weird. Can you have a look ?

aabadie · 2017-05-19T16:19:00Z

tests/openthread/main.c

+int _udp(int argc, char **argv)
+{
+    if (argc < 3) {
+		printf("Usage: udp server <port>\n");


indentation is off here as well

aabadie · 2017-05-19T16:19:44Z

pkg/openthread/Makefile

+    OPENTHREAD_ARGS+= --enable-cli-app=mtd
+endif
+ifneq (,$(filter libopenthread-ncp-ftd,$(USEMODULE)))
+      $(info Compile OpenThread with NCP)


indentation issue here (1 or 2 extra space)

aabadie · 2017-05-19T16:20:29Z

Please fix the indentation issues and squash and we are good

jia200x · 2017-05-19T16:33:44Z

@aabadie fixed and squashed. Can you check if everything is ok?

aabadie · 2017-05-19T16:45:21Z

pkg/openthread/Makefile

+all: git-download
+	cd $(PKG_BUILDDIR) && PREFIX="/" ./bootstrap
+	cd $(PKG_BUILDDIR) && ./configure \
+		--enable-application-coap INSTALL="/usr/bin/install -p" CPP="$(CPP)" \


/use/bin/install won't work on Mac I think. Maybe just using install is enough

let me check

I broke the Makefile because of missing tabs. I'm pushing the fix

it fails if I remove the /usr/bin part

the INSTALL part shouldn't be needed. I'm checking what happens if I remove that.

did that. Now should work

aabadie · 2017-05-19T16:51:42Z

tests/openthread/main.c

+    otUdpOpen(ot_instance, &socket, _handle_receive, NULL);
+
+    message = otUdpNewMessage(ot_instance, true);
+     otMessageSetLength(message, pkt->len);


jia200x · 2017-05-19T17:01:47Z

I'll be back in a couple of hours.

jia200x · 2017-05-19T20:12:42Z

@aabadie ok, now it's working again. Let me know if everything is OK so I can squash.ç
Cheers

biboc · 2017-05-22T16:13:58Z

I tried OpenThread example with two nodes and it appears that they both become leader. This mean that they are not on same network (because there is only one leader in a network)

Have you tried again your example?

jia200x · 2017-05-22T18:10:46Z

@biboc this might happen in the beginning, but after a while you should see only one leader.

Is not working for you?

aabadie · 2017-06-01T16:41:39Z

Let me check if murdock passes this time

It won't since you added extra commits ;)

jia200x · 2017-06-01T16:42:14Z

@aabadie why? Requires rebasing?

aabadie · 2017-06-01T16:43:17Z

why? Requires rebasing?

Yes, you need to play with git history to keep the 2 commits (git rebase -i)

aabadie · 2017-06-01T16:44:15Z

By the way, there are other issues (some reported by coccinelle and remaining trailing white spaces)

jia200x · 2017-06-01T16:46:24Z

do you know what is this one?:

Running './dist/tools/headerguards/check.sh' x
Command output:

	pkg/openthread/include/ot.h


---
--- compile job results (1 failed, 11692 passed, 11693 total):

miri64 · 2017-06-01T17:01:35Z

pkg/openthread/include/ot.h

+}
+#endif
+
+#endif


#endif /* OT_H */

and the warning will go away ;-)

jia200x · 2017-06-01T17:12:20Z

@biboc I added a copyright header with your name in the test

jia200x · 2017-06-01T17:15:05Z

@biboc @aabadie @miri64 murdock passed :)

aabadie

a few nits remaining

aabadie · 2017-06-01T17:19:48Z

pkg/openthread/contrib/platform_functions_wrapper.c

+#include "openthread/udp.h"
+#include "ot.h"
+
+#define ENABLE_DEBUG (1)


should be 0

aabadie · 2017-06-01T17:22:04Z

pkg/openthread/contrib/platform_settings.c

+#include "assert.h"
+#include "openthread/types.h"
+
+#include "debug.h"


include after define

aabadie · 2017-06-01T17:22:49Z

pkg/openthread/include/ot.h

+ * @brief   Struct containing an OpenThread job
+ */
+typedef struct {
+    const char *command;                      /**< A pointer to the job name string. */


doxygen not aligned

biboc · 2017-06-01T17:26:28Z

Ok @jia200x
I let you end this PR, i won't be available before next Tuesday.

jia200x · 2017-06-01T17:44:44Z

@biboc ok. Thank you for all the contribution!

@aabadie Done

aabadie

ACK. Thanks for the hard work @jia200x and @biboc !

aabadie · 2017-06-01T18:21:50Z

Now let's add support for NCP and MTD nodes. And add sock adaption :)

jia200x · 2017-06-01T18:25:32Z

@aabadie @biboc thank you so much!
The sock UDP part is ready. I will adapt to the merged OT and push a PR.
Cheers

miri64 · 2017-06-01T19:09:49Z

🎉 ✨ congratz @jia200x!

jia200x · 2017-06-01T19:43:25Z

🎉 ✨ congratz @jia200x!

Thanks!

emmanuelsearch · 2017-06-02T10:37:09Z

great! Nice work! @jia200x

jia200x mentioned this pull request May 18, 2017

OpenThread port #5552

Closed

aabadie requested changes May 19, 2017

View reviewed changes

jia200x force-pushed the openthread_core branch from 6265bac to b384548 Compare May 19, 2017 15:18

aabadie reviewed May 19, 2017

View reviewed changes

jia200x force-pushed the openthread_core branch from 3f10679 to b466f29 Compare May 19, 2017 16:33

aabadie reviewed May 19, 2017

View reviewed changes

kaspar030 added the Impact: major The PR changes a significant part of the code base. It should be reviewed carefully label May 20, 2017

jia200x force-pushed the openthread_core branch from ef33d93 to 9d4e1ce Compare June 1, 2017 16:52

miri64 reviewed Jun 1, 2017

View reviewed changes

jia200x force-pushed the openthread_core branch from 9d4e1ce to fa60436 Compare June 1, 2017 17:06

aabadie reviewed Jun 1, 2017

View reviewed changes

jia200x added 2 commits June 1, 2017 13:44

pkg/openthread: add OpenThread core

017280d

pkg/openthread: Add tests

fc4fd62

jia200x force-pushed the openthread_core branch from fa60436 to fc4fd62 Compare June 1, 2017 17:44

aabadie approved these changes Jun 1, 2017

View reviewed changes

aabadie merged commit 1a2097e into RIOT-OS:master Jun 1, 2017

beriberikix mentioned this pull request Jun 22, 2017

openthread seems like Contiki openthread/openthread#1928

Closed

aabadie modified the milestones: Release 2017.04, Release 2017.07 Jun 26, 2017

Hyungsin mentioned this pull request Aug 28, 2017

[OpenThread] report RSSI for openthread #7518

Merged

miri64 mentioned this pull request Nov 30, 2017

gnrc_netdev: remove remaining files and references #8180

Merged

+              }
+              #endif
+              #endif

Conversation

jia200x commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

jia200x commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jia200x May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

aabadie May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 19, 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

jia200x commented May 18, 2017 •

edited

Loading

jia200x May 19, 2017 •

edited

Loading

aabadie May 19, 2017 •

edited

Loading