sock_async: initial import of event-based implementation by miri64 · Pull Request #12602 · RIOT-OS/RIOT

miri64 · 2019-10-29T16:04:07Z

Contribution description

This provides a event-based front-end implementation for sock_async (see #11723).

Testing procedure

You can test (did not try this yet) the example provided in the header with the GNRC backend in #12625.

Issues/PRs references

Requires ~~#11723~~ (merged) and ~~#12921~~ (merged).

miri64 · 2019-12-06T14:49:35Z

I rewrote this so that the callback types from sock/async.h are now re-used also in the front-end. This has the advantage, that a user does not need to bother about the make-up of the event struct for the sock. They just get the sock and the flag field for the type of event that happened in the callback now, making it possibly reusable with other sock_async frontend implementations. The latter fact was an offline proposal by @kaspar030 a while back which is why I added him as a co-author to the header.

miri64 · 2019-12-06T14:59:27Z

@miri64 force-pushed the miri64:sock_async_event/feat/initial branch from 6650680 to 477bcc0

Forgot to amend the co-authorship 😅.

No longer waiting for another PR (#11723 was merged a while back and I rebased it to current master. Also no longer WIP as the work that was in progress is now done with the latest update

miri64 · 2019-12-09T12:00:55Z

With #12625 now merged, I rebased to current master and adapted the test tests/gnrc_sock_async (now tests/gnrc_sock_async_event) to use sock_async_event instead to not give anyone looking for examples in the tests the wrong ideas ;-).

miri64 · 2019-12-11T06:29:18Z

sys/include/net/sock/async/event.h

+#ifdef MODULE_SOCK_DTLS
+#include "net/sock/dtls.h"
+#endif  /* MODULE_SOCK_DTLS */
+#include "net/sock/ip.h"
+#include "net/sock/tcp.h"
+#include "net/sock/udp.h"
+#include "net/sock/async.h"


When re-ordering this to a more sensible order I fall into the same trap as I am currently stuck in with sock_dtls in #12907 :-/

diff --git a/sys/include/net/sock/async/event.h b/sys/include/net/sock/async/event.h index 9533334ff..02ad40cda 100644 --- a/sys/include/net/sock/async/event.h +++ b/sys/include/net/sock/async/event.h @@ -157,6 +157,7 @@ #define NET_SOCK_ASYNC_EVENT_H #include "event.h" +#include "net/sock/async.h" /* guard required since `sock_dtls_types.h` might not be provided */ #ifdef MODULE_SOCK_DTLS #include "net/sock/dtls.h" @@ -164,7 +165,6 @@ #include "net/sock/ip.h" #include "net/sock/tcp.h" #include "net/sock/udp.h" -#include "net/sock/async.h" #ifdef __cplusplus extern "C" {

In file included from /home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/ip.h:453, from /home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/async.h:31, from /home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/async/event.h:160, from /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/sock/async/event/sock_async_event.c:16: /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/sock/include/sock_types.h:58:36: error: unknown type name ‘sock_async_flags_t’ 58 | sock_async_flags_t flags); | ^~~~~~~~~~~~~~~~~~ /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/sock/include/sock_types.h:81:9: error: unknown type name ‘gnrc_sock_reg_cb_t’ 81 | gnrc_sock_reg_cb_t generic; /**< generic version */ | ^~~~~~~~~~~~~~~~~~ /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/sock/include/sock_types.h:83:9: error: unknown type name ‘sock_ip_cb_t’ 83 | sock_ip_cb_t ip; /**< IP version */ | ^~~~~~~~~~~~ /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/sock/include/sock_types.h:86:9: error: unknown type name ‘sock_udp_cb_t’ 86 | sock_udp_cb_t udp; /**< UDP version */ | ^~~~~~~~~~~~~ "make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/pktdump /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/sock/include/sock_types.h:90:5: error: unknown type name ‘sock_async_ctx_t’ 90 | sock_async_ctx_t async_ctx; /**< asynchronous event context */ | ^~~~~~~~~~~~~~~~ "make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/sock

The problem is that there is a circular dependency, where-in both sock_types.h/sock_dtls_types.h of the network stack requires to include net/sock/async.h to add members related to asynchronous behavior to the type definition of the socks, while net/sock/async.h needs to include net/sock/<prot>.h to access the sock types which in turn pulls in sock_types.h/sock_dtls_types.h for the actual type definition of the socks. I'm currently unsure how to solve this :-/.

miri64 · 2019-12-11T09:34:54Z

Added #12921 as a dependency for the cyclic header dependency problem.

fjmolinas · 2020-01-08T22:13:17Z

@miri64 you can rebase this one :)

miri64 · 2020-01-09T11:24:25Z

Rebased and squashed. No longer waiting for another PR.

miri64 · 2020-01-10T13:25:10Z

Just to be clear, this should be mergeable during soft feature freeze, right @fjmolinas?

fjmolinas · 2020-01-10T13:27:51Z

Just to be clear, this should be mergeable during soft feature freeze, right @fjmolinas?

Yes for me this is a contained feature so it can get in, but maybe tagged as experimental though. And by that I mean that bug fixes wouldn't get backported.

miri64 · 2020-01-10T15:56:49Z

[…] but maybe tagged as experimental though.

Done

sys/include/net/sock/async/event.h

fjmolinas

Tested the documentation and the code.

Minimal example:

2020-01-14 15:16:15,200 # �main(): This is RIOT! (Version: 2020.01-devel-1628-gfead8-pr-12602)
2020-01-14 15:16:18,227 #           
2020-01-14 15:16:20,895 # Received a message
2020-01-14 15:16:22,111 # Received a message

I would personally ACK this, code looks good and I have tested my issue is that it was tagged as RFC (only I have commented so ar) and I'm not that familiar with the event api. Can someone else give it a second look as well maybe @kaspar030 or @jia200x? (it shouldn't be too much work)

sys/include/net/sock/async/event.h

fjmolinas · 2020-01-14T14:25:16Z

sys/include/net/sock/async/event.h

+ *
+ * You need to [include](@ref including-modules) at least one module that
+ * implements a [`sock` API](@ref net_sock) and the module `sock_async_event` in
+ * your application's Makefile.


I used the following modules:

USEMODULE += auto_init_gnrc_netif USEMODULE += gnrc_netdev_default USEMODULE += gnrc_ipv6_default USEMODULE += sock_async_event USEMODULE += gnrc_sock_async USEMODULE += gnrc_sock_udp

I think it would make easier for a user to give it a test if those modules are given es examples. What I mean by that would be for example and at least one network device (e.g: gnrc_netdev_default) and implements a [sock API](@ref net_sock) (e.g: gnrc_sock_async, gnrc_sock_udp). I won't insist on this though, since it wasn't hard to figure out the needed MODULES

fjmolinas · 2020-01-15T07:47:43Z

IMO this one is good to go, please squash!

miri64 · 2020-01-15T08:56:33Z

Squashed

fjmolinas · 2020-01-15T09:32:50Z

I would personally ACK this, code looks good and I have tested my issue is that it was tagged as RFC (only I have commented so ar) and I'm not that familiar with the event api. Can someone else give it a second look as well maybe @kaspar030 or @jia200x? (it shouldn't be too much work)

This has been open for a while so I'm going with out implicit rule, no comment being a go ahead (not sure oh the exact formulation hahaha). Adding it to the long murdock queue.

miri64 · 2020-01-15T09:44:39Z

Adding it to the long murdock queue.

I've seen longer murdock queues :P

miri64 · 2020-01-15T11:57:39Z

The test fails for pkg_tensorflow-lite are unrelated, I believe.

fjmolinas · 2020-01-15T12:00:49Z

Murdock only failed on unrelated tests, re-triggering without tests.

    total cpu runtime: 2d:22h:26m:26.8s
---
--- run_test job results (2 failed, 14 passed, 16 total):
    failed:
    tests/pkg_tensorflow-lite/nrf52dk:llvm
    tests/pkg_tensorflow-lite/esp32-wroom-32:gnu

    passed:
    examples/suit_update/samr21-xpro:gnu
    examples/suit_update/nrf52dk:llvm
    examples/suit_update/samr21-xpro:llvm
    examples/suit_update/nrf52dk:gnu
    tests/pkg_qdsa/samr21-xpro:gnu
    tests/gnrc_sock_async_event/samr21-xpro:gnu
    tests/gnrc_sock_async_event/samr21-xpro:llvm
    tests/gnrc_sock_async_event/nrf52dk:llvm
    tests/gnrc_sock_async_event/esp32-wroom-32:gnu
    tests/gnrc_sock_async_event/nrf52dk:gnu
    tests/pkg_relic/nrf52dk:gnu
    tests/pkg_relic/samr21-xpro:gnu
    tests/pkg_relic/esp32-wroom-32:gnu
    tests/pkg_tensorflow-lite/nrf52dk:gnu

I'll take a look at the failing tests since I have both boards.

aabadie · 2020-01-15T12:56:52Z

failed:
    tests/pkg_tensorflow-lite/nrf52dk:llvm
    tests/pkg_tensorflow-lite/esp32-wroom-32:gnu

Would be great to have the output as well. Is it a setup/flash problem or a real failure of the test itself (a crash or something) ?

fjmolinas · 2020-01-15T13:02:35Z

Would be great to have the output as well. Is it a setup/flash problem or a real failure of the test itself (a crash or something) ?

I'll test locally.

miri64 · 2020-01-15T13:02:52Z

Since the build now went on (and I don't know if the logs are recoverable... @kaspar030?) I just can summarize from memory, that it was the test script itself that failed (on line 10)

fjmolinas

ACK!

fjmolinas · 2020-01-15T13:39:02Z

Since the build now went on (and I don't know if the logs are recoverable... @kaspar030?) I just can summarize from memory, that it was the test script itself that failed (on line 10)

Both timed-out on expecting the digit match.

fjmolinas · 2020-01-15T13:42:32Z

Cant reproduce with nrf52dk`.

RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 TRIBE_CI=1 BOARD=nrf52dk make -C tests/pkg_tensorflow-lite/ test

2020-01-15 14:42:01,164 # READY
s
2020-01-15 14:42:01,220 # START
2020-01-15 14:42:01,224 # main(): This is RIOT! (Version: buildtest)
2020-01-15 14:42:01,246 # Digit prediction: 7

miri64 · 2020-01-15T13:44:13Z

Thanks for testing and the review @fjmolinas!

miri64 · 2020-01-15T14:10:37Z

Cant reproduce with nrf52dk`.

RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 TRIBE_CI=1 BOARD=nrf52dk make -C tests/pkg_tensorflow-lite/ test

2020-01-15 14:42:01,164 # READY
s
2020-01-15 14:42:01,220 # START
2020-01-15 14:42:01,224 # main(): This is RIOT! (Version: buildtest)
2020-01-15 14:42:01,246 # Digit prediction: 7

In my experience both nrf52dk and esp32-wroom-32 are notoriously failing sometimes in the CI.

aabadie · 2020-01-15T14:12:51Z

Thanks for reporting @miri64. I tested tensorflow-lite on nrf52 and it was working. But the fact that 2 platforms are failing on the same test in the same run put some doubt :)

miri64 · 2020-01-15T14:17:26Z

Thanks for reporting @miri64. I tested tensorflow-lite on nrf52 and it was working. But the fact that 2 platforms are failing on the same test in the same run put some doubt :)

Only one build for nrf52dk was failing (with LLVM, the GNU port passed) and esp32-wroom-32-support in the CI is quite new, so it is by chance that they both failed on your test, yes, but not doubtful ;-).

kaspar030 · 2020-01-15T15:30:23Z

Would be great to have the output as well. Is it a setup/flash problem or a real failure of the test itself (a crash or something) ?

I captured the output from the build of #12848, will open an issue.

miri64 changed the title ~~Sock async event/feat/initial~~ sock_async: initial import of event-based implementation Oct 29, 2019

This was referenced Oct 29, 2019

sock: initial definitions for asynchronous event handling #11723

Merged

gnrc_sock: provide asynchronous event implementation #12625

Merged

miri64 force-pushed the sock_async_event/feat/initial branch from 022d687 to 6650680 Compare December 6, 2019 14:45

miri64 force-pushed the sock_async_event/feat/initial branch from 6650680 to 477bcc0 Compare December 6, 2019 14:53

miri64 removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 6, 2019

miri64 mentioned this pull request Dec 7, 2019

gcoap: can't build with network stacks other than GNRC #8130

Closed

miri64 force-pushed the sock_async_event/feat/initial branch from 477bcc0 to 3aae764 Compare December 9, 2019 11:59

kb2ma mentioned this pull request Dec 9, 2019

gcoap: add DTLS integration #12104

Closed

miri64 commented Dec 11, 2019

View reviewed changes

miri64 mentioned this pull request Dec 11, 2019

sock_async: fix cyclic include problem #12921

Merged

miri64 force-pushed the sock_async_event/feat/initial branch from 3aae764 to d996506 Compare December 11, 2019 09:31

miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 11, 2019

miri64 mentioned this pull request Dec 11, 2019

sock_async: initial import of msg-based implementation #12601

Closed

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 9, 2020

miri64 force-pushed the sock_async_event/feat/initial branch from d996506 to 0131708 Compare January 9, 2020 11:24

fjmolinas reviewed Jan 14, 2020

View reviewed changes

sys/include/net/sock/async/event.h Show resolved Hide resolved

fjmolinas reviewed Jan 14, 2020

View reviewed changes

miri64 added 2 commits January 15, 2020 09:56

sock_async_event: initial import of event-based implementation

e46ad92

tests/gnrc_sock_async: adapt to use sock_async_event

2687b24

miri64 force-pushed the sock_async_event/feat/initial branch from 05ba39d to 2687b24 Compare January 15, 2020 08:56

fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 15, 2020

fjmolinas approved these changes Jan 15, 2020

View reviewed changes

fjmolinas merged commit 43d2ca4 into RIOT-OS:master Jan 15, 2020

miri64 deleted the sock_async_event/feat/initial branch January 15, 2020 13:35

fjmolinas added this to the Release 2020.01 milestone Jan 15, 2020

Conversation

miri64 commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Dec 6, 2019

Uh oh!

miri64 commented Dec 6, 2019

Uh oh!

miri64 commented Dec 9, 2019

Uh oh!

miri64 Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 commented Dec 11, 2019

Uh oh!

fjmolinas commented Jan 8, 2020

Uh oh!

miri64 commented Jan 9, 2020

Uh oh!

miri64 commented Jan 10, 2020

Uh oh!

fjmolinas commented Jan 10, 2020

Uh oh!

miri64 commented Jan 10, 2020

Uh oh!

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

fjmolinas commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

fjmolinas commented Jan 15, 2020

Uh oh!

aabadie commented Jan 15, 2020

Uh oh!

fjmolinas commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jan 15, 2020

Uh oh!

fjmolinas commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

aabadie commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

kaspar030 commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

miri64 commented Oct 29, 2019 •

edited

Loading

miri64 commented Jan 15, 2020 •

edited

Loading