emb6: provide sock_udp port by miri64 · Pull Request #6012 · RIOT-OS/RIOT

miri64 · 2016-10-27T15:52:06Z

Basically just an adapted carbon copy of emb6_conn_udp. Testable with tests/emb6. Not in the mood to write more indepth test like for the more stable stacks ;-).

miri64 · 2017-01-13T16:07:29Z

Ping?

miri64 · 2017-01-18T15:53:16Z

Rebased for today's Hack'n'ACK.

miri64 · 2017-02-28T16:19:24Z

@PeterKietzmann can we maybe get this merged today, then we can delete conn soon-ish ;-).

miri64 · 2017-02-28T16:25:53Z

Rebased to current master

miri64 · 2017-02-28T17:56:48Z

Okay, I can't fix the issues Jenkins reports right now. Will do later.

miri64 · 2017-03-28T15:22:01Z

Rebased and adapted to master.

miri64 · 2017-04-11T11:38:13Z

If we merge this before the release, we can remove conn for the release ;-)

miri64 · 2017-04-25T08:01:58Z

Can we finally merge this tonight?

smlng · 2017-04-25T08:37:19Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+    /* change to emb6 thread context */
+    if (evproc_putEvent(E_EVPROC_TAIL, EVENT_TYPE_CONN_SEND, &send_cmd) == E_SUCCESS) {
+        /* block thread until data was send */
+        mutex_lock(&send_cmd.block);


mutex was already locked 4 lines above, need to lock again here?

is this because in _output_callback its unlocked if successful?

but why unlock in _output_callback and lock here again, couldn't you just omit the unlock in _output_callback.

Yes, this is basically a hacked way of waiting for the data to be sent (see comment above this line) ;-)

smlng · 2017-04-25T08:47:18Z

pkg/emb6/include/sock_types.h

+
+/**
+ * @defgroup
+ * @ingroup


looks a bit empty around here 😉

smlng · 2017-04-25T08:50:15Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+            res = -ETIMEDOUT;
+            break;
+        case _MSG_TYPE_RCV:
+            mutex_lock(&sock->mutex);


it is necessary to lock here already, if possible (and safe) move lock below one line above memcpy?

sock->recv_info in the line below needs protection. It is set in _input_callback() which is set in the emb6 thread's context.

miri64 · 2017-04-25T09:42:44Z

Addressed comments

kaspar030 · 2017-04-25T10:13:39Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+    mutex_lock(&sock->mutex);
+    mbox_init(&sock->mbox, sock->mbox_queue, SOCK_MBOX_SIZE);
+    atomic_flag_clear(&sock->receivers);
+    if ((res = _reg(&sock->sock, sock, _input_callback, local, remote)) < 0) {


_reg either returns 0 on success or < 0 on error. Thus you can shorten the function:

if ((res = _reg(...))) { sock->sock.input_callback = NULL; } mutex_unlock(&sock->mutex); return res;

I think this makes the code harder to understand.

How about:

res = _reg(...); if (res) { /* there was an error */ sock->sock.input_callback = NULL; } mutex_unlock(&sock->mutex); return res;

Disregarding the comment the still read "if there is a result, set input_callback to zero". Doing a boolean operation here makes this a lot clearer, IMHO (without requiring a comment).

Please rephrase, I honestly don't understand what you mean.

I think we talked past each other (thought you were talking about the < 0)... I wasn't aware anymore about the code below. Will fix.

kaspar030 · 2017-04-25T10:16:05Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+    }
+    atomic_fetch_add(&sock->receivers, 1);
+    if (_mbox_get(&sock->mbox, &msg, blocking) == 0) {
+        return -EAGAIN;


is it possible to get here with the timer still active? in that case it must be removed.

You mean the timeout timer? That's only active when timeout > 0. _mbox_get() returns 0 in non-blocking mode (timeout == 0). Will add a comment.

kaspar030 · 2017-04-25T10:20:03Z

tests/emb6/udp.c

-#ifdef MODULE_CONN_UDP
-static char conn_inbuf[CONN_INBUF_SIZE];
+#ifdef MODULE_EMB6_SOCK_UDP
+static char sock_inbuf[CONN_INBUF_SIZE];


miri64 · 2017-04-25T11:26:10Z

Addressed comments.

miri64 · 2017-04-25T12:49:31Z

Fixed issues reported by Murdock (needed to rebase for that).

miri64 · 2017-04-25T16:43:37Z

Fixed/explained away errors reported by cppcheck

miri64 · 2017-04-25T19:19:55Z

Any chance to get this merged today as well?

miri64 · 2017-04-25T19:20:23Z

(just want to make removal of conn possible ASAP for the next release ;-))

smlng · 2017-04-26T06:35:24Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+        }
+    }
+    if (remote != NULL) {
+        udp_socket_connect(c, (uip_ipaddr_t *)&remote->addr, remote->port);


could fail, too? returns 1 on success and -1 on error see https://github.com/hso-esk/emb6/blob/c73091fd12d04ce91d9dfcac2a94196f3e7cb162/emb6/inc/sock/udp-socket.h#L140

Because it only returns -1 if c or c->udp_conn is NULL, which can't happen at this point.

mhm, I think that is exactly what happens below in L243 where _reg is called with &tmp and tmp->udp_conn is never set, hence NULL or something random.

ah sorry remote == NULL, may bad

smlng · 2017-04-26T06:39:25Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+    if ((sock->sock.input_callback != NULL) &&
+        (sock->sock.udp_conn->lport != 0)) {
+        mutex_lock(&sock->mutex);
+        memset(&ep->addr, 0, sizeof(ipv6_addr_t));


why is address just set to all 0s and not copied from sock as in gnrc_sock_udp, for example?

Because the local end point in emb6 doesn't have an address (since there are no interfaces).

smlng · 2017-04-26T06:42:23Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+        return -ENOTCONN;
+    }
+    /* cppcheck-suppress nullPointerRedundantCheck
+     * remote == NULL implies that sock != NULL (see assert at start of functon)


/functon/function/

smlng · 2017-04-26T06:44:24Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+        }
+        send_cmd.remote = remote;
+    }
+    if ((remote == NULL) && (sock->sock.udp_conn->rport == 0)) {


this could be

else if (sock->sock.udp_conn->rport == 0) { return -ENOTCONN; }

cause above you check remote != NULL hence if that is not the case, it must be remote == NULL

smlng · 2017-04-26T06:48:16Z

pkg/emb6/contrib/sock/udp/emb6_sock_udp.c

+     * without checking (sock != NULL) first => this check afterwards isn't
+     * redundant */
+    if (sock == NULL) {
+        int res;


why not use send_cmd.res here?

Because it's the result of the send command, not the result of _reg() (and I don't want to dig into emb6's code now to make sure I don't have to zero it again bevore puttilng send_cmd in its event queue ;-))

kaspar030

minor changes, please squash after fixing!

kaspar030 · 2017-04-26T07:05:09Z

tests/emb6/udp.c

-    port = (uint16_t)atoi((char *)args);
-    if ((res = conn_udp_create(&server_conn, &server_addr,
-                               sizeof(server_addr), AF_INET6, port)) < 0) {
+    uint16_t port = (uint16_t)atoi((char *)args);


casts unneccessary

kaspar030 · 2017-04-26T07:05:52Z

tests/emb6/udp.c

    }
    /* parse port */
-    port = (uint16_t)atoi(port_str);
+    dst.port = (uint16_t)atoi(port_str);


cast not needed

miri64 · 2017-04-26T07:18:18Z

Addressed comments and squashed

miri64 · 2017-04-26T11:07:27Z

Murdock's happy. :-)

miri64 · 2017-04-26T12:21:09Z

@smlng are you okay too?

emmanuelsearch · 2017-04-27T12:36:04Z

@miri64 shall we not also remove conn from the /contrib dir?

miri64 · 2017-04-27T12:51:49Z

@emmanuelsearch #6976

miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports labels Oct 27, 2016

miri64 added this to the Release 2017.01 milestone Oct 27, 2016

miri64 assigned kaspar030 Oct 27, 2016

miri64 mentioned this pull request Jan 13, 2017

posix_sockets: port to sock #6004

Merged

miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 18, 2017

miri64 force-pushed the emb6/feat/sock branch from 69eaf02 to aa938c4 Compare January 18, 2017 15:52

PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Jan 26, 2017

miri64 assigned PeterKietzmann Feb 28, 2017

miri64 force-pushed the emb6/feat/sock branch from aa938c4 to c398518 Compare February 28, 2017 16:25

miri64 force-pushed the emb6/feat/sock branch from c398518 to e9ce32c Compare March 28, 2017 15:21

miri64 assigned smlng Apr 11, 2017

miri64 mentioned this pull request Apr 18, 2017

OpenThread port #5552

Closed

kaspar030 modified the milestone: Release 2017.04 Apr 21, 2017

smlng reviewed Apr 25, 2017

View reviewed changes

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 25, 2017

miri64 mentioned this pull request Apr 25, 2017

tests: port tests/lwip to sock #6372

Merged

kaspar030 requested changes Apr 25, 2017

View reviewed changes

miri64 force-pushed the emb6/feat/sock branch from 21a15e8 to cb99395 Compare April 25, 2017 12:49

smlng suggested changes Apr 26, 2017

View reviewed changes

kaspar030 requested changes Apr 26, 2017

View reviewed changes

emb6: provide sock_udp port

20f6ab6

miri64 force-pushed the emb6/feat/sock branch from 9a86ba8 to 20f6ab6 Compare April 26, 2017 07:18

kaspar030 approved these changes Apr 26, 2017

View reviewed changes

smlng approved these changes Apr 26, 2017

View reviewed changes

miri64 merged commit c23d624 into RIOT-OS:master Apr 26, 2017

miri64 deleted the emb6/feat/sock branch April 26, 2017 12:22

Conversation

miri64 commented Oct 27, 2016

Uh oh!

miri64 commented Jan 13, 2017

Uh oh!

miri64 commented Jan 18, 2017

Uh oh!

miri64 commented Feb 28, 2017

Uh oh!

miri64 commented Feb 28, 2017

Uh oh!

miri64 commented Feb 28, 2017

Uh oh!

miri64 commented Mar 28, 2017

Uh oh!

miri64 commented Apr 11, 2017

Uh oh!

miri64 commented Apr 25, 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!

miri64 commented Apr 25, 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!

miri64 commented Apr 25, 2017

Uh oh!

miri64 commented Apr 25, 2017

Uh oh!

miri64 commented Apr 25, 2017

Uh oh!

miri64 commented Apr 25, 2017

Uh oh!

miri64 commented Apr 25, 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!

smlng Apr 26, 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

smlng Apr 26, 2017 •

edited

Loading