gnrc_netif: Add support for internal event loop by bergzand · Pull Request #13669 · RIOT-OS/RIOT

bergzand · 2020-03-20T13:31:25Z

Adapted from #9326

Contribution description

From #9326:
Enabled by the gnrc_netif_events pseudo module. Using an internal event loop eliminates the risk of lost interrupts and lets ISR events always be handled before any send/receive requests from other threads are processed.
The events in the event loop is also a potential hook for MAC layers and other link layer modules which may need a way to inject and process events before any external IPC messages are handled.

Testing procedure

There is an extra commit in the PR to add the pseudomodule to examples/gnrc_networking. The example should still function as before.

Issues/PRs references

Adapted from #9326

jia200x · 2020-03-20T14:06:19Z

sys/net/gnrc/netif/gnrc_netif.c

+ */
+static void _process_events_await_msg(gnrc_netif_t *netif, msg_t *msg)
+{
+#ifdef MODULE_GNRC_NETIF_EVENTS


I suggest to change this to a regular if-else chain (with IS_ACTIVE). Then we get compile checks for free

That's not possible, some members of gnrc_netif_t are only available when this module is enabled. Modifying this to IS_ACTIVE will result in compilation failures when GNRC_NETIF_EVENTS is not enabled.

you can simply define a static inline function:

static inline _get_evq(netif_t *netif) { #if IS_ACTIVE(MODULE_GNRC_NETIF_EVENTS) return netif->evq; #else return NULL; #endif }

Then you call that function in the while loop.

This function does not compile when evq is not a member of gnrc_netif_t, which is the case when MODULE_GNRC_NETIF_EVENTS because of the ifdef guards here

This function does not compile when evq is not a member of gnrc_netif_t, which is the case when MODULE_GNRC_NETIF_EVENTS because of the ifdef guards here

Then it should still work with @jia200x's last proposal.

Then it should still work with @jia200x's last proposal.

Oh, of course, I misread his last suggestion.

Okay, while this is possible, it is starting to become more and more messy. For the solution posted by @jia200x I have to include both event.h and thread_flags.h. For thread_flags.h to be included it is required to enable MODULE_CORE_THREAD_FLAGS increasing the build size whether we're using MODULE_GNRC_NETIF_EVENTS or not.

Conditionally including event.h and thread_flags.h doesn't work because the code between the regular if-else chain (with IS_ACTIVE) requires those for one of the branches to compile.

Am I missing something silly here that allows me to get this to work or is the current #ifdef really the only way out here? I honestly don't want to start guarding every individual troublesome call in this snippet if we can avoid it.

It should be IS_USED() btw not, IS_ACTIVE(). If you use #if (not if () it should work even if you #ifdef the calls above. Let me give you some suggestions.

The root of the problem here is the #error in thread_flags header. It's misplaced IMO. It should be in the C file that defines the thread flags function.

I can open a PR to move it (unless you are faster :), it should be a low hanging fruit.

It's there: #13671

And @miri64 is right. It should be IS_USED for modules

sys/net/gnrc/netif/gnrc_netif.c

jia200x · 2020-03-20T14:09:45Z

codewise looks fine, just some minor comments above

miri64 · 2020-03-20T15:40:46Z

sys/net/gnrc/netif/gnrc_netif.c

+ */
+static void _process_events_await_msg(gnrc_netif_t *netif, msg_t *msg)
+{
+#ifdef MODULE_GNRC_NETIF_EVENTS


It should be IS_USED() btw not, IS_ACTIVE(). If you use #if (not if () it should work even if you #ifdef the calls above. Let me give you some suggestions.

sys/include/net/gnrc/netif.h

sys/net/gnrc/netif/gnrc_netif.c

bergzand · 2020-04-01T12:35:14Z

Reworked, comments should be addressed with this.

jia200x · 2020-04-01T13:21:45Z

Great! I will give it a deeper look in a few hours

miri64

One minor thing ;-)

sys/net/gnrc/netif/gnrc_netif.c

miri64 · 2020-04-01T13:27:50Z

sys/net/gnrc/netif/gnrc_netif.c

+ */
+static void _event_handler_isr(event_t *evp)
+{
+    (void)evp;


Why this? evp is used just in the line below.

Ah, leftover from shuffling around the #ifdefs, will remove

miri64 · 2020-04-01T13:31:10Z

This compiles for me and would result in less #ifdef hell:

diff --git a/sys/net/gnrc/netif/gnrc_netif.c b/sys/net/gnrc/netif/gnrc_netif.c
index 34f7c5f5c..ad62a6ac1 100644
--- a/sys/net/gnrc/netif/gnrc_netif.c
+++ b/sys/net/gnrc/netif/gnrc_netif.c
@@ -1359,7 +1359,6 @@ void gnrc_netif_default_init(gnrc_netif_t *netif)
 #endif
 }
 
-#if IS_USED(MODULE_GNRC_NETIF_EVENTS)
 /**
  * @brief   Call the ISR handler from an event
  *
@@ -1367,11 +1366,13 @@ void gnrc_netif_default_init(gnrc_netif_t *netif)
  */
 static void _event_handler_isr(event_t *evp)
 {
-    (void)evp;
+#if IS_USED(MODULE_GNRC_NETIF_EVENTS)
     gnrc_netif_t *netif = container_of(evp, gnrc_netif_t, event_isr);
     netif->dev->driver->isr(netif->dev);
-}
+#else
+    (void)evp;
 #endif
+}
 
 /**
  * @brief   Retrieve the netif event queue if enabled
@@ -1452,11 +1453,11 @@ static void *_gnrc_netif_thread(void *args)
     gnrc_netif_acquire(netif);
     dev = netif->dev;
     netif->pid = sched_active_pid;
-#if IS_USED(MODULE_GNRC_NETIF_EVENTS)
-    netif->event_isr.handler = _event_handler_isr,
-    /* set up the event queue */
-    event_queue_init(&netif->evq);
-#endif /* MODULE_GNRC_NETIF_EVENTS */
+    if (IS_USED(MODULE_GNRC_NETIF_EVENTS)) {
+        netif->event_isr.handler = _event_handler_isr,
+        /* set up the event queue */
+        event_queue_init(&netif->evq);
+    }
     /* setup the link-layer's message queue */
     msg_init_queue(msg_queue, CONFIG_GNRC_NETIF_MSG_QUEUE_SIZE);
     /* register the event callback with the device driver */

bergzand · 2020-04-01T13:36:04Z

This compiles for me and would result in less #ifdef hell:

This doesn't compile for me if I don't compile with gnrc_netif_events included 😢

miri64 · 2020-04-01T13:44:32Z

This compiles for me and would result in less #ifdef hell:

This doesn't compile for me if I don't compile with gnrc_netif_events included 😢

I might have forgotten, that gnrc_networking includes that module now 😅

Change request was addressed

jia200x · 2020-04-01T16:37:52Z

Tested on samr21-xpro. The gnrc_networking example works OK with and without the gnrc_netif_events module.

jia200x · 2020-04-01T16:41:29Z

sys/net/gnrc/netif/gnrc_netif.c

    gnrc_netif_acquire(netif);
    dev = netif->dev;
    netif->pid = sched_active_pid;
+#if IS_USED(MODULE_GNRC_NETIF_EVENTS)


Just nitpick comment, this could be moved to it's own function (e.g gnrc_netif_events_init()). Then you remove the #ifdef

That's just shuffling around #ifdef's right? Instead of the #ifdef in the init function they are moved to the new function. Do you think this makes the code more readable?

I mean:

if (IS_USED(MODULE_GNRC_NETIF_EVENTS)) { gnrc_netif_events_init(); }

Might be a matter of personal taste here, so I'm OK with what you decide. But in the other comment the msg_t logic is guarded

I prefer to leave this as is for now, it keeps the initialization code in one place as much as possible and I don't think the ifdef decreases readability much in this case.

jia200x · 2020-04-01T16:44:19Z

sys/net/gnrc/netif/gnrc_netif.c

    gnrc_netif_t *netif = (gnrc_netif_t *) dev->context;

    if (event == NETDEV_EVENT_ISR) {
+#if IS_USED(MODULE_GNRC_NETIF_EVENTS)


Same here. What speaks againts:

static inline gnrc_netif_events_post(gnrc_netif_t *netif) { #if IS_USED(GNRC_NETIF_EVENTS) event_post(&netif->evq, &netif->event_isr); #else (void) netif; #endif }

?

Nothing, implemented this in the latest fixup

jia200x · 2020-04-15T15:09:24Z

please squash

Enabled by the gnrc_netif_events pseudo module. Using an internal event loop within the gnrc_netif thread eliminates the risk of lost interrupts and lets ISR events always be handled before any send/receive requests from other threads are processed. The events in the event loop is also a potential hook for MAC layers and other link layer modules which may need to inject and process events before any external IPC messages are handled. Co-Authored-By: Koen Zandberg <koen@bergzand.net>

bergzand · 2020-04-17T10:03:41Z

Squashed!

I've also removed the "REMOVEME" commit that added the event module to gnrc_networking.

bergzand · 2020-04-24T08:35:05Z

@jia200x Ping!

jia200x

Ok, it looks good to me! ACK

bergzand added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 20, 2020

bergzand requested review from jia200x and miri64 March 20, 2020 13:31

bergzand requested review from PeterKietzmann, cgundogan and haukepetersen as code owners March 20, 2020 13:31

jia200x reviewed Mar 20, 2020

View reviewed changes

sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved

miri64 reviewed Mar 20, 2020

View reviewed changes

jia200x mentioned this pull request Mar 20, 2020

core/thread_flags: remove #error from header file #13671

Merged

jia200x mentioned this pull request Mar 31, 2020

[Tracking]: improve GNRC netif module #13763

Open

4 tasks

bergzand force-pushed the pr/netif/async_events branch from ddcd163 to 1c0ab40 Compare April 1, 2020 12:23

miri64 previously requested changes Apr 1, 2020

View reviewed changes

sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved

miri64 reviewed Apr 1, 2020

View reviewed changes

jia200x reviewed Apr 1, 2020

View reviewed changes

miri64 mentioned this pull request Apr 14, 2020

gnrc_netif: Add support for internal event loop #9326

Closed

jia200x mentioned this pull request Apr 15, 2020

gnrc_netif: use irq_handler instead of IPC #12462

Closed

bergzand force-pushed the pr/netif/async_events branch from bd4b45d to 2013ac7 Compare April 17, 2020 10:02

jia200x approved these changes Apr 24, 2020

View reviewed changes

miri64 merged commit 633d0f4 into RIOT-OS:master Apr 28, 2020

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

miri64 mentioned this pull request Jul 31, 2020

core/msg: Add submodule core_msg_prio #14677

Closed

Conversation

bergzand commented Mar 20, 2020

Contribution description

Testing procedure

Issues/PRs references

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!

Uh oh!

jia200x commented Mar 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bergzand commented Apr 1, 2020

Uh oh!

jia200x commented Apr 1, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Apr 1, 2020

Uh oh!

bergzand commented Apr 1, 2020

Uh oh!

miri64 commented Apr 1, 2020

Uh oh!

jia200x commented Apr 1, 2020

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 Apr 15, 2020

Uh oh!

bergzand commented Apr 17, 2020

Uh oh!

bergzand commented Apr 24, 2020

Uh oh!

jia200x left a comment

Choose a reason for hiding this comment

Uh oh!