gnrc_sock: provide asynchronous event implementation

[miri64]

Contribution description

This provides an implementation for GNRC for the asynchronous event management for sock_ip and sock_udp as provided in #11723.

Testing procedure

Tests are provided with this PR

BOARD="<pick one>" make -C tests/gnrc_sock_async flash test

Issues/PRs references

Follow-up on #11723
Requires #12624 (merged) to work.

[miri64]

Rebased to current master.

[miri64]

And updated BOARD_INSUFFICIENT_MEMORY for the new application.

[miri64]

And fixed some flubs pointed out by Murdock and I noticed myself.

[larseggert]

So I am trying to use this, but even with USEMODULE += sock_async added, I get errors about sock_async_flags_t and sock_udp_set_cb being undefined. It looks like async.h isn't even included as part of the build?

[miri64]

So I am trying to use this, but even with USEMODULE += sock_async added, I get errors about sock_async_flags_t and sock_udp_set_cb being undefined. It looks like async.h isn't even included as part of the build?

Yes, you have to select gnrc_sock_async when using with GNRC.

[miri64]

And don't include async.h otherwise conflicts will arise. Also, note this comment

RIOT/tests/gnrc_sock_async/main.c

Lines 125 to 127 in 734da1c

	/* XXX don't do it like this in production and use a proper `sock_async`
	* frontend! This is just for testing. */
	sock_udp_set_cb(&_udp_sock, _recv_udp);

A normal user is supposed to use an API like the ones proposed (but still under construction) in #12601 or #12602.

[larseggert]

Ah, OK. So this is not quite ready.

It also begins to feel like it's quite a bit of overhead, since I just need something that gives me poll- or select-type functionality, but I can deal with this if I have to.

[miri64]

It also begins to feel like it's quite a bit of overhead, since I just need something that gives me poll- or select-type functionality, but I can deal with this if I have to.

If its done, it will provide select() for the posix_socket module :-). The (code-size) overhead will however be way bigger than what just sock will provide, but I guess with QUIC it will be negligible 😜?

[benpicco]

It looks like SOCK_ASYNC_MSG_SENT is generated when the message when the packet has been handed to the lower layer.

That doesn't seem immediately useful - I would much rather have an event when a NETDEV_EVENT_TX_COMPLETE is generated or when the packet queue is ready to accept new entries (#11263) so I know when it's actually possible to send the next packet.

[miri64]

It looks like SOCK_ASYNC_MSG_SENT is generated when the message when the packet has been handed to the lower layer.

That doesn't seem immediately useful - I would much rather have an event when a NETDEV_EVENT_TX_COMPLETE is generated or when the packet queue is ready to accept new entries (#11263) so I know when it's actually possible to send the next packet.

a) you can't guarantee that NETDEV_EVENT_TX_COMPLETE is when the datagram is actually leaving device (e.g. if it is fragmented).
b) It shouldn't be the concern of the application layer to know, if the packet was sent in the PHY layer, to determine if it is possible to send the next packet. Otherwise, the abstraction of the network stack failed massively. This is typically the task of the MAC protocol and additionally the transport layer.
c) if you activate gnrc_neterr it should do just that or return an error if not (currently untested and probably needs work).

[benpicco]

a) you can't guarantee that NETDEV_EVENT_TX_COMPLETE is when the datagram is actually leaving device (e.g. if it is fragmented).

That was just an example. The event should be generated when it's actually possible to send a new datagram, what this is dependent on is specific to the implementation.

But what is SOCK_ASYNC_MSG_SENT supposed to be used for then?
My naiive interpretation would be that I can sent the next packet when I get this event. However this is not the case as the stack is still busy transmitting the first packet, it would be dropped immediately.

When adding asynchronous events, a "stack is ready to accept new datagram" event would be the least to ask for IMHO.

[miri64]

But what is SOCK_ASYNC_MSG_SENT supposed to be used for then?
My naiive interpretation would be that I can sent the next packet when I get this event. However this is not the case as the stack is still busy transmitting the first packet, it would be dropped immediately.

The use case is to only call functions when they are supposed to (similar to select() with POSIX sockets). If the packet is dropped at a module border instead of put into a queue than you have a resource problem anyway, which wouldn't be fixed by moving a SOCK specific event to a lower layer.

When adding asynchronous events, a "stack is ready to accept new datagram" event would be the least to ask for IMHO.

This is the case once the gnrc_sock module hands it over to the lower layer:

a) the packet is put into the lower layer thread's IPC message queue (which accept when it is empty ensures that it can handle more packets),
b) the lower layer threads are supposed to have a higher priority than the application thread. So the UDP or IP thread respectively handles its IPC message queue as soon as it gets preempted (which is the case when it receives an IPC message).

[miri64]

c) if you activate gnrc_neterr it should do just that or return an error if not (currently untested and probably needs work).

I can confirm now, that this still works (see #12678).

[miri64]

Ping?

[benpicco]

Code looks good, just two small things.
The name of sock_ip_get_async_ctx() is confusing. It's not a getter for async_ctx but a general access wrapper.
Not sure if it is even necessary, just makes the code slightly nicer to read. But then it might as well be static inline so the nicer syntax doesn't come at a cost.

I also don't know what a "proper sock_async frontend" would look like, so if I were to use the API I'd just copy the code from the example test.

[benpicco]

Where would async_ctx be set?
The function name confused me. I thought this was a getter, but it just returns a pointer to the field in the struct and the user can then do with it whatever they want.

Wouldn't sock_ip_async_ctx() be a less confusing name then?

Since this is just a 'nicer' way to access a field of the sock_ip_t, it should better be a static inline function so the compiler can optimize it away.

[miri64]

The function is part of the sock_async API so its name is already merged. However

Code looks good, just two small things.
The name of sock_ip_get_async_ctx() is confusing. It's not a getter for async_ctx but a general access wrapper.
Not sure if it is even necessary, just makes the code slightly nicer to read. But then it might as well be static inline so the nicer syntax doesn't come at a cost.

As far as I know this is a getter. So I don't know why the name is confusing.

Since this is just a 'nicer' way to access a field of the sock_ip_t, it should better be a static inline function so the compiler can optimize it away.

sock_ip_t is defined by the network stack, so different for any network stack. An inline is thus not easily possible. Edit: Also it is not just a "nicer" way, but the only legal way to access the async_ctx for the socket. With anything else you break yourself the abstraction and you've got yourself the same problem we have with the current gcoap all over again (see #8130)

[benpicco]

So is the idea to set async_ctx by calling *sock_ip_get_async_ctx(sock) = ctx; ?
And where is async_ctx defined to begin with?

[miri64]

(Sidenote: maybe its good its confusing, so the user is not tempted to use something he is not intended to, but is supposed to be used by the frontend like #12601 or #12602 ;-P)

[miri64]

So is the idea to set async_ctx by calling *sock_ip_get_async_ctx(sock) = ctx; ?
And where is async_ctx defined to begin with?

You don't set the context. It is just a helper for the frontend to have a storage type within the sock and get it from the sock stack-indepent. Have a look at #12601 and #12602. For #12601 async_ctx is just a kernel_pid_t and sock_ip_get_async_ctx() returns a pointer to that position within the sock object (so setting happens by dereferencing the pointer). For #12602 it is the event + pointer to the queue, so setting just happens by providing values for the members.

[benpicco]

I see, so this is not to be used/useful on it's own, but part of a larger feature.

I'd drop the #ifdef SOCK_HAS_ASYNC_CTX bits as those are never used and wouldn't even compile right now, but I see how it's convenient to not have that commit in two PRs.

[miri64]

I'd drop the #ifdef SOCK_HAS_ASYNC_CTX bits as those are never used and wouldn't even compile right now, but I see how it's convenient to not have that commit in two PRs.

I don't get what you mean by that. What do you propose to do if the front-end does not need a context?

[benpicco]

It's not about the #ifdef, it's that sock_async_ctx_t is not defined anywhere in current master or this PR.

[miri64]

Ah, so you miss the implementation ;-P

[benpicco]

Code looks good and tescase runs.

I find 'future use' code always a bit hard to review, but you're maintaining that subsystem, you'll know best what to do with it 😉.

[miri64]

I find 'future use' code always a bit hard to review, but you're maintaining that subsystem, you'll know best what to do with it 😉.

Divide and conquer ;-)

[benpicco]

Suggested change

	nucleo-f031k6 \
	nucleo-f031k6 \
	nucleo-f042k6 \

[benpicco]

You can squash directly.

[miri64]

Done